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

Tue Feb 22 06:43:36 UTC 2022

Hello,

On Fri, Feb 18, 2022 at 02:20:19PM +0100, Andrzej Hajda wrote:
> On 16.02.2022 17:59, Dave Stevenson 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].
> >
> > 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),

API-wise that's my only concern, the flag should go somewhere else.

> > 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.
> 
> Good summary, of multiple attempts of solving the issue (however I still 
> could add some more :) ).

Definitely good, thank you very much Dave for tackling this issue.

> I think the main issue is that we try to squeeze different hardware 
> protocol requirements into one quite restrictive framework - whole 
> crtc->encoder->bridges->(panel ||connector) is managed directly by drm core.
> No place to negotiate configuration directly between players 
> (bridges/panels).
> This patchset slightly looses the restrictions, so hopefully will help 
> for some time, but still every developer needs to solve riddles what to 
> put into callbacks, to allow driver working in different pipelines.

That's true, but documentation can help a lot there. Patch 2/2 turns the
riddle-solving task into documentation reading. Granted, not everybody
will read the documentation (and we should probably link to it from the
documentation of the pre_enable and post_disable operations), but the
behaviour is now defined, which is a major step forward.

> <DREAM MODE ON>
> Ideally I would like to drop idea of the bridge/panel and build 
> abstraction on data links.
> So for example DSI/EDP bridge during probe would register DSI sink with 
> their ops, and EDP source with their ops or just look for EDP sink (what 
> will suit better).
> To establish data link they could use their ops and helpers to provide 
> two-way conversation.
> This way if we need add support for new data link type or extend 
> existing one we do not need to touch whole framework and pray to not 
> break some strange bridge, or to add ops which will not be used by most 
> of users.
> <DREAM MODE OFF>

Protocol-specific operations can help, but I don't think they will
fundamentally change the problem. Yes, in some case, we can have
hardware requirements that are hard to express through generic
operations, but in most case the issue is more about defining the
semantic of the generic operations for a particular protocol than about
a need for a specific operation.

The core issue, in my opinion, is that we have a mechanism that
essentially works from source to sink, with the source controlling the
sink. With some protocols (DSI in particular), the start sequence
requires more fine-grained control of the operations, and the sink
should be in control. We should ideally start a pipeline by calling the
enable operation on the last element (connector or panel), whose driver
will then call operations on its source, and interleave those calls with
control of the local device, in the exact sequence required by the
device. That's how the omapdrm driver operated before I ported it to
drm_bridge. Reversing the order of the pipeline enable was a huge piece
of work for a single driver. Doing it again in the other direction for
*all* drivers seems like an even bigger dream (or nightmare) than yours
Andrzej :-)

> Putting dreams off, I think this patchset can add some value, at the 
> price of call chain complication. Lets see opinion of others.

I agree, I think it's a reasonable middleground. It improves the
situation, adds very little complexity in the API, has documentation to
specifies how the operations are meant to be implemented, and has a
reasonable increase of complexity for the pre_enable and post_disable
helpers (and the implementation could probably be simplified by moving
to recursive calls). I like this.

> > [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 (2):
> >    drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order
> >    drm/bridge: Document the expected behaviour of DSI host controllers
> >
> >   Documentation/gpu/drm-kms-helpers.rst |   7 +
> >   drivers/gpu/drm/drm_bridge.c          | 235 ++++++++++++++++++++++++++++++----
> >   include/drm/drm_bridge.h              |   8 ++
> >   3 files changed, 225 insertions(+), 25 deletions(-)

-- 
Regards,

Laurent Pinchart