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

[Alex Deucher]

On Thu, Sep 19, 2024 at 1:35 PM Mario Limonciello
<mario.limonciello at 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 at 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:
>