Message-ID: <CAD=FV=XOyMDRT9dtW_mC_yQy-38oOXugFXJdOY5pkVy7DJhAJg@mail.gmail.com>
Date: Mon, 5 Feb 2024 08:23:32 -0800
From: Doug Anderson <dianders@...omium.org>
To: Jani Nikula <jani.nikula@...el.com>
Cc: dri-devel@...ts.freedesktop.org, eizan@...omium.org, 
	Ankit Nautiyal <ankit.k.nautiyal@...el.com>, Daniel Vetter <daniel@...ll.ch>, 
	David Airlie <airlied@...il.com>, Heiner Kallweit <hkallweit1@...il.com>, 
	Imre Deak <imre.deak@...el.com>, Jessica Zhang <quic_jesszhan@...cinc.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Sam Ravnborg <sam@...nborg.org>, 
	Stanislav Lisovskiy <stanislav.lisovskiy@...el.com>, Thomas Zimmermann <tzimmermann@...e.de>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/dp: Don't attempt AUX transfers when eDP panels are
 not powered

Hi,

On Mon, Feb 5, 2024 at 3:17 AM Jani Nikula <jani.nikula@...el.com> wrote:
>
> On Fri, 02 Feb 2024, Douglas Anderson <dianders@...omium.org> wrote:
> > If an eDP panel is not powered on then any attempts to talk to it over
> > the DP AUX channel will timeout. Unfortunately these attempts may be
> > quite slow. Userspace can initiate these attempts either via a
> > /dev/drm_dp_auxN device or via the created i2c device.
> >
> > Making the DP AUX drivers timeout faster is a difficult proposition.
> > In theory we could just poll the panel's HPD line in the AUX transfer
> > function and immediately return an error there. However, this is
> > easier said than done. For one thing, there's no hard requirement to
> > hook the HPD line up for eDP panels and it's OK to just delay a fixed
> > amount. For another thing, the HPD line may not be fast to probe. On
> > parade-ps8640 we need to wait for the bridge chip's firmware to boot
> > before we can get the HPD line and this is a slow process.
> >
> > The fact that the transfers are taking so long to timeout is causing
> > real problems. The open source fwupd daemon sometimes scans DP busses
> > looking for devices whose firmware need updating. If it happens to
> > scan while a panel is turned off this scan can take a long time. The
> > fwupd daemon could try to be smarter and only scan when eDP panels are
> > turned on, but we can also improve the behavior in the kernel.
> >
> > Let's let eDP panels drivers specify that a panel is turned off and
> > then modify the common AUX transfer code not to attempt a transfer in
> > this case.
>
> I guess my question is, why not make the aux->transfer function handle
> the powered down case and return the appropriate error?

The basic problem is that the aux->transfer() function doesn't have
knowledge of the power state of the eDP panel and that's the component
whose power state matters here. The aux->transfer() function is
implemented by the DP controller driver and can't access the internal
state of the eDP panel driver.

The traditional solution here would be to use the "HPD" pin to know if
the DP device is powered and ready to accept AUX commands. That works
fine for external DP devices where HPD is mandatory, but it has issues
for eDP as talked about in my commit description. If nothing else, the
eDP spec lists "HPD" as optional. In addition to that, however, we
have additional difficulties for eDP because of the "connected but not
powered on" state that eDP panels can be in. For DP if you see HPD you
know that the device is connected+powered on. If you don't see HPD
then the device is disconnected and/or powered off. For eDP you may
power off components (like the controller / eDP panel) when the device
is still physically connected and that adds complexities.

Another possible solution someone could come up with would be to use
the DRM state of the DP controller driver and fail all AUX commands if
the DP controller isn't powered. Unfortunately this doesn't work
either. Specifically at panel probe time we need to do AUX transfers
even though the full DRM bridge isn't powered. We had many discussions
about this on the mailing lists when coming up with the generic eDP
panel driver and this is fairly well documented in the kernel-docs of
the transfer() function in "struct drm_dp_aux". As documented, an eDP
controller driver needs to return an error for transfer() if the panel
isn't powered, but nothing says that it needs to do it quickly. The
slowness is what we're trying to solve here.


> For example, the transfer hook in i915 checks whether the display is
> connected and bails out early if not.

I'm not massively familiar with the i915 code, so I'd love a pointer
to the exact code you're referring to. I took a quick look and found a
Type-C specific check in intel_dp_aux_xfer(). That doesn't help here
since we're not Type-C, though the comments do back up my argument
that the long timeouts are something worth avoiding. After that I
don't see anything obvious so I'd love a pointer.


> Having to track and set the state all over the place seems more
> complicated to me than dynamically checking where needed i.e. in the
> transfer hook.

Huh. I was actually surprised by how simple/straightforward my patch
was compared to how ugly I thought it was going to be. I guess it's
just a different perspective? Specifically it can be noted that there
aren't many distinct eDP panel drivers out there since all but one of
them was able to use the generic "panel-edp.c". However, there are
quite a number of eDP controller drivers, especially considering all
the bridge chips out there. That means that this short patch means we
don't need to add weird logic/hacks to all of the eDP controller
drivers...

-Doug

Powered by blists - more mailing lists