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

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

[Antheas Kapenekakis]

The following series moves the Display off/on calls outside of the suspend
sequence, as they are performed in Windows. This fixes certain issues that appear
in devices that use the calls and expect the kernel to be active during their
call (especially in the case of the ROG Ally devices) and opens the possibility
of implementing a "Screen Off" state in the future (which mirrors Windows).
In addition, it adds a quirk table that will allow for adding delays between
Modern Standby transitions, to help resolve racing conditions.

This series requires a bit of background on how modern standby works in Windows.
Fundamentally, it is composed of four states: "Active", "Screen Off", "Sleep",
and "DRIPS". Here, I take the liberty of naming the state "Active", as it is
implied in Windows documentation.

When the user actively interacts with a device, it is in the "Active" state.
The screen is on, all devices are connected, and desired software is running.
The other 3 stages play a role once the user stops interacting with the device.
This is triggered in two main ways: either by pressing the power button or by
inactivity. Once either of those targets is met, the system enters Modern Standby.

Modern Standby consists of an orchestration of the "Screen Off", "Sleep", and
"DRIPS" states. Windows is free to move throughout these states until the user
interacts with the device again, where the device will transition to being
"Active". Moving between the states implies a transition, where Windows performs
a set of actions. In addition, Windows can only move between adjacent states
as follows:

"Active" <-> "Screen Off" <-> "Sleep" <-> "DRIPS"

"Screen Off" is the state where all active displays in the device (whether
*virtual* or real; this means unrelated to DRM) are off. The user might still
be interacting with the device or running programs (e.g., compiling a kernel).

"Sleep" is a newer state, in which the device turns off its fan and pulses its
power button, but still supports running software activities. As part of this,
and to avoid overheating the device a lot of manufacturers lower the TDP (PLx)
of the device [3; _DSM 9 description].

Finally, DRIPS stands for Deepest Runtime Idle Power State, i.e. suspend.

While Windows may transition from any state to any state, doing so implies
performing all transitions to reach that state. All states other than DRIPS
have a fully active kernel (Wifi, USB at least) and allow userspace activity.
What changes is the extent of the activity, and whether some power consuming
devices are turned off (this is done with Modern Standby aware drivers and
firmware notifications). The Windows kernel suspends during the transition from
the "Sleep" state to the "DRIPS" state. In all other states it is active.

After finishing each transition, the kernel performs a firmware notification,
described in the _DSM document [3]. Moving from left to right with function num.,
we have Display Off (3; Active -> Screen Off), Sleep Entry (7; Screen Off -> Sleep),
and Lowest Power State Entry Notification (5; LPSEN; Sleep -> DRIPS). Then, from
right to left, we have Lowest Power State Exit Notification (6; DRIPS -> Sleep),
Sleep Exit (8; Sleep -> Screen) and Display On (4; Screen Off -> Active).

The Linux kernel is not currently Modern Standby aware but will still make these
calls. Currently, the problem is that the kernel calls all of the firmware
notifications at the point LPSEN (5, 6) should be called, which is when the
kernel is mostly suspended. This is a clear deviation from Windows, and causes
undesirable behavior in certain devices, the main one focused in this patch
series being the ROG Ally. Although series patch is aimed at Modern Standby
devices in general.

The ROG Ally is a Modern Standby capable device (uses Secure Core too; really
ticks all the MS boxes) and in it, there are issues with both Display 3,4
calls and Sleep 7,8 calls cause issues (7,8 are suspected and todo).

The Display 3,4 calls are responsible for the controller. The Display Off call
disconnects (if powersave is off) or powers off (if powersave is on and on DC
power) the MCU(s) responsible for the controller and deactivates the RGB of the
device. Display On powers on or reconnects the controller respectively.
This controller, in the Ally X, is composed of 6 HID devices that register to
the kernel. From testing, it seems that the majority of the problem in the Ally
comes from Display Off being called way too late timewise, and Display

The Sleep 7,8 calls, in general, are responsible for setting a low power state
that is safe to use while the device is sleeping, making the suspend light
pulse, and turning off the fan. Due to a variety of race conditions, there is
a rare occasion where the Ally EC can get stuck in its Sleep mode, where the
TDP is 5W, and prevent increasing it until the device reboots. The sleep entries
contain actions in the Ally, so there is a suspicion that calling them during
DRIPS is causing issues. However, this is not the subject of this patch and
has not been verified yet.

This patch centers around moving the Display 3,4 calls outside the suspend
sequence (which is the transition from Sleep to DRIPS in Modern Standby terms),
and by implementing the proper locks necessary, opening up the possibility of
making these calls as part of a more elaborate "Modern Standby"-like userspace
suspend/wakelock implementation. As of this patch, they are only called before
the suspend sequence, including with the possibility of adding a delay.

This makes the intent of this patch primarily compatibility focused, as it aims
to fix issues by the current implementation. And to that end it works.
After moving the calls outside of the suspend sequence, my ROG Ally X test unit
can suspend more than 50 times without rebooting, both with powersave on or off,
regardless of whether it is plugged/unplugged during suspend, and still have the
controller work with full reliability. In V1, there was an unsolved race condition
that was dealt by (5) before Display Off triggers. Essentially, Linux suspends
too fast for the current version of the firmware to deal with. After adding a
quirk table, which delays suspend after the Display Off call, the controller
of the original Ally should power off properly (a lot of testing will be done).

Moving the calls outside of the suspend sequence (and the validation work it
implies) is an important first step in including "Modern Standby"-like
features in Linux. For example, consider an endpoint /sys/power/standby, that
allows for entering "active", "inactive" (for Screen Off; since the name causes
too much confusion), "sleep" values. Those values will then in turn call the
respective firmware notifications (and driver callbacks in the very future)
for all transitions required to reach the entered state. Here, the value
"suspend" (for DRIPS; another confusing name as it can refer to drivers) is
missing, as userspace will never be able to see it. The kernel should support
suspending at all standby states, orchestrating the required transitions to
reach suspend/DRIPS and after suspend returning to the last state.

Therefore, if userspace is not standby aware, the kernel will work the same way
it works today. In addition, depending on hardware generation, certain power
states might not be supported. It is important to inform userspace of this, as
if the hardware does not support sleep, and userspace holds a wakelock for sleep,
it will just overheat and drain the device battery.

This series is worth backing this up with sources, so as part of it I reference
Microsoft's documentation on Modern standby [1-3] that explains the whole
process, including a document by Dell [7] and how to prepare for them and attach a
repository of more than 15 device DSDT tables [4] from different manufacturers.
This repository also contains instructions on how to decode the DSDT tables on
a test laptop, to figure out what the _DSM calls will do on that device.

Moreover, I conduct a short behavioral test in Windows with the Ally X to showcase
the documentation empirically. The Ally is great for such a test, as it contains
visual indicators for all Microsoft suspend points: "Display Off/On" calls are
indicated with the Controller RGB turning off/on, "Screen Off" is indicated with
the suspend light and fan being on, and Sleep is indicated with the suspend
light blinking.

Unfortunately, as part of this testing, I never found how to see if the device
is actually suspended. As the ROG Ally X NOOPs on firmware notifications 5,6,
and even though I disabled a Mouse from waking up a device, it still would wake
up my Ally X dev unit.

Referencing Microsoft's documentation, "Screen Off" is entered either through
inactivity or by pressing the power button, so I conduct two tests: one by pressing
the powerbutton, and one for entering Screen Off due to inactivity.

1) Powerbutton test:
When pressing the powerbutton, the screen of the Ally turns off, and the RGB of
the controller faints to off within 1s. Following, depending on whether the
system is plugged in, the power light and fan stay on for 5 seconds to 10 minutes.
After this point, the power light begins to blink and the fan turns off, showing
that the system has entered the "Sleep" state.

2) Inactivity test:
I set the Windows power settings to turn off the screen after 1 minute and wait.
After one minute, the display turns off, and after 5 seconds, the controller RGB
turns off. This indicates to me that "Screen Off" is not defined by the screen
being off, but is rather characterized by it. During those 5 seconds while the
RGB is on, I can use the controller to wake up the device. Afterwards it cannot.

Those tests validate Microsoft's documentation and show that "Screen Off"
seems to more closely correlate to lockscreen behavior (button locks instantly,
inactivity after 5 seconds) than the screen being off. One other behavior I
notice is that, as I look at my Ally X dev right now, with its screen off, I
notice the RGB is still on, which is kind of bothersome, as in Windows the
device would turn the RGB off. Whether as a side effect or planned, it is still
a nice touch.

This patch series is developed with help from Mario Limonciello, and, to be
bisection friendly, is structured based on a patch series he made connecting the
callbacks to the drm subsystem suspend [5]. It also references (already)
upstream work by Luke Jones on Asus-wmi for the Ally controller quirk that is
removed on patch (5) and an issue on amd-drm in 2023 in preparation for the
work in that quirk [6]. Since patch (3) now uses part of the dmi table removed
in patch (5) and adds a (small) delay, @Luke I can add you as Suggested-by.

We will begin testing on the patch series, and there will probably be a V3,
where testing acknowledgements are added. V2 patch adds a delay to display_off
(500ms), where 300-1300ms were tried, and there was no behavioral difference on
the Ally X. However, that is arbitrary so it warrants a lot of testing.
Current status is that my Ally X unit works perfectly other than a little quirk:
with powersave on, if asus_hid or a userspace program talks to it within
2 seconds, it causes the RGB to softfade to off and then on. This is a cosmetic
issue that can be dealt with by userspace (waiting 2s) or a firmware update
or both. Windows did not seem to fare much better either in that regard, with
RGB turning on and off randomly. Original Ally still needs to be verified.

I am personally going to take a bit of a breather on this patch, test it, and
revisit it next week. I send it today so I get comments on the revision.

Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/prepare-hardware-for-modern-standby [1]
Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/prepare-software-for-modern-standby [2]
Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications [3]
Link: https://github.com/hhd-dev/hwinfo/tree/master/devices [4]
Link: https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git/log/?h=superm1/dsm-screen-on-off [5]
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2719 [6]
Link: https://dl.dell.com/manuals/all-products/esuprt_solutions_int/esuprt_solutions_int_solutions_resources/client-mobile-solution-resources_white-papers45_en-us.pdf [7]

