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

[Dave Stevenson]

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.

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?

  Dave