Re: [PATCH v2 2/5] acpi/x86: s2idle: handle Display On/Off calls outside of suspend sequence

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

On 9/22/2024 12:22, Antheas Kapenekakis wrote:
> Currently, the Display On/Off calls are handled within the suspend
> sequence, which is a deviation from Windows. This causes issues with
> certain devices, where the notification interacts with a USB device
> that expects the kernel to be fully awake.
> 
> This patch calls the Display On/Off 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.
> 
> Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>   kernel/power/suspend.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index c527dc0ae5ae..610f8ecaeebd 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -589,6 +589,13 @@ static int enter_state(suspend_state_t state)
>   	if (state == PM_SUSPEND_TO_IDLE)
>   		s2idle_begin();
>   
> +	/*
> +	 * Linux does not have the concept of a "Screen Off" state, so call
> +	 * the platform functions for Display On/Off prior to the suspend
> +	 * sequence, mirroring Windows which calls them outside of it as well.
> +	 */
> +	platform_suspend_display_off();
> +

I thought about it some more over the weekend and if moving the screen 
on/off _DSM at all I don't feel this is the right place for triggering it.

IMO it should be called by DRM core.  That is when the number of CRTCs 
active goes 1+ -> 0 call screen off and when it goes 0->1+ call screen on.

There could be an argument made only for eDP this happens, but I think 
that's a slippery slope if you end up with an AIO design that uses DP 
instead of eDP or a desktop for example.  So the safest policy should be 
across all CRTCs of all GPUs.  During the suspend sequence any that were 
left on will get turned off, and then it could be triggered at that time 
instead.

By making such a change then when the compositor turns off all displays 
at runtime you can potentially support dark screen background downloads 
that have specifically called this _DSM and any actions that are taken 
from doing so.

>   	if (sync_on_suspend_enabled) {
>   		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
>   		ksys_sync_helper();
> @@ -616,6 +623,8 @@ static int enter_state(suspend_state_t state)
>   	suspend_finish();
>    Unlock:
>   	mutex_unlock(&system_transition_mutex);
> +
> +	platform_suspend_display_on();
>   	return error;
>   }
>