[PATCH 2/5] drm: lcdif: move controller enable into atomic_flush

[Ying Liu]

Hi,

On Wednesday, September 20, 2023 6:31 PM Lucas Stach <l.stach at pengutronix.de> wrote:
> Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
> state before the scanout is started.
> 
> Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
> ---
>  drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index f5bfe8b52920..4acf6914a8d1 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> *crtc,
>  static void lcdif_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 lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
>  	struct drm_pending_vblank_event *event;
>  	u32 reg;
> @@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct drm_crtc
> *crtc,
>  	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
>  	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> 
> +	if (drm_atomic_crtc_needs_modeset(crtc_state))
> +		lcdif_enable_controller(lcdif);

Moving lcdif_enable_controller() function call from atomic_enable to
atomic_flush would change CRTC vs bridge enablement order, which
effectively makes bridge enablement happen prior to CRTC enablement,
see drm_atomic_helper_commit_tail_rpm() detail implementation.  The
reversed order potentially causes malfunctions of the bridge.

BTW, even if it's ok to move the function call, it would be better to call
lcdif_enable_controller() before CTRLDESCL0_5_SHADOW_LOAD_EN is
set so that the original sequence is kept.  Also, LCDIF chapter in SoC RMs
indicates that the shadow load enablement is the last step in
"Software flow diagram".

Regards,
Liu Ying

> +
>  	event = crtc->state->event;
>  	crtc->state->event = NULL;
> 
> @@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc
> *crtc,
> 
> 	writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
>  		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
>  	}
> -	lcdif_enable_controller(lcdif);
> 
>  	drm_crtc_vblank_on(crtc);
>  }
> --
> 2.39.2