Hi Andy
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)
- VOP2_POST0_DSP_CTRL: gamma_update_en (seems to represent the same
concept as in VOP1 in case of RK3399)
- I also found dsp_lut_en but I presume it is a bug in documentation.
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
