| Message ID | 20240922172258.48435-1-lkml@antheas.dev |
|---|---|
| Headers | show |
| Series | acpi/x86: s2idle: move Display off/on calls outside suspend (fixes ROG Ally suspend) | expand |
Hello Antheas. > The following series moves the Display off/on calls outside of the suspend > sequence, as they are performed in Windows. This fixes certain issues that appear > in devices that use the calls and expect the kernel to be active during their > call (especially in the case of the ROG Ally devices) and opens the possibility > of implementing a "Screen Off" state in the future (which mirrors Windows). > In addition, it adds a quirk table that will allow for adding delays between > Modern Standby transitions, to help resolve racing conditions. > > This series requires a bit of background on how modern standby works in Windows. > Fundamentally, it is composed of four states: "Active", "Screen Off", "Sleep", > and "DRIPS". Here, I take the liberty of naming the state "Active", as it is > implied in Windows documentation. > > When the user actively interacts with a device, it is in the "Active" state. > The screen is on, all devices are connected, and desired software is running. > The other 3 stages play a role once the user stops interacting with the device. > This is triggered in two main ways: either by pressing the power button or by > inactivity. Once either of those targets is met, the system enters Modern Standby. > > Modern Standby consists of an orchestration of the "Screen Off", "Sleep", and > "DRIPS" states. Windows is free to move throughout these states until the user > interacts with the device again, where the device will transition to being > "Active". Moving between the states implies a transition, where Windows performs > a set of actions. In addition, Windows can only move between adjacent states > as follows: > > "Active" <-> "Screen Off" <-> "Sleep" <-> "DRIPS" > > "Screen Off" is the state where all active displays in the device (whether > *virtual* or real; this means unrelated to DRM) are off. The user might still > be interacting with the device or running programs (e.g., compiling a kernel). > > "Sleep" is a newer state, in which the device turns off its fan and pulses its > power button, but still supports running software activities. As part of this, > and to avoid overheating the device a lot of manufacturers lower the TDP (PLx) > of the device [3; _DSM 9 description]. > > Finally, DRIPS stands for Deepest Runtime Idle Power State, i.e. suspend. > > While Windows may transition from any state to any state, doing so implies > performing all transitions to reach that state. All states other than DRIPS > have a fully active kernel (Wifi, USB at least) and allow userspace activity. > What changes is the extent of the activity, and whether some power consuming > devices are turned off (this is done with Modern Standby aware drivers and > firmware notifications). The Windows kernel suspends during the transition from > the "Sleep" state to the "DRIPS" state. In all other states it is active. > > After finishing each transition, the kernel performs a firmware notification, > described in the _DSM document [3]. Moving from left to right with function num., > we have Display Off (3; Active -> Screen Off), Sleep Entry (7; Screen Off -> Sleep), > and Lowest Power State Entry Notification (5; LPSEN; Sleep -> DRIPS). Then, from > right to left, we have Lowest Power State Exit Notification (6; DRIPS -> Sleep), > Sleep Exit (8; Sleep -> Screen) and Display On (4; Screen Off -> Active). > > The Linux kernel is not currently Modern Standby aware but will still make these > calls. Currently, the problem is that the kernel calls all of the firmware > notifications at the point LPSEN (5, 6) should be called, which is when the > kernel is mostly suspended. This is a clear deviation from Windows, and causes > undesirable behavior in certain devices, the main one focused in this patch > series being the ROG Ally. Although series patch is aimed at Modern Standby > devices in general. > > The ROG Ally is a Modern Standby capable device (uses Secure Core too; really > ticks all the MS boxes) and in it, there are issues with both Display 3,4 > calls and Sleep 7,8 calls cause issues (7,8 are suspected and todo). > > The Display 3,4 calls are responsible for the controller. The Display Off call > disconnects (if powersave is off) or powers off (if powersave is on and on DC > power) the MCU(s) responsible for the controller and deactivates the RGB of the > device. Display On powers on or reconnects the controller respectively. > This controller, in the Ally X, is composed of 6 HID devices that register to > the kernel. From testing, it seems that the majority of the problem in the Ally > comes from Display Off being called way too late timewise, and Display > > The Sleep 7,8 calls, in general, are responsible for setting a low power state > that is safe to use while the device is sleeping, making the suspend light > pulse, and turning off the fan. Due to a variety of race conditions, there is > a rare occasion where the Ally EC can get stuck in its Sleep mode, where the > TDP is 5W, and prevent increasing it until the device reboots. The sleep entries > contain actions in the Ally, so there is a suspicion that calling them during > DRIPS is causing issues. However, this is not the subject of this patch and > has not been verified yet. > > This patch centers around moving the Display 3,4 calls outside the suspend > sequence (which is the transition from Sleep to DRIPS in Modern Standby terms), > and by implementing the proper locks necessary, opening up the possibility of > making these calls as part of a more elaborate "Modern Standby"-like userspace > suspend/wakelock implementation. As of this patch, they are only called before > the suspend sequence, including with the possibility of adding a delay. > > This makes the intent of this patch primarily compatibility focused, as it aims > to fix issues by the current implementation. And to that end it works. > After moving the calls outside of the suspend sequence, my ROG Ally X test unit > can suspend more than 50 times without rebooting, both with powersave on or off, > regardless of whether it is plugged/unplugged during suspend, and still have the > controller work with full reliability. In V1, there was an unsolved race condition > that was dealt by (5) before Display Off triggers. Essentially, Linux suspends > too fast for the current version of the firmware to deal with. After adding a > quirk table, which delays suspend after the Display Off call, the controller > of the original Ally should power off properly (a lot of testing will be done). > > Moving the calls outside of the suspend sequence (and the validation work it > implies) is an important first step in including "Modern Standby"-like > features in Linux. For example, consider an endpoint /sys/power/standby, that > allows for entering "active", "inactive" (for Screen Off; since the name causes > too much confusion), "sleep" values. Those values will then in turn call the > respective firmware notifications (and driver callbacks in the very future) > for all transitions required to reach the entered state. Here, the value > "suspend" (for DRIPS; another confusing name as it can refer to drivers) is > missing, as userspace will never be able to see it. The kernel should support > suspending at all standby states, orchestrating the required transitions to > reach suspend/DRIPS and after suspend returning to the last state. > > Therefore, if userspace is not standby aware, the kernel will work the same way > it works today. In addition, depending on hardware generation, certain power > states might not be supported. It is important to inform userspace of this, as > if the hardware does not support sleep, and userspace holds a wakelock for sleep, > it will just overheat and drain the device battery. > > This series is worth backing this up with sources, so as part of it I reference > Microsoft's documentation on Modern standby [1-3] that explains the whole > process, including a document by Dell [7] and how to prepare for them and attach a > repository of more than 15 device DSDT tables [4] from different manufacturers. > This repository also contains instructions on how to decode the DSDT tables on > a test laptop, to figure out what the _DSM calls will do on that device. > > Moreover, I conduct a short behavioral test in Windows with the Ally X to showcase > the documentation empirically. The Ally is great for such a test, as it contains > visual indicators for all Microsoft suspend points: "Display Off/On" calls are > indicated with the Controller RGB turning off/on, "Screen Off" is indicated with > the suspend light and fan being on, and Sleep is indicated with the suspend > light blinking. > > Unfortunately, as part of this testing, I never found how to see if the device > is actually suspended. As the ROG Ally X NOOPs on firmware notifications 5,6, > and even though I disabled a Mouse from waking up a device, it still would wake > up my Ally X dev unit. > > Referencing Microsoft's documentation, "Screen Off" is entered either through > inactivity or by pressing the power button, so I conduct two tests: one by pressing > the powerbutton, and one for entering Screen Off due to inactivity. > > 1) Powerbutton test: > When pressing the powerbutton, the screen of the Ally turns off, and the RGB of > the controller faints to off within 1s. Following, depending on whether the > system is plugged in, the power light and fan stay on for 5 seconds to 10 minutes. > After this point, the power light begins to blink and the fan turns off, showing > that the system has entered the "Sleep" state. > > 2) Inactivity test: > I set the Windows power settings to turn off the screen after 1 minute and wait. > After one minute, the display turns off, and after 5 seconds, the controller RGB > turns off. This indicates to me that "Screen Off" is not defined by the screen > being off, but is rather characterized by it. During those 5 seconds while the > RGB is on, I can use the controller to wake up the device. Afterwards it cannot. > > Those tests validate Microsoft's documentation and show that "Screen Off" > seems to more closely correlate to lockscreen behavior (button locks instantly, > inactivity after 5 seconds) than the screen being off. One other behavior I > notice is that, as I look at my Ally X dev right now, with its screen off, I > notice the RGB is still on, which is kind of bothersome, as in Windows the > device would turn the RGB off. Whether as a side effect or planned, it is still > a nice touch. > > This patch series is developed with help from Mario Limonciello, and, to be > bisection friendly, is structured based on a patch series he made connecting the > callbacks to the drm subsystem suspend [5]. It also references (already) > upstream work by Luke Jones on Asus-wmi for the Ally controller quirk that is > removed on patch (5) and an issue on amd-drm in 2023 in preparation for the > work in that quirk [6]. Since patch (3) now uses part of the dmi table removed > in patch (5) and adds a (small) delay, @Luke I can add you as Suggested-by. > > We will begin testing on the patch series, and there will probably be a V3, > where testing acknowledgements are added. V2 patch adds a delay to display_off > (500ms), where 300-1300ms were tried, and there was no behavioral difference on > the Ally X. However, that is arbitrary so it warrants a lot of testing. > Current status is that my Ally X unit works perfectly other than a little quirk: > with powersave on, if asus_hid or a userspace program talks to it within > 2 seconds, it causes the RGB to softfade to off and then on. This is a cosmetic > issue that can be dealt with by userspace (waiting 2s) or a firmware update > or both. Windows did not seem to fare much better either in that regard, with > RGB turning on and off randomly. Original Ally still needs to be verified. > > I am personally going to take a bit of a breather on this patch, test it, and > revisit it next week. I send it today so I get comments on the revision. I will get into this a little below, but this is not a full and accurate accounting of what the ROG Ally devices are doing. Before that, I want to summarize the premise of your proposal. A somewhat oversimplified version might read as: two systems have an issue where some of the hardware is resumed in a bad state, and you intend to resolve that issue by reorganizing the idle process. The reason you want to do this is because you have an intuitive idea of what those two devices are doing based on reading ACPI tables and guesswork. Furthermore, this proposal will have additional knock-on effects for millions of devices that aren't affected by the core issue you are trying to solve, and you have no way to test all of them. Finally, the solution provided introduces new issues (as reported by Denis and not yet resolved) for the hardware it is meant to fix. And you want to do this in spite of knowing the manufacturer is providing a firmware update that will resolve this issue without any changes needed in the kernel. I can confidently state that you don't fully understand the technical issue at hand as you are missing information on how and why the MCU functions as it does, so I will add a small amount of background containing information I am permitted to share. While developing the Ally in Windows, ASUS noticed an issue with spurious wakeups. To correct this, they added a flag as a workaround that reports state information internally so they can time certain events during the synchronous suspend and resume that Windows does to ensure the MCU is asleep or awake when it needs to be. As Linux has an asynchronous suspend and resume, this flag causes the extremely timing sensitive quirk to trigger at the wrong time quite often. There is a group of us who have spent collectively thousands of hours troubleshooting this issue, so I can confidently say your proposed DMI quirk suffers from the same issue that all previous attempts to fix it in kernel do; there is too much variability in how long each piece of this puzzle takes to reach its full sleep state. Every possible kernel fix is trying to manage the race condition in the firmware. This inevitably leads to situations where the Windows quirk triggers a bad state because it doesn't know it is already resuming, or it wakes early thinking it should have already resumed. The quirk in 3/5 is masking this issue for your setup, but that delay will be too long for some configurations and too short for others. Ironically, in an attempt to fix the issues Denis found, 3/5 moves the delay you're removing in 5/5 outside the device driver. While I'm not allowed to divulge all the technical details here, I can state your deeper dive into DRIPS is still missing some key information. My point being this approach appears, at least to me, to be a major paradigm shift in how suspend is handled so that a bandaid can be applied that solves a problem in *some* cases while introducing more problems in others. I do not think that is a valid approach. The 50 suspend/resume cycles you tested are only valid for your system, with your power profile settings, running your OS, using your kernel config, running the same userspace software you have. I know this because we also reached the same conclusion as you with dozens of patch iterations, many doing effectively the same thing as this series, only to have 1 device fail on a different OS or with a different kernel config. The details of this have been known by ASUS and us for almost a month now and we have been working with them directly to find a proper hardware level fix for it. They have provided two testing MCU firmware for us and I can confirm that they resolve the issue in every kernel and situation we previously tested on both devices. I would like to gently remind you that manufacturers intent should predominate in these situations, especially when their intent is to avoid making drastic changes to the kernel to support a small number of devices. We are also working on a fwupd module for the MCU so that it can be updated from within Linux directly. In the meantime, until it is available, dual booting and Win2Go are valid options for getting the MCU update while that interface is not completed. Luke will be providing a patch series soon to address backwards compatibility for the Ally, details to follow. As I have already upgraded both of my units testing the manufacturer approved fix, I cannot provide you with validation testing of the patches here. My devices simply work with any kernel. Having said that, I am intrigued by the proposal to add feature parity of Windows modern sleep into Linux. That topic deserves a request for comment thread in its own right that brings in all the major subsystem stakeholders and primary userspace components (systemd) so that a conversation can be had on how best to approach the topic while maintaining backwards compatibility, and not requiring the extreme level of validation this patch would require. In that same vein I recommend you drop the unnecessary adjustments to asus-wmi in 5/5 and the quirks in 3/5, and instead defer to the manufacturer intentions. If adding general modern sleep support is your intention then it would be best to focus on that issue. Thank you for your time, Derek
Hi Derek, It is ok you have upgraded both of your units. We have plenty of contributors and users that are not on the experimental firmware you are on and will provide ample testing for probably the next half year. As the update will need 1-2 months of validation and it is very difficult and painful to update firmware from linux. As I am tired, allow me to quickly jot down some thoughts so I can enjoy my evening. I only got my Ally X unit 5 days ago, so I cannot say I spent thousands of hours collectively but right now, I am pretty certain of the issue that was happening in the Ally units. It is the combination of three cascading issues: 1) Display On/Off calls are not part of the suspend sequence in neither WIndows or Linux currently. In Windows, they are called before suspend, and in Linux after. This is a large deviation that completely breaks the controller of the Ally. The previous solution was to insert a second call to those functions in the middle of the Linux suspend sequence, and then collectively spend months fighting with random racing conditions. Patches 1, 2, 4 refactor s2idle to make sure that never happens again, for any device, including the Ally. 2) The Ally Controller has a choreographed sequence with which it fades its RGB during suspend. This happens during the Display Off transition of Screen Off in Windows. In all of my testing in windows, Screen Off lasts AT LEAST 10 seconds, if not more. I had to stand around looking at that power light to turn off more than once. If Linux cuts off its power supply before that, it gets confused and restarts after suspend. If that restart happens during the resume sequence, see (1,3). Patch 3 fixes this. This flourish is an important part of user experience, so adding a delay here is required, even if firmware updates. 3) Finally, the Ally Controller, when it boots up, is sensitive to commands for 1-2 seconds, which will cause it to restart. Even with this patch series, this remains an issue on my device with powersave on, while the device initializes. asus_hid (or hhd) will instantly send a command to the device on connection, which causes this issue and then combines with 1 + 2. My patch series does not fix this. 1 and 2 will always be issues for the Ally, regardless of firmware updates and probably for other devices too. N3 I will solve through userspace + with distribution help, and it is not something that will take that much time. Patch N5 adds too much delay unfortunately, especially after resume. I would like to see it go, at least for my users. Your solution of making kernel changes for newer firmware + custom firmware + kernel changes for newer firmware with quirks somehow seems more convoluted to me than just cleaning up a bit of the s2idle subsystem to benefit all devices, with a little, firmware agnostic delay, for some flourish. Antheas
> Hi Derek, Hello > It is ok you have upgraded both of your units. We have plenty of > contributors and users that are not on the experimental firmware you > are on and will provide ample testing for probably the next half year. > As the update will need 1-2 months of validation and it is very > difficult and painful to update firmware from linux. > As I am tired, allow me to quickly jot down some thoughts so I can > enjoy my evening. > I only got my Ally X unit 5 days ago, so I cannot say I spent > thousands of hours collectively but right now, I am pretty certain of > the issue that was happening in the Ally units. > It is the combination of three cascading issues: > 1) Display On/Off calls are not part of the suspend sequence in > neither WIndows or Linux currently. In Windows, they are called before > suspend, and in Linux after. This is a large deviation that completely > breaks the controller of the Ally. The previous solution was to insert > a second call to those functions in the middle of the Linux suspend > sequence, and then collectively spend months fighting with random > racing conditions. Patches 1, 2, 4 refactor s2idle to make sure that > never happens again, for any device, including the Ally. > 2) The Ally Controller has a choreographed sequence with which it > fades its RGB during suspend. This happens during the Display Off > transition of Screen Off in Windows. In all of my testing in windows, > Screen Off lasts AT LEAST 10 seconds, if not more. I had to stand > around looking at that power light to turn off more than once. If > Linux cuts off its power supply before that, it gets confused and > restarts after suspend. If that restart happens during the resume > sequence, see (1,3). Patch 3 fixes this. This flourish is an important > part of user experience, so adding a delay here is required, even if > firmware updates. > 3) Finally, the Ally Controller, when it boots up, is sensitive to > commands for 1-2 seconds, which will cause it to restart. Even with > this patch series, this remains an issue on my device with powersave > on, while the device initializes. asus_hid (or hhd) will instantly > send a command to the device on connection, which causes this issue > and then combines with 1 + 2. My patch series does not fix this. > 1 and 2 will always be issues for the Ally, regardless of firmware > updates and probably for other devices too. N3 I will solve through > userspace + with distribution help, and it is not something that will > take that much time. Patch N5 adds too much delay unfortunately, > especially after resume. I would like to see it go, at least for my > users. I'm going to be somewhat brief here as I don't like repeating myself, you are working from incomplete information and from that you are inferring incorrect assertions. Due to NDA the full slate of information that would clarify this cannot be released here, but I will be clear: I am not sharing my opinion, I am stating facts. What you have described here is a missinterpretation of the symptoms and is not correct. The _DSM call is not relevant to the proper fix, the sequencing you observe is not applicable to Linux, and the sensitivity of the controller is another symptom of the Windows quirk behaving badly in Linux. Furthermore, the RGB "flourish" as you call it works as intended with the new firmware and no kernel changes required. What I can provide is information on a test we did that should hopefully elucidate the issue more clearly for you. We included a patch that allowed us to alter the delay in asus-wmi on the fly by writing to an attribute in sysfs. In addition, we pushed the _DSM calls as early as possible in the suspend sequence. We were unable to find a timing for this that would work consistently on different configurations. The same issue exists in your patch set and the testing bears this out with Denis still getting spurious wakes when using it. The problem with your approach is that you aren't listening to us despite our much broader understanding of the issue at hand. If this worked we would have submitted it ourselves nearly three weeks ago. > Your solution of making kernel changes for newer firmware + custom > firmware + kernel changes for newer firmware with quirks somehow seems > more convoluted to me than just cleaning up a bit of the s2idle > subsystem to benefit all devices, with a little, firmware agnostic > delay, for some flourish. > Antheas Our solution doesn't require any kernel changes for newer firmware, as I already stated the solution from ASUS fixes the root cause of the problem. Your attempt at a solution attempts to outrace the symptoms of it. Please don't mischaracterize my statements. Derek
>> It is perhaps best if I refer to what Denis and Derek told you about modern standby: focus on background downloads and proceed in that direction - the firmware really does fix the suspend/resume/reboot issues. You can also drop the asus-wmi patch (in submissions) as that will get dealt with soon enough. > > I personally do not have a problem with making the next variant of > this patch an RFC. But currently I think it has a good scope, so I > would like to ask what other kernel maintainers think. I don't think it's possible to have it both ways that the ordering is wrong AND the timing is wrong. To me; this series is looking like a lot of bandaids to work around what behave as race conditions. I'll leave some comments on the particular patches, but in the cover letter I want to see particularly patch 3 tells me that this series isn't doing it right and you're getting lucky on the timing. IoW if the call has been moved earlier but then you need to delay it to later was it really right to move it earlier? Rather than littering the kernel with quirks for a buggy firmware that the manufacturer is fixing I personally think it's a LOT better to invest the effort into getting the MCU updater ported to Linux and encourage users to update. I started to add some code to fwupd [1] to at least parse the MCU version for Ally and Ally-X. As this eventually lands higher level software could potentially communicate with the fwupd daemon over dbus to get this information and notify users that they need to update. If we manage to get the whole update flow ported it could even work with the daemon to do the update. https://github.com/fwupd/fwupd/tree/superm1/asus-hid
On 9/22/2024 12:22, Antheas Kapenekakis wrote: > The Display Off and Display On firmware notifications are meant to signify > the system entering a state where the user is not actively interacting > with it (i.e., in Windows this state is called "Screen Off" and the > system enters it once it turns the screen off e.g., due to inactivity). > > Currently, these functions are called within the suspend sequence, which > causes issues when these notifications interact with e.g., a USB device > and makes them unable to be called as part of the screen turning off. > > This patch adds a set of callbacks to allow calling the Display On/Off > notifications outside of the suspend/resume path. > > Co-developed-by: Mario Limonciello <mario.limonciello@amd.com> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> Make sure you run your patches through checkpatch. I don't believe you got these tags right. > --- > include/linux/suspend.h | 5 +++++ > kernel/power/suspend.c | 12 ++++++++++++ > 2 files changed, 17 insertions(+) > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h > index da6ebca3ff77..8f33249cc067 100644 > --- a/include/linux/suspend.h > +++ b/include/linux/suspend.h > @@ -132,6 +132,7 @@ struct platform_suspend_ops { > }; > > struct platform_s2idle_ops { > + int (*display_off)(void); > int (*begin)(void); > int (*prepare)(void); > int (*prepare_late)(void); > @@ -140,6 +141,7 @@ struct platform_s2idle_ops { > void (*restore_early)(void); > void (*restore)(void); > void (*end)(void); > + int (*display_on)(void); > }; > > #ifdef CONFIG_SUSPEND > @@ -160,6 +162,9 @@ extern unsigned int pm_suspend_global_flags; > #define PM_SUSPEND_FLAG_FW_RESUME BIT(1) > #define PM_SUSPEND_FLAG_NO_PLATFORM BIT(2) > > +int platform_suspend_display_off(void); > +int platform_suspend_display_on(void); > + > static inline void pm_suspend_clear_flags(void) > { > pm_suspend_global_flags = 0; > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c > index 09f8397bae15..c527dc0ae5ae 100644 > --- a/kernel/power/suspend.c > +++ b/kernel/power/suspend.c > @@ -254,6 +254,18 @@ static bool sleep_state_supported(suspend_state_t state) > (valid_state(state) && !cxl_mem_active()); > } > > +int platform_suspend_display_off(void) > +{ > + return s2idle_ops && s2idle_ops->display_off ? s2idle_ops->display_off() : 0; > +} > +EXPORT_SYMBOL_GPL(platform_suspend_display_off); > + > +int platform_suspend_display_on(void) > +{ > + return s2idle_ops && s2idle_ops->display_on ? s2idle_ops->display_on() : 0; > +} > +EXPORT_SYMBOL_GPL(platform_suspend_display_on); > + > static int platform_suspend_prepare(suspend_state_t state) > { > return state != PM_SUSPEND_TO_IDLE && suspend_ops->prepare ?
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; > } >
> I don't think it's possible to have it both ways that the ordering is > wrong AND the timing is wrong. I think the current firmware manifests 2 issues on the Ally X: Display On needed to be called once the kernel is ready otherwise it NOOPed and Display Off needed to be called 1 second before suspend. Then, there is the issue in my previous email that also exists in Windows. I tried from 0 - 4000ms for Display off and 0 - 1500ms for Display on. Display Off, as long as the RGB fades 100-200ms before, made no difference. Display On likewise made no difference. I am currently playing a bit with the calls in the Legion Go. Seems like one of them turns off its controller as well. I did not know fwupd could do microprocessors, if it is simple that would be great. Antheas
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:11, Antheas Kapenekakis wrote: >> I don't think it's possible to have it both ways that the ordering is >> wrong AND the timing is wrong. > > I think the current firmware manifests 2 issues on the Ally X: Display > On needed to be called once the kernel is ready otherwise it NOOPed > and Display Off needed to be called 1 second before suspend. Then, > there is the issue in my previous email that also exists in Windows. > > I tried from 0 - 4000ms for Display off and 0 - 1500ms for Display on. > Display Off, as long as the RGB fades 100-200ms before, made no > difference. Display On likewise made no difference. Part of the problem here is that we (Linux kernel developers) don't have the details of how the MCU actually interacts with the OS vs how it wants to interact with the OS. Only ASUS knows this and all we can do is guess (or join an NDA). "For example" (not saying this is the problem; I don't know) if we have a Linux sequence that we put the XHCI host controller into D3 but don't turn off the port first this might not matter to most devices because you're saving power by the controller being in D3. But if the device is looking for some sequence of packets there could be a failure if those never happen or happen at the wrong time. > > I am currently playing a bit with the calls in the Legion Go. Seems > like one of them turns off its controller as well. > > I did not know fwupd could do microprocessors, if it is simple that > would be great. Yeah in this case it interacts with the USB HID interface. Unfortunately we don't have the HID spec from ASUS, so the entire thing would need to be reverse engineered. I hope that more of these handhelds with MCUs that get regular updates make the same investment.
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
> Part of the problem here is that we (Linux kernel developers) don't have > the details of how the MCU actually interacts with the OS vs how it > wants to interact with the OS. Only ASUS knows this and all we can do > is guess (or join an NDA). I want to say that that is beginning to change, at least for these devices. Manufacturers want the experience that comes with owning the whole OS. Mobile phones and TVs figured that out long ago. > "For example" (not saying this is the problem; I don't know) if we have > a Linux sequence that we put the XHCI host controller into D3 but don't > turn off the port first this might not matter to most devices because > you're saving power by the controller being in D3. But if the device is > looking for some sequence of packets there could be a failure if those > never happen or happen at the wrong time. The problem here is probably that. Killing the power too early made it not save its state, asking it to be brought up before it powers on made it NOOP, and asking it to be brought up in prepare_early ended up with a bunch of race conditions with the kernel. Lenovo figured it out, so there is no reason Asus wouldn't. Although the latest bios from lenovo made the controllers take 2 seconds longer to wake up. We did not have any problem in Linux, that was in Windows. Unplugging XInput devices is really a mess. I am really happy with the patch on the Ally X. We will get to testing it on the Ally. It does a little reboot with powersave on, but that is a cosmetic failure and happens in Windows too. When Asus fixes that I will point them to Windows to update if they care about powersave so much. > > > > I am currently playing a bit with the calls in the Legion Go. Seems > > like one of them turns off its controller as well. > > > > I did not know fwupd could do microprocessors, if it is simple that > > would be great. > > Yeah in this case it interacts with the USB HID interface. > > Unfortunately we don't have the HID spec from ASUS, so the entire thing > would need to be reverse engineered. > > I hope that more of these handhelds with MCUs that get regular updates > make the same investment. I do not think every manufacturer creates a HID spec for updates. They use the same cookiecutter stuff, for which there is no linux implementation at all, so kind of the same. Probably after we do 1-3, we will have all of them. I saw that HID interface though, and the Legion Go has one and I am none the wiser. I've been doing HID for over a year by now, so I am used to it. Biggest problem is you cannot trigger an update in Windows whenever you want to sniff it. At least on the Lenovo. However, I think I have bigger fish to fry, so it is not high in my priority list. 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
On 9/23/2024 11:54, Antheas Kapenekakis wrote: >> I hope that more of these handhelds with MCUs that get regular updates >> make the same investment. > > I do not think every manufacturer creates a HID spec for updates. They > use the same cookiecutter stuff, for which there is no linux > implementation at all, so kind of the same. Probably after we do 1-3, > we will have all of them. Actually there are already about a dozen implementations for HID protocols in fwupd. I've written a few myself, Richard has written way more than me, and there are even have vendors contributing to their own plugins now. I do think that it's very likely that ASUS uses the same implementation on a lot of their devices, so yes if we get firmware updating figured out on one we'll probably have it for many more. > > I saw that HID interface though, and the Legion Go has one and I am > none the wiser. > > I've been doing HID for over a year by now, so I am used to it. Considering the previous experience, maybe you can help to document their update protocol? I'm happy to collaborate on the fwupd side if you can help push it along. > Biggest problem is you cannot trigger an update in Windows whenever > you want to sniff it. At least on the Lenovo. > > However, I think I have bigger fish to fry, so it is not high in my > priority list. > AFAICT any need for this series goes away when you have an easy way to update a handheld that is running Linux already. I would take a pile of userspace code over kernel quirks any day.
The following series moves the Display off/on calls outside of the suspend sequence, as they are performed in Windows. This fixes certain issues that appear in devices that use the calls and expect the kernel to be active during their call (especially in the case of the ROG Ally devices) and opens the possibility of implementing a "Screen Off" state in the future (which mirrors Windows). In addition, it adds a quirk table that will allow for adding delays between Modern Standby transitions, to help resolve racing conditions. This series requires a bit of background on how modern standby works in Windows. Fundamentally, it is composed of four states: "Active", "Screen Off", "Sleep", and "DRIPS". Here, I take the liberty of naming the state "Active", as it is implied in Windows documentation. When the user actively interacts with a device, it is in the "Active" state. The screen is on, all devices are connected, and desired software is running. The other 3 stages play a role once the user stops interacting with the device. This is triggered in two main ways: either by pressing the power button or by inactivity. Once either of those targets is met, the system enters Modern Standby. Modern Standby consists of an orchestration of the "Screen Off", "Sleep", and "DRIPS" states. Windows is free to move throughout these states until the user interacts with the device again, where the device will transition to being "Active". Moving between the states implies a transition, where Windows performs a set of actions. In addition, Windows can only move between adjacent states as follows: "Active" <-> "Screen Off" <-> "Sleep" <-> "DRIPS" "Screen Off" is the state where all active displays in the device (whether *virtual* or real; this means unrelated to DRM) are off. The user might still be interacting with the device or running programs (e.g., compiling a kernel). "Sleep" is a newer state, in which the device turns off its fan and pulses its power button, but still supports running software activities. As part of this, and to avoid overheating the device a lot of manufacturers lower the TDP (PLx) of the device [3; _DSM 9 description]. Finally, DRIPS stands for Deepest Runtime Idle Power State, i.e. suspend. While Windows may transition from any state to any state, doing so implies performing all transitions to reach that state. All states other than DRIPS have a fully active kernel (Wifi, USB at least) and allow userspace activity. What changes is the extent of the activity, and whether some power consuming devices are turned off (this is done with Modern Standby aware drivers and firmware notifications). The Windows kernel suspends during the transition from the "Sleep" state to the "DRIPS" state. In all other states it is active. After finishing each transition, the kernel performs a firmware notification, described in the _DSM document [3]. Moving from left to right with function num., we have Display Off (3; Active -> Screen Off), Sleep Entry (7; Screen Off -> Sleep), and Lowest Power State Entry Notification (5; LPSEN; Sleep -> DRIPS). Then, from right to left, we have Lowest Power State Exit Notification (6; DRIPS -> Sleep), Sleep Exit (8; Sleep -> Screen) and Display On (4; Screen Off -> Active). The Linux kernel is not currently Modern Standby aware but will still make these calls. Currently, the problem is that the kernel calls all of the firmware notifications at the point LPSEN (5, 6) should be called, which is when the kernel is mostly suspended. This is a clear deviation from Windows, and causes undesirable behavior in certain devices, the main one focused in this patch series being the ROG Ally. Although series patch is aimed at Modern Standby devices in general. The ROG Ally is a Modern Standby capable device (uses Secure Core too; really ticks all the MS boxes) and in it, there are issues with both Display 3,4 calls and Sleep 7,8 calls cause issues (7,8 are suspected and todo). The Display 3,4 calls are responsible for the controller. The Display Off call disconnects (if powersave is off) or powers off (if powersave is on and on DC power) the MCU(s) responsible for the controller and deactivates the RGB of the device. Display On powers on or reconnects the controller respectively. This controller, in the Ally X, is composed of 6 HID devices that register to the kernel. From testing, it seems that the majority of the problem in the Ally comes from Display Off being called way too late timewise, and Display The Sleep 7,8 calls, in general, are responsible for setting a low power state that is safe to use while the device is sleeping, making the suspend light pulse, and turning off the fan. Due to a variety of race conditions, there is a rare occasion where the Ally EC can get stuck in its Sleep mode, where the TDP is 5W, and prevent increasing it until the device reboots. The sleep entries contain actions in the Ally, so there is a suspicion that calling them during DRIPS is causing issues. However, this is not the subject of this patch and has not been verified yet. This patch centers around moving the Display 3,4 calls outside the suspend sequence (which is the transition from Sleep to DRIPS in Modern Standby terms), and by implementing the proper locks necessary, opening up the possibility of making these calls as part of a more elaborate "Modern Standby"-like userspace suspend/wakelock implementation. As of this patch, they are only called before the suspend sequence, including with the possibility of adding a delay. This makes the intent of this patch primarily compatibility focused, as it aims to fix issues by the current implementation. And to that end it works. After moving the calls outside of the suspend sequence, my ROG Ally X test unit can suspend more than 50 times without rebooting, both with powersave on or off, regardless of whether it is plugged/unplugged during suspend, and still have the controller work with full reliability. In V1, there was an unsolved race condition that was dealt by (5) before Display Off triggers. Essentially, Linux suspends too fast for the current version of the firmware to deal with. After adding a quirk table, which delays suspend after the Display Off call, the controller of the original Ally should power off properly (a lot of testing will be done). Moving the calls outside of the suspend sequence (and the validation work it implies) is an important first step in including "Modern Standby"-like features in Linux. For example, consider an endpoint /sys/power/standby, that allows for entering "active", "inactive" (for Screen Off; since the name causes too much confusion), "sleep" values. Those values will then in turn call the respective firmware notifications (and driver callbacks in the very future) for all transitions required to reach the entered state. Here, the value "suspend" (for DRIPS; another confusing name as it can refer to drivers) is missing, as userspace will never be able to see it. The kernel should support suspending at all standby states, orchestrating the required transitions to reach suspend/DRIPS and after suspend returning to the last state. Therefore, if userspace is not standby aware, the kernel will work the same way it works today. In addition, depending on hardware generation, certain power states might not be supported. It is important to inform userspace of this, as if the hardware does not support sleep, and userspace holds a wakelock for sleep, it will just overheat and drain the device battery. This series is worth backing this up with sources, so as part of it I reference Microsoft's documentation on Modern standby [1-3] that explains the whole process, including a document by Dell [7] and how to prepare for them and attach a repository of more than 15 device DSDT tables [4] from different manufacturers. This repository also contains instructions on how to decode the DSDT tables on a test laptop, to figure out what the _DSM calls will do on that device. Moreover, I conduct a short behavioral test in Windows with the Ally X to showcase the documentation empirically. The Ally is great for such a test, as it contains visual indicators for all Microsoft suspend points: "Display Off/On" calls are indicated with the Controller RGB turning off/on, "Screen Off" is indicated with the suspend light and fan being on, and Sleep is indicated with the suspend light blinking. Unfortunately, as part of this testing, I never found how to see if the device is actually suspended. As the ROG Ally X NOOPs on firmware notifications 5,6, and even though I disabled a Mouse from waking up a device, it still would wake up my Ally X dev unit. Referencing Microsoft's documentation, "Screen Off" is entered either through inactivity or by pressing the power button, so I conduct two tests: one by pressing the powerbutton, and one for entering Screen Off due to inactivity. 1) Powerbutton test: When pressing the powerbutton, the screen of the Ally turns off, and the RGB of the controller faints to off within 1s. Following, depending on whether the system is plugged in, the power light and fan stay on for 5 seconds to 10 minutes. After this point, the power light begins to blink and the fan turns off, showing that the system has entered the "Sleep" state. 2) Inactivity test: I set the Windows power settings to turn off the screen after 1 minute and wait. After one minute, the display turns off, and after 5 seconds, the controller RGB turns off. This indicates to me that "Screen Off" is not defined by the screen being off, but is rather characterized by it. During those 5 seconds while the RGB is on, I can use the controller to wake up the device. Afterwards it cannot. Those tests validate Microsoft's documentation and show that "Screen Off" seems to more closely correlate to lockscreen behavior (button locks instantly, inactivity after 5 seconds) than the screen being off. One other behavior I notice is that, as I look at my Ally X dev right now, with its screen off, I notice the RGB is still on, which is kind of bothersome, as in Windows the device would turn the RGB off. Whether as a side effect or planned, it is still a nice touch. This patch series is developed with help from Mario Limonciello, and, to be bisection friendly, is structured based on a patch series he made connecting the callbacks to the drm subsystem suspend [5]. It also references (already) upstream work by Luke Jones on Asus-wmi for the Ally controller quirk that is removed on patch (5) and an issue on amd-drm in 2023 in preparation for the work in that quirk [6]. Since patch (3) now uses part of the dmi table removed in patch (5) and adds a (small) delay, @Luke I can add you as Suggested-by. We will begin testing on the patch series, and there will probably be a V3, where testing acknowledgements are added. V2 patch adds a delay to display_off (500ms), where 300-1300ms were tried, and there was no behavioral difference on the Ally X. However, that is arbitrary so it warrants a lot of testing. Current status is that my Ally X unit works perfectly other than a little quirk: with powersave on, if asus_hid or a userspace program talks to it within 2 seconds, it causes the RGB to softfade to off and then on. This is a cosmetic issue that can be dealt with by userspace (waiting 2s) or a firmware update or both. Windows did not seem to fare much better either in that regard, with RGB turning on and off randomly. Original Ally still needs to be verified. I am personally going to take a bit of a breather on this patch, test it, and revisit it next week. I send it today so I get comments on the revision. Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/prepare-hardware-for-modern-standby [1] Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/prepare-software-for-modern-standby [2] Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications [3] Link: https://github.com/hhd-dev/hwinfo/tree/master/devices [4] Link: https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git/log/?h=superm1/dsm-screen-on-off [5] Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2719 [6] Link: https://dl.dell.com/manuals/all-products/esuprt_solutions_int/esuprt_solutions_int_solutions_resources/client-mobile-solution-resources_white-papers45_en-us.pdf [7] Changes in v2: - Rewrote cover letter to better reflect the Windows Modern Standby sequence - Renamed the "screen_" callbacks to "display_" to match Microsoft's naming - Added attribution to Mario Limonciello and changed the text to reflect that - Made the screen on/off callbacks warn and bail with -EINVAL if they are called in the wrong order (currently impossible) - Changed patch 2 to not bail the suspend sequence when receiving an error (as these calls are not part of the suspend sequence and failing the suspend sequence would cause a user visible fault) - Fixed bug reported by Denis Benato by adding a quirk table in patch (3) - The ROG controllers get a slight delay after the Display Off call - This delay fixes a race condition with the controller disconnecting from the system before being powered off - Reworded patches to reflect that - Moved the display calls a bit higher up the suspend sequence in patch (2) Antheas Kapenekakis (5): acpi/x86: s2idle: add support for Display Off and Display On callbacks acpi/x86: s2idle: handle Display On/Off calls outside of suspend sequence acpi/x86: s2idle: add quirk table for modern standby delays acpi/x86: s2idle: call Display On/Off as part of callbacks and rename platform/x86: asus-wmi: remove Ally (1st gen) and Ally X suspend quirk drivers/acpi/x86/s2idle.c | 89 +++++++++++++++++++++++---------- drivers/platform/x86/asus-wmi.c | 54 -------------------- include/linux/suspend.h | 10 ++++ kernel/power/suspend.c | 62 +++++++++++++++++++++++ 4 files changed, 134 insertions(+), 81 deletions(-)