Changes in v2:
 - Rewrote cover letter to better reflect the Windows Modern Standby sequence
 - Renamed the "screen_" callbacks to "display_" to match Microsoft's naming
 - Added attribution to Mario Limonciello and changed the text to reflect that
 - Made the screen on/off callbacks warn and bail with -EINVAL if they are
   called in the wrong order (currently impossible)
    - Changed patch 2 to not bail the suspend sequence when receiving an error
      (as these calls are not part of the suspend sequence and failing the
      suspend sequence would cause a user visible fault)
 - Fixed bug reported by Denis Benato by adding a quirk table in patch (3)
    - The ROG controllers get a slight delay after the Display Off call
    - This delay fixes a race condition with the controller disconnecting from
      the system before being powered off
    - Reworded patches to reflect that
 - Moved the display calls a bit higher up the suspend sequence in patch (2)

Antheas Kapenekakis (5):
  acpi/x86: s2idle: add support for Display Off and Display On callbacks
  acpi/x86: s2idle: handle Display On/Off calls outside of suspend
    sequence
  acpi/x86: s2idle: add quirk table for modern standby delays
  acpi/x86: s2idle: call Display On/Off as part of callbacks and rename
  platform/x86: asus-wmi: remove Ally (1st gen) and Ally X suspend quirk

 drivers/acpi/x86/s2idle.c       | 89 +++++++++++++++++++++++----------
 drivers/platform/x86/asus-wmi.c | 54 --------------------
 include/linux/suspend.h         | 10 ++++
 kernel/power/suspend.c          | 62 +++++++++++++++++++++++
 4 files changed, 134 insertions(+), 81 deletions(-)

-- 
2.46.1

[PATCH v2 1/5] acpi/x86: s2idle: add support for Display Off and Display On callbacks

[Antheas Kapenekakis]

The Display Off and Display On firmware notifications are meant to signify
the system entering a state where the user is not actively interacting
with it (i.e., in Windows this state is called "Screen Off" and the
system enters it once it turns the screen off e.g., due to inactivity).

Currently, these functions are called within the suspend sequence, which
causes issues when these notifications interact with e.g., a USB device
and makes them unable to be called as part of the screen turning off.

This patch adds a set of callbacks to allow calling the Display On/Off
notifications outside of the suspend/resume path.

Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 include/linux/suspend.h |  5 +++++
 kernel/power/suspend.c  | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index da6ebca3ff77..8f33249cc067 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -132,6 +132,7 @@ struct platform_suspend_ops {
 };
 
 struct platform_s2idle_ops {
+	int (*display_off)(void);
 	int (*begin)(void);
 	int (*prepare)(void);
 	int (*prepare_late)(void);
@@ -140,6 +141,7 @@ struct platform_s2idle_ops {
 	void (*restore_early)(void);
 	void (*restore)(void);
 	void (*end)(void);
+	int (*display_on)(void);
 };
 
 #ifdef CONFIG_SUSPEND
@@ -160,6 +162,9 @@ extern unsigned int pm_suspend_global_flags;
 #define PM_SUSPEND_FLAG_FW_RESUME	BIT(1)
 #define PM_SUSPEND_FLAG_NO_PLATFORM	BIT(2)
 
+int platform_suspend_display_off(void);
+int platform_suspend_display_on(void);
+
 static inline void pm_suspend_clear_flags(void)
 {
 	pm_suspend_global_flags = 0;
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 09f8397bae15..c527dc0ae5ae 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -254,6 +254,18 @@ static bool sleep_state_supported(suspend_state_t state)
 	       (valid_state(state) && !cxl_mem_active());
 }
 
+int platform_suspend_display_off(void)
+{
+	return s2idle_ops && s2idle_ops->display_off ? s2idle_ops->display_off() : 0;
+}
+EXPORT_SYMBOL_GPL(platform_suspend_display_off);
+
+int platform_suspend_display_on(void)
+{
+	return s2idle_ops && s2idle_ops->display_on ? s2idle_ops->display_on() : 0;
+}
+EXPORT_SYMBOL_GPL(platform_suspend_display_on);
+
 static int platform_suspend_prepare(suspend_state_t state)
 {
 	return state != PM_SUSPEND_TO_IDLE && suspend_ops->prepare ?
-- 
2.46.1

[PATCH v2 2/5] acpi/x86: s2idle: handle Display On/Off calls outside of suspend sequence

[Antheas Kapenekakis]

Currently, the Display On/Off calls are handled within the suspend
sequence, which is a deviation from Windows. This causes issues with
certain devices, where the notification interacts with a USB device
that expects the kernel to be fully awake.

This patch calls the Display On/Off callbacks before entering the suspend
sequence, which fixes this issue. In addition, it opens the possibility
of modelling a state such as "Screen Off" that mirrors Windows, as the
callbacks will be accessible and validated to work outside of the
suspend sequence.

Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 kernel/power/suspend.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index c527dc0ae5ae..610f8ecaeebd 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -589,6 +589,13 @@ static int enter_state(suspend_state_t state)
 	if (state == PM_SUSPEND_TO_IDLE)
 		s2idle_begin();
 
+	/*
+	 * Linux does not have the concept of a "Screen Off" state, so call
+	 * the platform functions for Display On/Off prior to the suspend
+	 * sequence, mirroring Windows which calls them outside of it as well.
+	 */
+	platform_suspend_display_off();
+
 	if (sync_on_suspend_enabled) {
 		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
 		ksys_sync_helper();
@@ -616,6 +623,8 @@ static int enter_state(suspend_state_t state)
 	suspend_finish();
  Unlock:
 	mutex_unlock(&system_transition_mutex);
+
+	platform_suspend_display_on();
 	return error;
 }
 
-- 
2.46.1

[PATCH v2 3/5] acpi/x86: s2idle: add quirk table for modern standby delays

[Antheas Kapenekakis]

Unfortunately, some modern standby systems, including the ROG Ally, rely
on a delay between modern standby transitions. Add a quirk table for
introducing delays between modern standby transitions, and quirk the
ROG Ally on "Display Off", which needs a bit of time to turn off its
controllers prior to suspending (i.e., entering DRIPS).

Reported-by: Denis Benato <benato.denis96@gmail.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 include/linux/suspend.h |  5 +++++
 kernel/power/suspend.c  | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 8f33249cc067..d7e2a4d8ab0c 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -144,6 +144,11 @@ struct platform_s2idle_ops {
 	int (*display_on)(void);
 };
 
+struct platform_s2idle_quirks {
+	int delay_display_off;
+	int delay_display_on;
+};
+
 #ifdef CONFIG_SUSPEND
 extern suspend_state_t pm_suspend_target_state;
 extern suspend_state_t mem_sleep_current;
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 610f8ecaeebd..af2abdd2f8c3 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -11,6 +11,7 @@
 
 #include <linux/string.h>
 #include <linux/delay.h>
+#include <linux/dmi.h>
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/console.h>
@@ -61,6 +62,30 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
 enum s2idle_states __read_mostly s2idle_state;
 static DEFINE_RAW_SPINLOCK(s2idle_lock);
 
+// The ROG Ally series disconnects its controllers on Display Off, without
+// holding a lock, introducing a race condition. Add a delay to allow the
+// controller to disconnect cleanly prior to suspend.
+static const struct platform_s2idle_quirks rog_ally_quirks = {
+	.delay_display_off = 500,
+};
+
+static const struct dmi_system_id platform_s2idle_quirks[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
+		},
+		.driver_data = (void *)&rog_ally_quirks
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_NAME, "RC72L"),
+		},
+		.driver_data = (void *)&rog_ally_quirks
+	},
+	{}
+};
+
+
 /**
  * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend.
  *
@@ -589,12 +614,26 @@ static int enter_state(suspend_state_t state)
 	if (state == PM_SUSPEND_TO_IDLE)
 		s2idle_begin();
 
+	/*
+	 * Windows transitions between Modern Standby states slowly, over multiple
+	 * seconds. Certain manufacturers may rely on this, introducing race
+	 * conditions. Until Linux can support modern standby, add the relevant
+	 * delays between transitions here.
+	 */
+	const struct dmi_system_id *s2idle_sysid = dmi_first_match(
+		platform_s2idle_quirks
+	);
+	const struct platform_s2idle_quirks *s2idle_quirks = s2idle_sysid ?
+		s2idle_sysid->driver_data : NULL;
+
 	/*
 	 * Linux does not have the concept of a "Screen Off" state, so call
 	 * the platform functions for Display On/Off prior to the suspend
 	 * sequence, mirroring Windows which calls them outside of it as well.
 	 */
 	platform_suspend_display_off();
+	if (s2idle_quirks && s2idle_quirks->delay_display_off)
+		msleep(s2idle_quirks->delay_display_off);
 
 	if (sync_on_suspend_enabled) {
 		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
@@ -624,6 +663,8 @@ static int enter_state(suspend_state_t state)
  Unlock:
 	mutex_unlock(&system_transition_mutex);
 
+	if (s2idle_quirks && s2idle_quirks->delay_display_on)
+		msleep(s2idle_quirks->delay_display_on);
 	platform_suspend_display_on();
 	return error;
 }
-- 
2.46.1

[PATCH v2 4/5] acpi/x86: s2idle: call Display On/Off as part of callbacks and rename

[Antheas Kapenekakis]

Move the Display On/Off notifications into dedicated callbacks that gate
the ACPI mutex, so they can be called outside of the suspend path.
This fixes issues on certain devices that expect kernel drivers to be
fully active during the calls, and allows for the flexibility of calling
them as part of a more elaborate userspace suspend sequence (such as
with "Screen Off" in Windows Modern Standby).

