linux-kernel - Re: [PATCH v3] rockchip/drm: vop2: add support for gamma LUT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <y_bpV0FizWkAqq0XPIKrauaZ07r_Ds-MnDr696Y1qFTLNiLsx7pL4C-Zsu-K9TzVBJ85L5cdkzDQOExcXXTf0owUYramMHdZd4erMRpJUXI=@proton.me>
Date: Mon, 29 Jul 2024 21:20:45 +0000
From: Piotr Zalewski <pZ010001011111@...ton.me>
To: Andy Yan <andy.yan@...k-chips.com>
Cc: "airlied@...il.com" <airlied@...il.com>, "daniel@...ll.ch" <daniel@...ll.ch>, "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>, "heiko@...ech.de" <heiko@...ech.de>, "hjc@...k-chips.com" <hjc@...k-chips.com>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-rockchip@...ts.infradead.org" <linux-rockchip@...ts.infradead.org>, "maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>, "mripard@...nel.org" <mripard@...nel.org>, "tzimmermann@...e.de" <tzimmermann@...e.de>
Subject: Re: [PATCH v3] rockchip/drm: vop2: add support for gamma LUT

Hi Andy

On Monday, July 29th, 2024 at 4:35 AM, Andy Yan <andy.yan@...k-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)
  - 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