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

Antheas Kapenekakis
2024-09-23T05:57:41Z

Hi Derek,

> 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.

Only thing I am seeing here is a bit of heresay. Please get in touch
with Asus, cross your t's and dot your i's, and share what you found
with the rest of the class.

I am in no rush to see this merge, and as all gaming distributions
carry custom kernels that update on a weekly cadence and 90% of users
are on those kernels with the rest being able to figure it out, there
is no practical reason for this to merge quickly. Ubuntu (+variant)
users will get this fix after they've thrown their ally away anyway.

> 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.

Here you assume I did not do the same. Yes, I did. The asus-wmi quirk
is subject to racing conditions that mean it will never work correctly
(well; without newer firmware maybe perhaps). This series is not, as
the calls happen before suspend starts.

Yes, my V1 of the patch did not include a delay at all. Since the
original Ally is XInput, one of the MCUs was left on, which caused
what Denis experienced. It also triggered a restart of the MCU on the
Ally X after resume. V2 fixes this restart on the Ally X and makes it
behave completely properly.

In fact, after being included in bazzite-testing yesterday, my Ally X
unit went on a deep slumber tonight with powersave on, and then woke
up today beautifully, having only lost 1% of battery.

It is not clear if the issue still exists on the original Ally, I
could not get a clear signal from Denis but do not worry, one of our
contributors is on it.

> *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.

I would hope that is not true and your solution completely removes the
quirk from asus-wmi for newer firmware, as it prolongs the suspend and
resume sequences noticeably. However, given my and Denis' limited
testing, I doubt the controller is able to suspend quickly enough even
with new firmware (you can see the RGB cut before it's able to fade).

In any case, this patch series is not expected to conflict with Asus'
newer firmware and is in fact complementary with it. Both fix the same
issue in a different way, which means our users will have a great
experience, squared. This series also greatly improves resume behavior
on the Ally, which, let me tell you, is blazing fast now.

Antheas