| Message ID | 20240919171952.403745-2-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | acpi/x86: s2idle: move Display off/on calls outside suspend (fixes ROG Ally suspend) | expand |
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 ?
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. (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 ? >
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 ? >>
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 ?
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. 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(+)