Re: [PATCH v2] drm/bridge: analogix_dp: Grab runtime PM reference for DP-AUX

[Doug Anderson <dianders@chromium.org>]

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:
> >
> > 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