On 9/19/2024 13:21, Alex Deucher wrote:
On Thu, Sep 19, 2024 at 1:35 PM Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
+dri-devel
For those joining late; this is the full series for context.
https://lore.kernel.org/linux-pm/20240919171952.403745-1-lkml@xxxxxxxxxxx/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@xxxxxxxxxxx>
---
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...
