[PATCH 0/2] DSI host and peripheral initialisation ordering

[Doug Anderson]

Hi,

On Wed, Mar 2, 2022 at 9:20 AM Dave Stevenson
<dave.stevenson at raspberrypi.com> wrote:
>
> Hi Doug
>
> On Wed, 2 Mar 2022 at 00:13, Doug Anderson <dianders at chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Feb 16, 2022 at 9:00 AM Dave Stevenson
> > <dave.stevenson at raspberrypi.com> 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