dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Brian Norris <briannorris@chromium.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Neil Armstrong <narmstrong@baylibre.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	"# 4.0+" <stable@vger.kernel.org>, Sean Paul <sean@poorly.run>
Subject: Re: [PATCH v2] drm/bridge: analogix_dp: Grab runtime PM reference for DP-AUX
Date: Tue, 15 Feb 2022 14:51:46 -0800	[thread overview]
Message-ID: <CA+ASDXPXKVwcZGYoagJYPm4E7DzaJmEVEv2FANhLH-juJw+r+Q@mail.gmail.com> (raw)
In-Reply-To: <CAD=FV=XU0bYVZk+-jPWZVoODW79QXOJ=NQy+RH=fYyX+LCZb2Q@mail.gmail.com>

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.

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.

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').

> 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 a reference, you could look at
> `drivers/gpu/drm/bridge/ti-sn65dsi86.c`. Also
> `drivers/gpu/drm/bridge/parade-ps8640.c`

Thanks for these. They look like reasonable patterns to follow.


Brian

  reply	other threads:[~2022-02-15 22:52 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-01 21:42 [PATCH v2] drm/bridge: analogix_dp: Grab runtime PM reference for DP-AUX Brian Norris
2022-01-05 19:55 ` Brian Norris
2022-01-11 13:26 ` Andrzej Hajda
2022-01-11 19:17   ` Brian Norris
2022-02-15 21:31 ` Doug Anderson
2022-02-15 22:51   ` Brian Norris [this message]
2022-02-15 23:46     ` Doug Anderson
2022-02-16  0:40       ` Brian Norris
2022-02-16 18:10         ` Doug Anderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CA+ASDXPXKVwcZGYoagJYPm4E7DzaJmEVEv2FANhLH-juJw+r+Q@mail.gmail.com \
    --to=briannorris@chromium.org \
    --cc=Laurent.pinchart@ideasonboard.com \
    --cc=a.hajda@samsung.com \
    --cc=dianders@chromium.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jernej.skrabec@gmail.com \
    --cc=jonas@kwiboo.se \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=narmstrong@baylibre.com \
    --cc=sean@poorly.run \
    --cc=stable@vger.kernel.org \
    --cc=tomeu.vizoso@collabora.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).