[PATCH v5] rockchip/drm: vop2: add support for gamma LUT

[Piotr Zalewski]

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 at proton.me> wrote:

> Hi Andy,
> 
> On Wednesday, October 16th, 2024 at 3:10 AM, Andy Yan andyshrk at 163.com wrote:
> 
> > Hi Piotr,
> > 
> > At 2024-10-16 04:13:40, "Piotr Zalewski" pZ010001011111 at proton.me wrote:
> > 
> > > Hi Andy
> > > 
> > > On Tuesday, October 15th, 2024 at 2:22 PM, Andy Yan andyshrk at 163.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