drm: Document that power requirements for DP AUX transfers

Commit Message

Doug Anderson May 3, 2022, 11:21 p.m. UTC

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.

2. We'll document that the official way to power on a panel is via the
bridge chain, specifically by making sure that the panel's prepare
function has been called (which is called by
panel_bridge_pre_enable()). It's already specified in the kernel doc
of drm_panel_prepare() that this is the way to power the panel on and
also that after this call "it is possible to communicate with any
integrated circuitry via a command bus."

3. We'll also document that for code running in the panel driver
itself that it is legal for the panel driver to power itself up
however it wants (it doesn't need to officially call
drm_panel_pre_enable()) and then it can do AUX bus transfers. This is
currently the way that edp-panel works when it's running atop the DP
AUX bus.

Signed-off-by: Douglas Anderson <[e-mail]>
---

 include/drm/display/drm_dp_helper.h | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Dmitry Baryshkov May 4, 2022, 6:12 a.m. UTC | #1

On 04/05/2022 02:21, 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.

Nit: sink

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

then

> 
> 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.
> 
> 2. We'll document that the official way to power on a panel is via the
> bridge chain, specifically by making sure that the panel's prepare
> function has been called (which is called by
> panel_bridge_pre_enable()). It's already specified in the kernel doc
> of drm_panel_prepare() that this is the way to power the panel on and
> also that after this call "it is possible to communicate with any
> integrated circuitry via a command bus."
> 
> 3. We'll also document that for code running in the panel driver
> itself that it is legal for the panel driver to power itself up
> however it wants (it doesn't need to officially call
> drm_panel_pre_enable()) and then it can do AUX bus transfers. This is
> currently the way that edp-panel works when it's running atop the DP
> AUX bus.
> 
> Signed-off-by: Douglas Anderson <[e-mail]>

Reviewed-by: Dmitry Baryshkov <[e-mail]>

> ---
> 
>   include/drm/display/drm_dp_helper.h | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index dca40a045dd6..e5165b708a40 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -370,9 +370,17 @@ struct drm_dp_aux {
>   	 * helpers assume this is the case.
>   	 *
>   	 * Also note that this callback can be called no matter the
> -	 * state @dev is in. Drivers that need that device to be powered
> -	 * to perform this operation will first need to make sure it's
> -	 * been properly enabled.
> +	 * state @dev is in and also no matter what state the panel is
> +	 * in. It's expected:
> +	 * - If the @dev providing the AUX bus is currently unpowered then
> +	 *   it will power itself up for the transfer.
> +	 * - If we're on eDP and the panel is not in a state where it can
> +	 *   respond (it's not powered or it's in a low power state) then this
> +	 *   function will return an error (but not crash). Note that if a
> +	 *   panel driver is initiating a DP AUX transfer it may power itself
> +	 *   up however it wants. All other code should ensure that the
> +	 *   pre_enable() bridge chain (which eventually calls the panel
> +	 *   prepare function) has powered the panel.
>   	 */
>   	ssize_t (*transfer)(struct drm_dp_aux *aux,
>   			    struct drm_dp_aux_msg *msg);

Ville Syrjälä May 4, 2022, 12:21 p.m. UTC | #2

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. Otherwise you can't trust that eg. the /dev/aux
stuff is actually usable.

Doug Anderson May 4, 2022, 4:04 p.m. UTC | #3

Hi,

On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
<[e-mail]> 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.

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=[e-mail]/


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

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
is a panel driver and we if we have an AUX backlight we create a real
backlight device. If we needed to do a firmware update of an eDP panel
it would make sense for the panel driver to present some interface for
the firmware update so that the panel driver could make sure that the
panel stayed powered for the duration of the firmware update, not just
for the duration of a single AUX transfer.

3. In general it feels a little awkward for userspace to be directly
poking at the same set of registers that a kernel driver is also
poking at.

To me it feels like /dev/aux is much like the /dev/i2c interface. Yes,
userspace can go talk to random i2c devices and can even talk to them
after a kernel driver has "claimed" an i2c device, but:
a) If an i2c device is powered off, then the i2c transfer won't work.
b) If you set a register of a device managed by a kernel driver behind
the back of the kernel driver, you're really asking for trouble.


So I guess my proposals would be to pick one of:

a) Leave things they way they are as I've documented. NOTE that my
documentation does document the way things are today. No aux transfer
function that I'm aware of powers up an eDP panel. In this case if
someone wants to use /dev/aux for an eDP panel it's really up to them
not to shoot themselves in the foot.

b) Stop populating /dev/aux for eDP panels and only do it for DP and
then if/when someone yells we figure out how they were using /dev/aux
and why it was safe. This is definitely an ABI change but I have no
idea if it would really break anyone. I suppose we could take a first
step by spewing a WARN_ON if someone directly uses /dev/aux for eDP?

c) Somehow dynamically create / remove the /dev/aux device as the eDP
panel turns off and on again. If /dev/aux is there then we know that
the panel is on. NOTE: this ignores PSR. I don't think we'd want to
delete / create the /dev/aux node that often. So we'd either have to
still accept that the transfers will sometimes fail (c1) or make it a
requirement that we bring the panel out of PSR for an AUX transfer
(c2).


Technically we could list option (d) to power the panel up, but as per
above I think it's pretty awkward and doesn't feel like the right way
to go. Obviously happy to hear other opinions, though.

Lyude Paul May 4, 2022, 6:10 p.m. UTC | #4

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ä
> <[e-mail]> 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.
> 
> 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=[e-mail]/
> 
> 
> > 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.

> 
> 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, where some MST docks actually do their firmware updates
over DPCD. I don't know of any equivalent usecase for eDP at the moment, but I
can definitely try asking some of the OEM contacts I have whether this is/may
eventually be a thing or not.

> is a panel driver and we if we have an AUX backlight we create a real
> backlight device. If we needed to do a firmware update of an eDP panel
> it would make sense for the panel driver to present some interface for
> the firmware update so that the panel driver could make sure that the
> panel stayed powered for the duration of the firmware update, not just
> for the duration of a single AUX transfer.

Yeah, I tried adding this at one point actually but ran into some issues
finding a nice solution. It wasn't the most important thing at the time, so I
ended up shifting my attention to other things. Honestly the biggest
complicating factor of this is the fact that we can't synchronously wake up a
device from sysfs without introducing a deadlock due to lock order inversion
between DRM and sysfs. If this could be solved nicely, I think a lot of this
would become far easier.

> 
> 3. In general it feels a little awkward for userspace to be directly
> poking at the same set of registers that a kernel driver is also
> poking at.

We could always consider limiting the ranges that the DP AUX interface allows
userspace to read from, although I haven't thought too hard about that since I
don't know that would fix the issue entirely.

> 
> To me it feels like /dev/aux is much like the /dev/i2c interface. Yes,
> userspace can go talk to random i2c devices and can even talk to them
> after a kernel driver has "claimed" an i2c device, but:
> a) If an i2c device is powered off, then the i2c transfer won't work.
> b) If you set a register of a device managed by a kernel driver behind
> the back of the kernel driver, you're really asking for trouble.
> 
> 
> So I guess my proposals would be to pick one of:
> 
> a) Leave things they way they are as I've documented. NOTE that my
> documentation does document the way things are today. No aux transfer
> function that I'm aware of powers up an eDP panel. In this case if
> someone wants to use /dev/aux for an eDP panel it's really up to them
> not to shoot themselves in the foot.

To be honest, I do totally agree though that /dev/aux has very limited
usecases for eDP. I do think it's definitely a useful debugging tool, and it's
been a big help in figuring out how things like backlight interfaces work when
I'm otherwise lacking in docs (and sometimes it's still useful, since you can
test various subleties of panel controllers). So at a bare minimum, I'd very
much like it if we can at least keep it around in some form (perhaps hidden
behind a kernel config option). Although, that brings up the question of if
that makes it harder for someone without kernel debugging experience to get me
DPCD output from a panel outside of what got logged to the kernel…

