Hi Laurent,
On Fri, Nov 21, 2025 at 3:42 AM Laurent Pinchart
<[email protected]> 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...)
> > - /* 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.
> 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.
Yours,
Linus Walleij
