Hi, On Tue, Feb 15, 2022 at 2:52 PM Brian Norris <briannorris@xxxxxxxxxxxx> wrote: > > On Tue, Feb 15, 2022 at 1:31 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > > > Hi, > > Hi! > > > On Fri, Oct 1, 2021 at 2:50 PM Brian Norris <briannorris@xxxxxxxxxxxx> 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: > > > > 1. 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. > > > 2. 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. > > 3. 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
