[PATCH 0/3] drm: lcdif: FIFO underrun/solid color bug fix

[PATCH 0/3] drm: lcdif: FIFO underrun/solid color bug fix

[Paul Kocialkowski]

This series brings a fix for the FIFO underrun/solid color bug (in the
last commit) and two other changes that may help with reliability.

Paul Kocialkowski (3):
  drm: lcdif: Set undocumented bit to clear FIFO at vsync
  drm: lcdif: Use dedicated set/clr registers for polarity/edge
  drm: lcdif: Wait for vblank before disabling DMA

 drivers/gpu/drm/mxsfb/lcdif_kms.c  | 42 ++++++++++++++++++++++++------
 drivers/gpu/drm/mxsfb/lcdif_regs.h |  1 +
 2 files changed, 35 insertions(+), 8 deletions(-)

-- 
2.53.0

[PATCH 1/3] drm: lcdif: Set undocumented bit to clear FIFO at vsync

[Paul Kocialkowski]

There is an undocumented bit used in the NXP BSP to clear the FIFO
systematically at vsync. In normal operation, the FIFO should already
be empty but it doesn't hurt to add it as an extra safety measure.

Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c  | 3 ++-
 drivers/gpu/drm/mxsfb/lcdif_regs.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index ef3250a5c54f..a00c4f6d63f4 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -338,7 +338,8 @@ static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
 	 * Downstream set it to 256B burst size to improve the memory
 	 * efficiency so set it here too.
 	 */
-	ctrl = CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) |
+	ctrl = CTRLDESCL0_3_STATE_CLEAR_VSYNC |
+	       CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) |
 	       CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb->pitches[0]);
 	writel(ctrl, lcdif->base + LCDC_V8_CTRLDESCL0_3);
 }
diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h
index c55dfb236c1d..17882c593d27 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_regs.h
+++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h
@@ -190,6 +190,7 @@
 #define CTRLDESCL0_1_WIDTH(n)		((n) & 0xffff)
 #define CTRLDESCL0_1_WIDTH_MASK		GENMASK(15, 0)
 
+#define CTRLDESCL0_3_STATE_CLEAR_VSYNC	BIT(23)
 #define CTRLDESCL0_3_P_SIZE(n)		(((n) << 20) & CTRLDESCL0_3_P_SIZE_MASK)
 #define CTRLDESCL0_3_P_SIZE_MASK	GENMASK(22, 20)
 #define CTRLDESCL0_3_T_SIZE(n)		(((n) << 16) & CTRLDESCL0_3_T_SIZE_MASK)
-- 
2.53.0

[PATCH 2/3] drm: lcdif: Use dedicated set/clr registers for polarity/edge

[Paul Kocialkowski]

The lcdif v3 hardware comes with dedicated registers to set and clear
polarity bits in the CTRL register. It is unclear if there is a
difference with writing to the CTRL register directly.