In addition, rename the notifications from "screen_" to "display_", as
there is no documentation referring to them as screen, either by
Intel or Microsoft.

Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/acpi/x86/s2idle.c | 89 +++++++++++++++++++++++++++------------
 1 file changed, 62 insertions(+), 27 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index dd0b40b9bbe8..a17e28b91326 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -39,8 +39,8 @@ static const struct acpi_device_id lps0_device_ids[] = {
 #define ACPI_LPS0_DSM_UUID	"c4eb40a0-6cd2-11e2-bcfd-0800200c9a66"
 
 #define ACPI_LPS0_GET_DEVICE_CONSTRAINTS	1
-#define ACPI_LPS0_SCREEN_OFF	3
-#define ACPI_LPS0_SCREEN_ON	4
+#define ACPI_LPS0_DISPLAY_OFF	3
+#define ACPI_LPS0_DISPLAY_ON	4
 #define ACPI_LPS0_ENTRY		5
 #define ACPI_LPS0_EXIT		6
 #define ACPI_LPS0_MS_ENTRY      7
@@ -50,8 +50,8 @@ static const struct acpi_device_id lps0_device_ids[] = {
 #define ACPI_LPS0_DSM_UUID_AMD      "e3f32452-febc-43ce-9039-932122d37721"
 #define ACPI_LPS0_ENTRY_AMD         2
 #define ACPI_LPS0_EXIT_AMD          3
-#define ACPI_LPS0_SCREEN_OFF_AMD    4
-#define ACPI_LPS0_SCREEN_ON_AMD     5
+#define ACPI_LPS0_DISPLAY_OFF_AMD   4
+#define ACPI_LPS0_DISPLAY_ON_AMD    5
 
 static acpi_handle lps0_device_handle;
 static guid_t lps0_dsm_guid;
@@ -60,6 +60,7 @@ static int lps0_dsm_func_mask;
 static guid_t lps0_dsm_guid_microsoft;
 static int lps0_dsm_func_mask_microsoft;
 static int lps0_dsm_state;
+static bool lsp0_dsm_in_display_off;
 
 /* Device constraint entry structure */
 struct lpi_device_info {
@@ -361,9 +362,9 @@ static const char *acpi_sleep_dsm_state_to_str(unsigned int state)
 {
 	if (lps0_dsm_func_mask_microsoft || !acpi_s2idle_vendor_amd()) {
 		switch (state) {
-		case ACPI_LPS0_SCREEN_OFF:
+		case ACPI_LPS0_DISPLAY_OFF:
 			return "screen off";
-		case ACPI_LPS0_SCREEN_ON:
+		case ACPI_LPS0_DISPLAY_ON:
 			return "screen on";
 		case ACPI_LPS0_ENTRY:
 			return "lps0 entry";
@@ -376,9 +377,9 @@ static const char *acpi_sleep_dsm_state_to_str(unsigned int state)
 		}
 	} else {
 		switch (state) {
-		case ACPI_LPS0_SCREEN_ON_AMD:
+		case ACPI_LPS0_DISPLAY_ON_AMD:
 			return "screen on";
-		case ACPI_LPS0_SCREEN_OFF_AMD:
+		case ACPI_LPS0_DISPLAY_OFF_AMD:
 			return "screen off";
 		case ACPI_LPS0_ENTRY_AMD:
 			return "lps0 entry";
@@ -539,27 +540,69 @@ static struct acpi_scan_handler lps0_handler = {
 	.attach = lps0_device_attach,
 };
 
-int acpi_s2idle_prepare_late(void)
+static int acpi_s2idle_display_off(void)
 {
-	struct acpi_s2idle_dev_ops *handler;
-
 	if (!lps0_device_handle || sleep_no_lps0)
 		return 0;
 
-	if (pm_debug_messages_on)
-		lpi_check_constraints();
+	if (unlikely(WARN_ON(lsp0_dsm_in_display_off)))
+		return -EINVAL;
+
+	lsp0_dsm_in_display_off = true;
+	acpi_scan_lock_acquire();
 
-	/* Screen off */
+	/* Display off */
 	if (lps0_dsm_func_mask > 0)
 		acpi_sleep_run_lps0_dsm(acpi_s2idle_vendor_amd() ?
-					ACPI_LPS0_SCREEN_OFF_AMD :
-					ACPI_LPS0_SCREEN_OFF,
+					ACPI_LPS0_DISPLAY_OFF_AMD :
+					ACPI_LPS0_DISPLAY_OFF,
 					lps0_dsm_func_mask, lps0_dsm_guid);
 
 	if (lps0_dsm_func_mask_microsoft > 0)
-		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF,
+		acpi_sleep_run_lps0_dsm(ACPI_LPS0_DISPLAY_OFF,
 				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
 
+	acpi_scan_lock_release();
+
+	return 0;
+}
+
+static int acpi_s2idle_display_on(void)
+{
+	if (!lps0_device_handle || sleep_no_lps0)
+		return 0;
+
+	if (unlikely(WARN_ON(!lsp0_dsm_in_display_off)))
+		return -EINVAL;
+
+	lsp0_dsm_in_display_off = false;
+	acpi_scan_lock_acquire();
+
+	/* Display on */
+	if (lps0_dsm_func_mask_microsoft > 0)
+		acpi_sleep_run_lps0_dsm(ACPI_LPS0_DISPLAY_ON,
+				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
+	if (lps0_dsm_func_mask > 0)
+		acpi_sleep_run_lps0_dsm(acpi_s2idle_vendor_amd() ?
+					ACPI_LPS0_DISPLAY_ON_AMD :
+					ACPI_LPS0_DISPLAY_ON,
+					lps0_dsm_func_mask, lps0_dsm_guid);
+
+	acpi_scan_lock_release();
+
+	return 0;
+}
+
+int acpi_s2idle_prepare_late(void)
+{
+	struct acpi_s2idle_dev_ops *handler;
+
+	if (!lps0_device_handle || sleep_no_lps0)
+		return 0;
+
+	if (pm_debug_messages_on)
+		lpi_check_constraints();
+
 	/* LPS0 entry */
 	if (lps0_dsm_func_mask > 0 && acpi_s2idle_vendor_amd())
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY_AMD,
@@ -623,19 +666,10 @@ void acpi_s2idle_restore_early(void)
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_EXIT,
 				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
 	}
-
-	/* Screen on */
-	if (lps0_dsm_func_mask_microsoft > 0)
-		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON,
-				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
-	if (lps0_dsm_func_mask > 0)
-		acpi_sleep_run_lps0_dsm(acpi_s2idle_vendor_amd() ?
-					ACPI_LPS0_SCREEN_ON_AMD :
-					ACPI_LPS0_SCREEN_ON,
-					lps0_dsm_func_mask, lps0_dsm_guid);
 }
 
 static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = {
+	.display_off = acpi_s2idle_display_off,
 	.begin = acpi_s2idle_begin,
 	.prepare = acpi_s2idle_prepare,
 	.prepare_late = acpi_s2idle_prepare_late,
@@ -644,6 +678,7 @@ static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = {
 	.restore_early = acpi_s2idle_restore_early,
 	.restore = acpi_s2idle_restore,
 	.end = acpi_s2idle_end,
+	.display_on = acpi_s2idle_display_on,
 };
 
 void __init acpi_s2idle_setup(void)
-- 
2.46.1

[PATCH v2 5/5] platform/x86: asus-wmi: remove Ally (1st gen) and Ally X suspend quirk

[Antheas Kapenekakis]

By moving the Display On/Off calls outside of the suspend sequence and
introducing a slight delay after Display Off, the ROG Ally controller
functions exactly as it does in Windows.

Therefore, remove the quirk that fixed the controller only when the
mcu_powersave attribute was disabled, while adding a large amount of
delay to the suspend and wake process.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/asus-wmi.c | 54 ---------------------------------
 1 file changed, 54 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 37636e5a38e3..2c9656e0afda 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -137,29 +137,10 @@ module_param(fnlock_default, bool, 0444);
 #define ASUS_MINI_LED_2024_STRONG	0x01
 #define ASUS_MINI_LED_2024_OFF		0x02
 
-/* Controls the power state of the USB0 hub on ROG Ally which input is on */
-#define ASUS_USB0_PWR_EC0_CSEE "\\_SB.PCI0.SBRG.EC0.CSEE"
-/* 300ms so far seems to produce a reliable result on AC and battery */
-#define ASUS_USB0_PWR_EC0_CSEE_WAIT 1500
-
 static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
 
 static int throttle_thermal_policy_write(struct asus_wmi *);
 
-static const struct dmi_system_id asus_ally_mcu_quirk[] = {
-	{
-		.matches = {
-			DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
-		},
-	},
-	{
-		.matches = {
-			DMI_MATCH(DMI_BOARD_NAME, "RC72L"),
-		},
-	},
-	{ },
-};
-
 static bool ashs_present(void)
 {
 	int i = 0;
@@ -269,9 +250,6 @@ struct asus_wmi {
 	u32 tablet_switch_dev_id;
 	bool tablet_switch_inverted;
 
-	/* The ROG Ally device requires the MCU USB device be disconnected before suspend */
-	bool ally_mcu_usb_switch;
-
 	enum fan_type fan_type;
 	enum fan_type gpu_fan_type;
 	enum fan_type mid_fan_type;
@@ -4698,8 +4676,6 @@ static int asus_wmi_add(struct platform_device *pdev)
 	asus->egpu_enable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU);
 	asus->dgpu_disable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU);
 	asus->kbd_rgb_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_STATE);
-	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
-						&& dmi_check_system(asus_ally_mcu_quirk);
 
 	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE))
 		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
@@ -4892,34 +4868,6 @@ static int asus_hotk_resume(struct device *device)
 	return 0;
 }
 
-static int asus_hotk_resume_early(struct device *device)
-{
-	struct asus_wmi *asus = dev_get_drvdata(device);
-
-	if (asus->ally_mcu_usb_switch) {
-		/* sleep required to prevent USB0 being yanked then reappearing rapidly */
-		if (ACPI_FAILURE(acpi_execute_simple_method(NULL, ASUS_USB0_PWR_EC0_CSEE, 0xB8)))
-			dev_err(device, "ROG Ally MCU failed to connect USB dev\n");
-		else
-			msleep(ASUS_USB0_PWR_EC0_CSEE_WAIT);
-	}
-	return 0;
-}
-
-static int asus_hotk_prepare(struct device *device)
-{
-	struct asus_wmi *asus = dev_get_drvdata(device);
-
-	if (asus->ally_mcu_usb_switch) {
-		/* sleep required to ensure USB0 is disabled before sleep continues */
-		if (ACPI_FAILURE(acpi_execute_simple_method(NULL, ASUS_USB0_PWR_EC0_CSEE, 0xB7)))
-			dev_err(device, "ROG Ally MCU failed to disconnect USB dev\n");
-		else
-			msleep(ASUS_USB0_PWR_EC0_CSEE_WAIT);
-	}
-	return 0;
-}
-
 static int asus_hotk_restore(struct device *device)
 {
 	struct asus_wmi *asus = dev_get_drvdata(device);
@@ -4964,8 +4912,6 @@ 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,
 };
 
 /* Registration ***************************************************************/
-- 
2.46.1

Re: [PATCH v2 3/5] acpi/x86: s2idle: add quirk table for modern standby delays

[Denis Benato]

On 22/09/24 19:22, Antheas Kapenekakis wrote:
> Unfortunately, some modern standby systems, including the ROG Ally, rely
> on a delay between modern standby transitions. Add a quirk table for
> introducing delays between modern standby transitions, and quirk the
> ROG Ally on "Display Off", which needs a bit of time to turn off its
> controllers prior to suspending (i.e., entering DRIPS).
> 
> Reported-by: Denis Benato <benato.denis96@gmail.com>

I told you privately that as stated here: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
Reported-By tags are to be followed by a Closes tag stating that the issue is solved.

What has been presented to me today was not about solving bugs, but changing how they manifests and therefore permission was not granted to you to represent me as satisfied with the work as it would be wrong to assume issues are solved and this is worth merging.

Furthermore everybody can see my answers to your v1 patch and there is no need to attach a Reported-by when the issue has been reported publicly and is not resolved.

I will add my Tested-off-by by myself whenever I will be fully satisfied by work presented: in the current state I am not.

> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>  include/linux/suspend.h |  5 +++++
>  kernel/power/suspend.c  | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 8f33249cc067..d7e2a4d8ab0c 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -144,6 +144,11 @@ struct platform_s2idle_ops {
>  	int (*display_on)(void);
>  };
>  
> +struct platform_s2idle_quirks {
> +	int delay_display_off;
> +	int delay_display_on;
> +};
> +
>  #ifdef CONFIG_SUSPEND
>  extern suspend_state_t pm_suspend_target_state;
>  extern suspend_state_t mem_sleep_current;
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 610f8ecaeebd..af2abdd2f8c3 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -11,6 +11,7 @@
>  
>  #include <linux/string.h>
>  #include <linux/delay.h>
> +#include <linux/dmi.h>
>  #include <linux/errno.h>
>  #include <linux/init.h>
>  #include <linux/console.h>
> @@ -61,6 +62,30 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
>  enum s2idle_states __read_mostly s2idle_state;
>  static DEFINE_RAW_SPINLOCK(s2idle_lock);
>  
> +// The ROG Ally series disconnects its controllers on Display Off, without
> +// holding a lock, introducing a race condition. Add a delay to allow the
> +// controller to disconnect cleanly prior to suspend.
> +static const struct platform_s2idle_quirks rog_ally_quirks = {
> +	.delay_display_off = 500,
> +};
> +
> +static const struct dmi_system_id platform_s2idle_quirks[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
> +		},
> +		.driver_data = (void *)&rog_ally_quirks
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_NAME, "RC72L"),
> +		},
> +		.driver_data = (void *)&rog_ally_quirks
> +	},
> +	{}
> +};
> +
> +
>  /**
>   * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend.
>   *
> @@ -589,12 +614,26 @@ static int enter_state(suspend_state_t state)
>  	if (state == PM_SUSPEND_TO_IDLE)
>  		s2idle_begin();
>  
> +	/*
> +	 * Windows transitions between Modern Standby states slowly, over multiple
> +	 * seconds. Certain manufacturers may rely on this, introducing race
> +	 * conditions. Until Linux can support modern standby, add the relevant
> +	 * delays between transitions here.
> +	 */
> +	const struct dmi_system_id *s2idle_sysid = dmi_first_match(
> +		platform_s2idle_quirks
> +	);
> +	const struct platform_s2idle_quirks *s2idle_quirks = s2idle_sysid ?
> +		s2idle_sysid->driver_data : NULL;
> +
>  	/*
>  	 * Linux does not have the concept of a "Screen Off" state, so call
>  	 * the platform functions for Display On/Off prior to the suspend
>  	 * sequence, mirroring Windows which calls them outside of it as well.
>  	 */
>  	platform_suspend_display_off();
> +	if (s2idle_quirks && s2idle_quirks->delay_display_off)
> +		msleep(s2idle_quirks->delay_display_off);
>  
>  	if (sync_on_suspend_enabled) {
>  		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
> @@ -624,6 +663,8 @@ static int enter_state(suspend_state_t state)
>   Unlock:
>  	mutex_unlock(&system_transition_mutex);
>  
> +	if (s2idle_quirks && s2idle_quirks->delay_display_on)
> +		msleep(s2idle_quirks->delay_display_on);
>  	platform_suspend_display_on();
>  	return error;
>  }