> 
> b) Stop populating /dev/aux for eDP panels and only do it for DP and
> then if/when someone yells we figure out how they were using /dev/aux
> and why it was safe. This is definitely an ABI change but I have no
> idea if it would really break anyone. I suppose we could take a first
> step by spewing a WARN_ON if someone directly uses /dev/aux for eDP?
> 
> c) Somehow dynamically create / remove the /dev/aux device as the eDP
> panel turns off and on again. If /dev/aux is there then we know that
> the panel is on. NOTE: this ignores PSR. I don't think we'd want to
> delete / create the /dev/aux node that often. So we'd either have to
> still accept that the transfers will sometimes fail (c1) or make it a
> requirement that we bring the panel out of PSR for an AUX transfer
> (c2).
> 
> 
> Technically we could list option (d) to power the panel up, but as per
> above I think it's pretty awkward and doesn't feel like the right way
> to go. Obviously happy to hear other opinions, though.
>

Ville Syrjälä May 5, 2022, 2:46 p.m. UTC | #5

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ä
> > <[e-mail]> 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=[e-mail]/
> > 
> > 
> > > 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.

Doug Anderson May 5, 2022, 2:47 p.m. UTC | #6

Hi,

On Wed, May 4, 2022 at 11:10 AM Lyude Paul <[e-mail]> 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ä
> > <[e-mail]> 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.
> >
> > 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=[e-mail]/
> >
> >
> > > 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.
>
> >
> > 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, where some MST docks actually do their firmware updates
> over DPCD. I don't know of any equivalent usecase for eDP at the moment, but I
> can definitely try asking some of the OEM contacts I have whether this is/may
> eventually be a thing or not.

Thanks for the history. Even if we want to do firmware updates for eDP
with this, then having the AUX transfer function temporarily power the
panel would almost certainly not be enough. You can't update the
firmware in one AUX transfer and you certainly wouldn't want any
chance of the panel being powered down mid-update.

That means either:

a) Userspace has to have some way itself of ensuring that the panel
stays powered on. If this is true then we don't need to worry about
powering it on as part of the AUX transfer.

b) We shouldn't use the DP AUX transfer function for panel FW updates
and should come up with a solution for eDP where FW updates are
coordinated by the panel driver.

As you said above, this can wait until someone has the need to
implement this. Neither a) nor b) contradicts my documentation.


> > is a panel driver and we if we have an AUX backlight we create a real
> > backlight device. If we needed to do a firmware update of an eDP panel
> > it would make sense for the panel driver to present some interface for
> > the firmware update so that the panel driver could make sure that the
> > panel stayed powered for the duration of the firmware update, not just
> > for the duration of a single AUX transfer.
>
> Yeah, I tried adding this at one point actually but ran into some issues
> finding a nice solution. It wasn't the most important thing at the time, so I
> ended up shifting my attention to other things. Honestly the biggest
> complicating factor of this is the fact that we can't synchronously wake up a
> device from sysfs without introducing a deadlock due to lock order inversion
> between DRM and sysfs. If this could be solved nicely, I think a lot of this
> would become far easier.
>
> >
> > 3. In general it feels a little awkward for userspace to be directly
> > poking at the same set of registers that a kernel driver is also
> > poking at.
>
> We could always consider limiting the ranges that the DP AUX interface allows
> userspace to read from, although I haven't thought too hard about that since I
> don't know that would fix the issue entirely.
>
> >
> > To me it feels like /dev/aux is much like the /dev/i2c interface. Yes,
> > userspace can go talk to random i2c devices and can even talk to them
> > after a kernel driver has "claimed" an i2c device, but:
> > a) If an i2c device is powered off, then the i2c transfer won't work.
> > b) If you set a register of a device managed by a kernel driver behind
> > the back of the kernel driver, you're really asking for trouble.
> >
> >
> > So I guess my proposals would be to pick one of:
> >
> > a) Leave things they way they are as I've documented. NOTE that my
> > documentation does document the way things are today. No aux transfer
> > function that I'm aware of powers up an eDP panel. In this case if
> > someone wants to use /dev/aux for an eDP panel it's really up to them
> > not to shoot themselves in the foot.
>
> To be honest, I do totally agree though that /dev/aux has very limited
> usecases for eDP. I do think it's definitely a useful debugging tool, and it's
> been a big help in figuring out how things like backlight interfaces work when
> I'm otherwise lacking in docs (and sometimes it's still useful, since you can
> test various subleties of panel controllers). So at a bare minimum, I'd very
> much like it if we can at least keep it around in some form (perhaps hidden
> behind a kernel config option). Although, that brings up the question of if
> that makes it harder for someone without kernel debugging experience to get me
> DPCD output from a panel outside of what got logged to the kernel…

My opinion is to simply land this documentation patch but otherwise
leave it alone. Any chance you'd be willing to provide a Reviewed-by
(assuming I fix the typo that Dmitry pointed out)?


> > b) Stop populating /dev/aux for eDP panels and only do it for DP and
> > then if/when someone yells we figure out how they were using /dev/aux
> > and why it was safe. This is definitely an ABI change but I have no
> > idea if it would really break anyone. I suppose we could take a first
> > step by spewing a WARN_ON if someone directly uses /dev/aux for eDP?
> >
> > c) Somehow dynamically create / remove the /dev/aux device as the eDP
> > panel turns off and on again. If /dev/aux is there then we know that
> > the panel is on. NOTE: this ignores PSR. I don't think we'd want to
> > delete / create the /dev/aux node that often. So we'd either have to
> > still accept that the transfers will sometimes fail (c1) or make it a
> > requirement that we bring the panel out of PSR for an AUX transfer
> > (c2).
> >
> >
> > Technically we could list option (d) to power the panel up, but as per
> > above I think it's pretty awkward and doesn't feel like the right way
> > to go. Obviously happy to hear other opinions, though.

Doug Anderson May 5, 2022, 3 p.m. UTC | #7

Hi,

On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
<[e-mail]> wrote:
>
> 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ä
> > > <[e-mail]> 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.?

It's not that we can't power it on without a full modeset. It' that at
panel probe time all the DRM components haven't been hooked together
yet, so the bridge chain isn't available yet. The panel can power
itself on, though. This is why the documentation I added says: "if a
panel driver is initiating a DP AUX transfer it may power itself up
however it wants"


> > > 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=[e-mail]/
> > >
> > >
> > > > 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.

OK, fair enough. If we can solve the PSR problem that would be great.


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

I haven't looked at the i915 DRM code much, but my understanding is
that it's more of an "all in one" approach. The one driver pretty much
handles everything itself. That means that powering the panel up isn't
too hard. Is that right?


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

If it's just for debugging, I'd argue that leaving it as-is should be
fine. Someone poking around with their system can find a way to make
sure that the panel stays on. This is similar to how if you're poking
around with /dev/i2c it's up to you to make sure that the i2c device
you're poking at stays powered.

-Doug

Ville Syrjälä May 5, 2022, 3:08 p.m. UTC | #8

On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> <[e-mail]> wrote:
> >
> > 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ä
> > > > <[e-mail]> 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.?
> 
> It's not that we can't power it on without a full modeset. It' that at
> panel probe time all the DRM components haven't been hooked together
> yet, so the bridge chain isn't available yet. The panel can power
> itself on, though. This is why the documentation I added says: "if a
> panel driver is initiating a DP AUX transfer it may power itself up
> however it wants"
> 
> 
> > > > 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=[e-mail]/
> > > >
> > > >
> > > > > 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.
> 
> OK, fair enough. If we can solve the PSR problem that would be great.
> 
> 
> > 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.
> 
> I haven't looked at the i915 DRM code much, but my understanding is
> that it's more of an "all in one" approach. The one driver pretty much
> handles everything itself. That means that powering the panel up isn't
> too hard. Is that right?

Yeah, we don't have too many "helpful" abstractions in the way ;)

> > > > 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.
> 
> If it's just for debugging, I'd argue that leaving it as-is should be
> fine. Someone poking around with their system can find a way to make
> sure that the panel stays on.

That could require altering the state of the system quite a bit, which
may defeat the purpose. At least I would not be willing to accept such 
a limitation.

> This is similar to how if you're poking
> around with /dev/i2c it's up to you to make sure that the i2c device
> you're poking at stays powered.
> 
> -Doug

Doug Anderson May 5, 2022, 3:53 p.m. UTC | #9

Hi,

