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

[Piotr Zalewski]

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:
```
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);
```


> > > > 
> > > > drm_crtc_helper_add(&vp->crtc, &vop2_crtc_helper_funcs);
> > > > + if (vop2->lut_regs && vp->crtc.dev != NULL) {
> > > > + const struct vop2_video_port_data *vp_data = &vop2_data->vp[vp->id];
> > > > 
> > > > + drm_mode_crtc_set_gamma_size(&vp->crtc, vp_data->gamma_lut_len);
> > > > + drm_crtc_enable_color_mgmt(&vp->crtc, 0, false,
> > > > + vp_data->gamma_lut_len);
> > > 
> > > It seems that we can keep it in one line, the default limit of linux kernel coding style is 100 characters now.
> > 
> > Thanks. I didn't know, I will amend it.
> 
> 
> See bdc48fa11e46("checkpatch/coding-style: deprecate 80-column warning")
> 

Interesting.

Best regards, Piotr Zalewski