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