On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
<[e-mail]> wrote:
>
> On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > <[e-mail]> wrote:
> > >
> > > 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ä
> > > > > <[e-mail]> 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.?
> >
> > It's not that we can't power it on without a full modeset. It' that at
> > panel probe time all the DRM components haven't been hooked together
> > yet, so the bridge chain isn't available yet. The panel can power
> > itself on, though. This is why the documentation I added says: "if a
> > panel driver is initiating a DP AUX transfer it may power itself up
> > however it wants"
> >
> >
> > > > > 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=[e-mail]/
> > > > >
> > > > >
> > > > > > 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.
> >
> > OK, fair enough. If we can solve the PSR problem that would be great.
> >
> >
> > > 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.
> >
> > I haven't looked at the i915 DRM code much, but my understanding is
> > that it's more of an "all in one" approach. The one driver pretty much
> > handles everything itself. That means that powering the panel up isn't
> > too hard. Is that right?
>
> Yeah, we don't have too many "helpful" abstractions in the way ;)
>
> > > > > 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.
> >
> > If it's just for debugging, I'd argue that leaving it as-is should be
> > fine. Someone poking around with their system can find a way to make
> > sure that the panel stays on.
>
> That could require altering the state of the system quite a bit, which
> may defeat the purpose.

It does? In my experience you just need to make sure that the panel is
turned on. ...or are you saying that you'd use this for debugging a
case where the system isn't probing properly?

If things are truly in bad shape, at least on boards using device tree
it's easy to tweak the device tree to force a regulator to stay on. I
suppose we could also add a "debugfs" entry for the panel that also
forces it to be powered on.


>  At least I would not be willing to accept such
> a limitation.

Hmm, so where does that leave us? Are you against landing this patch?
I've done a lot of cleanups recently and I just don't think I have the
time to rework all the AUX transfer functions and figure out how to
power the panel. It also seems like a lot of added complexity for a
debug path.


> > This is similar to how if you're poking
> > around with /dev/i2c it's up to you to make sure that the i2c device
> > you're poking at stays powered.
> >
> > -Doug
>
> --
> Ville Syrjälä
> Intel

Ville Syrjälä May 5, 2022, 7:19 p.m. UTC | #10

On Thu, May 05, 2022 at 08:53:12AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> <[e-mail]> wrote:
> >
> > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > <[e-mail]> wrote:
> > > >
> > > > 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ä
> > > > > > <[e-mail]> 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.?
> > >
> > > It's not that we can't power it on without a full modeset. It' that at
> > > panel probe time all the DRM components haven't been hooked together
> > > yet, so the bridge chain isn't available yet. The panel can power
> > > itself on, though. This is why the documentation I added says: "if a
> > > panel driver is initiating a DP AUX transfer it may power itself up
> > > however it wants"
> > >
> > >
> > > > > > 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=[e-mail]/
> > > > > >
> > > > > >
> > > > > > > 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.
> > >
> > > OK, fair enough. If we can solve the PSR problem that would be great.
> > >
> > >
> > > > 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.
> > >
> > > I haven't looked at the i915 DRM code much, but my understanding is
> > > that it's more of an "all in one" approach. The one driver pretty much
> > > handles everything itself. That means that powering the panel up isn't
> > > too hard. Is that right?
> >
> > Yeah, we don't have too many "helpful" abstractions in the way ;)
> >
> > > > > > 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.
> > >
> > > If it's just for debugging, I'd argue that leaving it as-is should be
> > > fine. Someone poking around with their system can find a way to make
> > > sure that the panel stays on.
> >
> > That could require altering the state of the system quite a bit, which
> > may defeat the purpose.
> 
> It does? In my experience you just need to make sure that the panel is
> turned on. ...or are you saying that you'd use this for debugging a
> case where the system isn't probing properly?

I don't want to have to eg. try to set a mode on the panel to get it
to reply to AUX/DDC. I want to be able to talk to it in any situation.
I don't need to jump through any extra hoops to talk to external DP
panels, and don't really see why eDP should be any different.

> If things are truly in bad shape, at least on boards using device tree
> it's easy to tweak the device tree to force a regulator to stay on. I
> suppose we could also add a "debugfs" entry for the panel that also
> forces it to be powered on.

Not really sure how adding a separate knob for it would be
somehow easier than just turning on the power from the
aux transfer hook.

> >  At least I would not be willing to accept such
> > a limitation.
> 
> Hmm, so where does that leave us? Are you against landing this patch?
> I've done a lot of cleanups recently and I just don't think I have the
> time to rework all the AUX transfer functions and figure out how to
> power the panel. It also seems like a lot of added complexity for a
> debug path.

If people don't feel like fixing this then I have no real
objection to documenting the fact that *some* drivers can't
power the panel on for AUX transfers for whatever random
reason. But I disagree with claims that it is the only
expected/desired behaviour.

Lyude Paul May 5, 2022, 7:47 p.m. UTC | #11

So I think if Ville is OK with it (an ack at least) I'm fine with this
documentation change. I think it's worth me noting for other reviewers I made
this decision based on the fact that the documentation is for the transfer()
function - which I agree shouldn't need to be responsible for powering the
panel on.

Since that doesn't actually specify what we expect the behavior for userspace
accesses to be (since we could in theory add more behavior in those codepaths
around the transfer() calls that don't exist for the driver's own AUX usages)
I think this is totally fine

Reviewed-by: Lyude Paul <[e-mail]>

On Tue, 2022-05-03 at 16:21 -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.
> 
> 2. We'll document that the official way to power on a panel is via the
> bridge chain, specifically by making sure that the panel's prepare
> function has been called (which is called by
> panel_bridge_pre_enable()). It's already specified in the kernel doc
> of drm_panel_prepare() that this is the way to power the panel on and
> also that after this call "it is possible to communicate with any
> integrated circuitry via a command bus."
> 
> 3. We'll also document that for code running in the panel driver
> itself that it is legal for the panel driver to power itself up
> however it wants (it doesn't need to officially call
> drm_panel_pre_enable()) and then it can do AUX bus transfers. This is
> currently the way that edp-panel works when it's running atop the DP
> AUX bus.
> 
> Signed-off-by: Douglas Anderson <[e-mail]>
> ---
> 
>  include/drm/display/drm_dp_helper.h | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/include/drm/display/drm_dp_helper.h
> b/include/drm/display/drm_dp_helper.h
> index dca40a045dd6..e5165b708a40 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -370,9 +370,17 @@ struct drm_dp_aux {
>          * helpers assume this is the case.
>          *
>          * Also note that this callback can be called no matter the
> -        * state @dev is in. Drivers that need that device to be powered
> -        * to perform this operation will first need to make sure it's
> -        * been properly enabled.
> +        * state @dev is in and also no matter what state the panel is
> +        * in. It's expected:
> +        * - If the @dev providing the AUX bus is currently unpowered then
> +        *   it will power itself up for the transfer.
> +        * - If we're on eDP and the panel is not in a state where it can
> +        *   respond (it's not powered or it's in a low power state) then
> this
> +        *   function will return an error (but not crash). Note that if a
> +        *   panel driver is initiating a DP AUX transfer it may power
> itself
> +        *   up however it wants. All other code should ensure that the
> +        *   pre_enable() bridge chain (which eventually calls the panel
> +        *   prepare function) has powered the panel.
>          */
>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>                             struct drm_dp_aux_msg *msg);

Dmitry Baryshkov May 5, 2022, 8:09 p.m. UTC | #12

On Thu, 5 May 2022 at 18:53, Doug Anderson <[e-mail]> wrote:
>
> Hi,
>
> On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> <[e-mail]> wrote:
> >
> > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > <[e-mail]> wrote:
> > > >
> > > > 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ä
> > > > > > <[e-mail]> 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.?
> > >
> > > It's not that we can't power it on without a full modeset. It' that at
> > > panel probe time all the DRM components haven't been hooked together
> > > yet, so the bridge chain isn't available yet. The panel can power
> > > itself on, though. This is why the documentation I added says: "if a
> > > panel driver is initiating a DP AUX transfer it may power itself up
> > > however it wants"
> > >
> > >
> > > > > > 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=[e-mail]/
> > > > > >
> > > > > >
> > > > > > > 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.
> > >
> > > OK, fair enough. If we can solve the PSR problem that would be great.
> > >
> > >
> > > > 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.
> > >
> > > I haven't looked at the i915 DRM code much, but my understanding is
> > > that it's more of an "all in one" approach. The one driver pretty much
> > > handles everything itself. That means that powering the panel up isn't
> > > too hard. Is that right?
> >
> > Yeah, we don't have too many "helpful" abstractions in the way ;)
> >
> > > > > > 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.
> > >
> > > If it's just for debugging, I'd argue that leaving it as-is should be
> > > fine. Someone poking around with their system can find a way to make
> > > sure that the panel stays on.
> >
> > That could require altering the state of the system quite a bit, which
> > may defeat the purpose.
>
> It does? In my experience you just need to make sure that the panel is
> turned on. ...or are you saying that you'd use this for debugging a
> case where the system isn't probing properly?
>
> If things are truly in bad shape, at least on boards using device tree
> it's easy to tweak the device tree to force a regulator to stay on. I
> suppose we could also add a "debugfs" entry for the panel that also
> forces it to be powered on.
>
>
> >  At least I would not be willing to accept such
> > a limitation.
>
> Hmm, so where does that leave us? Are you against landing this patch?
> I've done a lot of cleanups recently and I just don't think I have the
> time to rework all the AUX transfer functions and figure out how to
> power the panel. It also seems like a lot of added complexity for a
> debug path.

