Re: [PATCH v1 1/4] acpi/x86: s2idle: add support for screen off and screen on callbacks

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

On 9/19/2024 12:36, Antheas Kapenekakis wrote:
> As stated in the cover letter, I would like to add you as a co-author.
> Just did not (could not?) do it before asking.
> 
> I will do it on the next revision. Just tell me which patches you think
> it should be on.

Patch 1 and 3.

Thanks,

> 
> (hit reply instead of reply all by mistake, so you have this email twice now)
> 
> Best,
> Antheas
> 
> On Thu, 19 Sept 2024 at 19:29, Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 9/19/2024 12:19, Antheas Kapenekakis wrote:
>>> The screen off and screen on firmware functions are meant to signify
>>> the system entering a state where the user is not actively interacting
>>> with it (i.e., in Windows this state is called "Screen Off" and the
>>> system enters it once it turns the screen off e.g., due to inactivity).
>>>
>>> In this state, the kernel and userspace are fully active, and the user
>>> might still be interacting with the system somehow (such as with
>>> listening to music or having a hotspot). Userspace is supposed to
>>> minimize non-essential activities, but this is not required.
>>> In addition, there is no requirement of suspending post the screen off
>>> call. If the user interacts with the system, the kernel should call
>>> screen on and resume normal operation.
>>>
>>> This patch adds a set of callbacks to allow calling the screen on/off
>>> callbacks outside of the suspend/resume path. It is based on
>>> Mario Limonciello's patch on the superm1/dsm-screen-on-off branch.
>>
>> Based on?  It's nearly an identical patch [1].  The screen_off/screen_on
>> lines in struct platform_s2idle_ops are just placed in a different location.
>>
>> IMO there should be more attribution here, either a Co-developed-by tag
>> or sending my patch directly and adding your S-o-b to it.
>>
>> Link:
>> https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git/commit/?h=superm1/dsm-screen-on-off&id=7b80581428315f973410dccf0402a86266fb0d9a
>> [1]
>>
>>> However, the intent here is completely different.
>>>
>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>> ---
>>>    include/linux/suspend.h |  5 +++++
>>>    kernel/power/suspend.c  | 12 ++++++++++++
>>>    2 files changed, 17 insertions(+)
>>>
>>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>>> index da6ebca3ff77..96ceaad07839 100644
>>> --- a/include/linux/suspend.h
>>> +++ b/include/linux/suspend.h
>>> @@ -132,6 +132,7 @@ struct platform_suspend_ops {
>>>    };
>>>
>>>    struct platform_s2idle_ops {
>>> +     int (*screen_off)(void);
>>>        int (*begin)(void);
>>>        int (*prepare)(void);
>>>        int (*prepare_late)(void);
>>> @@ -140,6 +141,7 @@ struct platform_s2idle_ops {
>>>        void (*restore_early)(void);
>>>        void (*restore)(void);
>>>        void (*end)(void);
>>> +     int (*screen_on)(void);
>>>    };
>>>
>>>    #ifdef CONFIG_SUSPEND
>>> @@ -160,6 +162,9 @@ extern unsigned int pm_suspend_global_flags;
>>>    #define PM_SUSPEND_FLAG_FW_RESUME   BIT(1)
>>>    #define PM_SUSPEND_FLAG_NO_PLATFORM BIT(2)
>>>
>>> +int platform_suspend_screen_off(void);
>>> +int platform_suspend_screen_on(void);
>>> +
>>>    static inline void pm_suspend_clear_flags(void)
>>>    {
>>>        pm_suspend_global_flags = 0;
>>> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
>>> index 09f8397bae15..19734b297527 100644
>>> --- a/kernel/power/suspend.c
>>> +++ b/kernel/power/suspend.c
>>> @@ -254,6 +254,18 @@ static bool sleep_state_supported(suspend_state_t state)
>>>               (valid_state(state) && !cxl_mem_active());
>>>    }
>>>
>>> +int platform_suspend_screen_off(void)
>>> +{
>>> +     return s2idle_ops && s2idle_ops->screen_off ? s2idle_ops->screen_off() : 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(platform_suspend_screen_off);
>>> +
>>> +int platform_suspend_screen_on(void)
>>> +{
>>> +     return s2idle_ops && s2idle_ops->screen_on ? s2idle_ops->screen_on() : 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(platform_suspend_screen_on);
>>> +
>>>    static int platform_suspend_prepare(suspend_state_t state)
>>>    {
>>>        return state != PM_SUSPEND_TO_IDLE && suspend_ops->prepare ?
>>