Message-ID: <d019761504b540600d9fc7a585d6f95f@manjaro.org>
Date: Mon, 22 Jul 2024 08:37:37 +0200
From: Dragan Simic <dsimic@...jaro.org>
To: Piotr Zalewski <pZ010001011111@...ton.me>
Cc: linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, dri-devel@...ts.freedesktop.org,
 daniel@...ll.ch, airlied@...il.com, tzimmermann@...e.de, mripard@...nel.org,
 maarten.lankhorst@...ux.intel.com, andy.yan@...k-chips.com, heiko@...ech.de,
 hjc@...k-chips.com
Subject: Re: [PATCH RESEND] rockchip/drm: vop2: add support for gamma LUT

Hello Piotr,

Thanks for the patch.  Please see a few general comments below.

On 2024-07-21 12:06, Piotr Zalewski wrote:
> Add support for gamma LUT in VOP2 driver. The implementation is based 
> on
> the one found in VOP driver and modified to be compatible with VOP2. 
> Blue
> and red channels in gamma LUT register write were swapped with respect 
> to
> how gamma LUT values are written in VOP. Write of the current video 
> port id
> to VOP2_SYS_LUT_PORT_SEL register was added before the write of 
> DSP_LUT_EN
> bit. Gamma size is set and drm color management is enabled for each 
> video
> port's CRTC except ones which have no associated device. Tested on 
> RK3566
> (Pinetab2).
> 
> Signed-off-by: Piotr Zalewski <pZ010001011111@...ton.me>
> ---
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 9873172e3fd3..16abdc4a59a8 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -278,6 +278,15 @@ static u32 vop2_readl(struct vop2 *vop2, u32 
> offset)
>  	return val;
>  }
> 
> +static u32 vop2_vp_read(struct vop2_video_port *vp, u32 offset)
> +{
> +	u32 val;
> +
> +	regmap_read(vp->vop2->map, vp->data->offset + offset, &val);
> +
> +	return val;
> +}
> +
>  static void vop2_win_write(const struct vop2_win *win, unsigned int 
> reg, u32 v)
>  {
>  	regmap_field_write(win->reg[reg], v);
> @@ -1482,6 +1491,97 @@ static bool vop2_crtc_mode_fixup(struct drm_crtc 
> *crtc,
>  	return true;
>  }
> 
> +static bool vop2_vp_dsp_lut_is_enabled(struct vop2_video_port *vp)
> +{
> +	return (u32) (vop2_vp_read(vp, RK3568_VP_DSP_CTRL) &
> RK3568_VP_DSP_CTRL__DSP_LUT_EN) >
> +	    0;
> +}
> +
> +static void vop2_vp_dsp_lut_enable(struct vop2_video_port *vp)
> +{
> +	u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);
> +
> +	dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_LUT_EN;
> +	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
> +}
> +
> +static void vop2_vp_dsp_lut_disable(struct vop2_video_port *vp)
> +{
> +	u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);
> +
> +	dsp_ctrl &= ~RK3568_VP_DSP_CTRL__DSP_LUT_EN;
> +	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
> +}
> +
> +static void vop2_crtc_write_gamma_lut(struct vop2 *vop2, struct 
> drm_crtc *crtc)
> +{
> +	const struct vop2_data *vop2_data = vop2->data;
> +	const struct vop2_video_port *vp = to_vop2_video_port(crtc);
> +	const struct vop2_video_port_data *vp_data = &vop2_data->vp[vp->id];

Perhaps vop2_data could be dropped as a separate variable.

> +
> +	struct drm_color_lut *lut = crtc->state->gamma_lut->data;
> +	unsigned int i, bpc = ilog2(vp_data->gamma_lut_len);
> +	u32 word;
> +
> +	for (i = 0; i < crtc->gamma_size; i++) {
> +		word = (drm_color_lut_extract(lut[i].blue, bpc) << (2 * bpc)) |
> +		    (drm_color_lut_extract(lut[i].green, bpc) << bpc) |
> +		    drm_color_lut_extract(lut[i].red, bpc);
> +
> +		writel(word, vop2->lut_regs + i * 4);
> +	}
> +}
> +
> +static void vop2_crtc_gamma_set(struct vop2 *vop2, struct drm_crtc 
> *crtc,
> +				struct drm_crtc_state *old_state)
> +{
> +	struct drm_crtc_state *state = crtc->state;
> +	struct vop2_video_port *vp = to_vop2_video_port(crtc);
> +	u32 dsp_ctrl;
> +	int ret;
> +
> +	if (!vop2->lut_regs)
> +		return;
> +
> +	if (!state->gamma_lut) {
> +		/*
> +		 * To disable gamma (gamma_lut is null) or to write
> +		 * an update to the LUT, clear dsp_lut_en.
> +		 */
> +		vop2_lock(vop2);
> +
> +		vop2_vp_dsp_lut_disable(vp);
> +
> +		vop2_cfg_done(vp);
> +		vop2_unlock(vop2);
> +		/*
> +		 * In order to write the LUT to the internal memory,
> +		 * we need to first make sure the dsp_lut_en bit is cleared.
> +		 */
> +		ret =
> +		    readx_poll_timeout(vop2_vp_dsp_lut_is_enabled, vp, dsp_ctrl,
> !dsp_ctrl, 5,
> +				       30 * 1000);

It would look nicer to keep "ret =" and "readx_poll_timeout(..." in the 
same line,
and to introduce line breaks later in the same line.

> +
> +		if (ret) {
> +			DRM_DEV_ERROR(vop2->dev, "display LUT RAM enable timeout!\n");
> +			return;
> +		}
> +
> +		if (!state->gamma_lut)
> +			return;
> +	}
> +
> +	vop2_crtc_write_gamma_lut(vop2, crtc);
> +
> +	vop2_lock(vop2);
> +	vop2_writel(vp->vop2, RK3568_LUT_PORT_SEL, vp->id);
> +
> +	vop2_vp_dsp_lut_enable(vp);
> +
> +	vop2_cfg_done(vp);
> +	vop2_unlock(vop2);
> +}
> +
>  static void vop2_dither_setup(struct drm_crtc *crtc, u32 *dsp_ctrl)
>  {
>  	struct rockchip_crtc_state *vcstate = 
> to_rockchip_crtc_state(crtc->state);
> @@ -1925,6 +2025,7 @@ static void vop2_crtc_atomic_enable(struct 
> drm_crtc *crtc,
>  	const struct vop2_data *vop2_data = vop2->data;
>  	const struct vop2_video_port_data *vp_data = &vop2_data->vp[vp->id];
>  	struct drm_crtc_state *crtc_state =
> drm_atomic_get_new_crtc_state(state, crtc);
> +	struct drm_crtc_state *old_state = 
> drm_atomic_get_old_crtc_state(state, crtc);
>  	struct rockchip_crtc_state *vcstate = 
> to_rockchip_crtc_state(crtc->state);
>  	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
>  	unsigned long clock = mode->crtc_clock * 1000;
> @@ -2060,6 +2161,9 @@ static void vop2_crtc_atomic_enable(struct 
> drm_crtc *crtc,
>  	drm_crtc_vblank_on(crtc);
> 
>  	vop2_unlock(vop2);
> +
> +	if (crtc->state->gamma_lut)
> +		vop2_crtc_gamma_set(vop2, crtc, old_state);
>  }
> 
>  static int vop2_crtc_atomic_check(struct drm_crtc *crtc,
> @@ -2070,6 +2174,16 @@ static int vop2_crtc_atomic_check(struct 
> drm_crtc *crtc,
>  	int nplanes = 0;
>  	struct drm_crtc_state *crtc_state =
> drm_atomic_get_new_crtc_state(state, crtc);
> 
> +	if (vp->vop2->lut_regs && crtc_state->color_mgmt_changed &&
> crtc_state->gamma_lut) {
> +		unsigned int len = drm_color_lut_size(crtc_state->gamma_lut);
> +
> +		if (len != crtc->gamma_size) {
> +			DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
> +				      len, crtc->gamma_size);
> +			return -EINVAL;
> +		}
> +	}
> +
>  	drm_atomic_crtc_state_for_each_plane(plane, crtc_state)
>  		nplanes++;
> 
> @@ -2459,6 +2573,10 @@ static void vop2_setup_dly_for_windows(struct 
> vop2 *vop2)
>  static void vop2_crtc_atomic_begin(struct drm_crtc *crtc,
>  				   struct drm_atomic_state *state)
>  {
> +	struct drm_crtc_state *crtc_state = 
> drm_atomic_get_new_crtc_state(state,
> +									  crtc);
> +	struct drm_crtc_state *old_crtc_state = 
> drm_atomic_get_old_crtc_state(state,
> +									      crtc);
>  	struct vop2_video_port *vp = to_vop2_video_port(crtc);
>  	struct vop2 *vop2 = vp->vop2;
>  	struct drm_plane *plane;
> @@ -2482,6 +2600,9 @@ static void vop2_crtc_atomic_begin(struct 
> drm_crtc *crtc,
>  	vop2_setup_layer_mixer(vp);
>  	vop2_setup_alpha(vp);
>  	vop2_setup_dly_for_windows(vop2);
> +
> +	if (crtc_state->color_mgmt_changed && !crtc_state->active_changed)
> +		vop2_crtc_gamma_set(vop2, crtc, old_crtc_state);
>  }
> 
>  static void vop2_crtc_atomic_flush(struct drm_crtc *crtc,
> @@ -2791,6 +2912,14 @@ static int vop2_create_crtcs(struct vop2 *vop2)
> 
>  		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);
> +		}
> +
>  		init_completion(&vp->dsp_hold_completion);
>  	}
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
> index 615a16196aff..3a58b73fa876 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
> @@ -394,6 +394,7 @@ enum dst_factor_mode {
>  #define RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN		BIT(15)
> 
>  #define RK3568_VP_DSP_CTRL__STANDBY			BIT(31)
> +#define RK3568_VP_DSP_CTRL__DSP_LUT_EN			BIT(28)
>  #define RK3568_VP_DSP_CTRL__DITHER_DOWN_MODE		BIT(20)
>  #define RK3568_VP_DSP_CTRL__DITHER_DOWN_SEL		GENMASK(19, 18)
>  #define RK3568_VP_DSP_CTRL__DITHER_DOWN_EN		BIT(17)

Powered by blists - more mailing lists