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

[Luca Ceresoli]

Hello Liu,

On Fri May 15, 2026 at 8:54 AM CEST, Liu Ying wrote:
> 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@xxxxxxx>
> Signed-off-by: Liu Ying <victor.liu@xxxxxxx>

I'm perhaps not the most qualified to review this change, but let me try.

> --- 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;
>  		}
>  	}

Is this logic specific to the imx93 MIPI DSI host only? Or should it be
made generic for all dw-hdmi users, or even every DSI host?

Also, iterating over the bridge chain is not very clean. I'm working on
bridge hotplug (not upstream yet) and bad things would happen if a bridge
were hot-unplugged during this loop. If the core did this sort of algorithm
it would be able to be more robust.

Finally, out of my utter ignorance on the subject, is the VESA +/-0.5%
margin generic enough that this driver can always rely on it?

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com