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

[Lucas Stach]

Am Donnerstag, dem 21.09.2023 um 07:13 +0000 schrieb 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.
> 
This has nothing to do with the bridge after the LCDIF controller. The
RPM commit_tail implementation enables the CRTC before all the plane
state is set up. To avoid having to program the plane state in the CRTC
enable this series defers the actual controller enable to the last step
of the atomic commit. This way the plane state is programmed the usual
way via the atomic update_plane callback and we don't need to duplicate
this setup.

> 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".

This flow chart shows how the double buffered update should work, it
doesn't show the initial controller enable sequence. Without the shadow
load enable bit being set before the controller enable I could observe
sporadic issues on the first frame where the DMA engine would read the
wrong memory address.

Regards,
Lucas

> 
> 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
>