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

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

Hello Liu,

On Mon Jun 22, 2026 at 9:52 AM CEST, Liu Ying wrote:
> 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.

I see, OK, thanks for the clarification!

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

I had missed this is called from drm_bridge_chain_mode_valid(), which
already takes the bridge_chain_mutex thanks to
drm_for_each_bridge_in_chain_from().

So that means this loop is safe, but it would be nice to add a comment to
make it clear to future readers. E.g. "/* we are called by
drm_bridge_chain_mode_valid(), so the bridge_chain_mutex is locked */".

Still I'm not a fan of having a loop over the bridge chain (this function)
inside another loop over the same bridge chain (in
drm_bridge_chain_mode_valid(). And even more I'm not a fan of drivers
walking around the bridge chain on their own, IMO the core (perhaps
drm_bridge.c in this case) should to the loops . However this is a general
concern, it happens also elsewhere, and I have no immediate proposal to
improve this, so don't consider it as a blocker for this patch.

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

As I said I'm quite ignorance about this aspect, so I asked to better
understand. In reply to another part fo the thread: no, I don't have a
specific concern if this is the common practice.

Luca

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