On Tuesday, July 30th, 2024 at 12:43 PM, Andy Yan <andy.yan@xxxxxxxxxxxxxx> wrote:
> Hi Piotr,
Hi Andy
> On 7/30/24 05:20, Piotr Zalewski wrote:
>
> >
> > On Monday, July 29th, 2024 at 4:35 AM, Andy Yan andy.yan@xxxxxxxxxxxxxx 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
>
Thank you for further clarification. I will include it in the next version
of my patch.
> > 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
