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

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 12/2/25 07:43, Mario Limonciello (AMD) (kernel.org) wrote:
> ++ 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 */

Thanks a lot for the quick code review. All the comments sound good,
will address in v2.

-- 
Best regards,
Dmitry