Re: [PATCH v2 3/5] acpi/x86: s2idle: add quirk table for modern standby delays

[Antheas Kapenekakis]

Hi Denis,
I did not add a Tested-by for this reason. The Report-by is to credit
you for bringing this into my attention, I thought you were ok with
this. If not, I will remove it in the next revision.

Best,
Antheas

On Sun, 22 Sept 2024 at 19:55, Denis Benato <benato.denis96@gmail.com> wrote:
>
> On 22/09/24 19:22, Antheas Kapenekakis wrote:
> > Unfortunately, some modern standby systems, including the ROG Ally, rely
> > on a delay between modern standby transitions. Add a quirk table for
> > introducing delays between modern standby transitions, and quirk the
> > ROG Ally on "Display Off", which needs a bit of time to turn off its
> > controllers prior to suspending (i.e., entering DRIPS).
> >
> > Reported-by: Denis Benato <benato.denis96@gmail.com>
>
> I told you privately that as stated here: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
> Reported-By tags are to be followed by a Closes tag stating that the issue is solved.
>
> What has been presented to me today was not about solving bugs, but changing how they manifests and therefore permission was not granted to you to represent me as satisfied with the work as it would be wrong to assume issues are solved and this is worth merging.
>
> Furthermore everybody can see my answers to your v1 patch and there is no need to attach a Reported-by when the issue has been reported publicly and is not resolved.
>
> I will add my Tested-off-by by myself whenever I will be fully satisfied by work presented: in the current state I am not.
>
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> >  include/linux/suspend.h |  5 +++++
> >  kernel/power/suspend.c  | 41 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 46 insertions(+)
> >
> > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > index 8f33249cc067..d7e2a4d8ab0c 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -144,6 +144,11 @@ struct platform_s2idle_ops {
> >       int (*display_on)(void);
> >  };
> >
> > +struct platform_s2idle_quirks {
> > +     int delay_display_off;
> > +     int delay_display_on;
> > +};
> > +
> >  #ifdef CONFIG_SUSPEND
> >  extern suspend_state_t pm_suspend_target_state;
> >  extern suspend_state_t mem_sleep_current;
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 610f8ecaeebd..af2abdd2f8c3 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -11,6 +11,7 @@
> >
> >  #include <linux/string.h>
> >  #include <linux/delay.h>
> > +#include <linux/dmi.h>
> >  #include <linux/errno.h>
> >  #include <linux/init.h>
> >  #include <linux/console.h>
> > @@ -61,6 +62,30 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
> >  enum s2idle_states __read_mostly s2idle_state;
> >  static DEFINE_RAW_SPINLOCK(s2idle_lock);
> >
> > +// The ROG Ally series disconnects its controllers on Display Off, without
> > +// holding a lock, introducing a race condition. Add a delay to allow the
> > +// controller to disconnect cleanly prior to suspend.
> > +static const struct platform_s2idle_quirks rog_ally_quirks = {
> > +     .delay_display_off = 500,
> > +};
> > +
> > +static const struct dmi_system_id platform_s2idle_quirks[] = {
> > +     {
> > +             .matches = {
> > +                     DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
> > +             },
> > +             .driver_data = (void *)&rog_ally_quirks
> > +     },
> > +     {
> > +             .matches = {
> > +                     DMI_MATCH(DMI_BOARD_NAME, "RC72L"),
> > +             },
> > +             .driver_data = (void *)&rog_ally_quirks
> > +     },
> > +     {}
> > +};
> > +
> > +
> >  /**
> >   * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend.
> >   *
> > @@ -589,12 +614,26 @@ static int enter_state(suspend_state_t state)
> >       if (state == PM_SUSPEND_TO_IDLE)
> >               s2idle_begin();
> >
> > +     /*
> > +      * Windows transitions between Modern Standby states slowly, over multiple
> > +      * seconds. Certain manufacturers may rely on this, introducing race
> > +      * conditions. Until Linux can support modern standby, add the relevant
> > +      * delays between transitions here.
> > +      */
> > +     const struct dmi_system_id *s2idle_sysid = dmi_first_match(
> > +             platform_s2idle_quirks
> > +     );
> > +     const struct platform_s2idle_quirks *s2idle_quirks = s2idle_sysid ?
> > +             s2idle_sysid->driver_data : NULL;
> > +
> >       /*
> >        * Linux does not have the concept of a "Screen Off" state, so call
> >        * the platform functions for Display On/Off prior to the suspend
> >        * sequence, mirroring Windows which calls them outside of it as well.
> >        */
> >       platform_suspend_display_off();
> > +     if (s2idle_quirks && s2idle_quirks->delay_display_off)
> > +             msleep(s2idle_quirks->delay_display_off);
> >
> >       if (sync_on_suspend_enabled) {
> >               trace_suspend_resume(TPS("sync_filesystems"), 0, true);
> > @@ -624,6 +663,8 @@ static int enter_state(suspend_state_t state)
> >   Unlock:
> >       mutex_unlock(&system_transition_mutex);
> >
> > +     if (s2idle_quirks && s2idle_quirks->delay_display_on)
> > +             msleep(s2idle_quirks->delay_display_on);
> >       platform_suspend_display_on();
> >       return error;
> >  }
>

Re: [PATCH v2 1/5] acpi/x86: s2idle: add support for Display Off and Display On callbacks

[Mario Limonciello]

On 9/22/2024 12:22, Antheas Kapenekakis wrote:
> The Display Off and Display On firmware notifications are meant to signify
> the system entering a state where the user is not actively interacting
> with it (i.e., in Windows this state is called "Screen Off" and the
> system enters it once it turns the screen off e.g., due to inactivity).
> 
> Currently, these functions are called within the suspend sequence, which
> causes issues when these notifications interact with e.g., a USB device
> and makes them unable to be called as part of the screen turning off.
> 
> This patch adds a set of callbacks to allow calling the Display On/Off
> notifications outside of the suspend/resume path.
> 
> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>

Make sure you run your patches through checkpatch.  I don't believe you 
got these tags right.

