Re: [PATCH v5 11/31] drm/i915: Call acpi_video_register_backlight() (v3)
On Thu, 25 Aug 2022, Hans de Goede 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
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
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
> #include
> +#include
>
> #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
[PATCH v5 11/31] drm/i915: Call acpi_video_register_backlight() (v3)
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
---
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
#include
+#include
#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);
--
2.37.2
