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

[Piotr Zalewski]

On Friday, July 26th, 2024 at 1:31 PM, Daniel Stone <daniel@xxxxxxxxxxxxx> wrote:

> Hi Piotr,

Hi Daniel, sorry for delayed response.

>
> > static void vop2_dither_setup(struct drm_crtc *crtc, u32 *dsp_ctrl)
> > @@ -2152,6 +2127,9 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
> >
> > vop2_post_config(crtc);
> >
> > + if (crtc->state->gamma_lut)
> > + vop2_crtc_gamma_set(vop2, crtc, old_state, &dsp_ctrl);
>
>
> I think this call should be unconditional, so that we correctly
> program LUT_DIS if there is no LUT set up during enable.
>

Noted

> > @@ -2599,8 +2575,17 @@ static void vop2_crtc_atomic_begin(struct drm_crtc *crtc,
> > 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);
> > + if (crtc_state->color_mgmt_changed && !crtc_state->active_changed) {
> > + u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);;
> > +
> > + vop2_lock(vop2);
> > +
> > + vop2_crtc_gamma_set(vop2, crtc, old_crtc_state, &dsp_ctrl);
> > +
> > + vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
> > + vop2_cfg_done(vp);
> > + vop2_unlock(vop2);
> > + }
>
>
> Calling lock/set/write/done/unlock here seems like an anti-pattern;
> the cfg_done is already written in atomic_flush, so at least that part
> is not necessary.

Right sorry for sending such code. I wanted to demonstrate the idea.

> On platforms like RK3588, it looks like the new LUT can just be
> written directly from atomic_begin without needing to program
> DSP_CTRL, take locks, or synchronise against anything, so that should
> be an easy straight-line path.
>
> On platforms like RK3568, it would probably be better to set
> mode_changed when the colour management configuration changes. That
> will give you a good point to synchronise the cross-VOP dependencies
> (i.e. claim or release the shared LUT when it is being
> enabled/disabled), and also a hint to userspace that it is not going
> to be a seamless transition as the LUT is disabled, programmed, then
> re-enabled.
>
> I think this would end up in a net reduction of LoC as well as a net
> reduction of code weirdness.

Okay so if I understood you correctly you suggest setting mode_changed in 
order to trigger full modeset (check->begin->flush->enable) to cleanly 
handle the RK356x case and for RK3588 just write the LUT in begin and 
don't do anything more.

I tried to do this but couldn't get the thing to work. It seems like 
setting the mode_changed manually in atomic_check, messes something up 
with the CRTC states. Namely, the retrieved new state in subsequent 
atomic_begin call isn't the same state (as a result, flags 
color_mgmt_changed and mode_changed are both false when they are checked 
in atomic_begin which stops further gamma LUT reconfiguration). Below is 
how I reworked this (included only parts which changed).

As already mentioned, in atomic check the mode_changed flag is set, then in 
atomic begin gamma LUT is disabled and DSP_LUT_EN bit unset (or gamma LUT 
is written directly based on if it's RK356x or not). Then in atomic_flush 
video port is selected and gamma LUT is written. Gamma LUT is enabled in 
atomic_enable.

Perhaps I'm missing something important, if so please hint what exactly. 

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 9873172e3fd3..9c5ee2d85a58 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2051,6 +2092,13 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	clk_set_rate(vp->dclk, clock);
 
+	/**
+	 * Enable gamma LUT in atomic enable
+	 */
+	if (crtc_state->gamma_lut && (vop2->data->soc_id == 3566 ||
+				      vop2->data->soc_id == 3568))
+		dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_LUT_EN;
+
 	vop2_post_config(crtc);
 
 	vop2_cfg_done(vp);
@@ -2062,6 +2110,39 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 	vop2_unlock(vop2);
 }
 
