Hi, On 04/11/2025 16:36, Linus Walleij wrote: > On Tue, Oct 28, 2025 at 4:13 PM Tomi Valkeinen > <[email protected]> wrote: > >>> I don't know what that's for, the panel bridge isn't >>> using it so it doesn't help any panel driver? >> >> What do you mean? drivers/gpu/drm/bridge/panel.c uses it, see >> "panel->prepare_prev_first". I do find it confusing, and I'm not sure if >> I should say more about it until I read the code to refresh my memory =) > > OK yes it is there, I did a misgrep... > > panel_bridge->bridge.pre_enable_prev_first = panel->prepare_prev_first; > >> But I think the idea is that the panel's prepare is called after the DSI >> hosts pre_enable. Probably that doesn't help here, as you need the crtc >> to be active, and, afaik, the point with the whole pre-enable/prepare >> sequence is that the video is not active at that point: >> >> * The display pipe (i.e. clocks and timing signals) feeding this bridge >> * will not yet be running when the @atomic_pre_enable is called. > > Indeed. > > So what is needed is for .enable on the preceding bridge to have > been called first.
Marek sent https://lore.kernel.org/all/20251107230517.471894-1-marek.vasut%2Brenesas%40mailbox.org/ Linus, I don't know if we want to go there or not, but could you see if that solves the problem on mcde? >>> FWIW I think it is fine to require the DSI panels to only send >>> DSI commands in .enable/.disable and not in .prepare/.unprepare, >>> it's intuitive in a way, if we go for this semantic I can send a doc >>> update after the fixes and try to look over the panel drivers a bit >>> and see if there are more offenders. >> >> No, I don't think that works generally. In panel's enable() the video >> stream is already on, and the DSI link is in HS mode. You might need to >> configure the panel first (at least the first part of the setup) in LP >> mode. Say, to set the number of lanes. Or, see >> "[email protected]", tc358768 can >> send long DSI commands, but only if the video stream is not enabled. >> >> Conceptually it makes sense to do configuration in panel's prepare(), >> and do the final enablement in panel's enable(). But unfortunately I >> don't have any good ideas how to sort this out. Feels like whatever we >> do, it's not ok for some bridge/panel... > > So what shall we do with this regression? > > Shall we just revert > commit c9b1150a68d9362a0827609fc0dc1664c0d8bfe1 > "drm/atomic-helper: Re-order bridge chain pre-enable and post-disable" > while we still can? That's definitely an option, but it's not without pain either. We'll get broken platforms, and need to revert a bunch of stuff. If Marek's patch helps, we could go with it, or we could also try to apply it the other way around: revert the order back for, but add a new drm_atomic_helper_commit_modeset_enables variant, where the crtc is enabled after bridge pre_enables. I think bridge pre_enable before crtc enable makes a lot of sense, though. And that the documentation described even before Aradhya's patch: "The display pipe (i.e. clocks and timing signals) feeding this bridge will not yet be running when this callback is called." So far, afaik, we've only been hitting issues with DSI and DSI commands, and the issue is critical only when the DSI bridge depends on the incoming pixel clock for operation. The issues have been on platforms where the SoC has a crtc and a dsi, but I don't see why the same couldn't happen with external bridges too. So, we could (and probably should) fix this by making the crtc enabled early. But it feels like a work-around, for this specific case. But I don't really have a idea for a generic solution, there can be so many variations where component X requires an incoming video stream to do anything but then component Y can't cope with incoming video stream until it has done some setup, etc... Tomi
