| Message ID | 20220503162033.1.Ia8651894026707e4fa61267da944ff739610d180@changeid (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm: Document that power requirements for DP AUX transfers | expand |
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 <dianders@chromium.org> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > > 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);
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.
Hi, On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > When doing DP AUX transfers there are two actors that need to be > > powered in order for the DP AUX transfer to work: the DP source and > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > requirement on side-channel operations") added some documentation > > saying that the DP source is required to power itself up (if needed) > > to do AUX transfers. However, that commit doesn't talk anything about > > the DP sink. > > > > For full fledged DP the sink isn't really a problem. It's expected > > that if an external DP monitor isn't plugged in that attempting to do > > AUX transfers won't work. It's also expected that if a DP monitor is > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > When we're looking at eDP, however, things are less obvious. Let's add > > some documentation about expectations. Here's what we'll say: > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > panel. If an eDP panel is physically connected but powered off then it > > makes sense for the transfer to fail. > > I don't agree with this. I think the panel should just get powred up > for AUX transfers. That's definitely a fair thing to think about and I have at times thought about trying to make it work that way. It always ends up hitting a roadblock. The biggest roadblock that I recall is that to make this work then you'd have to somehow ensure that the bridge chain's pre_enable() call was made as part of the AUX transfer, right? Since the transfer function can be called in any context at all, we have to coordinate this with DRM. If, for instance, DRM is mid way through powering the panel down then we need to wait for DRM to fully finish powering down, then we need to power the panel back up. I don't believe that we can just force the panel to stay on if DRM is turning it off because of panel power sequencing requirements. At least I know it would have the potential to break "samsung-atna33xc20.c" which absolutely needs to see the panel power off after it's been disabled. We also, I believe, need to handle the fact that the bridge chain may not have even been created yet. We do AUX transfers to read the EDID and also to setup the backlight in the probe function of panel-edp. At that point the panel hasn't been linked into the chain. We had _long_ discussions [1] about moving these out of probe and decided that we could move the EDID read to be later but that it was going to really ugly to move the AUX backlight later. The backlight would end up popping up at some point in time later (the first call to panel prepare() or maybe get_modes()) and that seemed weird. [1] https://lore.kernel.org/lkml/CAD=FV=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > Otherwise you can't trust that eg. the /dev/aux > stuff is actually usable. Yeah, it's been on my mind to talk more about /dev/aux. I think /dev/aux has some problems, at least with eDP. Specifically: 1. Even if we somehow figure out how to power the panel on as part of the aux transfer, we actually _still_ not guaranteed to be able to talk to it as far as I understand. My colleague reported to me that on a system he was working with that had PSR (panel self refresh) that when the panel was powered on but in PSR mode that it wouldn't talk over AUX. Assuming that this is correct then I guess we'd also have to do even more coordination with DRM to exit PSR and block future transitions of PSR. (NOTE: it's always possible that my colleague ran into some other bug and that panels are _supposed_ to be able to talk in PSR. If you think this is the case, I can always try to dig more). 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.
On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote: > Hi, > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > > When doing DP AUX transfers there are two actors that need to be > > > powered in order for the DP AUX transfer to work: the DP source and > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > > requirement on side-channel operations") added some documentation > > > saying that the DP source is required to power itself up (if needed) > > > to do AUX transfers. However, that commit doesn't talk anything about > > > the DP sink. > > > > > > For full fledged DP the sink isn't really a problem. It's expected > > > that if an external DP monitor isn't plugged in that attempting to do > > > AUX transfers won't work. It's also expected that if a DP monitor is > > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > > > When we're looking at eDP, however, things are less obvious. Let's add > > > some documentation about expectations. Here's what we'll say: > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > > panel. If an eDP panel is physically connected but powered off then it > > > makes sense for the transfer to fail. > > > > I don't agree with this. I think the panel should just get powred up > > for AUX transfers. > > That's definitely a fair thing to think about and I have at times > thought about trying to make it work that way. It always ends up > hitting a roadblock. > > The biggest roadblock that I recall is that to make this work then > you'd have to somehow ensure that the bridge chain's pre_enable() call > was made as part of the AUX transfer, right? Since the transfer > function can be called in any context at all, we have to coordinate > this with DRM. If, for instance, DRM is mid way through powering the > panel down then we need to wait for DRM to fully finish powering down, > then we need to power the panel back up. I don't believe that we can > just force the panel to stay on if DRM is turning it off because of > panel power sequencing requirements. At least I know it would have the > potential to break "samsung-atna33xc20.c" which absolutely needs to > see the panel power off after it's been disabled. > > We also, I believe, need to handle the fact that the bridge chain may > not have even been created yet. We do AUX transfers to read the EDID > and also to setup the backlight in the probe function of panel-edp. At > that point the panel hasn't been linked into the chain. We had _long_ > discussions [1] about moving these out of probe and decided that we > could move the EDID read to be later but that it was going to really > ugly to move the AUX backlight later. The backlight would end up > popping up at some point in time later (the first call to panel > prepare() or maybe get_modes()) and that seemed weird. > > [1] > https://lore.kernel.org/lkml/CAD=FV=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > > > > Otherwise you can't trust that eg. the /dev/aux > > stuff is actually usable. > > Yeah, it's been on my mind to talk more about /dev/aux. I think > /dev/aux has some problems, at least with eDP. Specifically: > > 1. Even if we somehow figure out how to power the panel on as part of > the aux transfer, we actually _still_ not guaranteed to be able to > talk to it as far as I understand. My colleague reported to me that on > a system he was working with that had PSR (panel self refresh) that > when the panel was powered on but in PSR mode that it wouldn't talk > over AUX. Assuming that this is correct then I guess we'd also have to > do even more coordination with DRM to exit PSR and block future > transitions of PSR. (NOTE: it's always possible that my colleague ran > into some other bug and that panels are _supposed_ to be able to talk > in PSR. If you think this is the case, I can always try to dig more). TBH - the coordination with drm I don't think would be the difficult part, as we'd just need to add some sort of property (ideally invisible to userspace) that can be used in an atomic commit to disable PSR - similar to how we enable CRC readback from sysfs in the majority of DRM drivers. That being said though, I think we can just leave the work of solving this problem up to whoever ends up needing this to work. > > 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. >
On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote: > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote: > > Hi, > > > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä > > <ville.syrjala@linux.intel.com> wrote: > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > > > When doing DP AUX transfers there are two actors that need to be > > > > powered in order for the DP AUX transfer to work: the DP source and > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > > > requirement on side-channel operations") added some documentation > > > > saying that the DP source is required to power itself up (if needed) > > > > to do AUX transfers. However, that commit doesn't talk anything about > > > > the DP sink. > > > > > > > > For full fledged DP the sink isn't really a problem. It's expected > > > > that if an external DP monitor isn't plugged in that attempting to do > > > > AUX transfers won't work. It's also expected that if a DP monitor is > > > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > > > > > When we're looking at eDP, however, things are less obvious. Let's add > > > > some documentation about expectations. Here's what we'll say: > > > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > > > panel. If an eDP panel is physically connected but powered off then it > > > > makes sense for the transfer to fail. > > > > > > I don't agree with this. I think the panel should just get powred up > > > for AUX transfers. > > > > That's definitely a fair thing to think about and I have at times > > thought about trying to make it work that way. It always ends up > > hitting a roadblock. How do you even probe the panel initially if you can't power it on without doing some kind of full modeset/etc.? > > > > The biggest roadblock that I recall is that to make this work then > > you'd have to somehow ensure that the bridge chain's pre_enable() call > > was made as part of the AUX transfer, right? Since the transfer > > function can be called in any context at all, we have to coordinate > > this with DRM. If, for instance, DRM is mid way through powering the > > panel down then we need to wait for DRM to fully finish powering down, > > then we need to power the panel back up. I don't believe that we can > > just force the panel to stay on if DRM is turning it off because of > > panel power sequencing requirements. At least I know it would have the > > potential to break "samsung-atna33xc20.c" which absolutely needs to > > see the panel power off after it's been disabled. > > > > We also, I believe, need to handle the fact that the bridge chain may > > not have even been created yet. We do AUX transfers to read the EDID > > and also to setup the backlight in the probe function of panel-edp. At > > that point the panel hasn't been linked into the chain. We had _long_ > > discussions [1] about moving these out of probe and decided that we > > could move the EDID read to be later but that it was going to really > > ugly to move the AUX backlight later. The backlight would end up > > popping up at some point in time later (the first call to panel > > prepare() or maybe get_modes()) and that seemed weird. > > > > [1] > > https://lore.kernel.org/lkml/CAD=FV=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > > > > > > > Otherwise you can't trust that eg. the /dev/aux > > > stuff is actually usable. > > > > Yeah, it's been on my mind to talk more about /dev/aux. I think > > /dev/aux has some problems, at least with eDP. Specifically: > > > > 1. Even if we somehow figure out how to power the panel on as part of > > the aux transfer, we actually _still_ not guaranteed to be able to > > talk to it as far as I understand. My colleague reported to me that on > > a system he was working with that had PSR (panel self refresh) that > > when the panel was powered on but in PSR mode that it wouldn't talk > > over AUX. Assuming that this is correct then I guess we'd also have to > > do even more coordination with DRM to exit PSR and block future > > transitions of PSR. (NOTE: it's always possible that my colleague ran > > into some other bug and that panels are _supposed_ to be able to talk > > in PSR. If you think this is the case, I can always try to dig more). > > TBH - the coordination with drm I don't think would be the difficult part, as > we'd just need to add some sort of property (ideally invisible to userspace) > that can be used in an atomic commit to disable PSR - similar to how we enable > CRC readback from sysfs in the majority of DRM drivers. That being said > though, I think we can just leave the work of solving this problem up to > whoever ends up needing this to work. The driver should just disable/prevent PSR when doing AUX if the hardware can't guarantee the PSR and AUX won't interfere with each other. For i915 we have no problems with powering the panel on for AUX, but there is still a race with PSR vs. AUX because both use the same hardware internally. I've been nagging at people to fix this for i915 but I don't think it still got done :( Originally we were supposed to get a hardware mutex for this but that plan got scrapped for some reason. > > > > > 2. I'm not totally convinced that it's a great idea, at least for eDP, > > for userspace to be mucking with /dev/aux. For DP's case I guess > > /dev/aux is essentially enabling userspace drivers to do things like > > update firmware on DP monitors or play with the backlight. I guess we > > decided that we didn't want to add drivers in the kernel to handle > > this type of stuff so we left it for userspace? For eDP, though, there > > The main reason DP AUX got exposed to userspace in the first place was for > usecases like fwupd, My memory says the original reason was debugging. Or at least I had no idea fwupd had started to use this until I saw some weird looking DPCD addresses in some debug log. But I suppose it's possible there were already plans for firmware updates and whatnot and it just wasn't being discussed when this was being developed.
Hi, On Wed, May 4, 2022 at 11:10 AM Lyude Paul <lyude@redhat.com> wrote: > > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote: > > Hi, > > > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä > > <ville.syrjala@linux.intel.com> wrote: > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > > > When doing DP AUX transfers there are two actors that need to be > > > > powered in order for the DP AUX transfer to work: the DP source and > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > > > requirement on side-channel operations") added some documentation > > > > saying that the DP source is required to power itself up (if needed) > > > > to do AUX transfers. However, that commit doesn't talk anything about > > > > the DP sink. > > > > > > > > For full fledged DP the sink isn't really a problem. It's expected > > > > that if an external DP monitor isn't plugged in that attempting to do > > > > AUX transfers won't work. It's also expected that if a DP monitor is > > > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > > > > > When we're looking at eDP, however, things are less obvious. Let's add > > > > some documentation about expectations. Here's what we'll say: > > > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > > > panel. If an eDP panel is physically connected but powered off then it > > > > makes sense for the transfer to fail. > > > > > > I don't agree with this. I think the panel should just get powred up > > > for AUX transfers. > > > > That's definitely a fair thing to think about and I have at times > > thought about trying to make it work that way. It always ends up > > hitting a roadblock. > > > > The biggest roadblock that I recall is that to make this work then > > you'd have to somehow ensure that the bridge chain's pre_enable() call > > was made as part of the AUX transfer, right? Since the transfer > > function can be called in any context at all, we have to coordinate > > this with DRM. If, for instance, DRM is mid way through powering the > > panel down then we need to wait for DRM to fully finish powering down, > > then we need to power the panel back up. I don't believe that we can > > just force the panel to stay on if DRM is turning it off because of > > panel power sequencing requirements. At least I know it would have the > > potential to break "samsung-atna33xc20.c" which absolutely needs to > > see the panel power off after it's been disabled. > > > > We also, I believe, need to handle the fact that the bridge chain may > > not have even been created yet. We do AUX transfers to read the EDID > > and also to setup the backlight in the probe function of panel-edp. At > > that point the panel hasn't been linked into the chain. We had _long_ > > discussions [1] about moving these out of probe and decided that we > > could move the EDID read to be later but that it was going to really > > ugly to move the AUX backlight later. The backlight would end up > > popping up at some point in time later (the first call to panel > > prepare() or maybe get_modes()) and that seemed weird. > > > > [1] > > https://lore.kernel.org/lkml/CAD=FV=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > > > > > > > Otherwise you can't trust that eg. the /dev/aux > > > stuff is actually usable. > > > > Yeah, it's been on my mind to talk more about /dev/aux. I think > > /dev/aux has some problems, at least with eDP. Specifically: > > > > 1. Even if we somehow figure out how to power the panel on as part of > > the aux transfer, we actually _still_ not guaranteed to be able to > > talk to it as far as I understand. My colleague reported to me that on > > a system he was working with that had PSR (panel self refresh) that > > when the panel was powered on but in PSR mode that it wouldn't talk > > over AUX. Assuming that this is correct then I guess we'd also have to > > do even more coordination with DRM to exit PSR and block future > > transitions of PSR. (NOTE: it's always possible that my colleague ran > > into some other bug and that panels are _supposed_ to be able to talk > > in PSR. If you think this is the case, I can always try to dig more). > > TBH - the coordination with drm I don't think would be the difficult part, as > we'd just need to add some sort of property (ideally invisible to userspace) > that can be used in an atomic commit to disable PSR - similar to how we enable > CRC readback from sysfs in the majority of DRM drivers. That being said > though, I think we can just leave the work of solving this problem up to > whoever ends up needing this to work. > > > > > 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.
Hi, On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä <ville.syrjala@linux.intel.com> 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ä > > > <ville.syrjala@linux.intel.com> wrote: > > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > > > > When doing DP AUX transfers there are two actors that need to be > > > > > powered in order for the DP AUX transfer to work: the DP source and > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > > > > requirement on side-channel operations") added some documentation > > > > > saying that the DP source is required to power itself up (if needed) > > > > > to do AUX transfers. However, that commit doesn't talk anything about > > > > > the DP sink. > > > > > > > > > > For full fledged DP the sink isn't really a problem. It's expected > > > > > that if an external DP monitor isn't plugged in that attempting to do > > > > > AUX transfers won't work. It's also expected that if a DP monitor is > > > > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > > > > > > > When we're looking at eDP, however, things are less obvious. Let's add > > > > > some documentation about expectations. Here's what we'll say: > > > > > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > > > > panel. If an eDP panel is physically connected but powered off then it > > > > > makes sense for the transfer to fail. > > > > > > > > I don't agree with this. I think the panel should just get powred up > > > > for AUX transfers. > > > > > > That's definitely a fair thing to think about and I have at times > > > thought about trying to make it work that way. It always ends up > > > hitting a roadblock. > > How do you even probe the panel initially if you can't power it on > without doing some kind of full modeset/etc.? 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=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > > > > > > > > > > Otherwise you can't trust that eg. the /dev/aux > > > > stuff is actually usable. > > > > > > Yeah, it's been on my mind to talk more about /dev/aux. I think > > > /dev/aux has some problems, at least with eDP. Specifically: > > > > > > 1. Even if we somehow figure out how to power the panel on as part of > > > the aux transfer, we actually _still_ not guaranteed to be able to > > > talk to it as far as I understand. My colleague reported to me that on > > > a system he was working with that had PSR (panel self refresh) that > > > when the panel was powered on but in PSR mode that it wouldn't talk > > > over AUX. Assuming that this is correct then I guess we'd also have to > > > do even more coordination with DRM to exit PSR and block future > > > transitions of PSR. (NOTE: it's always possible that my colleague ran > > > into some other bug and that panels are _supposed_ to be able to talk > > > in PSR. If you think this is the case, I can always try to dig more). > > > > TBH - the coordination with drm I don't think would be the difficult part, as > > we'd just need to add some sort of property (ideally invisible to userspace) > > that can be used in an atomic commit to disable PSR - similar to how we enable > > CRC readback from sysfs in the majority of DRM drivers. That being said > > though, I think we can just leave the work of solving this problem up to > > whoever ends up needing this to work. > > The driver should just disable/prevent PSR when doing AUX if the hardware > can't guarantee the PSR and AUX won't interfere with each other. 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
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ä > <ville.syrjala@linux.intel.com> 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ä > > > > <ville.syrjala@linux.intel.com> wrote: > > > > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > > > > > When doing DP AUX transfers there are two actors that need to be > > > > > > powered in order for the DP AUX transfer to work: the DP source and > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > > > > > requirement on side-channel operations") added some documentation > > > > > > saying that the DP source is required to power itself up (if needed) > > > > > > to do AUX transfers. However, that commit doesn't talk anything about > > > > > > the DP sink. > > > > > > > > > > > > For full fledged DP the sink isn't really a problem. It's expected > > > > > > that if an external DP monitor isn't plugged in that attempting to do > > > > > > AUX transfers won't work. It's also expected that if a DP monitor is > > > > > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > > > > > > > > > When we're looking at eDP, however, things are less obvious. Let's add > > > > > > some documentation about expectations. Here's what we'll say: > > > > > > > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > > > > > panel. If an eDP panel is physically connected but powered off then it > > > > > > makes sense for the transfer to fail. > > > > > > > > > > I don't agree with this. I think the panel should just get powred up > > > > > for AUX transfers. > > > > > > > > That's definitely a fair thing to think about and I have at times > > > > thought about trying to make it work that way. It always ends up > > > > hitting a roadblock. > > > > How do you even probe the panel initially if you can't power it on > > without doing some kind of full modeset/etc.? > > 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=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > > > > > > > > > > > > > Otherwise you can't trust that eg. the /dev/aux > > > > > stuff is actually usable. > > > > > > > > Yeah, it's been on my mind to talk more about /dev/aux. I think > > > > /dev/aux has some problems, at least with eDP. Specifically: > > > > > > > > 1. Even if we somehow figure out how to power the panel on as part of > > > > the aux transfer, we actually _still_ not guaranteed to be able to > > > > talk to it as far as I understand. My colleague reported to me that on > > > > a system he was working with that had PSR (panel self refresh) that > > > > when the panel was powered on but in PSR mode that it wouldn't talk > > > > over AUX. Assuming that this is correct then I guess we'd also have to > > > > do even more coordination with DRM to exit PSR and block future > > > > transitions of PSR. (NOTE: it's always possible that my colleague ran > > > > into some other bug and that panels are _supposed_ to be able to talk > > > > in PSR. If you think this is the case, I can always try to dig more). > > > > > > TBH - the coordination with drm I don't think would be the difficult part, as > > > we'd just need to add some sort of property (ideally invisible to userspace) > > > that can be used in an atomic commit to disable PSR - similar to how we enable > > > CRC readback from sysfs in the majority of DRM drivers. That being said > > > though, I think we can just leave the work of solving this problem up to > > > whoever ends up needing this to work. > > > > The driver should just disable/prevent PSR when doing AUX if the hardware > > can't guarantee the PSR and AUX won't interfere with each other. > > 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
Hi, On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä <ville.syrjala@linux.intel.com> 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ä > > <ville.syrjala@linux.intel.com> 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ä > > > > > <ville.syrjala@linux.intel.com> wrote: > > > > > > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > > > > > > When doing DP AUX transfers there are two actors that need to be > > > > > > > powered in order for the DP AUX transfer to work: the DP source and > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > > > > > > requirement on side-channel operations") added some documentation > > > > > > > saying that the DP source is required to power itself up (if needed) > > > > > > > to do AUX transfers. However, that commit doesn't talk anything about > > > > > > > the DP sink. > > > > > > > > > > > > > > For full fledged DP the sink isn't really a problem. It's expected > > > > > > > that if an external DP monitor isn't plugged in that attempting to do > > > > > > > AUX transfers won't work. It's also expected that if a DP monitor is > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > > > > > > > > > > > When we're looking at eDP, however, things are less obvious. Let's add > > > > > > > some documentation about expectations. Here's what we'll say: > > > > > > > > > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > > > > > > panel. If an eDP panel is physically connected but powered off then it > > > > > > > makes sense for the transfer to fail. > > > > > > > > > > > > I don't agree with this. I think the panel should just get powred up > > > > > > for AUX transfers. > > > > > > > > > > That's definitely a fair thing to think about and I have at times > > > > > thought about trying to make it work that way. It always ends up > > > > > hitting a roadblock. > > > > > > How do you even probe the panel initially if you can't power it on > > > without doing some kind of full modeset/etc.? > > > > 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=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > > > > > > > > > > > > > > > > Otherwise you can't trust that eg. the /dev/aux > > > > > > stuff is actually usable. > > > > > > > > > > Yeah, it's been on my mind to talk more about /dev/aux. I think > > > > > /dev/aux has some problems, at least with eDP. Specifically: > > > > > > > > > > 1. Even if we somehow figure out how to power the panel on as part of > > > > > the aux transfer, we actually _still_ not guaranteed to be able to > > > > > talk to it as far as I understand. My colleague reported to me that on > > > > > a system he was working with that had PSR (panel self refresh) that > > > > > when the panel was powered on but in PSR mode that it wouldn't talk > > > > > over AUX. Assuming that this is correct then I guess we'd also have to > > > > > do even more coordination with DRM to exit PSR and block future > > > > > transitions of PSR. (NOTE: it's always possible that my colleague ran > > > > > into some other bug and that panels are _supposed_ to be able to talk > > > > > in PSR. If you think this is the case, I can always try to dig more). > > > > > > > > TBH - the coordination with drm I don't think would be the difficult part, as > > > > we'd just need to add some sort of property (ideally invisible to userspace) > > > > that can be used in an atomic commit to disable PSR - similar to how we enable > > > > CRC readback from sysfs in the majority of DRM drivers. That being said > > > > though, I think we can just leave the work of solving this problem up to > > > > whoever ends up needing this to work. > > > > > > The driver should just disable/prevent PSR when doing AUX if the hardware > > > can't guarantee the PSR and AUX won't interfere with each other. > > > > 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
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ä > <ville.syrjala@linux.intel.com> 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ä > > > <ville.syrjala@linux.intel.com> 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ä > > > > > > <ville.syrjala@linux.intel.com> wrote: > > > > > > > > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > > > > > > > When doing DP AUX transfers there are two actors that need to be > > > > > > > > powered in order for the DP AUX transfer to work: the DP source and > > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > > > > > > > requirement on side-channel operations") added some documentation > > > > > > > > saying that the DP source is required to power itself up (if needed) > > > > > > > > to do AUX transfers. However, that commit doesn't talk anything about > > > > > > > > the DP sink. > > > > > > > > > > > > > > > > For full fledged DP the sink isn't really a problem. It's expected > > > > > > > > that if an external DP monitor isn't plugged in that attempting to do > > > > > > > > AUX transfers won't work. It's also expected that if a DP monitor is > > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > > > > > > > > > > > > > When we're looking at eDP, however, things are less obvious. Let's add > > > > > > > > some documentation about expectations. Here's what we'll say: > > > > > > > > > > > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > > > > > > > panel. If an eDP panel is physically connected but powered off then it > > > > > > > > makes sense for the transfer to fail. > > > > > > > > > > > > > > I don't agree with this. I think the panel should just get powred up > > > > > > > for AUX transfers. > > > > > > > > > > > > That's definitely a fair thing to think about and I have at times > > > > > > thought about trying to make it work that way. It always ends up > > > > > > hitting a roadblock. > > > > > > > > How do you even probe the panel initially if you can't power it on > > > > without doing some kind of full modeset/etc.? > > > > > > 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=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > > > > > > > > > > > > > > > > > > > Otherwise you can't trust that eg. the /dev/aux > > > > > > > stuff is actually usable. > > > > > > > > > > > > Yeah, it's been on my mind to talk more about /dev/aux. I think > > > > > > /dev/aux has some problems, at least with eDP. Specifically: > > > > > > > > > > > > 1. Even if we somehow figure out how to power the panel on as part of > > > > > > the aux transfer, we actually _still_ not guaranteed to be able to > > > > > > talk to it as far as I understand. My colleague reported to me that on > > > > > > a system he was working with that had PSR (panel self refresh) that > > > > > > when the panel was powered on but in PSR mode that it wouldn't talk > > > > > > over AUX. Assuming that this is correct then I guess we'd also have to > > > > > > do even more coordination with DRM to exit PSR and block future > > > > > > transitions of PSR. (NOTE: it's always possible that my colleague ran > > > > > > into some other bug and that panels are _supposed_ to be able to talk > > > > > > in PSR. If you think this is the case, I can always try to dig more). > > > > > > > > > > TBH - the coordination with drm I don't think would be the difficult part, as > > > > > we'd just need to add some sort of property (ideally invisible to userspace) > > > > > that can be used in an atomic commit to disable PSR - similar to how we enable > > > > > CRC readback from sysfs in the majority of DRM drivers. That being said > > > > > though, I think we can just leave the work of solving this problem up to > > > > > whoever ends up needing this to work. > > > > > > > > The driver should just disable/prevent PSR when doing AUX if the hardware > > > > can't guarantee the PSR and AUX won't interfere with each other. > > > > > > 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.
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 <lyude@redhat.com> 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 <dianders@chromium.org> > --- > > 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);
On Thu, 5 May 2022 at 18:53, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä > <ville.syrjala@linux.intel.com> 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ä > > > <ville.syrjala@linux.intel.com> 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ä > > > > > > <ville.syrjala@linux.intel.com> wrote: > > > > > > > > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > > > > > > > When doing DP AUX transfers there are two actors that need to be > > > > > > > > powered in order for the DP AUX transfer to work: the DP source and > > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > > > > > > > requirement on side-channel operations") added some documentation > > > > > > > > saying that the DP source is required to power itself up (if needed) > > > > > > > > to do AUX transfers. However, that commit doesn't talk anything about > > > > > > > > the DP sink. > > > > > > > > > > > > > > > > For full fledged DP the sink isn't really a problem. It's expected > > > > > > > > that if an external DP monitor isn't plugged in that attempting to do > > > > > > > > AUX transfers won't work. It's also expected that if a DP monitor is > > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > > > > > > > > > > > > > When we're looking at eDP, however, things are less obvious. Let's add > > > > > > > > some documentation about expectations. Here's what we'll say: > > > > > > > > > > > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > > > > > > > panel. If an eDP panel is physically connected but powered off then it > > > > > > > > makes sense for the transfer to fail. > > > > > > > > > > > > > > I don't agree with this. I think the panel should just get powred up > > > > > > > for AUX transfers. > > > > > > > > > > > > That's definitely a fair thing to think about and I have at times > > > > > > thought about trying to make it work that way. It always ends up > > > > > > hitting a roadblock. > > > > > > > > How do you even probe the panel initially if you can't power it on > > > > without doing some kind of full modeset/etc.? > > > > > > 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=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > > > > > > > > > > > > > > > > > > > Otherwise you can't trust that eg. the /dev/aux > > > > > > > stuff is actually usable. > > > > > > > > > > > > Yeah, it's been on my mind to talk more about /dev/aux. I think > > > > > > /dev/aux has some problems, at least with eDP. Specifically: > > > > > > > > > > > > 1. Even if we somehow figure out how to power the panel on as part of > > > > > > the aux transfer, we actually _still_ not guaranteed to be able to > > > > > > talk to it as far as I understand. My colleague reported to me that on > > > > > > a system he was working with that had PSR (panel self refresh) that > > > > > > when the panel was powered on but in PSR mode that it wouldn't talk > > > > > > over AUX. Assuming that this is correct then I guess we'd also have to > > > > > > do even more coordination with DRM to exit PSR and block future > > > > > > transitions of PSR. (NOTE: it's always possible that my colleague ran > > > > > > into some other bug and that panels are _supposed_ to be able to talk > > > > > > in PSR. If you think this is the case, I can always try to dig more). > > > > > > > > > > TBH - the coordination with drm I don't think would be the difficult part, as > > > > > we'd just need to add some sort of property (ideally invisible to userspace) > > > > > that can be used in an atomic commit to disable PSR - similar to how we enable > > > > > CRC readback from sysfs in the majority of DRM drivers. That being said > > > > > though, I think we can just leave the work of solving this problem up to > > > > > whoever ends up needing this to work. > > > > > > > > The driver should just disable/prevent PSR when doing AUX if the hardware > > > > can't guarantee the PSR and AUX won't interfere with each other. > > > > > > 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 <dmitry.baryshkov@linaro.org> 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.
Hi, On Thu, May 5, 2022 at 12:19 PM Ville Syrjälä <ville.syrjala@linux.intel.com> 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ä > > <ville.syrjala@linux.intel.com> 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ä > > > > <ville.syrjala@linux.intel.com> 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ä > > > > > > > <ville.syrjala@linux.intel.com> wrote: > > > > > > > > > > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > > > > > > > > When doing DP AUX transfers there are two actors that need to be > > > > > > > > > powered in order for the DP AUX transfer to work: the DP source and > > > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > > > > > > > > requirement on side-channel operations") added some documentation > > > > > > > > > saying that the DP source is required to power itself up (if needed) > > > > > > > > > to do AUX transfers. However, that commit doesn't talk anything about > > > > > > > > > the DP sink. > > > > > > > > > > > > > > > > > > For full fledged DP the sink isn't really a problem. It's expected > > > > > > > > > that if an external DP monitor isn't plugged in that attempting to do > > > > > > > > > AUX transfers won't work. It's also expected that if a DP monitor is > > > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > > > > > > > > > > > > > > > When we're looking at eDP, however, things are less obvious. Let's add > > > > > > > > > some documentation about expectations. Here's what we'll say: > > > > > > > > > > > > > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > > > > > > > > panel. If an eDP panel is physically connected but powered off then it > > > > > > > > > makes sense for the transfer to fail. > > > > > > > > > > > > > > > > I don't agree with this. I think the panel should just get powred up > > > > > > > > for AUX transfers. > > > > > > > > > > > > > > That's definitely a fair thing to think about and I have at times > > > > > > > thought about trying to make it work that way. It always ends up > > > > > > > hitting a roadblock. > > > > > > > > > > How do you even probe the panel initially if you can't power it on > > > > > without doing some kind of full modeset/etc.? > > > > > > > > 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=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > > > > > > > > > > > > > > > > > > > > > > Otherwise you can't trust that eg. the /dev/aux > > > > > > > > stuff is actually usable. > > > > > > > > > > > > > > Yeah, it's been on my mind to talk more about /dev/aux. I think > > > > > > > /dev/aux has some problems, at least with eDP. Specifically: > > > > > > > > > > > > > > 1. Even if we somehow figure out how to power the panel on as part of > > > > > > > the aux transfer, we actually _still_ not guaranteed to be able to > > > > > > > talk to it as far as I understand. My colleague reported to me that on > > > > > > > a system he was working with that had PSR (panel self refresh) that > > > > > > > when the panel was powered on but in PSR mode that it wouldn't talk > > > > > > > over AUX. Assuming that this is correct then I guess we'd also have to > > > > > > > do even more coordination with DRM to exit PSR and block future > > > > > > > transitions of PSR. (NOTE: it's always possible that my colleague ran > > > > > > > into some other bug and that panels are _supposed_ to be able to talk > > > > > > > in PSR. If you think this is the case, I can always try to dig more). > > > > > > > > > > > > TBH - the coordination with drm I don't think would be the difficult part, as > > > > > > we'd just need to add some sort of property (ideally invisible to userspace) > > > > > > that can be used in an atomic commit to disable PSR - similar to how we enable > > > > > > CRC readback from sysfs in the majority of DRM drivers. That being said > > > > > > though, I think we can just leave the work of solving this problem up to > > > > > > whoever ends up needing this to work. > > > > > > > > > > The driver should just disable/prevent PSR when doing AUX if the hardware > > > > > can't guarantee the PSR and AUX won't interfere with each other. > > > > > > > > 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
Hi, On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Thu, 5 May 2022 at 18:53, Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä > > <ville.syrjala@linux.intel.com> 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ä > > > > <ville.syrjala@linux.intel.com> 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ä > > > > > > > <ville.syrjala@linux.intel.com> wrote: > > > > > > > > > > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > > > > > > > > When doing DP AUX transfers there are two actors that need to be > > > > > > > > > powered in order for the DP AUX transfer to work: the DP source and > > > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > > > > > > > > requirement on side-channel operations") added some documentation > > > > > > > > > saying that the DP source is required to power itself up (if needed) > > > > > > > > > to do AUX transfers. However, that commit doesn't talk anything about > > > > > > > > > the DP sink. > > > > > > > > > > > > > > > > > > For full fledged DP the sink isn't really a problem. It's expected > > > > > > > > > that if an external DP monitor isn't plugged in that attempting to do > > > > > > > > > AUX transfers won't work. It's also expected that if a DP monitor is > > > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > > > > > > > > > > > > > > > When we're looking at eDP, however, things are less obvious. Let's add > > > > > > > > > some documentation about expectations. Here's what we'll say: > > > > > > > > > > > > > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > > > > > > > > panel. If an eDP panel is physically connected but powered off then it > > > > > > > > > makes sense for the transfer to fail. > > > > > > > > > > > > > > > > I don't agree with this. I think the panel should just get powred up > > > > > > > > for AUX transfers. > > > > > > > > > > > > > > That's definitely a fair thing to think about and I have at times > > > > > > > thought about trying to make it work that way. It always ends up > > > > > > > hitting a roadblock. > > > > > > > > > > How do you even probe the panel initially if you can't power it on > > > > > without doing some kind of full modeset/etc.? > > > > > > > > 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=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > > > > > > > > > > > > > > > > > > > > > > Otherwise you can't trust that eg. the /dev/aux > > > > > > > > stuff is actually usable. > > > > > > > > > > > > > > Yeah, it's been on my mind to talk more about /dev/aux. I think > > > > > > > /dev/aux has some problems, at least with eDP. Specifically: > > > > > > > > > > > > > > 1. Even if we somehow figure out how to power the panel on as part of > > > > > > > the aux transfer, we actually _still_ not guaranteed to be able to > > > > > > > talk to it as far as I understand. My colleague reported to me that on > > > > > > > a system he was working with that had PSR (panel self refresh) that > > > > > > > when the panel was powered on but in PSR mode that it wouldn't talk > > > > > > > over AUX. Assuming that this is correct then I guess we'd also have to > > > > > > > do even more coordination with DRM to exit PSR and block future > > > > > > > transitions of PSR. (NOTE: it's always possible that my colleague ran > > > > > > > into some other bug and that panels are _supposed_ to be able to talk > > > > > > > in PSR. If you think this is the case, I can always try to dig more). > > > > > > > > > > > > TBH - the coordination with drm I don't think would be the difficult part, as > > > > > > we'd just need to add some sort of property (ideally invisible to userspace) > > > > > > that can be used in an atomic commit to disable PSR - similar to how we enable > > > > > > CRC readback from sysfs in the majority of DRM drivers. That being said > > > > > > though, I think we can just leave the work of solving this problem up to > > > > > > whoever ends up needing this to work. > > > > > > > > > > The driver should just disable/prevent PSR when doing AUX if the hardware > > > > > can't guarantee the PSR and AUX won't interfere with each other. > > > > > > > > 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 <dmitry.baryshkov@linaro.org> > > 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=UmXzPyVOa-Y0gpY0qcukqW3ge5DBPx6ak88ydEqTsBiQ@mail.gmail.com/ -Doug
On Thu, 5 May 2022 at 23:21, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Thu, 5 May 2022 at 18:53, Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä > > > <ville.syrjala@linux.intel.com> 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ä > > > > > <ville.syrjala@linux.intel.com> 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ä > > > > > > > > <ville.syrjala@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > > > > > > > > > When doing DP AUX transfers there are two actors that need to be > > > > > > > > > > powered in order for the DP AUX transfer to work: the DP source and > > > > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > > > > > > > > > requirement on side-channel operations") added some documentation > > > > > > > > > > saying that the DP source is required to power itself up (if needed) > > > > > > > > > > to do AUX transfers. However, that commit doesn't talk anything about > > > > > > > > > > the DP sink. > > > > > > > > > > > > > > > > > > > > For full fledged DP the sink isn't really a problem. It's expected > > > > > > > > > > that if an external DP monitor isn't plugged in that attempting to do > > > > > > > > > > AUX transfers won't work. It's also expected that if a DP monitor is > > > > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > > > > > > > > > > > > > > > > > When we're looking at eDP, however, things are less obvious. Let's add > > > > > > > > > > some documentation about expectations. Here's what we'll say: > > > > > > > > > > > > > > > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > > > > > > > > > panel. If an eDP panel is physically connected but powered off then it > > > > > > > > > > makes sense for the transfer to fail. > > > > > > > > > > > > > > > > > > I don't agree with this. I think the panel should just get powred up > > > > > > > > > for AUX transfers. > > > > > > > > > > > > > > > > That's definitely a fair thing to think about and I have at times > > > > > > > > thought about trying to make it work that way. It always ends up > > > > > > > > hitting a roadblock. > > > > > > > > > > > > How do you even probe the panel initially if you can't power it on > > > > > > without doing some kind of full modeset/etc.? > > > > > > > > > > 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=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > > > > > > > > > > > > > > > > > > > > > > > > > Otherwise you can't trust that eg. the /dev/aux > > > > > > > > > stuff is actually usable. > > > > > > > > > > > > > > > > Yeah, it's been on my mind to talk more about /dev/aux. I think > > > > > > > > /dev/aux has some problems, at least with eDP. Specifically: > > > > > > > > > > > > > > > > 1. Even if we somehow figure out how to power the panel on as part of > > > > > > > > the aux transfer, we actually _still_ not guaranteed to be able to > > > > > > > > talk to it as far as I understand. My colleague reported to me that on > > > > > > > > a system he was working with that had PSR (panel self refresh) that > > > > > > > > when the panel was powered on but in PSR mode that it wouldn't talk > > > > > > > > over AUX. Assuming that this is correct then I guess we'd also have to > > > > > > > > do even more coordination with DRM to exit PSR and block future > > > > > > > > transitions of PSR. (NOTE: it's always possible that my colleague ran > > > > > > > > into some other bug and that panels are _supposed_ to be able to talk > > > > > > > > in PSR. If you think this is the case, I can always try to dig more). > > > > > > > > > > > > > > TBH - the coordination with drm I don't think would be the difficult part, as > > > > > > > we'd just need to add some sort of property (ideally invisible to userspace) > > > > > > > that can be used in an atomic commit to disable PSR - similar to how we enable > > > > > > > CRC readback from sysfs in the majority of DRM drivers. That being said > > > > > > > though, I think we can just leave the work of solving this problem up to > > > > > > > whoever ends up needing this to work. > > > > > > > > > > > > The driver should just disable/prevent PSR when doing AUX if the hardware > > > > > > can't guarantee the PSR and AUX won't interfere with each other. > > > > > > > > > > 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 <dmitry.baryshkov@linaro.org> > > > > 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=UmXzPyVOa-Y0gpY0qcukqW3ge5DBPx6ak88ydEqTsBiQ@mail.gmail.com/ > > -Doug
Hi, On Thu, May 5, 2022 at 1:56 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Thu, 5 May 2022 at 23:21, Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On Thu, 5 May 2022 at 18:53, Doug Anderson <dianders@chromium.org> wrote: > > > > > > > > Hi, > > > > > > > > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä > > > > <ville.syrjala@linux.intel.com> 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ä > > > > > > <ville.syrjala@linux.intel.com> 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ä > > > > > > > > > <ville.syrjala@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > > > > > > > > > > > When doing DP AUX transfers there are two actors that need to be > > > > > > > > > > > powered in order for the DP AUX transfer to work: the DP source and > > > > > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state > > > > > > > > > > > requirement on side-channel operations") added some documentation > > > > > > > > > > > saying that the DP source is required to power itself up (if needed) > > > > > > > > > > > to do AUX transfers. However, that commit doesn't talk anything about > > > > > > > > > > > the DP sink. > > > > > > > > > > > > > > > > > > > > > > For full fledged DP the sink isn't really a problem. It's expected > > > > > > > > > > > that if an external DP monitor isn't plugged in that attempting to do > > > > > > > > > > > AUX transfers won't work. It's also expected that if a DP monitor is > > > > > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers will work. > > > > > > > > > > > > > > > > > > > > > > When we're looking at eDP, however, things are less obvious. Let's add > > > > > > > > > > > some documentation about expectations. Here's what we'll say: > > > > > > > > > > > > > > > > > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP > > > > > > > > > > > panel. If an eDP panel is physically connected but powered off then it > > > > > > > > > > > makes sense for the transfer to fail. > > > > > > > > > > > > > > > > > > > > I don't agree with this. I think the panel should just get powred up > > > > > > > > > > for AUX transfers. > > > > > > > > > > > > > > > > > > That's definitely a fair thing to think about and I have at times > > > > > > > > > thought about trying to make it work that way. It always ends up > > > > > > > > > hitting a roadblock. > > > > > > > > > > > > > > How do you even probe the panel initially if you can't power it on > > > > > > > without doing some kind of full modeset/etc.? > > > > > > > > > > > > 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=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > > > > > > > > > > > > > > > > > > > > > > > > > > > > Otherwise you can't trust that eg. the /dev/aux > > > > > > > > > > stuff is actually usable. > > > > > > > > > > > > > > > > > > Yeah, it's been on my mind to talk more about /dev/aux. I think > > > > > > > > > /dev/aux has some problems, at least with eDP. Specifically: > > > > > > > > > > > > > > > > > > 1. Even if we somehow figure out how to power the panel on as part of > > > > > > > > > the aux transfer, we actually _still_ not guaranteed to be able to > > > > > > > > > talk to it as far as I understand. My colleague reported to me that on > > > > > > > > > a system he was working with that had PSR (panel self refresh) that > > > > > > > > > when the panel was powered on but in PSR mode that it wouldn't talk > > > > > > > > > over AUX. Assuming that this is correct then I guess we'd also have to > > > > > > > > > do even more coordination with DRM to exit PSR and block future > > > > > > > > > transitions of PSR. (NOTE: it's always possible that my colleague ran > > > > > > > > > into some other bug and that panels are _supposed_ to be able to talk > > > > > > > > > in PSR. If you think this is the case, I can always try to dig more). > > > > > > > > > > > > > > > > TBH - the coordination with drm I don't think would be the difficult part, as > > > > > > > > we'd just need to add some sort of property (ideally invisible to userspace) > > > > > > > > that can be used in an atomic commit to disable PSR - similar to how we enable > > > > > > > > CRC readback from sysfs in the majority of DRM drivers. That being said > > > > > > > > though, I think we can just leave the work of solving this problem up to > > > > > > > > whoever ends up needing this to work. > > > > > > > > > > > > > > The driver should just disable/prevent PSR when doing AUX if the hardware > > > > > > > can't guarantee the PSR and AUX won't interfere with each other. > > > > > > > > > > > > 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 <dmitry.baryshkov@linaro.org> > > > > > > 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
On 06/05/2022 00:24, Doug Anderson wrote: > Hi, > > On Thu, May 5, 2022 at 1:56 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> >> On Thu, 5 May 2022 at 23:21, Doug Anderson <dianders@chromium.org> wrote: >>> >>> Hi, >>> >>> On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov >>> <dmitry.baryshkov@linaro.org> wrote: >>>> >>>> On Thu, 5 May 2022 at 18:53, Doug Anderson <dianders@chromium.org> wrote: >>>>> >>>>> Hi, >>>>> >>>>> On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä >>>>> <ville.syrjala@linux.intel.com> 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ä >>>>>>> <ville.syrjala@linux.intel.com> 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ä >>>>>>>>>> <ville.syrjala@linux.intel.com> wrote: >>>>>>>>>>> >>>>>>>>>>> On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: >>>>>>>>>>>> When doing DP AUX transfers there are two actors that need to be >>>>>>>>>>>> powered in order for the DP AUX transfer to work: the DP source and >>>>>>>>>>>> the DP sync. Commit bacbab58f09d ("drm: Mention the power state >>>>>>>>>>>> requirement on side-channel operations") added some documentation >>>>>>>>>>>> saying that the DP source is required to power itself up (if needed) >>>>>>>>>>>> to do AUX transfers. However, that commit doesn't talk anything about >>>>>>>>>>>> the DP sink. >>>>>>>>>>>> >>>>>>>>>>>> For full fledged DP the sink isn't really a problem. It's expected >>>>>>>>>>>> that if an external DP monitor isn't plugged in that attempting to do >>>>>>>>>>>> AUX transfers won't work. It's also expected that if a DP monitor is >>>>>>>>>>>> plugged in (and thus asserting HPD) that it AUX transfers will work. >>>>>>>>>>>> >>>>>>>>>>>> When we're looking at eDP, however, things are less obvious. Let's add >>>>>>>>>>>> some documentation about expectations. Here's what we'll say: >>>>>>>>>>>> >>>>>>>>>>>> 1. We don't expect the DP AUX transfer function to power on an eDP >>>>>>>>>>>> panel. If an eDP panel is physically connected but powered off then it >>>>>>>>>>>> makes sense for the transfer to fail. >>>>>>>>>>> >>>>>>>>>>> I don't agree with this. I think the panel should just get powred up >>>>>>>>>>> for AUX transfers. >>>>>>>>>> >>>>>>>>>> That's definitely a fair thing to think about and I have at times >>>>>>>>>> thought about trying to make it work that way. It always ends up >>>>>>>>>> hitting a roadblock. >>>>>>>> >>>>>>>> How do you even probe the panel initially if you can't power it on >>>>>>>> without doing some kind of full modeset/etc.? >>>>>>> >>>>>>> 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=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> Otherwise you can't trust that eg. the /dev/aux >>>>>>>>>>> stuff is actually usable. >>>>>>>>>> >>>>>>>>>> Yeah, it's been on my mind to talk more about /dev/aux. I think >>>>>>>>>> /dev/aux has some problems, at least with eDP. Specifically: >>>>>>>>>> >>>>>>>>>> 1. Even if we somehow figure out how to power the panel on as part of >>>>>>>>>> the aux transfer, we actually _still_ not guaranteed to be able to >>>>>>>>>> talk to it as far as I understand. My colleague reported to me that on >>>>>>>>>> a system he was working with that had PSR (panel self refresh) that >>>>>>>>>> when the panel was powered on but in PSR mode that it wouldn't talk >>>>>>>>>> over AUX. Assuming that this is correct then I guess we'd also have to >>>>>>>>>> do even more coordination with DRM to exit PSR and block future >>>>>>>>>> transitions of PSR. (NOTE: it's always possible that my colleague ran >>>>>>>>>> into some other bug and that panels are _supposed_ to be able to talk >>>>>>>>>> in PSR. If you think this is the case, I can always try to dig more). >>>>>>>>> >>>>>>>>> TBH - the coordination with drm I don't think would be the difficult part, as >>>>>>>>> we'd just need to add some sort of property (ideally invisible to userspace) >>>>>>>>> that can be used in an atomic commit to disable PSR - similar to how we enable >>>>>>>>> CRC readback from sysfs in the majority of DRM drivers. That being said >>>>>>>>> though, I think we can just leave the work of solving this problem up to >>>>>>>>> whoever ends up needing this to work. >>>>>>>> >>>>>>>> The driver should just disable/prevent PSR when doing AUX if the hardware >>>>>>>> can't guarantee the PSR and AUX won't interfere with each other. >>>>>>> >>>>>>> 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 <dmitry.baryshkov@linaro.org> >>>> >>>> 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.
Hi, On Thu, May 5, 2022 at 3:15 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On 06/05/2022 00:24, Doug Anderson wrote: > > Hi, > > > > On Thu, May 5, 2022 at 1:56 PM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > >> > >> On Thu, 5 May 2022 at 23:21, Doug Anderson <dianders@chromium.org> wrote: > >>> > >>> Hi, > >>> > >>> On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov > >>> <dmitry.baryshkov@linaro.org> wrote: > >>>> > >>>> On Thu, 5 May 2022 at 18:53, Doug Anderson <dianders@chromium.org> wrote: > >>>>> > >>>>> Hi, > >>>>> > >>>>> On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä > >>>>> <ville.syrjala@linux.intel.com> 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ä > >>>>>>> <ville.syrjala@linux.intel.com> 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ä > >>>>>>>>>> <ville.syrjala@linux.intel.com> wrote: > >>>>>>>>>>> > >>>>>>>>>>> On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote: > >>>>>>>>>>>> When doing DP AUX transfers there are two actors that need to be > >>>>>>>>>>>> powered in order for the DP AUX transfer to work: the DP source and > >>>>>>>>>>>> the DP sync. Commit bacbab58f09d ("drm: Mention the power state > >>>>>>>>>>>> requirement on side-channel operations") added some documentation > >>>>>>>>>>>> saying that the DP source is required to power itself up (if needed) > >>>>>>>>>>>> to do AUX transfers. However, that commit doesn't talk anything about > >>>>>>>>>>>> the DP sink. > >>>>>>>>>>>> > >>>>>>>>>>>> For full fledged DP the sink isn't really a problem. It's expected > >>>>>>>>>>>> that if an external DP monitor isn't plugged in that attempting to do > >>>>>>>>>>>> AUX transfers won't work. It's also expected that if a DP monitor is > >>>>>>>>>>>> plugged in (and thus asserting HPD) that it AUX transfers will work. > >>>>>>>>>>>> > >>>>>>>>>>>> When we're looking at eDP, however, things are less obvious. Let's add > >>>>>>>>>>>> some documentation about expectations. Here's what we'll say: > >>>>>>>>>>>> > >>>>>>>>>>>> 1. We don't expect the DP AUX transfer function to power on an eDP > >>>>>>>>>>>> panel. If an eDP panel is physically connected but powered off then it > >>>>>>>>>>>> makes sense for the transfer to fail. > >>>>>>>>>>> > >>>>>>>>>>> I don't agree with this. I think the panel should just get powred up > >>>>>>>>>>> for AUX transfers. > >>>>>>>>>> > >>>>>>>>>> That's definitely a fair thing to think about and I have at times > >>>>>>>>>> thought about trying to make it work that way. It always ends up > >>>>>>>>>> hitting a roadblock. > >>>>>>>> > >>>>>>>> How do you even probe the panel initially if you can't power it on > >>>>>>>> without doing some kind of full modeset/etc.? > >>>>>>> > >>>>>>> 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=U5-sTDLYdkeJWLAOG-0wgxR49VxtwUyUO7z2PuibLGsg@mail.gmail.com/ > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>>> Otherwise you can't trust that eg. the /dev/aux > >>>>>>>>>>> stuff is actually usable. > >>>>>>>>>> > >>>>>>>>>> Yeah, it's been on my mind to talk more about /dev/aux. I think > >>>>>>>>>> /dev/aux has some problems, at least with eDP. Specifically: > >>>>>>>>>> > >>>>>>>>>> 1. Even if we somehow figure out how to power the panel on as part of > >>>>>>>>>> the aux transfer, we actually _still_ not guaranteed to be able to > >>>>>>>>>> talk to it as far as I understand. My colleague reported to me that on > >>>>>>>>>> a system he was working with that had PSR (panel self refresh) that > >>>>>>>>>> when the panel was powered on but in PSR mode that it wouldn't talk > >>>>>>>>>> over AUX. Assuming that this is correct then I guess we'd also have to > >>>>>>>>>> do even more coordination with DRM to exit PSR and block future > >>>>>>>>>> transitions of PSR. (NOTE: it's always possible that my colleague ran > >>>>>>>>>> into some other bug and that panels are _supposed_ to be able to talk > >>>>>>>>>> in PSR. If you think this is the case, I can always try to dig more). > >>>>>>>>> > >>>>>>>>> TBH - the coordination with drm I don't think would be the difficult part, as > >>>>>>>>> we'd just need to add some sort of property (ideally invisible to userspace) > >>>>>>>>> that can be used in an atomic commit to disable PSR - similar to how we enable > >>>>>>>>> CRC readback from sysfs in the majority of DRM drivers. That being said > >>>>>>>>> though, I think we can just leave the work of solving this problem up to > >>>>>>>>> whoever ends up needing this to work. > >>>>>>>> > >>>>>>>> The driver should just disable/prevent PSR when doing AUX if the hardware > >>>>>>>> can't guarantee the PSR and AUX won't interfere with each other. > >>>>>>> > >>>>>>> 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 <dmitry.baryshkov@linaro.org> > >>>> > >>>> 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
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);
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 <dianders@chromium.org> --- include/drm/display/drm_dp_helper.h | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)