| Message ID | 20240922172258.48435-3-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | acpi/x86: s2idle: move Display off/on calls outside suspend (fixes ROG Ally suspend) | expand |
On 9/22/2024 12:22, Antheas Kapenekakis wrote: > Currently, the Display On/Off calls are handled within the suspend > sequence, which is a deviation from Windows. This causes issues with > certain devices, where the notification interacts with a USB device > that expects the kernel to be fully awake. > > This patch calls the Display On/Off callbacks before entering the suspend > sequence, which fixes this issue. In addition, it opens the possibility > of modelling a state such as "Screen Off" that mirrors Windows, as the > callbacks will be accessible and validated to work outside of the > suspend sequence. > > Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- > kernel/power/suspend.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c > index c527dc0ae5ae..610f8ecaeebd 100644 > --- a/kernel/power/suspend.c > +++ b/kernel/power/suspend.c > @@ -589,6 +589,13 @@ static int enter_state(suspend_state_t state) > if (state == PM_SUSPEND_TO_IDLE) > s2idle_begin(); > > + /* > + * Linux does not have the concept of a "Screen Off" state, so call > + * the platform functions for Display On/Off prior to the suspend > + * sequence, mirroring Windows which calls them outside of it as well. > + */ > + platform_suspend_display_off(); > + I thought about it some more over the weekend and if moving the screen on/off _DSM at all I don't feel this is the right place for triggering it. IMO it should be called by DRM core. That is when the number of CRTCs active goes 1+ -> 0 call screen off and when it goes 0->1+ call screen on. There could be an argument made only for eDP this happens, but I think that's a slippery slope if you end up with an AIO design that uses DP instead of eDP or a desktop for example. So the safest policy should be across all CRTCs of all GPUs. During the suspend sequence any that were left on will get turned off, and then it could be triggered at that time instead. By making such a change then when the compositor turns off all displays at runtime you can potentially support dark screen background downloads that have specifically called this _DSM and any actions that are taken from doing so. > if (sync_on_suspend_enabled) { > trace_suspend_resume(TPS("sync_filesystems"), 0, true); > ksys_sync_helper(); > @@ -616,6 +623,8 @@ static int enter_state(suspend_state_t state) > suspend_finish(); > Unlock: > mutex_unlock(&system_transition_mutex); > + > + platform_suspend_display_on(); > return error; > } >
Does DRM core handle virtual displays like VNC? As mentioned in the cover letter, the _DSM specifies both virtual and actual displays. Also, Windows behavior is like a lockscreen. 5 seconds after screen turns off after inactivity, instantly when you press the power button. I tend towards making a sysfs entry. Not sure. Antheas On Mon, 23 Sept 2024 at 18:03, Mario Limonciello <mario.limonciello@amd.com> wrote: > > On 9/22/2024 12:22, Antheas Kapenekakis wrote: > > Currently, the Display On/Off calls are handled within the suspend > > sequence, which is a deviation from Windows. This causes issues with > > certain devices, where the notification interacts with a USB device > > that expects the kernel to be fully awake. > > > > This patch calls the Display On/Off callbacks before entering the suspend > > sequence, which fixes this issue. In addition, it opens the possibility > > of modelling a state such as "Screen Off" that mirrors Windows, as the > > callbacks will be accessible and validated to work outside of the > > suspend sequence. > > > > Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > > --- > > kernel/power/suspend.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c > > index c527dc0ae5ae..610f8ecaeebd 100644 > > --- a/kernel/power/suspend.c > > +++ b/kernel/power/suspend.c > > @@ -589,6 +589,13 @@ static int enter_state(suspend_state_t state) > > if (state == PM_SUSPEND_TO_IDLE) > > s2idle_begin(); > > > > + /* > > + * Linux does not have the concept of a "Screen Off" state, so call > > + * the platform functions for Display On/Off prior to the suspend > > + * sequence, mirroring Windows which calls them outside of it as well. > > + */ > > + platform_suspend_display_off(); > > + > > I thought about it some more over the weekend and if moving the screen > on/off _DSM at all I don't feel this is the right place for triggering it. > > IMO it should be called by DRM core. That is when the number of CRTCs > active goes 1+ -> 0 call screen off and when it goes 0->1+ call screen on. > > There could be an argument made only for eDP this happens, but I think > that's a slippery slope if you end up with an AIO design that uses DP > instead of eDP or a desktop for example. So the safest policy should be > across all CRTCs of all GPUs. During the suspend sequence any that were > left on will get turned off, and then it could be triggered at that time > instead. > > By making such a change then when the compositor turns off all displays > at runtime you can potentially support dark screen background downloads > that have specifically called this _DSM and any actions that are taken > from doing so. > > > if (sync_on_suspend_enabled) { > > trace_suspend_resume(TPS("sync_filesystems"), 0, true); > > ksys_sync_helper(); > > @@ -616,6 +623,8 @@ static int enter_state(suspend_state_t state) > > suspend_finish(); > > Unlock: > > mutex_unlock(&system_transition_mutex); > > + > > + platform_suspend_display_on(); > > return error; > > } > > >
On 9/23/2024 11:15, Antheas Kapenekakis wrote: > Does DRM core handle virtual displays like VNC? > You can make use of virtual display connectors in amdgpu. This is how drivers for new ASICs are first developed in emulation and also what's used for early hardware bring up. You can see virtual_display from https://www.kernel.org/doc/html/v6.11/gpu/amdgpu/module-parameters.html for more details. > As mentioned in the cover letter, the _DSM specifies both virtual and > actual displays. > > Also, Windows behavior is like a lockscreen. 5 seconds after screen > turns off after inactivity, instantly when you press the power button. > > I tend towards making a sysfs entry. Not sure. > Who would call this sysfs file? Systemd? The compositor? When? In Linux the compositor is in charge of doing the modesets that involve turning on/off the screen. In most cases if you press the power button in Linux systemd-logind picks up the event. It triggers a behavior that's controlled by the logind configuration. Typically that's turning on a lockscreen and/or starting the suspend sequence. Important to note; it DOESN'T explicitly turn off displays. If you configured it to suspend then displays get turned off as part of the kernel suspend sequence (drm_atomic_helper_disable_all). If it is configured to trigger a lockscreen then the compositor will turn off displays after whatever the DPMS configuration is set to.
> On 9/23/2024 11:15, Antheas Kapenekakis wrote: > > Does DRM core handle virtual displays like VNC? > > > > You can make use of virtual display connectors in amdgpu. This is how > drivers for new ASICs are first developed in emulation and also what's > used for early hardware bring up. Microsoft specificies all displays "state where all displays—local and remote, if any—have been turned off" If any means that no displays = no call perhaps. Would be interesting when designing a system for disabled people though. Given how it interacts in Windows, I want to say the target here is inactivity. > > As mentioned in the cover letter, the _DSM specifies both virtual and > > actual displays. > > > > Also, Windows behavior is like a lockscreen. 5 seconds after screen > > turns off after inactivity, instantly when you press the power button. > > > > I tend towards making a sysfs entry. Not sure. > > > > Who would call this sysfs file? Systemd? The compositor? When? > > In Linux the compositor is in charge of doing the modesets that involve > turning on/off the screen. In most cases if you press the power button > in Linux systemd-logind picks up the event. It triggers a behavior > that's controlled by the logind configuration. Typically that's turning > on a lockscreen and/or starting the suspend sequence. That's where it gets hairy and an area WIndows uniquely does not have a problem with because the OS is monolithic. If it were me, yes, systemd would switch the system to the inactive "Screen Off" state 5 seconds after the system displays are off due to inactivity. If we are talking about implementing something Modern Standby-like, then pressing the powerbutton would no longer suspend the device. Instead systemd would use two tiers of activators like windows (first tier for "Screen Off", second tier for "Sleep"; right now there is only one tier that mirrors screen off) and once all those activators are released, suspend the kernel. Then it would handle the transition from "Active" to "Screen Off" to "Sleep" through a sysfs endpoint, with the platform responding by e.g., lowering TDP and using a different fan curve. If the kernel is asked to suspend outside of the Sleep state, then it does the transitions to reach that state and references the quirk table to avoid racing conditions in manufacturer firmware (not with the kernel), before it suspends. > Important to note; it DOESN'T explicitly turn off displays. If you > configured it to suspend then displays get turned off as part of the > kernel suspend sequence (drm_atomic_helper_disable_all). > > If it is configured to trigger a lockscreen then the compositor will > turn off displays after whatever the DPMS configuration is set to. The problem with a DRM atomic helper is that we cannot control how it is called and do debouncing. WIndows does debouncing (part of that is waiting for 5 seconds so that if you move the mouse the call is skipped). You could have edge conditions that spam the calls. Antheas
Here I should note that I will not wait for systemd. As part of adding support for these devices with Handheld Daemon, I snag the powerbutton already and I only have one userspace app running anyway, Steam. The idea here would be double tap the powerbutton to enter a "Download Assistant", custom lockscreen pops up, and two seconds later DPMS triggers, system enters Sleep state and either when a battery target is reached or the download finishes, it transitions to actual suspend. The challenges here would be if the _DSM calls for Display Off and Sleep Entry are a nothingburger and do not save much battery and that Steam uses very heavy compression that does a lot of CPU. If I find it not useful, maybe I will skip it. Antheas On Mon, 23 Sept 2024 at 18:43, Antheas Kapenekakis <lkml@antheas.dev> wrote: > > > On 9/23/2024 11:15, Antheas Kapenekakis wrote: > > > Does DRM core handle virtual displays like VNC? > > > > > > > You can make use of virtual display connectors in amdgpu. This is how > > drivers for new ASICs are first developed in emulation and also what's > > used for early hardware bring up. > > Microsoft specificies all displays "state where all displays—local and > remote, if any—have been turned off" > > If any means that no displays = no call perhaps. Would be interesting > when designing a system for disabled people though. Given how it > interacts in Windows, I want to say the target here is inactivity. > > > > As mentioned in the cover letter, the _DSM specifies both virtual and > > > actual displays. > > > > > > Also, Windows behavior is like a lockscreen. 5 seconds after screen > > > turns off after inactivity, instantly when you press the power button. > > > > > > I tend towards making a sysfs entry. Not sure. > > > > > > > Who would call this sysfs file? Systemd? The compositor? When? > > > > In Linux the compositor is in charge of doing the modesets that involve > > turning on/off the screen. In most cases if you press the power button > > in Linux systemd-logind picks up the event. It triggers a behavior > > that's controlled by the logind configuration. Typically that's turning > > on a lockscreen and/or starting the suspend sequence. > > That's where it gets hairy and an area WIndows uniquely does not have > a problem with because the OS is monolithic. > > If it were me, yes, systemd would switch the system to the inactive > "Screen Off" state 5 seconds after the system displays are off due to > inactivity. If we are talking about implementing something Modern > Standby-like, then pressing the powerbutton would no longer suspend > the device. Instead systemd would use two tiers of activators like > windows (first tier for "Screen Off", second tier for "Sleep"; right > now there is only one tier that mirrors screen off) and once all those > activators are released, suspend the kernel. > > Then it would handle the transition from "Active" to "Screen Off" to > "Sleep" through a sysfs endpoint, with the platform responding by > e.g., lowering TDP and using a different fan curve. > > If the kernel is asked to suspend outside of the Sleep state, then it > does the transitions to reach that state and references the quirk > table to avoid racing conditions in manufacturer firmware (not with > the kernel), before it suspends. > > > Important to note; it DOESN'T explicitly turn off displays. If you > > configured it to suspend then displays get turned off as part of the > > kernel suspend sequence (drm_atomic_helper_disable_all). > > > > If it is configured to trigger a lockscreen then the compositor will > > turn off displays after whatever the DPMS configuration is set to. > > The problem with a DRM atomic helper is that we cannot control how it > is called and do debouncing. WIndows does debouncing (part of that is > waiting for 5 seconds so that if you move the mouse the call is > skipped). You could have edge conditions that spam the calls. > > Antheas
On 9/23/2024 11:43, Antheas Kapenekakis wrote: > > If it were me, yes, systemd would switch the system to the inactive > "Screen Off" state 5 seconds after the system displays are off due to > inactivity. If we are talking about implementing something Modern > Standby-like, then pressing the powerbutton would no longer suspend > the device. Instead systemd would use two tiers of activators like > windows (first tier for "Screen Off", second tier for "Sleep"; right > now there is only one tier that mirrors screen off) and once all those > activators are released, suspend the kernel. > > Then it would handle the transition from "Active" to "Screen Off" to > "Sleep" through a sysfs endpoint, with the platform responding by > e.g., lowering TDP and using a different fan curve. > > If the kernel is asked to suspend outside of the Sleep state, then it > does the transitions to reach that state and references the quirk > table to avoid racing conditions in manufacturer firmware (not with > the kernel), before it suspends. > >> Important to note; it DOESN'T explicitly turn off displays. If you >> configured it to suspend then displays get turned off as part of the >> kernel suspend sequence (drm_atomic_helper_disable_all). >> >> If it is configured to trigger a lockscreen then the compositor will >> turn off displays after whatever the DPMS configuration is set to. > > The problem with a DRM atomic helper is that we cannot control how it > is called and do debouncing. WIndows does debouncing (part of that is > waiting for 5 seconds so that if you move the mouse the call is > skipped). You could have edge conditions that spam the calls. > > Antheas I'm not meaning that anything in userspace SHOULD be calling `drm_atomic_helper_disable_all`, my point was that this is how it's normally called in the suspend sequence. Folks who work on the compositors don't like anyone other than the kernel touching their configuration at runtime. I think a lot of what you're looking for is the concept of a systemwide "userspace only" suspend sequence. A lot of devices already support runtime PM and will already go into the low power state when not in use. For example USB4 routers you'll see in D3 until you plug something into the USB4 port. IMO your proposal needs to cross at least 3 projects. Here's my thoughts on it. 1) systemd * To be able to handle behaviors associates with a dark screen only suspend/resume. I did start a discussion on dark resume some time back but it went nowhere. [1] * Make sure that all devices have been put into runtime PM. * Notify compositor that screens should be turned off. * Manage transitions from dark screen to full suspend and vice versa. 2) Kernel A. To be able to notify the _DSM at the right time that all CRTCs have been turned off). B. To be able to notify the _DSM at the right time that at least one CRTC is now on. 3) Wayland protocols Introduce a new protocol for requesting userspace suspend. To notify that dark screen only suspend is being triggered. 4) Compositor use. All the popular compositors would need to add support for the new protocol. IE Gamescope, mutter, kwin. This isn't a trivial effort, there are a lot of people to convince. I think the lowest effort is to start with kernel. IE having DRM core call the _DSM when the CRTCs are off. If you get that far, you can at least get this power saving behavior when DPMS is enacted. Then you can work with systemd and wayland protocol folks. [1] https://github.com/systemd/systemd/issues/27077
Did not mean how it is called as who will call it. But as in the way it is called does not match the desired behavior. In any case, as I said, I am happy to work in a downstream POC. My target usecase is very simple and I do not need/want to tie Display Off to CRTC. ie, I will do 2 and 4. I am already familiar with gamescope (including having two sockets open to it at any given time). Then if we get interesting results, we move on from there. As I said I also want to catch the Sleep _DSMs. I do not care about Display On/Off other than calling it properly (where properly = as in Windows + a lot of debouncing) so that these handhelds do not get confused. Antheas On Mon, 23 Sept 2024 at 19:05, Mario Limonciello <mario.limonciello@amd.com> wrote: > > On 9/23/2024 11:43, Antheas Kapenekakis wrote: > > > > If it were me, yes, systemd would switch the system to the inactive > > "Screen Off" state 5 seconds after the system displays are off due to > > inactivity. If we are talking about implementing something Modern > > Standby-like, then pressing the powerbutton would no longer suspend > > the device. Instead systemd would use two tiers of activators like > > windows (first tier for "Screen Off", second tier for "Sleep"; right > > now there is only one tier that mirrors screen off) and once all those > > activators are released, suspend the kernel. > > > > Then it would handle the transition from "Active" to "Screen Off" to > > "Sleep" through a sysfs endpoint, with the platform responding by > > e.g., lowering TDP and using a different fan curve. > > > > If the kernel is asked to suspend outside of the Sleep state, then it > > does the transitions to reach that state and references the quirk > > table to avoid racing conditions in manufacturer firmware (not with > > the kernel), before it suspends. > > > >> Important to note; it DOESN'T explicitly turn off displays. If you > >> configured it to suspend then displays get turned off as part of the > >> kernel suspend sequence (drm_atomic_helper_disable_all). > >> > >> If it is configured to trigger a lockscreen then the compositor will > >> turn off displays after whatever the DPMS configuration is set to. > > > > The problem with a DRM atomic helper is that we cannot control how it > > is called and do debouncing. WIndows does debouncing (part of that is > > waiting for 5 seconds so that if you move the mouse the call is > > skipped). You could have edge conditions that spam the calls. > > > > Antheas > > I'm not meaning that anything in userspace SHOULD be calling > `drm_atomic_helper_disable_all`, my point was that this is how it's > normally called in the suspend sequence. Folks who work on the > compositors don't like anyone other than the kernel touching their > configuration at runtime. > > I think a lot of what you're looking for is the concept of a systemwide > "userspace only" suspend sequence. A lot of devices already support > runtime PM and will already go into the low power state when not in use. > For example USB4 routers you'll see in D3 until you plug something > into the USB4 port. > > IMO your proposal needs to cross at least 3 projects. Here's my > thoughts on it. > > 1) systemd > * To be able to handle behaviors associates with a dark screen only > suspend/resume. I did start a discussion on dark resume some time > back but it went nowhere. [1] > > * Make sure that all devices have been put into runtime PM. > * Notify compositor that screens should be turned off. > * Manage transitions from dark screen to full suspend and vice versa. > > 2) Kernel > A. To be able to notify the _DSM at the right time that all CRTCs have > been turned off). > B. To be able to notify the _DSM at the right time that at least one > CRTC is now on. > > 3) Wayland protocols > Introduce a new protocol for requesting userspace suspend. > To notify that dark screen only suspend is being triggered. > 4) Compositor use. > All the popular compositors would need to add support for the new > protocol. IE Gamescope, mutter, kwin. > > This isn't a trivial effort, there are a lot of people to convince. I > think the lowest effort is to start with kernel. IE having DRM core > call the _DSM when the CRTCs are off. If you get that far, you can at > least get this power saving behavior when DPMS is enacted. Then you can > work with systemd and wayland protocol folks. > > > [1] https://github.com/systemd/systemd/issues/27077
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c index c527dc0ae5ae..610f8ecaeebd 100644 --- a/kernel/power/suspend.c +++ b/kernel/power/suspend.c @@ -589,6 +589,13 @@ static int enter_state(suspend_state_t state) if (state == PM_SUSPEND_TO_IDLE) s2idle_begin(); + /* + * Linux does not have the concept of a "Screen Off" state, so call + * the platform functions for Display On/Off prior to the suspend + * sequence, mirroring Windows which calls them outside of it as well. + */ + platform_suspend_display_off(); + if (sync_on_suspend_enabled) { trace_suspend_resume(TPS("sync_filesystems"), 0, true); ksys_sync_helper(); @@ -616,6 +623,8 @@ static int enter_state(suspend_state_t state) suspend_finish(); Unlock: mutex_unlock(&system_transition_mutex); + + platform_suspend_display_on(); return error; }
Currently, the Display On/Off calls are handled within the suspend sequence, which is a deviation from Windows. This causes issues with certain devices, where the notification interacts with a USB device that expects the kernel to be fully awake. This patch calls the Display On/Off callbacks before entering the suspend sequence, which fixes this issue. In addition, it opens the possibility of modelling a state such as "Screen Off" that mirrors Windows, as the callbacks will be accessible and validated to work outside of the suspend sequence. Suggested-by: Mario Limonciello <mario.limonciello@amd.com> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> --- kernel/power/suspend.c | 9 +++++++++ 1 file changed, 9 insertions(+)