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

Antheas Kapenekakis
2024-09-23T16:15:28Z

Does DRM core handle virtual displays like VNC?

As mentioned in the cover letter, the _DSM specifies both virtual and
actual displays.

Also, Windows behavior is like a lockscreen. 5 seconds after screen
turns off after inactivity, instantly when you press the power button.

I tend towards making a sysfs entry. Not sure.

Antheas

On Mon, 23 Sept 2024 at 18:03, Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> 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;
> > }
> >
>