| Message ID | 20240919171952.403745-3-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | acpi/x86: s2idle: move Display off/on calls outside suspend (fixes ROG Ally suspend) | expand |
+dri-devel For those joining late; this is the full series for context. https://lore.kernel.org/linux-pm/20240919171952.403745-1-lkml@antheas.dev/T/#maee308be5349d8df25c8ccf12144ea96bbd4cbbd On 9/19/2024 12:19, Antheas Kapenekakis wrote: > Currently, the screen off/on calls are handled within the suspend > sequence, which is a deviation from Windows. This causes issues with > certain devices, such as the ROG Ally, which expects this call to be > executed with the kernel fully awake. The subsequent half-suspended > state makes the controller of the device to fail to suspend properly. > > This patch calls the screen off/on callbacks before entering the suspend > sequence, which fixes this issue. In addition, it opens the possibility > of modelling a state such as "Screen Off" that mirrors Windows, as the > callbacks will be accessible and validated to work outside of the > suspend sequence. > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- > kernel/power/suspend.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c > index 19734b297527..afa95271ef00 100644 > --- a/kernel/power/suspend.c > +++ b/kernel/power/suspend.c > @@ -507,6 +507,19 @@ int suspend_devices_and_enter(suspend_state_t state) > > pm_suspend_target_state = state; > > + /* > + * Linux does not have the concept of a "Screen Off" state, so call > + * the platform functions for screen off prior to beginning the suspend > + * sequence, mirroring Windows which calls them outside of it as well. > + * > + * If Linux ever gains a "Screen Off" state, the following callbacks can > + * be replaced with a call that checks if we are in "Screen Off", in which > + * case they will NOOP and if not call them as a fallback. > + */ > + error = platform_suspend_screen_off(); It's a bit muddy; but I wonder if calling drm_atomic_helper_disable_all() makes sense here. > + if (error) > + goto Screen_on; > + > if (state == PM_SUSPEND_TO_IDLE) > pm_set_suspend_no_platform(); > > @@ -540,6 +553,9 @@ int suspend_devices_and_enter(suspend_state_t state) > Close: > platform_resume_end(state); > pm_suspend_target_state = PM_SUSPEND_ON; > + > + Screen_on: > + platform_suspend_screen_on(); The problem with my suggestion above is what would you put here for symmetry? drm_atomic_helper_resume() doesn't look right to me. Maybe it's a no-op from DRM perspective and the drivers handle it. > return error; > > Recover_platform:
On Thu, Sep 19, 2024 at 1:35 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > +dri-devel > > For those joining late; this is the full series for context. > > https://lore.kernel.org/linux-pm/20240919171952.403745-1-lkml@antheas.dev/T/#maee308be5349d8df25c8ccf12144ea96bbd4cbbd > > On 9/19/2024 12:19, Antheas Kapenekakis wrote: > > Currently, the screen off/on calls are handled within the suspend > > sequence, which is a deviation from Windows. This causes issues with > > certain devices, such as the ROG Ally, which expects this call to be > > executed with the kernel fully awake. The subsequent half-suspended > > state makes the controller of the device to fail to suspend properly. > > > > This patch calls the screen off/on callbacks before entering the suspend > > sequence, which fixes this issue. In addition, it opens the possibility > > of modelling a state such as "Screen Off" that mirrors Windows, as the > > callbacks will be accessible and validated to work outside of the > > suspend sequence. > > > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > > --- > > kernel/power/suspend.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c > > index 19734b297527..afa95271ef00 100644 > > --- a/kernel/power/suspend.c > > +++ b/kernel/power/suspend.c > > @@ -507,6 +507,19 @@ int suspend_devices_and_enter(suspend_state_t state) > > > > pm_suspend_target_state = state; > > > > + /* > > + * Linux does not have the concept of a "Screen Off" state, so call > > + * the platform functions for screen off prior to beginning the suspend > > + * sequence, mirroring Windows which calls them outside of it as well. > > + * > > + * If Linux ever gains a "Screen Off" state, the following callbacks can > > + * be replaced with a call that checks if we are in "Screen Off", in which > > + * case they will NOOP and if not call them as a fallback. > > + */ > > + error = platform_suspend_screen_off(); > > It's a bit muddy; but I wonder if calling > drm_atomic_helper_disable_all() makes sense here. I think we either want to call this after devices have suspended or it's something the drm drivers would call themselves once they have turned off the displays as part of their suspend handling. > > > + if (error) > > + goto Screen_on; > > + > > if (state == PM_SUSPEND_TO_IDLE) > > pm_set_suspend_no_platform(); > > > > @@ -540,6 +553,9 @@ int suspend_devices_and_enter(suspend_state_t state) > > Close: > > platform_resume_end(state); > > pm_suspend_target_state = PM_SUSPEND_ON; > > + > > + Screen_on: > > + platform_suspend_screen_on(); > > The problem with my suggestion above is what would you put here for > symmetry? drm_atomic_helper_resume() doesn't look right to me. > > Maybe it's a no-op from DRM perspective and the drivers handle it. if suspend is aborted, this should be called after devices resume or from the relevant drm drivers. The question is whether platforms with multiple GPUs care whether all GPUs have their displays off or if just the integrated GPU matters. Maybe after all PCI display class devices have suspended? Alex > > > return error; > > > > Recover_platform: >
On 9/19/2024 13:21, Alex Deucher wrote: > On Thu, Sep 19, 2024 at 1:35 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: >> >> +dri-devel >> >> For those joining late; this is the full series for context. >> >> https://lore.kernel.org/linux-pm/20240919171952.403745-1-lkml@antheas.dev/T/#maee308be5349d8df25c8ccf12144ea96bbd4cbbd >> >> On 9/19/2024 12:19, Antheas Kapenekakis wrote: >>> Currently, the screen off/on calls are handled within the suspend >>> sequence, which is a deviation from Windows. This causes issues with >>> certain devices, such as the ROG Ally, which expects this call to be >>> executed with the kernel fully awake. The subsequent half-suspended >>> state makes the controller of the device to fail to suspend properly. >>> >>> This patch calls the screen off/on callbacks before entering the suspend >>> sequence, which fixes this issue. In addition, it opens the possibility >>> of modelling a state such as "Screen Off" that mirrors Windows, as the >>> callbacks will be accessible and validated to work outside of the >>> suspend sequence. >>> >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> >>> --- >>> kernel/power/suspend.c | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c >>> index 19734b297527..afa95271ef00 100644 >>> --- a/kernel/power/suspend.c >>> +++ b/kernel/power/suspend.c >>> @@ -507,6 +507,19 @@ int suspend_devices_and_enter(suspend_state_t state) >>> >>> pm_suspend_target_state = state; >>> >>> + /* >>> + * Linux does not have the concept of a "Screen Off" state, so call >>> + * the platform functions for screen off prior to beginning the suspend >>> + * sequence, mirroring Windows which calls them outside of it as well. >>> + * >>> + * If Linux ever gains a "Screen Off" state, the following callbacks can >>> + * be replaced with a call that checks if we are in "Screen Off", in which >>> + * case they will NOOP and if not call them as a fallback. >>> + */ >>> + error = platform_suspend_screen_off(); >> >> It's a bit muddy; but I wonder if calling >> drm_atomic_helper_disable_all() makes sense here. > > I think we either want to call this after devices have suspended or > it's something the drm drivers would call themselves once they have > turned off the displays as part of their suspend handling. Well the DRM devices do call drm_atomic_helper_disable_all() already as part of the sequence. I had what I thought was a more logical way to approach this done in this PoC: https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git/commit/?h=superm1/dsm-screen-on-off&id=db65ddd1c745108e0ad42fe613f5d2c1146ed3d9 But from the testing Luke and Antheas have done this doesn't work or change things. It's still "too late" in the sequence. > >> >>> + if (error) >>> + goto Screen_on; >>> + >>> if (state == PM_SUSPEND_TO_IDLE) >>> pm_set_suspend_no_platform(); >>> >>> @@ -540,6 +553,9 @@ int suspend_devices_and_enter(suspend_state_t state) >>> Close: >>> platform_resume_end(state); >>> pm_suspend_target_state = PM_SUSPEND_ON; >>> + >>> + Screen_on: >>> + platform_suspend_screen_on(); >> >> The problem with my suggestion above is what would you put here for >> symmetry? drm_atomic_helper_resume() doesn't look right to me. >> >> Maybe it's a no-op from DRM perspective and the drivers handle it. > > if suspend is aborted, this should be called after devices resume or > from the relevant drm drivers. > > The question is whether platforms with multiple GPUs care whether all > GPUs have their displays off or if just the integrated GPU matters. > Maybe after all PCI display class devices have suspended? From what Luke has told me about this problem unfortunately there's a nuanced "ordering problem" with a difference between Windows and Linux at play here. Asus does $STUFF in this _DSM screen off callback with assumptions about what happens next in the Windows suspend sequence. Luke can talk more about these assumptions; he's studied it in HEAVY detail. Before this series the order of events on the way down was like this: (devices suspend) (LPS0 screen off) (LPS0 modern standby) (LPS0 low power) On resume it was: (LPS0 exit low power) (LPS0 exit modern standby) (LPS0 screen on) (devices resume) What this series is "aiming" to do is move the LPS0 screen off and on to the very beginning of the suspend and screen on to the very end of resume. The way it's done right now; it's got nothing to actually do with DRM. It would just be so much more logical to me if we really tied a _DSM screen off call with screens being off...
Hi, as noted in the cover letter, I think the connection to DRM is a red herring. The Display off callback in Windows is always called 5 seconds after the screen turns off due to inactivity or instantly if the user presses the power button. Likewise, Display on is called when the display is about to turn on. In fact, pressing the powerbutton in Windows no longer suspends the device. It just locks the session and turns off the screen. Then Windows can do whatever it wants for however long it wants before suspending the device. It took me 2 days to realise this, as my dev unit was updating and it stayed happily on for 10-20 minutes after pressing the power button when connected to a charger. I think "Screen Off" is what Microsoft decided to call their background wakelock state (as it is called in Android). That means that if there is ever a laptop with an Always On display, it will be on the "Screen Off" state while the screen is on. Antheas
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c index 19734b297527..afa95271ef00 100644 --- a/kernel/power/suspend.c +++ b/kernel/power/suspend.c @@ -507,6 +507,19 @@ int suspend_devices_and_enter(suspend_state_t state) pm_suspend_target_state = state; + /* + * Linux does not have the concept of a "Screen Off" state, so call + * the platform functions for screen off prior to beginning the suspend + * sequence, mirroring Windows which calls them outside of it as well. + * + * If Linux ever gains a "Screen Off" state, the following callbacks can + * be replaced with a call that checks if we are in "Screen Off", in which + * case they will NOOP and if not call them as a fallback. + */ + error = platform_suspend_screen_off(); + if (error) + goto Screen_on; + if (state == PM_SUSPEND_TO_IDLE) pm_set_suspend_no_platform(); @@ -540,6 +553,9 @@ int suspend_devices_and_enter(suspend_state_t state) Close: platform_resume_end(state); pm_suspend_target_state = PM_SUSPEND_ON; + + Screen_on: + platform_suspend_screen_on(); return error; Recover_platform:
Currently, the screen off/on calls are handled within the suspend sequence, which is a deviation from Windows. This causes issues with certain devices, such as the ROG Ally, which expects this call to be executed with the kernel fully awake. The subsequent half-suspended state makes the controller of the device to fail to suspend properly. This patch calls the screen off/on callbacks before entering the suspend sequence, which fixes this issue. In addition, it opens the possibility of modelling a state such as "Screen Off" that mirrors Windows, as the callbacks will be accessible and validated to work outside of the suspend sequence. Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> --- kernel/power/suspend.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)