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 <[email protected]> >> --- >> 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 <[email protected]> >> +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
