On Fri, Nov 21, 2025 at 09:17:26AM +0100, Linus Walleij wrote:
> On Fri, Nov 22, 2025 at 3:42 AM Laurent Pinchart wrote:
>
> > > This is needed on R-Car DU, where the CRTC provides clock to LVDS
> > > and DSI, and has to be started before a bridge may call .prepare,
> > > which may trigger e.g. a DSI transfer.
> >
> > Is there an issue with LVDS ? The LVDS encoder receivers its pixel clock
> > from the CRTC the same way any encoder does (except on R-Car D3 and E3
> > where the encoder *provides* the pixel clock to the CRTC, which is
> > handled through explicit function calls from the CRTC to the LVDS
> > encoder). There's no command mode with LVDS. Is the concern that we may
> > have an external LVDS to DSI bridge ?
>
> Question to Marek, this commit text is from his original patch (which
> I modified heavily so almost only the commit message is left...)
Marek, could you comment on this ?
> > > - /* Apply the atomic update. */
> > > - drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > > + /*
> > > + * Apply the atomic update.
> > > + *
> > > + * We need special ordering to make sure the CRTC disabled last
> > > + * and enabled first. We do this with modified versions of the
> > > + * common modeset_disables/enables functions.
> > > + */
> > > +
> > > + /* Variant of drm_atomic_helper_commit_modeset_disables() */
> > > + drm_encoder_bridge_disable(dev, state);
> > > + drm_encoder_bridge_post_disable(dev, state);
> > > + drm_crtc_disable(dev, state);
> >
> > I think we have a fundamental issue here. Commit c9b1150a68d9
> > ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
> > states that
> >
> > The definition of bridge pre_enable hook says that,
> > "The display pipe (i.e. clocks and timing signals) feeding this bridge
> > will not yet be running when this callback is called".
> >
> > This is right, and the above sequence does not comply with the
> > documentation, which is a concern. Quite clearly the bridge API isn't up
> > to the task here. I don't know how we'll fix it, the pre/post
> > enable/disable operations are really a hack and don't scale, and fixing
> > that will likely not be a simple task.
>
> Well in the v1 patch I tried to go with this definition, if:
>
> 1. The display pipe is not running and
> 2. The hardware is such that DSI will not work unless the display
> pipe is running then it follows logically that:
>
> 3. We cannot send DSI commands in bridge prepare()/unprepare()
> execution paths.
>
> Thus the v1 patch moves all DSI commands to the enable/disable
> callbacks. It fixes the regression, too.
>
> We would need to comb over the existing DSI bridges and panels
> to convert them to this semantic if we wanna be strict, what I
> did was to just fix all panels used by this one hardware. I'm pretty
> sure the same can be done of any R-Car DU-related panel/bridge.
I just noticed a similar issue was reported for the Rockchip display
drivers, see [1]. That's three platforms broken by commit
c9b1150a68d9362a0827609fc0dc1664c0d8bfe1 and there may be more we
haven't noticed yet. I think we should revert the commit and work on a
proper solution on top.
[1]
https://lore.kernel.org/r/caamcf8di8sc_xvzanzq9suiuf-ayvg2yjhx2dwmvvcnff3p...@mail.gmail.com
> > The short term question is how to deal with the regression that
> > c9b1150a68d9 caused in the MCDE and R-Car DU drivers. This patch
> > probably works. The complexity makes me worry that we'll introduce other
> > regressions, but it can be argued that we're merely restoring the
> > previous order of operations, which should therefore be safe. I'm still
> > concerned about maintainability though. Commit c9b1150a68d9 should
> > probably have been rejected, we should have developed a proper solution
> > instead :-(
>
> Yeah, it's a bit of a mess when regressions get detected late.
> I'm also worried about more regressions popping up. They will
> all be with DSI panels/bridges I think.
--
Regards,
Laurent Pinchart
