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
