Re: [PATCH v1 2/4] acpi/x86: s2idle: handle screen off/on calls outside of suspend sequence

[Mario Limonciello <mario.limonciello@amd.com>]

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...