| Message ID | 20220825143726.269890-12-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 Thu, 25 Aug 2022, Hans de Goede <hdegoede@redhat.com> wrote: > On machins without an i915 opregion the acpi_video driver immediately > probes the ACPI video bus and used to also immediately register > acpi_video# backlight devices when supported. > > Once the drm/kms driver then loaded later and possibly registered > a native backlight device then the drivers/acpi/video_detect.c code > unregistered the acpi_video0 device to avoid there being 2 backlight > devices (when acpi_video_get_backlight_type()==native). > > This means that userspace used to briefly see 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 the ACPI video code has been modified to make backlight class > device registration a separate step, relying on the drm/kms driver to > ask for the acpi_video backlight registration after it is done setting up > its native backlight device. > > Add a call to the new acpi_video_register_backlight() after the i915 calls > acpi_video_register() (after setting up the i915 opregion) so that the > acpi_video backlight devices get registered on systems where the i915 > native backlight device is not registered. > > Changes in v2: > -Only call acpi_video_register_backlight() when a panel is detected > > Changes in v3: > -Add a new intel_acpi_video_register() helper which checks if a panel > is present and then calls acpi_video_register_backlight() > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Apologies for the delay. I truly appreciate the effort you've put into this series, and I'm looking forward to seeing the next steps in drm! Reviewed-by: Jani Nikula <jani.nikula@intel.com> And ack for merging via whichever tree you think best. > --- > drivers/gpu/drm/i915/display/intel_acpi.c | 27 ++++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_acpi.h | 3 +++ > drivers/gpu/drm/i915/display/intel_display.c | 2 +- > 3 files changed, 31 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c > index e78430001f07..9df78e7caa2b 100644 > --- a/drivers/gpu/drm/i915/display/intel_acpi.c > +++ b/drivers/gpu/drm/i915/display/intel_acpi.c > @@ -7,6 +7,7 @@ > > #include <linux/pci.h> > #include <linux/acpi.h> > +#include <acpi/video.h> > > #include "i915_drv.h" > #include "intel_acpi.h" > @@ -331,3 +332,29 @@ void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) > */ > fwnode_handle_put(fwnode); > } > + > +void intel_acpi_video_register(struct drm_i915_private *i915) > +{ > + struct drm_connector_list_iter conn_iter; > + struct drm_connector *connector; > + > + acpi_video_register(); > + > + /* > + * If i915 is driving an internal panel without registering its native > + * backlight handler try to register the acpi_video backlight. > + * For panels not driven by i915 another GPU driver may still register > + * a native backlight later and acpi_video_register_backlight() should > + * only be called after any native backlights have been registered. > + */ > + drm_connector_list_iter_begin(&i915->drm, &conn_iter); > + drm_for_each_connector_iter(connector, &conn_iter) { > + struct intel_panel *panel = &to_intel_connector(connector)->panel; > + > + if (panel->backlight.funcs && !panel->backlight.device) { > + acpi_video_register_backlight(); > + break; > + } > + } > + drm_connector_list_iter_end(&conn_iter); > +} > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h b/drivers/gpu/drm/i915/display/intel_acpi.h > index 4a760a2baed9..6a0007452f95 100644 > --- a/drivers/gpu/drm/i915/display/intel_acpi.h > +++ b/drivers/gpu/drm/i915/display/intel_acpi.h > @@ -14,6 +14,7 @@ void intel_unregister_dsm_handler(void); > void intel_dsm_get_bios_data_funcs_supported(struct drm_i915_private *i915); > void intel_acpi_device_id_update(struct drm_i915_private *i915); > void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915); > +void intel_acpi_video_register(struct drm_i915_private *i915); > #else > static inline void intel_register_dsm_handler(void) { return; } > static inline void intel_unregister_dsm_handler(void) { return; } > @@ -23,6 +24,8 @@ static inline > void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; } > static inline > void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) { return; } > +static inline > +void intel_acpi_video_register(struct drm_i915_private *i915) { return; } > #endif /* CONFIG_ACPI */ > > #endif /* __INTEL_ACPI_H__ */ > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 6103b02c081f..129a13375101 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -9087,7 +9087,7 @@ void intel_display_driver_register(struct drm_i915_private *i915) > > /* Must be done after probing outputs */ > intel_opregion_register(i915); > - acpi_video_register(); > + intel_acpi_video_register(i915); > > intel_audio_init(i915);
diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c index e78430001f07..9df78e7caa2b 100644 --- a/drivers/gpu/drm/i915/display/intel_acpi.c +++ b/drivers/gpu/drm/i915/display/intel_acpi.c @@ -7,6 +7,7 @@ #include <linux/pci.h> #include <linux/acpi.h> +#include <acpi/video.h> #include "i915_drv.h" #include "intel_acpi.h" @@ -331,3 +332,29 @@ void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) */ fwnode_handle_put(fwnode); } + +void intel_acpi_video_register(struct drm_i915_private *i915) +{ + struct drm_connector_list_iter conn_iter; + struct drm_connector *connector; + + acpi_video_register(); + + /* + * If i915 is driving an internal panel without registering its native + * backlight handler try to register the acpi_video backlight. + * For panels not driven by i915 another GPU driver may still register + * a native backlight later and acpi_video_register_backlight() should + * only be called after any native backlights have been registered. + */ + drm_connector_list_iter_begin(&i915->drm, &conn_iter); + drm_for_each_connector_iter(connector, &conn_iter) { + struct intel_panel *panel = &to_intel_connector(connector)->panel; + + if (panel->backlight.funcs && !panel->backlight.device) { + acpi_video_register_backlight(); + break; + } + } + drm_connector_list_iter_end(&conn_iter); +} diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h b/drivers/gpu/drm/i915/display/intel_acpi.h index 4a760a2baed9..6a0007452f95 100644 --- a/drivers/gpu/drm/i915/display/intel_acpi.h +++ b/drivers/gpu/drm/i915/display/intel_acpi.h @@ -14,6 +14,7 @@ void intel_unregister_dsm_handler(void); void intel_dsm_get_bios_data_funcs_supported(struct drm_i915_private *i915); void intel_acpi_device_id_update(struct drm_i915_private *i915); void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915); +void intel_acpi_video_register(struct drm_i915_private *i915); #else static inline void intel_register_dsm_handler(void) { return; } static inline void intel_unregister_dsm_handler(void) { return; } @@ -23,6 +24,8 @@ static inline void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; } static inline void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) { return; } +static inline +void intel_acpi_video_register(struct drm_i915_private *i915) { return; } #endif /* CONFIG_ACPI */ #endif /* __INTEL_ACPI_H__ */ diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 6103b02c081f..129a13375101 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -9087,7 +9087,7 @@ void intel_display_driver_register(struct drm_i915_private *i915) /* Must be done after probing outputs */ intel_opregion_register(i915); - acpi_video_register(); + intel_acpi_video_register(i915); intel_audio_init(i915);
On machins without an i915 opregion the acpi_video driver immediately probes the ACPI video bus and used to also immediately register acpi_video# backlight devices when supported. Once the drm/kms driver then loaded later and possibly registered a native backlight device then the drivers/acpi/video_detect.c code unregistered the acpi_video0 device to avoid there being 2 backlight devices (when acpi_video_get_backlight_type()==native). This means that userspace used to briefly see 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 the ACPI video code has been modified to make backlight class device registration a separate step, relying on the drm/kms driver to ask for the acpi_video backlight registration after it is done setting up its native backlight device. Add a call to the new acpi_video_register_backlight() after the i915 calls acpi_video_register() (after setting up the i915 opregion) so that the acpi_video backlight devices get registered on systems where the i915 native backlight device is not registered. Changes in v2: -Only call acpi_video_register_backlight() when a panel is detected Changes in v3: -Add a new intel_acpi_video_register() helper which checks if a panel is present and then calls acpi_video_register_backlight() Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/i915/display/intel_acpi.c | 27 ++++++++++++++++++++ drivers/gpu/drm/i915/display/intel_acpi.h | 3 +++ drivers/gpu/drm/i915/display/intel_display.c | 2 +- 3 files changed, 31 insertions(+), 1 deletion(-)