+static int vop2_crtc_atomic_check_gamma(struct vop2_video_port *vp,
+					struct drm_crtc *crtc,
+					struct drm_crtc_state *crtc_state)
+{
+	struct vop2 *vop2 = vp->vop2;
+	unsigned int len;
+
+	if (!vp->vop2->lut_regs || !crtc_state->color_mgmt_changed ||
+	    !crtc_state->gamma_lut)
+		return 0;
+
+	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;
+	}
+
+	if (!crtc_state->mode_changed && (vop2->data->soc_id == 3566 ||
+					  vop2->data->soc_id == 3568)) {
+		int ret;
+
+		crtc_state->mode_changed = true;
+
+		ret = drm_atomic_helper_check_modeset(crtc->dev,
+				crtc_state->state);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int vop2_crtc_atomic_check(struct drm_crtc *crtc,
 				  struct drm_atomic_state *state)
 {
@@ -2069,6 +2150,11 @@ static int vop2_crtc_atomic_check(struct drm_crtc *crtc,
 	struct drm_plane *plane;
 	int nplanes = 0;
 	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	int ret;
+
+	ret = vop2_crtc_atomic_check_gamma(vp, crtc, crtc_state);
+	if (ret)
+		return ret;
 
 	drm_atomic_crtc_state_for_each_plane(plane, crtc_state)
 		nplanes++;
@@ -2456,9 +2542,36 @@ static void vop2_setup_dly_for_windows(struct vop2 *vop2)
 	vop2_writel(vop2, RK3568_SMART_DLY_NUM, sdly);
 }
 
+static void vop2_crtc_atomic_begin_gamma(struct vop2 *vop2,
+					struct drm_crtc *crtc) {
+	struct vop2_video_port *vp = to_vop2_video_port(crtc);
+	int ret;
+	u32 dsp_ctrl;
+
+	vop2_lock(vop2);
+	/**
+	 * Gamma lut always has to be disabled in both cases. When gamma
+	 * lut is enabled it needs to be disabled before writing (Applies
+	 * only to RK356x SoCs).
+	 */
+	vop2_vp_dsp_lut_disable(vp);
+
+	vop2_cfg_done(vp);
+	vop2_unlock(vop2);
+
+	ret = readx_poll_timeout(vop2_vp_dsp_lut_is_enabled, vp, dsp_ctrl,
+			!dsp_ctrl, 5, 30 * 1000);
+	if (ret) {
+		DRM_DEV_ERROR(vop2->dev,
+			"display LUT RAM enable timeout!\n");
+	}
+}
+
 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 vop2_video_port *vp = to_vop2_video_port(crtc);
 	struct vop2 *vop2 = vp->vop2;
 	struct drm_plane *plane;
@@ -2482,13 +2595,50 @@ 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) {
+		/**
+		 * When the SoC is RK356x gamma lut change always triggers
+		 * mode_changed - proceed with gamma lut disable
+		 */
+		if (crtc_state->mode_changed &&
+		    (vop2->data->soc_id == 3566 ||
+		     vop2->data->soc_id == 3568))
+			vop2_crtc_atomic_begin_gamma(vop2, crtc);
+		/**
+		 * When the Soc isn't RK356x (eg. RK3588) gamma lut can be
+		 * written directly
+		 */
+		else if (crtc_state->gamma_lut)
+			vop2_crtc_write_gamma_lut(vop2, crtc);
+	}
+}
+
+static void vop2_crtc_atomic_flush_gamma(struct vop2 *vop2, struct vop2_video_port *vp, struct drm_crtc *crtc)
+{
+	if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568) {
+		vop2_lock(vop2);
+
+		vop2_crtc_write_gamma_lut(vop2, crtc);
+		vop2_writel(vp->vop2, RK3568_LUT_PORT_SEL, vp->id);
+
+		vop2_unlock(vop2);
+	}
 }
 
 static void vop2_crtc_atomic_flush(struct drm_crtc *crtc,
 				   struct drm_atomic_state *state)
 {
+	struct drm_crtc_state *crtc_state =
+		drm_atomic_get_new_crtc_state(state, crtc);
 	struct vop2_video_port *vp = to_vop2_video_port(crtc);
 
+	/**
+	 * Write gamma LUT in atomic flush
+	 */
+	if (crtc_state->mode_changed && crtc_state->gamma_lut)
+		vop2_crtc_atomic_flush_gamma(vp->vop2, vp, crtc);
+
 	vop2_post_config(crtc);
 
 	vop2_cfg_done(vp);