If my 2c counts, I support landing this patch. It clearly documents
current behaviour and expectations.

If that helps,
Acked-by: Dmitry Baryshkov <[e-mail]>

As for the /dev/aux, question, I think we can make the following plan work:
- Document that eDP panel power up can be handled by using the
pm_runtime API (which is the case for both panel-edp and atna33xc20)).
I think this is a sensible requirement anyway. And both panels show
how to handle different poweron/poweroff timings.
- Make drm_dp_aux_dev_get_by_minor() pm_runtime_get() the attached panel.

> > > This is similar to how if you're poking
> > > around with /dev/i2c it's up to you to make sure that the i2c device
> > > you're poking at stays powered.

Doug Anderson May 5, 2022, 8:12 p.m. UTC | #13

Hi,

On Thu, May 5, 2022 at 12:19 PM Ville Syrjälä
<[e-mail]> wrote:
>
> On Thu, May 05, 2022 at 08:53:12AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> > <[e-mail]> wrote:
> > >
> > > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > > <[e-mail]> wrote:
> > > > >
> > > > > 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ä
> > > > > > > <[e-mail]> 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.?
> > > >
> > > > It's not that we can't power it on without a full modeset. It' that at
> > > > panel probe time all the DRM components haven't been hooked together
> > > > yet, so the bridge chain isn't available yet. The panel can power
> > > > itself on, though. This is why the documentation I added says: "if a
> > > > panel driver is initiating a DP AUX transfer it may power itself up
> > > > however it wants"
> > > >
> > > >
> > > > > > > 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=[e-mail]/
> > > > > > >
> > > > > > >
> > > > > > > > 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.
> > > >
> > > > OK, fair enough. If we can solve the PSR problem that would be great.
> > > >
> > > >
> > > > > 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.
> > > >
> > > > I haven't looked at the i915 DRM code much, but my understanding is
> > > > that it's more of an "all in one" approach. The one driver pretty much
> > > > handles everything itself. That means that powering the panel up isn't
> > > > too hard. Is that right?
> > >
> > > Yeah, we don't have too many "helpful" abstractions in the way ;)
> > >
> > > > > > > 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.
> > > >
> > > > If it's just for debugging, I'd argue that leaving it as-is should be
> > > > fine. Someone poking around with their system can find a way to make
> > > > sure that the panel stays on.
> > >
> > > That could require altering the state of the system quite a bit, which
> > > may defeat the purpose.
> >
> > It does? In my experience you just need to make sure that the panel is
> > turned on. ...or are you saying that you'd use this for debugging a
> > case where the system isn't probing properly?
>
> I don't want to have to eg. try to set a mode on the panel to get it
> to reply to AUX/DDC. I want to be able to talk to it in any situation.
> I don't need to jump through any extra hoops to talk to external DP
> panels, and don't really see why eDP should be any different.
>
> > If things are truly in bad shape, at least on boards using device tree
> > it's easy to tweak the device tree to force a regulator to stay on. I
> > suppose we could also add a "debugfs" entry for the panel that also
> > forces it to be powered on.
>
> Not really sure how adding a separate knob for it would be
> somehow easier than just turning on the power from the
> aux transfer hook.
>
> > >  At least I would not be willing to accept such
> > > a limitation.
> >
> > Hmm, so where does that leave us? Are you against landing this patch?
> > I've done a lot of cleanups recently and I just don't think I have the
> > time to rework all the AUX transfer functions and figure out how to
> > power the panel. It also seems like a lot of added complexity for a
> > debug path.
>
> If people don't feel like fixing this then I have no real
> objection to documenting the fact that *some* drivers can't
> power the panel on for AUX transfers for whatever random
> reason. But I disagree with claims that it is the only
> expected/desired behaviour.

OK. I'll spin the wording like this:

* - If the @dev providing the AUX bus is currently unpowered then
*   it will power itself up for the transfer.
* - If we're on eDP (using a drm_panel) and the panel is not in a
*   state where it can respond (it's not powered or it's in a
*   low power state) then this function may return an error, but
*   not crash. It's up to the caller of this code to make sure that
*   the panel is powered on if getting an error back is not OK. If a
*   drm_panel driver is initiating a DP AUX transfer it may power
*   itself up however it wants. All other code should ensure that
*   the pre_enable() bridge chain (which eventually calls the
*   drm_panel prepare function) has powered the panel.

I'll keep Lyude and Dmitry's R-b tags. I'll plan to spin with that in
a few days if I don't hear anything else.

-Doug

Doug Anderson May 5, 2022, 8:21 p.m. UTC | #14

Hi,

On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> On Thu, 5 May 2022 at 18:53, Doug Anderson <[e-mail]> wrote:
> >
> > Hi,
> >
> > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> > <[e-mail]> wrote:
> > >
> > > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > > <[e-mail]> wrote:
> > > > >
> > > > > 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ä
> > > > > > > <[e-mail]> 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.?
> > > >
> > > > It's not that we can't power it on without a full modeset. It' that at
> > > > panel probe time all the DRM components haven't been hooked together
> > > > yet, so the bridge chain isn't available yet. The panel can power
> > > > itself on, though. This is why the documentation I added says: "if a
> > > > panel driver is initiating a DP AUX transfer it may power itself up
> > > > however it wants"
> > > >
> > > >
> > > > > > > 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=[e-mail]/
> > > > > > >
> > > > > > >
> > > > > > > > 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.
> > > >
> > > > OK, fair enough. If we can solve the PSR problem that would be great.
> > > >
> > > >
> > > > > 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.
> > > >
> > > > I haven't looked at the i915 DRM code much, but my understanding is
> > > > that it's more of an "all in one" approach. The one driver pretty much
> > > > handles everything itself. That means that powering the panel up isn't
> > > > too hard. Is that right?
> > >
> > > Yeah, we don't have too many "helpful" abstractions in the way ;)
> > >
> > > > > > > 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.
> > > >
> > > > If it's just for debugging, I'd argue that leaving it as-is should be
> > > > fine. Someone poking around with their system can find a way to make
> > > > sure that the panel stays on.
> > >
> > > That could require altering the state of the system quite a bit, which
> > > may defeat the purpose.
> >
> > It does? In my experience you just need to make sure that the panel is
> > turned on. ...or are you saying that you'd use this for debugging a
> > case where the system isn't probing properly?
> >
> > If things are truly in bad shape, at least on boards using device tree
> > it's easy to tweak the device tree to force a regulator to stay on. I
> > suppose we could also add a "debugfs" entry for the panel that also
> > forces it to be powered on.
> >
> >
> > >  At least I would not be willing to accept such
> > > a limitation.
> >
> > Hmm, so where does that leave us? Are you against landing this patch?
> > I've done a lot of cleanups recently and I just don't think I have the
> > time to rework all the AUX transfer functions and figure out how to
> > power the panel. It also seems like a lot of added complexity for a
> > debug path.
>
> If my 2c counts, I support landing this patch. It clearly documents
> current behaviour and expectations.
>
> If that helps,
> Acked-by: Dmitry Baryshkov <[e-mail]>
>
> As for the /dev/aux, question, I think we can make the following plan work:
> - Document that eDP panel power up can be handled by using the
> pm_runtime API (which is the case for both panel-edp and atna33xc20)).
> I think this is a sensible requirement anyway. And both panels show
> how to handle different poweron/poweroff timings.
> - Make drm_dp_aux_dev_get_by_minor() pm_runtime_get() the attached panel.

This matches what you suggested previously, but I still think it has a
potential problem as I talked about in the my previous (very long)
reply [1]. The relevant part was:

