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

[Ying Liu]

On Thursday, September 21, 2023 3:55 PM Lucas Stach <l.stach at pengutronix.de> wrote:
> 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

IMHO, the reserved CRTC vs bridge enablement order is relevant for
the overall display pipeline.

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

I understand the patch avoids some duplications via deferral controller
enablement time point, but the reversed CRTC vs bridge enablement order
is the concern here.

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

You are right.  The downstream driver also enables shadow load before
the entire controller.

Regards,
Liu Ying

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