Hi, On 22-Sep-24 7:22 PM, Antheas Kapenekakis wrote: > 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). Thank you for your work on this and thank you for the comprehensive write-up on how Windows does modern standby. First of all may I suggest that you take the above write-up, minus the ROG Ally specific bits and turn this into a new documentation file under Documentation/power ? And also document at which point Linux currently makes the various transitions. And then in patches where you move the transitions, also update the docs on what Linux does to match. I have read the discussion about tying the display on/off calls to CRTC state and/or exposing a userspace knob for that. I think that this needs more discussion / design work. OTOH IMHO it would be good to take patches 1 - 3 . Certainly 1 + 2 would be good to have. 3 is a bit unfortunate and not necessary with the current special ROG Ally handling in the asus-wmi driver. It might be better to just keep the quirks there. IMHO it would be good to submit a v2 of just patches 1 - 3 run through checkpatch. Also the commit message of patch 3 should point to the existing quirk code in asus-wmi.c and mention that then is no longer necessary after patch 3, then we can discuss what is the best place for these quirks. Rafael, what do you think about at least taking patches 1 - 3 upstream? Reading through how Windows handles things making the display on/off calls before suspending devices sounds like it is the right thing to do to me. Regards, Hans
