[PATCH] drm/imx/ipuv3: do not adjust hactive for non-8-pixel-aligned modes

[PATCH] drm/imx/ipuv3: do not adjust hactive for non-8-pixel-aligned modes

[Magnus Hörlin]

The IPUv3 display driver rounds up hactive to 8-pixel alignment in
mode_set_nofb(), stealing pixels from the front porch.
ipu_src_rect_width() applies the same rounding to the IDMAC read
width. While this keeps the DC and IDMAC in sync, it causes the
display to receive more active pixels than the mode specifies.

On fixed-pixel displays whose native width is not 8-pixel aligned
(e.g. 854x480), the extra pixels cause visible skewing: each
scanline is offset, making vertical lines lean.

Remove both alignment overrides. The IDMAC and DC now use the exact
mode width. The DMA burst size is already adapted to arbitrary
widths by ipu_calculate_bursts(), so no alignment is required at
the pixel level.

Demote the DC warning about non-8-aligned hactive to dev_dbg, since
it is informational rather than indicative of a problem.

Signed-off-by: Magnus Hörlin <magnus@alefors.se>
---
 drivers/gpu/drm/imx/ipuv3/ipuv3-crtc.c  | 9 ---------
 drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c | 9 ++++++++-
 drivers/gpu/ipu-v3/ipu-dc.c             | 5 +++--
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3/ipuv3-crtc.c
index a18b9d1a6..47b88f29a 100644
--- a/drivers/gpu/drm/imx/ipuv3/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3/ipuv3-crtc.c
@@ -304,15 +304,6 @@ static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	sig_cfg.vsync_pin = imx_crtc_state->di_vsync_pin;
 
 	drm_display_mode_to_videomode(mode, &sig_cfg.mode);
