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

[Ying Liu]

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

Not sure if the first bridges(all in SoCs for now) connected to LCDIF care
about the order.  But, it's really about the overall display pipeline including
wild out-of-SoC bridges in a chain.

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

Right, seems to be a generic issue.

Regards,
Liu Ying

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