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

[Andy Yan]

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.

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


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