Hi,
On Wed, Mar 2, 2022 at 9:20 AM Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi Doug
On Wed, 2 Mar 2022 at 00:13, Doug Anderson dianders@chromium.org wrote:
Hi,
On Wed, Feb 16, 2022 at 9:00 AM Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
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.
dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so the bridge/panel pre_enable can send commands. In their post_disable they then call the downstream bridge/panel post_disable op manually so that shutdown commands can be sent before shutting down the PHY. Nothing handles that fact, so the framework then continues down the bridge chain and calls the post_disable again, so we get unbalanced panel prepare/unprepare calls being reported [3].
There have been patches[4] proposing reversing the entire direction of pre_enable and post_disable, but that risks driving voltage into devices that have yet to be powered up. There have been discussions about adding either a pre_pre_enable, or adding a DSI host_op to initialise the host[5]. Both require significant reworking to all existing drivers in moving initialisation phases. We have patches that look like they may well be addressing race conditions in starting up a DSI peripheral[6].
In general I'm happy to let the more senior people in DRM set the direction here so I probably won't do lots of review, but I will point out that I did have another proposal that sorta got lost in the noise of the whole "reversing the entire direction". That's basically:
https://lists.freedesktop.org/archives/dri-devel/2021-October/328934.html
I have no idea if something like that would work for your use case, but after analyzing it it felt like a surprisingly clean proposal even if my first instinct when I thought about it was that it was a hack. ;-) I suspect (but haven't analyzed your code) that it might be equivalent to your proposal of using a flag but maybe easier to wrap ones head around?
If I'm reading that right, then you're proposing adding after_pre_enable and before_post_disable hooks. That's almost the same as the power_up() and power_down() ops that Dmitry suggested earlier, or pre_pre_enable / post_post_disable that had also been considered.
Neither of those options handles the case of a longer chain in which two non-consecutive links want their upstream bridge enabled first. As per the clarification in patch 1/2, considering the chain
- Panel
- Bridge 1
- Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
- Bridge 3
- Bridge 4 DRM_BRIDGE_OP_UPSTREAM_FIRST
- Bridge 5
- Encoder
With the flag option we call pre_enables as Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 5, Bridge 4, Encoder. If adding after_pre_enable, then we end up with Panel, Bridge 1, Bridge 3, Bridge 5, Bridge 4 (after_pre_enable), Bridge 2 (after_pre_enable), Encoder. (power_on / pre_pre_enable from encoder to connector would end up with Bridge 5 (power_on), Bridge 3 (power_on), Bridge 1 (power_on), Panel, Bridge 2, Bridge 4, Encoder). Those potentially work, but it seems a less logical order compared to using a flag to swap only the bridges of interest. I think power_on / pre_pre_enable covers DSI better than after_pre_enable.
Adding the extra ops requires the source bridge (eg DSI host) to know the behaviour the sink bridge/panel wants. So do all DSI hosts have to implement power_on to power up and enter LP-11. Some DSI peripherals may be quite happy or even prefer to have the bus totally idle / powered down at pre_enable, but there would be no way of implementing that.
Ah, that makes it super clear, thanks! :-) If the local swap of just two components that you're doing is more useful to you than the two stage approach and everyone likes it then I have no objections.
You seem to be looking at DP, which I have very little knowledge of, and I don't quite understand your comments about the AUX bus and how ordering should be configured. If your panel isn't a generic driver, couldn't it request that the upstream bridge is pre_enabled first?
I basically ended up solving my problem in a different way, so I have no immediate need of swapping the order right now. I am happy you are tackling it, though, and I can definitely see myself needing something like this in the future. :-)
-Doug