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