Follow the NXP BSP reference by using these registers, in case there is
a subtle difference caused by using them.

Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index a00c4f6d63f4..1aac354041c7 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -296,18 +296,27 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
 static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
 {
 	struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
-	u32 ctrl = 0;
+	u32 ctrl;
 
 	if (m->flags & DRM_MODE_FLAG_NHSYNC)
-		ctrl |= CTRL_INV_HS;
+		writel(CTRL_INV_HS, lcdif->base + LCDC_V8_CTRL + REG_SET);
+	else
+		writel(CTRL_INV_HS, lcdif->base + LCDC_V8_CTRL + REG_CLR);
+
 	if (m->flags & DRM_MODE_FLAG_NVSYNC)
-		ctrl |= CTRL_INV_VS;
+		writel(CTRL_INV_VS, lcdif->base + LCDC_V8_CTRL + REG_SET);
+	else
+		writel(CTRL_INV_VS, lcdif->base + LCDC_V8_CTRL + REG_CLR);
+
 	if (bus_flags & DRM_BUS_FLAG_DE_LOW)
-		ctrl |= CTRL_INV_DE;
-	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
-		ctrl |= CTRL_INV_PXCK;
+		writel(CTRL_INV_DE, lcdif->base + LCDC_V8_CTRL + REG_SET);
+	else
+		writel(CTRL_INV_DE, lcdif->base + LCDC_V8_CTRL + REG_CLR);
 
-	writel(ctrl, lcdif->base + LCDC_V8_CTRL);
+	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
+		writel(CTRL_INV_PXCK, lcdif->base + LCDC_V8_CTRL + REG_SET);
+	else
+		writel(CTRL_INV_PXCK, lcdif->base + LCDC_V8_CTRL + REG_CLR);
 
 	writel(DISP_SIZE_DELTA_Y(m->vdisplay) |
 	       DISP_SIZE_DELTA_X(m->hdisplay),
-- 
2.53.0

[PATCH 3/3] drm: lcdif: Wait for vblank before disabling DMA

[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 <paulk@sys-base.io>
Co-developed-by: Lucas Stach <l.stach@pengutronix.de>
---
 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");
 
+	/*
+	 * 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);
+
+	ret = readl_poll_timeout(lcdif->base + LCDC_V8_CTRLDESCL0_5,
+				 reg, !(reg & CTRLDESCL0_5_SHADOW_LOAD_EN),
+				 0, 36000);	/* Wait ~2 frame times max */
+	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);
-- 
2.53.0

Claude review: drm: lcdif: FIFO underrun/solid color bug fix

[Claude Code Review Bot]

Overall Series Review

Subject: drm: lcdif: FIFO underrun/solid color bug fix
Author: Paul Kocialkowski <paulk@sys-base.io>
Patches: 4
Reviewed: 2026-03-31T16:48:35.199574

---

A reasonable 3-patch series fixing a sporadic FIFO underrun / solid-color display bug on i.MX8MP LCDIF. The core fix is in patch 3; patches 1 and 2 are ancillary hardening changes ported from the NXP BSP. The series is small and well-scoped. There are a few issues to address, the most important being a malformed `Co-developed-by` tag and a duplicated error message that will hamper debugging.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm: lcdif: Set undocumented bit to clear FIFO at vsync

[Claude Code Review Bot]

Patch Review

**Looks reasonable.** BIT(23) sits directly above `P_SIZE` at GENMASK(22,20), so there's no bit overlap.

Minor nit: the commit message says "it doesn't hurt to add it as an extra safety measure," which is a weak justification for setting an undocumented bit in a register. It would be better to reference the specific NXP BSP commit or file where this bit is used, so future readers can cross-reference. For an undocumented bit, any available context on what it actually does would strengthen the patch.

No functional issues.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm: lcdif: Use dedicated set/clr registers for polarity/edge

[Claude Code Review Bot]

Patch Review

**Improvement over the original code, but the commit message undersells it.**

The old code:
```c
writel(ctrl, lcdif->base + LCDC_V8_CTRL);
```
This was a full register write with only polarity bits set, which would **clobber** any other bits previously configured in `LCDC_V8_CTRL` (e.g., `CTRL_NEG`, `CTRL_FETCH_START_OPTION_*`). Using `REG_SET`/`REG_CLR` at offsets +4/+8 is strictly safer because it modifies only the targeted bits via atomic set/clear.

The commit message says "It is unclear if there is a difference" — but there **is** a clear difference: the old code was a plain write that could zero other CTRL bits, while `REG_SET`/`REG_CLR` are read-modify-write at the hardware level. The commit message should mention this as the primary motivation rather than presenting it as cargo-culting the BSP.

No functional issues with the code itself. The `else` branches correctly clear the bits when the condition is false, which the old code did implicitly (by not OR-ing them into a zero-initialized variable and then doing a full write).

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm: lcdif: Wait for vblank before disabling DMA

[Claude Code Review Bot]

Patch Review

This is the actual bug fix. Two issues:

**1. Malformed `Co-developed-by` tag (must fix):**

The trailer has:
```
Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
Co-developed-by: Lucas Stach <l.stach@pengutronix.de>
```

Per `Documentation/process/submitting-patches.rst`, every `Co-developed-by` **must** be immediately followed by a `Signed-off-by` from that same person. The correct form would be:

```
Co-developed-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
```

(The patch author's `Signed-off-by` should generally come last.)

**2. Duplicate error message (should fix):**

```c
if (ret)
    drm_err(lcdif->drm, "Failed to disable controller!\n");
```

This exact same string appears for both the existing `CTRLDESCL0_5_EN` poll timeout and the new `CTRLDESCL0_5_SHADOW_LOAD_EN` poll timeout. If either fails, the log gives no indication of *which* step timed out. The new message should be distinct, e.g., `"Failed to wait for shadow load completion!\n"` or `"Timed out waiting for DMA frame completion!\n"`.

**3. Approach observation (minor):**

The technique of setting `SHADOW_LOAD_EN` and polling for hardware to clear it as a proxy for "frame DMA is complete" is clever. The comment in the code explains the rationale well. The 36000 µs timeout (~2 frames at 60 Hz) matches the existing timeout above and is reasonable.

One subtle question: after the first poll confirms `CTRLDESCL0_5_EN` is cleared, is the hardware guaranteed to still process a `SHADOW_LOAD_EN` request? If `EN` being cleared means the layer is fully off, the shadow load might never be consumed. This deserves verification against the hardware reference or NXP BSP behavior — though the fact that the NXP BSP uses a similar (if cruder) approach with a 25 ms sleep suggests the hardware does honor it.

---
Generated by Claude Code Patch Reviewer