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

Hoi all,


> 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 <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > >  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.

Thanks for all the input, since Ahmad also pointed out that there is already a
driver, i'll restructure this patchstack around the upstream on and weave in all
the input so far -> expect a v2.

> 
> I am curious if this is actually all necessary.
>

i'll admit that i don't have a very deep understanding of the whole pixel
pipeline, and am just putting/copying (from the kernel mostly) things together
and "massaging" them until the display showed something (without any weird
distortions or other artifacts)

> In the driver Johannes send, there is the 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. */
> 
> in lcdif_set_plane of the imx-lcdif driver.
> 
> This exactly flushes the buffer once. I also did this but also
> implemented lcdif_crtc_atomic_flush in drivers/video/lcdif_kms.c as the
> fb_flush callback which will do exactly this LOAD_EN call every time a
> new frame was written. With this change I had no issues with distorted
> images.
> 
> > > > > +}
> > > > > +
> > > > >  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)
> > > >
> > > >
>

gruß
Johannes