> Now, despite the fact that the generic eDP panel code doesn't follow
> the "strict"ness I just described, the _other_ DP panel I worked on
> recently (samsung-atna33xc20) does. In testing we found that this
> panel would sometimes (like 1 in 20 times?) crash if you ever stopped
> outputting data to the display and then started again. You absolutely
> needed to fully power cycle the display each time. I tried to document
> this to the best of my ability in atana33xc20_unprepare(). There's
> also a WARN_ON() in atana33xc20_enable() trying to detect if someone
> is doing something the panel driver doesn't expect.

Specifically, I think you could get in trouble if you did:

a) drm wants to power down the panel.

b) drm calls the panel's disable() function

c) we start an aux transfer and grab a runtime pm reference

d) drm calls the panel's unprepare() function => atana33xc20_unprepare()

e) atana33xc20_unprepare()'s pm_runtime_put_sync_suspend() _won't_
power off the panel (we still have the reference from step c), even
though it needs to.

f) we'll finish an aux transfer and, presumably, call
pm_runtime_put_autosuspend()

g) drm wants to power the panel back up

h) drm calls the panel's prepare() function, but power wasn't properly cycled

This was the whole reason why I wanted to document that the official
API for powering the panel was via the panel's prepare() function.

[1] https://lore.kernel.org/r/CAD=FV=[e-mail]/

-Doug

Dmitry Baryshkov May 5, 2022, 8:56 p.m. UTC | #15

On Thu, 5 May 2022 at 23:21, Doug Anderson <[e-mail]> wrote:
>
> Hi,
>
> On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov
> <[e-mail]> wrote:
> >
> > On Thu, 5 May 2022 at 18:53, Doug Anderson <[e-mail]> wrote:
> > >
> > > Hi,
> > >
> > > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> > > <[e-mail]> wrote:
> > > >
> > > > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > > > Hi,
> > > > >
> > > > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > 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ä
> > > > > > > > <[e-mail]> 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.?
> > > > >
> > > > > It's not that we can't power it on without a full modeset. It' that at
> > > > > panel probe time all the DRM components haven't been hooked together
> > > > > yet, so the bridge chain isn't available yet. The panel can power
> > > > > itself on, though. This is why the documentation I added says: "if a
> > > > > panel driver is initiating a DP AUX transfer it may power itself up
> > > > > however it wants"
> > > > >
> > > > >
> > > > > > > > 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=[e-mail]/
> > > > > > > >
> > > > > > > >
> > > > > > > > > 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.
> > > > >
> > > > > OK, fair enough. If we can solve the PSR problem that would be great.
> > > > >
> > > > >
> > > > > > 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.
> > > > >
> > > > > I haven't looked at the i915 DRM code much, but my understanding is
> > > > > that it's more of an "all in one" approach. The one driver pretty much
> > > > > handles everything itself. That means that powering the panel up isn't
> > > > > too hard. Is that right?
> > > >
> > > > Yeah, we don't have too many "helpful" abstractions in the way ;)
> > > >
> > > > > > > > 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.
> > > > >
> > > > > If it's just for debugging, I'd argue that leaving it as-is should be
> > > > > fine. Someone poking around with their system can find a way to make
> > > > > sure that the panel stays on.
> > > >
> > > > That could require altering the state of the system quite a bit, which
> > > > may defeat the purpose.
> > >
> > > It does? In my experience you just need to make sure that the panel is
> > > turned on. ...or are you saying that you'd use this for debugging a
> > > case where the system isn't probing properly?
> > >
> > > If things are truly in bad shape, at least on boards using device tree
> > > it's easy to tweak the device tree to force a regulator to stay on. I
> > > suppose we could also add a "debugfs" entry for the panel that also
> > > forces it to be powered on.
> > >
> > >
> > > >  At least I would not be willing to accept such
> > > > a limitation.
> > >
> > > Hmm, so where does that leave us? Are you against landing this patch?
> > > I've done a lot of cleanups recently and I just don't think I have the
> > > time to rework all the AUX transfer functions and figure out how to
> > > power the panel. It also seems like a lot of added complexity for a
> > > debug path.
> >
> > If my 2c counts, I support landing this patch. It clearly documents
> > current behaviour and expectations.
> >
> > If that helps,
> > Acked-by: Dmitry Baryshkov <[e-mail]>
> >
> > As for the /dev/aux, question, I think we can make the following plan work:
> > - Document that eDP panel power up can be handled by using the
> > pm_runtime API (which is the case for both panel-edp and atna33xc20)).
> > I think this is a sensible requirement anyway. And both panels show
> > how to handle different poweron/poweroff timings.
> > - Make drm_dp_aux_dev_get_by_minor() pm_runtime_get() the attached panel.
>
> This matches what you suggested previously, but I still think it has a
> potential problem as I talked about in the my previous (very long)
> reply [1]. The relevant part was:
>
> > Now, despite the fact that the generic eDP panel code doesn't follow
> > the "strict"ness I just described, the _other_ DP panel I worked on
> > recently (samsung-atna33xc20) does. In testing we found that this
> > panel would sometimes (like 1 in 20 times?) crash if you ever stopped
> > outputting data to the display and then started again. You absolutely
> > needed to fully power cycle the display each time. I tried to document
> > this to the best of my ability in atana33xc20_unprepare(). There's
> > also a WARN_ON() in atana33xc20_enable() trying to detect if someone
> > is doing something the panel driver doesn't expect.
>
> Specifically, I think you could get in trouble if you did:
>
> a) drm wants to power down the panel.
>
> b) drm calls the panel's disable() function
>
> c) we start an aux transfer and grab a runtime pm reference
>
> d) drm calls the panel's unprepare() function => atana33xc20_unprepare()
>
> e) atana33xc20_unprepare()'s pm_runtime_put_sync_suspend() _won't_
> power off the panel (we still have the reference from step c), even
> though it needs to.
>
> f) we'll finish an aux transfer and, presumably, call
> pm_runtime_put_autosuspend()
>
> g) drm wants to power the panel back up
>
> h) drm calls the panel's prepare() function, but power wasn't properly cycled

Why? We'd need to extend the prepare() function with the flag
data_was_on, which is set in the enable() and cleared in the suspend
path. If we see this flag in the prepare() callback, we should
forcibly power cycle the panel by toggling the regulator. Yes, this
will cause additional wait.

Another option might be to toggle the autosuspend support. Let the
disable() call pm_runtime_dont_autosuspend() (which would turn all
autosuspend calls into plain pm_runtime_put()) and allow it again in
the resume(). I'm not 100% sure that this will work, but it looks like
it should.

The second option leaves a possible window for the panel issues if the
userspace AUX transfer is ongoing while the panel is being unprepared
and then prepared again. In this case it will never be power cycled.
However after some thought I think this is correct. You wouldn't like
to power cycle a panel while you are e.g. updating the firmware.

>
> This was the whole reason why I wanted to document that the official
> API for powering the panel was via the panel's prepare() function.
>
> [1] https://lore.kernel.org/r/CAD=FV=[e-mail]/
>
> -Doug

Doug Anderson May 5, 2022, 9:24 p.m. UTC | #16

Hi,