> ---
>   include/linux/suspend.h |  5 +++++
>   kernel/power/suspend.c  | 12 ++++++++++++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index da6ebca3ff77..8f33249cc067 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -132,6 +132,7 @@ struct platform_suspend_ops {
>   };
>   
>   struct platform_s2idle_ops {
> +	int (*display_off)(void);
>   	int (*begin)(void);
>   	int (*prepare)(void);
>   	int (*prepare_late)(void);
> @@ -140,6 +141,7 @@ struct platform_s2idle_ops {
>   	void (*restore_early)(void);
>   	void (*restore)(void);
>   	void (*end)(void);
> +	int (*display_on)(void);
>   };
>   
>   #ifdef CONFIG_SUSPEND
> @@ -160,6 +162,9 @@ extern unsigned int pm_suspend_global_flags;
>   #define PM_SUSPEND_FLAG_FW_RESUME	BIT(1)
>   #define PM_SUSPEND_FLAG_NO_PLATFORM	BIT(2)
>   
> +int platform_suspend_display_off(void);
> +int platform_suspend_display_on(void);
> +
>   static inline void pm_suspend_clear_flags(void)
>   {
>   	pm_suspend_global_flags = 0;
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 09f8397bae15..c527dc0ae5ae 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -254,6 +254,18 @@ static bool sleep_state_supported(suspend_state_t state)
>   	       (valid_state(state) && !cxl_mem_active());
>   }
>   
> +int platform_suspend_display_off(void)
> +{
> +	return s2idle_ops && s2idle_ops->display_off ? s2idle_ops->display_off() : 0;
> +}
> +EXPORT_SYMBOL_GPL(platform_suspend_display_off);
> +
> +int platform_suspend_display_on(void)
> +{
> +	return s2idle_ops && s2idle_ops->display_on ? s2idle_ops->display_on() : 0;
> +}
> +EXPORT_SYMBOL_GPL(platform_suspend_display_on);
> +
>   static int platform_suspend_prepare(suspend_state_t state)
>   {
>   	return state != PM_SUSPEND_TO_IDLE && suspend_ops->prepare ?

Re: [PATCH v2 2/5] acpi/x86: s2idle: handle Display On/Off calls outside of suspend sequence

[Mario Limonciello]

On 9/22/2024 12:22, Antheas Kapenekakis wrote:
> Currently, the Display On/Off calls are handled within the suspend
> sequence, which is a deviation from Windows. This causes issues with
> certain devices, where the notification interacts with a USB device
> that expects the kernel to be fully awake.
> 
> This patch calls the Display On/Off callbacks before entering the suspend
> sequence, which fixes this issue. In addition, it opens the possibility
> of modelling a state such as "Screen Off" that mirrors Windows, as the
> callbacks will be accessible and validated to work outside of the
> suspend sequence.
> 
> Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>   kernel/power/suspend.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index c527dc0ae5ae..610f8ecaeebd 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -589,6 +589,13 @@ static int enter_state(suspend_state_t state)
>   	if (state == PM_SUSPEND_TO_IDLE)
>   		s2idle_begin();
>   
> +	/*
> +	 * Linux does not have the concept of a "Screen Off" state, so call
> +	 * the platform functions for Display On/Off prior to the suspend
> +	 * sequence, mirroring Windows which calls them outside of it as well.
> +	 */
> +	platform_suspend_display_off();
> +

I thought about it some more over the weekend and if moving the screen 
on/off _DSM at all I don't feel this is the right place for triggering it.

IMO it should be called by DRM core.  That is when the number of CRTCs 
active goes 1+ -> 0 call screen off and when it goes 0->1+ call screen on.

There could be an argument made only for eDP this happens, but I think 
that's a slippery slope if you end up with an AIO design that uses DP 
instead of eDP or a desktop for example.  So the safest policy should be 
across all CRTCs of all GPUs.  During the suspend sequence any that were 
left on will get turned off, and then it could be triggered at that time 
instead.

By making such a change then when the compositor turns off all displays 
at runtime you can potentially support dark screen background downloads 
that have specifically called this _DSM and any actions that are taken 
from doing so.

>   	if (sync_on_suspend_enabled) {
>   		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
>   		ksys_sync_helper();
> @@ -616,6 +623,8 @@ static int enter_state(suspend_state_t state)
>   	suspend_finish();
>    Unlock:
>   	mutex_unlock(&system_transition_mutex);
> +
> +	platform_suspend_display_on();
>   	return error;
>   }
>   

Re: [PATCH v2 2/5] acpi/x86: s2idle: handle Display On/Off calls outside of suspend sequence

[Antheas Kapenekakis]

Does DRM core handle virtual displays like VNC?

As mentioned in the cover letter, the _DSM specifies both virtual and
actual displays.

Also, Windows behavior is like a lockscreen. 5 seconds after screen
turns off after inactivity, instantly when you press the power button.

I tend towards making a sysfs entry. Not sure.

Antheas

On Mon, 23 Sept 2024 at 18:03, Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 9/22/2024 12:22, Antheas Kapenekakis wrote:
> > Currently, the Display On/Off calls are handled within the suspend
> > sequence, which is a deviation from Windows. This causes issues with
> > certain devices, where the notification interacts with a USB device
> > that expects the kernel to be fully awake.
> >
> > This patch calls the Display On/Off callbacks before entering the suspend
> > sequence, which fixes this issue. In addition, it opens the possibility
> > of modelling a state such as "Screen Off" that mirrors Windows, as the
> > callbacks will be accessible and validated to work outside of the
> > suspend sequence.
> >
> > Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> >   kernel/power/suspend.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index c527dc0ae5ae..610f8ecaeebd 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -589,6 +589,13 @@ static int enter_state(suspend_state_t state)
> >       if (state == PM_SUSPEND_TO_IDLE)
> >               s2idle_begin();
> >
> > +     /*
> > +      * Linux does not have the concept of a "Screen Off" state, so call
> > +      * the platform functions for Display On/Off prior to the suspend
> > +      * sequence, mirroring Windows which calls them outside of it as well.
> > +      */
> > +     platform_suspend_display_off();
> > +
>
> I thought about it some more over the weekend and if moving the screen
> on/off _DSM at all I don't feel this is the right place for triggering it.
>
> IMO it should be called by DRM core.  That is when the number of CRTCs
> active goes 1+ -> 0 call screen off and when it goes 0->1+ call screen on.
>
> There could be an argument made only for eDP this happens, but I think
> that's a slippery slope if you end up with an AIO design that uses DP
> instead of eDP or a desktop for example.  So the safest policy should be
> across all CRTCs of all GPUs.  During the suspend sequence any that were
> left on will get turned off, and then it could be triggered at that time
> instead.
>
> By making such a change then when the compositor turns off all displays
> at runtime you can potentially support dark screen background downloads
> that have specifically called this _DSM and any actions that are taken
> from doing so.
>
> >       if (sync_on_suspend_enabled) {
> >               trace_suspend_resume(TPS("sync_filesystems"), 0, true);
> >               ksys_sync_helper();
> > @@ -616,6 +623,8 @@ static int enter_state(suspend_state_t state)
> >       suspend_finish();
> >    Unlock:
> >       mutex_unlock(&system_transition_mutex);
> > +
> > +     platform_suspend_display_on();
> >       return error;
> >   }
> >
>

Re: [PATCH v2 2/5] acpi/x86: s2idle: handle Display On/Off calls outside of suspend sequence

[Mario Limonciello]

On 9/23/2024 11:15, Antheas Kapenekakis wrote:
> Does DRM core handle virtual displays like VNC?
> 

You can make use of virtual display connectors in amdgpu.  This is how 
drivers for new ASICs are first developed in emulation and also what's 
used for early hardware bring up.

You can see virtual_display from 
https://www.kernel.org/doc/html/v6.11/gpu/amdgpu/module-parameters.html 
for more details.

> As mentioned in the cover letter, the _DSM specifies both virtual and
> actual displays.
> 
> Also, Windows behavior is like a lockscreen. 5 seconds after screen
> turns off after inactivity, instantly when you press the power button.
> 
> I tend towards making a sysfs entry. Not sure.
> 

Who would call this sysfs file?  Systemd?  The compositor?  When?

In Linux the compositor is in charge of doing the modesets that involve 
turning on/off the screen.  In most cases if you press the power button 
in Linux systemd-logind picks up the event.  It triggers a behavior 
that's controlled by the logind configuration.  Typically that's turning 
on a lockscreen and/or starting the suspend sequence.

Important to note; it DOESN'T explicitly turn off displays.  If you 
configured it to suspend then displays get turned off as part of the 
kernel suspend sequence (drm_atomic_helper_disable_all).

If it is configured to trigger a lockscreen then the compositor will 
turn off displays after whatever the DPMS configuration is set to.

Re: [PATCH v2 2/5] acpi/x86: s2idle: handle Display On/Off calls outside of suspend sequence

[Antheas Kapenekakis]

> On 9/23/2024 11:15, Antheas Kapenekakis wrote:
> > Does DRM core handle virtual displays like VNC?
> >
>
> You can make use of virtual display connectors in amdgpu.  This is how
> drivers for new ASICs are first developed in emulation and also what's
> used for early hardware bring up.

Microsoft specificies all displays "state where all displays—local and
remote, if any—have been turned off"

If any means that no displays = no call perhaps. Would be interesting
when designing a system for disabled people though. Given how it
interacts in Windows, I want to say the target here is inactivity.

> > As mentioned in the cover letter, the _DSM specifies both virtual and
> > actual displays.
> >
> > Also, Windows behavior is like a lockscreen. 5 seconds after screen
> > turns off after inactivity, instantly when you press the power button.
> >
> > I tend towards making a sysfs entry. Not sure.
> >
>
> Who would call this sysfs file?  Systemd?  The compositor?  When?
>
> In Linux the compositor is in charge of doing the modesets that involve
> turning on/off the screen.  In most cases if you press the power button
> in Linux systemd-logind picks up the event.  It triggers a behavior
> that's controlled by the logind configuration.  Typically that's turning
> on a lockscreen and/or starting the suspend sequence.

That's where it gets hairy and an area WIndows uniquely does not have
a problem with because the OS is monolithic.

If it were me, yes, systemd would switch the system to the inactive
"Screen Off" state 5 seconds after the system displays are off due to
inactivity. If we are talking about implementing something Modern
Standby-like, then pressing the powerbutton would no longer suspend
the device. Instead systemd would use two tiers of activators like
windows (first tier for "Screen Off", second tier for "Sleep"; right
now there is only one tier that mirrors screen off) and once all those
activators are released, suspend the kernel.

Then it would handle the transition from "Active" to "Screen Off" to
"Sleep" through a sysfs endpoint, with the platform responding by
e.g., lowering TDP and using a different fan curve.

If the kernel is asked to suspend outside of the Sleep state, then it
does the transitions to reach that state and references the quirk
table to avoid racing conditions in manufacturer firmware (not with
the kernel), before it suspends.

> Important to note; it DOESN'T explicitly turn off displays.  If you
> configured it to suspend then displays get turned off as part of the
> kernel suspend sequence (drm_atomic_helper_disable_all).
>
> If it is configured to trigger a lockscreen then the compositor will
> turn off displays after whatever the DPMS configuration is set to.

The problem with a DRM atomic helper is that we cannot control how it
is called and do debouncing. WIndows does debouncing (part of that is
waiting for 5 seconds so that if you move the mouse the call is
skipped). You could have edge conditions that spam the calls.

Antheas

Re: [PATCH v2 2/5] acpi/x86: s2idle: handle Display On/Off calls outside of suspend sequence

[Antheas Kapenekakis]

Here I should note that I will not wait for systemd.

As part of adding support for these devices with Handheld Daemon, I
snag the powerbutton already and I only have one userspace app running
anyway, Steam. The idea here would be double tap the powerbutton to
enter a "Download Assistant", custom lockscreen pops up, and two
seconds later DPMS triggers, system enters Sleep state and either when
a battery target is reached or the download finishes, it transitions
to actual suspend.

The challenges here would be if the _DSM calls for Display Off and
Sleep Entry are a nothingburger and do not save much battery and that
Steam uses very heavy compression that does a lot of CPU.

If I find it not useful, maybe I will skip it.

Antheas

On Mon, 23 Sept 2024 at 18:43, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>
> > On 9/23/2024 11:15, Antheas Kapenekakis wrote:
> > > Does DRM core handle virtual displays like VNC?
> > >
> >
> > You can make use of virtual display connectors in amdgpu.  This is how
> > drivers for new ASICs are first developed in emulation and also what's
> > used for early hardware bring up.
>
> Microsoft specificies all displays "state where all displays—local and
> remote, if any—have been turned off"
>
> If any means that no displays = no call perhaps. Would be interesting
> when designing a system for disabled people though. Given how it
> interacts in Windows, I want to say the target here is inactivity.
>
> > > As mentioned in the cover letter, the _DSM specifies both virtual and
> > > actual displays.
> > >
> > > Also, Windows behavior is like a lockscreen. 5 seconds after screen
> > > turns off after inactivity, instantly when you press the power button.
> > >
> > > I tend towards making a sysfs entry. Not sure.
> > >
> >
> > Who would call this sysfs file?  Systemd?  The compositor?  When?
> >
> > In Linux the compositor is in charge of doing the modesets that involve
> > turning on/off the screen.  In most cases if you press the power button
> > in Linux systemd-logind picks up the event.  It triggers a behavior
> > that's controlled by the logind configuration.  Typically that's turning
> > on a lockscreen and/or starting the suspend sequence.
>
> That's where it gets hairy and an area WIndows uniquely does not have
> a problem with because the OS is monolithic.
>
> If it were me, yes, systemd would switch the system to the inactive
> "Screen Off" state 5 seconds after the system displays are off due to
> inactivity. If we are talking about implementing something Modern
> Standby-like, then pressing the powerbutton would no longer suspend
> the device. Instead systemd would use two tiers of activators like
> windows (first tier for "Screen Off", second tier for "Sleep"; right
> now there is only one tier that mirrors screen off) and once all those
> activators are released, suspend the kernel.
>
> Then it would handle the transition from "Active" to "Screen Off" to
> "Sleep" through a sysfs endpoint, with the platform responding by
> e.g., lowering TDP and using a different fan curve.
>
> If the kernel is asked to suspend outside of the Sleep state, then it
> does the transitions to reach that state and references the quirk
> table to avoid racing conditions in manufacturer firmware (not with
> the kernel), before it suspends.
>
> > Important to note; it DOESN'T explicitly turn off displays.  If you
> > configured it to suspend then displays get turned off as part of the
> > kernel suspend sequence (drm_atomic_helper_disable_all).
> >
> > If it is configured to trigger a lockscreen then the compositor will
> > turn off displays after whatever the DPMS configuration is set to.
>
> The problem with a DRM atomic helper is that we cannot control how it
> is called and do debouncing. WIndows does debouncing (part of that is
> waiting for 5 seconds so that if you move the mouse the call is
> skipped). You could have edge conditions that spam the calls.
>
> Antheas

Re: [PATCH v2 2/5] acpi/x86: s2idle: handle Display On/Off calls outside of suspend sequence

[Mario Limonciello]

On 9/23/2024 11:43, Antheas Kapenekakis wrote:
> 
> If it were me, yes, systemd would switch the system to the inactive
> "Screen Off" state 5 seconds after the system displays are off due to
> inactivity. If we are talking about implementing something Modern
> Standby-like, then pressing the powerbutton would no longer suspend
> the device. Instead systemd would use two tiers of activators like
> windows (first tier for "Screen Off", second tier for "Sleep"; right
> now there is only one tier that mirrors screen off) and once all those
> activators are released, suspend the kernel.
> 
> Then it would handle the transition from "Active" to "Screen Off" to
> "Sleep" through a sysfs endpoint, with the platform responding by
> e.g., lowering TDP and using a different fan curve.
> 
> If the kernel is asked to suspend outside of the Sleep state, then it
> does the transitions to reach that state and references the quirk
> table to avoid racing conditions in manufacturer firmware (not with
> the kernel), before it suspends.
> 
>> Important to note; it DOESN'T explicitly turn off displays.  If you
>> configured it to suspend then displays get turned off as part of the
>> kernel suspend sequence (drm_atomic_helper_disable_all).
>>
>> If it is configured to trigger a lockscreen then the compositor will
>> turn off displays after whatever the DPMS configuration is set to.
> 
> The problem with a DRM atomic helper is that we cannot control how it
> is called and do debouncing. WIndows does debouncing (part of that is
> waiting for 5 seconds so that if you move the mouse the call is
> skipped). You could have edge conditions that spam the calls.
> 
> Antheas

I'm not meaning that anything in userspace SHOULD be calling 
`drm_atomic_helper_disable_all`, my point was that this is how it's 
normally called in the suspend sequence.  Folks who work on the 
compositors don't like anyone other than the kernel touching their
configuration at runtime.

I think a lot of what you're looking for is the concept of a systemwide 
"userspace only" suspend sequence.  A lot of devices already support 
runtime PM and will already go into the low power state when not in use. 
  For example USB4 routers you'll see in D3 until you plug something 
into the USB4 port.

IMO your proposal needs to cross at least 3 projects.  Here's my 
thoughts on it.

1) systemd
    * To be able to handle behaviors associates with a dark screen only
      suspend/resume.  I did start a discussion on dark resume some time
      back but it went nowhere. [1]

    * Make sure that all devices have been put into runtime PM.
    * Notify compositor that screens should be turned off.
    * Manage transitions from dark screen to full suspend and vice versa.

2) Kernel
   A. To be able to notify the _DSM at the right time that all CRTCs have
      been turned off).
   B. To be able to notify the _DSM at the right time that at least one
      CRTC is now on.

