Hi,
On Tue, Feb 15, 2022 at 2:52 PM Brian Norris briannorris@chromium.org wrote:
On Tue, Feb 15, 2022 at 1:31 PM Doug Anderson dianders@chromium.org wrote:
Hi,
Hi!
On Fri, Oct 1, 2021 at 2:50 PM Brian Norris briannorris@chromium.org wrote:
If the display is not enable()d, then we aren't holding a runtime PM reference here. Thus, it's easy to accidentally cause a hang, if user space is poking around at /dev/drm_dp_aux0 at the "wrong" time.
Let's get the panel and PM state right before trying to talk AUX.
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index b7d2e4449cfa..6fc46ac93ef8 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1632,8 +1632,27 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux,
...
pm_runtime_get_sync(dp->dev);ret = analogix_dp_transfer(dp, msg);pm_runtime_put(dp->dev);I've spent an unfortunate amount of time digging around the DP AUX bus recently, so I can at least say that I have some experience and some opinions here.
Thanks! Experience is welcome, and opinions too sometimes ;)
IMO:
- Don't power the panel on. If the panel isn't powered on then the DP
AUX transfer will timeout. Tough nuggies. Think of yourself more like an i2c controller and of this as an i2c transfer implementation. The i2c controller isn't in charge of powering up the i2c devices on the bus. If userspace does an "i2c detect" on an i2c bus and some of the devices aren't powered then they won't be found. If you try to read/write from a powered off device that won't work either.
I guess this, paired with the driver examples below (ti-sn65dsi86.c, especially, which specifically throws errors if the panel isn't on), makes some sense. It's approximately (but more verbosely) what Andrzej was recommending too, I guess. It still makes me wonder what the point of the /dev/drm_dp_aux<N> interface is though, because it seems like you're pretty much destined to not have reliable operation through that means.
I can't say I have tons of history for those files. I seem to recall maybe someone using them to have userspace tweak the embedded backlight on some external DP connected panels? I think we also might use it in Chrome OS to update the firmware of panels (dunno if internal or external) in some cases too? I suspect that it works OK for certain situations but it's really not going to work in all cases...
Also note: I found that the AUX bus is really not working properly at all (even with this patch) in some cases due to self-refresh. Not only do we need the panel enabled, but we need to not be in self-refresh mode. Self-refresh is not currently exposed to user space, so user space has no way of knowing the panel is currently active, aside from racily inducing artificial display activity.
I suppose this just further proves the point that this is really not a great interface to rely on. It's fine for debugging during hardware bringup and I guess in limited situations it might be OK, but it's really not something we want userspace tweaking with anyway, right? In general I expect it's up to the kernel to be controlling peripherals on the DP AUX bus. The kernel should have a backlight driver and should do the AUX transfers needed. Having userspace in there mucking with things is just a bad idea. I mean, userspace also doesn't know when a panel has been power cycled and potentially lost any changes that they might have written, right?
I sorta suspect that most of the uses of these files are there because there wasn't a kernel driver and someone thought that doing it in userspace was the way to go?
But if we're OK with "just throw errors" or "just let stuff time out", then I guess that's not a big deal. My purpose is to avoid hanging the system, not to make /dev/drm_dp_aux<N> useful.
- In theory if the DP driver can read HPD (I haven't looked through
the analogix code to see how it handles it) then you can fail an AUX transfer right away if HPD isn't asserted instead of timing out. If this is hard, it's probably fine to just time out though.
This driver does handle HPD, but it also has overrides because apparently it doesn't work on some systems. I might see if we can leverage it, or I might just follow the bridge-enabled state (similar to ti-sn65dsi86.c's 'comms_enabled').
The "comms_enabled" is a bit ugly and is mostly there because we couldn't enable the bridge chip at the right time for some (probably unused) configuration, so I wouldn't necessarily say that it's the best model to follow. That being said, happy to review something if this model looks like the best way to go.
- Do the "pm_runtime" calls, but enable "autosuspend" with something
~1 second autosuspend delay. When using the AUX bus to read an EDID the underlying code will call your function 16 times in quick succession. If you're powering up and down constantly that'll be a bit of a waste.
Does this part really matter? For properly active cases, the bridge remains enabled, and it holds a runtime PM reference. For "maybe active" (your "tough nuggies" situation above), you're probably right that it's inefficient, but does it matter, when it's going to be a slow timed-out operation anyway? The AUX failure will be much slower than the PM transition.
I guess I can do this anyway, but frankly, I'll just be copy/pasting stuff from other drivers, because the runtime PM documentation still confuses me, and moreso once you involve autosuspend.
For the ti-sn65dsi86 it could take a few ms to power it up and down each time and it seemed wasteful to do this over and over again. Agreed that pm_runtime can easily get confusing.
-Doug