Message-ID: <QvjHFQ4xeCu-8Isrm_jtNRWLowVNFzC8qnHJ6LUGI2iFTTJoEK8fBrXjG9LUrn5Wt9fJ9F04ukEf-koifwCR0uH9nr0AelyiWI85KASNkOQ=@proton.me>
Date: Wed, 16 Oct 2024 09:16:56 +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,

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.
> 
> 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

When I think about it, userspace tools set gamma LUT for all
active CRTCs with gamma LUT support[2] so essentially when there is only 
one which can be active implicitly (like in the case of rk356x) in BSP code 
you provided[1], it will go with the first one and in the second approach 
it will end up setting second one in the end. I think this 
solution[1] is better if assumed that first CRTC is always "main" one.

I will rework it to[1] and test it. (Have to check if hdmi out on pt2 works).

[2] https://gitlab.com/chinstrap/gammastep/-/blob/master/src/gamma-drm.c?ref_type=heads#L261

> > ```
> > 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

Powered by blists - more mailing lists