[PATCH v3] drm/bridge: imx93-mipi-dsi: Fix mode validation

[PATCH v3] drm/bridge: imx93-mipi-dsi: Fix mode validation

[Liu Ying]

i.MX93 MIPI DPHY PLL has limitation for matching with some pixel clock
rates, e.g., the best DPHY PLL frequency is 445.333333MHz for a typical
1920x1080p@60Hz CEA/DMT display mode with a pixel clock rate running
at 148.5MHz with 4 data lanes + RGB888 pixel in MIPI DSI sync pulse mode,
while the expected PLL frequency is (148.5 * 24) / 4 / 2 MHz = 445.5MHz.
Fortunately, VESA Display Monitor Timing Standard allows +/-0.5% pixel
clock rate deviation for timings.  So, for those display modes read
from EDID through a bridge with DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_EDID
operation bit masks set, pixel clock rate could be adjusted to match
with the PLL frequency(for the above example, the pixel clock rate is
adjusted to be 148.444444MHz with about -0.03% deviation from the 148.5MHz
nominal rate so that the adjusted rate matches with the 445.333333MHz PLL
frequency).

Instead of checking the last bridge's operation bit masks against
DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_EDID to determine if allowing
+/-0.5% pixel clock rate deviation, check any bridge after this bridge,
because the last bridge is usually a display connector bridge without
any operation bit mask when the clock rate deviation is allowed.

Fixes: ce62f8ea7e3f ("drm/bridge: imx: Add i.MX93 MIPI DSI support")
Fixes: 5849eff7f067 ("drm/bridge: imx93-mipi-dsi: use drm_bridge_chain_get_last_bridge()")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
Changes in v3:
- Iterate over next bridges manually instead of calling
  drm_for_each_bridge_in_chain_from() to avoid deadlock issue.  (sashiko bot)
- Fix a typo in commit message - s/modes/mode/.
- Link to v2: https://patch.msgid.link/20260512-imx93-mipi-dsi-fix-mode-validation-v2-1-7aec3be5da2c@nxp.com

Changes in v2:
- Collect Frank's R-b tag.
- Add an explanation to commit message about the reason why mode validation
  checks bridge's operation bit masks.  (Dmitry)
- Copy Dmitry.
- Link to v1: https://lore.kernel.org/r/20260227-imx93-mipi-dsi-fix-mode-validation-v1-1-a9cd67991280@nxp.com

To: Liu Ying <victor.liu@nxp.com>
To: Andrzej Hajda <andrzej.hajda@intel.com>
To: Neil Armstrong <neil.armstrong@linaro.org>
To: Robert Foss <rfoss@kernel.org>
To: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
To: Jonas Karlman <jonas@kwiboo.se>
To: Jernej Skrabec <jernej.skrabec@gmail.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Frank Li <Frank.Li@nxp.com>
To: Sascha Hauer <s.hauer@pengutronix.de>
To: Pengutronix Kernel Team <kernel@pengutronix.de>
To: Fabio Estevam <festevam@gmail.com>
To: Luca Ceresoli <luca.ceresoli@bootlin.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Cc: dri-devel@lists.freedesktop.org
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c | 44 ++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
index 8f312f9edf97..4a84e00ae563 100644
--- a/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
@@ -489,25 +489,43 @@ static int imx93_dsi_get_phy_configure_opts(struct imx93_dsi *dsi,
 	return 0;
 }
 
