Re: [Nouveau] [PATCH v5 11/31] drm/i915: Call acpi_video_register_backlight() (v3)

[Jani Nikula <jani.nikula@linux.intel.com>]

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);

-- 
Jani Nikula, Intel Open Source Graphics Center