* [RFC 01/13] Documentation: PM: Add documentation for S0ix Standby States
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-21 18:58 ` Mario Limonciello
2024-11-21 17:22 ` [RFC 02/13] acpi/x86: s2idle: add support for Display Off and Display On callbacks Antheas Kapenekakis
` (13 subsequent siblings)
14 siblings, 1 reply; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, Antheas Kapenekakis
Add documentation about the S0ix Standby States that will be exposed
to userspace as part of this series.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
.../admin-guide/pm/standby-states.rst | 133 ++++++++++++++++++
Documentation/admin-guide/pm/system-wide.rst | 1 +
2 files changed, 134 insertions(+)
create mode 100644 Documentation/admin-guide/pm/standby-states.rst
diff --git a/Documentation/admin-guide/pm/standby-states.rst b/Documentation/admin-guide/pm/standby-states.rst
new file mode 100644
index 000000000000..96727574312d
--- /dev/null
+++ b/Documentation/admin-guide/pm/standby-states.rst
@@ -0,0 +1,133 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. include:: <isonum.txt>
+
+=====================
+S0ix Standby States
+=====================
+
+:Copyright: |copy| 2024 Antheas Kapenekakis
+
+:Author: Antheas Kapenekakis <lkml@antheas.dev>
+
+With the advent of modern mobile devices, users have become accustomed to instant
+wake-up times and always-on connectivity. To meet these expectations, modern
+standby was created, which is a standard that allows the platform to seamlessly
+transition between an S3-like low-power idle state and a set of low power active
+states, where connectivity is maintained, and the system is responsive to user
+input. Current x86 hardware supports 5 different standby states, which are:
+"Deepest run-time idle platform state" or "DRIPS" (S3-like), "Sleep", "Resume",
+"Screen Off", and "Active".
+
+The system begins in the "Active" state. Either due to user inactivity or
+user action (e.g., pressing the power button), it transitions to the "Screen Off"
+state. Afterwards, it is free to transition between the "Sleep", "DRIPS", and
+"Screen Off" states until user action is received. Once that happens, the system
+begins to transition to the "Active" state. From "DRIPS" or "Sleep", it
+transitions to "Resume", where the Power Limit (PLx) is restored to its normal
+level, to speed up finishing "Sleep". Then, it transitions to "Screen Off".
+If on "Screen Off" or after the transition, the display is prepared to turn on
+and the system transitions to "Active" alongside turning it on.
+
+To maintain battery life, in the Windows implementation, the system is allocated
+a maximum percentage of battery and time it can use while staying in idle states.
+By default, this is 5% of battery or up to 2 days, where the system designer/OEM
+is able to tweak these values. If the system exceeds either the battery
+percentage or time limit, it enters Hibernation (S4), through a concept
+called "Adaptive Hibernate".
+
+
+S0ix Standby States
+==================================
+The following idle states are supported::
+
+ ↓→ <Hibernate (S4)>
+ <DRIPS> ↔ <Sleep> ↔ <Screen Off> ↔ <Active>
+ → → <Resume> ↑
+
+.. _s2idle_drips:
+
+DRIPS
+-----
+
+The "Deepest run-time idle platform state" or "DRIPS" is the lowest power idle
+state that the system can enter. It is similar to the S3 state, with the
+difference that the system may wake up faster than S3 and due to a larger number
+of interrupts (e.g., fingerprint sensor, touchpad, touchscreen). This state
+is entered when the system is told to suspend to idle, through conventional
+means (see :doc:`sleep states <sleep-states>`). The system can only transition
+to "DRIPS" while it is in the "Sleep" state. If it is not, the kernel will
+automatically transition to the "Sleep" state before beginning the suspend
+sequence and restore the previous state afterwards. After the kernel has
+suspended, the notifications LSP0 Entry and Exit are used.
+
+.. _s2idle_sleep:
+
+Sleep
+-----
+
+The "Sleep" state is a low power idle state where the kernel is fully active.
+However, userspace has been partially frozen, particularly desktop applications,
+and only essential "value adding" activities are allowed to run. This is not
+enforced by the kernel and is the responsibility of userspace (e.g., systemd).
+Hardware wise, the Sleep Entry and Exit firmware notifications are fired, which
+may lower the Power Limit (PLx), pulse the suspend light, turn off the keyboard
+lighting or disable a handheld device's gamepad. This state is associated with
+the firmware notifications "Sleep Entry" and "Sleep Exit".
+
+.. _s2idle_resume:
+
+Resume
+------
+
+The "Resume" state is a faux "Sleep" state that is used to fire the Turn On
+Display firmware notification when the system is in the "Sleep" state but
+intends to turn on the display. It solves the problem of system designers
+limiting the Power Limit (PLx) while the system is in the "Sleep" state causing
+the system to wake up slower than desired. This firmware notification is used
+to restore the normal Power Limit of the system, while having it stay in the
+"Sleep" state. As such, the system can only transition to the "Resume" state
+while in the "Sleep" state and cannot re-transition to the "Sleep" state
+afterwards.
+
+.. _s2idle_screen_off:
+
+Screen Off
+----------
+
+The "Screen Off" state is the state the system enters when all its displays
+(virtual or real) turn off. It is used to signify the user is not actively
+using the system. The associated firmware notifications of "Display On" and
+"Display Off" are used by manufacturers to turn off certain hardware
+components that are associated with the display being on, e.g., a handheld
+device's controller and RGB. Windows implements a 5-second grace period
+before firing this callback when the screen turns off due to inactivity.
+
+.. _s2idle_active:
+
+Active
+------
+
+Finally, the "Active" state is the default state of the system and the one it
+has when it is turned on. It is the state where the system is fully operational,
+the displays of the device are on, and the user is actively interacting with
+the system.
+
+Basic ``sysfs`` Interface for S0ix Standby transitions
+=============================================================
+
+The file :file:`/sys/power/standby` can be used to transition the system between
+the different standby states. The file accepts the following values: ``active``,
+``screen_off``, ``sleep``, and ``resume``. File writes will block until the
+transition completes. It will return ``-EINVAL`` when asking for an unsupported
+state or, e.g., requesting ``resume`` when not in the ``sleep`` state. If there
+is an error during the transition, the transition will pause on the last
+error-free state and return an error. The file can be read to retrieve the
+current state (and potential ones) using the following format:
+``[active] screen_off sleep resume``. The state "DRIPS" is omitted, as it is
+entered through the conventional suspend to idle path and userspace will never
+be able to see its value due to being suspended.
+
+Before entering the "Screen Off" state or suspending, it is recommended that
+userspace marks all CRTCs as inactive (DPMS). Otherwise, there will be a split
+second where the display of the device is on, but the presentation of the system
+is inactive (e.g., the power button pulses), which is undesirable.
\ No newline at end of file
diff --git a/Documentation/admin-guide/pm/system-wide.rst b/Documentation/admin-guide/pm/system-wide.rst
index 1a1924d71006..411775fae4ac 100644
--- a/Documentation/admin-guide/pm/system-wide.rst
+++ b/Documentation/admin-guide/pm/system-wide.rst
@@ -8,4 +8,5 @@ System-Wide Power Management
:maxdepth: 2
sleep-states
+ standby-states
suspend-flows
--
2.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [RFC 01/13] Documentation: PM: Add documentation for S0ix Standby States
2024-11-21 17:22 ` [RFC 01/13] Documentation: PM: Add documentation for S0ix Standby States Antheas Kapenekakis
@ 2024-11-21 18:58 ` Mario Limonciello
2024-11-21 19:11 ` Antheas Kapenekakis
0 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello @ 2024-11-21 18:58 UTC (permalink / raw)
To: Antheas Kapenekakis, linux-pm
Cc: platform-driver-x86, dri-devel, Hans de Goede, Kyle Gospodnetich
On 11/21/2024 11:22, Antheas Kapenekakis wrote:
> Add documentation about the S0ix Standby States that will be exposed
> to userspace as part of this series.
>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
> .../admin-guide/pm/standby-states.rst | 133 ++++++++++++++++++
> Documentation/admin-guide/pm/system-wide.rst | 1 +
> 2 files changed, 134 insertions(+)
> create mode 100644 Documentation/admin-guide/pm/standby-states.rst
>
> diff --git a/Documentation/admin-guide/pm/standby-states.rst b/Documentation/admin-guide/pm/standby-states.rst
> new file mode 100644
> index 000000000000..96727574312d
> --- /dev/null
> +++ b/Documentation/admin-guide/pm/standby-states.rst
> @@ -0,0 +1,133 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. include:: <isonum.txt>
> +
> +=====================
> +S0ix Standby States
> +=====================
> +
> +:Copyright: |copy| 2024 Antheas Kapenekakis
> +
> +:Author: Antheas Kapenekakis <lkml@antheas.dev>
> +
> +With the advent of modern mobile devices, users have become accustomed to instant
> +wake-up times and always-on connectivity. To meet these expectations, modern
> +standby was created, which is a standard that allows the platform to seamlessly
> +transition between an S3-like low-power idle state and a set of low power active
> +states, where connectivity is maintained, and the system is responsive to user
> +input. Current x86 hardware supports 5 different standby states, which are:
> +"Deepest run-time idle platform state" or "DRIPS" (S3-like), "Sleep", "Resume",
> +"Screen Off", and "Active".
> +
> +The system begins in the "Active" state. Either due to user inactivity or
> +user action (e.g., pressing the power button), it transitions to the "Screen Off"
> +state.
So are you implicitly suggesting that userspace should be responsible
for *telling* the kernel that the screen is off? I feel some DRM
helpers are missing to make it easy, but after such helpers are made the
kernel "should" be able to easily tell this on it's own.
> Afterwards, it is free to transition between the "Sleep", "DRIPS", and
> +"Screen Off" states until user action is received. Once that happens, the system
> +begins to transition to the "Active" state. From "DRIPS" or "Sleep", it
> +transitions to "Resume", where the Power Limit (PLx) is restored to its normal
> +level, to speed up finishing "Sleep". Then, it transitions to "Screen Off".
> +If on "Screen Off" or after the transition, the display is prepared to turn on
> +and the system transitions to "Active" alongside turning it on.
> +
> +To maintain battery life, in the Windows implementation, the system is allocated
> +a maximum percentage of battery and time it can use while staying in idle states.
> +By default, this is 5% of battery or up to 2 days, where the system designer/OEM
> +is able to tweak these values. If the system exceeds either the battery
> +percentage or time limit, it enters Hibernation (S4), through a concept
> +called "Adaptive Hibernate".
> +
> +
> +S0ix Standby States
> +==================================
> +The following idle states are supported::
> +
> + ↓→ <Hibernate (S4)>
I think S4 distracts in this context.
> + <DRIPS> ↔ <Sleep> ↔ <Screen Off> ↔ <Active>
> + → → <Resume> ↑
> +
> +.. _s2idle_drips:
> +
> +DRIPS
> +-----
> +
> +The "Deepest run-time idle platform state" or "DRIPS" is the lowest power idle
> +state that the system can enter. It is similar to the S3 state, with the
> +difference that the system may wake up faster than S3 and due to a larger number
> +of interrupts (e.g., fingerprint sensor, touchpad, touchscreen). This state
> +is entered when the system is told to suspend to idle, through conventional
> +means (see :doc:`sleep states <sleep-states>`). The system can only transition
> +to "DRIPS" while it is in the "Sleep" state. If it is not, the kernel will
> +automatically transition to the "Sleep" state before beginning the suspend
> +sequence and restore the previous state afterwards. After the kernel has
> +suspended, the notifications LSP0 Entry and Exit are used.
> +
> +.. _s2idle_sleep:
> +
> +Sleep
> +-----
> +
> +The "Sleep" state is a low power idle state where the kernel is fully active.
> +However, userspace has been partially frozen, particularly desktop applications,
> +and only essential "value adding" activities are allowed to run. This is not
> +enforced by the kernel and is the responsibility of userspace (e.g., systemd).
> +Hardware wise, the Sleep Entry and Exit firmware notifications are fired, which
> +may lower the Power Limit (PLx), pulse the suspend light, turn off the keyboard
> +lighting or disable a handheld device's gamepad. This state is associated with
> +the firmware notifications "Sleep Entry" and "Sleep Exit".
> +
> +.. _s2idle_resume:
> +
> +Resume
> +------
> +
> +The "Resume" state is a faux "Sleep" state that is used to fire the Turn On
> +Display firmware notification when the system is in the "Sleep" state but
> +intends to turn on the display. It solves the problem of system designers
> +limiting the Power Limit (PLx) while the system is in the "Sleep" state causing
AFAIK, PLx is an Intel specific acronym, it's probably better to be more
generic in documentation. You mentioned PLx in a commit too.
> +the system to wake up slower than desired. This firmware notification is used
> +to restore the normal Power Limit of the system, while having it stay in the
> +"Sleep" state. As such, the system can only transition to the "Resume" state
> +while in the "Sleep" state and cannot re-transition to the "Sleep" state
> +afterwards.
> +
> +.. _s2idle_screen_off:
> +
> +Screen Off
> +----------
> +
> +The "Screen Off" state is the state the system enters when all its displays
> +(virtual or real) turn off. It is used to signify the user is not actively
> +using the system. The associated firmware notifications of "Display On" and
> +"Display Off" are used by manufacturers to turn off certain hardware
> +components that are associated with the display being on, e.g., a handheld
> +device's controller and RGB. Windows implements a 5-second grace period
> +before firing this callback when the screen turns off due to inactivity.
> +
> +.. _s2idle_active:
> +
> +Active
> +------
> +
> +Finally, the "Active" state is the default state of the system and the one it
> +has when it is turned on. It is the state where the system is fully operational,
> +the displays of the device are on, and the user is actively interacting with
> +the system.
> +
> +Basic ``sysfs`` Interface for S0ix Standby transitions
> +=============================================================
> +
> +The file :file:`/sys/power/standby` can be used to transition the system between
> +the different standby states. The file accepts the following values: ``active``,
> +``screen_off``, ``sleep``, and ``resume``. File writes will block until the
> +transition completes. It will return ``-EINVAL`` when asking for an unsupported
> +state or, e.g., requesting ``resume`` when not in the ``sleep`` state. If there
> +is an error during the transition, the transition will pause on the last
> +error-free state and return an error. The file can be read to retrieve the
> +current state (and potential ones) using the following format:
> +``[active] screen_off sleep resume``. The state "DRIPS" is omitted, as it is
> +entered through the conventional suspend to idle path and userspace will never
> +be able to see its value due to being suspended.
If you follow my above suggestion, I think this file is totally
unnecessary and then there is no compatibility issue.
It would mean that userspace if it wants to see this "screen off" state
and associated performance needs to do literally just that - turn the
screens off.
> +
> +Before entering the "Screen Off" state or suspending, it is recommended that
> +userspace marks all CRTCs as inactive (DPMS). Otherwise, there will be a split
> +second where the display of the device is on, but the presentation of the system
> +is inactive (e.g., the power button pulses), which is undesirable.
> \ No newline at end of file
> diff --git a/Documentation/admin-guide/pm/system-wide.rst b/Documentation/admin-guide/pm/system-wide.rst
> index 1a1924d71006..411775fae4ac 100644
> --- a/Documentation/admin-guide/pm/system-wide.rst
> +++ b/Documentation/admin-guide/pm/system-wide.rst
> @@ -8,4 +8,5 @@ System-Wide Power Management
> :maxdepth: 2
>
> sleep-states
> + standby-states
> suspend-flows
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC 01/13] Documentation: PM: Add documentation for S0ix Standby States
2024-11-21 18:58 ` Mario Limonciello
@ 2024-11-21 19:11 ` Antheas Kapenekakis
2024-11-21 19:40 ` Mario Limonciello
0 siblings, 1 reply; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 19:11 UTC (permalink / raw)
To: Mario Limonciello
Cc: linux-pm, platform-driver-x86, dri-devel, Hans de Goede,
Kyle Gospodnetich
On Thu, 21 Nov 2024 at 19:58, Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 11/21/2024 11:22, Antheas Kapenekakis wrote:
> > Add documentation about the S0ix Standby States that will be exposed
> > to userspace as part of this series.
> >
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> > .../admin-guide/pm/standby-states.rst | 133 ++++++++++++++++++
> > Documentation/admin-guide/pm/system-wide.rst | 1 +
> > 2 files changed, 134 insertions(+)
> > create mode 100644 Documentation/admin-guide/pm/standby-states.rst
> >
> > diff --git a/Documentation/admin-guide/pm/standby-states.rst b/Documentation/admin-guide/pm/standby-states.rst
> > new file mode 100644
> > index 000000000000..96727574312d
> > --- /dev/null
> > +++ b/Documentation/admin-guide/pm/standby-states.rst
> > @@ -0,0 +1,133 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +.. include:: <isonum.txt>
> > +
> > +=====================
> > +S0ix Standby States
> > +=====================
> > +
> > +:Copyright: |copy| 2024 Antheas Kapenekakis
> > +
> > +:Author: Antheas Kapenekakis <lkml@antheas.dev>
> > +
> > +With the advent of modern mobile devices, users have become accustomed to instant
> > +wake-up times and always-on connectivity. To meet these expectations, modern
> > +standby was created, which is a standard that allows the platform to seamlessly
> > +transition between an S3-like low-power idle state and a set of low power active
> > +states, where connectivity is maintained, and the system is responsive to user
> > +input. Current x86 hardware supports 5 different standby states, which are:
> > +"Deepest run-time idle platform state" or "DRIPS" (S3-like), "Sleep", "Resume",
> > +"Screen Off", and "Active".
> > +
> > +The system begins in the "Active" state. Either due to user inactivity or
> > +user action (e.g., pressing the power button), it transitions to the "Screen Off"
> > +state.
>
> So are you implicitly suggesting that userspace should be responsible
> for *telling* the kernel that the screen is off? I feel some DRM
> helpers are missing to make it easy, but after such helpers are made the
> kernel "should" be able to easily tell this on it's own.
There are two issues with this
1) Windows implements a 5 second grace period on idle before firing
that firmware notification [1]. This is also a partial debounce, the
kernel cannot do that reliably or with the finesse required for such a
notification
2) Windows clearly states virtual or real and virtual can really
mean anything here.
In the end, only systemd and the compositor know if both conditions 1
and 2 are met and as such can be responsible for the notification.
However, if that notification firing before certain CRTCs are
deactivated causes issues, such DRM helpers could be used to block the
transition
Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/display--sleep--and-hibernate-idle-timers
[1]
> > Afterwards, it is free to transition between the "Sleep", "DRIPS", and
> > +"Screen Off" states until user action is received. Once that happens, the system
> > +begins to transition to the "Active" state. From "DRIPS" or "Sleep", it
> > +transitions to "Resume", where the Power Limit (PLx) is restored to its normal
> > +level, to speed up finishing "Sleep". Then, it transitions to "Screen Off".
> > +If on "Screen Off" or after the transition, the display is prepared to turn on
> > +and the system transitions to "Active" alongside turning it on.
> > +
> > +To maintain battery life, in the Windows implementation, the system is allocated
> > +a maximum percentage of battery and time it can use while staying in idle states.
> > +By default, this is 5% of battery or up to 2 days, where the system designer/OEM
> > +is able to tweak these values. If the system exceeds either the battery
> > +percentage or time limit, it enters Hibernation (S4), through a concept
> > +called "Adaptive Hibernate".
> > +
> > +
> > +S0ix Standby States
> > +==================================
> > +The following idle states are supported::
> > +
> > + ↓→ <Hibernate (S4)>
>
> I think S4 distracts in this context.
Sure, can be removed.
> > + <DRIPS> ↔ <Sleep> ↔ <Screen Off> ↔ <Active>
> > + → → <Resume> ↑
> > +
> > +.. _s2idle_drips:
> > +
> > +DRIPS
> > +-----
> > +
> > +The "Deepest run-time idle platform state" or "DRIPS" is the lowest power idle
> > +state that the system can enter. It is similar to the S3 state, with the
> > +difference that the system may wake up faster than S3 and due to a larger number
> > +of interrupts (e.g., fingerprint sensor, touchpad, touchscreen). This state
> > +is entered when the system is told to suspend to idle, through conventional
> > +means (see :doc:`sleep states <sleep-states>`). The system can only transition
> > +to "DRIPS" while it is in the "Sleep" state. If it is not, the kernel will
> > +automatically transition to the "Sleep" state before beginning the suspend
> > +sequence and restore the previous state afterwards. After the kernel has
> > +suspended, the notifications LSP0 Entry and Exit are used.
> > +
> > +.. _s2idle_sleep:
> > +
> > +Sleep
> > +-----
> > +
> > +The "Sleep" state is a low power idle state where the kernel is fully active.
> > +However, userspace has been partially frozen, particularly desktop applications,
> > +and only essential "value adding" activities are allowed to run. This is not
> > +enforced by the kernel and is the responsibility of userspace (e.g., systemd).
> > +Hardware wise, the Sleep Entry and Exit firmware notifications are fired, which
> > +may lower the Power Limit (PLx), pulse the suspend light, turn off the keyboard
> > +lighting or disable a handheld device's gamepad. This state is associated with
> > +the firmware notifications "Sleep Entry" and "Sleep Exit".
> > +
> > +.. _s2idle_resume:
> > +
> > +Resume
> > +------
> > +
> > +The "Resume" state is a faux "Sleep" state that is used to fire the Turn On
> > +Display firmware notification when the system is in the "Sleep" state but
> > +intends to turn on the display. It solves the problem of system designers
> > +limiting the Power Limit (PLx) while the system is in the "Sleep" state causing
>
> AFAIK, PLx is an Intel specific acronym, it's probably better to be more
> generic in documentation. You mentioned PLx in a commit too.
Microsoft used this term in their documentation [2]. Can update to
generic terms.
Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications#turn-on-display-notification-function-9
[2]
> > +the system to wake up slower than desired. This firmware notification is used
> > +to restore the normal Power Limit of the system, while having it stay in the
> > +"Sleep" state. As such, the system can only transition to the "Resume" state
> > +while in the "Sleep" state and cannot re-transition to the "Sleep" state
> > +afterwards.
> > +
> > +.. _s2idle_screen_off:
> > +
> > +Screen Off
> > +----------
> > +
> > +The "Screen Off" state is the state the system enters when all its displays
> > +(virtual or real) turn off. It is used to signify the user is not actively
> > +using the system. The associated firmware notifications of "Display On" and
> > +"Display Off" are used by manufacturers to turn off certain hardware
> > +components that are associated with the display being on, e.g., a handheld
> > +device's controller and RGB. Windows implements a 5-second grace period
> > +before firing this callback when the screen turns off due to inactivity.
> > +
> > +.. _s2idle_active:
> > +
> > +Active
> > +------
> > +
> > +Finally, the "Active" state is the default state of the system and the one it
> > +has when it is turned on. It is the state where the system is fully operational,
> > +the displays of the device are on, and the user is actively interacting with
> > +the system.
> > +
> > +Basic ``sysfs`` Interface for S0ix Standby transitions
> > +=============================================================
> > +
> > +The file :file:`/sys/power/standby` can be used to transition the system between
> > +the different standby states. The file accepts the following values: ``active``,
> > +``screen_off``, ``sleep``, and ``resume``. File writes will block until the
> > +transition completes. It will return ``-EINVAL`` when asking for an unsupported
> > +state or, e.g., requesting ``resume`` when not in the ``sleep`` state. If there
> > +is an error during the transition, the transition will pause on the last
> > +error-free state and return an error. The file can be read to retrieve the
> > +current state (and potential ones) using the following format:
> > +``[active] screen_off sleep resume``. The state "DRIPS" is omitted, as it is
> > +entered through the conventional suspend to idle path and userspace will never
> > +be able to see its value due to being suspended.
>
> If you follow my above suggestion, I think this file is totally
> unnecessary and then there is no compatibility issue.
>
> It would mean that userspace if it wants to see this "screen off" state
> and associated performance needs to do literally just that - turn the
> screens off.
Please see the reasoning above for Display On/Off. Also, you omitted
sleep and resume, which have no hardware analogues you can hook into
and are just as important if not more than Display On/Off.
> > +
> > +Before entering the "Screen Off" state or suspending, it is recommended that
> > +userspace marks all CRTCs as inactive (DPMS). Otherwise, there will be a split
> > +second where the display of the device is on, but the presentation of the system
> > +is inactive (e.g., the power button pulses), which is undesirable.
> > \ No newline at end of file
> > diff --git a/Documentation/admin-guide/pm/system-wide.rst b/Documentation/admin-guide/pm/system-wide.rst
> > index 1a1924d71006..411775fae4ac 100644
> > --- a/Documentation/admin-guide/pm/system-wide.rst
> > +++ b/Documentation/admin-guide/pm/system-wide.rst
> > @@ -8,4 +8,5 @@ System-Wide Power Management
> > :maxdepth: 2
> >
> > sleep-states
> > + standby-states
> > suspend-flows
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC 01/13] Documentation: PM: Add documentation for S0ix Standby States
2024-11-21 19:11 ` Antheas Kapenekakis
@ 2024-11-21 19:40 ` Mario Limonciello
2024-11-21 20:33 ` Antheas Kapenekakis
0 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello @ 2024-11-21 19:40 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: linux-pm, platform-driver-x86, dri-devel, Hans de Goede,
Kyle Gospodnetich
On 11/21/2024 13:11, Antheas Kapenekakis wrote:
> On Thu, 21 Nov 2024 at 19:58, Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 11/21/2024 11:22, Antheas Kapenekakis wrote:
>>> Add documentation about the S0ix Standby States that will be exposed
>>> to userspace as part of this series.
>>>
>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>> ---
>>> .../admin-guide/pm/standby-states.rst | 133 ++++++++++++++++++
>>> Documentation/admin-guide/pm/system-wide.rst | 1 +
>>> 2 files changed, 134 insertions(+)
>>> create mode 100644 Documentation/admin-guide/pm/standby-states.rst
>>>
>>> diff --git a/Documentation/admin-guide/pm/standby-states.rst b/Documentation/admin-guide/pm/standby-states.rst
>>> new file mode 100644
>>> index 000000000000..96727574312d
>>> --- /dev/null
>>> +++ b/Documentation/admin-guide/pm/standby-states.rst
>>> @@ -0,0 +1,133 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +.. include:: <isonum.txt>
>>> +
>>> +=====================
>>> +S0ix Standby States
>>> +=====================
>>> +
>>> +:Copyright: |copy| 2024 Antheas Kapenekakis
>>> +
>>> +:Author: Antheas Kapenekakis <lkml@antheas.dev>
>>> +
>>> +With the advent of modern mobile devices, users have become accustomed to instant
>>> +wake-up times and always-on connectivity. To meet these expectations, modern
>>> +standby was created, which is a standard that allows the platform to seamlessly
>>> +transition between an S3-like low-power idle state and a set of low power active
>>> +states, where connectivity is maintained, and the system is responsive to user
>>> +input. Current x86 hardware supports 5 different standby states, which are:
>>> +"Deepest run-time idle platform state" or "DRIPS" (S3-like), "Sleep", "Resume",
>>> +"Screen Off", and "Active".
>>> +
>>> +The system begins in the "Active" state. Either due to user inactivity or
>>> +user action (e.g., pressing the power button), it transitions to the "Screen Off"
>>> +state.
>>
>> So are you implicitly suggesting that userspace should be responsible
>> for *telling* the kernel that the screen is off? I feel some DRM
>> helpers are missing to make it easy, but after such helpers are made the
>> kernel "should" be able to easily tell this on it's own.
>
> There are two issues with this
> 1) Windows implements a 5 second grace period on idle before firing
> that firmware notification [1]. This is also a partial debounce, the
> kernel cannot do that reliably or with the finesse required for such a
> notification
Why can't the kernel do this? I'm thinking something like this pseudo
code that is triggered when number of enabled CRTCs changes:
if (in_suspend_sequence)
return;
switch (old_num_displays) {
case 0:
display_on_cb();
default:
schedule_delayed_work(&drm_s2idle_wq);
}
Then if the "normal" suspend sequence is started the delayed work is
cancelled.
If the "normal" suspend sequence doesn't start when it fires then it
would call the display off callback.
> 2) Windows clearly states virtual or real and virtual can really
> mean anything here.
In the context of the kernel, to me this is a DRM driver that has made
outputs that are not tied to a physical display. Does it mean anything
else? They should still be DRM connectors, and they should still have a
CRTC AFAICT.
>
> In the end, only systemd and the compositor know if both conditions 1
> and 2 are met and as such can be responsible for the notification.
>
> However, if that notification firing before certain CRTCs are
> deactivated causes issues, such DRM helpers could be used to block the
> transition
>
> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/display--sleep--and-hibernate-idle-timers
> [1]
>
>>> Afterwards, it is free to transition between the "Sleep", "DRIPS", and
>>> +"Screen Off" states until user action is received. Once that happens, the system
>>> +begins to transition to the "Active" state. From "DRIPS" or "Sleep", it
>>> +transitions to "Resume", where the Power Limit (PLx) is restored to its normal
>>> +level, to speed up finishing "Sleep". Then, it transitions to "Screen Off".
>>> +If on "Screen Off" or after the transition, the display is prepared to turn on
>>> +and the system transitions to "Active" alongside turning it on.
>>> +
>>> +To maintain battery life, in the Windows implementation, the system is allocated
>>> +a maximum percentage of battery and time it can use while staying in idle states.
>>> +By default, this is 5% of battery or up to 2 days, where the system designer/OEM
>>> +is able to tweak these values. If the system exceeds either the battery
>>> +percentage or time limit, it enters Hibernation (S4), through a concept
>>> +called "Adaptive Hibernate".
>>> +
>>> +
>>> +S0ix Standby States
>>> +==================================
>>> +The following idle states are supported::
>>> +
>>> + ↓→ <Hibernate (S4)>
>>
>> I think S4 distracts in this context.
>
> Sure, can be removed.
>
>>> + <DRIPS> ↔ <Sleep> ↔ <Screen Off> ↔ <Active>
>>> + → → <Resume> ↑
>>> +
>>> +.. _s2idle_drips:
>>> +
>>> +DRIPS
>>> +-----
>>> +
>>> +The "Deepest run-time idle platform state" or "DRIPS" is the lowest power idle
>>> +state that the system can enter. It is similar to the S3 state, with the
>>> +difference that the system may wake up faster than S3 and due to a larger number
>>> +of interrupts (e.g., fingerprint sensor, touchpad, touchscreen). This state
>>> +is entered when the system is told to suspend to idle, through conventional
>>> +means (see :doc:`sleep states <sleep-states>`). The system can only transition
>>> +to "DRIPS" while it is in the "Sleep" state. If it is not, the kernel will
>>> +automatically transition to the "Sleep" state before beginning the suspend
>>> +sequence and restore the previous state afterwards. After the kernel has
>>> +suspended, the notifications LSP0 Entry and Exit are used.
>>> +
>>> +.. _s2idle_sleep:
>>> +
>>> +Sleep
>>> +-----
>>> +
>>> +The "Sleep" state is a low power idle state where the kernel is fully active.
>>> +However, userspace has been partially frozen, particularly desktop applications,
>>> +and only essential "value adding" activities are allowed to run. This is not
>>> +enforced by the kernel and is the responsibility of userspace (e.g., systemd).
>>> +Hardware wise, the Sleep Entry and Exit firmware notifications are fired, which
>>> +may lower the Power Limit (PLx), pulse the suspend light, turn off the keyboard
>>> +lighting or disable a handheld device's gamepad. This state is associated with
>>> +the firmware notifications "Sleep Entry" and "Sleep Exit".
>>> +
>>> +.. _s2idle_resume:
>>> +
>>> +Resume
>>> +------
>>> +
>>> +The "Resume" state is a faux "Sleep" state that is used to fire the Turn On
>>> +Display firmware notification when the system is in the "Sleep" state but
>>> +intends to turn on the display. It solves the problem of system designers
>>> +limiting the Power Limit (PLx) while the system is in the "Sleep" state causing
>>
>> AFAIK, PLx is an Intel specific acronym, it's probably better to be more
>> generic in documentation. You mentioned PLx in a commit too.
>
> Microsoft used this term in their documentation [2]. Can update to
> generic terms.
>
> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications#turn-on-display-notification-function-9
> [2]
>
>>> +the system to wake up slower than desired. This firmware notification is used
>>> +to restore the normal Power Limit of the system, while having it stay in the
>>> +"Sleep" state. As such, the system can only transition to the "Resume" state
>>> +while in the "Sleep" state and cannot re-transition to the "Sleep" state
>>> +afterwards.
>>> +
>>> +.. _s2idle_screen_off:
>>> +
>>> +Screen Off
>>> +----------
>>> +
>>> +The "Screen Off" state is the state the system enters when all its displays
>>> +(virtual or real) turn off. It is used to signify the user is not actively
>>> +using the system. The associated firmware notifications of "Display On" and
>>> +"Display Off" are used by manufacturers to turn off certain hardware
>>> +components that are associated with the display being on, e.g., a handheld
>>> +device's controller and RGB. Windows implements a 5-second grace period
>>> +before firing this callback when the screen turns off due to inactivity.
>>> +
>>> +.. _s2idle_active:
>>> +
>>> +Active
>>> +------
>>> +
>>> +Finally, the "Active" state is the default state of the system and the one it
>>> +has when it is turned on. It is the state where the system is fully operational,
>>> +the displays of the device are on, and the user is actively interacting with
>>> +the system.
>>> +
>>> +Basic ``sysfs`` Interface for S0ix Standby transitions
>>> +=============================================================
>>> +
>>> +The file :file:`/sys/power/standby` can be used to transition the system between
>>> +the different standby states. The file accepts the following values: ``active``,
>>> +``screen_off``, ``sleep``, and ``resume``. File writes will block until the
>>> +transition completes. It will return ``-EINVAL`` when asking for an unsupported
>>> +state or, e.g., requesting ``resume`` when not in the ``sleep`` state. If there
>>> +is an error during the transition, the transition will pause on the last
>>> +error-free state and return an error. The file can be read to retrieve the
>>> +current state (and potential ones) using the following format:
>>> +``[active] screen_off sleep resume``. The state "DRIPS" is omitted, as it is
>>> +entered through the conventional suspend to idle path and userspace will never
>>> +be able to see its value due to being suspended.
>>
>> If you follow my above suggestion, I think this file is totally
>> unnecessary and then there is no compatibility issue.
>>
>> It would mean that userspace if it wants to see this "screen off" state
>> and associated performance needs to do literally just that - turn the
>> screens off.
>
> Please see the reasoning above for Display On/Off. Also, you omitted
> sleep and resume, which have no hardware analogues you can hook into
> and are just as important if not more than Display On/Off.
I suppose I'm not seeing the argument yet for why "sleep" and HW DRIPS
need to be different. What kind of things would be allowed to run in
this state? Who draws that line?
As it stands today the kernel freezes all tasks when suspending, so in
this "half" suspend state I feel like there would need to be some sort
of allow list, no?
>
>>> +
>>> +Before entering the "Screen Off" state or suspending, it is recommended that
>>> +userspace marks all CRTCs as inactive (DPMS). Otherwise, there will be a split
>>> +second where the display of the device is on, but the presentation of the system
>>> +is inactive (e.g., the power button pulses), which is undesirable.
>>> \ No newline at end of file
>>> diff --git a/Documentation/admin-guide/pm/system-wide.rst b/Documentation/admin-guide/pm/system-wide.rst
>>> index 1a1924d71006..411775fae4ac 100644
>>> --- a/Documentation/admin-guide/pm/system-wide.rst
>>> +++ b/Documentation/admin-guide/pm/system-wide.rst
>>> @@ -8,4 +8,5 @@ System-Wide Power Management
>>> :maxdepth: 2
>>>
>>> sleep-states
>>> + standby-states
>>> suspend-flows
>>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [RFC 01/13] Documentation: PM: Add documentation for S0ix Standby States
2024-11-21 19:40 ` Mario Limonciello
@ 2024-11-21 20:33 ` Antheas Kapenekakis
2024-11-21 21:08 ` Mario Limonciello
0 siblings, 1 reply; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 20:33 UTC (permalink / raw)
To: Mario Limonciello
Cc: linux-pm, platform-driver-x86, dri-devel, Hans de Goede,
Kyle Gospodnetich
On Thu, 21 Nov 2024 at 20:40, Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 11/21/2024 13:11, Antheas Kapenekakis wrote:
> > On Thu, 21 Nov 2024 at 19:58, Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> On 11/21/2024 11:22, Antheas Kapenekakis wrote:
> >>> Add documentation about the S0ix Standby States that will be exposed
> >>> to userspace as part of this series.
> >>>
> >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>> ---
> >>> .../admin-guide/pm/standby-states.rst | 133 ++++++++++++++++++
> >>> Documentation/admin-guide/pm/system-wide.rst | 1 +
> >>> 2 files changed, 134 insertions(+)
> >>> create mode 100644 Documentation/admin-guide/pm/standby-states.rst
> >>>
> >>> diff --git a/Documentation/admin-guide/pm/standby-states.rst b/Documentation/admin-guide/pm/standby-states.rst
> >>> new file mode 100644
> >>> index 000000000000..96727574312d
> >>> --- /dev/null
> >>> +++ b/Documentation/admin-guide/pm/standby-states.rst
> >>> @@ -0,0 +1,133 @@
> >>> +.. SPDX-License-Identifier: GPL-2.0
> >>> +.. include:: <isonum.txt>
> >>> +
> >>> +=====================
> >>> +S0ix Standby States
> >>> +=====================
> >>> +
> >>> +:Copyright: |copy| 2024 Antheas Kapenekakis
> >>> +
> >>> +:Author: Antheas Kapenekakis <lkml@antheas.dev>
> >>> +
> >>> +With the advent of modern mobile devices, users have become accustomed to instant
> >>> +wake-up times and always-on connectivity. To meet these expectations, modern
> >>> +standby was created, which is a standard that allows the platform to seamlessly
> >>> +transition between an S3-like low-power idle state and a set of low power active
> >>> +states, where connectivity is maintained, and the system is responsive to user
> >>> +input. Current x86 hardware supports 5 different standby states, which are:
> >>> +"Deepest run-time idle platform state" or "DRIPS" (S3-like), "Sleep", "Resume",
> >>> +"Screen Off", and "Active".
> >>> +
> >>> +The system begins in the "Active" state. Either due to user inactivity or
> >>> +user action (e.g., pressing the power button), it transitions to the "Screen Off"
> >>> +state.
> >>
> >> So are you implicitly suggesting that userspace should be responsible
> >> for *telling* the kernel that the screen is off? I feel some DRM
> >> helpers are missing to make it easy, but after such helpers are made the
> >> kernel "should" be able to easily tell this on it's own.
> >
> > There are two issues with this
> > 1) Windows implements a 5 second grace period on idle before firing
> > that firmware notification [1]. This is also a partial debounce, the
> > kernel cannot do that reliably or with the finesse required for such a
> > notification
>
> Why can't the kernel do this? I'm thinking something like this pseudo
> code that is triggered when number of enabled CRTCs changes:
>
> if (in_suspend_sequence)
> return;
> switch (old_num_displays) {
> case 0:
> display_on_cb();
> default:
> schedule_delayed_work(&drm_s2idle_wq);
> }
>
> Then if the "normal" suspend sequence is started the delayed work is
> cancelled.
>
> If the "normal" suspend sequence doesn't start when it fires then it
> would call the display off callback.
Fundamentally, it is more complicated and error prone than 2 systemd
suspend targets that fire at the same time DEs lock the lock screen
(or any init system for that matter). This pseudocode also hardcodes
the delay and does not debounce the display on callback.
There is the theoretical risk of a device misbehaving if the callbacks
fire at the wrong time. But this risk is theoretical and could be
solved by a device driver quirk that blocks the transition for that
specific device. Which is also much simpler than trying to hardcode an
implementation that works with all devices.
> > 2) Windows clearly states virtual or real and virtual can really
> > mean anything here.
>
> In the context of the kernel, to me this is a DRM driver that has made
> outputs that are not tied to a physical display. Does it mean anything
> else? They should still be DRM connectors, and they should still have a
> CRTC AFAICT.
For all the devices I tested, the display calls change the
presentation of the device such as RGB or aux devices that drain power
during suspend. I do not see a connection to DRM. This points me to
userspace being more appropriate for handling this. It also solves all
UX edge cases because userspace knows when it is inactive.
Userspace handling this will not be backwards compatible in the sense
that it will not fire when the displays turn off with current
userspace. But it preserves current behavior and as such it is not a
breaking change.
> >
> > In the end, only systemd and the compositor know if both conditions 1
> > and 2 are met and as such can be responsible for the notification.
> >
> > However, if that notification firing before certain CRTCs are
> > deactivated causes issues, such DRM helpers could be used to block the
> > transition
> >
> > Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/display--sleep--and-hibernate-idle-timers
> > [1]
> >
> >>> Afterwards, it is free to transition between the "Sleep", "DRIPS", and
> >>> +"Screen Off" states until user action is received. Once that happens, the system
> >>> +begins to transition to the "Active" state. From "DRIPS" or "Sleep", it
> >>> +transitions to "Resume", where the Power Limit (PLx) is restored to its normal
> >>> +level, to speed up finishing "Sleep". Then, it transitions to "Screen Off".
> >>> +If on "Screen Off" or after the transition, the display is prepared to turn on
> >>> +and the system transitions to "Active" alongside turning it on.
> >>> +
> >>> +To maintain battery life, in the Windows implementation, the system is allocated
> >>> +a maximum percentage of battery and time it can use while staying in idle states.
> >>> +By default, this is 5% of battery or up to 2 days, where the system designer/OEM
> >>> +is able to tweak these values. If the system exceeds either the battery
> >>> +percentage or time limit, it enters Hibernation (S4), through a concept
> >>> +called "Adaptive Hibernate".
> >>> +
> >>> +
> >>> +S0ix Standby States
> >>> +==================================
> >>> +The following idle states are supported::
> >>> +
> >>> + ↓→ <Hibernate (S4)>
> >>
> >> I think S4 distracts in this context.
> >
> > Sure, can be removed.
> >
> >>> + <DRIPS> ↔ <Sleep> ↔ <Screen Off> ↔ <Active>
> >>> + → → <Resume> ↑
> >>> +
> >>> +.. _s2idle_drips:
> >>> +
> >>> +DRIPS
> >>> +-----
> >>> +
> >>> +The "Deepest run-time idle platform state" or "DRIPS" is the lowest power idle
> >>> +state that the system can enter. It is similar to the S3 state, with the
> >>> +difference that the system may wake up faster than S3 and due to a larger number
> >>> +of interrupts (e.g., fingerprint sensor, touchpad, touchscreen). This state
> >>> +is entered when the system is told to suspend to idle, through conventional
> >>> +means (see :doc:`sleep states <sleep-states>`). The system can only transition
> >>> +to "DRIPS" while it is in the "Sleep" state. If it is not, the kernel will
> >>> +automatically transition to the "Sleep" state before beginning the suspend
> >>> +sequence and restore the previous state afterwards. After the kernel has
> >>> +suspended, the notifications LSP0 Entry and Exit are used.
> >>> +
> >>> +.. _s2idle_sleep:
> >>> +
> >>> +Sleep
> >>> +-----
> >>> +
> >>> +The "Sleep" state is a low power idle state where the kernel is fully active.
> >>> +However, userspace has been partially frozen, particularly desktop applications,
> >>> +and only essential "value adding" activities are allowed to run. This is not
> >>> +enforced by the kernel and is the responsibility of userspace (e.g., systemd).
> >>> +Hardware wise, the Sleep Entry and Exit firmware notifications are fired, which
> >>> +may lower the Power Limit (PLx), pulse the suspend light, turn off the keyboard
> >>> +lighting or disable a handheld device's gamepad. This state is associated with
> >>> +the firmware notifications "Sleep Entry" and "Sleep Exit".
> >>> +
> >>> +.. _s2idle_resume:
> >>> +
> >>> +Resume
> >>> +------
> >>> +
> >>> +The "Resume" state is a faux "Sleep" state that is used to fire the Turn On
> >>> +Display firmware notification when the system is in the "Sleep" state but
> >>> +intends to turn on the display. It solves the problem of system designers
> >>> +limiting the Power Limit (PLx) while the system is in the "Sleep" state causing
> >>
> >> AFAIK, PLx is an Intel specific acronym, it's probably better to be more
> >> generic in documentation. You mentioned PLx in a commit too.
> >
> > Microsoft used this term in their documentation [2]. Can update to
> > generic terms.
> >
> > Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications#turn-on-display-notification-function-9
> > [2]
> >
> >>> +the system to wake up slower than desired. This firmware notification is used
> >>> +to restore the normal Power Limit of the system, while having it stay in the
> >>> +"Sleep" state. As such, the system can only transition to the "Resume" state
> >>> +while in the "Sleep" state and cannot re-transition to the "Sleep" state
> >>> +afterwards.
> >>> +
> >>> +.. _s2idle_screen_off:
> >>> +
> >>> +Screen Off
> >>> +----------
> >>> +
> >>> +The "Screen Off" state is the state the system enters when all its displays
> >>> +(virtual or real) turn off. It is used to signify the user is not actively
> >>> +using the system. The associated firmware notifications of "Display On" and
> >>> +"Display Off" are used by manufacturers to turn off certain hardware
> >>> +components that are associated with the display being on, e.g., a handheld
> >>> +device's controller and RGB. Windows implements a 5-second grace period
> >>> +before firing this callback when the screen turns off due to inactivity.
> >>> +
> >>> +.. _s2idle_active:
> >>> +
> >>> +Active
> >>> +------
> >>> +
> >>> +Finally, the "Active" state is the default state of the system and the one it
> >>> +has when it is turned on. It is the state where the system is fully operational,
> >>> +the displays of the device are on, and the user is actively interacting with
> >>> +the system.
> >>> +
> >>> +Basic ``sysfs`` Interface for S0ix Standby transitions
> >>> +=============================================================
> >>> +
> >>> +The file :file:`/sys/power/standby` can be used to transition the system between
> >>> +the different standby states. The file accepts the following values: ``active``,
> >>> +``screen_off``, ``sleep``, and ``resume``. File writes will block until the
> >>> +transition completes. It will return ``-EINVAL`` when asking for an unsupported
> >>> +state or, e.g., requesting ``resume`` when not in the ``sleep`` state. If there
> >>> +is an error during the transition, the transition will pause on the last
> >>> +error-free state and return an error. The file can be read to retrieve the
> >>> +current state (and potential ones) using the following format:
> >>> +``[active] screen_off sleep resume``. The state "DRIPS" is omitted, as it is
> >>> +entered through the conventional suspend to idle path and userspace will never
> >>> +be able to see its value due to being suspended.
> >>
> >> If you follow my above suggestion, I think this file is totally
> >> unnecessary and then there is no compatibility issue.
> >>
> >> It would mean that userspace if it wants to see this "screen off" state
> >> and associated performance needs to do literally just that - turn the
> >> screens off.
> >
> > Please see the reasoning above for Display On/Off. Also, you omitted
> > sleep and resume, which have no hardware analogues you can hook into
> > and are just as important if not more than Display On/Off.
>
> I suppose I'm not seeing the argument yet for why "sleep" and HW DRIPS
> need to be different. What kind of things would be allowed to run in
> this state? Who draws that line?
The most useful thing would be maintaining some basic connectivity so
that the device can resume faster if it suspended a couple of minutes
before and handling transitions such as to hibernation. The transition
to hibernation is especially important, as if both DPMS and the sleep
transition fire the transition looks proper. Being able to run certain
maintenance tasks without changing the presentation of the device from
sleep (e.g., the APM timer to check the battery level) is important.
Even without that, if userspace transitions to sleep and fires DPMS
before beginning freezing and the suspend sequence, it halfs the user
perceived delay to sleep. It is a big deal. This is a planned feature
for the next version of bazzite so I am testing it right now. It looks
really professional.
> As it stands today the kernel freezes all tasks when suspending, so in
> this "half" suspend state I feel like there would need to be some sort
> of allow list, no?
I do sympathize with this. The most important part would be to lower
the power limit of the device which the manufacturers can already do
via the notification and perhaps other kernel drivers could do too.
Non-root software can be limited by the init system in general.
As a side note, after all tasks have frozen, including compositors,
you can fire DPMS safely before beginning the suspend sequence for
backwards compatibility and to lower the span the old framebuffer is
shown. This would be a useful addition to this series.
> >
> >>> +
> >>> +Before entering the "Screen Off" state or suspending, it is recommended that
> >>> +userspace marks all CRTCs as inactive (DPMS). Otherwise, there will be a split
> >>> +second where the display of the device is on, but the presentation of the system
> >>> +is inactive (e.g., the power button pulses), which is undesirable.
> >>> \ No newline at end of file
> >>> diff --git a/Documentation/admin-guide/pm/system-wide.rst b/Documentation/admin-guide/pm/system-wide.rst
> >>> index 1a1924d71006..411775fae4ac 100644
> >>> --- a/Documentation/admin-guide/pm/system-wide.rst
> >>> +++ b/Documentation/admin-guide/pm/system-wide.rst
> >>> @@ -8,4 +8,5 @@ System-Wide Power Management
> >>> :maxdepth: 2
> >>>
> >>> sleep-states
> >>> + standby-states
> >>> suspend-flows
> >>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [RFC 01/13] Documentation: PM: Add documentation for S0ix Standby States
2024-11-21 20:33 ` Antheas Kapenekakis
@ 2024-11-21 21:08 ` Mario Limonciello
2024-11-21 21:23 ` Antheas Kapenekakis
0 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello @ 2024-11-21 21:08 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: linux-pm, platform-driver-x86, dri-devel, Hans de Goede,
Kyle Gospodnetich
On 11/21/2024 14:33, Antheas Kapenekakis wrote:
> On Thu, 21 Nov 2024 at 20:40, Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 11/21/2024 13:11, Antheas Kapenekakis wrote:
>>> On Thu, 21 Nov 2024 at 19:58, Mario Limonciello
>>> <mario.limonciello@amd.com> wrote:
>>>>
>>>> On 11/21/2024 11:22, Antheas Kapenekakis wrote:
>>>>> Add documentation about the S0ix Standby States that will be exposed
>>>>> to userspace as part of this series.
>>>>>
>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>>> ---
>>>>> .../admin-guide/pm/standby-states.rst | 133 ++++++++++++++++++
>>>>> Documentation/admin-guide/pm/system-wide.rst | 1 +
>>>>> 2 files changed, 134 insertions(+)
>>>>> create mode 100644 Documentation/admin-guide/pm/standby-states.rst
>>>>>
>>>>> diff --git a/Documentation/admin-guide/pm/standby-states.rst b/Documentation/admin-guide/pm/standby-states.rst
>>>>> new file mode 100644
>>>>> index 000000000000..96727574312d
>>>>> --- /dev/null
>>>>> +++ b/Documentation/admin-guide/pm/standby-states.rst
>>>>> @@ -0,0 +1,133 @@
>>>>> +.. SPDX-License-Identifier: GPL-2.0
>>>>> +.. include:: <isonum.txt>
>>>>> +
>>>>> +=====================
>>>>> +S0ix Standby States
>>>>> +=====================
>>>>> +
>>>>> +:Copyright: |copy| 2024 Antheas Kapenekakis
>>>>> +
>>>>> +:Author: Antheas Kapenekakis <lkml@antheas.dev>
>>>>> +
>>>>> +With the advent of modern mobile devices, users have become accustomed to instant
>>>>> +wake-up times and always-on connectivity. To meet these expectations, modern
>>>>> +standby was created, which is a standard that allows the platform to seamlessly
>>>>> +transition between an S3-like low-power idle state and a set of low power active
>>>>> +states, where connectivity is maintained, and the system is responsive to user
>>>>> +input. Current x86 hardware supports 5 different standby states, which are:
>>>>> +"Deepest run-time idle platform state" or "DRIPS" (S3-like), "Sleep", "Resume",
>>>>> +"Screen Off", and "Active".
>>>>> +
>>>>> +The system begins in the "Active" state. Either due to user inactivity or
>>>>> +user action (e.g., pressing the power button), it transitions to the "Screen Off"
>>>>> +state.
>>>>
>>>> So are you implicitly suggesting that userspace should be responsible
>>>> for *telling* the kernel that the screen is off? I feel some DRM
>>>> helpers are missing to make it easy, but after such helpers are made the
>>>> kernel "should" be able to easily tell this on it's own.
>>>
>>> There are two issues with this
>>> 1) Windows implements a 5 second grace period on idle before firing
>>> that firmware notification [1]. This is also a partial debounce, the
>>> kernel cannot do that reliably or with the finesse required for such a
>>> notification
>>
>> Why can't the kernel do this? I'm thinking something like this pseudo
>> code that is triggered when number of enabled CRTCs changes:
>>
>> if (in_suspend_sequence)
>> return;
>> switch (old_num_displays) {
>> case 0:
>> display_on_cb();
>> default:
>> schedule_delayed_work(&drm_s2idle_wq);
>> }
>>
>> Then if the "normal" suspend sequence is started the delayed work is
>> cancelled.
>>
>> If the "normal" suspend sequence doesn't start when it fires then it
>> would call the display off callback.
>
> Fundamentally, it is more complicated and error prone than 2 systemd
> suspend targets that fire at the same time DEs lock the lock screen
> (or any init system for that matter).
2 userspace jobs for the suspend sequence firing at same time vying for
similar resources?
That sounds inherently racy.
> This pseudocode also hardcodes
> the delay and does not debounce the display on callback.
>
If sticking to the Microsoft way of doing this, then it would be
hardcoded. But yeah if going this direction it "could" be something
configurable by userspace.
An actual implementation would need some locking protection like a mutex.
> There is the theoretical risk of a device misbehaving if the callbacks
> fire at the wrong time. But this risk is theoretical and could be
> solved by a device driver quirk that blocks the transition for that
> specific device. Which is also much simpler than trying to hardcode an
> implementation that works with all devices.
>
>>> 2) Windows clearly states virtual or real and virtual can really
>>> mean anything here.
>>
>> In the context of the kernel, to me this is a DRM driver that has made
>> outputs that are not tied to a physical display. Does it mean anything
>> else? They should still be DRM connectors, and they should still have a
>> CRTC AFAICT.
>
> For all the devices I tested, the display calls change the
> presentation of the device such as RGB or aux devices that drain power
> during suspend. I do not see a connection to DRM. This points me to
> userspace being more appropriate for handling this. It also solves all
> UX edge cases because userspace knows when it is inactive.
>
> Userspace handling this will not be backwards compatible in the sense
> that it will not fire when the displays turn off with current
> userspace. But it preserves current behavior and as such it is not a
> breaking change.
>
>>>
>>> In the end, only systemd and the compositor know if both conditions 1
>>> and 2 are met and as such can be responsible for the notification.
>>>
>>> However, if that notification firing before certain CRTCs are
>>> deactivated causes issues, such DRM helpers could be used to block the
>>> transition
>>>
>>> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/display--sleep--and-hibernate-idle-timers
>>> [1]
>>>
>>>>> Afterwards, it is free to transition between the "Sleep", "DRIPS", and
>>>>> +"Screen Off" states until user action is received. Once that happens, the system
>>>>> +begins to transition to the "Active" state. From "DRIPS" or "Sleep", it
>>>>> +transitions to "Resume", where the Power Limit (PLx) is restored to its normal
>>>>> +level, to speed up finishing "Sleep". Then, it transitions to "Screen Off".
>>>>> +If on "Screen Off" or after the transition, the display is prepared to turn on
>>>>> +and the system transitions to "Active" alongside turning it on.
>>>>> +
>>>>> +To maintain battery life, in the Windows implementation, the system is allocated
>>>>> +a maximum percentage of battery and time it can use while staying in idle states.
>>>>> +By default, this is 5% of battery or up to 2 days, where the system designer/OEM
>>>>> +is able to tweak these values. If the system exceeds either the battery
>>>>> +percentage or time limit, it enters Hibernation (S4), through a concept
>>>>> +called "Adaptive Hibernate".
>>>>> +
>>>>> +
>>>>> +S0ix Standby States
>>>>> +==================================
>>>>> +The following idle states are supported::
>>>>> +
>>>>> + ↓→ <Hibernate (S4)>
>>>>
>>>> I think S4 distracts in this context.
>>>
>>> Sure, can be removed.
>>>
>>>>> + <DRIPS> ↔ <Sleep> ↔ <Screen Off> ↔ <Active>
>>>>> + → → <Resume> ↑
>>>>> +
>>>>> +.. _s2idle_drips:
>>>>> +
>>>>> +DRIPS
>>>>> +-----
>>>>> +
>>>>> +The "Deepest run-time idle platform state" or "DRIPS" is the lowest power idle
>>>>> +state that the system can enter. It is similar to the S3 state, with the
>>>>> +difference that the system may wake up faster than S3 and due to a larger number
>>>>> +of interrupts (e.g., fingerprint sensor, touchpad, touchscreen). This state
>>>>> +is entered when the system is told to suspend to idle, through conventional
>>>>> +means (see :doc:`sleep states <sleep-states>`). The system can only transition
>>>>> +to "DRIPS" while it is in the "Sleep" state. If it is not, the kernel will
>>>>> +automatically transition to the "Sleep" state before beginning the suspend
>>>>> +sequence and restore the previous state afterwards. After the kernel has
>>>>> +suspended, the notifications LSP0 Entry and Exit are used.
>>>>> +
>>>>> +.. _s2idle_sleep:
>>>>> +
>>>>> +Sleep
>>>>> +-----
>>>>> +
>>>>> +The "Sleep" state is a low power idle state where the kernel is fully active.
>>>>> +However, userspace has been partially frozen, particularly desktop applications,
>>>>> +and only essential "value adding" activities are allowed to run. This is not
>>>>> +enforced by the kernel and is the responsibility of userspace (e.g., systemd).
>>>>> +Hardware wise, the Sleep Entry and Exit firmware notifications are fired, which
>>>>> +may lower the Power Limit (PLx), pulse the suspend light, turn off the keyboard
>>>>> +lighting or disable a handheld device's gamepad. This state is associated with
>>>>> +the firmware notifications "Sleep Entry" and "Sleep Exit".
>>>>> +
>>>>> +.. _s2idle_resume:
>>>>> +
>>>>> +Resume
>>>>> +------
>>>>> +
>>>>> +The "Resume" state is a faux "Sleep" state that is used to fire the Turn On
>>>>> +Display firmware notification when the system is in the "Sleep" state but
>>>>> +intends to turn on the display. It solves the problem of system designers
>>>>> +limiting the Power Limit (PLx) while the system is in the "Sleep" state causing
>>>>
>>>> AFAIK, PLx is an Intel specific acronym, it's probably better to be more
>>>> generic in documentation. You mentioned PLx in a commit too.
>>>
>>> Microsoft used this term in their documentation [2]. Can update to
>>> generic terms.
>>>
>>> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications#turn-on-display-notification-function-9
>>> [2]
>>>
>>>>> +the system to wake up slower than desired. This firmware notification is used
>>>>> +to restore the normal Power Limit of the system, while having it stay in the
>>>>> +"Sleep" state. As such, the system can only transition to the "Resume" state
>>>>> +while in the "Sleep" state and cannot re-transition to the "Sleep" state
>>>>> +afterwards.
>>>>> +
>>>>> +.. _s2idle_screen_off:
>>>>> +
>>>>> +Screen Off
>>>>> +----------
>>>>> +
>>>>> +The "Screen Off" state is the state the system enters when all its displays
>>>>> +(virtual or real) turn off. It is used to signify the user is not actively
>>>>> +using the system. The associated firmware notifications of "Display On" and
>>>>> +"Display Off" are used by manufacturers to turn off certain hardware
>>>>> +components that are associated with the display being on, e.g., a handheld
>>>>> +device's controller and RGB. Windows implements a 5-second grace period
>>>>> +before firing this callback when the screen turns off due to inactivity.
>>>>> +
>>>>> +.. _s2idle_active:
>>>>> +
>>>>> +Active
>>>>> +------
>>>>> +
>>>>> +Finally, the "Active" state is the default state of the system and the one it
>>>>> +has when it is turned on. It is the state where the system is fully operational,
>>>>> +the displays of the device are on, and the user is actively interacting with
>>>>> +the system.
>>>>> +
>>>>> +Basic ``sysfs`` Interface for S0ix Standby transitions
>>>>> +=============================================================
>>>>> +
>>>>> +The file :file:`/sys/power/standby` can be used to transition the system between
>>>>> +the different standby states. The file accepts the following values: ``active``,
>>>>> +``screen_off``, ``sleep``, and ``resume``. File writes will block until the
>>>>> +transition completes. It will return ``-EINVAL`` when asking for an unsupported
>>>>> +state or, e.g., requesting ``resume`` when not in the ``sleep`` state. If there
>>>>> +is an error during the transition, the transition will pause on the last
>>>>> +error-free state and return an error. The file can be read to retrieve the
>>>>> +current state (and potential ones) using the following format:
>>>>> +``[active] screen_off sleep resume``. The state "DRIPS" is omitted, as it is
>>>>> +entered through the conventional suspend to idle path and userspace will never
>>>>> +be able to see its value due to being suspended.
>>>>
>>>> If you follow my above suggestion, I think this file is totally
>>>> unnecessary and then there is no compatibility issue.
>>>>
>>>> It would mean that userspace if it wants to see this "screen off" state
>>>> and associated performance needs to do literally just that - turn the
>>>> screens off.
>>>
>>> Please see the reasoning above for Display On/Off. Also, you omitted
>>> sleep and resume, which have no hardware analogues you can hook into
>>> and are just as important if not more than Display On/Off.
>>
>> I suppose I'm not seeing the argument yet for why "sleep" and HW DRIPS
>> need to be different. What kind of things would be allowed to run in
>> this state? Who draws that line?
>
> The most useful thing would be maintaining some basic connectivity so
> that the device can resume faster if it suspended a couple of minutes
> before and handling transitions such as to hibernation. The transition
> to hibernation is especially important, as if both DPMS and the sleep
> transition fire the transition looks proper. Being able to run certain
> maintenance tasks without changing the presentation of the device from
> sleep (e.g., the APM timer to check the battery level) is important.
>
These points still seem to argue for "display on" vs "display off"
though, not a "sleep" vs "HW DRIPS".
> Even without that, if userspace transitions to sleep and fires DPMS
> before beginning freezing and the suspend sequence, it halfs the user
> perceived delay to sleep. It is a big deal. This is a planned feature
> for the next version of bazzite so I am testing it right now. It looks
> really professional.
>
I feel that what you're mostly vying for is "Dark Resume", which is
something that exists in the Chrome OS world:
https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/power_manager/docs/dark_resume.md
I feel with cooperation between the compositor and the initiator of
suspend the same thing can be done outside of ChromeOS.
>> As it stands today the kernel freezes all tasks when suspending, so in
>> this "half" suspend state I feel like there would need to be some sort
>> of allow list, no?
>
> I do sympathize with this. The most important part would be to lower
> the power limit of the device which the manufacturers can already do
> via the notification and perhaps other kernel drivers could do too.
> Non-root software can be limited by the init system in general.
Why does the power limit specifically need to be lowered? The goal is
to avoid excessive power consumption in this kind of state, right?
There are lots of other things that can be done to accomplish this:
For example:
* CPU boost be turned off
* EPP bias be adjusted to efficiency
* NVME APST can be tuned (idle timeout and transition latency tolerance)
See this table for more info on what Microsoft does while in Modern
Standby:
https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-nvme
See this comment in the kernel:
https://github.com/torvalds/linux/blob/4a4be1ad3a6efea16c56615f31117590fd881358/drivers/nvme/host/core.c#L2503
* Wifi power savings can be enacted
Those are all things clearly that userspace can accomplish.
What I'm getting at is perhaps the "suspend initiator" would be better
to do things than change the flow from the kernel.
1) Freeze relevant tasks
2) Work with the compositor to disable the display
3) Save/restore EPP, boost, APST and WPS values.
4) After a timeout (or whatever reason) when ready to go into "HW DRIPS"
then it can call the traditional suspend routine.
Then when the system wakes up from "HW DRIPS" the "suspend initiator"
can decide when to restore all those values, work with compositor to
turn on the display etc.
>
> As a side note, after all tasks have frozen, including compositors,
> you can fire DPMS safely before beginning the suspend sequence for
> backwards compatibility and to lower the span the old framebuffer is
> shown. This would be a useful addition to this series.
>
>>>
>>>>> +
>>>>> +Before entering the "Screen Off" state or suspending, it is recommended that
>>>>> +userspace marks all CRTCs as inactive (DPMS). Otherwise, there will be a split
>>>>> +second where the display of the device is on, but the presentation of the system
>>>>> +is inactive (e.g., the power button pulses), which is undesirable.
>>>>> \ No newline at end of file
>>>>> diff --git a/Documentation/admin-guide/pm/system-wide.rst b/Documentation/admin-guide/pm/system-wide.rst
>>>>> index 1a1924d71006..411775fae4ac 100644
>>>>> --- a/Documentation/admin-guide/pm/system-wide.rst
>>>>> +++ b/Documentation/admin-guide/pm/system-wide.rst
>>>>> @@ -8,4 +8,5 @@ System-Wide Power Management
>>>>> :maxdepth: 2
>>>>>
>>>>> sleep-states
>>>>> + standby-states
>>>>> suspend-flows
>>>>
>>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [RFC 01/13] Documentation: PM: Add documentation for S0ix Standby States
2024-11-21 21:08 ` Mario Limonciello
@ 2024-11-21 21:23 ` Antheas Kapenekakis
0 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 21:23 UTC (permalink / raw)
To: Mario Limonciello
Cc: linux-pm, platform-driver-x86, dri-devel, Hans de Goede,
Kyle Gospodnetich
On Thu, 21 Nov 2024 at 22:08, Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 11/21/2024 14:33, Antheas Kapenekakis wrote:
> > On Thu, 21 Nov 2024 at 20:40, Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> On 11/21/2024 13:11, Antheas Kapenekakis wrote:
> >>> On Thu, 21 Nov 2024 at 19:58, Mario Limonciello
> >>> <mario.limonciello@amd.com> wrote:
> >>>>
> >>>> On 11/21/2024 11:22, Antheas Kapenekakis wrote:
> >>>>> Add documentation about the S0ix Standby States that will be exposed
> >>>>> to userspace as part of this series.
> >>>>>
> >>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>>>> ---
> >>>>> .../admin-guide/pm/standby-states.rst | 133 ++++++++++++++++++
> >>>>> Documentation/admin-guide/pm/system-wide.rst | 1 +
> >>>>> 2 files changed, 134 insertions(+)
> >>>>> create mode 100644 Documentation/admin-guide/pm/standby-states.rst
> >>>>>
> >>>>> diff --git a/Documentation/admin-guide/pm/standby-states.rst b/Documentation/admin-guide/pm/standby-states.rst
> >>>>> new file mode 100644
> >>>>> index 000000000000..96727574312d
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/admin-guide/pm/standby-states.rst
> >>>>> @@ -0,0 +1,133 @@
> >>>>> +.. SPDX-License-Identifier: GPL-2.0
> >>>>> +.. include:: <isonum.txt>
> >>>>> +
> >>>>> +=====================
> >>>>> +S0ix Standby States
> >>>>> +=====================
> >>>>> +
> >>>>> +:Copyright: |copy| 2024 Antheas Kapenekakis
> >>>>> +
> >>>>> +:Author: Antheas Kapenekakis <lkml@antheas.dev>
> >>>>> +
> >>>>> +With the advent of modern mobile devices, users have become accustomed to instant
> >>>>> +wake-up times and always-on connectivity. To meet these expectations, modern
> >>>>> +standby was created, which is a standard that allows the platform to seamlessly
> >>>>> +transition between an S3-like low-power idle state and a set of low power active
> >>>>> +states, where connectivity is maintained, and the system is responsive to user
> >>>>> +input. Current x86 hardware supports 5 different standby states, which are:
> >>>>> +"Deepest run-time idle platform state" or "DRIPS" (S3-like), "Sleep", "Resume",
> >>>>> +"Screen Off", and "Active".
> >>>>> +
> >>>>> +The system begins in the "Active" state. Either due to user inactivity or
> >>>>> +user action (e.g., pressing the power button), it transitions to the "Screen Off"
> >>>>> +state.
> >>>>
> >>>> So are you implicitly suggesting that userspace should be responsible
> >>>> for *telling* the kernel that the screen is off? I feel some DRM
> >>>> helpers are missing to make it easy, but after such helpers are made the
> >>>> kernel "should" be able to easily tell this on it's own.
> >>>
> >>> There are two issues with this
> >>> 1) Windows implements a 5 second grace period on idle before firing
> >>> that firmware notification [1]. This is also a partial debounce, the
> >>> kernel cannot do that reliably or with the finesse required for such a
> >>> notification
> >>
> >> Why can't the kernel do this? I'm thinking something like this pseudo
> >> code that is triggered when number of enabled CRTCs changes:
> >>
> >> if (in_suspend_sequence)
> >> return;
> >> switch (old_num_displays) {
> >> case 0:
> >> display_on_cb();
> >> default:
> >> schedule_delayed_work(&drm_s2idle_wq);
> >> }
> >>
> >> Then if the "normal" suspend sequence is started the delayed work is
> >> cancelled.
> >>
> >> If the "normal" suspend sequence doesn't start when it fires then it
> >> would call the display off callback.
> >
> > Fundamentally, it is more complicated and error prone than 2 systemd
> > suspend targets that fire at the same time DEs lock the lock screen
> > (or any init system for that matter).
>
> 2 userspace jobs for the suspend sequence firing at same time vying for
> similar resources?
One presuspend target and one post suspend target
> That sounds inherently racy.
>
> > This pseudocode also hardcodes
> > the delay and does not debounce the display on callback.
> >
>
> If sticking to the Microsoft way of doing this, then it would be
> hardcoded. But yeah if going this direction it "could" be something
> configurable by userspace.
>
> An actual implementation would need some locking protection like a mutex.
>
> > There is the theoretical risk of a device misbehaving if the callbacks
> > fire at the wrong time. But this risk is theoretical and could be
> > solved by a device driver quirk that blocks the transition for that
> > specific device. Which is also much simpler than trying to hardcode an
> > implementation that works with all devices.
> >
> >>> 2) Windows clearly states virtual or real and virtual can really
> >>> mean anything here.
> >>
> >> In the context of the kernel, to me this is a DRM driver that has made
> >> outputs that are not tied to a physical display. Does it mean anything
> >> else? They should still be DRM connectors, and they should still have a
> >> CRTC AFAICT.
> >
> > For all the devices I tested, the display calls change the
> > presentation of the device such as RGB or aux devices that drain power
> > during suspend. I do not see a connection to DRM. This points me to
> > userspace being more appropriate for handling this. It also solves all
> > UX edge cases because userspace knows when it is inactive.
> >
> > Userspace handling this will not be backwards compatible in the sense
> > that it will not fire when the displays turn off with current
> > userspace. But it preserves current behavior and as such it is not a
> > breaking change.
> >
> >>>
> >>> In the end, only systemd and the compositor know if both conditions 1
> >>> and 2 are met and as such can be responsible for the notification.
> >>>
> >>> However, if that notification firing before certain CRTCs are
> >>> deactivated causes issues, such DRM helpers could be used to block the
> >>> transition
> >>>
> >>> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/display--sleep--and-hibernate-idle-timers
> >>> [1]
> >>>
> >>>>> Afterwards, it is free to transition between the "Sleep", "DRIPS", and
> >>>>> +"Screen Off" states until user action is received. Once that happens, the system
> >>>>> +begins to transition to the "Active" state. From "DRIPS" or "Sleep", it
> >>>>> +transitions to "Resume", where the Power Limit (PLx) is restored to its normal
> >>>>> +level, to speed up finishing "Sleep". Then, it transitions to "Screen Off".
> >>>>> +If on "Screen Off" or after the transition, the display is prepared to turn on
> >>>>> +and the system transitions to "Active" alongside turning it on.
> >>>>> +
> >>>>> +To maintain battery life, in the Windows implementation, the system is allocated
> >>>>> +a maximum percentage of battery and time it can use while staying in idle states.
> >>>>> +By default, this is 5% of battery or up to 2 days, where the system designer/OEM
> >>>>> +is able to tweak these values. If the system exceeds either the battery
> >>>>> +percentage or time limit, it enters Hibernation (S4), through a concept
> >>>>> +called "Adaptive Hibernate".
> >>>>> +
> >>>>> +
> >>>>> +S0ix Standby States
> >>>>> +==================================
> >>>>> +The following idle states are supported::
> >>>>> +
> >>>>> + ↓→ <Hibernate (S4)>
> >>>>
> >>>> I think S4 distracts in this context.
> >>>
> >>> Sure, can be removed.
> >>>
> >>>>> + <DRIPS> ↔ <Sleep> ↔ <Screen Off> ↔ <Active>
> >>>>> + → → <Resume> ↑
> >>>>> +
> >>>>> +.. _s2idle_drips:
> >>>>> +
> >>>>> +DRIPS
> >>>>> +-----
> >>>>> +
> >>>>> +The "Deepest run-time idle platform state" or "DRIPS" is the lowest power idle
> >>>>> +state that the system can enter. It is similar to the S3 state, with the
> >>>>> +difference that the system may wake up faster than S3 and due to a larger number
> >>>>> +of interrupts (e.g., fingerprint sensor, touchpad, touchscreen). This state
> >>>>> +is entered when the system is told to suspend to idle, through conventional
> >>>>> +means (see :doc:`sleep states <sleep-states>`). The system can only transition
> >>>>> +to "DRIPS" while it is in the "Sleep" state. If it is not, the kernel will
> >>>>> +automatically transition to the "Sleep" state before beginning the suspend
> >>>>> +sequence and restore the previous state afterwards. After the kernel has
> >>>>> +suspended, the notifications LSP0 Entry and Exit are used.
> >>>>> +
> >>>>> +.. _s2idle_sleep:
> >>>>> +
> >>>>> +Sleep
> >>>>> +-----
> >>>>> +
> >>>>> +The "Sleep" state is a low power idle state where the kernel is fully active.
> >>>>> +However, userspace has been partially frozen, particularly desktop applications,
> >>>>> +and only essential "value adding" activities are allowed to run. This is not
> >>>>> +enforced by the kernel and is the responsibility of userspace (e.g., systemd).
> >>>>> +Hardware wise, the Sleep Entry and Exit firmware notifications are fired, which
> >>>>> +may lower the Power Limit (PLx), pulse the suspend light, turn off the keyboard
> >>>>> +lighting or disable a handheld device's gamepad. This state is associated with
> >>>>> +the firmware notifications "Sleep Entry" and "Sleep Exit".
> >>>>> +
> >>>>> +.. _s2idle_resume:
> >>>>> +
> >>>>> +Resume
> >>>>> +------
> >>>>> +
> >>>>> +The "Resume" state is a faux "Sleep" state that is used to fire the Turn On
> >>>>> +Display firmware notification when the system is in the "Sleep" state but
> >>>>> +intends to turn on the display. It solves the problem of system designers
> >>>>> +limiting the Power Limit (PLx) while the system is in the "Sleep" state causing
> >>>>
> >>>> AFAIK, PLx is an Intel specific acronym, it's probably better to be more
> >>>> generic in documentation. You mentioned PLx in a commit too.
> >>>
> >>> Microsoft used this term in their documentation [2]. Can update to
> >>> generic terms.
> >>>
> >>> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications#turn-on-display-notification-function-9
> >>> [2]
> >>>
> >>>>> +the system to wake up slower than desired. This firmware notification is used
> >>>>> +to restore the normal Power Limit of the system, while having it stay in the
> >>>>> +"Sleep" state. As such, the system can only transition to the "Resume" state
> >>>>> +while in the "Sleep" state and cannot re-transition to the "Sleep" state
> >>>>> +afterwards.
> >>>>> +
> >>>>> +.. _s2idle_screen_off:
> >>>>> +
> >>>>> +Screen Off
> >>>>> +----------
> >>>>> +
> >>>>> +The "Screen Off" state is the state the system enters when all its displays
> >>>>> +(virtual or real) turn off. It is used to signify the user is not actively
> >>>>> +using the system. The associated firmware notifications of "Display On" and
> >>>>> +"Display Off" are used by manufacturers to turn off certain hardware
> >>>>> +components that are associated with the display being on, e.g., a handheld
> >>>>> +device's controller and RGB. Windows implements a 5-second grace period
> >>>>> +before firing this callback when the screen turns off due to inactivity.
> >>>>> +
> >>>>> +.. _s2idle_active:
> >>>>> +
> >>>>> +Active
> >>>>> +------
> >>>>> +
> >>>>> +Finally, the "Active" state is the default state of the system and the one it
> >>>>> +has when it is turned on. It is the state where the system is fully operational,
> >>>>> +the displays of the device are on, and the user is actively interacting with
> >>>>> +the system.
> >>>>> +
> >>>>> +Basic ``sysfs`` Interface for S0ix Standby transitions
> >>>>> +=============================================================
> >>>>> +
> >>>>> +The file :file:`/sys/power/standby` can be used to transition the system between
> >>>>> +the different standby states. The file accepts the following values: ``active``,
> >>>>> +``screen_off``, ``sleep``, and ``resume``. File writes will block until the
> >>>>> +transition completes. It will return ``-EINVAL`` when asking for an unsupported
> >>>>> +state or, e.g., requesting ``resume`` when not in the ``sleep`` state. If there
> >>>>> +is an error during the transition, the transition will pause on the last
> >>>>> +error-free state and return an error. The file can be read to retrieve the
> >>>>> +current state (and potential ones) using the following format:
> >>>>> +``[active] screen_off sleep resume``. The state "DRIPS" is omitted, as it is
> >>>>> +entered through the conventional suspend to idle path and userspace will never
> >>>>> +be able to see its value due to being suspended.
> >>>>
> >>>> If you follow my above suggestion, I think this file is totally
> >>>> unnecessary and then there is no compatibility issue.
> >>>>
> >>>> It would mean that userspace if it wants to see this "screen off" state
> >>>> and associated performance needs to do literally just that - turn the
> >>>> screens off.
> >>>
> >>> Please see the reasoning above for Display On/Off. Also, you omitted
> >>> sleep and resume, which have no hardware analogues you can hook into
> >>> and are just as important if not more than Display On/Off.
> >>
> >> I suppose I'm not seeing the argument yet for why "sleep" and HW DRIPS
> >> need to be different. What kind of things would be allowed to run in
> >> this state? Who draws that line?
> >
> > The most useful thing would be maintaining some basic connectivity so
> > that the device can resume faster if it suspended a couple of minutes
> > before and handling transitions such as to hibernation. The transition
> > to hibernation is especially important, as if both DPMS and the sleep
> > transition fire the transition looks proper. Being able to run certain
> > maintenance tasks without changing the presentation of the device from
> > sleep (e.g., the APM timer to check the battery level) is important.
> >
>
> These points still seem to argue for "display on" vs "display off"
> though, not a "sleep" vs "HW DRIPS".
So the way the spec is designed is that the sleep entry points are the
ones that pulse the suspend light in new devices. The display off
turns off the keyboard backlight.
Just tested my thinkpad and on state screen_off it turns off the
keyboard light. It also pulses the suspend light. But its a 2021 intel
model so I suspect it predates the Sleep entry points. The legion Go
uses the sleep points.
In any case, 90% of this patch series is controlling when the suspend
light pulses and turning off the keyboard backlight.
>
> > Even without that, if userspace transitions to sleep and fires DPMS
> > before beginning freezing and the suspend sequence, it halfs the user
> > perceived delay to sleep. It is a big deal. This is a planned feature
> > for the next version of bazzite so I am testing it right now. It looks
> > really professional.
> >
>
> I feel that what you're mostly vying for is "Dark Resume", which is
> something that exists in the Chrome OS world:
>
> https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/power_manager/docs/dark_resume.md
>
> I feel with cooperation between the compositor and the initiator of
> suspend the same thing can be done outside of ChromeOS.
>
> >> As it stands today the kernel freezes all tasks when suspending, so in
> >> this "half" suspend state I feel like there would need to be some sort
> >> of allow list, no?
> >
> > I do sympathize with this. The most important part would be to lower
> > the power limit of the device which the manufacturers can already do
> > via the notification and perhaps other kernel drivers could do too.
> > Non-root software can be limited by the init system in general.
>
> Why does the power limit specifically need to be lowered? The goal is
> to avoid excessive power consumption in this kind of state, right?
Moreso to avoid overheating in a bag
> There are lots of other things that can be done to accomplish this:
> For example:
> * CPU boost be turned off
> * EPP bias be adjusted to efficiency
> * NVME APST can be tuned (idle timeout and transition latency tolerance)
> See this table for more info on what Microsoft does while in Modern
> Standby:
> https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-nvme
> See this comment in the kernel:
> https://github.com/torvalds/linux/blob/4a4be1ad3a6efea16c56615f31117590fd881358/drivers/nvme/host/core.c#L2503
> * Wifi power savings can be enacted
>
> Those are all things clearly that userspace can accomplish.
>
> What I'm getting at is perhaps the "suspend initiator" would be better
> to do things than change the flow from the kernel.
>
> 1) Freeze relevant tasks
> 2) Work with the compositor to disable the display
> 3) Save/restore EPP, boost, APST and WPS values.
> 4) After a timeout (or whatever reason) when ready to go into "HW DRIPS"
> then it can call the traditional suspend routine.
>
> Then when the system wakes up from "HW DRIPS" the "suspend initiator"
> can decide when to restore all those values, work with compositor to
> turn on the display etc.
Yes, that is indeed what I am planning to work on. This patch series is part of.
Systemd already has an applet that does some of that [1], and it can
also calculate hibernate offsets, handle APM timer for battery
warnings, and freeze userspace as of 255.
It would also be able to transition between DRIPS to hibernation after
a certain battery drop. It is quite limited now but the structure is
there.
[1] https://github.com/systemd/systemd/blob/main/src/sleep/sleep.c
> >
> > As a side note, after all tasks have frozen, including compositors,
> > you can fire DPMS safely before beginning the suspend sequence for
> > backwards compatibility and to lower the span the old framebuffer is
> > shown. This would be a useful addition to this series.
> >
> >>>
> >>>>> +
> >>>>> +Before entering the "Screen Off" state or suspending, it is recommended that
> >>>>> +userspace marks all CRTCs as inactive (DPMS). Otherwise, there will be a split
> >>>>> +second where the display of the device is on, but the presentation of the system
> >>>>> +is inactive (e.g., the power button pulses), which is undesirable.
> >>>>> \ No newline at end of file
> >>>>> diff --git a/Documentation/admin-guide/pm/system-wide.rst b/Documentation/admin-guide/pm/system-wide.rst
> >>>>> index 1a1924d71006..411775fae4ac 100644
> >>>>> --- a/Documentation/admin-guide/pm/system-wide.rst
> >>>>> +++ b/Documentation/admin-guide/pm/system-wide.rst
> >>>>> @@ -8,4 +8,5 @@ System-Wide Power Management
> >>>>> :maxdepth: 2
> >>>>>
> >>>>> sleep-states
> >>>>> + standby-states
> >>>>> suspend-flows
> >>>>
> >>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [RFC 02/13] acpi/x86: s2idle: add support for Display Off and Display On callbacks
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
2024-11-21 17:22 ` [RFC 01/13] Documentation: PM: Add documentation for S0ix Standby States Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-21 17:22 ` [RFC 03/13] acpi/x86: s2idle: add support for Sleep Entry and Sleep Exit callbacks Antheas Kapenekakis
` (12 subsequent siblings)
14 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, 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: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
include/linux/suspend.h | 2 ++
kernel/power/suspend.c | 10 ++++++++++
2 files changed, 12 insertions(+)
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index da6ebca3ff77..57c083754e8b 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
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 09f8397bae15..cded6b9b439b 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -254,6 +254,16 @@ static bool sleep_state_supported(suspend_state_t state)
(valid_state(state) && !cxl_mem_active());
}
+static int platform_standby_display_off(void)
+{
+ return s2idle_ops && s2idle_ops->display_off ? s2idle_ops->display_off() : 0;
+}
+
+static int platform_standby_display_on(void)
+{
+ return s2idle_ops && s2idle_ops->display_on ? s2idle_ops->display_on() : 0;
+}
+
static int platform_suspend_prepare(suspend_state_t state)
{
return state != PM_SUSPEND_TO_IDLE && suspend_ops->prepare ?
--
2.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* [RFC 03/13] acpi/x86: s2idle: add support for Sleep Entry and Sleep Exit callbacks
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
2024-11-21 17:22 ` [RFC 01/13] Documentation: PM: Add documentation for S0ix Standby States Antheas Kapenekakis
2024-11-21 17:22 ` [RFC 02/13] acpi/x86: s2idle: add support for Display Off and Display On callbacks Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-21 17:22 ` [RFC 04/13] acpi/x86: s2idle: add support for Turn On Display callback Antheas Kapenekakis
` (11 subsequent siblings)
14 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, Antheas Kapenekakis
The Sleep Entry and Sleep Exit firmware notifications allow the platform
to enter Modern Standby. In this state, if supported, the platform turns
off auxiliary USB devices (e.g., the controllers of the Legion Go),
makes the power light of the device flash, and lowers the power envelope
to a minimum that still allows for software activity without affecting
battery life.
Allow for entering this state prior to initiating the suspend sequence.
This fixes issues where the EC or the USB of the device need time to
power down before entering the suspend sequence, and allows for entering
this power state without suspending the device.
Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
include/linux/suspend.h | 2 ++
kernel/power/suspend.c | 10 ++++++++++
2 files changed, 12 insertions(+)
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 57c083754e8b..733dffb09b28 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -133,6 +133,7 @@ struct platform_suspend_ops {
struct platform_s2idle_ops {
int (*display_off)(void);
+ int (*sleep_entry)(void);
int (*begin)(void);
int (*prepare)(void);
int (*prepare_late)(void);
@@ -141,6 +142,7 @@ struct platform_s2idle_ops {
void (*restore_early)(void);
void (*restore)(void);
void (*end)(void);
+ int (*sleep_exit)(void);
int (*display_on)(void);
};
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index cded6b9b439b..3f4bbefa9b82 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -259,6 +259,16 @@ static int platform_standby_display_off(void)
return s2idle_ops && s2idle_ops->display_off ? s2idle_ops->display_off() : 0;
}
+static int platform_standby_sleep_entry(void)
+{
+ return s2idle_ops && s2idle_ops->sleep_entry ? s2idle_ops->sleep_entry() : 0;
+}
+
+static int platform_standby_sleep_exit(void)
+{
+ return s2idle_ops && s2idle_ops->sleep_exit ? s2idle_ops->sleep_exit() : 0;
+}
+
static int platform_standby_display_on(void)
{
return s2idle_ops && s2idle_ops->display_on ? s2idle_ops->display_on() : 0;
--
2.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* [RFC 04/13] acpi/x86: s2idle: add support for Turn On Display callback
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
` (2 preceding siblings ...)
2024-11-21 17:22 ` [RFC 03/13] acpi/x86: s2idle: add support for Sleep Entry and Sleep Exit callbacks Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-21 17:22 ` [RFC 05/13] acpi/x86: s2idle: add modern standby transition function Antheas Kapenekakis
` (10 subsequent siblings)
14 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, Antheas Kapenekakis
The Turn On Display callback was introduced in Windows 22H2, to allow
devices to resume faster from sleep. Essentially, if the device lowers
its power limit (PLx) while it is in the Sleep state, this might lengthen
the suspend sequence in an undesirable manner. Therefore, this callback
can be used to restore PLx while still remaining in the sleep state.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
include/linux/suspend.h | 1 +
kernel/power/suspend.c | 7 +++++++
2 files changed, 8 insertions(+)
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 733dffb09b28..01ee64321cda 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -142,6 +142,7 @@ struct platform_s2idle_ops {
void (*restore_early)(void);
void (*restore)(void);
void (*end)(void);
+ int (*turn_on_display)(void);
int (*sleep_exit)(void);
int (*display_on)(void);
};
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3f4bbefa9b82..a42e8514ee7a 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -264,6 +264,13 @@ static int platform_standby_sleep_entry(void)
return s2idle_ops && s2idle_ops->sleep_entry ? s2idle_ops->sleep_entry() : 0;
}
+static int platform_standby_turn_on_display(void)
+{
+ return s2idle_ops && s2idle_ops->turn_on_display ?
+ s2idle_ops->turn_on_display() :
+ 0;
+}
+
static int platform_standby_sleep_exit(void)
{
return s2idle_ops && s2idle_ops->sleep_exit ? s2idle_ops->sleep_exit() : 0;
--
2.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* [RFC 05/13] acpi/x86: s2idle: add modern standby transition function
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
` (3 preceding siblings ...)
2024-11-21 17:22 ` [RFC 04/13] acpi/x86: s2idle: add support for Turn On Display callback Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-21 18:15 ` Mario Limonciello
2024-11-21 17:22 ` [RFC 06/13] acpi/x86: s2idle: rename Screen On/Off to Display On/Off Antheas Kapenekakis
` (9 subsequent siblings)
14 siblings, 1 reply; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, Antheas Kapenekakis
Add a new function to transition modern standby states and call it
as part of the suspend sequence to make sure it begins under the
Modern Standby "Sleep" state.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
include/linux/suspend.h | 11 ++++
kernel/power/power.h | 1 +
kernel/power/suspend.c | 127 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 139 insertions(+)
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 01ee64321cda..b8fe781d8026 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -40,6 +40,15 @@ typedef int __bitwise suspend_state_t;
#define PM_SUSPEND_MIN PM_SUSPEND_TO_IDLE
#define PM_SUSPEND_MAX ((__force suspend_state_t) 4)
+typedef int __bitwise standby_state_t;
+
+#define PM_STANDBY_ACTIVE ((__force standby_state_t) 0)
+#define PM_STANDBY_SCREEN_OFF ((__force standby_state_t) 1)
+#define PM_STANDBY_SLEEP ((__force standby_state_t) 2)
+#define PM_STANDBY_RESUME ((__force standby_state_t) 3)
+#define PM_STANDBY_MIN PM_STANDBY_ACTIVE
+#define PM_STANDBY_MAX ((__force standby_state_t) 4)
+
/**
* struct platform_suspend_ops - Callbacks for managing platform dependent
* system sleep states.
@@ -281,6 +290,8 @@ extern void arch_suspend_enable_irqs(void);
extern int pm_suspend(suspend_state_t state);
extern bool sync_on_suspend_enabled;
+extern int pm_standby_transition(standby_state_t state);
+extern int pm_standby_state(void);
#else /* !CONFIG_SUSPEND */
#define suspend_valid_only_mem NULL
diff --git a/kernel/power/power.h b/kernel/power/power.h
index de0e6b1077f2..4ee067cd0d4d 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -207,6 +207,7 @@ extern void swsusp_show_speed(ktime_t, ktime_t, unsigned int, char *);
extern const char * const pm_labels[];
extern const char *pm_states[];
extern const char *mem_sleep_states[];
+extern const char *standby_states[];
extern int suspend_devices_and_enter(suspend_state_t state);
#else /* !CONFIG_SUSPEND */
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index a42e8514ee7a..1865db71a0c2 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -45,12 +45,21 @@ static const char * const mem_sleep_labels[] = {
[PM_SUSPEND_MEM] = "deep",
};
const char *mem_sleep_states[PM_SUSPEND_MAX];
+static const char * const standby_labels[] = {
+ [PM_STANDBY_ACTIVE] = "active",
+ [PM_STANDBY_SCREEN_OFF] = "screen_off",
+ [PM_STANDBY_SLEEP] = "sleep",
+ [PM_STANDBY_RESUME] = "resume",
+};
+const char *standby_states[PM_STANDBY_MAX];
suspend_state_t mem_sleep_current = PM_SUSPEND_TO_IDLE;
suspend_state_t mem_sleep_default = PM_SUSPEND_MAX;
suspend_state_t pm_suspend_target_state;
EXPORT_SYMBOL_GPL(pm_suspend_target_state);
+standby_state_t standby_current = PM_STANDBY_ACTIVE;
+
unsigned int pm_suspend_global_flags;
EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
@@ -188,6 +197,16 @@ void __init pm_states_init(void)
* initialize mem_sleep_states[] accordingly here.
*/
mem_sleep_states[PM_SUSPEND_TO_IDLE] = mem_sleep_labels[PM_SUSPEND_TO_IDLE];
+ /* All systems support the "active" state. */
+ standby_states[PM_STANDBY_ACTIVE] = standby_labels[PM_STANDBY_ACTIVE];
+ /*
+ * Not all systems support these states, where they will have increased
+ * power consumption. If deemed necessary, they should be gated to not
+ * mislead userspace.
+ */
+ standby_states[PM_STANDBY_SCREEN_OFF] = standby_labels[PM_STANDBY_SCREEN_OFF];
+ standby_states[PM_STANDBY_SLEEP] = standby_labels[PM_STANDBY_SLEEP];
+ standby_states[PM_STANDBY_RESUME] = standby_labels[PM_STANDBY_RESUME];
}
static int __init mem_sleep_default_setup(char *str)
@@ -354,6 +373,108 @@ static bool platform_suspend_again(suspend_state_t state)
suspend_ops->suspend_again() : false;
}
+static int platform_standby_transition_internal(standby_state_t state)
+{
+ int error;
+
+ if (state == standby_current)
+ return 0;
+ if (state > PM_STANDBY_MAX)
+ return -EINVAL;
+
+ pm_pr_dbg("Transitioning from standby state %s to %s\n",
+ standby_states[standby_current], standby_states[state]);
+
+ /* Resume can only be entered if we are on the sleep state. */
+ if (state == PM_STANDBY_RESUME) {
+ if (standby_current != PM_STANDBY_SLEEP)
+ return -EINVAL;
+ standby_current = PM_STANDBY_RESUME;
+ return platform_standby_turn_on_display();
+ }
+
+ /*
+ * The system should not be able to re-enter Sleep from resume as it
+ * is undefined behavior. As part of setting the state to "Resume",
+ * were promised a transition to "Screen Off" or "Active".
+ */
+ if (standby_current == PM_STANDBY_RESUME && state == PM_STANDBY_SLEEP)
+ return -EINVAL;
+
+ /* Resume is the Sleep state logic-wise. */
+ if (standby_current == PM_STANDBY_RESUME)
+ standby_current = PM_STANDBY_SLEEP;
+
+ if (standby_current < state) {
+ for (; standby_current < state; standby_current++) {
+ switch (standby_current + 1) {
+ case PM_STANDBY_SCREEN_OFF:
+ error = platform_standby_display_off();
+ break;
+ case PM_STANDBY_SLEEP:
+ error = platform_standby_sleep_entry();
+ break;
+ }
+
+ if (error)
+ return error;
+ }
+ } else if (standby_current > state) {
+ for (; standby_current > state; standby_current--) {
+ switch (standby_current) {
+ case PM_STANDBY_SLEEP:
+ error = platform_standby_sleep_exit();
+ break;
+ case PM_STANDBY_SCREEN_OFF:
+ error = platform_standby_display_on();
+ break;
+ }
+
+ if (error)
+ return error;
+ }
+ }
+
+ return 0;
+}
+
+/**
+ * pm_standby_transition - Transition between Modern Standby states
+ *
+ * Fires the appropriate firmware notifications to transition to the requested
+ * state. Returns an error if the transition fails. The function does not
+ * rollback. It is up to userspace to handle the error and re-transition when
+ * appropriate.
+ */
+int pm_standby_transition(standby_state_t state)
+{
+ unsigned int sleep_flags;
+ int error;
+
+ sleep_flags = lock_system_sleep();
+ error = platform_standby_transition_internal(state);
+ unlock_system_sleep(sleep_flags);
+
+ return error;
+}
+EXPORT_SYMBOL_GPL(pm_standby_transition);
+
+/**
+ * pm_standby_state - Returns the current standby state
+ */
+int pm_standby_state(void)
+{
+ unsigned int sleep_flags;
+ int state;
+
+ sleep_flags = lock_system_sleep();
+ state = standby_current;
+ unlock_system_sleep(sleep_flags);
+
+ return state;
+}
+EXPORT_SYMBOL_GPL(pm_standby_state);
+
#ifdef CONFIG_PM_DEBUG
static unsigned int pm_test_delay = 5;
module_param(pm_test_delay, uint, 0644);
@@ -586,6 +707,7 @@ static void suspend_finish(void)
static int enter_state(suspend_state_t state)
{
int error;
+ standby_state_t standby_prior;
trace_suspend_resume(TPS("suspend_enter"), state, true);
if (state == PM_SUSPEND_TO_IDLE) {
@@ -601,6 +723,9 @@ static int enter_state(suspend_state_t state)
if (!mutex_trylock(&system_transition_mutex))
return -EBUSY;
+ standby_prior = standby_current;
+ platform_standby_transition_internal(PM_STANDBY_SLEEP);
+
if (state == PM_SUSPEND_TO_IDLE)
s2idle_begin();
@@ -630,6 +755,8 @@ static int enter_state(suspend_state_t state)
pm_pr_dbg("Finishing wakeup.\n");
suspend_finish();
Unlock:
+ platform_standby_transition_internal(standby_prior);
+
mutex_unlock(&system_transition_mutex);
return error;
}
--
2.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [RFC 05/13] acpi/x86: s2idle: add modern standby transition function
2024-11-21 17:22 ` [RFC 05/13] acpi/x86: s2idle: add modern standby transition function Antheas Kapenekakis
@ 2024-11-21 18:15 ` Mario Limonciello
2024-11-21 18:29 ` Antheas Kapenekakis
0 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello @ 2024-11-21 18:15 UTC (permalink / raw)
To: Antheas Kapenekakis, linux-pm
Cc: platform-driver-x86, dri-devel, Hans de Goede, Kyle Gospodnetich
On 11/21/2024 11:22, Antheas Kapenekakis wrote:
> Add a new function to transition modern standby states and call it
> as part of the suspend sequence to make sure it begins under the
> Modern Standby "Sleep" state.
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
> include/linux/suspend.h | 11 ++++
> kernel/power/power.h | 1 +
> kernel/power/suspend.c | 127 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 139 insertions(+)
>
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 01ee64321cda..b8fe781d8026 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -40,6 +40,15 @@ typedef int __bitwise suspend_state_t;
> #define PM_SUSPEND_MIN PM_SUSPEND_TO_IDLE
> #define PM_SUSPEND_MAX ((__force suspend_state_t) 4)
>
> +typedef int __bitwise standby_state_t;
As this is series is working on emulating semantics of "Modern Standby"
on Windows, why not name it all "modern_standby"?
IE
modern_standby_state_t
PM_MODERN_STANDBY_ACTIVE
PM_MODERN_STANDBY_SCREEN_OFF
> +
> +#define PM_STANDBY_ACTIVE ((__force standby_state_t) 0)
> +#define PM_STANDBY_SCREEN_OFF ((__force standby_state_t) 1)
> +#define PM_STANDBY_SLEEP ((__force standby_state_t) 2)
> +#define PM_STANDBY_RESUME ((__force standby_state_t) 3)
> +#define PM_STANDBY_MIN PM_STANDBY_ACTIVE
> +#define PM_STANDBY_MAX ((__force standby_state_t) 4)
> +
> /**
> * struct platform_suspend_ops - Callbacks for managing platform dependent
> * system sleep states.
> @@ -281,6 +290,8 @@ extern void arch_suspend_enable_irqs(void);
>
> extern int pm_suspend(suspend_state_t state);
> extern bool sync_on_suspend_enabled;
> +extern int pm_standby_transition(standby_state_t state);
> +extern int pm_standby_state(void);
> #else /* !CONFIG_SUSPEND */
> #define suspend_valid_only_mem NULL
>
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index de0e6b1077f2..4ee067cd0d4d 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -207,6 +207,7 @@ extern void swsusp_show_speed(ktime_t, ktime_t, unsigned int, char *);
> extern const char * const pm_labels[];
> extern const char *pm_states[];
> extern const char *mem_sleep_states[];
> +extern const char *standby_states[];
>
> extern int suspend_devices_and_enter(suspend_state_t state);
> #else /* !CONFIG_SUSPEND */
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index a42e8514ee7a..1865db71a0c2 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -45,12 +45,21 @@ static const char * const mem_sleep_labels[] = {
> [PM_SUSPEND_MEM] = "deep",
> };
> const char *mem_sleep_states[PM_SUSPEND_MAX];
> +static const char * const standby_labels[] = {
> + [PM_STANDBY_ACTIVE] = "active",
> + [PM_STANDBY_SCREEN_OFF] = "screen_off",
> + [PM_STANDBY_SLEEP] = "sleep",
> + [PM_STANDBY_RESUME] = "resume",
> +};
> +const char *standby_states[PM_STANDBY_MAX];
>
> suspend_state_t mem_sleep_current = PM_SUSPEND_TO_IDLE;
> suspend_state_t mem_sleep_default = PM_SUSPEND_MAX;
> suspend_state_t pm_suspend_target_state;
> EXPORT_SYMBOL_GPL(pm_suspend_target_state);
>
> +standby_state_t standby_current = PM_STANDBY_ACTIVE;
> +
> unsigned int pm_suspend_global_flags;
> EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
>
> @@ -188,6 +197,16 @@ void __init pm_states_init(void)
> * initialize mem_sleep_states[] accordingly here.
> */
> mem_sleep_states[PM_SUSPEND_TO_IDLE] = mem_sleep_labels[PM_SUSPEND_TO_IDLE];
> + /* All systems support the "active" state. */
> + standby_states[PM_STANDBY_ACTIVE] = standby_labels[PM_STANDBY_ACTIVE];
> + /*
> + * Not all systems support these states, where they will have increased
> + * power consumption. If deemed necessary, they should be gated to not
> + * mislead userspace.
> + */
> + standby_states[PM_STANDBY_SCREEN_OFF] = standby_labels[PM_STANDBY_SCREEN_OFF];
> + standby_states[PM_STANDBY_SLEEP] = standby_labels[PM_STANDBY_SLEEP];
> + standby_states[PM_STANDBY_RESUME] = standby_labels[PM_STANDBY_RESUME];
Shouldn't these states only be enabled when LPS0 support was found? IE
shouldn't they be enabled by acpi_register_lps0_dev() and disabled by
acpi_unregister_lps0_dev()
> }
>
> static int __init mem_sleep_default_setup(char *str)
> @@ -354,6 +373,108 @@ static bool platform_suspend_again(suspend_state_t state)
> suspend_ops->suspend_again() : false;
> }
>
> +static int platform_standby_transition_internal(standby_state_t state)
> +{
> + int error;
> +
> + if (state == standby_current)
> + return 0;
> + if (state > PM_STANDBY_MAX)
> + return -EINVAL;
> +
> + pm_pr_dbg("Transitioning from standby state %s to %s\n",
> + standby_states[standby_current], standby_states[state]);
> +
> + /* Resume can only be entered if we are on the sleep state. */
> + if (state == PM_STANDBY_RESUME) {
> + if (standby_current != PM_STANDBY_SLEEP)
> + return -EINVAL;
> + standby_current = PM_STANDBY_RESUME;
> + return platform_standby_turn_on_display();
> + }
> +
> + /*
> + * The system should not be able to re-enter Sleep from resume as it
> + * is undefined behavior. As part of setting the state to "Resume",
> + * were promised a transition to "Screen Off" or "Active".
> + */
> + if (standby_current == PM_STANDBY_RESUME && state == PM_STANDBY_SLEEP)
> + return -EINVAL;
> +
> + /* Resume is the Sleep state logic-wise. */
> + if (standby_current == PM_STANDBY_RESUME)
> + standby_current = PM_STANDBY_SLEEP;
> +
> + if (standby_current < state) {
> + for (; standby_current < state; standby_current++) {
> + switch (standby_current + 1) {
> + case PM_STANDBY_SCREEN_OFF:
> + error = platform_standby_display_off();
> + break;
> + case PM_STANDBY_SLEEP:
> + error = platform_standby_sleep_entry();
> + break;
> + }
> +
> + if (error)
> + return error;
> + }
> + } else if (standby_current > state) {
> + for (; standby_current > state; standby_current--) {
> + switch (standby_current) {
> + case PM_STANDBY_SLEEP:
> + error = platform_standby_sleep_exit();
> + break;
> + case PM_STANDBY_SCREEN_OFF:
> + error = platform_standby_display_on();
> + break;
> + }
> +
> + if (error)
> + return error;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * pm_standby_transition - Transition between Modern Standby states
> + *
> + * Fires the appropriate firmware notifications to transition to the requested
> + * state. Returns an error if the transition fails. The function does not
> + * rollback. It is up to userspace to handle the error and re-transition when
> + * appropriate.
> + */
> +int pm_standby_transition(standby_state_t state)
> +{
> + unsigned int sleep_flags;
> + int error;
> +
> + sleep_flags = lock_system_sleep();
> + error = platform_standby_transition_internal(state);
> + unlock_system_sleep(sleep_flags);
> +
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(pm_standby_transition);
> +
> +/**
> + * pm_standby_state - Returns the current standby state
> + */
> +int pm_standby_state(void)
> +{
> + unsigned int sleep_flags;
> + int state;
> +
> + sleep_flags = lock_system_sleep();
> + state = standby_current;
> + unlock_system_sleep(sleep_flags);
> +
> + return state;
> +}
> +EXPORT_SYMBOL_GPL(pm_standby_state);
> +
> #ifdef CONFIG_PM_DEBUG
> static unsigned int pm_test_delay = 5;
> module_param(pm_test_delay, uint, 0644);
> @@ -586,6 +707,7 @@ static void suspend_finish(void)
> static int enter_state(suspend_state_t state)
> {
> int error;
> + standby_state_t standby_prior;
>
> trace_suspend_resume(TPS("suspend_enter"), state, true);
> if (state == PM_SUSPEND_TO_IDLE) {
> @@ -601,6 +723,9 @@ static int enter_state(suspend_state_t state)
> if (!mutex_trylock(&system_transition_mutex))
> return -EBUSY;
>
> + standby_prior = standby_current;
> + platform_standby_transition_internal(PM_STANDBY_SLEEP);
> +
> if (state == PM_SUSPEND_TO_IDLE)
> s2idle_begin();
>
> @@ -630,6 +755,8 @@ static int enter_state(suspend_state_t state)
> pm_pr_dbg("Finishing wakeup.\n");
> suspend_finish();
> Unlock:
> + platform_standby_transition_internal(standby_prior);
> +
> mutex_unlock(&system_transition_mutex);
> return error;
> }
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [RFC 05/13] acpi/x86: s2idle: add modern standby transition function
2024-11-21 18:15 ` Mario Limonciello
@ 2024-11-21 18:29 ` Antheas Kapenekakis
0 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 18:29 UTC (permalink / raw)
To: Mario Limonciello
Cc: linux-pm, platform-driver-x86, dri-devel, Hans de Goede,
Kyle Gospodnetich
On Thu, 21 Nov 2024 at 19:16, Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 11/21/2024 11:22, Antheas Kapenekakis wrote:
> > Add a new function to transition modern standby states and call it
> > as part of the suspend sequence to make sure it begins under the
> > Modern Standby "Sleep" state.
> > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> > include/linux/suspend.h | 11 ++++
> > kernel/power/power.h | 1 +
> > kernel/power/suspend.c | 127 ++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 139 insertions(+)
> >
> > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > index 01ee64321cda..b8fe781d8026 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -40,6 +40,15 @@ typedef int __bitwise suspend_state_t;
> > #define PM_SUSPEND_MIN PM_SUSPEND_TO_IDLE
> > #define PM_SUSPEND_MAX ((__force suspend_state_t) 4)
> >
> > +typedef int __bitwise standby_state_t;
>
> As this is series is working on emulating semantics of "Modern Standby"
> on Windows, why not name it all "modern_standby"?
>
> IE
>
> modern_standby_state_t
> PM_MODERN_STANDBY_ACTIVE
> PM_MODERN_STANDBY_SCREEN_OFF
I would rather keep the name more generalizable. Modern Standby is the
Microsoft term. That was the idea behind it at least. If other drivers
want to hook into this, I think it would be desirable as well. As far
as the s2ildle x86 semantics thats up in the air htough.
> > +
> > +#define PM_STANDBY_ACTIVE ((__force standby_state_t) 0)
> > +#define PM_STANDBY_SCREEN_OFF ((__force standby_state_t) 1)
> > +#define PM_STANDBY_SLEEP ((__force standby_state_t) 2)
> > +#define PM_STANDBY_RESUME ((__force standby_state_t) 3)
> > +#define PM_STANDBY_MIN PM_STANDBY_ACTIVE
> > +#define PM_STANDBY_MAX ((__force standby_state_t) 4)
> > +
> > /**
> > * struct platform_suspend_ops - Callbacks for managing platform dependent
> > * system sleep states.
> > @@ -281,6 +290,8 @@ extern void arch_suspend_enable_irqs(void);
> >
> > extern int pm_suspend(suspend_state_t state);
> > extern bool sync_on_suspend_enabled;
> > +extern int pm_standby_transition(standby_state_t state);
> > +extern int pm_standby_state(void);
> > #else /* !CONFIG_SUSPEND */
> > #define suspend_valid_only_mem NULL
> >
> > diff --git a/kernel/power/power.h b/kernel/power/power.h
> > index de0e6b1077f2..4ee067cd0d4d 100644
> > --- a/kernel/power/power.h
> > +++ b/kernel/power/power.h
> > @@ -207,6 +207,7 @@ extern void swsusp_show_speed(ktime_t, ktime_t, unsigned int, char *);
> > extern const char * const pm_labels[];
> > extern const char *pm_states[];
> > extern const char *mem_sleep_states[];
> > +extern const char *standby_states[];
> >
> > extern int suspend_devices_and_enter(suspend_state_t state);
> > #else /* !CONFIG_SUSPEND */
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index a42e8514ee7a..1865db71a0c2 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -45,12 +45,21 @@ static const char * const mem_sleep_labels[] = {
> > [PM_SUSPEND_MEM] = "deep",
> > };
> > const char *mem_sleep_states[PM_SUSPEND_MAX];
> > +static const char * const standby_labels[] = {
> > + [PM_STANDBY_ACTIVE] = "active",
> > + [PM_STANDBY_SCREEN_OFF] = "screen_off",
> > + [PM_STANDBY_SLEEP] = "sleep",
> > + [PM_STANDBY_RESUME] = "resume",
> > +};
> > +const char *standby_states[PM_STANDBY_MAX];
> >
> > suspend_state_t mem_sleep_current = PM_SUSPEND_TO_IDLE;
> > suspend_state_t mem_sleep_default = PM_SUSPEND_MAX;
> > suspend_state_t pm_suspend_target_state;
> > EXPORT_SYMBOL_GPL(pm_suspend_target_state);
> >
> > +standby_state_t standby_current = PM_STANDBY_ACTIVE;
> > +
> > unsigned int pm_suspend_global_flags;
> > EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
> >
> > @@ -188,6 +197,16 @@ void __init pm_states_init(void)
> > * initialize mem_sleep_states[] accordingly here.
> > */
> > mem_sleep_states[PM_SUSPEND_TO_IDLE] = mem_sleep_labels[PM_SUSPEND_TO_IDLE];
> > + /* All systems support the "active" state. */
> > + standby_states[PM_STANDBY_ACTIVE] = standby_labels[PM_STANDBY_ACTIVE];
> > + /*
> > + * Not all systems support these states, where they will have increased
> > + * power consumption. If deemed necessary, they should be gated to not
> > + * mislead userspace.
> > + */
> > + standby_states[PM_STANDBY_SCREEN_OFF] = standby_labels[PM_STANDBY_SCREEN_OFF];
> > + standby_states[PM_STANDBY_SLEEP] = standby_labels[PM_STANDBY_SLEEP];
> > + standby_states[PM_STANDBY_RESUME] = standby_labels[PM_STANDBY_RESUME];
>
> Shouldn't these states only be enabled when LPS0 support was found? IE
> shouldn't they be enabled by acpi_register_lps0_dev() and disabled by
> acpi_unregister_lps0_dev()
This is a place i want to look more into. Yes I agree that these
states should be hidden. However, we have the issue right now where
s2idle is globally enabled too, even if the platform does not support
S3 or advertise S0 support in the FADT. Windows correctly hibernates
Ayaneo devices that do not advertise either for example, where linux
just makes them enter a semi broken state. Ayaneo has started
releasing updated BIOSes that fix that though so that fixes that issue
until another manufacturer decides to do that.
I would also like to see if it is possible to hide individual states
if the hardware does not support them. However, I think ACPI does not
advertise support for any of these states. IE you either support all
of them or none of them through the UUID existing.
> > }
> >
> > static int __init mem_sleep_default_setup(char *str)
> > @@ -354,6 +373,108 @@ static bool platform_suspend_again(suspend_state_t state)
> > suspend_ops->suspend_again() : false;
> > }
> >
> > +static int platform_standby_transition_internal(standby_state_t state)
> > +{
> > + int error;
> > +
> > + if (state == standby_current)
> > + return 0;
> > + if (state > PM_STANDBY_MAX)
> > + return -EINVAL;
> > +
> > + pm_pr_dbg("Transitioning from standby state %s to %s\n",
> > + standby_states[standby_current], standby_states[state]);
> > +
> > + /* Resume can only be entered if we are on the sleep state. */
> > + if (state == PM_STANDBY_RESUME) {
> > + if (standby_current != PM_STANDBY_SLEEP)
> > + return -EINVAL;
> > + standby_current = PM_STANDBY_RESUME;
> > + return platform_standby_turn_on_display();
> > + }
> > +
> > + /*
> > + * The system should not be able to re-enter Sleep from resume as it
> > + * is undefined behavior. As part of setting the state to "Resume",
> > + * were promised a transition to "Screen Off" or "Active".
> > + */
> > + if (standby_current == PM_STANDBY_RESUME && state == PM_STANDBY_SLEEP)
> > + return -EINVAL;
> > +
> > + /* Resume is the Sleep state logic-wise. */
> > + if (standby_current == PM_STANDBY_RESUME)
> > + standby_current = PM_STANDBY_SLEEP;
> > +
> > + if (standby_current < state) {
> > + for (; standby_current < state; standby_current++) {
> > + switch (standby_current + 1) {
> > + case PM_STANDBY_SCREEN_OFF:
> > + error = platform_standby_display_off();
> > + break;
> > + case PM_STANDBY_SLEEP:
> > + error = platform_standby_sleep_entry();
> > + break;
> > + }
> > +
> > + if (error)
> > + return error;
> > + }
> > + } else if (standby_current > state) {
> > + for (; standby_current > state; standby_current--) {
> > + switch (standby_current) {
> > + case PM_STANDBY_SLEEP:
> > + error = platform_standby_sleep_exit();
> > + break;
> > + case PM_STANDBY_SCREEN_OFF:
> > + error = platform_standby_display_on();
> > + break;
> > + }
> > +
> > + if (error)
> > + return error;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * pm_standby_transition - Transition between Modern Standby states
> > + *
> > + * Fires the appropriate firmware notifications to transition to the requested
> > + * state. Returns an error if the transition fails. The function does not
> > + * rollback. It is up to userspace to handle the error and re-transition when
> > + * appropriate.
> > + */
> > +int pm_standby_transition(standby_state_t state)
> > +{
> > + unsigned int sleep_flags;
> > + int error;
> > +
> > + sleep_flags = lock_system_sleep();
> > + error = platform_standby_transition_internal(state);
> > + unlock_system_sleep(sleep_flags);
> > +
> > + return error;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_standby_transition);
> > +
> > +/**
> > + * pm_standby_state - Returns the current standby state
> > + */
> > +int pm_standby_state(void)
> > +{
> > + unsigned int sleep_flags;
> > + int state;
> > +
> > + sleep_flags = lock_system_sleep();
> > + state = standby_current;
> > + unlock_system_sleep(sleep_flags);
> > +
> > + return state;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_standby_state);
> > +
> > #ifdef CONFIG_PM_DEBUG
> > static unsigned int pm_test_delay = 5;
> > module_param(pm_test_delay, uint, 0644);
> > @@ -586,6 +707,7 @@ static void suspend_finish(void)
> > static int enter_state(suspend_state_t state)
> > {
> > int error;
> > + standby_state_t standby_prior;
> >
> > trace_suspend_resume(TPS("suspend_enter"), state, true);
> > if (state == PM_SUSPEND_TO_IDLE) {
> > @@ -601,6 +723,9 @@ static int enter_state(suspend_state_t state)
> > if (!mutex_trylock(&system_transition_mutex))
> > return -EBUSY;
> >
> > + standby_prior = standby_current;
> > + platform_standby_transition_internal(PM_STANDBY_SLEEP);
> > +
> > if (state == PM_SUSPEND_TO_IDLE)
> > s2idle_begin();
> >
> > @@ -630,6 +755,8 @@ static int enter_state(suspend_state_t state)
> > pm_pr_dbg("Finishing wakeup.\n");
> > suspend_finish();
> > Unlock:
> > + platform_standby_transition_internal(standby_prior);
> > +
> > mutex_unlock(&system_transition_mutex);
> > return error;
> > }
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [RFC 06/13] acpi/x86: s2idle: rename Screen On/Off to Display On/Off
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
` (4 preceding siblings ...)
2024-11-21 17:22 ` [RFC 05/13] acpi/x86: s2idle: add modern standby transition function Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-21 17:22 ` [RFC 07/13] acpi/x86: s2idle: call Display On/Off as part of callbacks Antheas Kapenekakis
` (8 subsequent siblings)
14 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, Antheas Kapenekakis
Microsoft and Intel use the term "Display" to refer to the _DSM 3,4
calls and the term "Screen" for the state ("Screen Off"). Currently,
the code uses "Screen On/Off" to name the variables, which is about
to become confusing as they become callbacks. To prepare for that,
rename the variables to "Display On/Off".
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/acpi/x86/s2idle.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index dd0b40b9bbe8..7391f87f3aa0 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;
@@ -361,10 +361,10 @@ 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:
- return "screen off";
- case ACPI_LPS0_SCREEN_ON:
- return "screen on";
+ case ACPI_LPS0_DISPLAY_OFF:
+ return "display off";
+ case ACPI_LPS0_DISPLAY_ON:
+ return "display on";
case ACPI_LPS0_ENTRY:
return "lps0 entry";
case ACPI_LPS0_EXIT:
@@ -376,10 +376,10 @@ static const char *acpi_sleep_dsm_state_to_str(unsigned int state)
}
} else {
switch (state) {
- case ACPI_LPS0_SCREEN_ON_AMD:
- return "screen on";
- case ACPI_LPS0_SCREEN_OFF_AMD:
- return "screen off";
+ case ACPI_LPS0_DISPLAY_ON_AMD:
+ return "display on";
+ case ACPI_LPS0_DISPLAY_OFF_AMD:
+ return "display off";
case ACPI_LPS0_ENTRY_AMD:
return "lps0 entry";
case ACPI_LPS0_EXIT_AMD:
@@ -552,12 +552,12 @@ int acpi_s2idle_prepare_late(void)
/* Screen 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);
/* LPS0 entry */
@@ -626,12 +626,12 @@ void acpi_s2idle_restore_early(void)
/* Screen on */
if (lps0_dsm_func_mask_microsoft > 0)
- acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON,
+ 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_SCREEN_ON_AMD :
- ACPI_LPS0_SCREEN_ON,
+ ACPI_LPS0_DISPLAY_ON_AMD :
+ ACPI_LPS0_DISPLAY_ON,
lps0_dsm_func_mask, lps0_dsm_guid);
}
--
2.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* [RFC 07/13] acpi/x86: s2idle: call Display On/Off as part of callbacks
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
` (5 preceding siblings ...)
2024-11-21 17:22 ` [RFC 06/13] acpi/x86: s2idle: rename Screen On/Off to Display On/Off Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-21 17:22 ` [RFC 08/13] acpi/x86: s2idle: rename MS Exit/Entry to Sleep Exit/Entry Antheas Kapenekakis
` (7 subsequent siblings)
14 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, 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.
Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/acpi/x86/s2idle.c | 67 +++++++++++++++++++++++++++++----------
1 file changed, 51 insertions(+), 16 deletions(-)
diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 7391f87f3aa0..8b39e3b12ec0 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_display_off;
/* Device constraint entry structure */
struct lpi_device_info {
@@ -539,17 +540,18 @@ 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 (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_DISPLAY_OFF_AMD :
@@ -560,6 +562,47 @@ int acpi_s2idle_prepare_late(void)
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 (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_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);
}
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.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* [RFC 08/13] acpi/x86: s2idle: rename MS Exit/Entry to Sleep Exit/Entry
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
` (6 preceding siblings ...)
2024-11-21 17:22 ` [RFC 07/13] acpi/x86: s2idle: call Display On/Off as part of callbacks Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-21 17:22 ` [RFC 09/13] acpi/x86: s2idle: call Sleep Entry/Exit as part of callbacks Antheas Kapenekakis
` (6 subsequent siblings)
14 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, Antheas Kapenekakis
Microsoft refers to the _DSMs 7,8 as "Sleep Entry" and "Sleep Exit".
Currently, the code uses "MS Entry/Exit" to name the variables, which is
confusing as it could either mean "Modern Standby" or "Microsoft" and is
not representative of the state. Rename as part of converting it into
a transition.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/acpi/x86/s2idle.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 8b39e3b12ec0..49dcbdea903a 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -43,8 +43,8 @@ static const struct acpi_device_id lps0_device_ids[] = {
#define ACPI_LPS0_DISPLAY_ON 4
#define ACPI_LPS0_ENTRY 5
#define ACPI_LPS0_EXIT 6
-#define ACPI_LPS0_MS_ENTRY 7
-#define ACPI_LPS0_MS_EXIT 8
+#define ACPI_LPS0_SLEEP_ENTRY 7
+#define ACPI_LPS0_SLEEP_EXIT 8
/* AMD */
#define ACPI_LPS0_DSM_UUID_AMD "e3f32452-febc-43ce-9039-932122d37721"
@@ -370,10 +370,10 @@ static const char *acpi_sleep_dsm_state_to_str(unsigned int state)
return "lps0 entry";
case ACPI_LPS0_EXIT:
return "lps0 exit";
- case ACPI_LPS0_MS_ENTRY:
- return "lps0 ms entry";
- case ACPI_LPS0_MS_EXIT:
- return "lps0 ms exit";
+ case ACPI_LPS0_SLEEP_ENTRY:
+ return "sleep entry";
+ case ACPI_LPS0_SLEEP_EXIT:
+ return "sleep exit";
}
} else {
switch (state) {
@@ -610,7 +610,7 @@ int acpi_s2idle_prepare_late(void)
if (lps0_dsm_func_mask_microsoft > 0) {
/* Modern Standby entry */
- acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_ENTRY,
+ acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_ENTRY,
lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY,
lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
@@ -663,7 +663,7 @@ void acpi_s2idle_restore_early(void)
acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT,
lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
/* Modern Standby exit */
- acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_EXIT,
+ acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_EXIT,
lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
}
}
--
2.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* [RFC 09/13] acpi/x86: s2idle: call Sleep Entry/Exit as part of callbacks
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
` (7 preceding siblings ...)
2024-11-21 17:22 ` [RFC 08/13] acpi/x86: s2idle: rename MS Exit/Entry to Sleep Exit/Entry Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-21 17:22 ` [RFC 10/13] acpi/x86: s2idle: add Turn On Display and call as part of callback Antheas Kapenekakis
` (5 subsequent siblings)
14 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, Antheas Kapenekakis
Move the Sleep Entry/Exit notifications outside the suspend sequence,
with their own ACPI lock, as was done for Display On/Off.
Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/acpi/x86/s2idle.c | 57 ++++++++++++++++++++++++++++++++-------
1 file changed, 47 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 49dcbdea903a..bdc2cc8d4994 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -61,6 +61,7 @@ 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;
+static bool lsp0_dsm_in_sleep;
/* Device constraint entry structure */
struct lpi_device_info {
@@ -567,6 +568,48 @@ static int acpi_s2idle_display_off(void)
return 0;
}
+static int acpi_s2idle_sleep_entry(void)
+{
+ if (!lps0_device_handle || sleep_no_lps0 || lps0_dsm_func_mask_microsoft <= 0)
+ return 0;
+
+ if (WARN_ON(lsp0_dsm_in_sleep))
+ return -EINVAL;
+
+ lsp0_dsm_in_sleep = true;
+ acpi_scan_lock_acquire();
+
+ /* Modern Standby Sleep Entry */
+ if (lps0_dsm_func_mask_microsoft > 0)
+ acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_ENTRY,
+ lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
+
+ acpi_scan_lock_release();
+
+ return 0;
+}
+
+static int acpi_s2idle_sleep_exit(void)
+{
+ if (!lps0_device_handle || sleep_no_lps0 || lps0_dsm_func_mask_microsoft <= 0)
+ return 0;
+
+ if (WARN_ON(!lsp0_dsm_in_sleep))
+ return -EINVAL;
+
+ lsp0_dsm_in_sleep = false;
+ acpi_scan_lock_acquire();
+
+ /* Modern Standby 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);
+
+ acpi_scan_lock_release();
+
+ return 0;
+}
+
static int acpi_s2idle_display_on(void)
{
if (!lps0_device_handle || sleep_no_lps0)
@@ -608,13 +651,9 @@ int acpi_s2idle_prepare_late(void)
acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY_AMD,
lps0_dsm_func_mask, lps0_dsm_guid);
- if (lps0_dsm_func_mask_microsoft > 0) {
- /* Modern Standby entry */
- acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_ENTRY,
- lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
+ if (lps0_dsm_func_mask_microsoft > 0)
acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY,
lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
- }
if (lps0_dsm_func_mask > 0 && !acpi_s2idle_vendor_amd())
acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY,
@@ -659,17 +698,14 @@ void acpi_s2idle_restore_early(void)
ACPI_LPS0_EXIT,
lps0_dsm_func_mask, lps0_dsm_guid);
- if (lps0_dsm_func_mask_microsoft > 0) {
+ if (lps0_dsm_func_mask_microsoft > 0)
acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT,
lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
- /* Modern Standby exit */
- acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_EXIT,
- lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
- }
}
static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = {
.display_off = acpi_s2idle_display_off,
+ .sleep_entry = acpi_s2idle_sleep_entry,
.begin = acpi_s2idle_begin,
.prepare = acpi_s2idle_prepare,
.prepare_late = acpi_s2idle_prepare_late,
@@ -678,6 +714,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,
+ .sleep_exit = acpi_s2idle_sleep_exit,
.display_on = acpi_s2idle_display_on,
};
--
2.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* [RFC 10/13] acpi/x86: s2idle: add Turn On Display and call as part of callback
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
` (8 preceding siblings ...)
2024-11-21 17:22 ` [RFC 09/13] acpi/x86: s2idle: call Sleep Entry/Exit as part of callbacks Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-21 17:22 ` [RFC 11/13] acpi/x86: s2idle: add quirk table for modern standby delays Antheas Kapenekakis
` (4 subsequent siblings)
14 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, Antheas Kapenekakis
The Turn On Display callback was introduced in Windows 22H2, to allow
devices to resume faster from sleep. Essentially, if the device lowers
its power limit (PLx) while it is in the Sleep state, this might lengthen
the suspend sequence in an undesirable manner. Implement this callback,
which corresponds to Modern Standby Firmware notification (_DSM) 9.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/acpi/x86/s2idle.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index bdc2cc8d4994..d389c57d2963 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -45,6 +45,7 @@ static const struct acpi_device_id lps0_device_ids[] = {
#define ACPI_LPS0_EXIT 6
#define ACPI_LPS0_SLEEP_ENTRY 7
#define ACPI_LPS0_SLEEP_EXIT 8
+#define ACPI_LPS0_TURN_ON_DISPLAY 9
/* AMD */
#define ACPI_LPS0_DSM_UUID_AMD "e3f32452-febc-43ce-9039-932122d37721"
@@ -375,6 +376,8 @@ static const char *acpi_sleep_dsm_state_to_str(unsigned int state)
return "sleep entry";
case ACPI_LPS0_SLEEP_EXIT:
return "sleep exit";
+ case ACPI_LPS0_TURN_ON_DISPLAY:
+ return "turn on display";
}
} else {
switch (state) {
@@ -589,6 +592,29 @@ static int acpi_s2idle_sleep_entry(void)
return 0;
}
+static int acpi_s2idle_turn_on_display(void)
+{
+ if (!lps0_device_handle || sleep_no_lps0 ||
+ lps0_dsm_func_mask_microsoft <= 0)
+ return 0;
+
+ /* This call is only valid while we are in a sleep state */
+ if (WARN_ON(!lsp0_dsm_in_sleep))
+ return -EINVAL;
+
+ acpi_scan_lock_acquire();
+
+ /* Modern Standby Turn On Display */
+ if (lps0_dsm_func_mask_microsoft > 0)
+ acpi_sleep_run_lps0_dsm(ACPI_LPS0_TURN_ON_DISPLAY,
+ lps0_dsm_func_mask_microsoft,
+ lps0_dsm_guid_microsoft);
+
+ acpi_scan_lock_release();
+
+ return 0;
+}
+
static int acpi_s2idle_sleep_exit(void)
{
if (!lps0_device_handle || sleep_no_lps0 || lps0_dsm_func_mask_microsoft <= 0)
@@ -714,6 +740,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,
+ .turn_on_display = acpi_s2idle_turn_on_display,
.sleep_exit = acpi_s2idle_sleep_exit,
.display_on = acpi_s2idle_display_on,
};
--
2.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* [RFC 11/13] acpi/x86: s2idle: add quirk table for modern standby delays
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
` (9 preceding siblings ...)
2024-11-21 17:22 ` [RFC 10/13] acpi/x86: s2idle: add Turn On Display and call as part of callback Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-21 18:04 ` Mario Limonciello
2024-11-21 17:22 ` [RFC 12/13] platform/x86: asus-wmi: remove Ally (1st gen) and Ally X suspend quirk Antheas Kapenekakis
` (3 subsequent siblings)
14 siblings, 1 reply; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, 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.
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;
+};
+
+/*
+ * 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;
--
2.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [RFC 11/13] acpi/x86: s2idle: add quirk table for modern standby delays
2024-11-21 17:22 ` [RFC 11/13] acpi/x86: s2idle: add quirk table for modern standby delays Antheas Kapenekakis
@ 2024-11-21 18:04 ` Mario Limonciello
2024-11-21 18:19 ` Antheas Kapenekakis
0 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello @ 2024-11-21 18:04 UTC (permalink / raw)
To: Antheas Kapenekakis, linux-pm
Cc: platform-driver-x86, dri-devel, Hans de Goede, Kyle Gospodnetich
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;
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [RFC 11/13] acpi/x86: s2idle: add quirk table for modern standby delays
2024-11-21 18:04 ` Mario Limonciello
@ 2024-11-21 18:19 ` Antheas Kapenekakis
0 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 18:19 UTC (permalink / raw)
To: Mario Limonciello
Cc: linux-pm, platform-driver-x86, dri-devel, Hans de Goede,
Kyle Gospodnetich
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;
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [RFC 12/13] platform/x86: asus-wmi: remove Ally (1st gen) and Ally X suspend quirk
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
` (10 preceding siblings ...)
2024-11-21 17:22 ` [RFC 11/13] acpi/x86: s2idle: add quirk table for modern standby delays Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-21 17:22 ` [RFC 13/13] PM: standby: Add sysfs attribute for modern standby transitions Antheas Kapenekakis
` (2 subsequent siblings)
14 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, 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 abdca3f05c5c..73a2ab214f56 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -142,29 +142,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;
@@ -274,9 +255,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;
@@ -4770,8 +4748,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;
@@ -4962,34 +4938,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);
@@ -5034,8 +4982,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.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* [RFC 13/13] PM: standby: Add sysfs attribute for modern standby transitions
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
` (11 preceding siblings ...)
2024-11-21 17:22 ` [RFC 12/13] platform/x86: asus-wmi: remove Ally (1st gen) and Ally X suspend quirk Antheas Kapenekakis
@ 2024-11-21 17:22 ` Antheas Kapenekakis
2024-11-28 11:45 ` Ilpo Järvinen
2024-11-21 17:41 ` [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Rafael J. Wysocki
2024-11-22 19:25 ` Xaver Hugl
14 siblings, 1 reply; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-21 17:22 UTC (permalink / raw)
To: linux-pm
Cc: platform-driver-x86, dri-devel, Mario Limonciello, Hans de Goede,
Kyle Gospodnetich, Antheas Kapenekakis
Add a sysfs attribute to allow informing the kernel about the current
standby state, those being: "active", "screen_off", "sleep", and
"resume" (to prepare turning the display on). The final modern
standby state DRIPS is omitted, as that is entered during the kernel
suspend process and userspace will never see it.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
Documentation/ABI/testing/sysfs-power | 34 ++++++++++++
kernel/power/main.c | 75 +++++++++++++++++++++++++++
2 files changed, 109 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index a3942b1036e2..eff13980cc7c 100644
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -39,6 +39,40 @@ Description:
See Documentation/admin-guide/pm/sleep-states.rst for more
information.
+What: /sys/power/standby
+Date: November 2024
+Contact: Antheas Kapenekakis <lkml@antheas.dev>
+Description:
+ The /sys/power/standby file controls the standby state of the
+ system. Modern S0ix capable systems can enter a set of low power
+ states while the kernel is still active. Transitioning into those
+ states may 1) deactivate tertiary hardware, and 2) change the
+ presentation of the device (e.g., pulse the suspend light, turn
+ off the keyboard backlight).
+
+ Available states are "active" (fully active), "screen-off" (fully
+ active but all displays of the system are off; virtual and real),
+ "sleep" (major userspace components have been frozen; light
+ background tasks may still run; this state may affect the power
+ envelope of the device). The final state is DRIPS or LSP0, where
+ the kernel suspends, and is entered by writing "mem" to
+ /sys/power/state. There is a secondary sleep state called "resume"
+ that can only be entered from "sleep" and is used in certain
+ devices to boost the Power Limit (PLx) while remaining in sleep
+ to hasten preparing for transitioning to "active".
+
+ Writing one of the above strings to this file causes the system
+ to transition into the corresponding state, by firing the
+ corresponding firmware notifications during the transition.
+
+ DRIPS or LSP0 (i.e., mem "s2idle") can only be entered from the
+ "sleep" state. If the kernel is asked to transition to DRIPS from
+ a different state, it will transition to "sleep" and then suspend.
+ On wakeup, the kernel will transition back to the previous state.
+
+ See Documentation/admin-guide/pm/standby-states.rst for more
+ information.
+
What: /sys/power/disk
Date: September 2006
Contact: Rafael J. Wysocki <rjw@rjwysocki.net>
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 6254814d4817..4377fdaf4a8d 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -748,6 +748,80 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
power_attr(state);
+#ifdef CONFIG_SUSPEND
+/*
+ * standby - control system s2idle standby state.
+ *
+ * show() returns available standby states, which may be "active", "screen_off",
+ * "sleep" and "resume" (still in sleep but preparing to turn on display).
+ * See Documentation/admin-guide/pm/standby-states.rst for a description of
+ * what they mean.
+ *
+ * store() accepts one of those strings, translates it into the proper
+ * enumerated value, and initiates a transition to that standby state.
+ *
+ * When the system suspends, it will first enter the state "sleep", suspend,
+ * and then restore the last state before entering "sleep". I.e., if userspace
+ * is not S0ix-aware, the transitions expected by Modern Standby devices will
+ * always be performed.
+ */
+static ssize_t standby_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ char *s = buf;
+ standby_state_t i;
+ standby_state_t curr = pm_standby_state();
+
+ if (curr < 0)
+ return -EBUSY;
+
+ for (i = PM_STANDBY_MIN; i < PM_STANDBY_MAX; i++)
+ if (standby_states[i])
+ s += sprintf(s, curr == i ? "[%s] " : "%s ", standby_states[i]);
+
+ if (s != buf)
+ /* convert the last space to a newline */
+ *(s - 1) = '\n';
+ return (s - buf);
+}
+
+static standby_state_t decode_standby_state(const char *buf, size_t n)
+{
+ standby_state_t state;
+ char *p;
+ int len;
+
+ p = memchr(buf, '\n', n);
+ len = p ? p - buf : n;
+
+ for (state = PM_STANDBY_MIN; state < PM_STANDBY_MAX; state++) {
+ const char *label = standby_states[state];
+
+ if (label && len == strlen(label) && !strncmp(buf, label, len))
+ return state;
+ }
+
+ return PM_STANDBY_MAX;
+}
+
+static ssize_t standby_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t n)
+{
+ int error;
+ standby_state_t state;
+
+ state = decode_standby_state(buf, n);
+
+ if (state >= PM_STANDBY_MAX)
+ return -EINVAL;
+
+ error = pm_standby_transition(state);
+ return error ? error : n;
+}
+
+power_attr(standby);
+#endif
+
#ifdef CONFIG_PM_SLEEP
/*
* The 'wakeup_count' attribute, along with the functions defined in
@@ -974,6 +1048,7 @@ static struct attribute * g[] = {
#ifdef CONFIG_SUSPEND
&mem_sleep_attr.attr,
&sync_on_suspend_attr.attr,
+ &standby_attr.attr,
#endif
#ifdef CONFIG_PM_AUTOSLEEP
&autosleep_attr.attr,
--
2.47.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [RFC 13/13] PM: standby: Add sysfs attribute for modern standby transitions
2024-11-21 17:22 ` [RFC 13/13] PM: standby: Add sysfs attribute for modern standby transitions Antheas Kapenekakis
@ 2024-11-28 11:45 ` Ilpo Järvinen
0 siblings, 0 replies; 29+ messages in thread
From: Ilpo Järvinen @ 2024-11-28 11:45 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: linux-pm, platform-driver-x86, dri-devel, Mario Limonciello,
Hans de Goede, Kyle Gospodnetich
On Thu, 21 Nov 2024, Antheas Kapenekakis wrote:
> Add a sysfs attribute to allow informing the kernel about the current
> standby state, those being: "active", "screen_off", "sleep", and
> "resume" (to prepare turning the display on). The final modern
> standby state DRIPS is omitted, as that is entered during the kernel
> suspend process and userspace will never see it.
>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
> Documentation/ABI/testing/sysfs-power | 34 ++++++++++++
> kernel/power/main.c | 75 +++++++++++++++++++++++++++
> 2 files changed, 109 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
> index a3942b1036e2..eff13980cc7c 100644
> --- a/Documentation/ABI/testing/sysfs-power
> +++ b/Documentation/ABI/testing/sysfs-power
> @@ -39,6 +39,40 @@ Description:
> See Documentation/admin-guide/pm/sleep-states.rst for more
> information.
>
> +What: /sys/power/standby
> +Date: November 2024
> +Contact: Antheas Kapenekakis <lkml@antheas.dev>
> +Description:
> + The /sys/power/standby file controls the standby state of the
> + system. Modern S0ix capable systems can enter a set of low power
> + states while the kernel is still active. Transitioning into those
> + states may 1) deactivate tertiary hardware, and 2) change the
> + presentation of the device (e.g., pulse the suspend light, turn
> + off the keyboard backlight).
> +
> + Available states are "active" (fully active), "screen-off" (fully
> + active but all displays of the system are off; virtual and real),
> + "sleep" (major userspace components have been frozen; light
> + background tasks may still run; this state may affect the power
> + envelope of the device). The final state is DRIPS or LSP0, where
> + the kernel suspends, and is entered by writing "mem" to
> + /sys/power/state. There is a secondary sleep state called "resume"
> + that can only be entered from "sleep" and is used in certain
> + devices to boost the Power Limit (PLx) while remaining in sleep
> + to hasten preparing for transitioning to "active".
> +
> + Writing one of the above strings to this file causes the system
> + to transition into the corresponding state, by firing the
> + corresponding firmware notifications during the transition.
> +
> + DRIPS or LSP0 (i.e., mem "s2idle") can only be entered from the
> + "sleep" state. If the kernel is asked to transition to DRIPS from
> + a different state, it will transition to "sleep" and then suspend.
> + On wakeup, the kernel will transition back to the previous state.
> +
> + See Documentation/admin-guide/pm/standby-states.rst for more
> + information.
> +
> What: /sys/power/disk
> Date: September 2006
> Contact: Rafael J. Wysocki <rjw@rjwysocki.net>
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 6254814d4817..4377fdaf4a8d 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -748,6 +748,80 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>
> power_attr(state);
>
> +#ifdef CONFIG_SUSPEND
> +/*
> + * standby - control system s2idle standby state.
> + *
> + * show() returns available standby states, which may be "active", "screen_off",
> + * "sleep" and "resume" (still in sleep but preparing to turn on display).
> + * See Documentation/admin-guide/pm/standby-states.rst for a description of
> + * what they mean.
> + *
> + * store() accepts one of those strings, translates it into the proper
> + * enumerated value, and initiates a transition to that standby state.
> + *
> + * When the system suspends, it will first enter the state "sleep", suspend,
> + * and then restore the last state before entering "sleep". I.e., if userspace
> + * is not S0ix-aware, the transitions expected by Modern Standby devices will
> + * always be performed.
> + */
> +static ssize_t standby_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + char *s = buf;
Instead of char *, add size_t len for the offset.
Order these to reverse xmas tree.
> + standby_state_t i;
> + standby_state_t curr = pm_standby_state();
> +
> + if (curr < 0)
> + return -EBUSY;
> +
> + for (i = PM_STANDBY_MIN; i < PM_STANDBY_MAX; i++)
> + if (standby_states[i])
> + s += sprintf(s, curr == i ? "[%s] " : "%s ", standby_states[i]);
Do not use sprintf() for anything new.
For sysfs, sysfs_emit_at() (or sysfs_emit()) is the correct function.
You could consider using reverse logic + continue to bring down the
indentation level.
> +
> + if (s != buf)
> + /* convert the last space to a newline */
> + *(s - 1) = '\n';
> + return (s - buf);
> +}
> +
> +static standby_state_t decode_standby_state(const char *buf, size_t n)
> +{
> + standby_state_t state;
> + char *p;
> + int len;
size_t
> + p = memchr(buf, '\n', n);
> + len = p ? p - buf : n;
> +
> + for (state = PM_STANDBY_MIN; state < PM_STANDBY_MAX; state++) {
> + const char *label = standby_states[state];
> +
> + if (label && len == strlen(label) && !strncmp(buf, label, len))
Isn't len == strlen(label) && !strncmp(buf, label, len) same as using just
using !strcmp() ?
> + return state;
> + }
> +
> + return PM_STANDBY_MAX;
> +}
> +
> +static ssize_t standby_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, size_t n)
> +{
> + int error;
> + standby_state_t state;
> +
> + state = decode_standby_state(buf, n);
> +
> + if (state >= PM_STANDBY_MAX)
> + return -EINVAL;
> +
> + error = pm_standby_transition(state);
> + return error ? error : n;
return error ?: n
> +}
> +
> +power_attr(standby);
> +#endif
> +
> #ifdef CONFIG_PM_SLEEP
> /*
> * The 'wakeup_count' attribute, along with the functions defined in
> @@ -974,6 +1048,7 @@ static struct attribute * g[] = {
> #ifdef CONFIG_SUSPEND
> &mem_sleep_attr.attr,
> &sync_on_suspend_attr.attr,
> + &standby_attr.attr,
> #endif
> #ifdef CONFIG_PM_AUTOSLEEP
> &autosleep_attr.attr,
>
--
i.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
` (12 preceding siblings ...)
2024-11-21 17:22 ` [RFC 13/13] PM: standby: Add sysfs attribute for modern standby transitions Antheas Kapenekakis
@ 2024-11-21 17:41 ` Rafael J. Wysocki
2024-12-06 21:37 ` Antheas Kapenekakis
2024-11-22 19:25 ` Xaver Hugl
14 siblings, 1 reply; 29+ messages in thread
From: Rafael J. Wysocki @ 2024-11-21 17:41 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: linux-pm, platform-driver-x86, dri-devel, Mario Limonciello,
Hans de Goede, Kyle Gospodnetich
On Thu, Nov 21, 2024 at 6:28 PM Antheas Kapenekakis <lkml@antheas.dev> wrote:
>
> The following series moves the _DSM 3,4,7,8 firmware notifications outside
> the suspend sequence, and makes them part of a transition function, where
> the system can transition freely between them when it is not suspended.
> This transition function is exposed to userspace, which now gains the
> ability to control the presentation of the device (e.g., pulse the suspend
> light) without forcing the kernel to suspend. In addition, it adds support
> for the _DSM 9 call Turn Display On, which was introduced in Windows 22H2
> and aims to speed up device wake-up while remaining in the "Sleep" state.
> If userspace is not standby aware, the kernel will bring the system into
> the "Sleep" state before beginning the suspend sequence.
I'll get to this when 6.13-rc1 is out, but I can tell you right away
that some of the above cannot be done without breaking backwards
compatibility.
> This series requires a bit of background on how modern standby works in
> Windows. Windows has a concept of "Modern Standby" [1], where it performs
> an elaborate userspace and kernel suspend choreography while the device is
> inactive in order to maintain fast wake-up times and connectivity while the
> display of the device is off. This is done through 5 hardware states and
> the OS takes the liberty of transitioning between them, by following a set
> of rules (e.g., "Adaptive Hibernate").
>
> ```
> \/-> "Hibernate (S4)"
> "Active" <-> "Screen Off" <-> "Sleep" <-> "DRIPS"
> /\- "Resume" <- <-
> ```
>
> When the display is on and the user is interacting with the device, it is
> in the "Active" state. The moment the display turns off, the device
> transitions to the "Screen Off" state, where hardware and userspace are
> fully active. Userspace will then decide when appropriate to freeze major
> components (such as the DE) and transition into the "Sleep" state, where
> the kernel is still active and connectivity is maintained. Finally, the
> conventional "Suspend-to-idle" path can be used to bring the system into
> the deepest runtime idle platform state (DRIPS) state, which is named
> "s2idle" in the Linux kernel.
>
> After wake-up, the system re-transitions into the "Sleep" state, where
> userspace can run housekeeping and/or hibernate if the wake-up was not user
> initiated (e.g., timer). If user-initiated, userspace can hasten the
> transition out of the "Sleep" state by transitioning into the state
> "Resume" that certain devices use to boost the Power Limit (PLx) while
> remaining in sleep (support for this new notification is rare). Then, it
> transitions back into "Screen Off" and "Active" to prepare for the user.
>
> All transitions between these states feature unique firmware notifications
> [3] that change the presentation of the device (e.g., pulse the suspend
> light, turn off RGB). For more information, see the docs in [8]. Making
> these transitions accessible from userspace moves them out of the suspend
> sequence and has them happen while the kernel is fully active, mirroring
> Windows.
>
> As a side effect, this patch series completely fixes the ROG Ally
> controller issue [5], which expects for .5s to lapse before its
> controller's USB hub goes into D3 and otherwise malfunctions. It also fixes
> an issue present in (allegedly only) older firmwares where they check the
> USB subsystem is not in D3 before allowing the controller to wake up while
> in powersave mode (for avoiding spurious wake-ups). As such, this patch
> series is also a universal fix for the ROG Ally controller.
>
> Moreover, this patch series allows turning off the controller and RGB of
> most Windows handhelds (OneXPlayer, Lenovo Legion Go, GPD, and Asus ROG
> Ally), opening the possibility of implementing suspend-then-hibernate and
> other standby features, such as background downloads, without waking up the
> RGB/controller of those devices. A Thinkpad T14 2021 was also tested, and
> it pulses its suspend light during sleep.
>
> There is still the question of where LSP0 entry/exit (_DSM 5,6) should be
> fired or whether they should be fired in the path to hibernation. However,
> as they cause no issues currently, and they fire when software activity has
> seized, they are fine where they are.
>
> It is important to note that the effects of these _DSMs persist during
> reboots. I.e., if the Legion Go reboots while in the "Sleep" state, it will
> boot into the "Sleep" state and have its controller disabled and suspend
> light pulsing. The reboot persistence is undesirable, so the reboot path
> will need to include a transition to active prior to reboot (not
> included in this series). This is not the case after shutdown and
> hibernation, where the device boots into the "Active" state.
>
> The issue of DPMS is still present. Currently, gamescope and KDE (at least)
> do not fire DPMS before suspending. This causes an undesirable frozen
> screen while the system is suspending and looks quite ugly in general. This
> is especially true if the firmware notifications fire earlier. Therefore,
> should the kernel fire DPMS before forcing the transition to sleep for
> backwards compat.? If yes, it will be quite the effort. Moreover, should
> the kernel allow graphics drivers hook the transition function and block
> transitions to "Screen Off" if there is an active CRTC? As that would be a
> significant undertaking, there should be proof that there exists such a
> device that has an issue firing the notifications with an active CRTC.
>
> A variant of this series has been tested by thousands of users by now,
> where the notifications fire around .5s before the CRTC is disabled and no
> ill-effects have found in regard to this quirk. AFAIK, it is a visual
> quirk. Making DPMS fire before the backwards compat. transition is a good
> idea in any case, as it will sync the 200ms between Display Off/Sleep Entry
> firing and the graphics driver turning off the display, but it might not be
> worth the effort.
>
> We are currently testing a DPMS patch for gamescope and it completely fixes
> this visual quirk while allowing for e.g., hibernation without turning on
> the screen. The DPMS gamescope patch + performing the transitions in
> userspace in such a way where it blends the Ally's suspend delay halves the
> user perceived delay to sleep and results in a very professional
> presentation. This presentation extends to other devices as well, such as
> the Legion Go.
>
> 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/customize/power-settings/adaptive-hibernate [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]
> File: Documentation/admin-guide/pm/standby-states.rst [8]
>
> Changes from previous series (`acpi/x86: s2idle: move Display off/on calls
> outside suspend (fixes ROG Ally suspend)`):
> - Separate Display On/Off rename into its own commit (suggested by Hans)
> - Move delay quirks into s2idle.c (suggested by Hans)
> - Add documentation on Documentation/admin-guide/pm/standby-states.rst
> - Callbacks are now static and a transition function is used
> - Fixed all checkpatch warnings
> - The rest of the series is completely re-written
>
> Antheas Kapenekakis (13):
> Documentation: PM: Add documentation for S0ix Standby States
> acpi/x86: s2idle: add support for Display Off and Display On callbacks
> acpi/x86: s2idle: add support for Sleep Entry and Sleep Exit callbacks
> acpi/x86: s2idle: add support for Turn On Display callback
> acpi/x86: s2idle: add modern standby transition function
> acpi/x86: s2idle: rename Screen On/Off to Display On/Off
> acpi/x86: s2idle: call Display On/Off as part of callbacks
> acpi/x86: s2idle: rename MS Exit/Entry to Sleep Exit/Entry
> acpi/x86: s2idle: call Sleep Entry/Exit as part of callbacks
> acpi/x86: s2idle: add Turn On Display and call as part of callback
> acpi/x86: s2idle: add quirk table for modern standby delays
> platform/x86: asus-wmi: remove Ally (1st gen) and Ally X suspend quirk
> PM: standby: Add sysfs attribute for modern standby transitions
>
> Documentation/ABI/testing/sysfs-power | 34 +++
> .../admin-guide/pm/standby-states.rst | 133 ++++++++++
> Documentation/admin-guide/pm/system-wide.rst | 1 +
> drivers/acpi/x86/s2idle.c | 249 ++++++++++++++----
> drivers/platform/x86/asus-wmi.c | 54 ----
> include/linux/suspend.h | 16 ++
> kernel/power/main.c | 75 ++++++
> kernel/power/power.h | 1 +
> kernel/power/suspend.c | 154 +++++++++++
> 9 files changed, 616 insertions(+), 101 deletions(-)
> create mode 100644 Documentation/admin-guide/pm/standby-states.rst
>
> --
> 2.47.0
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace
2024-11-21 17:41 ` [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Rafael J. Wysocki
@ 2024-12-06 21:37 ` Antheas Kapenekakis
0 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-12-06 21:37 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-pm, platform-driver-x86, dri-devel, Mario Limonciello,
Hans de Goede, Kyle Gospodnetich
Hi Rafael,
since 6.13-rc1 is out, hopefully you can have a look over the next few days
We have deployed a variant of this patchset now on desktop builds as
well for over 2 months now, and we haven't had any regressions
reported. We have also been using it on handheld builds, where for the
last 2 or so weeks we transition to the sleep state and fire the dpms
as part of the systemd sleep target, and it makes a big difference in
how devices look when suspending and hibernating.
Essentially, as soon as the suspend animation plays, the screen and
rgb of the devices turn off instantly, and the power light of devices
that have it as part of the sleep call begins to flash. Then, after a
few seconds, the fan of the devices turns off. Before, they'd show a
stale framebuffer and have the RGB be on until almost the suspend
sequence is over.
This is also true for hibernation, where before the RGB lights of the
devices would stay on during the suspend sequence and the device would
show a stale frame buffer on the screen. Now the devices look like
they are suspended while initializing hibernation and then just turn
off
Maybe I need to shoot a video with it..
So I'd love to hear your thoughts. Can you expand on what you mean by
not backwards compatible?
I know that it is not backwards compatible in the way where if the
compositor/init system are not aware of it, the display on/off
notifications will not fire automatically when the displays turn off.
Antheas
On Thu, 21 Nov 2024 at 18:41, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Nov 21, 2024 at 6:28 PM Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >
> > The following series moves the _DSM 3,4,7,8 firmware notifications outside
> > the suspend sequence, and makes them part of a transition function, where
> > the system can transition freely between them when it is not suspended.
> > This transition function is exposed to userspace, which now gains the
> > ability to control the presentation of the device (e.g., pulse the suspend
> > light) without forcing the kernel to suspend. In addition, it adds support
> > for the _DSM 9 call Turn Display On, which was introduced in Windows 22H2
> > and aims to speed up device wake-up while remaining in the "Sleep" state.
> > If userspace is not standby aware, the kernel will bring the system into
> > the "Sleep" state before beginning the suspend sequence.
>
> I'll get to this when 6.13-rc1 is out, but I can tell you right away
> that some of the above cannot be done without breaking backwards
> compatibility.
>
> > This series requires a bit of background on how modern standby works in
> > Windows. Windows has a concept of "Modern Standby" [1], where it performs
> > an elaborate userspace and kernel suspend choreography while the device is
> > inactive in order to maintain fast wake-up times and connectivity while the
> > display of the device is off. This is done through 5 hardware states and
> > the OS takes the liberty of transitioning between them, by following a set
> > of rules (e.g., "Adaptive Hibernate").
> >
> > ```
> > \/-> "Hibernate (S4)"
> > "Active" <-> "Screen Off" <-> "Sleep" <-> "DRIPS"
> > /\- "Resume" <- <-
> > ```
> >
> > When the display is on and the user is interacting with the device, it is
> > in the "Active" state. The moment the display turns off, the device
> > transitions to the "Screen Off" state, where hardware and userspace are
> > fully active. Userspace will then decide when appropriate to freeze major
> > components (such as the DE) and transition into the "Sleep" state, where
> > the kernel is still active and connectivity is maintained. Finally, the
> > conventional "Suspend-to-idle" path can be used to bring the system into
> > the deepest runtime idle platform state (DRIPS) state, which is named
> > "s2idle" in the Linux kernel.
> >
> > After wake-up, the system re-transitions into the "Sleep" state, where
> > userspace can run housekeeping and/or hibernate if the wake-up was not user
> > initiated (e.g., timer). If user-initiated, userspace can hasten the
> > transition out of the "Sleep" state by transitioning into the state
> > "Resume" that certain devices use to boost the Power Limit (PLx) while
> > remaining in sleep (support for this new notification is rare). Then, it
> > transitions back into "Screen Off" and "Active" to prepare for the user.
> >
> > All transitions between these states feature unique firmware notifications
> > [3] that change the presentation of the device (e.g., pulse the suspend
> > light, turn off RGB). For more information, see the docs in [8]. Making
> > these transitions accessible from userspace moves them out of the suspend
> > sequence and has them happen while the kernel is fully active, mirroring
> > Windows.
> >
> > As a side effect, this patch series completely fixes the ROG Ally
> > controller issue [5], which expects for .5s to lapse before its
> > controller's USB hub goes into D3 and otherwise malfunctions. It also fixes
> > an issue present in (allegedly only) older firmwares where they check the
> > USB subsystem is not in D3 before allowing the controller to wake up while
> > in powersave mode (for avoiding spurious wake-ups). As such, this patch
> > series is also a universal fix for the ROG Ally controller.
> >
> > Moreover, this patch series allows turning off the controller and RGB of
> > most Windows handhelds (OneXPlayer, Lenovo Legion Go, GPD, and Asus ROG
> > Ally), opening the possibility of implementing suspend-then-hibernate and
> > other standby features, such as background downloads, without waking up the
> > RGB/controller of those devices. A Thinkpad T14 2021 was also tested, and
> > it pulses its suspend light during sleep.
> >
> > There is still the question of where LSP0 entry/exit (_DSM 5,6) should be
> > fired or whether they should be fired in the path to hibernation. However,
> > as they cause no issues currently, and they fire when software activity has
> > seized, they are fine where they are.
> >
> > It is important to note that the effects of these _DSMs persist during
> > reboots. I.e., if the Legion Go reboots while in the "Sleep" state, it will
> > boot into the "Sleep" state and have its controller disabled and suspend
> > light pulsing. The reboot persistence is undesirable, so the reboot path
> > will need to include a transition to active prior to reboot (not
> > included in this series). This is not the case after shutdown and
> > hibernation, where the device boots into the "Active" state.
> >
> > The issue of DPMS is still present. Currently, gamescope and KDE (at least)
> > do not fire DPMS before suspending. This causes an undesirable frozen
> > screen while the system is suspending and looks quite ugly in general. This
> > is especially true if the firmware notifications fire earlier. Therefore,
> > should the kernel fire DPMS before forcing the transition to sleep for
> > backwards compat.? If yes, it will be quite the effort. Moreover, should
> > the kernel allow graphics drivers hook the transition function and block
> > transitions to "Screen Off" if there is an active CRTC? As that would be a
> > significant undertaking, there should be proof that there exists such a
> > device that has an issue firing the notifications with an active CRTC.
> >
> > A variant of this series has been tested by thousands of users by now,
> > where the notifications fire around .5s before the CRTC is disabled and no
> > ill-effects have found in regard to this quirk. AFAIK, it is a visual
> > quirk. Making DPMS fire before the backwards compat. transition is a good
> > idea in any case, as it will sync the 200ms between Display Off/Sleep Entry
> > firing and the graphics driver turning off the display, but it might not be
> > worth the effort.
> >
> > We are currently testing a DPMS patch for gamescope and it completely fixes
> > this visual quirk while allowing for e.g., hibernation without turning on
> > the screen. The DPMS gamescope patch + performing the transitions in
> > userspace in such a way where it blends the Ally's suspend delay halves the
> > user perceived delay to sleep and results in a very professional
> > presentation. This presentation extends to other devices as well, such as
> > the Legion Go.
> >
> > 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/customize/power-settings/adaptive-hibernate [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]
> > File: Documentation/admin-guide/pm/standby-states.rst [8]
> >
> > Changes from previous series (`acpi/x86: s2idle: move Display off/on calls
> > outside suspend (fixes ROG Ally suspend)`):
> > - Separate Display On/Off rename into its own commit (suggested by Hans)
> > - Move delay quirks into s2idle.c (suggested by Hans)
> > - Add documentation on Documentation/admin-guide/pm/standby-states.rst
> > - Callbacks are now static and a transition function is used
> > - Fixed all checkpatch warnings
> > - The rest of the series is completely re-written
> >
> > Antheas Kapenekakis (13):
> > Documentation: PM: Add documentation for S0ix Standby States
> > acpi/x86: s2idle: add support for Display Off and Display On callbacks
> > acpi/x86: s2idle: add support for Sleep Entry and Sleep Exit callbacks
> > acpi/x86: s2idle: add support for Turn On Display callback
> > acpi/x86: s2idle: add modern standby transition function
> > acpi/x86: s2idle: rename Screen On/Off to Display On/Off
> > acpi/x86: s2idle: call Display On/Off as part of callbacks
> > acpi/x86: s2idle: rename MS Exit/Entry to Sleep Exit/Entry
> > acpi/x86: s2idle: call Sleep Entry/Exit as part of callbacks
> > acpi/x86: s2idle: add Turn On Display and call as part of callback
> > acpi/x86: s2idle: add quirk table for modern standby delays
> > platform/x86: asus-wmi: remove Ally (1st gen) and Ally X suspend quirk
> > PM: standby: Add sysfs attribute for modern standby transitions
> >
> > Documentation/ABI/testing/sysfs-power | 34 +++
> > .../admin-guide/pm/standby-states.rst | 133 ++++++++++
> > Documentation/admin-guide/pm/system-wide.rst | 1 +
> > drivers/acpi/x86/s2idle.c | 249 ++++++++++++++----
> > drivers/platform/x86/asus-wmi.c | 54 ----
> > include/linux/suspend.h | 16 ++
> > kernel/power/main.c | 75 ++++++
> > kernel/power/power.h | 1 +
> > kernel/power/suspend.c | 154 +++++++++++
> > 9 files changed, 616 insertions(+), 101 deletions(-)
> > create mode 100644 Documentation/admin-guide/pm/standby-states.rst
> >
> > --
> > 2.47.0
> >
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace
2024-11-21 17:22 [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Antheas Kapenekakis
` (13 preceding siblings ...)
2024-11-21 17:41 ` [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace Rafael J. Wysocki
@ 2024-11-22 19:25 ` Xaver Hugl
2024-11-22 23:55 ` Antheas Kapenekakis
14 siblings, 1 reply; 29+ messages in thread
From: Xaver Hugl @ 2024-11-22 19:25 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: linux-pm, platform-driver-x86, dri-devel, Mario Limonciello,
Hans de Goede, Kyle Gospodnetich
Am Do., 21. Nov. 2024 um 18:22 Uhr schrieb Antheas Kapenekakis
<lkml@antheas.dev>:
>
> The following series moves the _DSM 3,4,7,8 firmware notifications outside
> the suspend sequence, and makes them part of a transition function, where
> the system can transition freely between them when it is not suspended.
> This transition function is exposed to userspace, which now gains the
> ability to control the presentation of the device (e.g., pulse the suspend
> light) without forcing the kernel to suspend. In addition, it adds support
> for the _DSM 9 call Turn Display On, which was introduced in Windows 22H2
> and aims to speed up device wake-up while remaining in the "Sleep" state.
> If userspace is not standby aware, the kernel will bring the system into
> the "Sleep" state before beginning the suspend sequence.
>
> This series requires a bit of background on how modern standby works in
> Windows. Windows has a concept of "Modern Standby" [1], where it performs
> an elaborate userspace and kernel suspend choreography while the device is
> inactive in order to maintain fast wake-up times and connectivity while the
> display of the device is off. This is done through 5 hardware states and
> the OS takes the liberty of transitioning between them, by following a set
> of rules (e.g., "Adaptive Hibernate").
>
> ```
> \/-> "Hibernate (S4)"
> "Active" <-> "Screen Off" <-> "Sleep" <-> "DRIPS"
> /\- "Resume" <- <-
> ```
>
> When the display is on and the user is interacting with the device, it is
> in the "Active" state. The moment the display turns off, the device
> transitions to the "Screen Off" state, where hardware and userspace are
> fully active. Userspace will then decide when appropriate to freeze major
> components (such as the DE) and transition into the "Sleep" state, where
> the kernel is still active and connectivity is maintained. Finally, the
> conventional "Suspend-to-idle" path can be used to bring the system into
> the deepest runtime idle platform state (DRIPS) state, which is named
> "s2idle" in the Linux kernel.
>
> After wake-up, the system re-transitions into the "Sleep" state, where
> userspace can run housekeeping and/or hibernate if the wake-up was not user
> initiated (e.g., timer). If user-initiated, userspace can hasten the
> transition out of the "Sleep" state by transitioning into the state
> "Resume" that certain devices use to boost the Power Limit (PLx) while
> remaining in sleep (support for this new notification is rare). Then, it
> transitions back into "Screen Off" and "Active" to prepare for the user.
>
> All transitions between these states feature unique firmware notifications
> [3] that change the presentation of the device (e.g., pulse the suspend
> light, turn off RGB). For more information, see the docs in [8]. Making
> these transitions accessible from userspace moves them out of the suspend
> sequence and has them happen while the kernel is fully active, mirroring
> Windows.
>
> As a side effect, this patch series completely fixes the ROG Ally
> controller issue [5], which expects for .5s to lapse before its
> controller's USB hub goes into D3 and otherwise malfunctions. It also fixes
> an issue present in (allegedly only) older firmwares where they check the
> USB subsystem is not in D3 before allowing the controller to wake up while
> in powersave mode (for avoiding spurious wake-ups). As such, this patch
> series is also a universal fix for the ROG Ally controller.
>
> Moreover, this patch series allows turning off the controller and RGB of
> most Windows handhelds (OneXPlayer, Lenovo Legion Go, GPD, and Asus ROG
> Ally), opening the possibility of implementing suspend-then-hibernate and
> other standby features, such as background downloads, without waking up the
> RGB/controller of those devices. A Thinkpad T14 2021 was also tested, and
> it pulses its suspend light during sleep.
>
> There is still the question of where LSP0 entry/exit (_DSM 5,6) should be
> fired or whether they should be fired in the path to hibernation. However,
> as they cause no issues currently, and they fire when software activity has
> seized, they are fine where they are.
>
> It is important to note that the effects of these _DSMs persist during
> reboots. I.e., if the Legion Go reboots while in the "Sleep" state, it will
> boot into the "Sleep" state and have its controller disabled and suspend
> light pulsing. The reboot persistence is undesirable, so the reboot path
> will need to include a transition to active prior to reboot (not
> included in this series). This is not the case after shutdown and
> hibernation, where the device boots into the "Active" state.
>
> The issue of DPMS is still present. Currently, gamescope and KDE (at least)
> do not fire DPMS before suspending. This causes an undesirable frozen
> screen while the system is suspending and looks quite ugly in general. This
> is especially true if the firmware notifications fire earlier. Therefore,
> should the kernel fire DPMS before forcing the transition to sleep for
> backwards compat.?
FWIW in KDE we already planned to turn the screen off before suspend
to deal better with spurious wakeups, and that'll be in the next
version of Plasma. I think it's fine if you just leave this up to
userspace, and maybe write to wayland-devel, so that other compositor
developers are aware they should do the same.
> If yes, it will be quite the effort. Moreover, should
> the kernel allow graphics drivers hook the transition function and block
> transitions to "Screen Off" if there is an active CRTC? As that would be a
> significant undertaking, there should be proof that there exists such a
> device that has an issue firing the notifications with an active CRTC.
>
> A variant of this series has been tested by thousands of users by now,
> where the notifications fire around .5s before the CRTC is disabled and no
> ill-effects have found in regard to this quirk. AFAIK, it is a visual
> quirk. Making DPMS fire before the backwards compat. transition is a good
> idea in any case, as it will sync the 200ms between Display Off/Sleep Entry
> firing and the graphics driver turning off the display, but it might not be
> worth the effort.
>
> We are currently testing a DPMS patch for gamescope and it completely fixes
> this visual quirk while allowing for e.g., hibernation without turning on
> the screen. The DPMS gamescope patch + performing the transitions in
> userspace in such a way where it blends the Ally's suspend delay halves the
> user perceived delay to sleep and results in a very professional
> presentation. This presentation extends to other devices as well, such as
> the Legion Go.
>
> 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/customize/power-settings/adaptive-hibernate [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]
> File: Documentation/admin-guide/pm/standby-states.rst [8]
>
> Changes from previous series (`acpi/x86: s2idle: move Display off/on calls
> outside suspend (fixes ROG Ally suspend)`):
> - Separate Display On/Off rename into its own commit (suggested by Hans)
> - Move delay quirks into s2idle.c (suggested by Hans)
> - Add documentation on Documentation/admin-guide/pm/standby-states.rst
> - Callbacks are now static and a transition function is used
> - Fixed all checkpatch warnings
> - The rest of the series is completely re-written
>
> Antheas Kapenekakis (13):
> Documentation: PM: Add documentation for S0ix Standby States
> acpi/x86: s2idle: add support for Display Off and Display On callbacks
> acpi/x86: s2idle: add support for Sleep Entry and Sleep Exit callbacks
> acpi/x86: s2idle: add support for Turn On Display callback
> acpi/x86: s2idle: add modern standby transition function
> acpi/x86: s2idle: rename Screen On/Off to Display On/Off
> acpi/x86: s2idle: call Display On/Off as part of callbacks
> acpi/x86: s2idle: rename MS Exit/Entry to Sleep Exit/Entry
> acpi/x86: s2idle: call Sleep Entry/Exit as part of callbacks
> acpi/x86: s2idle: add Turn On Display and call as part of callback
> acpi/x86: s2idle: add quirk table for modern standby delays
> platform/x86: asus-wmi: remove Ally (1st gen) and Ally X suspend quirk
> PM: standby: Add sysfs attribute for modern standby transitions
>
> Documentation/ABI/testing/sysfs-power | 34 +++
> .../admin-guide/pm/standby-states.rst | 133 ++++++++++
> Documentation/admin-guide/pm/system-wide.rst | 1 +
> drivers/acpi/x86/s2idle.c | 249 ++++++++++++++----
> drivers/platform/x86/asus-wmi.c | 54 ----
> include/linux/suspend.h | 16 ++
> kernel/power/main.c | 75 ++++++
> kernel/power/power.h | 1 +
> kernel/power/suspend.c | 154 +++++++++++
> 9 files changed, 616 insertions(+), 101 deletions(-)
> create mode 100644 Documentation/admin-guide/pm/standby-states.rst
>
> --
> 2.47.0
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [RFC 00/13] acpi/x86: s2idle: implement Modern Standby transition states and expose to userspace
2024-11-22 19:25 ` Xaver Hugl
@ 2024-11-22 23:55 ` Antheas Kapenekakis
0 siblings, 0 replies; 29+ messages in thread
From: Antheas Kapenekakis @ 2024-11-22 23:55 UTC (permalink / raw)
To: Xaver Hugl
Cc: linux-pm, platform-driver-x86, dri-devel, Mario Limonciello,
Hans de Goede, Kyle Gospodnetich
On Fri, 22 Nov 2024 at 20:26, Xaver Hugl <xaver.hugl@kde.org> wrote:
>
> Am Do., 21. Nov. 2024 um 18:22 Uhr schrieb Antheas Kapenekakis
> <lkml@antheas.dev>:
> >
> > The following series moves the _DSM 3,4,7,8 firmware notifications outside
> > the suspend sequence, and makes them part of a transition function, where
> > the system can transition freely between them when it is not suspended.
> > This transition function is exposed to userspace, which now gains the
> > ability to control the presentation of the device (e.g., pulse the suspend
> > light) without forcing the kernel to suspend. In addition, it adds support
> > for the _DSM 9 call Turn Display On, which was introduced in Windows 22H2
> > and aims to speed up device wake-up while remaining in the "Sleep" state.
> > If userspace is not standby aware, the kernel will bring the system into
> > the "Sleep" state before beginning the suspend sequence.
> >
> > This series requires a bit of background on how modern standby works in
> > Windows. Windows has a concept of "Modern Standby" [1], where it performs
> > an elaborate userspace and kernel suspend choreography while the device is
> > inactive in order to maintain fast wake-up times and connectivity while the
> > display of the device is off. This is done through 5 hardware states and
> > the OS takes the liberty of transitioning between them, by following a set
> > of rules (e.g., "Adaptive Hibernate").
> >
> > ```
> > \/-> "Hibernate (S4)"
> > "Active" <-> "Screen Off" <-> "Sleep" <-> "DRIPS"
> > /\- "Resume" <- <-
> > ```
> >
> > When the display is on and the user is interacting with the device, it is
> > in the "Active" state. The moment the display turns off, the device
> > transitions to the "Screen Off" state, where hardware and userspace are
> > fully active. Userspace will then decide when appropriate to freeze major
> > components (such as the DE) and transition into the "Sleep" state, where
> > the kernel is still active and connectivity is maintained. Finally, the
> > conventional "Suspend-to-idle" path can be used to bring the system into
> > the deepest runtime idle platform state (DRIPS) state, which is named
> > "s2idle" in the Linux kernel.
> >
> > After wake-up, the system re-transitions into the "Sleep" state, where
> > userspace can run housekeeping and/or hibernate if the wake-up was not user
> > initiated (e.g., timer). If user-initiated, userspace can hasten the
> > transition out of the "Sleep" state by transitioning into the state
> > "Resume" that certain devices use to boost the Power Limit (PLx) while
> > remaining in sleep (support for this new notification is rare). Then, it
> > transitions back into "Screen Off" and "Active" to prepare for the user.
> >
> > All transitions between these states feature unique firmware notifications
> > [3] that change the presentation of the device (e.g., pulse the suspend
> > light, turn off RGB). For more information, see the docs in [8]. Making
> > these transitions accessible from userspace moves them out of the suspend
> > sequence and has them happen while the kernel is fully active, mirroring
> > Windows.
> >
> > As a side effect, this patch series completely fixes the ROG Ally
> > controller issue [5], which expects for .5s to lapse before its
> > controller's USB hub goes into D3 and otherwise malfunctions. It also fixes
> > an issue present in (allegedly only) older firmwares where they check the
> > USB subsystem is not in D3 before allowing the controller to wake up while
> > in powersave mode (for avoiding spurious wake-ups). As such, this patch
> > series is also a universal fix for the ROG Ally controller.
> >
> > Moreover, this patch series allows turning off the controller and RGB of
> > most Windows handhelds (OneXPlayer, Lenovo Legion Go, GPD, and Asus ROG
> > Ally), opening the possibility of implementing suspend-then-hibernate and
> > other standby features, such as background downloads, without waking up the
> > RGB/controller of those devices. A Thinkpad T14 2021 was also tested, and
> > it pulses its suspend light during sleep.
> >
> > There is still the question of where LSP0 entry/exit (_DSM 5,6) should be
> > fired or whether they should be fired in the path to hibernation. However,
> > as they cause no issues currently, and they fire when software activity has
> > seized, they are fine where they are.
> >
> > It is important to note that the effects of these _DSMs persist during
> > reboots. I.e., if the Legion Go reboots while in the "Sleep" state, it will
> > boot into the "Sleep" state and have its controller disabled and suspend
> > light pulsing. The reboot persistence is undesirable, so the reboot path
> > will need to include a transition to active prior to reboot (not
> > included in this series). This is not the case after shutdown and
> > hibernation, where the device boots into the "Active" state.
> >
> > The issue of DPMS is still present. Currently, gamescope and KDE (at least)
> > do not fire DPMS before suspending. This causes an undesirable frozen
> > screen while the system is suspending and looks quite ugly in general. This
> > is especially true if the firmware notifications fire earlier. Therefore,
> > should the kernel fire DPMS before forcing the transition to sleep for
> > backwards compat.?
>
> FWIW in KDE we already planned to turn the screen off before suspend
> to deal better with spurious wakeups, and that'll be in the next
> version of Plasma. I think it's fine if you just leave this up to
> userspace, and maybe write to wayland-devel, so that other compositor
> developers are aware they should do the same.
This is really good to hear! I think that that is the proper solution,
along with instantly transitioning to screen off. This way the
keyboard backlight and the screen turn off instantly as the user
presses the power button. The fastest the kernel can do it is after
userspace freeze, which takes around .5s.
I still think we will have to do something in-kernel compat though, as
if we don't there is around 300ms between the device suspend light
starting to pulse and the drm driver turning off the display
currently. While it is a small visual regression, it is a regression
nonetheless and there are devices that depend on the USB subsystem
being up when the firmware notifications fire, so it cannot happen
after drm suspends.
We have not had any other reports whatsoever though and it is not as
if anyone has noticed this quirk.
@Mario while bisecting the hibernation issue, I also tested the Ally
with powersave on on a 6.10 stock kernel. While the controller does
wake up, setting RGB stops working after a few suspends
> > If yes, it will be quite the effort. Moreover, should
> > the kernel allow graphics drivers hook the transition function and block
> > transitions to "Screen Off" if there is an active CRTC? As that would be a
> > significant undertaking, there should be proof that there exists such a
> > device that has an issue firing the notifications with an active CRTC.
> >
> > A variant of this series has been tested by thousands of users by now,
> > where the notifications fire around .5s before the CRTC is disabled and no
> > ill-effects have found in regard to this quirk. AFAIK, it is a visual
> > quirk. Making DPMS fire before the backwards compat. transition is a good
> > idea in any case, as it will sync the 200ms between Display Off/Sleep Entry
> > firing and the graphics driver turning off the display, but it might not be
> > worth the effort.
> >
> > We are currently testing a DPMS patch for gamescope and it completely fixes
> > this visual quirk while allowing for e.g., hibernation without turning on
> > the screen. The DPMS gamescope patch + performing the transitions in
> > userspace in such a way where it blends the Ally's suspend delay halves the
> > user perceived delay to sleep and results in a very professional
> > presentation. This presentation extends to other devices as well, such as
> > the Legion Go.
> >
> > 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/customize/power-settings/adaptive-hibernate [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]
> > File: Documentation/admin-guide/pm/standby-states.rst [8]
> >
> > Changes from previous series (`acpi/x86: s2idle: move Display off/on calls
> > outside suspend (fixes ROG Ally suspend)`):
> > - Separate Display On/Off rename into its own commit (suggested by Hans)
> > - Move delay quirks into s2idle.c (suggested by Hans)
> > - Add documentation on Documentation/admin-guide/pm/standby-states.rst
> > - Callbacks are now static and a transition function is used
> > - Fixed all checkpatch warnings
> > - The rest of the series is completely re-written
> >
> > Antheas Kapenekakis (13):
> > Documentation: PM: Add documentation for S0ix Standby States
> > acpi/x86: s2idle: add support for Display Off and Display On callbacks
> > acpi/x86: s2idle: add support for Sleep Entry and Sleep Exit callbacks
> > acpi/x86: s2idle: add support for Turn On Display callback
> > acpi/x86: s2idle: add modern standby transition function
> > acpi/x86: s2idle: rename Screen On/Off to Display On/Off
> > acpi/x86: s2idle: call Display On/Off as part of callbacks
> > acpi/x86: s2idle: rename MS Exit/Entry to Sleep Exit/Entry
> > acpi/x86: s2idle: call Sleep Entry/Exit as part of callbacks
> > acpi/x86: s2idle: add Turn On Display and call as part of callback
> > acpi/x86: s2idle: add quirk table for modern standby delays
> > platform/x86: asus-wmi: remove Ally (1st gen) and Ally X suspend quirk
> > PM: standby: Add sysfs attribute for modern standby transitions
> >
> > Documentation/ABI/testing/sysfs-power | 34 +++
> > .../admin-guide/pm/standby-states.rst | 133 ++++++++++
> > Documentation/admin-guide/pm/system-wide.rst | 1 +
> > drivers/acpi/x86/s2idle.c | 249 ++++++++++++++----
> > drivers/platform/x86/asus-wmi.c | 54 ----
> > include/linux/suspend.h | 16 ++
> > kernel/power/main.c | 75 ++++++
> > kernel/power/power.h | 1 +
> > kernel/power/suspend.c | 154 +++++++++++
> > 9 files changed, 616 insertions(+), 101 deletions(-)
> > create mode 100644 Documentation/admin-guide/pm/standby-states.rst
> >
> > --
> > 2.47.0
> >
> >
^ permalink raw reply [flat|nested] 29+ messages in thread