On Thursday, September 21, 2023 3:55 PM Lucas Stach <l.stach@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > > ---
> > > 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
> >
