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 <[email protected]>
>> > Signed-off-by: Liu Ying <[email protected]>
>>
>> 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
