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

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

On 17/11/2022 17:34, Maxime Ripard wrote:
> On Thu, Nov 17, 2022 at 03:24:07PM +0200, Dmitry Baryshkov wrote:
>> On 15/11/2022 17:38, Dave Stevenson wrote:
>>> Hi Dmitry
>>>
>>> On Tue, 15 Nov 2022 at 14:21, Dmitry Baryshkov
>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>
>>>> On 15/11/2022 17:14, Dave Stevenson wrote:
>>>>> Hi Dmitry
>>>>>
>>>>> On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov
>>>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>>>
>>>>>> 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?
>>>>>
>>>>> If there was anything that could actually be worked on, then I'm happy
>>>>> to respin it, but if the approach is generally being rejected then I
>>>>> don't want to waste the effort.
>>>>>
>>>>> I'm not totally clear who the maintainers are that the final arbiters
>>>>> and need to sign off on this.
>>>>> drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND
>>>>> MISC GPU PATCHES"
>>>>> drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then
>>>>> Maarten, Maxime, and Thomas.
>>>>> Only Sam has responded publicly. I have had discussions with Maxime,
>>>>> but it's not directly his area of knowledge.
>>>>>
>>>>> Looking at the patch series:
>>>>> Patch 1: Your comment "update parade-ps8640 to use
>>>>> drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does
>>>>> this, but the patchset went wrong and is missing patches 8-11 and
>>>>> therefore hasn't been merged.
>>>>> Patch 2: Comment from Jagan that it's like an old patch. It has
>>>>> similarities, but isn't the same.
>>>>> Patch 3: R-b by you (thank you), but concerns from Jagan which I still
>>>>> don't understand. Without clarification on the issue and whether my
>>>>> suggested alternative place for the hook solves the issue, IMHO it's
>>>>> not worth respinning.
>>>>> Patch 4: R-b Laurent.
>>>>>
>>>>> This cover note got totally subverted with Exynos issues.
>>>>> Sam did request use of prev / next instead of upstream / downstream,
>>>>> which can be done and perhaps warrants a respin now.
>>>>>
>>>>>> [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].
>>>>>
>>>>> That can fix the power up sequence, but how do you propose telling the
>>>>> DSI controller NOT to power down in post_disable before the DSI
>>>>> peripheral post_disable has potentially sent DCS commands - i.e. the
>>>>> case you were discussing on Friday in [2].
>>>>
>>>> I thought that the same 'call the parent beforehand' switch applied to
>>>> the deinit paths, didn't it?
>>>
>>> My proposed flag does indeed swap the order of post_disable as well.
>>>
>>> Perhaps I was misunderstanding your personal preference.
>>> I was taking it as an explicit mipi_dsi_host call to initialise the
>>> DSI link, which then also needs an explicit mipi_dsi_host call to tear
>>> it down as well. In that situation there is a need to rework the
>>> bridge chain post_disable to allow for the panel post_disable to send
>>> DCS commands before the DSI host is disabled.
>>>
>>>>> If a panel/bridge driver doesn't call mipi_dsi_host_init then the
>>>>> expectation must be that it will be called by the DSI controller's
>>>>> pre_enable, and deinit from post_disable. Likewise init & deinit would
>>>>> be called if host_transfer is used when the host isn't initialised.
>>>>>
>>>>> If the panel/bridge driver explicitly calls mipi_dsi_host_init, then
>>>>> does that mandate that it must also call mipi_dsi_host_deinint. The
>>>>> controller post_disable is then effectively a no-op. This can be
>>>>> covered in documentation, but also leaves the potential for strange
>>>>> behaviour if the requirement is not followed, and I can't think of a
>>>>> nice place to drop a WARN to flag the issue in the driver.
>>>>>
>>>>>
>>>>> TBH The lack of interest in solving the issues almost makes me want to
>>>>> just document the total brokenness of it and throw in the towel.
>>>>> Seeing as we as Raspberry Pi run a vendor kernel, we can run with
>>>>> downstream patches until those who care finally make a decision for
>>>>> mainline. I'd prefer to solve it properly, but it requires some
>>>>> engagement from the community.
>>>>
>>>> I see. I can probably try spinning a patchset doing explicit mipi_dsi
>>>> calls. Let's see if it gains more attention.
>>>
>>> Is it better to have 2 competing patchsets floating around, or try and
>>> get responses on one first? I'll try and get an updated set out today.
>>
>> I'm a bit in a tough position here. I can't say that I like this approach,
>> but it seems to fix all the issues that we have with DSI hosts, so it's
>> better than the current state.
> 
> I'd say the bridge support in general is under-maintained. Historically,
> Boris and Laurent did most of the architecture work, but are either
> completely drowning under patches or have moved on.
> 
> I can't really speak for Thomas and Maarten, but I don't really have
> such a good knowledge about the bridge infrastructure and haven't been
> very involved. I have the same impression from Maarten and Thomas
> though.
> 
> Which means that it's pretty much a blindspot for us :)
> 
> I'm sorry if it's taking a while, but I'd say that if you two have a
> good comprehension of the issue (and I know Dave has), if you can reach
> a reasonable solution for both of you, and if there is proper
> documentation for the new work, I'd consider this a net improvement.
> 
> And as far as I know from that discussion, we're pretty much there
> already. So yeah, go ahead with a new version and we'll merge it.

Ack, then I'll hold on my proposal to let Dave's version to be merged.

-- 
With best wishes
Dmitry