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

[Lucas Stach]

Am Donnerstag, dem 21.09.2023 um 08:18 +0000 schrieb 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.
> 
Ah, I see what you mean now. Moving the controller enable into flush is
violating the documented flow that bridges might assume the display
link to be running in their enable callback. I don't think any of the
bridges connected to the LCDIF really care about this, but it's bad
nevertheless.

This seems to be a generic issue with the RPM commit tail. Enabling the
display link in the CRTC atomic_enable without all the plane state
being set up in the HW sounds wrong and I think we also don't want to
hoist (duplicate) all the plane setup into the CRTC enable.

I'll look into this some more to see if we can do something better that
doesn't violate the bridge chain constraints.

Regards,
Lucas

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