Date:   Thu, 5 May 2022 17:46:19 +0300
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     Lyude Paul <lyude@...hat.com>
Cc:     Doug Anderson <dianders@...omium.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Maxime Ripard <maxime@...no.tech>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        freedreno <freedreno@...ts.freedesktop.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Robert Foss <robert.foss@...aro.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Hsin-Yi Wang <hsinyi@...omium.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Imre Deak <imre.deak@...el.com>,
        Jani Nikula <jani.nikula@...el.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm: Document that power requirements for DP AUX
 transfers

On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > Hi,
> > 
> > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > <ville.syrjala@...ux.intel.com> wrote:
> > > 
> > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> > > > When doing DP AUX transfers there are two actors that need to be
> > > > powered in order for the DP AUX transfer to work: the DP source and
> > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> > > > requirement on side-channel operations") added some documentation
> > > > saying that the DP source is required to power itself up (if needed)
> > > > to do AUX transfers. However, that commit doesn't talk anything about
> > > > the DP sink.
> > > > 
> > > > For full fledged DP the sink isn't really a problem. It's expected
> > > > that if an external DP monitor isn't plugged in that attempting to do
> > > > AUX transfers won't work. It's also expected that if a DP monitor is
> > > > plugged in (and thus asserting HPD) that it AUX transfers will work.
> > > > 
> > > > When we're looking at eDP, however, things are less obvious. Let's add
> > > > some documentation about expectations. Here's what we'll say:
> > > > 
> > > > 1. We don't expect the DP AUX transfer function to power on an eDP
> > > > panel. If an eDP panel is physically connected but powered off then it
> > > > makes sense for the transfer to fail.
> > > 
> > > I don't agree with this. I think the panel should just get powred up
> > > for AUX transfers.
> > 
> > That's definitely a fair thing to think about and I have at times
> > thought about trying to make it work that way. It always ends up
> > hitting a roadblock.

How do you even probe the panel initially if you can't power it on
without doing some kind of full modeset/etc.?

> > 
> > The biggest roadblock that I recall is that to make this work then
> > you'd have to somehow ensure that the bridge chain's pre_enable() call
> > was made as part of the AUX transfer, right? Since the transfer
> > function can be called in any context at all, we have to coordinate
> > this with DRM. If, for instance, DRM is mid way through powering the
> > panel down then we need to wait for DRM to fully finish powering down,
> > then we need to power the panel back up. I don't believe that we can
> > just force the panel to stay on if DRM is turning it off because of
> > panel power sequencing requirements. At least I know it would have the
> > potential to break "samsung-atna33xc20.c" which absolutely needs to
> > see the panel power off after it's been disabled.
> > 
> > We also, I believe, need to handle the fact that the bridge chain may
> > not have even been created yet. We do AUX transfers to read the EDID
> > and also to setup the backlight in the probe function of panel-edp. At
> > that point the panel hasn't been linked into the chain. We had _long_
> > discussions [1] about moving these out of probe and decided that we
> > could move the EDID read to be later but that it was going to really
> > ugly to move the AUX backlight later. The backlight would end up
> > popping up at some point in time later (the first call to panel
> > prepare() or maybe get_modes()) and that seemed weird.
> > 
> > [1]
> > https://lore.kernel.org/lkml/CAD=FV=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/
> > 
> > 
> > > Otherwise you can't trust that eg. the /dev/aux
> > > stuff is actually usable.
> > 
> > Yeah, it's been on my mind to talk more about /dev/aux. I think
> > /dev/aux has some problems, at least with eDP. Specifically:
> > 
> > 1. Even if we somehow figure out how to power the panel on as part of
> > the aux transfer, we actually _still_ not guaranteed to be able to
> > talk to it as far as I understand. My colleague reported to me that on
> > a system he was working with that had PSR (panel self refresh) that
> > when the panel was powered on but in PSR mode that it wouldn't talk
> > over AUX. Assuming that this is correct then I guess we'd also have to
> > do even more coordination with DRM to exit PSR and block future
> > transitions of PSR. (NOTE: it's always possible that my colleague ran
> > into some other bug and that panels are _supposed_ to be able to talk
> > in PSR. If you think this is the case, I can always try to dig more).
> 
> TBH - the coordination with drm I don't think would be the difficult part, as
> we'd just need to add some sort of property (ideally invisible to userspace)
> that can be used in an atomic commit to disable PSR - similar to how we enable
> CRC readback from sysfs in the majority of DRM drivers. That being said
> though, I think we can just leave the work of solving this problem up to
> whoever ends up needing this to work.

The driver should just disable/prevent PSR when doing AUX if the hardware
can't guarantee the PSR and AUX won't interfere with each other.

For i915 we have no problems with powering the panel on for AUX, but
there is still a race with PSR vs. AUX because both use the same hardware
internally. I've been nagging at people to fix this for i915 but I don't 
think it still got done :( Originally we were supposed to get a hardware
mutex for this but that plan got scrapped for some reason.

> 
> > 
> > 2. I'm not totally convinced that it's a great idea, at least for eDP,
> > for userspace to be mucking with /dev/aux. For DP's case I guess
> > /dev/aux is essentially enabling userspace drivers to do things like
> > update firmware on DP monitors or play with the backlight. I guess we
> > decided that we didn't want to add drivers in the kernel to handle
> > this type of stuff so we left it for userspace? For eDP, though, there
> 
> The main reason DP AUX got exposed to userspace in the first place was for
> usecases like fwupd,

My memory says the original reason was debugging. Or at least I had
no idea fwupd had started to use this until I saw some weird looking
DPCD addresses in some debug log.

But I suppose it's possible there were already plans for firmware
updates and whatnot and it just wasn't being discussed when this was
being developed.

-- 
Ville Syrjälä
Intel

Powered by blists - more mailing lists