On Thu, May 5, 2022 at 1:56 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> On Thu, 5 May 2022 at 23:21, Doug Anderson <[e-mail]> wrote:
> >
> > Hi,
> >
> > On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov
> > <[e-mail]> wrote:
> > >
> > > On Thu, 5 May 2022 at 18:53, Doug Anderson <[e-mail]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> > > > <[e-mail]> wrote:
> > > > >
> > > > > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > > > > <[e-mail]> wrote:
> > > > > > >
> > > > > > > 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ä
> > > > > > > > > <[e-mail]> 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.?
> > > > > >
> > > > > > It's not that we can't power it on without a full modeset. It' that at
> > > > > > panel probe time all the DRM components haven't been hooked together
> > > > > > yet, so the bridge chain isn't available yet. The panel can power
> > > > > > itself on, though. This is why the documentation I added says: "if a
> > > > > > panel driver is initiating a DP AUX transfer it may power itself up
> > > > > > however it wants"
> > > > > >
> > > > > >
> > > > > > > > > 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=[e-mail]/
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > 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.
> > > > > >
> > > > > > OK, fair enough. If we can solve the PSR problem that would be great.
> > > > > >
> > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > I haven't looked at the i915 DRM code much, but my understanding is
> > > > > > that it's more of an "all in one" approach. The one driver pretty much
> > > > > > handles everything itself. That means that powering the panel up isn't
> > > > > > too hard. Is that right?
> > > > >
> > > > > Yeah, we don't have too many "helpful" abstractions in the way ;)
> > > > >
> > > > > > > > > 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.
> > > > > >
> > > > > > If it's just for debugging, I'd argue that leaving it as-is should be
> > > > > > fine. Someone poking around with their system can find a way to make
> > > > > > sure that the panel stays on.
> > > > >
> > > > > That could require altering the state of the system quite a bit, which
> > > > > may defeat the purpose.
> > > >
> > > > It does? In my experience you just need to make sure that the panel is
> > > > turned on. ...or are you saying that you'd use this for debugging a
> > > > case where the system isn't probing properly?
> > > >
> > > > If things are truly in bad shape, at least on boards using device tree
> > > > it's easy to tweak the device tree to force a regulator to stay on. I
> > > > suppose we could also add a "debugfs" entry for the panel that also
> > > > forces it to be powered on.
> > > >
> > > >
> > > > >  At least I would not be willing to accept such
> > > > > a limitation.
> > > >
> > > > Hmm, so where does that leave us? Are you against landing this patch?
> > > > I've done a lot of cleanups recently and I just don't think I have the
> > > > time to rework all the AUX transfer functions and figure out how to
> > > > power the panel. It also seems like a lot of added complexity for a
> > > > debug path.
> > >
> > > If my 2c counts, I support landing this patch. It clearly documents
> > > current behaviour and expectations.
> > >
> > > If that helps,
> > > Acked-by: Dmitry Baryshkov <[e-mail]>
> > >
> > > As for the /dev/aux, question, I think we can make the following plan work:
> > > - Document that eDP panel power up can be handled by using the
> > > pm_runtime API (which is the case for both panel-edp and atna33xc20)).
> > > I think this is a sensible requirement anyway. And both panels show
> > > how to handle different poweron/poweroff timings.
> > > - Make drm_dp_aux_dev_get_by_minor() pm_runtime_get() the attached panel.
> >
> > This matches what you suggested previously, but I still think it has a
> > potential problem as I talked about in the my previous (very long)
> > reply [1]. The relevant part was:
> >
> > > Now, despite the fact that the generic eDP panel code doesn't follow
> > > the "strict"ness I just described, the _other_ DP panel I worked on
> > > recently (samsung-atna33xc20) does. In testing we found that this
> > > panel would sometimes (like 1 in 20 times?) crash if you ever stopped
> > > outputting data to the display and then started again. You absolutely
> > > needed to fully power cycle the display each time. I tried to document
> > > this to the best of my ability in atana33xc20_unprepare(). There's
> > > also a WARN_ON() in atana33xc20_enable() trying to detect if someone
> > > is doing something the panel driver doesn't expect.
> >
> > Specifically, I think you could get in trouble if you did:
> >
> > a) drm wants to power down the panel.
> >
> > b) drm calls the panel's disable() function
> >
> > c) we start an aux transfer and grab a runtime pm reference
> >
> > d) drm calls the panel's unprepare() function => atana33xc20_unprepare()
> >
> > e) atana33xc20_unprepare()'s pm_runtime_put_sync_suspend() _won't_
> > power off the panel (we still have the reference from step c), even
> > though it needs to.
> >
> > f) we'll finish an aux transfer and, presumably, call
> > pm_runtime_put_autosuspend()
> >
> > g) drm wants to power the panel back up
> >
> > h) drm calls the panel's prepare() function, but power wasn't properly cycled
>
> Why? We'd need to extend the prepare() function with the flag
> data_was_on, which is set in the enable() and cleared in the suspend
> path. If we see this flag in the prepare() callback, we should
> forcibly power cycle the panel by toggling the regulator. Yes, this
> will cause additional wait.
>
> Another option might be to toggle the autosuspend support. Let the
> disable() call pm_runtime_dont_autosuspend() (which would turn all
> autosuspend calls into plain pm_runtime_put()) and allow it again in
> the resume(). I'm not 100% sure that this will work, but it looks like
> it should.

It turns out, though, that we _want_ autosuspend sometimes, even when
the panel is disabled. Specifically, if the panel is disabled and then
atana33xc20_get_modes() gets called then we _don't_ want to fully
power the panel off. It's expected that there will be a future call to
prepare() soon after the get_modes() and we don't want a full power
cycle (500 ms) there. This mechanism is also fully allowed by the eDP
spec. The only time we _need_ a full power cycle is after disable().

In any case, we can come up with complex ways to solve this, but I'm
still just not convinced that it's worth it. There's no valid use case
other than debugging. IMO if we're poking around and want to read DP
registers while the panel is on then it works just fine. A user doing
this can ensure that the panel is on while poking. Certainly I've done
that and it wasn't a big imposition.