3) Wayland protocols
    Introduce a new protocol for requesting userspace suspend.
    To notify that dark screen only suspend is being triggered.
4) Compositor use.
    All the popular compositors would need to add support for the new
    protocol.  IE Gamescope, mutter, kwin.

This isn't a trivial effort, there are a lot of people to convince.  I 
think the lowest effort is to start with kernel.  IE having DRM core 
call the _DSM when the CRTCs are off.  If you get that far, you can at 
least get this power saving behavior when DPMS is enacted.  Then you can 
work with systemd and wayland protocol folks.


[1] https://github.com/systemd/systemd/issues/27077

Re: [PATCH v2 2/5] acpi/x86: s2idle: handle Display On/Off calls outside of suspend sequence

[Antheas Kapenekakis]

Did not mean how it is called as who will call it. But as in the way
it is called does not match the desired behavior.

In any case, as I said, I am happy to work in a downstream POC. My
target usecase is very simple and I do not need/want to tie Display
Off to CRTC.

ie, I will do 2 and 4. I am already familiar with gamescope (including
having two sockets open to it at any given time). Then if we get
interesting results, we move on from there.

As I said I also want to catch the Sleep _DSMs. I do not care about
Display On/Off other than calling it properly (where properly = as in
Windows + a lot of debouncing) so that these handhelds do not get
confused.

Antheas

