| 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 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? This feels cleaner if you used "struct acpi_s2idle_dev_ops" and callbacks. More below. > + > +/* > + * 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. 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. > + > +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;
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(+)