Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dave Stevenson <dave.stevenson@raspberrypi.com>]

On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi All
>
> A gentle ping on this series. Any comments on the approach?
> Thanks.

I realise the merge window has just closed and therefore folks have
been busy, but no responses on this after a month?

Do I give up and submit a patch to document that DSI is broken and no one cares?

  Dave

> > Changes from v1:
> > - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
> >   to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
> >   but with a NULL state.
> > - New patch that adds a pre_enable_upstream_first to drm_panel.
> > - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
> > - Followed Andrzej's suggestion of using continue in the main loop to avoid
> >   needing 2 additional loops (one forward to find the last bridge wanting
> >   upstream first, and the second backwards again).
> > - Actioned Laurent's review comments on docs patch.
> >
> > Original cover letter:
> >
> > 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].
> >
> > This patch takes a hybrid of the two: an optional reversing of the order for
> > specific links within the bridge chain within pre_enable and post_disable done
> > within the drm_bridge framework.
> > I'm more than happy to move where the flag exists in structures (currently as
> > DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op), but does
> > this solve the problem posed? If not, then can you describe the actual scenario
> > it doesn't cover?
> > A DSI peripheral can set the flag to get the DSI host initialised first, and
> > therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral
> > can still send shutdown commands prior to the DSI host being shut down in
> > post_disable. It also handles the case where there are multiple devices in the
> > chain that all want their upstream bridge enabled first, so should there be a
> > DSI mux between host and peripheral, then it can still get the host to the
> > correct state.
> >
> > An example tree is at [7] which is drm-misc-next with these patches and then a
> > conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed
> > once we're over this hurdle). It is working happily with the Toshiba TC358762 on
> > a Raspberry Pi 7" panel.
> > The same approach but on our vendor 5.15 tree[8] has also been tested
> > successfully on a TI SN65DSI83 and LVDS panel.
> >
> > Whilst here, I've also documented the expected behaviour of DSI hosts and
> > peripherals to aid those who come along after.
> >
> > Thanks
> >   Dave
> >
> > [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L940
> > [2] https://lists.freedesktop.org/archives/dri-devel/2022-January/337769.html
> > [3] https://lists.freedesktop.org/archives/dri-devel/2021-December/333908.html
> > [4] https://lists.freedesktop.org/archives/dri-devel/2021-October/328476.html
> > [5] https://lists.freedesktop.org/archives/dri-devel/2021-October/325853.html
> > [6] https://lists.freedesktop.org/archives/dri-devel/2022-February/341852.html
> > [7] https://github.com/6by9/linux/tree/drm-misc-next-vc4_dsi
> > [8] https://github.com/6by9/linux/tree/rpi-5.15.y-sn65dsi83
> >
> > Dave Stevenson (4):
> >   drm/bridge: Remove duplication from drm_bridge and drm_atomic_bridge
> >     chains
> >   drm/bridge: Introduce pre_enable_upstream_first to alter bridge init
> >     order
> >   drm/panel: Add prepare_upstream_first flag to drm_panel
> >   drm/bridge: Document the expected behaviour of DSI host controllers
> >
> >  Documentation/gpu/drm-kms-helpers.rst |   7 ++
> >  drivers/gpu/drm/bridge/panel.c        |   3 +
> >  drivers/gpu/drm/drm_bridge.c          | 181 ++++++++++++++++++++++++----------
> >  include/drm/drm_bridge.h              |   8 ++
> >  include/drm/drm_panel.h               |  10 ++
> >  5 files changed, 159 insertions(+), 50 deletions(-)
> >
> > --
> > 2.7.4
> >