linux-kernel - Re:Re:Re:[PATCH v5] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <NxE_v2487UegHySoZJi6ZnC6t8tn63tH7DDaCBVIsShJHz1_dH_6o1pRHqGBhHq7LWIKlWitYH7Fbh0YDyEBNN8Pt0u27irk1VwuqMj8Y9c=@proton.me>
Date: Wed, 16 Oct 2024 07:43:12 +0000
From: Piotr Zalewski <pZ010001011111@...ton.me>
To: Andy Yan <andyshrk@....com>
Cc: hjc@...k-chips.com, heiko@...ech.de, andy.yan@...k-chips.com, maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch, dri-devel@...ts.freedesktop.org, linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, skhan@...uxfoundation.org, Daniel Stone <daniel@...ishbar.org>, Dragan Simic <dsimic@...jaro.org>, Diederik de Haas <didi.debian@...ow.org>
Subject: Re:Re:Re:[PATCH v5] rockchip/drm: vop2: add support for gamma LUT

Hi Andy, Ignore the message I'm answering to. I accidentally sent it prematurely. 

On Wednesday, October 16th, 2024 at 9:41 AM, Piotr Zalewski <pZ010001011111@...ton.me> wrote:

> Hi Andy,
> 
> On Wednesday, October 16th, 2024 at 3:10 AM, Andy Yan andyshrk@....com wrote:
> 
> > Hi Piotr,
> > 
> > At 2024-10-16 04:13:40, "Piotr Zalewski" pZ010001011111@...ton.me wrote:
> > 
> > > Hi Andy
> > > 
> > > On Tuesday, October 15th, 2024 at 2:22 PM, Andy Yan andyshrk@....com wrote:
> > > 
> > > > > > > + struct vop2_video_port *vp,
> > > > > > > + struct drm_crtc *crtc,
> > > > > > > + struct drm_crtc_state *crtc_state)
> > > > > > > +{
> > > > > > > +
> > > > > > > + if (vop2->lut_regs && crtc_state->color_mgmt_changed) {
> > > > > > > + if (!crtc_state->gamma_lut) {
> > > > > > > + vop2_vp_dsp_lut_disable(vp);
> > > > > > > + return;
> > > > > > > + }
> > > > > > > +
> > > > > > > + if (vop2_supports_seamless_gamma_lut_update(vop2)) {
> > > > > > 
> > > > > > I think it's bettery to check for rk3568/rk3566 here, the newer soc will all follow
> > > > > > rk3588 support seamless gamma lut update.
> > > > > 
> > > > > I will change in the next version.
> > > > > 
> > > > > > > + vop2_writel(vop2, RK3568_LUT_PORT_SEL, FIELD_PREP(
> > > > > > > + RK3588_LUT_PORT_SEL__GAMMA_AHB_WRITE_SEL,
> > > > > > > + vp->id));
> > > > > > > + vop2_crtc_write_gamma_lut(vop2, crtc);
> > > > > > > + vop2_vp_dsp_lut_enable(vp);
> > > > > > > + vop2_vp_dsp_lut_update_enable(vp);
> > > > > > > + } else {
> > > > > > 
> > > > > > As for rk3566/68, we should do exclusive check here, because there is only
> > > > > > one gamma , only one VP can use it at a time. See my comments in V3:
> > > > > 
> > > > > What do you mean exactly by exclusive check in this case.It's true that
> > > > > gamma LUT is shared across video ports in rk356x but, if I correctly
> > > > > understand, this doesn't forbid to reprogram LUT port sel and allow other
> > > > > VP to use gamma LUT.
> > > > 
> > > > Yes, we can reprogram LUT port sel, but we need to make sure the the dsp_lut_en bit in VPx is cleared if we
> > > > want reprogram LUT port sel form VPx to VPy.
> > > 
> > > Ok I get it now. Is such rework correct? - when gamma LUT for rk356x is
> > > being set, instead of disabling the LUT before the gamma LUT write for the
> > > current CRTC's video port, active video port is selected. Selection is
> > > based on if DSP LUT EN bit is set for particular video port. eg:
> > 
> > If the userspace want to set gamma for CRTCx, then that is indeed where they want to set the
> > gamma on。The driver silently sets the gamma on another CRTC, which is not what the user wants.
> 
> 
> Hello maybe there is some misunderstanding, here is full version of my rework
> ```
> 
> > I think there are two options：
> > （1）return a error if gamma is enable on other CRTC， this is what we done in our BSP code[1]
> > (2) disable the dsp_lut on privious CRTC, then switch to the current CRTC which userspace wants.
> > 
> > [1]https://github.com/armbian/linux-rockchip/blob/rk3576-6.1-dev-2024_04_19/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3666
> 
> 
> Hmm ok in my solution I supposedo
> 
> > > ```
> > > static struct vop2_video_port *vop2_vp_dsp_lut_get_active_vp(struct vop2 *vop2)
> > > {
> > > struct vop2_video_port *vp;
> > > int i;
> > > for (i = 0; i < vop2->data->nr_vps; i++) {
> > > vp = &vop2->vps[i];
> > > 
> > > if (vp->crtc.dev != NULL && vop2_vp_dsp_lut_is_enabled(vp)) {
> > > return vp;
> > > }
> > > }
> > > return NULL;
> > > }
> > > 
> > > (...)
> > > 
> > > struct vop2_video_port *active_vp = vop2_vp_dsp_lut_get_active_vp(vop2);
> > > 
> > > if (active_vp) {
> > > vop2_vp_dsp_lut_disable(active_vp);
> > > vop2_cfg_done(active_vp);
> > > if (!vop2_vp_dsp_lut_poll_disable(active_vp))
> > > return;
> > > }
> > > 
> > > vop2_writel(vop2, RK3568_LUT_PORT_SEL, vp->id);
> > > vop2_crtc_write_gamma_lut(vop2, crtc);
> > > vop2_vp_dsp_lut_enable(vp);
> > > ```
> 
> 
> Best Regards, Piotr Zalewski