+static inline struct drm_bridge *
+imx93_dsi_get_next_bridge_in_chain(struct drm_bridge *bridge)
+{
+	struct drm_bridge *next = drm_bridge_get_next_bridge(bridge);
+
+	drm_bridge_put(bridge);
+
+	return next;
+}
+
 static enum drm_mode_status
 imx93_dsi_validate_mode(struct imx93_dsi *dsi, const struct drm_display_mode *mode)
 {
 	struct drm_bridge *dmd_bridge = dw_mipi_dsi_get_bridge(dsi->dmd);
-	struct drm_bridge *last_bridge __free(drm_bridge_put) =
-		drm_bridge_chain_get_last_bridge(dmd_bridge->encoder);
+	struct drm_bridge *bridge;
 
-	if ((last_bridge->ops & DRM_BRIDGE_OP_DETECT) &&
-	    (last_bridge->ops & DRM_BRIDGE_OP_EDID)) {
-		unsigned long pixel_clock_rate = mode->clock * 1000;
-		unsigned long rounded_rate;
+	for (bridge = drm_bridge_get_next_bridge(dmd_bridge);
+	     bridge;
+	     bridge = imx93_dsi_get_next_bridge_in_chain(bridge)) {
+		if ((bridge->ops & DRM_BRIDGE_OP_DETECT) &&
+		    (bridge->ops & DRM_BRIDGE_OP_EDID)) {
+			unsigned long pixel_clock_rate = mode->clock * 1000;
+			unsigned long rounded_rate;
 
-		/* Allow +/-0.5% pixel clock rate deviation */
-		rounded_rate = clk_round_rate(dsi->clk_pixel, pixel_clock_rate);
-		if (rounded_rate < pixel_clock_rate * 995 / 1000 ||
-		    rounded_rate > pixel_clock_rate * 1005 / 1000) {
-			dev_dbg(dsi->dev, "failed to round clock for mode " DRM_MODE_FMT "\n",
-				DRM_MODE_ARG(mode));
-			return MODE_NOCLOCK;
+			/* Allow +/-0.5% pixel clock rate deviation */
+			rounded_rate = clk_round_rate(dsi->clk_pixel, pixel_clock_rate);
+			if (rounded_rate < pixel_clock_rate * 995 / 1000 ||
+			    rounded_rate > pixel_clock_rate * 1005 / 1000) {
+				dev_dbg(dsi->dev,
+					"failed to round clock for mode " DRM_MODE_FMT "\n",
+					DRM_MODE_ARG(mode));
+				drm_bridge_put(bridge);
+				return MODE_NOCLOCK;
+			}
+
+			drm_bridge_put(bridge);
+			break;
 		}
 	}
 

---
base-commit: 877552aa875839314afad7154b5a561889e87ea9
change-id: 20260227-imx93-mipi-dsi-fix-mode-validation-425c872a2493

Best regards,
--  
Regards,
Liu Ying

Claude review: drm/bridge: imx93-mipi-dsi: Fix mode validation

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: imx93-mipi-dsi: Fix mode validation
Author: Liu Ying <victor.liu@nxp.com>
Patches: 1
Reviewed: 2026-05-16T09:49:36.532814

---

This is a single-patch fix for mode validation in the i.MX93 MIPI DSI bridge driver. The problem: the old code checked only the **last** bridge in the chain for `DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID`, but when the last bridge is a connector bridge (with no ops set), the check always fails, and the +/-0.5% pixel clock deviation tolerance is never applied. The fix iterates over all bridges after the DSI bridge, looking for the first one that has both ops set.

The patch is well-reasoned and correct. The refcounting is handled properly in all paths (normal exit, early return on `MODE_NOCLOCK`, loop exhaustion). The manual iteration (instead of using `drm_for_each_bridge_in_chain_from()`) is the right approach because `bridge_chain_mutex` is already held by the caller `drm_bridge_chain_mode_valid()` — using the macro would deadlock. This was identified in v2 review and fixed in v3.

**Verdict: Looks good.** One minor observation below but nothing blocking.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: imx93-mipi-dsi: Fix mode validation

[Claude Code Review Bot]

Patch Review

**Logic change — correct:**

The key change from checking the last bridge to iterating any bridge after the DSI bridge is correct. The commit message clearly explains the scenario: the last bridge is typically a display connector bridge without ops, while an intermediate bridge (e.g., an HDMI bridge chip) is the one that sets `DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID`.

**Refcount handling — correct:**

The helper `imx93_dsi_get_next_bridge_in_chain()` correctly drops the reference on the current bridge before returning the next one:

```c
static inline struct drm_bridge *
imx93_dsi_get_next_bridge_in_chain(struct drm_bridge *bridge)
{
	struct drm_bridge *next = drm_bridge_get_next_bridge(bridge);

	drm_bridge_put(bridge);

	return next;
}
```

Both early-exit paths inside the loop body correctly call `drm_bridge_put(bridge)` before returning/breaking:

```c
			drm_bridge_put(bridge);
			return MODE_NOCLOCK;
		}

		drm_bridge_put(bridge);
		break;
```

If no bridge matches and the loop exhausts, the last iteration's `imx93_dsi_get_next_bridge_in_chain()` returns NULL after putting the last bridge — also correct.

**Locking — correctly handled:**

The call chain is: `drm_bridge_chain_mode_valid()` → `drm_for_each_bridge_in_chain_from()` (acquires `bridge_chain_mutex`) → `dw_mipi_dsi_bridge_mode_valid()` → `imx93_dsi_mode_valid()` → `imx93_dsi_validate_mode()`. Since `bridge_chain_mutex` is already held, using `drm_for_each_bridge_in_chain_from()` here would deadlock (as discovered in v2). The manual iteration via `drm_bridge_get_next_bridge()` is safe because that function doesn't take the mutex — it just traverses the list, which is protected by the caller's lock.

**Minor observation (non-blocking):**

`dw_mipi_dsi_get_bridge()` returns `&dsi->bridge` — a raw pointer to an embedded struct without incrementing the refcount. The `drm_bridge_get_next_bridge()` API contract states "The caller is responsible of having a reference to @bridge via drm_bridge_get() or equivalent." In practice this is safe because the embedded bridge outlives the call, and the pre-existing code already relied on this same pattern. But a `drm_bridge_get(dmd_bridge)` / `drm_bridge_put(dmd_bridge)` pair would be more strictly correct. This is pre-existing and not introduced by this patch, so not a concern for this fix.

**Reviewed-by: appropriate.** The patch has Frank Li's R-b tag and the fix is straightforward and correct.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: imx93-mipi-dsi: Fix mode validation

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: imx93-mipi-dsi: Fix mode validation
Author: Liu Ying <victor.liu@nxp.com>
Patches: 2
Reviewed: 2026-05-16T13:49:54.773137

---

This is a single-patch fix for the imx93 MIPI DSI bridge driver's mode validation logic. The bug is real and well-described: the original code checked only the last bridge in the chain for `DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID`, but the last bridge is typically a connector bridge without those ops set. The fix iterates over the bridge chain to find any bridge with those capabilities, which correctly enables the +/-0.5% pixel clock tolerance for EDID-sourced modes.

The patch is clean, correct, and well-explained. The use of `drm_for_each_bridge_in_chain_from()` is appropriate and replaces the manual `drm_bridge_chain_get_last_bridge()` + `__free(drm_bridge_put)` pattern with the iterator's built-in cleanup. **Reviewed-by is appropriate.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: imx93-mipi-dsi: Fix mode validation

[Claude Code Review Bot]

Patch Review

**Problem analysis:** Correct. The old code at line 496-497:
```c
struct drm_bridge *last_bridge __free(drm_bridge_put) =
    drm_bridge_chain_get_last_bridge(dmd_bridge->encoder);
```
only checked the terminal bridge. When a connector bridge (e.g., `drm_connector_bridge`) sits at the end of the chain without `DRM_BRIDGE_OP_DETECT` or `DRM_BRIDGE_OP_EDID`, the tolerance path was never entered, causing valid modes to be rejected.

**Fix analysis:** The replacement iterates from `dmd_bridge` forward:
```c
drm_for_each_bridge_in_chain_from(dmd_bridge, bridge) {
    if ((bridge->ops & DRM_BRIDGE_OP_DETECT) &&
        (bridge->ops & DRM_BRIDGE_OP_EDID)) {
        ...
        break;
    }
}
```

This is correct. The `drm_for_each_bridge_in_chain_from()` macro (defined in `drm_bridge.h:1532`) declares `bridge` with `__free(__drm_for_each_bridge_in_chain_cleanup)`, so both the `return MODE_NOCLOCK` early-exit and the normal `break` path properly unlock the chain mutex and drop the bridge reference. This replaces the explicit `__free(drm_bridge_put)` cleanup on the old `last_bridge` variable.

**Minor observation:** The iteration starts from `dmd_bridge` inclusive, so the DW MIPI DSI bridge itself is checked. It won't have the ops bits set, so this is harmless but slightly broader than "bridges after this bridge" as stated in the commit message. Not a problem.

**Nit (not blocking):** The `break` after the tolerance check exits the loop on the first matching bridge. If there were somehow multiple bridges with `DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID` in the chain, only the first one would be considered. This is the right behavior for the described topology.

No issues found. The patch is correct and ready to apply.

---
Generated by Claude Code Patch Reviewer