| Message ID | 20220818184302.10051-10-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/kms: Stop registering multiple /sys/class/backlight devs for a single display | expand |
On 8/18/22 1:42 PM, Hans de Goede wrote: > On x86/ACPI boards the acpi_video driver will usually initialize before > the kms driver (except i915). This causes /sys/class/backlight/acpi_video0 > to show up and then the kms driver registers its own native backlight > device after which the drivers/acpi/video_detect.c code unregisters > the acpi_video0 device (when acpi_video_get_backlight_type()==native). > > This means that userspace briefly sees 2 devices and the disappearing of > acpi_video0 after a brief time confuses the systemd backlight level > save/restore code, see e.g.: > https://bbs.archlinux.org/viewtopic.php?id=269920 > > To fix this make backlight class device registration a separate step > done by a new acpi_video_register_backlight() function. The intend is for > this to be called by the drm/kms driver *after* it is done setting up its > own native backlight device. So that acpi_video_get_backlight_type() knows > if a native backlight will be available or not at acpi_video backlight > registration time, avoiding the add + remove dance. > > Note the new acpi_video_register_backlight() function is also called from > a delayed work to ensure that the acpi_video backlight devices does get > registered if necessary even if there is no drm/kms driver or when it is > disabled. > > Changes in v2: > - Make register_backlight_delay a module parameter, mainly so that it can > be disabled by Nvidia binary driver users > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/acpi/acpi_video.c | 50 ++++++++++++++++++++++++++++++++++++--- > include/acpi/video.h | 2 ++ > 2 files changed, 49 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > index 8545bf94866f..09dd86f86cf3 100644 > --- a/drivers/acpi/acpi_video.c > +++ b/drivers/acpi/acpi_video.c > @@ -73,6 +73,16 @@ module_param(device_id_scheme, bool, 0444); > static int only_lcd = -1; > module_param(only_lcd, int, 0444); > > +/* > + * Display probing is known to take up to 5 seconds, so delay the fallback > + * backlight registration by 5 seconds + 3 seconds for some extra margin. > + */ > +static int register_backlight_delay = 8; > +module_param(register_backlight_delay, int, 0444); Would it make sense to make this parameter writable from userspace, e.g. so that it can be set by a udev rule rather than relying on a riskier kernel command line edit? Then again, that probably makes things more complicated, since you'd have to check the parameter again when the worker fires, and changing the parameter to a non-zero value from either zero or a different non-zero value would be too weird. And making a separate writable parameter to allow userspace to turn the worker into a noop despite it being enabled when the kernel was initially loaded seems wrong, too. > +MODULE_PARM_DESC(register_backlight_delay, > + "Delay in seconds before doing fallback (non GPU driver triggered) " > + "backlight registration, set to 0 to disable."); > + > static bool may_report_brightness_keys; > static int register_count; > static DEFINE_MUTEX(register_count_mutex); > @@ -81,6 +91,9 @@ static LIST_HEAD(video_bus_head); > static int acpi_video_bus_add(struct acpi_device *device); > static int acpi_video_bus_remove(struct acpi_device *device); > static void acpi_video_bus_notify(struct acpi_device *device, u32 event); > +static void acpi_video_bus_register_backlight_work(struct work_struct *ignored); > +static DECLARE_DELAYED_WORK(video_bus_register_backlight_work, > + acpi_video_bus_register_backlight_work); > void acpi_video_detect_exit(void); > > /* > @@ -1859,8 +1872,6 @@ static int acpi_video_bus_register_backlight(struct acpi_video_bus *video) > if (video->backlight_registered) > return 0; > > - acpi_video_run_bcl_for_osi(video); > - > if (acpi_video_get_backlight_type() != acpi_backlight_video) > return 0; > > @@ -2086,7 +2097,11 @@ static int acpi_video_bus_add(struct acpi_device *device) > list_add_tail(&video->entry, &video_bus_head); > mutex_unlock(&video_list_lock); > > - acpi_video_bus_register_backlight(video); > + /* > + * The userspace visible backlight_device gets registered separately > + * from acpi_video_register_backlight(). > + */ > + acpi_video_run_bcl_for_osi(video); > acpi_video_bus_add_notify_handler(video); > > return 0; > @@ -2125,6 +2140,11 @@ static int acpi_video_bus_remove(struct acpi_device *device) > return 0; > } > > +static void acpi_video_bus_register_backlight_work(struct work_struct *ignored) > +{ > + acpi_video_register_backlight(); > +} > + > static int __init is_i740(struct pci_dev *dev) > { > if (dev->device == 0x00D1) > @@ -2235,6 +2255,18 @@ int acpi_video_register(void) > */ > register_count = 1; > > + /* > + * acpi_video_bus_add() skips registering the userspace visible > + * backlight_device. The intend is for this to be registered by the > + * drm/kms driver calling acpi_video_register_backlight() *after* it is > + * done setting up its own native backlight device. The delayed work > + * ensures that acpi_video_register_backlight() always gets called > + * eventually, in case there is no drm/kms driver or it is disabled. > + */ > + if (register_backlight_delay) > + schedule_delayed_work(&video_bus_register_backlight_work, > + register_backlight_delay * HZ); > + > leave: > mutex_unlock(®ister_count_mutex); > return ret; > @@ -2245,6 +2277,7 @@ void acpi_video_unregister(void) > { > mutex_lock(®ister_count_mutex); > if (register_count) { > + cancel_delayed_work_sync(&video_bus_register_backlight_work); > acpi_bus_unregister_driver(&acpi_video_bus); > register_count = 0; > may_report_brightness_keys = false; > @@ -2253,6 +2286,17 @@ void acpi_video_unregister(void) > } > EXPORT_SYMBOL(acpi_video_unregister); > > +void acpi_video_register_backlight(void) > +{ > + struct acpi_video_bus *video; > + > + mutex_lock(&video_list_lock); > + list_for_each_entry(video, &video_bus_head, entry) > + acpi_video_bus_register_backlight(video); > + mutex_unlock(&video_list_lock); > +} > +EXPORT_SYMBOL(acpi_video_register_backlight); > + > void acpi_video_unregister_backlight(void) > { > struct acpi_video_bus *video; > diff --git a/include/acpi/video.h b/include/acpi/video.h > index 4705e339c252..0625806d3bbd 100644 > --- a/include/acpi/video.h > +++ b/include/acpi/video.h > @@ -53,6 +53,7 @@ enum acpi_backlight_type { > #if IS_ENABLED(CONFIG_ACPI_VIDEO) > extern int acpi_video_register(void); > extern void acpi_video_unregister(void); > +extern void acpi_video_register_backlight(void); > extern int acpi_video_get_edid(struct acpi_device *device, int type, > int device_id, void **edid); > extern enum acpi_backlight_type acpi_video_get_backlight_type(void); > @@ -69,6 +70,7 @@ extern int acpi_video_get_levels(struct acpi_device *device, > #else > static inline int acpi_video_register(void) { return -ENODEV; } > static inline void acpi_video_unregister(void) { return; } > +static inline void acpi_video_register_backlight(void) { return; } > static inline int acpi_video_get_edid(struct acpi_device *device, int type, > int device_id, void **edid) > {
Hi, On 8/18/22 22:07, Daniel Dadap wrote: > > On 8/18/22 1:42 PM, Hans de Goede wrote: >> On x86/ACPI boards the acpi_video driver will usually initialize before >> the kms driver (except i915). This causes /sys/class/backlight/acpi_video0 >> to show up and then the kms driver registers its own native backlight >> device after which the drivers/acpi/video_detect.c code unregisters >> the acpi_video0 device (when acpi_video_get_backlight_type()==native). >> >> This means that userspace briefly sees 2 devices and the disappearing of >> acpi_video0 after a brief time confuses the systemd backlight level >> save/restore code, see e.g.: >> https://bbs.archlinux.org/viewtopic.php?id=269920 >> >> To fix this make backlight class device registration a separate step >> done by a new acpi_video_register_backlight() function. The intend is for >> this to be called by the drm/kms driver *after* it is done setting up its >> own native backlight device. So that acpi_video_get_backlight_type() knows >> if a native backlight will be available or not at acpi_video backlight >> registration time, avoiding the add + remove dance. >> >> Note the new acpi_video_register_backlight() function is also called from >> a delayed work to ensure that the acpi_video backlight devices does get >> registered if necessary even if there is no drm/kms driver or when it is >> disabled. >> >> Changes in v2: >> - Make register_backlight_delay a module parameter, mainly so that it can >> be disabled by Nvidia binary driver users >> >> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/acpi/acpi_video.c | 50 ++++++++++++++++++++++++++++++++++++--- >> include/acpi/video.h | 2 ++ >> 2 files changed, 49 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c >> index 8545bf94866f..09dd86f86cf3 100644 >> --- a/drivers/acpi/acpi_video.c >> +++ b/drivers/acpi/acpi_video.c >> @@ -73,6 +73,16 @@ module_param(device_id_scheme, bool, 0444); >> static int only_lcd = -1; >> module_param(only_lcd, int, 0444); >> +/* >> + * Display probing is known to take up to 5 seconds, so delay the fallback >> + * backlight registration by 5 seconds + 3 seconds for some extra margin. >> + */ >> +static int register_backlight_delay = 8; >> +module_param(register_backlight_delay, int, 0444); > > > Would it make sense to make this parameter writable from userspace, e.g. so that it can be set by a udev rule rather than relying on a riskier kernel command line edit? Then again, that probably makes things more complicated, since you'd have to check the parameter again when the worker fires, and changing the parameter to a non-zero value from either zero or a different non-zero value would be too weird. And making a separate writable parameter to allow userspace to turn the worker into a noop despite it being enabled when the kernel was initially loaded seems wrong, too. Right, you have pretty much described yourself why making this parameter runtime configurable is not really feasible :) Regards, Hans > > >> +MODULE_PARM_DESC(register_backlight_delay, >> + "Delay in seconds before doing fallback (non GPU driver triggered) " >> + "backlight registration, set to 0 to disable."); >> + >> static bool may_report_brightness_keys; >> static int register_count; >> static DEFINE_MUTEX(register_count_mutex); >> @@ -81,6 +91,9 @@ static LIST_HEAD(video_bus_head); >> static int acpi_video_bus_add(struct acpi_device *device); >> static int acpi_video_bus_remove(struct acpi_device *device); >> static void acpi_video_bus_notify(struct acpi_device *device, u32 event); >> +static void acpi_video_bus_register_backlight_work(struct work_struct *ignored); >> +static DECLARE_DELAYED_WORK(video_bus_register_backlight_work, >> + acpi_video_bus_register_backlight_work); >> void acpi_video_detect_exit(void); >> /* >> @@ -1859,8 +1872,6 @@ static int acpi_video_bus_register_backlight(struct acpi_video_bus *video) >> if (video->backlight_registered) >> return 0; >> - acpi_video_run_bcl_for_osi(video); >> - >> if (acpi_video_get_backlight_type() != acpi_backlight_video) >> return 0; >> @@ -2086,7 +2097,11 @@ static int acpi_video_bus_add(struct acpi_device *device) >> list_add_tail(&video->entry, &video_bus_head); >> mutex_unlock(&video_list_lock); >> - acpi_video_bus_register_backlight(video); >> + /* >> + * The userspace visible backlight_device gets registered separately >> + * from acpi_video_register_backlight(). >> + */ >> + acpi_video_run_bcl_for_osi(video); >> acpi_video_bus_add_notify_handler(video); >> return 0; >> @@ -2125,6 +2140,11 @@ static int acpi_video_bus_remove(struct acpi_device *device) >> return 0; >> } >> +static void acpi_video_bus_register_backlight_work(struct work_struct *ignored) >> +{ >> + acpi_video_register_backlight(); >> +} >> + >> static int __init is_i740(struct pci_dev *dev) >> { >> if (dev->device == 0x00D1) >> @@ -2235,6 +2255,18 @@ int acpi_video_register(void) >> */ >> register_count = 1; >> + /* >> + * acpi_video_bus_add() skips registering the userspace visible >> + * backlight_device. The intend is for this to be registered by the >> + * drm/kms driver calling acpi_video_register_backlight() *after* it is >> + * done setting up its own native backlight device. The delayed work >> + * ensures that acpi_video_register_backlight() always gets called >> + * eventually, in case there is no drm/kms driver or it is disabled. >> + */ >> + if (register_backlight_delay) >> + schedule_delayed_work(&video_bus_register_backlight_work, >> + register_backlight_delay * HZ); >> + >> leave: >> mutex_unlock(®ister_count_mutex); >> return ret; >> @@ -2245,6 +2277,7 @@ void acpi_video_unregister(void) >> { >> mutex_lock(®ister_count_mutex); >> if (register_count) { >> + cancel_delayed_work_sync(&video_bus_register_backlight_work); >> acpi_bus_unregister_driver(&acpi_video_bus); >> register_count = 0; >> may_report_brightness_keys = false; >> @@ -2253,6 +2286,17 @@ void acpi_video_unregister(void) >> } >> EXPORT_SYMBOL(acpi_video_unregister); >> +void acpi_video_register_backlight(void) >> +{ >> + struct acpi_video_bus *video; >> + >> + mutex_lock(&video_list_lock); >> + list_for_each_entry(video, &video_bus_head, entry) >> + acpi_video_bus_register_backlight(video); >> + mutex_unlock(&video_list_lock); >> +} >> +EXPORT_SYMBOL(acpi_video_register_backlight); >> + >> void acpi_video_unregister_backlight(void) >> { >> struct acpi_video_bus *video; >> diff --git a/include/acpi/video.h b/include/acpi/video.h >> index 4705e339c252..0625806d3bbd 100644 >> --- a/include/acpi/video.h >> +++ b/include/acpi/video.h >> @@ -53,6 +53,7 @@ enum acpi_backlight_type { >> #if IS_ENABLED(CONFIG_ACPI_VIDEO) >> extern int acpi_video_register(void); >> extern void acpi_video_unregister(void); >> +extern void acpi_video_register_backlight(void); >> extern int acpi_video_get_edid(struct acpi_device *device, int type, >> int device_id, void **edid); >> extern enum acpi_backlight_type acpi_video_get_backlight_type(void); >> @@ -69,6 +70,7 @@ extern int acpi_video_get_levels(struct acpi_device *device, >> #else >> static inline int acpi_video_register(void) { return -ENODEV; } >> static inline void acpi_video_unregister(void) { return; } >> +static inline void acpi_video_register_backlight(void) { return; } >> static inline int acpi_video_get_edid(struct acpi_device *device, int type, >> int device_id, void **edid) >> { >
diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c index 8545bf94866f..09dd86f86cf3 100644 --- a/drivers/acpi/acpi_video.c +++ b/drivers/acpi/acpi_video.c @@ -73,6 +73,16 @@ module_param(device_id_scheme, bool, 0444); static int only_lcd = -1; module_param(only_lcd, int, 0444); +/* + * Display probing is known to take up to 5 seconds, so delay the fallback + * backlight registration by 5 seconds + 3 seconds for some extra margin. + */ +static int register_backlight_delay = 8; +module_param(register_backlight_delay, int, 0444); +MODULE_PARM_DESC(register_backlight_delay, + "Delay in seconds before doing fallback (non GPU driver triggered) " + "backlight registration, set to 0 to disable."); + static bool may_report_brightness_keys; static int register_count; static DEFINE_MUTEX(register_count_mutex); @@ -81,6 +91,9 @@ static LIST_HEAD(video_bus_head); static int acpi_video_bus_add(struct acpi_device *device); static int acpi_video_bus_remove(struct acpi_device *device); static void acpi_video_bus_notify(struct acpi_device *device, u32 event); +static void acpi_video_bus_register_backlight_work(struct work_struct *ignored); +static DECLARE_DELAYED_WORK(video_bus_register_backlight_work, + acpi_video_bus_register_backlight_work); void acpi_video_detect_exit(void); /* @@ -1859,8 +1872,6 @@ static int acpi_video_bus_register_backlight(struct acpi_video_bus *video) if (video->backlight_registered) return 0; - acpi_video_run_bcl_for_osi(video); - if (acpi_video_get_backlight_type() != acpi_backlight_video) return 0; @@ -2086,7 +2097,11 @@ static int acpi_video_bus_add(struct acpi_device *device) list_add_tail(&video->entry, &video_bus_head); mutex_unlock(&video_list_lock); - acpi_video_bus_register_backlight(video); + /* + * The userspace visible backlight_device gets registered separately + * from acpi_video_register_backlight(). + */ + acpi_video_run_bcl_for_osi(video); acpi_video_bus_add_notify_handler(video); return 0; @@ -2125,6 +2140,11 @@ static int acpi_video_bus_remove(struct acpi_device *device) return 0; } +static void acpi_video_bus_register_backlight_work(struct work_struct *ignored) +{ + acpi_video_register_backlight(); +} + static int __init is_i740(struct pci_dev *dev) { if (dev->device == 0x00D1) @@ -2235,6 +2255,18 @@ int acpi_video_register(void) */ register_count = 1; + /* + * acpi_video_bus_add() skips registering the userspace visible + * backlight_device. The intend is for this to be registered by the + * drm/kms driver calling acpi_video_register_backlight() *after* it is + * done setting up its own native backlight device. The delayed work + * ensures that acpi_video_register_backlight() always gets called + * eventually, in case there is no drm/kms driver or it is disabled. + */ + if (register_backlight_delay) + schedule_delayed_work(&video_bus_register_backlight_work, + register_backlight_delay * HZ); + leave: mutex_unlock(®ister_count_mutex); return ret; @@ -2245,6 +2277,7 @@ void acpi_video_unregister(void) { mutex_lock(®ister_count_mutex); if (register_count) { + cancel_delayed_work_sync(&video_bus_register_backlight_work); acpi_bus_unregister_driver(&acpi_video_bus); register_count = 0; may_report_brightness_keys = false; @@ -2253,6 +2286,17 @@ void acpi_video_unregister(void) } EXPORT_SYMBOL(acpi_video_unregister); +void acpi_video_register_backlight(void) +{ + struct acpi_video_bus *video; + + mutex_lock(&video_list_lock); + list_for_each_entry(video, &video_bus_head, entry) + acpi_video_bus_register_backlight(video); + mutex_unlock(&video_list_lock); +} +EXPORT_SYMBOL(acpi_video_register_backlight); + void acpi_video_unregister_backlight(void) { struct acpi_video_bus *video; diff --git a/include/acpi/video.h b/include/acpi/video.h index 4705e339c252..0625806d3bbd 100644 --- a/include/acpi/video.h +++ b/include/acpi/video.h @@ -53,6 +53,7 @@ enum acpi_backlight_type { #if IS_ENABLED(CONFIG_ACPI_VIDEO) extern int acpi_video_register(void); extern void acpi_video_unregister(void); +extern void acpi_video_register_backlight(void); extern int acpi_video_get_edid(struct acpi_device *device, int type, int device_id, void **edid); extern enum acpi_backlight_type acpi_video_get_backlight_type(void); @@ -69,6 +70,7 @@ extern int acpi_video_get_levels(struct acpi_device *device, #else static inline int acpi_video_register(void) { return -ENODEV; } static inline void acpi_video_unregister(void) { return; } +static inline void acpi_video_register_backlight(void) { return; } static inline int acpi_video_get_edid(struct acpi_device *device, int type, int device_id, void **edid) {