Re: [RFC PATCH v1 1/1] ACPI: PM: s2idle: Add lps0_screen_off sysfs interface

["Mario Limonciello (AMD) (kernel.org)" <superm1@kernel.org>]

++ dri-devel

On 12/1/2025 10:34 PM, Dmitry Osipenko wrote:
> Add `/sys/power/lps0_screen_off` interface to allow userspace to control
> Display OFF/ON DSM notifications at runtime. Writing "1" to this file
> triggers the OFF notification, and "0" triggers the ON notification.
> 
> Userspace should write "1" after turning off all physical and remote
> displays. It should write "0" before turning on any of displays.

As this has to do with actions when displays are off I think you should 
also CC dri-devel on the next submission.

> 
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>   Documentation/ABI/testing/sysfs-power |  13 +++
>   drivers/acpi/x86/s2idle.c             | 149 +++++++++++++++++++++++---
>   2 files changed, 145 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
> index d38da077905a..af7c81ae517c 100644
> --- a/Documentation/ABI/testing/sysfs-power
> +++ b/Documentation/ABI/testing/sysfs-power
> @@ -470,3 +470,16 @@ Description:
>   
>                   Minimum value: 1
>                   Default value: 3
> +
> +What:		/sys/power/lps0_screen_off
> +Date:		November 2025

This should be matching the kernel you're targeting this to land, which 
is definitely not in the past.

> +Contact:	Dmitrii Osipenko <dmitry.osipenko@collabora.com>
> +Description:
> +		This file is available if the ACPI/OSPM system supports
> +		Display Off/On DSM notifications. It controls state of the
> +		notification.
> +
> +		Writing a "1" to this file invokes Display Off Notification.
> +		Writing a "0" to this file invokes Display On Notification.
> +
> +		Notifications are only triggered on state transitions.
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index 6d4d06236f61..d5cb5e22431d 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -18,7 +18,10 @@
>   #include <linux/acpi.h>
>   #include <linux/device.h>
>   #include <linux/dmi.h>
> +#include <linux/kobject.h>
> +#include <linux/mutex.h>
>   #include <linux/suspend.h>
> +#include <linux/sysfs.h>
>   
>   #include "../sleep.h"
>   
> @@ -61,6 +64,11 @@ static guid_t lps0_dsm_guid_microsoft;
>   static int lps0_dsm_func_mask_microsoft;
>   static int lps0_dsm_state;
>   
> +static DEFINE_MUTEX(lps0_dsm_screen_off_lock);
> +static bool lps0_dsm_screen_state_off;
> +static bool lps0_screen_off_suspended;
> +static bool lps0_screen_off_sysfs_inhibit;
> +
>   /* Device constraint entry structure */
>   struct lpi_device_info {
>   	char *name;
> @@ -513,6 +521,76 @@ static struct acpi_scan_handler lps0_handler = {
>   	.attach = lps0_device_attach,
>   };
>   
> +static bool lps0_has_screen_off_dsm(void)
> +{
> +	int id = acpi_s2idle_vendor_amd() ?
> +		 ACPI_LPS0_SCREEN_ON_AMD : ACPI_LPS0_SCREEN_OFF;
> +
> +	if (lps0_dsm_func_mask_microsoft > 0 &&
> +	    (lps0_dsm_func_mask & BIT(ACPI_LPS0_SCREEN_OFF)))
> +		return true;
> +
> +	if (lps0_dsm_func_mask > 0 && (lps0_dsm_func_mask & BIT(id)))
> +		return true;
> +
> +	return false;
> +}
> +
> +static void lps0_dsm_screen_off(void)
> +{
> +	if (lps0_dsm_screen_state_off)
> +		return;
> +
> +	if (lps0_dsm_func_mask > 0)
> +		acpi_sleep_run_lps0_dsm(acpi_s2idle_vendor_amd() ?
> +					ACPI_LPS0_SCREEN_OFF_AMD :
> +					ACPI_LPS0_SCREEN_OFF,
> +					lps0_dsm_func_mask, lps0_dsm_guid);
> +
> +	if (lps0_dsm_func_mask_microsoft > 0)
> +		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF,
> +					lps0_dsm_func_mask_microsoft,
> +					lps0_dsm_guid_microsoft);
> +
> +	lps0_dsm_screen_state_off = true;
> +}
> +
> +static void lps0_dsm_screen_on(void)
> +{
> +	if (!lps0_dsm_screen_state_off)
> +		return;
> +
> +	if (lps0_dsm_func_mask_microsoft > 0)
> +		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON,
> +					lps0_dsm_func_mask_microsoft,
> +					lps0_dsm_guid_microsoft);
> +
> +	if (lps0_dsm_func_mask > 0)
> +		acpi_sleep_run_lps0_dsm(acpi_s2idle_vendor_amd() ?
> +					ACPI_LPS0_SCREEN_ON_AMD :
> +					ACPI_LPS0_SCREEN_ON,
> +					lps0_dsm_func_mask, lps0_dsm_guid);
> +
> +	lps0_dsm_screen_state_off = false;
> +}
> +
> +static void lps0_dsm_screen_off_set(int sysfs_off, int suspended)

