On 11/27/2023 14:46, Luke Jones wrote:
On Mon, Nov 27 2023 at 02:14:23 PM -06:00:00, Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
On 11/26/2023 17:05, Luke D. Jones wrote:
ASUS have worked around an issue in XInput where it doesn't
support USB
selective suspend, which causes suspend issues in Windows. They
worked
around this by adjusting the MCU firmware to disable the USB0
hub when
the screen is switched off during the Microsoft DSM suspend
path
in ACPI.
The issue we have with this however is one of timing - the call
the tells
the MCU to this isn't able to complete before suspend is done
so
we call
this in a prepare() and add a small msleep() to ensure it is
done. This
must be done before the screen is switched off to prevent a
variety of
possible races.
Right now the way that Linux handles the LPS0 calls is that
they're all back to back. Luke did try to inject a delay after
the LPS0 calls were done but before it went to sleep but this
wasn't sufficient.
Another "potential" way to solve this problem from Linux may be
to actually glue the LPS0 screen off call to when DRM actually
has
eDP turned off.
Making such a change would essentially push back the "screen
off"
LPS0 command to when the user has run 'systemctl suspend' (or an
action that did this) because the compositor usually turns it off
with DPMS at this time.
I would be willing to test this if you want some concrete data.
It would require some cross subsystem plumbing to evaluate
feasibility.
I don't currently have any plans to do it.
I think your patch makes sense; I just want to make it known that
"might" clean this up if it ever happens.
See my big block of text below.
This is a much bigger change though and *much more ripe for
breakage*.
So I think in may be worth leaving a TODO comment to look into
doing that in the future.
Do you mean add the TODO to a line in this patch?
Yeah. In case someone ever does it (me or otherwise) I think it
would be good to have some reference in the comments that the
commit
'might' be possible to revert.
If that ever happens; it's possible that this change could be
reverted too.
Further to this the MCU powersave option must also be disabled
as it can
cause a number of issues such as:
- unreliable resume connection of N-Key
- complete loss of N-Key if the power is plugged in while
suspended
Disabling the powersave option prevents this.
Without this the MCU is unable to initialise itself correctly
on
resume.
initialize
Are we forced to use USA spelling? I'm from NZ
"initialise is predominantly used in British English (used in
UK/AU/NZ) ( en-GB )"
Ah I didn't realize it's an acceptable spelling for en-GB, and
thought it was just a typo; sorry.
Signed-off-by: Luke D. Jones <luke@xxxxxxxxxx>
I think it would be good to add a Closes: tag to the AMD Gitlab
issue that this was discussed within as well as any other public
references you know about.
Additionally as Phoenix APU support goes back as far as kernel
6.1 and this is well contained to only run on the ROG I suggest
to
CC stable so that people can use the ROG on that LTS kernel or
later.
---
-SNIP-
@@ -4701,6 +4749,8 @@ static const struct dev_pm_ops
asus_pm_ops = {
.thaw = asus_hotk_thaw,
.restore = asus_hotk_restore,
.resume = asus_hotk_resume,
+ .resume_early = asus_hotk_resume_early,
+ .prepare = asus_hotk_prepare,
Have you experimented with only using the prepare() call or only
the resume_early() call? Are both really needed?
I have yes. Although the device comes back eventually in resume
after only a prepare call it's not preferable as it tends to
change
the device path. With resume_early we can get the device replugged
super early (before anything notices it's gone in fact).
This whole thing is a bit of a mess. It ends up being a race
between various things to prevent a HUB0 disconnect being
registered by the xhci subsystem, and adding the device back
before
the xhci subsystem gets control.
If I add a sleep longer than 1300ms in prepare then the xhci
subsys registers a disconnect of the USB0 hub. If the sleep is
under 250ms it isn't quite enough for the MCU to do its thing, and
on battery it seems worse.
I have asked the ASUS guys I'm in contact with for something to
disable this MCU behaviour since it is purely a workaround for a
broken Windows thing :( They are open to something, maybe an OS
detect in ACPI or a WMI method addition similar to the MCU
powersave method, from what I'm told it would require an MCU
firmware update along with BIOS update. If this eventuates I'll
submit an additional patch to check and set that plus disable
this.
Don't let them do an OS detection in ACPI, it's going to be too
painful.
I would instead suggest that they can have a bit that you can
program in via ACPI or WMI from the ASUS WMI driver that says to
skip the MCU disconnect behavior.
