[PATCH v3] rockchip/drm: vop2: add support for gamma LUT

[Andy Yan]

Hi Piotr,

On 7/30/24 05:20, Piotr Zalewski wrote:
> Hi Andy
> 
> On Monday, July 29th, 2024 at 4:35 AM, Andy Yan <andy.yan at rock-chips.com> wrote:
> 
>>>>> +
>>>>> +static void vop2_crtc_gamma_set(struct vop2 *vop2, struct drm_crtc *crtc,
>>>>> + struct drm_crtc_state *old_state)
>>>>> +{
>>>>> + struct drm_crtc_state *state = crtc->state;
>>>>> + struct vop2_video_port *vp = to_vop2_video_port(crtc);
>>>>> + u32 dsp_ctrl;
>>>>> + int ret;
>>>>> +
>>>>> + if (!vop2->lut_regs)
>>>>> + return;
>>>>> +
>>>>> + if (!state->gamma_lut) {
>>>>
>>>> What's the purpose of checking !state->gamma_lut here,
>>>>
>>>> and you check it again at the end for return.
>>>> This makes me very confused.
>>>
>>> I understood it this way - since the vop2 lock is unlocked after disabling
>>> gamma LUT, the CRTC state can change while waiting for DSP_LUT_EN bit to
>>> be unset. With the change I sent in response to Daniel's reply, gamma LUT
>>> state modification should now be fully atomic so there shouldn't be a need
>>> for the second state check there anymore (if my logic is incorrect please
>>> explain).
>>
>>
>> After reading the commit message for adding gamma control for rk3399[0] i understand
>> what is going on here:
>>
>> we should run into the if block in two cases:
>>
>> (1) if the state->gamma_lut is null, we just need to disable dsp_lut and return,
>>
>> this is why vop1 code check !state->gamma_lut again at the end.
>>
>> (2) for platform unlinke rk3399(rk3566/8), we also need to disable dsp_lut befor we
>> write gamma_lut data, for platform like rk3399(rk3588), we don't need do the disable,
>> this is why vop1 code also has a !VOP_HAS_REG(vop, common, update_gamma_lut) check.
>>
>> so we also need a similary check here:
>> (1) if the state->gamma_lut is null, disable dsp lut and return directly.
>>
>> (1) if the state has a gamma_lut, we shoud dsiable dsp_lut than write gamma lut data on rk3566/8,
>> buf for rk3588, we should not disable dsp_lut before write gamma
>>
>>
>> [0]https://lists.infradead.org/pipermail/linux-rockchip/2021-October/028132.html
>>
> 
> Ok I see it. So In my patch it doesn't make sense at all to check it again
> (forgot about that extra if statement condition there, which I cut out
> when porting to VOP2). I reworked my patch further for it to handle RK3588
> case and to better utilize DRM atomic updates. It's contained in the
> response to Daniel's review [1]. I experienced some problems so I'm waiting
> for his response/comment on that.
> 
> Regarding RK3588, I checked RK3588 TRM v1.0 part2. In its VOP2 section I
> found:
>    - SYS_CTRL_LUT_PORT_SEL: gamma_ahb_write_sel (seems to represent the
>      same concept as LUT_PORT_SEL in case of RK356x)

  We should also setting it to she VP id we want write gamma, this is used for selet
  ahb bus.

>    - VOP2_POST0_DSP_CTRL: gamma_update_en (seems to represent the same
>      concept as in VOP1 in case of RK3399)
we also need to set it every time we update the gamma lut.

>    - I also found dsp_lut_en but I presume it is a bug in documentation.

No, it is not a bug, we should set it when we enable gamma lut, we just don't
need to disable it before we update gamma lut.

Here is some code you can take as reference [0]
[0]:https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3437
> 
> Should RK3588 be handled as RK3399? (gamma LUT can be written directly but
> gamma_update_en bit has to be set before). What about gamma_ahb_write_sel?
>   
> [1] https://lkml.org/lkml/2024/7/27/293
> 
> Best Regards, Piotr Zalewski