From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/imx/ipuv3: do not adjust hactive for non-8-pixel-aligned modes Date: Sun, 12 Apr 2026 14:28:31 +1000 Message-ID: In-Reply-To: <20260406153654.2094908-1-magnus@alefors.se> References: <20260406153654.2094908-1-magnus@alefors.se> <20260406153654.2094908-1-magnus@alefors.se> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Positive aspects:** 1. The commit message is clear about the problem (visible skewing on 854x48= 0 displays) and the rationale (IDMAC burst sizing is handled by `ipu_calcul= ate_bursts()`). 2. The removal of the CRTC-side alignment in `ipuv3-crtc.c` is clean =E2=80= =94 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` genuine= ly does handle arbitrary widths =E2=80=94 it iterates to find burst sizes t= hat 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 `ipu= v3-plane.c:686`. With the 8-pixel alignment removed, this will now receive = unaligned widths. The calculation still works mathematically (integer divis= ion), but the FIFO threshold decision changes slightly =E2=80=94 worth conf= irming this doesn't cause FIFO overflow issues on the specific display conf= igurations 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 me= ssage says "The IDMAC read width must match the DC/DI hactive exactly" =E2= =80=94 this was previously guaranteed by both sides aligning to 8 pixels. A= fter the patch, both use raw values, so they should still match for fullscr= een scanout, which is correct. 3. **The comment added in `ipu_src_rect_width()` references DMFC FIFO overf= lows/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 slight= ly misleading =E2=80=94 it implies that alignment *caused* DMFC FIFO issues= , when the real problem is the mismatch between IDMAC and DC widths. Consid= er 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 wi= dth must match it exactly to avoid DMFC FIFO overflows or underflows." 4. **Missing Fixes tag.** If this fixes a real user-visible display corrupt= ion on 854x480 panels, it should carry a `Fixes:` tag pointing to the commi= t that introduced the alignment. This would help with backporting to stable= kernels. 5. **No `mode_fixup` / `atomic_check` validation.** The old code in `mode_s= et_nofb` blindly subtracted from `hfront_porch` without checking for underf= low =E2=80=94 that was itself a bug (if `hfront_porch < 6` for an 854-pixel= mode, it would underflow). The patch removes this dangerous arithmetic, wh= ich is good. But there is no new validation in `mode_fixup` or `atomic_chec= k` to reject modes that might be problematic for other reasons at non-align= ed 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 `wi= dth` 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 =E2=80=94 just noting this is a nice touch. **Overall assessment:** The patch is well-motivated and technically sound. = The key insight =E2=80=94 that `ipu_calculate_bursts()` handles arbitrary w= idths and that the foreground plane already works without alignment =E2=80= =94 provides good evidence that this is safe. I'd like to see a `Fixes:` ta= g and a slightly clearer comment, but neither is a blocking issue. The patc= h should be tested on both 8-pixel-aligned displays (regression) and non-al= igned displays (854x480) to confirm no DMFC FIFO issues arise. --- Generated by Claude Code Patch Reviewer