If someone wants to submit patches to attempt this then I'm happy to
test them, but I feel like it's adding complexity for very little
value. The way it works now is simple / understandable and mathes my
intuition from other busses, like i2c. The bus is just responsible for
powering itself, not the devices on the bus. It has also long been
documented since commit 83127f67e450 ("drm/panel: Flesh out
kerneldoc") in 2016 that the way to turn on a panel for communication
over the command bus is via drm_panel_prepare(). I don't think we need
to change this.


> The second option leaves a possible window for the panel issues if the
> userspace AUX transfer is ongoing while the panel is being unprepared
> and then prepared again. In this case it will never be power cycled.
> However after some thought I think this is correct. You wouldn't like
> to power cycle a panel while you are e.g. updating the firmware.

As per my earlier responses, nothing we are doing here solves the
firmware update anyway. Even if we automatically power the panel for
the duration of a single aux transfer, nothing prevents the panel from
turning off between transfers. There's no API to "keep the power on
until I say stop". You certainly wouldn't want a panel to turn off
midway through a firmware update. IMO if we want to use this for
firmware update, we either need an special way to force the panel on
(in which case, we don't need to worry about it in the aux transfer
function) or (better IMO) we need to manage the firmware update in the
panel driver and prevent some type of sysfs interface to provide the
new firmware and kick off the update. Presumably having this managed
by the panel driver would be best because the panel driver could know
to, for instance, re-read the EDID after the firmware update took
place.

-Doug

Dmitry Baryshkov May 5, 2022, 10:15 p.m. UTC | #17

On 06/05/2022 00:24, Doug Anderson wrote:
> Hi,
> 
> On Thu, May 5, 2022 at 1:56 PM Dmitry Baryshkov
> <[e-mail]> wrote:
>>
>> On Thu, 5 May 2022 at 23:21, Doug Anderson <[e-mail]> wrote:
>>>
>>> Hi,
>>>
>>> On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov
>>> <[e-mail]> wrote:
>>>>
>>>> On Thu, 5 May 2022 at 18:53, Doug Anderson <[e-mail]> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
>>>>> <[e-mail]> wrote:
>>>>>>
>>>>>> On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
>>>>>>> <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> 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ä
>>>>>>>>>> <[e-mail]> 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.?
>>>>>>>
>>>>>>> It's not that we can't power it on without a full modeset. It' that at
>>>>>>> panel probe time all the DRM components haven't been hooked together
>>>>>>> yet, so the bridge chain isn't available yet. The panel can power
>>>>>>> itself on, though. This is why the documentation I added says: "if a
>>>>>>> panel driver is initiating a DP AUX transfer it may power itself up
>>>>>>> however it wants"
>>>>>>>
>>>>>>>
>>>>>>>>>> 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=[e-mail]/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> 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.
>>>>>>>
>>>>>>> OK, fair enough. If we can solve the PSR problem that would be great.
>>>>>>>
>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> I haven't looked at the i915 DRM code much, but my understanding is
>>>>>>> that it's more of an "all in one" approach. The one driver pretty much
>>>>>>> handles everything itself. That means that powering the panel up isn't
>>>>>>> too hard. Is that right?
>>>>>>
>>>>>> Yeah, we don't have too many "helpful" abstractions in the way ;)
>>>>>>
>>>>>>>>>> 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.
>>>>>>>
>>>>>>> If it's just for debugging, I'd argue that leaving it as-is should be
>>>>>>> fine. Someone poking around with their system can find a way to make
>>>>>>> sure that the panel stays on.
>>>>>>
>>>>>> That could require altering the state of the system quite a bit, which
>>>>>> may defeat the purpose.
>>>>>
>>>>> It does? In my experience you just need to make sure that the panel is
>>>>> turned on. ...or are you saying that you'd use this for debugging a
>>>>> case where the system isn't probing properly?
>>>>>
>>>>> If things are truly in bad shape, at least on boards using device tree
>>>>> it's easy to tweak the device tree to force a regulator to stay on. I
>>>>> suppose we could also add a "debugfs" entry for the panel that also
>>>>> forces it to be powered on.
>>>>>
>>>>>
>>>>>>   At least I would not be willing to accept such
>>>>>> a limitation.
>>>>>
>>>>> Hmm, so where does that leave us? Are you against landing this patch?
>>>>> I've done a lot of cleanups recently and I just don't think I have the
>>>>> time to rework all the AUX transfer functions and figure out how to
>>>>> power the panel. It also seems like a lot of added complexity for a
>>>>> debug path.
>>>>
>>>> If my 2c counts, I support landing this patch. It clearly documents
>>>> current behaviour and expectations.
>>>>
>>>> If that helps,
>>>> Acked-by: Dmitry Baryshkov <[e-mail]>
>>>>
>>>> As for the /dev/aux, question, I think we can make the following plan work:
>>>> - Document that eDP panel power up can be handled by using the
>>>> pm_runtime API (which is the case for both panel-edp and atna33xc20)).
>>>> I think this is a sensible requirement anyway. And both panels show
>>>> how to handle different poweron/poweroff timings.
>>>> - Make drm_dp_aux_dev_get_by_minor() pm_runtime_get() the attached panel.
>>>
>>> This matches what you suggested previously, but I still think it has a
>>> potential problem as I talked about in the my previous (very long)
>>> reply [1]. The relevant part was:
>>>
>>>> Now, despite the fact that the generic eDP panel code doesn't follow
>>>> the "strict"ness I just described, the _other_ DP panel I worked on
>>>> recently (samsung-atna33xc20) does. In testing we found that this
>>>> panel would sometimes (like 1 in 20 times?) crash if you ever stopped
>>>> outputting data to the display and then started again. You absolutely
>>>> needed to fully power cycle the display each time. I tried to document
>>>> this to the best of my ability in atana33xc20_unprepare(). There's
>>>> also a WARN_ON() in atana33xc20_enable() trying to detect if someone
>>>> is doing something the panel driver doesn't expect.
>>>
>>> Specifically, I think you could get in trouble if you did:
>>>
>>> a) drm wants to power down the panel.
>>>
>>> b) drm calls the panel's disable() function
>>>
>>> c) we start an aux transfer and grab a runtime pm reference
>>>
>>> d) drm calls the panel's unprepare() function => atana33xc20_unprepare()
>>>
>>> e) atana33xc20_unprepare()'s pm_runtime_put_sync_suspend() _won't_
>>> power off the panel (we still have the reference from step c), even
>>> though it needs to.
>>>
>>> f) we'll finish an aux transfer and, presumably, call
>>> pm_runtime_put_autosuspend()
>>>
>>> g) drm wants to power the panel back up
>>>
>>> h) drm calls the panel's prepare() function, but power wasn't properly cycled
>>
>> Why? We'd need to extend the prepare() function with the flag
>> data_was_on, which is set in the enable() and cleared in the suspend
>> path. If we see this flag in the prepare() callback, we should
>> forcibly power cycle the panel by toggling the regulator. Yes, this
>> will cause additional wait.
>>
>> Another option might be to toggle the autosuspend support. Let the
>> disable() call pm_runtime_dont_autosuspend() (which would turn all
>> autosuspend calls into plain pm_runtime_put()) and allow it again in
>> the resume(). I'm not 100% sure that this will work, but it looks like
>> it should.
> 
> It turns out, though, that we _want_ autosuspend sometimes, even when
> the panel is disabled. Specifically, if the panel is disabled and then
> atana33xc20_get_modes() gets called then we _don't_ want to fully
> power the panel off. It's expected that there will be a future call to
> prepare() soon after the get_modes() and we don't want a full power
> cycle (500 ms) there. This mechanism is also fully allowed by the eDP
> spec. The only time we _need_ a full power cycle is after disable().

Yes. That's why I proposed to put pm_runtime_dont_autosuspend() call to 
the disable(). So that after get_modes() there will be no power cycle, 
but only after disable() call.

