Re: [PATCH 6/6] video: imx-lcdif: drain write-combine buffer before LCDIF DMA reads pixels

[Michael Grzeschik <mgr@kernel.org>]

Hi Johannes

On Tue, Jun 02, 2026 at 02:12:43PM +0200, Lucas Stach wrote:
> Am Dienstag, dem 02.06.2026 um 11:37 +0200 schrieb Ahmad Fatoum:
> > 
> > On 6/2/26 10:12 AM, Lucas Stach wrote:
> > > Am Dienstag, dem 02.06.2026 um 04:09 +0000 schrieb Johannes Schneider:
> > > > From: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
> > > > 
> > > > The LCDIF framebuffer is allocated as Normal Non-Cacheable (write-combine)
> > > > memory.  After the splash command calls gu_screen_blit(), which copies the
> > > > decoded PNG via memcpy() from a cached shadow buffer into the hardware
> > > > framebuffer, writes may still reside in the CPU write buffer and not yet
> > > > be visible to the LCDIF DMA engine.  On hardware this manifests as
> > > > partial or corrupted rendering — the bottom portion of the splash image
> > > > renders black or shows garbage.
> > > > 
> > > > Implement fb_damage() to execute a DSB SY barrier after each blit,
> > > > ensuring all pending write-combine writes are committed to DRAM before
> > > > the LCDIF reads the pixel data.
> > > > 
> > > > Assisted-by: Claude:claude-sonnet-4-6
> > > > Signed-of-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
> > > > ---
> > > >  drivers/video/imx-lcdif.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > > 
> > > > diff --git a/drivers/video/imx-lcdif.c b/drivers/video/imx-lcdif.c
> > > > index ae5976c771..8d9b40be0f 100644
> > > > --- a/drivers/video/imx-lcdif.c
> > > > +++ b/drivers/video/imx-lcdif.c
> > > > @@ -20,6 +20,7 @@
> > > >  #include <of_graph.h>
> > > >  #include <video/media-bus-format.h>
> > > >  #include <video/vpl.h>
> > > > +#include <asm/system.h>
> > > >  
> > > >  /* LCDIF V8 register map */
> > > >  #define LCDC_V8_CTRL			0x00
> > > > @@ -262,9 +263,21 @@ static void lcdif_fb_disable(struct fb_info *info)
> > > >  	clk_disable_unprepare(priv->clk_axi);
> > > >  }
> > > >  
> > > > +static void lcdif_fb_damage(struct fb_info *info, struct fb_rect *rect)
> > > > +{
> > > > +	/*
> > > > +	 * The hardware framebuffer is Normal Non-Cacheable (write-combine).
> > > > +	 * Writes from the shadow-buffer memcpy may sit in the CPU write buffer
> > > > +	 * and not yet be visible to the LCDIF DMA engine. Execute DSB SY to
> > > > +	 * drain the write buffer to DRAM before the LCDIF reads the pixels.
> > > > +	 */
> > > > +	dsb();
> > > 
> > > This will probably work in practice, but it doesn't exactly do what the
> > > comment above states. A dsb does not drain the write combine buffers,
> > > it just makes sure that they are drained before the next instruction is
> > > executed.
> > > So if the fb_damage is the last thing that gets executed, this will not
> > > ensure that the writes will actually become visible to external agents.
> > > 
> > > A more robust way to drain the write combine buffers is to execute a
> > > readback of a memory location within the WC region.
> > 
> > Would that on its own suffice though? If we have just the single buffer,
> > flushing the write buffer only reduces the race window, but doesn't
> > remove it as the framebuffer write may still happen in parallel to the
> > scanout.
> 
> Updating the (front-)framebuffer will always be racy against the
> scanout. The flushes just ensure that the writes eventually reach
> memory visible to the scanout DMA master. Without any flushes there is
> nothing in the memory model which would prohibit writes from being
> stuck in the write buffers indefinitely. 

I wonder why this all is actually necessary. The driver has an shadowed
buffer which can be flushed when ready.

Your implementation already has this comment.

+       /* 32bpp XRGB8888 → ARGB8888 pixel format.
+        * SHADOW_LOAD_EN causes the descriptor registers (address, format, pitch)
+        * to be latched into the active registers on the next VSYNC, which is
+        * required for the initial descriptor load in barebox's single-enable
+        * sequence (no DRM page-flip mechanism).  EN is ORed in later by
+        * lcdif_enable() after DISP_ON. */

Look into mine and you will find the function flush function, which will
exactly do this after every frame write.


> > > > +}
> > > > +
> > > >  static struct fb_ops lcdif_fb_ops = {
> > > >  	.fb_enable  = lcdif_fb_enable,
> > > >  	.fb_disable = lcdif_fb_disable,
> > > > +	.fb_damage  = lcdif_fb_damage,
> > > >  };
> > > >  
> > > >  static int lcdif_probe(struct device *dev)
> > > 
> > > 

Regards,
Michael