Hi Antheas, On 5-Oct-24 5:10 PM, Antheas Kapenekakis wrote: > Hi Hans, > >> 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. > > I will try to move some of that documentation there, this is a great idea. > >> 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. > > Yes, you are right. To become a knob this would require a much bigger > discussion. I would also like to move Sleep calls as part of that. The > Legion Go and OneXPlayer devices turn off their controllers as part of > that and other modern standby devices limit their power envelope > (perhaps the Legion Go too). I think the Sleep call is where most of > the userspace usability will come from. Display On/Off is a bit of a > NOOP on most devices. > > As for the LSB0 enter and exit, I do not know where the correct place > for those would be, and perhaps someone from Microsoft needs to be > consulted on that. The documentation is very vague. However it is > clear to me that they should be close to where they are right now, so > they very likely do not need to move. > > There is also the new _DSM intent to turn display on 9 call. Which > meshes with the sleep call. That call is made before Sleep Exit, if > the kernel knows that the wake-up will cause the display to turn on, > to boost the thermal envelope of the device and help it wake up > quicker. If the Sleep call is moved then we would also have to > introduce that somewhere to avoid wake-up time regressions on devices > that support it, which also raises the question of how would the > kernel decide if an interrupt will cause the display to turn on before > unfreezing userspace (bulk of resume) (or should it be done after > unfreezing?). > >> 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. > > From what I know Luke plans to remove that quirk ASAP due to new > firmware. I would keep it around until this patch series merges > personally and remove it as part of that. Ack I replied to Luke to say something like that. > As it will probably cause regressions if both are in place I don't see how having both this patch-sets + the asus-wmi quirks will cause regressions? The suspend delay will be done twice, but that is harmless. > and require manual intervention if > either is not. I will also note that the quirk in asus-wmi calls the > Display On/Off calls a second time and during the suspend sequence, > which is not in any way proper. AFAICT asus-wmi does not call the display on / off callbacks instead it directly calls "\\_SB.PCI0.SBRG.EC0.CSEE" to control the power ? > So if future devices need this kind of > quirk, it really does not seem like a good idea to me to paper over > their problems by calling the notifications a second time in random > platform drivers. There is the question of where that quirk should be > placed, that is true, but I IMO it should be a pm problem. > > Perhaps not in the location where I put it though and perhaps it > should be done with LSB0 callbacks instead. Although, being done this > way allows for it to blend with the suspend sequence. Ideally, the > Display Off delay would be blended with userspace going down such that > if e.g., there is heavy userspace activity that requires ~2s to > freeze, the quirk would add no delay. Instead, it would only add delay > if userspace freezes quickly (less than .5s). Same can be said with > Sleep Entry and beginning prepare_late, which blocks the EC interrupts > (that would need a lot of investigation though). > > On that note, it seems to me that the Ally has 2 bugs related to the > _DSM calls 3 and 4. First bug is that Display On is gated on current > firmware and only works when the USB subsystem is powered on. > Allegedly, this is fixed on the upcoming firmware but it is not > something I have verified personally. I will verify it in 10 days or > so, if the new firmware does not fail QA I guess. > > However, there is a second bug with Display Off in _DSM 4. The > controller of the Ally needs time to power off, around 500ms. > Otherwise it gets its power clipped and/or does not power off > correctly. This causes the issues mentioned in the discussion and I > have no indication that this is fixed with newer controller firmware. > It is also my understanding that most of the testing of the new > firmware happened with the asus-wmi quirk in place, which papers over > that issue, so removing the quirk might be premature in any case. Ok. > We have currently released this patch series in Bazzite and I am happy > to report that it completely fixes all controller related issues in > the Ally devices and makes them behave exactly as they do in Windows, > regardless of firmware and bug for bug. > > So we will be keeping it around and extending it as appropriate to > include the Sleep calls. I am reminded multiple times per week that > the Ally has TDP suspend bugs, where if the user is playing a heavy > game, the EC of the device tends to get stuck at 6W and fail to > respond after waking the device. So moving calls 7, 8 is the natural > next step in this investigation. I already have a draft patch on > standby, that we plan to AB test soon. > >> 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. > > I did run it through before sending the patch. However, some of the > warnings were a bit cryptic to me... I will run it again. > > I will add a note for asus-wmi on future patch series. > > First 3 patches of the series are designed to NOOP before patch 4. Did > you mean patch 3 (which adds the delay) instead of 4? Ah I misread the series and failed to notice that patch 4 actually hooks things up, I was under the impression that patch 4 hooks things up. But I did mean that patch 3 might lead to discussion not patch 4. Now that I have taken a better look some more detailed review comments: * Patches 1 and 2 should be squashed into a single patch IMHO. * Patch 3 adds the quirks to kernel/power/suspend.c but this really should be added to drivers/acpi/x86/s2idle.c and then do the sleep as part of the display_off callback. * Given my comment on patch 3 I would swap the order of patch 3 and 4 and only add the quirks after moving the display off ACPI call to the new callback * Patch 4 does too much in a single patch, specifically besides actually implementing the new callbacks it also does s/SCREEN/DISPLAY on all the ACPI_LPS0_* defines. This renaming of the defines must be done in a separate patch. Regards, Hans
