Re: [PATCH] drm/dp: Don't attempt AUX transfers when eDP panels are not powered

[Doug Anderson]

Hi,

On Thu, Feb 15, 2024 at 8:53 AM Neil Armstrong
<neil.armstrong@xxxxxxxxxx> wrote:
>
> Hi Doug,
>
> On 15/02/2024 16:08, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Feb 15, 2024 at 2:24 AM Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> >>
> >> On Wed, 14 Feb 2024, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> >>> Hi,
> >>>
> >>> On Tue, Feb 13, 2024 at 10:25 PM Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> wrote:
> >>>>
> >>>> On Wed, Feb 14, 2024 at 2:23 PM Douglas Anderson <dianders@xxxxxxxxxxxx> 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.
> >>>>>
> >>>>> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> >>>>> ---
> >>>>
> >>>> Reviewed-by: Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx>
> >>>
> >>> Thanks for the review!
> >>>
> >>> Given that this touches core DRM code and that I never got
> >>> confirmation that Jani's concerns were addressed with my previous
> >>> response, I'm still going to wait a little while before applying. I'm
> >>> on vacation for most of next week, but if there are no further replies
> >>> between now and then I'll plan to apply this to "drm-misc-next" the
> >>> week of Feb 26th. If someone else wants to apply this before I do then
> >>> I certainly won't object. Jani: if you feel this needs more discussion
> >>> or otherwise object to this patch landing then please yell. Likewise
> >>> if anyone else in the community wants to throw in their opinion, feel
> >>> free.
> >>
> >> Sorry for dropping the ball after my initial response. I simply have not
> >> had the time to look into this.
> >>
> >> It would be great to get, say, drm-misc maintainer ack on this before
> >> merging. It's not fair for me to stall this any longer, I'll trust their
> >> judgement.
> >>
> >> Reasonable?
> >
> > I'd be more than happy for one of the drm-misc maintainers to Ack.
> > I'll move Maxime, Thomas, and Maarten to the "To:" line to see if that
> > helps get through their filters.
>
> I'll like some test reports to be sure it doesn't break anything,
> then I'll be happy to give my ack !

Sounds good. I know Eizan (CCed, but also a ChromeOS person) was going
to poke at it a bit and seemed willing to provide a Tested-by. I'll
let him chime in.

..but perhaps some better evidence that it's not breaking hardware is
that we actually landed this into one (but not all) ChromeOS kernel
trees [1] and it's rolled out to at least "beta" channel without
anyone screaming. Normally we like things to land upstream before
picking, but in this case we needed to land another patch to gather
in-field data [2] that would have made the problem even worse.

The kernel tree we landed on was the v5.15 tree, which is currently
serving all Qualcomm sc7180-based Chromebooks, all Mediatek 8173
Chromebooks and all Mediatek 8186 Chromebooks. There are also a pile
of x86 Chromebooks running our v5.15 kernel. This code shouldn't
affect them because (unless I'm mistaken) they don't use the two
affected panel drivers. In any case, I haven't heard any screams from
them either. Given my landing plans of "the week of the 26th", this
still gives another 1.5 weeks for any screams to reach my ears.

..or are you looking for non-ChromeOS test reports? I'm not sure how
to encourage those. I suppose sometimes folks at Red Hat end up
stumbling over similar panel problems to those of us in ChromeOS.
Maybe +Javier would be interested in providing a Tested-by?

[1] https://crrev.com/c/5277322
[2] https://crrev.com/c/5277736