diff options
Diffstat (limited to 'SOURCES/v2-ally-suspend-fix.patch')
| -rw-r--r-- | SOURCES/v2-ally-suspend-fix.patch | 534 |
1 files changed, 534 insertions, 0 deletions
diff --git a/SOURCES/v2-ally-suspend-fix.patch b/SOURCES/v2-ally-suspend-fix.patch new file mode 100644 index 0000000..ef4716e --- /dev/null +++ b/SOURCES/v2-ally-suspend-fix.patch @@ -0,0 +1,534 @@ +From 3153735388746f8033a08f1388f3ee31de8975a3 Mon Sep 17 00:00:00 2001 +From: Antheas Kapenekakis <lkml@antheas.dev> +Date: Fri, 20 Sep 2024 08:32:52 +0200 +Subject: [PATCH v2 0/4] acpi/x86: s2idle: move Display off/on calls outside + suspend (fixes ROG Ally suspend) + +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). + +This series requires a bit of background on how modern standby works in Windows. +Fundamentally, it is composed of two stages: "Screen Off" and "Sleep". +Each stage consists of a series of steps, with the former focusing on software +and the latter focusing on the kernel. + +The "Screen Off" stage is a software only stage, where the kernel and drivers +are fully active. In this stage, software is supposed to minimize non-essential +activity and to slowly coalesce into a state of non-activity over a period of +minutes. This can last from 1 second to hours, depending on device state (e.g., +if it is plugged in). During research of this patch on Windows, most times it +fluxuates between 5 seconds to 10 minutes. To aid in battery life and in how +fast the system suspends, Microsoft provides the _DSM firmware notifications +"Display On" and "Display Off" that can be used to deactivate unnecessary +hardware (e.g., RGB). + +The "Sleep" stage mirrors the traditional suspend sequence, in which kernel +components are succesively suspended to reach a low power state. + +Currently, the kernel calls the Display On/Off calls during a late part of the +suspend sequence, where most drivers have already suspended. Since these calls +are not often used (and if they are, they are mostly used for lid switches), +this has not caused issues up to now (although a little birdie tells me those +lid sensors might not be behaving correctly currently). + +That was until the release of ROG Ally, where Asus placed a callback that turns +the controller on and off in those _DSM calls. The Display Off call disconnects +(if powersave is off) or powers off (if powersave is on and on DC power) the MCU +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. As can be inferred, the handling of the calls during the suspend +sequence leads to a set of undesirable outcomes, such as the controller +soft-locking or only half of the HID devices coming back after resume. + +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 addition, moving the calls outside of the suspend sequence (and the validation +work it implies) opens the possibility of implementing a "Screen Off" state in +the future. This state would make a great addition to something like logind or +systemd, if it is exposed over a sysfs endpoint. The recommendation here would +be to allow userspace to instruct the kernel to enter "Screen Off" state when +the device becomes inactive. The kernel would then call "Display Off" and +delegade the responsibility of exiting "Screen Off" (and calling Display On) +to userspace, regardless of the number of the suspensions afterwards. +If userspace does not make the kernel enter "Screen Off" prior to suspend, the +kernel would call Display Off and On before suspending, in the same way it is +done with this patch. + +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 difference +between "Screen Off" and "Sleep" 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 (in most +cases it is a NOOP or a lid sensor). + +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 suspend is indicated with the suspend +light blinking. + +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 and as such it is +not something that would be a great fit for tying into the DRM subsystem. +If controlled by userspace and part of the screen turning off, it would also +solve several behavioral issues that currently exist. For example, 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, now that I know what the expected behavior is in +Windows. + +This patch series is co-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 (4) and an issue on amd-drm in 2023 in preparation for the +work in that quirk [6]. + +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] + +Changes in v2: + - 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) + +Antheas Kapenekakis (4): + acpi/x86: s2idle: add support for screen off and screen on callbacks + acpi/x86: s2idle: handle screen off/on calls outside of suspend + sequence + acpi/x86: s2idle: call screen on and off as part of callbacks + platform/x86: asus-wmi: remove Ally (1st gen) and Ally X suspend quirk + + drivers/acpi/x86/s2idle.c | 65 +++++++++++++++++++++++++-------- + drivers/platform/x86/asus-wmi.c | 54 --------------------------- + include/linux/suspend.h | 5 +++ + kernel/power/suspend.c | 28 ++++++++++++++ + 4 files changed, 83 insertions(+), 69 deletions(-) + +-- +2.46.1 + +From f9867c795cd94123c7a401a3adf50e0a74081fd6 Mon Sep 17 00:00:00 2001 +From: Antheas Kapenekakis <lkml@antheas.dev> +Date: Wed, 18 Sep 2024 23:53:53 +0200 +Subject: [PATCH v2 1/4] acpi/x86: s2idle: add support for screen off and + screen on callbacks + +The screen off and screen on firmware functions 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). + +In this state, the kernel and userspace are fully active, and the user +might still be interacting with the system somehow (such as with +listening to music or having a hotspot). Userspace is supposed to +minimize non-essential activities, but this is not required. +In addition, there is no requirement of suspending post the screen off +call. If the user interacts with the system, the kernel should call +screen on and resume normal operation. + +This patch adds a set of callbacks to allow calling the Display On/Off +firmware calls 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..96ceaad07839 100644 +--- a/include/linux/suspend.h ++++ b/include/linux/suspend.h +@@ -132,6 +132,7 @@ struct platform_suspend_ops { + }; + + struct platform_s2idle_ops { ++ int (*screen_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 (*screen_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_screen_off(void); ++int platform_suspend_screen_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..19734b297527 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_screen_off(void) ++{ ++ return s2idle_ops && s2idle_ops->screen_off ? s2idle_ops->screen_off() : 0; ++} ++EXPORT_SYMBOL_GPL(platform_suspend_screen_off); ++ ++int platform_suspend_screen_on(void) ++{ ++ return s2idle_ops && s2idle_ops->screen_on ? s2idle_ops->screen_on() : 0; ++} ++EXPORT_SYMBOL_GPL(platform_suspend_screen_on); ++ + static int platform_suspend_prepare(suspend_state_t state) + { + return state != PM_SUSPEND_TO_IDLE && suspend_ops->prepare ? +-- +2.46.1 + + +From 487bbe29975a115e7af69c07acc8888f10f77d93 Mon Sep 17 00:00:00 2001 +From: Antheas Kapenekakis <lkml@antheas.dev> +Date: Thu, 19 Sep 2024 00:02:32 +0200 +Subject: [PATCH v2 2/4] acpi/x86: s2idle: handle screen off/on calls outside + of suspend sequence + +Currently, the screen off/on calls are handled within the suspend +sequence, which is a deviation from Windows. This causes issues with +certain devices, such as the ROG Ally, which expects this call to be +executed with the kernel fully awake. The subsequent half-suspended +state makes the controller of the device to fail to suspend properly. + +This patch calls the screen off/on 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 | 16 ++++++++++++++++ + 1 file changed, 16 insertions(+) + +diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c +index 19734b297527..2e391c1b1d20 100644 +--- a/kernel/power/suspend.c ++++ b/kernel/power/suspend.c +@@ -507,6 +507,20 @@ int suspend_devices_and_enter(suspend_state_t state) + + pm_suspend_target_state = state; + ++ /* ++ * Linux does not have the concept of a "Screen Off" state, so call ++ * the platform functions for screen off prior to beginning the suspend ++ * sequence, mirroring Windows which calls them outside of it as well. ++ * ++ * If Linux ever gains a "Screen Off" state, the following callbacks can ++ * be replaced with a call that checks if we are in "Screen Off", in which ++ * case they will NOOP and if not call them as a fallback. ++ * ++ * In case of an error, since these calls are not part of the suspend ++ * sequence, do not bail. ++ */ ++ platform_suspend_screen_off(); ++ + if (state == PM_SUSPEND_TO_IDLE) + pm_set_suspend_no_platform(); + +@@ -540,6 +554,8 @@ int suspend_devices_and_enter(suspend_state_t state) + Close: + platform_resume_end(state); + pm_suspend_target_state = PM_SUSPEND_ON; ++ ++ platform_suspend_screen_on(); + return error; + + Recover_platform: +-- +2.46.1 + + +From 7ea23d70a9d945ff1b80290a1dd65af313d21172 Mon Sep 17 00:00:00 2001 +From: Antheas Kapenekakis <lkml@antheas.dev> +Date: Thu, 19 Sep 2024 00:22:03 +0200 +Subject: [PATCH v2 3/4] acpi/x86: s2idle: call screen on and off as part of + callbacks + +Move the screen on and off calls 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). + +Co-developed-by: Mario Limonciello <mario.limonciello@amd.com> +Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> +--- + drivers/acpi/x86/s2idle.c | 65 ++++++++++++++++++++++++++++++--------- + 1 file changed, 50 insertions(+), 15 deletions(-) + +diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c +index dd0b40b9bbe8..6b28d8b892d8 100644 +--- a/drivers/acpi/x86/s2idle.c ++++ b/drivers/acpi/x86/s2idle.c +@@ -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_screen_off; + + /* Device constraint entry structure */ + struct lpi_device_info { +@@ -539,15 +540,16 @@ static struct acpi_scan_handler lps0_handler = { + .attach = lps0_device_attach, + }; + +-int acpi_s2idle_prepare_late(void) ++static int acpi_s2idle_screen_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_screen_off))) ++ return -EINVAL; ++ ++ lsp0_dsm_in_screen_off = true; ++ acpi_scan_lock_acquire(); + + /* Screen off */ + if (lps0_dsm_func_mask > 0) +@@ -560,6 +562,47 @@ int acpi_s2idle_prepare_late(void) + acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF, + lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); + ++ acpi_scan_lock_release(); ++ ++ return 0; ++} ++ ++static int acpi_s2idle_screen_on(void) ++{ ++ if (!lps0_device_handle || sleep_no_lps0) ++ return 0; ++ ++ if (unlikely(WARN_ON(!lsp0_dsm_in_screen_off))) ++ return -EINVAL; ++ ++ lsp0_dsm_in_screen_off = false; ++ acpi_scan_lock_acquire(); ++ ++ /* 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); ++ ++ 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 = { ++ .screen_off = acpi_s2idle_screen_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, ++ .screen_on = acpi_s2idle_screen_on, + }; + + void __init acpi_s2idle_setup(void) +-- +2.46.1 + + +From 3153735388746f8033a08f1388f3ee31de8975a3 Mon Sep 17 00:00:00 2001 +From: Antheas Kapenekakis <lkml@antheas.dev> +Date: Thu, 19 Sep 2024 00:29:59 +0200 +Subject: [PATCH v2 4/4] platform/x86: asus-wmi: remove Ally (1st gen) and Ally + X suspend quirk + +By moving the Display On/Off calls outside of the suspend sequence, +the racing conditions that made the Ally controller suspend unreliable +are completely fixed. This includes both when mcu_powersave is enabled +and disabled. Therefore, remove the quirk that fixed them only when +the mcu_powersave attribute was disabled. + +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 +@@ -126,11 +126,6 @@ + #define ASUS_SCREENPAD_BRIGHT_MAX 255 + #define ASUS_SCREENPAD_BRIGHT_DEFAULT 60 + +-/* 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 300 +- + static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL }; + + static int throttle_thermal_policy_write(struct asus_wmi *); +@@ -298,9 +298,6 @@ + + bool fnlock_locked; + +- /* The ROG Ally device requires the MCU USB device be disconnected before suspend */ +- bool ally_mcu_usb_switch; +- + struct asus_wmi_debug debug; + + struct asus_wmi_driver *driver; +@@ -4445,8 +4445,6 @@ + asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_THERM_TARGET); + asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD); + asus->mini_led_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE); +- asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE) +- && dmi_match(DMI_BOARD_NAME, "RC71L"); + + err = fan_boost_mode_check_present(asus); + if (err) +@@ -4624,42 +4624,6 @@ + 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) { +- 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); +- int result, err; +- +- if (asus->ally_mcu_usb_switch) { +- /* When powersave is enabled it causes many issues with resume of USB hub */ +- result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MCU_POWERSAVE); +- if (result == 1) { +- dev_warn(device, "MCU powersave enabled, disabling to prevent resume issues"); +- err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, 0, &result); +- if (err || result != 1) +- dev_err(device, "Failed to set MCU powersave mode: %d\n", err); +- } +- /* 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); +@@ -4912,8 +4872,6 @@ + .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 + |