Hi Sam On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@xxxxxxxxxxxx> wrote: > > 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. Sure, I have no issues with switching to prev/next from upstream/downstream. To the outsider it can be confusing - in pre_enable and disable, the next bridge to be called is the previous one. At least it is documented. > 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() No point in drm_bridge_enable_prev_bridge() and drm_bridge_post_disable_prev_bridge() as the call order down the chain will mean that they have already been called. drm_bridge_enable_next_bridge() and drm_bridge_post_disable_next_bridge() possibly. > 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() Unfortunately it gets ugly with post_disable. The DSI host controller post_disable will have been called before the DSI peripheral's post_disable, and there are conditions where the peripheral needs to send DSI commands in post_disable (eg panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call drm_bridge_post_disable_next_bridge feels like the wrong thing to do. There are currently hacks in dw-mipi-dsi that do call the next panel/bridge post_disable [2] and it would be nice to get rid of them. Currently the calls aren't tracked for state, so you end up with post_disable being called twice, and panels having to track state (eg jdi_lt070me050000 [3]). [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off() https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107 [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889 [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44 > This is more explicit than a flag that triggers some magic behaviour. > It may even see uses we have not realised yet. Personally it feels like more boilerplate in almost all DSI drivers, and generally I see a push to remove boilerplate. > 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. It's been raised, but the justification for most use cases hasn't been made. The Exynos conversion looks to be doing the wrong thing in checking state, and that's why it is currently needing it. Again it's also more boilerplate. TC358767 is an odd one as it wants the DSI interface enabled very early in order to have a clock for the DP aux channel well before video is running. I had a thought on that, but It looks like I haven't hit send on a reply to Lucas on that one - too many distractions. > Have we seen a new revision of some of these? > Chances are high that I have missed it then. No, still on V2. Other than Dmitry's comment over updating parade-ps8640 and dropping drm_bridge_chain_*, no real comments had been made. Dave
