Hi, On Wed, Mar 2, 2022 at 9:20 AM Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> wrote: > > Hi Doug > > On Wed, 2 Mar 2022 at 00:13, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > > > Hi, > > > > On Wed, Feb 16, 2022 at 9:00 AM Dave Stevenson > > <dave.stevenson@xxxxxxxxxxxxxxx> wrote: > > > > > > Hi All > > > > > > Hopefully I've cc'ed all those that have bashed this problem around previously, > > > or are otherwise linked to DRM bridges. > > > > > > There have been numerous discussions around how DSI support is currently broken > > > as it doesn't support initialising the PHY to LP-11 and potentially the clock > > > lane to HS prior to configuring the DSI peripheral. There is no op where the > > > interface is initialised but HS video isn't also being sent. > > > Currently you have: > > > - peripheral pre_enable (host not initialised yet) > > > - host pre_enable > > > - encoder enable > > > - host enable > > > - peripheral enable (video already running) > > > > > > vc4 and exynos currently implement the DSI host as an encoder, and split the > > > bridge_chain. This fails if you want to switch to being a bridge and/or use > > > atomic calls as the state of all the elements split off are not added by > > > drm_atomic_add_encoder_bridges. > > > > > > dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so > > > the bridge/panel pre_enable can send commands. In their post_disable they then > > > call the downstream bridge/panel post_disable op manually so that shutdown > > > commands can be sent before shutting down the PHY. Nothing handles that fact, > > > so the framework then continues down the bridge chain and calls the post_disable > > > again, so we get unbalanced panel prepare/unprepare calls being reported [3]. > > > > > > There have been patches[4] proposing reversing the entire direction of > > > pre_enable and post_disable, but that risks driving voltage into devices that > > > have yet to be powered up. > > > There have been discussions about adding either a pre_pre_enable, or adding a > > > DSI host_op to initialise the host[5]. Both require significant reworking to all > > > existing drivers in moving initialisation phases. > > > We have patches that look like they may well be addressing race conditions in > > > starting up a DSI peripheral[6]. > > > > In general I'm happy to let the more senior people in DRM set the > > direction here so I probably won't do lots of review, but I will point > > out that I did have another proposal that sorta got lost in the noise > > of the whole "reversing the entire direction". That's basically: > > > > https://lists.freedesktop.org/archives/dri-devel/2021-October/328934.html > > > > I have no idea if something like that would work for your use case, > > but after analyzing it it felt like a surprisingly clean proposal even > > if my first instinct when I thought about it was that it was a hack. > > ;-) I suspect (but haven't analyzed your code) that it might be > > equivalent to your proposal of using a flag but maybe easier to wrap > > ones head around? > > If I'm reading that right, then you're proposing adding > after_pre_enable and before_post_disable hooks. > That's almost the same as the power_up() and power_down() ops that > Dmitry suggested earlier, or pre_pre_enable / post_post_disable that > had also been considered. > > Neither of those options handles the case of a longer chain in which > two non-consecutive links want their upstream bridge enabled first. > As per the clarification in patch 1/2, considering the chain > - Panel > - Bridge 1 > - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST > - Bridge 3 > - Bridge 4 DRM_BRIDGE_OP_UPSTREAM_FIRST > - Bridge 5 > - Encoder > With the flag option we call pre_enables as Panel, Bridge 1, Bridge 3, > Bridge 2, Bridge 5, Bridge 4, Encoder. > If adding after_pre_enable, then we end up with Panel, Bridge 1, > Bridge 3, Bridge 5, Bridge 4 (after_pre_enable), Bridge 2 > (after_pre_enable), Encoder. > (power_on / pre_pre_enable from encoder to connector would end up with > Bridge 5 (power_on), Bridge 3 (power_on), Bridge 1 (power_on), Panel, > Bridge 2, Bridge 4, Encoder). > Those potentially work, but it seems a less logical order compared to > using a flag to swap only the bridges of interest. I think power_on / > pre_pre_enable covers DSI better than after_pre_enable. > > Adding the extra ops requires the source bridge (eg DSI host) to know > the behaviour the sink bridge/panel wants. So do all DSI hosts have to > implement power_on to power up and enter LP-11. Some DSI peripherals > may be quite happy or even prefer to have the bus totally idle / > powered down at pre_enable, but there would be no way of implementing > that. Ah, that makes it super clear, thanks! :-) If the local swap of just two components that you're doing is more useful to you than the two stage approach and everyone likes it then I have no objections. > You seem to be looking at DP, which I have very little knowledge of, > and I don't quite understand your comments about the AUX bus and how > ordering should be configured. If your panel isn't a generic driver, > couldn't it request that the upstream bridge is pre_enabled first? I basically ended up solving my problem in a different way, so I have no immediate need of swapping the order right now. I am happy you are tackling it, though, and I can definitely see myself needing something like this in the future. :-) -Doug