I don't really like passing in arbitrary integers, it makes this code 
hard to follow.

Could you perhaps use an enum instead?

> +{
> +	mutex_lock(&lps0_dsm_screen_off_lock);
> +

I'd use a guard(mutex) here instead.

> +	if (sysfs_off > -1)
> +		lps0_screen_off_sysfs_inhibit = sysfs_off;
> +	if (suspended > -1)
> +		lps0_screen_off_suspended = suspended;
> +
> +	if (lps0_screen_off_suspended || lps0_screen_off_sysfs_inhibit)
> +		lps0_dsm_screen_off();
> +	else
> +		lps0_dsm_screen_on();
> +
> +	mutex_unlock(&lps0_dsm_screen_off_lock);
> +}
> +
>   static int acpi_s2idle_begin_lps0(void)
>   {
>   	if (pm_debug_messages_on && !lpi_constraints_table) {
> @@ -543,15 +621,7 @@ static int acpi_s2idle_prepare_late_lps0(void)
>   		lpi_check_constraints();
>   
>   	/* Screen off */
> -	if (lps0_dsm_func_mask > 0)
> -		acpi_sleep_run_lps0_dsm(acpi_s2idle_vendor_amd() ?
> -					ACPI_LPS0_SCREEN_OFF_AMD :
> -					ACPI_LPS0_SCREEN_OFF,
> -					lps0_dsm_func_mask, lps0_dsm_guid);
> -
> -	if (lps0_dsm_func_mask_microsoft > 0)
> -		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF,
> -				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
> +	lps0_dsm_screen_off_set(-1, true);
>   
>   	/* LPS0 entry */
>   	if (lps0_dsm_func_mask > 0 && acpi_s2idle_vendor_amd())
> @@ -618,14 +688,7 @@ static void acpi_s2idle_restore_early_lps0(void)
>   	}
>   
>   	/* Screen on */
> -	if (lps0_dsm_func_mask_microsoft > 0)
> -		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON,
> -				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
> -	if (lps0_dsm_func_mask > 0)
> -		acpi_sleep_run_lps0_dsm(acpi_s2idle_vendor_amd() ?
> -					ACPI_LPS0_SCREEN_ON_AMD :
> -					ACPI_LPS0_SCREEN_ON,
> -					lps0_dsm_func_mask, lps0_dsm_guid);
> +	lps0_dsm_screen_off_set(-1, false);
>   }
>   
>   static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = {
> @@ -673,4 +736,56 @@ void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg)
>   }
>   EXPORT_SYMBOL_GPL(acpi_unregister_lps0_dev);
>   
> +static ssize_t lps0_screen_off_store(struct kobject *kobj,
> +				     struct kobj_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 10, &val))
> +		return -EINVAL;
> +
> +	if (val > 1)
> +		return -EINVAL;

Based upon how you are using this maybe just use kstrtobool() instead.

> +
> +	lps0_dsm_screen_off_set(val, -1);
> +
> +	return count;
> +}
> +
> +static ssize_t lps0_screen_off_show(struct kobject *kobj,
> +				    struct kobj_attribute *attr,
> +				    char *buf)
> +{
> +	return sprintf(buf, "%d\n", lps0_screen_off_sysfs_inhibit);

You should use sysfs_emit().

> +}
> +
> +static struct kobj_attribute lps0_screen_off_attr =
> +	__ATTR(lps0_screen_off, 0644,
> +	       lps0_screen_off_show, lps0_screen_off_store);
> +
> +static struct attribute *lps0_screen_off_attrs[] = {
> +	&lps0_screen_off_attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group lps0_screen_off_attr_group = {
> +	.attrs = lps0_screen_off_attrs,
> +};
> +
> +static int lps0_dsm_screen_off_init(void)
> +{
> +	int ret;
> +
> +	if (!lps0_has_screen_off_dsm())
> +		return 0;
> +
> +	ret = sysfs_create_group(power_kobj, &lps0_screen_off_attr_group);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +late_initcall(lps0_dsm_screen_off_init);
> +
>   #endif /* CONFIG_SUSPEND */