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

[Liu Ying <victor.liu@nxp.com>]

On Fri, Jun 19, 2026 at 06:49:55PM +0200, Luca Ceresoli wrote:
> Hello Liu,

Hi Luca,

> 
> 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@nxp.com>
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> 
> I'm perhaps not the most qualified to review this change, but let me try.

Thanks for your review.

> 
> > --- 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?

I think it's kind of specific to the i.MX93 MIPI DSI host only, because
1) the i.MX93 MIPI DPHY PLL(integrated into i.MX93 MIPI DPHY IP) supports
the best DPHY PLL frequency @445.333333MHz for the typical 1920x1080p@60Hz
display mode, which is lower than the expected/nominal frequency @445.5MHz
and 2) the generic DW MIPI DSI driver(dw-mipi-dsi.c) is PHY-agnostic, which
means vendors may attach different MIPI DPHY IPs to the common MIPI DSI host
IP and likely there would be no frequency mismatch between the pixel clock
and the PLL like i.MX93 has.

> 
> 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.

The iterating is essentially the same to drm_for_each_bridge_in_chain_from()
except that bridge_chain_mutex is not taken(since it's already taken) and
the starting bridge is fixed to be the next bridge.  A few bridge core APIs
like drm_bridge_chain_mode_valid() call drm_for_each_bridge_in_chain_from().
So, the iterating looks clean to me and I'm not aware of any bad things which
would happen when bridge hotplug is considered.

> If the core did this sort of algorithm
> it would be able to be more robust.

Is the core dw-mipi-dsi.c?
If yes, do you think it's worth doing that even if the frequency mismatch
between the pixel clock and the PLL is kind of specific to the i.MX93 MIPI
DSI host?

> 
> 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?

I see several upstream drivers rely on it, see "git grep '0.5%' drivers/gpu/"
output.  And every display mode allows -/+ 0.5% pixel clock rate deviation
according to VESA Display Monitor Timing Standard [1], though [1] is a found
by a random Google search.

[1] https://glenwing.github.io/docs/VESA-DMT-1.13.pdf

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

-- 
Regards,
Liu Ying