| Message ID | 20241121172239.119590-12-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace | expand |
On Thu, 21 Nov 2024 at 19:04, Mario Limonciello <mario.limonciello@amd.com> wrote: > > On 11/21/2024 11: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. > > > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > > --- > > drivers/acpi/x86/s2idle.c | 56 +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 56 insertions(+) > > > > diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c > > index d389c57d2963..504e6575d7ad 100644 > > --- a/drivers/acpi/x86/s2idle.c > > +++ b/drivers/acpi/x86/s2idle.c > > @@ -18,6 +18,7 @@ > > #include <linux/acpi.h> > > #include <linux/device.h> > > #include <linux/dmi.h> > > +#include <linux/delay.h> > > #include <linux/suspend.h> > > > > #include "../sleep.h" > > @@ -91,11 +92,50 @@ struct lpi_device_constraint_amd { > > int min_dstate; > > }; > > > > +struct s2idle_delay_quirks { > > + int delay_display_off; > > + int delay_sleep_entry; > > + int delay_sleep_exit; > > + int delay_display_on; > > +}; > > Historically these "kinds" of quirks are kept in drivers/acpi/x86/utils.c. > > Could it be moved there? Or perhaps stored in the ASUS drivers and > callbacks? Yes, it could definitely be moved there. > This feels cleaner if you used "struct acpi_s2idle_dev_ops" and > callbacks. More below. I can convert the quirk into 4 callbacks and put it there if it is better. But note that the exit delays are added before the firmware call and the entry delays are added after. I guess this makes sense as a callback form as well. > > + > > +/* > > + * The ROG Ally series disconnects its controllers on Display Off and performs > > + * a fancy shutdown sequence, which requires around half a second to complete. > > + * If the power is cut earlier by entering it into D3, the original Ally unit > > + * might not disconnect its XInput MCU, causing excess battery drain, and the > > + * Ally X will make the controller restart post-suspend. In addition, the EC > > + * of the device rarely (1/20 attempts) may get stuck asserting PROCHOT after > > + * suspend (for various reasons), so split the delay between Display Off and > > + * Sleep Entry. > > + */ > > +static const struct s2idle_delay_quirks rog_ally_quirks = { > > + .delay_display_off = 350, > > + .delay_sleep_entry = 150, > > +}; > > Is this delay still needed with Ally MCU 319 that has the fixes from ASUS? > > I'm suspecting not, which means this quirk should be made more narrow IMO. Yes, it is definitely needed. I have had at least two users break the new firmware when removing the whole quirk. The controller shuts down uncleanly and performs a restart/can break its RGB after suspend. The new firmware was mostly tested with the old quirk in place. I have not tried removing the quirk myself on the new firmware. Perhaps I should. > In the various ASUS drivers you can lookup the MCU firmware version. > Those drivers can do acpi_register_lps0_dev() when the older firmware is > present and use the callbacks. If the newer firmware is there less code > to worry about. > > This also would mean less static quirk tables in the kernel tree. I would prefer to avoid bringing in other drivers in this or depending on their functionality, as in this case it is also not needed. I guess as a callback form this can be somewhat flexible. > > + > > +static const struct dmi_system_id s2idle_delay_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 > > + }, > > + {} > > +}; > > + > > static LIST_HEAD(lps0_s2idle_devops_head); > > > > static struct lpi_constraints *lpi_constraints_table; > > static int lpi_constraints_table_size; > > static int rev_id; > > +struct s2idle_delay_quirks *delay_quirks; > > > > #define for_each_lpi_constraint(entry) \ > > for (int i = 0; \ > > @@ -566,6 +606,9 @@ static int acpi_s2idle_display_off(void) > > acpi_sleep_run_lps0_dsm(ACPI_LPS0_DISPLAY_OFF, > > lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); > > > > + if (delay_quirks && delay_quirks->delay_display_off) > > + msleep(delay_quirks->delay_display_off); > > + > > acpi_scan_lock_release(); > > > > return 0; > > @@ -587,6 +630,9 @@ static int acpi_s2idle_sleep_entry(void) > > acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_ENTRY, > > lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); > > > > + if (delay_quirks && delay_quirks->delay_sleep_entry) > > + msleep(delay_quirks->delay_sleep_entry); > > + > > acpi_scan_lock_release(); > > > > return 0; > > @@ -627,6 +673,9 @@ static int acpi_s2idle_sleep_exit(void) > > acpi_scan_lock_acquire(); > > > > /* Modern Standby Sleep Exit */ > > + if (delay_quirks && delay_quirks->delay_sleep_exit) > > + msleep(delay_quirks->delay_sleep_exit); > > + > > if (lps0_dsm_func_mask_microsoft > 0) > > acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_EXIT, > > lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); > > @@ -648,6 +697,9 @@ static int acpi_s2idle_display_on(void) > > acpi_scan_lock_acquire(); > > > > /* Display on */ > > + if (delay_quirks && delay_quirks->delay_display_on) > > + msleep(delay_quirks->delay_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); > > @@ -760,6 +812,10 @@ int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg) > > > > sleep_flags = lock_system_sleep(); > > list_add(&arg->list_node, &lps0_s2idle_devops_head); > > + const struct dmi_system_id *s2idle_sysid = dmi_first_match( > > + s2idle_delay_quirks > > + ); > > + delay_quirks = s2idle_sysid ? s2idle_sysid->driver_data : NULL; > > unlock_system_sleep(sleep_flags); > > > > return 0; >
diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c index d389c57d2963..504e6575d7ad 100644 --- a/drivers/acpi/x86/s2idle.c +++ b/drivers/acpi/x86/s2idle.c @@ -18,6 +18,7 @@ #include <linux/acpi.h> #include <linux/device.h> #include <linux/dmi.h> +#include <linux/delay.h> #include <linux/suspend.h> #include "../sleep.h" @@ -91,11 +92,50 @@ struct lpi_device_constraint_amd { int min_dstate; }; +struct s2idle_delay_quirks { + int delay_display_off; + int delay_sleep_entry; + int delay_sleep_exit; + int delay_display_on; +}; + +/* + * The ROG Ally series disconnects its controllers on Display Off and performs + * a fancy shutdown sequence, which requires around half a second to complete. + * If the power is cut earlier by entering it into D3, the original Ally unit + * might not disconnect its XInput MCU, causing excess battery drain, and the + * Ally X will make the controller restart post-suspend. In addition, the EC + * of the device rarely (1/20 attempts) may get stuck asserting PROCHOT after + * suspend (for various reasons), so split the delay between Display Off and + * Sleep Entry. + */ +static const struct s2idle_delay_quirks rog_ally_quirks = { + .delay_display_off = 350, + .delay_sleep_entry = 150, +}; + +static const struct dmi_system_id s2idle_delay_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 + }, + {} +}; + static LIST_HEAD(lps0_s2idle_devops_head); static struct lpi_constraints *lpi_constraints_table; static int lpi_constraints_table_size; static int rev_id; +struct s2idle_delay_quirks *delay_quirks; #define for_each_lpi_constraint(entry) \ for (int i = 0; \ @@ -566,6 +606,9 @@ static int acpi_s2idle_display_off(void) acpi_sleep_run_lps0_dsm(ACPI_LPS0_DISPLAY_OFF, lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); + if (delay_quirks && delay_quirks->delay_display_off) + msleep(delay_quirks->delay_display_off); + acpi_scan_lock_release(); return 0; @@ -587,6 +630,9 @@ static int acpi_s2idle_sleep_entry(void) acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_ENTRY, lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); + if (delay_quirks && delay_quirks->delay_sleep_entry) + msleep(delay_quirks->delay_sleep_entry); + acpi_scan_lock_release(); return 0; @@ -627,6 +673,9 @@ static int acpi_s2idle_sleep_exit(void) acpi_scan_lock_acquire(); /* Modern Standby Sleep Exit */ + if (delay_quirks && delay_quirks->delay_sleep_exit) + msleep(delay_quirks->delay_sleep_exit); + if (lps0_dsm_func_mask_microsoft > 0) acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_EXIT, lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); @@ -648,6 +697,9 @@ static int acpi_s2idle_display_on(void) acpi_scan_lock_acquire(); /* Display on */ + if (delay_quirks && delay_quirks->delay_display_on) + msleep(delay_quirks->delay_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); @@ -760,6 +812,10 @@ int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg) sleep_flags = lock_system_sleep(); list_add(&arg->list_node, &lps0_s2idle_devops_head); + const struct dmi_system_id *s2idle_sysid = dmi_first_match( + s2idle_delay_quirks + ); + delay_quirks = s2idle_sysid ? s2idle_sysid->driver_data : NULL; unlock_system_sleep(sleep_flags); return 0;
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. Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> --- drivers/acpi/x86/s2idle.c | 56 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)