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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

Hi Dave,

On 19/07/2022 16:45, Dave Stevenson wrote:
> Hi Sam
> 
> On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> 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.

It's been a while now. Do you still plan to pursue this patchset?

[personal notice: I'd prefer something less strange, e.g. an explicit 
calls to mipi_dsi_host, but as this patchset seems to fix the issues, 
I'm fine with it].

> 
>    Dave

-- 
With best wishes
Dmitry