On Mon, 23 Sept 2024 at 19:05, Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 9/23/2024 11:43, Antheas Kapenekakis wrote:
> >
> > If it were me, yes, systemd would switch the system to the inactive
> > "Screen Off" state 5 seconds after the system displays are off due to
> > inactivity. If we are talking about implementing something Modern
> > Standby-like, then pressing the powerbutton would no longer suspend
> > the device. Instead systemd would use two tiers of activators like
> > windows (first tier for "Screen Off", second tier for "Sleep"; right
> > now there is only one tier that mirrors screen off) and once all those
> > activators are released, suspend the kernel.
> >
> > Then it would handle the transition from "Active" to "Screen Off" to
> > "Sleep" through a sysfs endpoint, with the platform responding by
> > e.g., lowering TDP and using a different fan curve.
> >
> > If the kernel is asked to suspend outside of the Sleep state, then it
> > does the transitions to reach that state and references the quirk
> > table to avoid racing conditions in manufacturer firmware (not with
> > the kernel), before it suspends.
> >
> >> Important to note; it DOESN'T explicitly turn off displays.  If you
> >> configured it to suspend then displays get turned off as part of the
> >> kernel suspend sequence (drm_atomic_helper_disable_all).
> >>
> >> If it is configured to trigger a lockscreen then the compositor will
> >> turn off displays after whatever the DPMS configuration is set to.
> >
> > The problem with a DRM atomic helper is that we cannot control how it
> > is called and do debouncing. WIndows does debouncing (part of that is
> > waiting for 5 seconds so that if you move the mouse the call is
> > skipped). You could have edge conditions that spam the calls.
> >
> > Antheas
>
> I'm not meaning that anything in userspace SHOULD be calling
> `drm_atomic_helper_disable_all`, my point was that this is how it's
> normally called in the suspend sequence.  Folks who work on the
> compositors don't like anyone other than the kernel touching their
> configuration at runtime.
>
> I think a lot of what you're looking for is the concept of a systemwide
> "userspace only" suspend sequence.  A lot of devices already support
> runtime PM and will already go into the low power state when not in use.
>   For example USB4 routers you'll see in D3 until you plug something
> into the USB4 port.
>
> IMO your proposal needs to cross at least 3 projects.  Here's my
> thoughts on it.
>
> 1) systemd
>     * To be able to handle behaviors associates with a dark screen only
>       suspend/resume.  I did start a discussion on dark resume some time
>       back but it went nowhere. [1]
>
>     * Make sure that all devices have been put into runtime PM.
>     * Notify compositor that screens should be turned off.
>     * Manage transitions from dark screen to full suspend and vice versa.
>
> 2) Kernel
>    A. To be able to notify the _DSM at the right time that all CRTCs have
>       been turned off).
>    B. To be able to notify the _DSM at the right time that at least one
>       CRTC is now on.
>
> 3) Wayland protocols
>     Introduce a new protocol for requesting userspace suspend.
>     To notify that dark screen only suspend is being triggered.
> 4) Compositor use.
>     All the popular compositors would need to add support for the new
>     protocol.  IE Gamescope, mutter, kwin.
>
> This isn't a trivial effort, there are a lot of people to convince.  I
> think the lowest effort is to start with kernel.  IE having DRM core
> call the _DSM when the CRTCs are off.  If you get that far, you can at
> least get this power saving behavior when DPMS is enacted.  Then you can
> work with systemd and wayland protocol folks.
>
>
> [1] https://github.com/systemd/systemd/issues/27077

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

[Hans de Goede]

Hi,

On 22-Sep-24 7:22 PM, Antheas Kapenekakis wrote:
> The following series moves the Display off/on calls outside of the suspend
> sequence, as they are performed in Windows. This fixes certain issues that appear
> in devices that use the calls and expect the kernel to be active during their
> call (especially in the case of the ROG Ally devices) and opens the possibility
> of implementing a "Screen Off" state in the future (which mirrors Windows).
> In addition, it adds a quirk table that will allow for adding delays between
> Modern Standby transitions, to help resolve racing conditions.
> 
> This series requires a bit of background on how modern standby works in Windows.
> Fundamentally, it is composed of four states: "Active", "Screen Off", "Sleep",
> and "DRIPS". Here, I take the liberty of naming the state "Active", as it is
> implied in Windows documentation.
> 
> When the user actively interacts with a device, it is in the "Active" state.
> The screen is on, all devices are connected, and desired software is running.
> The other 3 stages play a role once the user stops interacting with the device.
> This is triggered in two main ways: either by pressing the power button or by
> inactivity. Once either of those targets is met, the system enters Modern Standby.
> 
> Modern Standby consists of an orchestration of the "Screen Off", "Sleep", and
> "DRIPS" states. Windows is free to move throughout these states until the user
> interacts with the device again, where the device will transition to being
> "Active". Moving between the states implies a transition, where Windows performs
> a set of actions. In addition, Windows can only move between adjacent states
> as follows:
> 
> "Active" <-> "Screen Off" <-> "Sleep" <-> "DRIPS"
> 
> "Screen Off" is the state where all active displays in the device (whether
> *virtual* or real; this means unrelated to DRM) are off. The user might still
> be interacting with the device or running programs (e.g., compiling a kernel).
> 
> "Sleep" is a newer state, in which the device turns off its fan and pulses its
> power button, but still supports running software activities. As part of this,
> and to avoid overheating the device a lot of manufacturers lower the TDP (PLx)
> of the device [3; _DSM 9 description].
> 
> Finally, DRIPS stands for Deepest Runtime Idle Power State, i.e. suspend.
> 
> While Windows may transition from any state to any state, doing so implies
> performing all transitions to reach that state. All states other than DRIPS
> have a fully active kernel (Wifi, USB at least) and allow userspace activity.
> What changes is the extent of the activity, and whether some power consuming
> devices are turned off (this is done with Modern Standby aware drivers and
> firmware notifications). The Windows kernel suspends during the transition from
> the "Sleep" state to the "DRIPS" state. In all other states it is active.
> 
> After finishing each transition, the kernel performs a firmware notification,
> described in the _DSM document [3]. Moving from left to right with function num.,
> we have Display Off (3; Active -> Screen Off), Sleep Entry (7; Screen Off -> Sleep),
> and Lowest Power State Entry Notification (5; LPSEN; Sleep -> DRIPS). Then, from
> right to left, we have Lowest Power State Exit Notification (6; DRIPS -> Sleep),
> Sleep Exit (8; Sleep -> Screen) and Display On (4; Screen Off -> Active).
> 
> The Linux kernel is not currently Modern Standby aware but will still make these
> calls. Currently, the problem is that the kernel calls all of the firmware
> notifications at the point LPSEN (5, 6) should be called, which is when the
> kernel is mostly suspended. This is a clear deviation from Windows, and causes
> undesirable behavior in certain devices, the main one focused in this patch
> series being the ROG Ally. Although series patch is aimed at Modern Standby
> devices in general.
> 
> The ROG Ally is a Modern Standby capable device (uses Secure Core too; really
> ticks all the MS boxes) and in it, there are issues with both Display 3,4
> calls and Sleep 7,8 calls cause issues (7,8 are suspected and todo).
> 
> The Display 3,4 calls are responsible for the controller. The Display Off call
> disconnects (if powersave is off) or powers off (if powersave is on and on DC
> power) the MCU(s) responsible for the controller and deactivates the RGB of the
> device. Display On powers on or reconnects the controller respectively.
> This controller, in the Ally X, is composed of 6 HID devices that register to
> the kernel. From testing, it seems that the majority of the problem in the Ally
> comes from Display Off being called way too late timewise, and Display
> 
> The Sleep 7,8 calls, in general, are responsible for setting a low power state
> that is safe to use while the device is sleeping, making the suspend light
> pulse, and turning off the fan. Due to a variety of race conditions, there is
> a rare occasion where the Ally EC can get stuck in its Sleep mode, where the
> TDP is 5W, and prevent increasing it until the device reboots. The sleep entries
> contain actions in the Ally, so there is a suspicion that calling them during
> DRIPS is causing issues. However, this is not the subject of this patch and
> has not been verified yet.
> 
> This patch centers around moving the Display 3,4 calls outside the suspend
> sequence (which is the transition from Sleep to DRIPS in Modern Standby terms),
> and by implementing the proper locks necessary, opening up the possibility of
> making these calls as part of a more elaborate "Modern Standby"-like userspace
> suspend/wakelock implementation. As of this patch, they are only called before
> the suspend sequence, including with the possibility of adding a delay.
> 
> This makes the intent of this patch primarily compatibility focused, as it aims
> to fix issues by the current implementation. And to that end it works.
> After moving the calls outside of the suspend sequence, my ROG Ally X test unit
> can suspend more than 50 times without rebooting, both with powersave on or off,
> regardless of whether it is plugged/unplugged during suspend, and still have the
> controller work with full reliability. In V1, there was an unsolved race condition
> that was dealt by (5) before Display Off triggers. Essentially, Linux suspends
> too fast for the current version of the firmware to deal with. After adding a
> quirk table, which delays suspend after the Display Off call, the controller
> of the original Ally should power off properly (a lot of testing will be done).

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.

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.

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.

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.

Rafael, what do you think about at least taking patches 1 - 3 upstream?
Reading through how Windows handles things making the display on/off
calls before suspending devices sounds like it is the right thing to do
to me.

Regards,

Hans

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

[Antheas Kapenekakis]

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. As it will probably cause
regressions if both are in place 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. 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.

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?

> Rafael, what do you think about at least taking patches 1 - 3 upstream?
> Reading through how Windows handles things making the display on/off
> calls before suspending devices sounds like it is the right thing to do
> to me.

Antheas

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

[Hans de Goede]

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

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

[Hans de Goede]

p.s.

On 5-Oct-24 6:24 PM, Hans de Goede wrote:
> Hi Antheas,
> 
> On 5-Oct-24 5:10 PM, Antheas Kapenekakis wrote:
>> Hi Hans,
>>

<snip>

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

Oh and upon re-reading the series I see that pathc 5 is just dropping
the quirks from asus-wmi.c, which is fine.

I somehow thought that the later patches where adding a way for userspace
to already enter the LPS0 display off state earlier. No idea how that
idea got in my head ...

Regards,

Hans

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

[Antheas Kapenekakis]

On Sat, 5 Oct 2024 at 18:27, Hans de Goede <hdegoede@redhat.com> wrote:
>
> p.s.
>
> On 5-Oct-24 6:24 PM, Hans de Goede wrote:
> > Hi Antheas,
> >
> > On 5-Oct-24 5:10 PM, Antheas Kapenekakis wrote:
> >> Hi Hans,
> >>
>
> <snip>
>
> >>> 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.
>
> Oh and upon re-reading the series I see that pathc 5 is just dropping
> the quirks from asus-wmi.c, which is fine.
>
> I somehow thought that the later patches where adding a way for userspace
> to already enter the LPS0 display off state earlier. No idea how that
> idea got in my head ...

Done this way to hopefully be easier to upstream and get this fix out
sooner. The plan here would be 3 series: 1) move Display On/Off +
quirk, 2) move Sleep Entry/Exit + Quirk, 3) RFC for exposing to
userspace, in which case if the kernel starts to suspend while in
standby it would skip those calls.

Antheas

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

[Antheas Kapenekakis]

On Sat, 5 Oct 2024 at 18:24, Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.

Probably it will be harmless, but I think the Display On being done
twice, and one of the times being inside the suspend sequence might
result in sub-optimal behavior. Well, the behavior that exists now.

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

Refer to [1]. CSEE with B7, B8 is in fact the _DSM 3,4 Display On/Off
internal call. There is also a spurious Lid switch call on Display On
that does not exist and causes dmesg errors.

