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

["Derek J. Clark" <derekjohn.clark@gmail.com>]

> 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