Hi Lucas,
On Tue 31 Mar 26, 11:14, Lucas Stach wrote:
> Am Dienstag, dem 31.03.2026 um 00:46 +0200 schrieb Paul Kocialkowski:
> > It is necessary to wait for the full frame to finish streaming
> > through the DMA engine before we can safely disable it by removing
> > the DISP_PARA_DISP_ON bit. Disabling it in-flight can leave the
> > hardware confused and unable to resume streaming for the next frame.
> >
> > This causes the FIFO underrun and empty status bits to be set and
> > a single solid color to be shown on the display, coming from one of
> > the pixels of the previous frame. The issue occurs sporadically when
> > a new mode is set, which triggers the crtc disable and enable paths.
> >
> > Setting the shadow load bit and waiting for it to be cleared by the
> > DMA engine allows waiting for completion.
> >
> > The NXP BSP driver addresses this issue with a hardcoded 25 ms sleep.
> >
> > Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > Co-developed-by: Lucas Stach <[email protected]>
> > ---
> > drivers/gpu/drm/mxsfb/lcdif_kms.c | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index 1aac354041c7..7dce7f48d938 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > @@ -393,6 +393,22 @@ static void lcdif_disable_controller(struct
> > lcdif_drm_private *lcdif)
> > if (ret)
> > drm_err(lcdif->drm, "Failed to disable controller!\n");
> >
> You can drop this no-op poll above...You're right, it looks a bit weird to keep it as it's not needed. > > + /* > > + * It is necessary to wait for the full frame to finish streaming > > + * through the DMA engine before we can safely disable it by removing > > + * the DISP_PARA_DISP_ON bit. Disabling it in-flight can leave the > > + * hardware confused and unable to resume streaming for the next frame. > > + */ > > + reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5); > > + reg |= CTRLDESCL0_5_SHADOW_LOAD_EN; > > + writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5); > > + > .. then setting the shadow load enable bit can be merged with the > access clearing the DMA enable bit. I've just tried it out and it works just as well! > > + ret = readl_poll_timeout(lcdif->base + LCDC_V8_CTRLDESCL0_5, > > + reg, !(reg & CTRLDESCL0_5_SHADOW_LOAD_EN), > > + 0, 36000); /* Wait ~2 frame times max */ > > I know this is just a copy from the existing poll, but I don't think > the busy looping makes a lot of sense. I guess relaxing the poll by a > 100us or even 200us wait between checks wouldn't hurt. Yeah good point too. I think 200 us is definitely fine since we're looking at an average wait of a few ms. Will respin the series then! Thanks for the review, Paul > > + if (ret) > > + drm_err(lcdif->drm, "Failed to disable controller!\n"); > > + > > reg = readl(lcdif->base + LCDC_V8_DISP_PARA); > > reg &= ~DISP_PARA_DISP_ON; > > writel(reg, lcdif->base + LCDC_V8_DISP_PARA); > -- Paul Kocialkowski, Independent contractor - sys-base - https://www.sys-base.io/ Free software developer - https://www.paulk.fr/ Expert in multimedia, graphics and embedded hardware support with Linux.
signature.asc
Description: PGP signature