-	if (!IS_ALIGNED(sig_cfg.mode.hactive, 8)) {
-		unsigned int new_hactive = ALIGN(sig_cfg.mode.hactive, 8);
-
-		dev_warn(ipu_crtc->dev, "8-pixel align hactive %d -> %d\n",
-			 sig_cfg.mode.hactive, new_hactive);
-
-		sig_cfg.mode.hfront_porch -= new_hactive - sig_cfg.mode.hactive;
-		sig_cfg.mode.hactive = new_hactive;
-	}
 
 	ipu_dc_init_sync(ipu_crtc->dc, ipu_crtc->di,
 			 mode->flags & DRM_MODE_FLAG_INTERLACE,
diff --git a/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c
index c1c7be4e2..58e7853e6 100644
--- a/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c
@@ -34,7 +34,14 @@ to_ipu_plane_state(struct drm_plane_state *p)
 
 static unsigned int ipu_src_rect_width(const struct drm_plane_state *state)
 {
-	return ALIGN(drm_rect_width(&state->src) >> 16, 8);
+	/*
+	 * Do not align width to 8 pixels here. The IDMAC read width
+	 * must match the DC/DI hactive exactly, otherwise the DMFC
+	 * FIFO overflows or underflows causing IDMAC timeouts or
+	 * display corruption. The DMA burst size is adapted to the
+	 * actual width by ipu_calculate_bursts().
+	 */
+	return drm_rect_width(&state->src) >> 16;
 }
 
 static inline struct ipu_plane *to_ipu_plane(struct drm_plane *p)
diff --git a/drivers/gpu/ipu-v3/ipu-dc.c b/drivers/gpu/ipu-v3/ipu-dc.c
index b038a6d73..f5990e3dc 100644
--- a/drivers/gpu/ipu-v3/ipu-dc.c
+++ b/drivers/gpu/ipu-v3/ipu-dc.c
@@ -168,8 +168,9 @@ int ipu_dc_init_sync(struct ipu_dc *dc, struct ipu_di *di, bool interlaced,
 	dc->di = ipu_di_get_num(di);
 
 	if (!IS_ALIGNED(width, 8)) {
-		dev_warn(priv->dev,
-			 "%s: hactive does not align to 8 byte\n", __func__);
+		dev_dbg(priv->dev,
+			"%s: hactive %u is not 8-pixel aligned\n",
+			__func__, width);
 	}
 
 	map = ipu_bus_format_to_map(bus_format);
-- 
2.39.5

Re: [PATCH] drm/imx/ipuv3: do not adjust hactive for non-8-pixel-aligned modes

[Philipp Zabel]

On Mo, 2026-04-06 at 17:36 +0200, Magnus Hörlin wrote:
> The IPUv3 display driver rounds up hactive to 8-pixel alignment in
> mode_set_nofb(), stealing pixels from the front porch.
> ipu_src_rect_width() applies the same rounding to the IDMAC read
> width. While this keeps the DC and IDMAC in sync, it causes the
> display to receive more active pixels than the mode specifies.
> 
> On fixed-pixel displays whose native width is not 8-pixel aligned
> (e.g. 854x480), the extra pixels cause visible skewing: each
> scanline is offset, making vertical lines lean.
> 
> Remove both alignment overrides. The IDMAC and DC now use the exact
> mode width.

This partially reverts 94dfec48fca7 ("drm/imx: Add 8 pixel alignment
fix") [1] without mentioning it, so I assume this patch breaks imx6dl-
b105pv2.

[1] https://lore.kernel.org/all/20210428222953.235280-6-sebastian.reichel@collabora.com/

> The DMA burst size is already adapted to arbitrary
> widths by ipu_calculate_bursts(), so no alignment is required at
> the pixel level.

The reference manual mentions burst size (NPB) has to be a multiple of
8 pixels at least for planar and semi-planar YUV formats. Without the
width alignment, ipu_calculate_bursts() doesn't guarantee this.

> Demote the DC warning about non-8-aligned hactive to dev_dbg, since
> it is informational rather than indicative of a problem.
> 
> Signed-off-by: Magnus Hörlin <magnus@alefors.se>

I wonder why the 8-pixel alignment check in ipu_di_adjust_videomode()
which has not been removed, doesn't cause ipu_crtc_mode_fixup() to
fail.

regards
Philipp

Claude review: drm/imx/ipuv3: do not adjust hactive for non-8-pixel-aligned modes

[Claude Code Review Bot]

Overall Series Review

Subject: drm/imx/ipuv3: do not adjust hactive for non-8-pixel-aligned modes
Author: =?UTF-8?q?Magnus=20H=C3=B6rlin?= <magnus@alefors.se>
Patches: 2
Reviewed: 2026-04-12T14:28:30.850076

---

This is a single patch that removes the 8-pixel alignment rounding of `hactive` in the IPUv3 display driver, motivated by fixing visible skewing on displays whose native width is not 8-pixel aligned (e.g. 854x480). The approach is reasonable and the commit message is well-written with clear motivation.

The core argument is sound: `ipu_calculate_bursts()` already handles arbitrary widths mathematically, and the foreground plane (`SYNC_FG`) path already uses unaligned widths without issue. However, I have a few concerns about completeness and potential side effects.

**Verdict:** The change looks correct in principle but would benefit from addressing a couple of points before merging.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imx/ipuv3: do not adjust hactive for non-8-pixel-aligned modes

[Claude Code Review Bot]

Patch Review

**Positive aspects:**

1. The commit message is clear about the problem (visible skewing on 854x480 displays) and the rationale (IDMAC burst sizing is handled by `ipu_calculate_bursts()`).

2. The removal of the CRTC-side alignment in `ipuv3-crtc.c` is clean — it was stealing pixels from the front porch, which is problematic when the front porch is small.

3. The `ipu_calculate_bursts()` function at `ipuv3-plane.c:553-571` genuinely does handle arbitrary widths — it iterates to find burst sizes that fit within the stride, with no alignment assumptions.

4. Demoting the DC warning to `dev_dbg` is appropriate if this is no longer a problematic condition.

**Concerns:**

1. **DMFC wait4eot calculation with unaligned width.** The `ipu_dmfc_config_wait4eot()` function at `ipu-dmfc.c:135` uses `width` in a division:

   ```c
   if ((dmfc->slots * 64 * 4) / width > dmfc->data->max_fifo_lines)
   ```

   This is called with the same `width` from `ipu_src_rect_width()` at `ipuv3-plane.c:686`. With the 8-pixel alignment removed, this will now receive unaligned widths. The calculation still works mathematically (integer division), but the FIFO threshold decision changes slightly — worth confirming this doesn't cause FIFO overflow issues on the specific display configurations being targeted.

2. **Asymmetry between CRTC and plane width paths.** After this patch, the CRTC's `ipu_dc_init_sync()` will receive the raw (unaligned) `hactive` from `drm_display_mode_to_videomode()`, and the plane's IDMAC will also use the raw width. This is consistent, which is good. However, I want to confirm: is there any path where the plane `width` (from `ipu_src_rect_width`) could differ from the CRTC `hactive` passed to `ipu_dc_init_sync`? The commit message says "The IDMAC read width must match the DC/DI hactive exactly" — this was previously guaranteed by both sides aligning to 8 pixels. After the patch, both use raw values, so they should still match for fullscreen scanout, which is correct.

3. **The comment added in `ipu_src_rect_width()` references DMFC FIFO overflows/underflows:**

   ```c
   /*
    * Do not align width to 8 pixels here. The IDMAC read width
    * must match the DC/DI hactive exactly, otherwise the DMFC
    * FIFO overflows or underflows causing IDMAC timeouts or
    * display corruption. The DMA burst size is adapted to the
    * actual width by ipu_calculate_bursts().
    */
   ```

   This comment explains why the alignment is *removed* but could be slightly misleading — it implies that alignment *caused* DMFC FIFO issues, when the real problem is the mismatch between IDMAC and DC widths. Consider rewording to something like: "Use the exact source width without 8-pixel alignment. The DC/DI hactive is no longer rounded up, so the IDMAC read width must match it exactly to avoid DMFC FIFO overflows or underflows."

4. **Missing Fixes tag.** If this fixes a real user-visible display corruption on 854x480 panels, it should carry a `Fixes:` tag pointing to the commit that introduced the alignment. This would help with backporting to stable kernels.

5. **No `mode_fixup` / `atomic_check` validation.** The old code in `mode_set_nofb` blindly subtracted from `hfront_porch` without checking for underflow — that was itself a bug (if `hfront_porch < 6` for an 854-pixel mode, it would underflow). The patch removes this dangerous arithmetic, which is good. But there is no new validation in `mode_fixup` or `atomic_check` to reject modes that might be problematic for other reasons at non-aligned widths. This may be fine if the hardware truly handles any width, but it would be prudent to confirm on hardware.

6. **Minor style nit in ipu-dc.c:** The warning message change adds the `width` value, which is a good improvement for debuggability:

   ```c
   dev_dbg(priv->dev,
           "%s: hactive %u is not 8-pixel aligned\n",
           __func__, width);
   ```

   No issue here — just noting this is a nice touch.

**Overall assessment:** The patch is well-motivated and technically sound. The key insight — that `ipu_calculate_bursts()` handles arbitrary widths and that the foreground plane already works without alignment — provides good evidence that this is safe. I'd like to see a `Fixes:` tag and a slightly clearer comment, but neither is a blocking issue. The patch should be tested on both 8-pixel-aligned displays (regression) and non-aligned displays (854x480) to confirm no DMFC FIFO issues arise.

---
Generated by Claude Code Patch Reviewer