Hi,
On Mon, Apr 18, 2022 at 4:10 PM Doug Anderson dianders@chromium.org wrote:
- In general I've been asserting that it should be up to the panel to
power things on and drive all AUX transactions. ...but clearly my model isn't reality. We certainly do AUX transactions from the DP driver because the DP driver needs to know things about the connected device, like the number of lanes it has, the version of eDP it supports, and the available bit rates to name a few. Those things all work today by relying on the fact that pre-enable powers the panel on. It's pretty easy to say that reading the EDID (and I guess AUX backlight) is the odd one out. So right now I guess my model is:
5a) If the panel code wants to access the AUX bus it can do so by powering itself on and then just doing an AUX transaction and assuming that the provider of the AUX bus can power itself on as needed.
5b) If the DP code wants to access the AUX bus it should make sure that the next bridge's pre_enable() has been called. It can then assume that the device is powered on until the next bridge's post_disable() has been called.
So I guess tl;dr: I'm not really a huge fan of the DP driver powering the panel on by doing a pm_runtime_get() on it. I'd prefer to keep with the interface that we have to pre_enable() the panel to turn it on.
Again, thank for the explanation. Your concerns make more sense now. As much as I hate writing docs, maybe we should put at least basic notes (regarding panel requirements) somewhere to the DP/DP AUX documentation?
Sure. I actually don't mind writing docs, but my problem is trying to figure out where the heck to put them where someone would actually notice them. I could throw a blurb in the kernel doc for `struct drm_dp_aux` I guess? How about a deal: if you can tell me where to put the above facts (essentially 5a and 5b) then I'm happy to post a patch adding them.
Huh, actually, maybe the right place is in the "transfer" function of that structure which, as of commit bacbab58f09d ("drm: Mention the power state requirement on side-channel operations"), actually has a blurb. ...but I don't think the blurb there is totally complete. What if I changed it to this:
- Also note that this callback can be called no matter the
- state @dev is in and also no matter what state the panel is
- in. It's expected:
- If the @dev providing the AUX bus is currently unpowered then
- it will power itself up for the transfer.
- If the panel is not in a state where it can respond (it's not
- powered or it's in a low power state) then this function will
- fail. Note that if a panel driver is initiating a DP AUX transfer
- it may power itself up however it wants. All other code should
- use the pre_enable() bridge chain (which eventually calls the
- panel prepare function) to power the panel.
I didn't ignore this documentation request. Please take a look here and see what you think:
https://lore.kernel.org/r/20220503162033.1.Ia8651894026707e4fa61267da944ff73...
-Doug