> 
> In any case, we can come up with complex ways to solve this, but I'm
> still just not convinced that it's worth it. There's no valid use case
> other than debugging. IMO if we're poking around and want to read DP
> registers while the panel is on then it works just fine. A user doing
> this can ensure that the panel is on while poking. Certainly I've done
> that and it wasn't a big imposition.
> 
> If someone wants to submit patches to attempt this then I'm happy to
> test them, but I feel like it's adding complexity for very little
> value. The way it works now is simple / understandable and mathes my
> intuition from other busses, like i2c. The bus is just responsible for
> powering itself, not the devices on the bus. It has also long been
> documented since commit 83127f67e450 ("drm/panel: Flesh out
> kerneldoc") in 2016 that the way to turn on a panel for communication
> over the command bus is via drm_panel_prepare(). I don't think we need
> to change this.

Yes.


>> The second option leaves a possible window for the panel issues if the
>> userspace AUX transfer is ongoing while the panel is being unprepared
>> and then prepared again. In this case it will never be power cycled.
>> However after some thought I think this is correct. You wouldn't like
>> to power cycle a panel while you are e.g. updating the firmware.
> 
> As per my earlier responses, nothing we are doing here solves the
> firmware update anyway. Even if we automatically power the panel for
> the duration of a single aux transfer, nothing prevents the panel from
> turning off between transfers. There's no API to "keep the power on
> until I say stop". You certainly wouldn't want a panel to turn off
> midway through a firmware update. IMO if we want to use this for
> firmware update, we either need an special way to force the panel on
> (in which case, we don't need to worry about it in the aux transfer
> function) or (better IMO) we need to manage the firmware update in the
> panel driver and prevent some type of sysfs interface to provide the
> new firmware and kick off the update. Presumably having this managed
> by the panel driver would be best because the panel driver could know
> to, for instance, re-read the EDID after the firmware update took
> place.

That's why I suggested pm_runtime_get()'ting the panel from 
drm_dp_aux_dev_get_by_minor(). This way the whole userspace session will 
be protected.

But as you said, this becomes more and more complex with little added value.

Doug Anderson May 5, 2022, 10:28 p.m. UTC | #18

Hi,

On Thu, May 5, 2022 at 3:15 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> On 06/05/2022 00:24, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, May 5, 2022 at 1:56 PM Dmitry Baryshkov
> > <[e-mail]> wrote:
> >>
> >> On Thu, 5 May 2022 at 23:21, Doug Anderson <[e-mail]> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov
> >>> <[e-mail]> wrote:
> >>>>
> >>>> On Thu, 5 May 2022 at 18:53, Doug Anderson <[e-mail]> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> >>>>> <[e-mail]> wrote:
> >>>>>>
> >>>>>> On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> >>>>>>> <[e-mail]> wrote:
> >>>>>>>>
> >>>>>>>> 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ä
> >>>>>>>>>> <[e-mail]> 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.?
> >>>>>>>
> >>>>>>> It's not that we can't power it on without a full modeset. It' that at
> >>>>>>> panel probe time all the DRM components haven't been hooked together
> >>>>>>> yet, so the bridge chain isn't available yet. The panel can power
> >>>>>>> itself on, though. This is why the documentation I added says: "if a
> >>>>>>> panel driver is initiating a DP AUX transfer it may power itself up
> >>>>>>> however it wants"
> >>>>>>>
> >>>>>>>
> >>>>>>>>>> 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=[e-mail]/
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>> 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.
> >>>>>>>
> >>>>>>> OK, fair enough. If we can solve the PSR problem that would be great.
> >>>>>>>
> >>>>>>>
> >>>>>>>> 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.
> >>>>>>>
> >>>>>>> I haven't looked at the i915 DRM code much, but my understanding is
> >>>>>>> that it's more of an "all in one" approach. The one driver pretty much
> >>>>>>> handles everything itself. That means that powering the panel up isn't
> >>>>>>> too hard. Is that right?
> >>>>>>
> >>>>>> Yeah, we don't have too many "helpful" abstractions in the way ;)
> >>>>>>
> >>>>>>>>>> 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.
> >>>>>>>
> >>>>>>> If it's just for debugging, I'd argue that leaving it as-is should be
> >>>>>>> fine. Someone poking around with their system can find a way to make
> >>>>>>> sure that the panel stays on.
> >>>>>>
> >>>>>> That could require altering the state of the system quite a bit, which
> >>>>>> may defeat the purpose.
> >>>>>
> >>>>> It does? In my experience you just need to make sure that the panel is
> >>>>> turned on. ...or are you saying that you'd use this for debugging a
> >>>>> case where the system isn't probing properly?
> >>>>>
> >>>>> If things are truly in bad shape, at least on boards using device tree
> >>>>> it's easy to tweak the device tree to force a regulator to stay on. I
> >>>>> suppose we could also add a "debugfs" entry for the panel that also
> >>>>> forces it to be powered on.
> >>>>>
> >>>>>
> >>>>>>   At least I would not be willing to accept such
> >>>>>> a limitation.
> >>>>>
> >>>>> Hmm, so where does that leave us? Are you against landing this patch?
> >>>>> I've done a lot of cleanups recently and I just don't think I have the
> >>>>> time to rework all the AUX transfer functions and figure out how to
> >>>>> power the panel. It also seems like a lot of added complexity for a
> >>>>> debug path.
> >>>>
> >>>> If my 2c counts, I support landing this patch. It clearly documents
> >>>> current behaviour and expectations.
> >>>>
> >>>> If that helps,
> >>>> Acked-by: Dmitry Baryshkov <[e-mail]>
> >>>>
> >>>> As for the /dev/aux, question, I think we can make the following plan work:
> >>>> - Document that eDP panel power up can be handled by using the
> >>>> pm_runtime API (which is the case for both panel-edp and atna33xc20)).
> >>>> I think this is a sensible requirement anyway. And both panels show
> >>>> how to handle different poweron/poweroff timings.
> >>>> - Make drm_dp_aux_dev_get_by_minor() pm_runtime_get() the attached panel.
> >>>
> >>> This matches what you suggested previously, but I still think it has a
> >>> potential problem as I talked about in the my previous (very long)
> >>> reply [1]. The relevant part was:
> >>>
> >>>> Now, despite the fact that the generic eDP panel code doesn't follow
> >>>> the "strict"ness I just described, the _other_ DP panel I worked on
> >>>> recently (samsung-atna33xc20) does. In testing we found that this
> >>>> panel would sometimes (like 1 in 20 times?) crash if you ever stopped
> >>>> outputting data to the display and then started again. You absolutely
> >>>> needed to fully power cycle the display each time. I tried to document
> >>>> this to the best of my ability in atana33xc20_unprepare(). There's
> >>>> also a WARN_ON() in atana33xc20_enable() trying to detect if someone
> >>>> is doing something the panel driver doesn't expect.
> >>>
> >>> Specifically, I think you could get in trouble if you did:
> >>>
> >>> a) drm wants to power down the panel.
> >>>
> >>> b) drm calls the panel's disable() function
> >>>
> >>> c) we start an aux transfer and grab a runtime pm reference
> >>>
> >>> d) drm calls the panel's unprepare() function => atana33xc20_unprepare()
> >>>
> >>> e) atana33xc20_unprepare()'s pm_runtime_put_sync_suspend() _won't_
> >>> power off the panel (we still have the reference from step c), even
> >>> though it needs to.
> >>>
> >>> f) we'll finish an aux transfer and, presumably, call
> >>> pm_runtime_put_autosuspend()
> >>>
> >>> g) drm wants to power the panel back up
> >>>
> >>> h) drm calls the panel's prepare() function, but power wasn't properly cycled
> >>
> >> Why? We'd need to extend the prepare() function with the flag
> >> data_was_on, which is set in the enable() and cleared in the suspend
> >> path. If we see this flag in the prepare() callback, we should
> >> forcibly power cycle the panel by toggling the regulator. Yes, this
> >> will cause additional wait.
> >>
> >> Another option might be to toggle the autosuspend support. Let the
> >> disable() call pm_runtime_dont_autosuspend() (which would turn all
> >> autosuspend calls into plain pm_runtime_put()) and allow it again in
> >> the resume(). I'm not 100% sure that this will work, but it looks like
> >> it should.
> >
> > It turns out, though, that we _want_ autosuspend sometimes, even when
> > the panel is disabled. Specifically, if the panel is disabled and then
> > atana33xc20_get_modes() gets called then we _don't_ want to fully
> > power the panel off. It's expected that there will be a future call to
> > prepare() soon after the get_modes() and we don't want a full power
> > cycle (500 ms) there. This mechanism is also fully allowed by the eDP
> > spec. The only time we _need_ a full power cycle is after disable().
>
> Yes. That's why I proposed to put pm_runtime_dont_autosuspend() call to
> the disable(). So that after get_modes() there will be no power cycle,
> but only after disable() call.

Ah, I see! One issue is that it's valid to call get_modes() but then
_not_ follow it up with a subsequent power on of the panel. We can't
leave the panel in a high power state in that case. I think that would
be a problem..


> > In any case, we can come up with complex ways to solve this, but I'm
> > still just not convinced that it's worth it. There's no valid use case
> > other than debugging. IMO if we're poking around and want to read DP
> > registers while the panel is on then it works just fine. A user doing
> > this can ensure that the panel is on while poking. Certainly I've done
> > that and it wasn't a big imposition.
> >
> > If someone wants to submit patches to attempt this then I'm happy to
> > test them, but I feel like it's adding complexity for very little
> > value. The way it works now is simple / understandable and mathes my
> > intuition from other busses, like i2c. The bus is just responsible for
> > powering itself, not the devices on the bus. It has also long been
> > documented since commit 83127f67e450 ("drm/panel: Flesh out
> > kerneldoc") in 2016 that the way to turn on a panel for communication
> > over the command bus is via drm_panel_prepare(). I don't think we need
> > to change this.
>
> Yes.
>
>
> >> The second option leaves a possible window for the panel issues if the
> >> userspace AUX transfer is ongoing while the panel is being unprepared
> >> and then prepared again. In this case it will never be power cycled.
> >> However after some thought I think this is correct. You wouldn't like
> >> to power cycle a panel while you are e.g. updating the firmware.
> >
> > As per my earlier responses, nothing we are doing here solves the
> > firmware update anyway. Even if we automatically power the panel for
> > the duration of a single aux transfer, nothing prevents the panel from
> > turning off between transfers. There's no API to "keep the power on
> > until I say stop". You certainly wouldn't want a panel to turn off
> > midway through a firmware update. IMO if we want to use this for
> > firmware update, we either need an special way to force the panel on
> > (in which case, we don't need to worry about it in the aux transfer
> > function) or (better IMO) we need to manage the firmware update in the
> > panel driver and prevent some type of sysfs interface to provide the
> > new firmware and kick off the update. Presumably having this managed
> > by the panel driver would be best because the panel driver could know
> > to, for instance, re-read the EDID after the firmware update took
> > place.
>
> That's why I suggested pm_runtime_get()'ting the panel from
> drm_dp_aux_dev_get_by_minor(). This way the whole userspace session will
> be protected.
>
> But as you said, this becomes more and more complex with little added value.

Ah! I had missed the drm_dp_aux_dev_get_by_minor(). Seems like that
would work. If we ever come back to this, IMO I'd rather see it
actually end up calling the panel prepare() if we can figure out how
to do that. ...and also make sure that we're not currently in the
middle of a drm call that's turning on or turning off the panel. The
fact that (I think) we need to coordinate with DRM anyway means that I
think using the existing panel API wouldn't be that hard to do.

-Doug

Patch

diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index dca40a045dd6..e5165b708a40 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -370,9 +370,17 @@  struct drm_dp_aux {
 	 * helpers assume this is the case.
 	 *
 	 * Also note that this callback can be called no matter the
-	 * state @dev is in. Drivers that need that device to be powered
-	 * to perform this operation will first need to make sure it's
-	 * been properly enabled.
+	 * state @dev is in and also no matter what state the panel is
+	 * in. It's expected:
+	 * - If the @dev providing the AUX bus is currently unpowered then
+	 *   it will power itself up for the transfer.
+	 * - If we're on eDP and the panel is not in a state where it can
+	 *   respond (it's not powered or it's in a low power state) then this
+	 *   function will return an error (but not crash). Note that if a
+	 *   panel driver is initiating a DP AUX transfer it may power itself
+	 *   up however it wants. All other code should ensure that the
+	 *   pre_enable() bridge chain (which eventually calls the panel
+	 *   prepare function) has powered the panel.
 	 */
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);