Link: https://github.com/hhd-dev/hwinfo/blob/21b7442219922233c41c0568995214ba92873f69/devices/ally_x/decoded/ssdt28.dsl#L841-L855
[1]

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

All are fair comments. I will fix them on the next revision.

On the Patch 3 comment, do you think there is merit with blending the
quirk with userspace freezing? Moving it inside LPS0 would make that
difficult, however at the same time 500ms for just the Ally (and
perhaps 2-3 other affected devices) is not something particularly
noticeable anyway.

Antheas

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

[Hans de Goede]

Hi Antheas,

On 5-Oct-24 6:24 PM, Hans de Goede wrote:
> 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.

Thinking some more about this I am having second doubts about
moving the LPS0 display power off call to before devices are suspended,
doing so would mean that the display might still be on when that call
is made and that call could disable power-resources which are necessary
for the display causing issues when the display driver's suspend method
runs.

So I think that we need something closer to Mario's POC from:

https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git/log/?h=superm1/dsm-screen-on-off

here where the call is made when the last display is turned off.

IOW have the drm modesetting core call this.

Maybe have something like a enabled_displays counter in the
drm-core which gets increased / decreased by helpers and
have the drm-core call platform_suspend_screen_off() /
platform_suspend_screen_on() when the counter goes from 1 -> 0
resp. 0 -> 1, ignoring the very first 0 -> 1 transition
which will be done when the first GPU with an enabled
output is found ?

The idea being that the first increase() call gets made when
a drm/kms driver probes a display and finds outputs which are
light up during probe() and then further increase / decrease
calls are made either when all displays go off; or maybe
per crtc when the crtc gets enabled / disabled.

Anyways how best to do this at display off time should be
discussed with the drm/kms community on the dri-devel list.

Regards,

Hans

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

[Antheas Kapenekakis]

<skip>

> Thinking some more about this I am having second doubts about
> moving the LPS0 display power off call to before devices are suspended,
> doing so would mean that the display might still be on when that call
> is made and that call could disable power-resources which are necessary
> for the display causing issues when the display driver's suspend method
> runs.

Is there any device where that is used for display powersaving?

> So I think that we need something closer to Mario's POC from:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git/log/?h=superm1/dsm-screen-on-off
>
> here where the call is made when the last display is turned off.
>
> IOW have the drm modesetting core call this.

I can see two problems with this approach: 1) it would happen in a
random point in the suspend sequence, introducing race conditions with
sensitive modern standby devices (e.g., Ally). 2) It would not be
gated and debounced properly, so a drm driver could call it 5 times
when you e.g., plug in an HDMI cable.

And indeed that is the case, that PR horribly breaks the Ally even
while any asus-wmi quirk was active. Perhaps DRM can be consulted
though, see below.

> Maybe have something like a enabled_displays counter in the
> drm-core which gets increased / decreased by helpers and
> have the drm-core call platform_suspend_screen_off() /
> platform_suspend_screen_on() when the counter goes from 1 -> 0
> resp. 0 -> 1, ignoring the very first 0 -> 1 transition
> which will be done when the first GPU with an enabled
> output is found ?

To quote Microsoft [1], "This _DSM Function will be invoked when the
operating system has entered a state where all displays—local and
*remote*, if any—have been turned off."

Since it says remote, binding it to DRM could prove difficult. In
addition, the call in Windows is made 5 seconds after the displays
turn off due to inactivity. To mirror this behavior you would need
userspace.

If there is strong indication that the Display On/Off calls interfere
with the DRM subsystem, e.g., turn off a GPU in certain laptops, the
call could be gated with a counter similar to Mario's PR and error
out. In that way, it is still controllable by userspace while ensuring
the device display is off. Is there such an indication/do we know of
such a device?

The Ally and Legion Go which I tested happily had their display turn
on after I yanked their display on and sleep exit callbacks. The
Legion Go even had its suspend light blink while the screen was on.
And both had disabled controllers. This behavior was sticky even after
a reboot. I suppose this is due to the fact that the device might
hibernate, so the EC would have to remember the last state before
power off.

> The idea being that the first increase() call gets made when
> a drm/kms driver probes a display and finds outputs which are
> light up during probe() and then further increase / decrease
> calls are made either when all displays go off; or maybe
> per crtc when the crtc gets enabled / disabled.
>
> Anyways how best to do this at display off time should be
> discussed with the drm/kms community on the dri-devel list.

I can cc on the next version.

Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications#display-off-notification-function-3
[1]

Antheas

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

[Hans de Goede]

Hi,

On 6-Oct-24 12:15 AM, Antheas Kapenekakis wrote:
> <skip>
> 
>> Thinking some more about this I am having second doubts about
>> moving the LPS0 display power off call to before devices are suspended,
>> doing so would mean that the display might still be on when that call
>> is made and that call could disable power-resources which are necessary
>> for the display causing issues when the display driver's suspend method
>> runs.
> 
> Is there any device where that is used for display powersaving?

The problem is that we cannot rule out that the LPS0 display off
call relies on the displays actually being off.

I have seen ACPI AML code do all sort of crazy stuff.

So IMHO we really need to make sure that all physical displays
are off before we make the LPS0 display off call.

I have read what you wrote about this also applying to virtual
displays, I guess that means that there should be no rendering done
(so also no GPU non display tasks) when this is called.

IOW it might be best to tie this to all VGA class PCI devices
being in D3 as Mario suggested.

Regards,

Hans








>> So I think that we need something closer to Mario's POC from:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git/log/?h=superm1/dsm-screen-on-off
>>
>> here where the call is made when the last display is turned off.
>>
>> IOW have the drm modesetting core call this.
> 
> I can see two problems with this approach: 1) it would happen in a
> random point in the suspend sequence, introducing race conditions with
> sensitive modern standby devices (e.g., Ally). 2) It would not be
> gated and debounced properly, so a drm driver could call it 5 times
> when you e.g., plug in an HDMI cable.
> 
> And indeed that is the case, that PR horribly breaks the Ally even
> while any asus-wmi quirk was active. Perhaps DRM can be consulted
> though, see below.
> 
>> Maybe have something like a enabled_displays counter in the
>> drm-core which gets increased / decreased by helpers and
>> have the drm-core call platform_suspend_screen_off() /
>> platform_suspend_screen_on() when the counter goes from 1 -> 0
>> resp. 0 -> 1, ignoring the very first 0 -> 1 transition
>> which will be done when the first GPU with an enabled
>> output is found ?
> 
> To quote Microsoft [1], "This _DSM Function will be invoked when the
> operating system has entered a state where all displays—local and
> *remote*, if any—have been turned off."
> 
> Since it says remote, binding it to DRM could prove difficult. In
> addition, the call in Windows is made 5 seconds after the displays
> turn off due to inactivity. To mirror this behavior you would need
> userspace.
> 
> If there is strong indication that the Display On/Off calls interfere
> with the DRM subsystem, e.g., turn off a GPU in certain laptops, the
> call could be gated with a counter similar to Mario's PR and error
> out. In that way, it is still controllable by userspace while ensuring
> the device display is off. Is there such an indication/do we know of
> such a device?
> 
> The Ally and Legion Go which I tested happily had their display turn
> on after I yanked their display on and sleep exit callbacks. The
> Legion Go even had its suspend light blink while the screen was on.
> And both had disabled controllers. This behavior was sticky even after
> a reboot. I suppose this is due to the fact that the device might
> hibernate, so the EC would have to remember the last state before
> power off.
> 
>> The idea being that the first increase() call gets made when
>> a drm/kms driver probes a display and finds outputs which are
>> light up during probe() and then further increase / decrease
>> calls are made either when all displays go off; or maybe
>> per crtc when the crtc gets enabled / disabled.
>>
>> Anyways how best to do this at display off time should be
>> discussed with the drm/kms community on the dri-devel list.
> 
> I can cc on the next version.
> 
> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications#display-off-notification-function-3
> [1]
> 
> Antheas
> 

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

[Antheas Kapenekakis]

Hi Hans,
there is no rush from my end to see this series merge. The current
asus-wmi quirk works well for the Ally, all firmwares. New firmware is
also supposed to fix powersaving with it.

Yes, that quirk is suboptimal as it adds a noticeable delay to suspend
and resume and blocks powersaving from working correctly in old
firmwares. However, as most Ally users are on custom kernels anyway,
and this patch series can be merged quite easily into them, broadly
speaking it is a non-issue if the mainline kernel has ideal behavior
on the Ally for the next few months. From user feedback, they did not
notice changing to this patch anyway, other than the powersaving
benefit ("did you change anything? my ally just uses less power now").

Next revision I will cc dri so we get feedback from there too.

> >> Thinking some more about this I am having second doubts about
> >> moving the LPS0 display power off call to before devices are suspended,
> >> doing so would mean that the display might still be on when that call
> >> is made and that call could disable power-resources which are necessary
> >> for the display causing issues when the display driver's suspend method
> >> runs.
> >
> > Is there any device where that is used for display powersaving?
>
> The problem is that we cannot rule out that the LPS0 display off
> call relies on the displays actually being off.
>
> I have seen ACPI AML code do all sort of crazy stuff.

Indeed, as this series shows (for other reasons).

> So IMHO we really need to make sure that all physical displays
> are off before we make the LPS0 display off call.

In my use-case I'd like to be able to fire the display off
notification prior to turning off the screen or turn on the screen
after both Display Off and Sleep Entry have fired to be able to show a
dim lockscreen if the user briefly interacts with the device. I will
be doing this downstream for a limited set of prevalidated devices
though.

Therefore, before we block this behavior in a non-certain manner (as
it will be up to each gpu driver to do it), there needs to be some
documentation showing there is an issue, certain manufacturers rely on
the behavior, or that Microsoft has guaranteed that this is the case
in Windows. Even with any of the former, a blacklist quirk for those
manufacturers can be put in place in their GPU driver that blocks the
Display Off _DSM from firing and automatically calls the Display On
_DSM if the GPU wants to wake up the display.

> I have read what you wrote about this also applying to virtual
> displays, I guess that means that there should be no rendering done
> (so also no GPU non display tasks) when this is called.
>
> IOW it might be best to tie this to all VGA class PCI devices
> being in D3 as Mario suggested.

GPU can do stuff while the screen is off: render videos, hold browser
videos in main memory, have a game in the background, etc. If by D3
you mean the whole GPU has powered off, this would be a deviation from
Windows.

Antheas