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

[Piotr Zalewski]

On Thursday, July 25th, 2024 at 12:28 PM, Daniel Stone <daniel@xxxxxxxxxxxxx> wrote:

> Hi Piotr,

Hi, Daniel!

Thank you for the review.

> 
> In the atomic_enable callback, we are already holding the VOP lock,
> and waiting to set cfg_done etc - we then do it all over again here.
> This should all be atomic, so that we configure the LUT whilst doing
> the setup, and then only call cfg_done once, to avoid showing the user
> intermediate states which only later converge on the desired final
> state.
> 

I based my patch on how gamma LUT is handled in VOP. There, in atomic 
enable, gamma LUT write takes places at the end too, after the mutex was 
already first-time unlocked. I understand the concept of DRM atomic state 
updates and what you wrote makes sense.

Below is what I came up with to make it fulfill atomicity requirement. 
Frankly, the code ended up simpler. I tested it on RK3566 (Pinetab2).
Let me know what do you think.

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 37fcf544a5fd..cba92239dcbc 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1497,22 +1497,6 @@ static bool vop2_vp_dsp_lut_is_enabled(struct vop2_video_port *vp)
 	    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_video_port *vp = to_vop2_video_port(crtc);
@@ -1532,11 +1516,12 @@ static void vop2_crtc_write_gamma_lut(struct vop2 *vop2, struct drm_crtc *crtc)
 }
 
 static void vop2_crtc_gamma_set(struct vop2 *vop2, struct drm_crtc *crtc,
-				struct drm_crtc_state *old_state)
+				struct drm_crtc_state *old_state,
+				u32* dsp_ctrl)
 {
 	struct drm_crtc_state *state = crtc->state;
 	struct vop2_video_port *vp = to_vop2_video_port(crtc);
-	u32 dsp_ctrl;
+	u32 _dsp_ctrl;
 	int ret;
 
 	if (!vop2->lut_regs)
@@ -1547,37 +1532,27 @@ static void vop2_crtc_gamma_set(struct vop2 *vop2, struct drm_crtc *crtc,
 		 * 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);
-
-		if (ret) {
-			DRM_DEV_ERROR(vop2->dev, "display LUT RAM enable timeout!\n");
-			return;
-		}
+		*dsp_ctrl &= ~RK3568_VP_DSP_CTRL__DSP_LUT_EN;
 
 		if (!state->gamma_lut)
 			return;
 	}
 
+	/*
+	 * 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);
+	if (ret) {
+		DRM_DEV_ERROR(vop2->dev, "display LUT RAM enable timeout!\n");
+		return;
+	}
 
-	vop2_lock(vop2);
 	vop2_crtc_write_gamma_lut(vop2, crtc);
 	vop2_writel(vp->vop2, RK3568_LUT_PORT_SEL, vp->id);
 
-	vop2_vp_dsp_lut_enable(vp);
-
-	vop2_cfg_done(vp);
-	vop2_unlock(vop2);
+	*dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_LUT_EN;
 }
 
 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);
+
 	vop2_cfg_done(vp);
 
 	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
@@ -2160,8 +2138,6 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *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,
@@ -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);
+	}
 }
 
 static void vop2_crtc_atomic_flush(struct drm_crtc *crtc,


Best regards, Piotr Zalewski