Re: [PATCH v2 0/5] acpi/x86: s2idle: move Display off/on calls outside suspend (fixes ROG Ally suspend)

Antheas Kapenekakis
2024-09-22T19:40:25Z

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