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

[Sam Ravnborg <sam@ravnborg.org>]

Hi Dave,

a long overdue reply on this series.

On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote:
> Hi All
> 
> 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.

A typically chain looks like this:

CRTC => Encoder => Bridge A => Bridge B

We have in DRM bridges established what is the "next" bridge - indicated
with the direction of the arrows in the drawing.

This set of patches introduces the concept of "upstream" bridges.

pre_enable_prev_bridge_first would be easier to understand as it uses
the current terminology.
I get that "upstream" is used in the DSI specification - but we are
dealing with bridges that happens to support DSI and more, and mixing
the two terminologies is not good.

Note: Upstream is also used in a bridge doc section - here it should
      most likely be updated too.

The current approach set a flag that magically makes the core do something
else. Have you considered a much more explicit approach?

A few helpers like:

	drm_bridge_pre_enable_prev_bridge()
	drm_bridge_enable_prev_bridge()
	drm_bridge_disable_prev_bridge()
	drm_bridge_post_disable_prev_bridge()

And then update the core so the relevant function is only called once
for a bridge.
Then the need for DSI lanes in LP-11 can be archived by a call to

	drm_bridge_pre_enable_prev_bridge()

This is more explicit than a flag that triggers some magic behaviour.
It may even see uses we have not realised yet.

It is late here - so maybe the above is not a good idea tomorrow - but
right now I like the simplicity of it.

Other than the above I read that a mipi_dsi_host_init() is planned,
which is also explicit and simple - good.

Have we seen a new revision of some of these?
Chances are high that I have missed it then.

	Sam