thr3ads.net - Nouveau - [Nouveau] [PATCH 00/14] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display [May 2022]

If this information is useful, please help other people find it:
Share via:

Hans de Goede

2022-May-17 15:23 UTC

[Nouveau] [PATCH 00/14] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display

Hi All,

As mentioned in my RFC titled "drm/kms: control display brightness through
drm_connector properties":
https://lore.kernel.org/dri-devel/0d188965-d809-81b5-74ce-7d30c49fee2d at
redhat.com/

The first step towards this is to deal with some existing technical debt
in backlight handling on x86/ACPI boards, specifically we need to stop
registering multiple /sys/class/backlight devs for a single display.

This series implements my RFC describing my plan for these cleanups:
https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158 at
redhat.com/

Specifically patches 1-6 implement the "Fixing kms driver unconditionally
register their "native" backlight dev" part.

And patches 7-14 implement the "Fixing acpi_video0 getting registered for
a brief time" time.

Note this series does not deal yet with the "Other issues" part, I
plan
to do a follow up series for that.

The changes in this series are good to have regardless of the further
"drm/kms: control display brightness through drm_connector properties"
plans. So I plan to push these upstream once they are ready (once
reviewed). Since this crosses various subsystems / touches multiple
kms drivers my plan is to provide an immutable branch based on say
5.19-rc1 and then have that get merged into all the relevant trees.

Please review.

Regards,

Hans


Hans de Goede (14):
  ACPI: video: Add a native function parameter to
    acpi_video_get_backlight_type()
  drm/i915: Don't register backlight when another backlight should be
    used
  drm/amdgpu: Don't register backlight when another backlight should be
    used
  drm/radeon: Don't register backlight when another backlight should be
    used
  drm/nouveau: Don't register backlight when another backlight should be
    used
  ACPI: video: Drop backlight_device_get_by_type() call from
    acpi_video_get_backlight_type()
  ACPI: video: Remove acpi_video_bus from list before tearing it down
  ACPI: video: Simplify acpi_video_unregister_backlight()
  ACPI: video: Make backlight class device registration a separate step
  ACPI: video: Remove code to unregister acpi_video backlight when a
    native backlight registers
  drm/i915: Call acpi_video_register_backlight()
  drm/nouveau: Register ACPI video backlight when nv_backlight
    registration fails
  drm/amdgpu: Register ACPI video backlight when skipping amdgpu
    backlight registration
  drm/radeon: Register ACPI video backlight when skipping radeon
    backlight registration

 drivers/acpi/acpi_video.c                     | 69 ++++++++++++++-----
 drivers/acpi/video_detect.c                   | 53 +++-----------
 drivers/gpu/drm/Kconfig                       |  2 +
 .../gpu/drm/amd/amdgpu/atombios_encoders.c    | 14 +++-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  9 +++
 .../gpu/drm/i915/display/intel_backlight.c    |  7 ++
 drivers/gpu/drm/i915/display/intel_display.c  |  1 +
 drivers/gpu/drm/i915/display/intel_opregion.c |  2 +-
 drivers/gpu/drm/nouveau/nouveau_backlight.c   | 14 ++++
 drivers/gpu/drm/radeon/atombios_encoders.c    |  7 ++
 drivers/gpu/drm/radeon/radeon_encoders.c      | 11 ++-
 .../gpu/drm/radeon/radeon_legacy_encoders.c   |  7 ++
 drivers/platform/x86/acer-wmi.c               |  2 +-
 drivers/platform/x86/asus-laptop.c            |  2 +-
 drivers/platform/x86/asus-wmi.c               |  4 +-
 drivers/platform/x86/compal-laptop.c          |  2 +-
 drivers/platform/x86/dell/dell-laptop.c       |  2 +-
 drivers/platform/x86/eeepc-laptop.c           |  2 +-
 drivers/platform/x86/fujitsu-laptop.c         |  4 +-
 drivers/platform/x86/ideapad-laptop.c         |  2 +-
 drivers/platform/x86/intel/oaktrail.c         |  2 +-
 drivers/platform/x86/msi-laptop.c             |  2 +-
 drivers/platform/x86/msi-wmi.c                |  2 +-
 drivers/platform/x86/samsung-laptop.c         |  2 +-
 drivers/platform/x86/sony-laptop.c            |  2 +-
 drivers/platform/x86/thinkpad_acpi.c          |  4 +-
 drivers/platform/x86/toshiba_acpi.c           |  2 +-
 include/acpi/video.h                          |  8 ++-
 28 files changed, 156 insertions(+), 84 deletions(-)

-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 01/14] ACPI: video: Add a native function parameter to acpi_video_get_backlight_type()

ATM on x86 laptops where we want userspace to use the acpi_video backlight
device we often register both the GPU's native backlight device and
acpi_video's firmware acpi_video# backlight device. This relies on
userspace preferring firmware type backlight devices over native ones, but
registering 2 backlight devices for a single display really is undesirable.

On x86 laptops where the native GPU backlight device should be used,
the registering of other backlight devices is avoided by their drivers
using acpi_video_get_backlight_type() and only registering their backlight
if the return value matches their type.

acpi_video_get_backlight_type() uses
backlight_device_get_by_type(BACKLIGHT_RAW) to determine if a native
driver is available and will never return native if this returns
false. This means that the GPU's native backlight registering code
cannot just call acpi_video_get_backlight_type() to determine if it
should register its backlight, since acpi_video_get_backlight_type() will
never return native until the native backlight has already registered.

To fix this add a native function parameter to
acpi_video_get_backlight_type(), which when set to true will make
acpi_video_get_backlight_type() behave as if a native backlight has
already been registered.

Note that all current callers are updated to pass false for the new
parameter, so this change in itself causes no functional changes.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/acpi/acpi_video.c                     |  2 +-
 drivers/acpi/video_detect.c                   | 20 ++++++++++++-------
 drivers/gpu/drm/i915/display/intel_opregion.c |  2 +-
 drivers/platform/x86/acer-wmi.c               |  2 +-
 drivers/platform/x86/asus-laptop.c            |  2 +-
 drivers/platform/x86/asus-wmi.c               |  4 ++--
 drivers/platform/x86/compal-laptop.c          |  2 +-
 drivers/platform/x86/dell/dell-laptop.c       |  2 +-
 drivers/platform/x86/eeepc-laptop.c           |  2 +-
 drivers/platform/x86/fujitsu-laptop.c         |  4 ++--
 drivers/platform/x86/ideapad-laptop.c         |  2 +-
 drivers/platform/x86/intel/oaktrail.c         |  2 +-
 drivers/platform/x86/msi-laptop.c             |  2 +-
 drivers/platform/x86/msi-wmi.c                |  2 +-
 drivers/platform/x86/samsung-laptop.c         |  2 +-
 drivers/platform/x86/sony-laptop.c            |  2 +-
 drivers/platform/x86/thinkpad_acpi.c          |  4 ++--
 drivers/platform/x86/toshiba_acpi.c           |  2 +-
 include/acpi/video.h                          |  6 +++---
 19 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 990ff5b0aeb8..cebef3403620 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1864,7 +1864,7 @@ static int acpi_video_bus_register_backlight(struct
acpi_video_bus *video)
 
 	acpi_video_run_bcl_for_osi(video);
 
-	if (acpi_video_get_backlight_type() != acpi_backlight_video)
+	if (acpi_video_get_backlight_type(false) != acpi_backlight_video)
 		return 0;
 
 	mutex_lock(&video->device_list_lock);
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index becc198e4c22..0a06f0edd298 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -17,12 +17,14 @@
  * Otherwise vendor specific drivers like thinkpad_acpi, asus-laptop,
  * sony_acpi,... can take care about backlight brightness.
  *
- * Backlight drivers can use acpi_video_get_backlight_type() to determine
- * which driver should handle the backlight.
+ * Backlight drivers can use acpi_video_get_backlight_type() to determine which
+ * driver should handle the backlight. RAW/GPU-driver backlight drivers must
+ * pass true for the native function argument, other drivers must pass false.
  *
  * If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a
module (m)
  * this file will not be compiled and acpi_video_get_backlight_type() will
- * always return acpi_backlight_vendor.
+ * return acpi_backlight_native when its native argument is true and
+ * acpi_backlight_vendor when it is false.
  */
 
 #include <linux/export.h>
@@ -517,7 +519,7 @@ static const struct dmi_system_id video_detect_dmi_table[] =
{
 /* This uses a workqueue to avoid various locking ordering issues */
 static void acpi_video_backlight_notify_work(struct work_struct *work)
 {
-	if (acpi_video_get_backlight_type() != acpi_backlight_video)
+	if (acpi_video_get_backlight_type(false) != acpi_backlight_video)
 		acpi_video_unregister_backlight();
 }
 
@@ -548,9 +550,10 @@ static int acpi_video_backlight_notify(struct
notifier_block *nb,
  * Arguably the native on win8 check should be done first, but that would
  * be a behavior change, which may causes issues.
  */
-enum acpi_backlight_type acpi_video_get_backlight_type(void)
+enum acpi_backlight_type acpi_video_get_backlight_type(bool native)
 {
 	static DEFINE_MUTEX(init_mutex);
+	static bool native_available;
 	static bool init_done;
 	static long video_caps;
 
@@ -570,6 +573,8 @@ enum acpi_backlight_type acpi_video_get_backlight_type(void)
 			backlight_notifier_registered = true;
 		init_done = true;
 	}
+	if (native)
+		native_available = true;
 	mutex_unlock(&init_mutex);
 
 	if (acpi_backlight_cmdline != acpi_backlight_undef)
@@ -581,7 +586,8 @@ enum acpi_backlight_type acpi_video_get_backlight_type(void)
 	if (!(video_caps & ACPI_VIDEO_BACKLIGHT))
 		return acpi_backlight_vendor;
 
-	if (acpi_osi_is_win8() && backlight_device_get_by_type(BACKLIGHT_RAW))
+	if (acpi_osi_is_win8() &&
+	    (native_available || backlight_device_get_by_type(BACKLIGHT_RAW)))
 		return acpi_backlight_native;
 
 	return acpi_backlight_video;
@@ -597,7 +603,7 @@ void acpi_video_set_dmi_backlight_type(enum
acpi_backlight_type type)
 {
 	acpi_backlight_dmi = type;
 	/* Remove acpi-video backlight interface if it is no longer desired */
-	if (acpi_video_get_backlight_type() != acpi_backlight_video)
+	if (acpi_video_get_backlight_type(false) != acpi_backlight_video)
 		acpi_video_unregister_backlight();
 }
 EXPORT_SYMBOL(acpi_video_set_dmi_backlight_type);
diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c
b/drivers/gpu/drm/i915/display/intel_opregion.c
index f31e8c3f8ce0..ed726a8af478 100644
--- a/drivers/gpu/drm/i915/display/intel_opregion.c
+++ b/drivers/gpu/drm/i915/display/intel_opregion.c
@@ -463,7 +463,7 @@ static u32 asle_set_backlight(struct drm_i915_private
*dev_priv, u32 bclp)
 
 	drm_dbg(&dev_priv->drm, "bclp = 0x%08x\n", bclp);
 
-	if (acpi_video_get_backlight_type() == acpi_backlight_native) {
+	if (acpi_video_get_backlight_type(false) == acpi_backlight_native) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "opregion backlight request ignored\n");
 		return 0;
diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index 9c6943e401a6..0f665106692b 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -2485,7 +2485,7 @@ static int __init acer_wmi_init(void)
 	if (dmi_check_system(video_vendor_dmi_table))
 		acpi_video_set_dmi_backlight_type(acpi_backlight_vendor);
 
-	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
+	if (acpi_video_get_backlight_type(false) != acpi_backlight_vendor)
 		interface->capability &= ~ACER_CAP_BRIGHTNESS;
 
 	if (wmi_has_guid(WMID_GUID3))
diff --git a/drivers/platform/x86/asus-laptop.c
b/drivers/platform/x86/asus-laptop.c
index 4d2d32bfbe2a..eb78bbf79894 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -1854,7 +1854,7 @@ static int asus_acpi_add(struct acpi_device *device)
 	if (result)
 		goto fail_platform;
 
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+	if (acpi_video_get_backlight_type(false) == acpi_backlight_vendor) {
 		result = asus_backlight_init(asus);
 		if (result)
 			goto fail_backlight;
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 62ce198a3463..30171ce9ba96 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3055,7 +3055,7 @@ static void asus_wmi_handle_event_code(int code, struct
asus_wmi *asus)
 		code = ASUS_WMI_BRN_DOWN;
 
 	if (code == ASUS_WMI_BRN_DOWN || code == ASUS_WMI_BRN_UP) {
-		if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+		if (acpi_video_get_backlight_type(false) == acpi_backlight_vendor) {
 			asus_wmi_backlight_notify(asus, orig_code);
 			return;
 		}
@@ -3625,7 +3625,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (asus->driver->quirks->xusb2pr)
 		asus_wmi_set_xusb2pr(asus);
 
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+	if (acpi_video_get_backlight_type(false) == acpi_backlight_vendor) {
 		err = asus_wmi_backlight_init(asus);
 		if (err && err != -ENODEV)
 			goto fail_backlight;
diff --git a/drivers/platform/x86/compal-laptop.c
b/drivers/platform/x86/compal-laptop.c
index ab610376fdad..252a5f83c778 100644
--- a/drivers/platform/x86/compal-laptop.c
+++ b/drivers/platform/x86/compal-laptop.c
@@ -981,7 +981,7 @@ static int __init compal_init(void)
 		return -ENODEV;
 	}
 
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+	if (acpi_video_get_backlight_type(false) == acpi_backlight_vendor) {
 		struct backlight_properties props;
 		memset(&props, 0, sizeof(struct backlight_properties));
 		props.type = BACKLIGHT_PLATFORM;
diff --git a/drivers/platform/x86/dell/dell-laptop.c
b/drivers/platform/x86/dell/dell-laptop.c
index 1321687d923e..9c19248f45dd 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -2230,7 +2230,7 @@ static int __init dell_init(void)
 		micmute_led_registered = true;
 	}
 
-	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
+	if (acpi_video_get_backlight_type(false) != acpi_backlight_vendor)
 		return 0;
 
 	token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
diff --git a/drivers/platform/x86/eeepc-laptop.c
b/drivers/platform/x86/eeepc-laptop.c
index ba08c9235f76..f534208798f7 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -1400,7 +1400,7 @@ static int eeepc_acpi_add(struct acpi_device *device)
 	if (result)
 		goto fail_platform;
 
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+	if (acpi_video_get_backlight_type(false) == acpi_backlight_vendor) {
 		result = eeepc_backlight_init(eeepc);
 		if (result)
 			goto fail_backlight;
diff --git a/drivers/platform/x86/fujitsu-laptop.c
b/drivers/platform/x86/fujitsu-laptop.c
index 80929380ec7e..04e85404760f 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -387,7 +387,7 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	struct fujitsu_bl *priv;
 	int ret;
 
-	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
+	if (acpi_video_get_backlight_type(false) != acpi_backlight_vendor)
 		return -ENODEV;
 
 	priv = devm_kzalloc(&device->dev, sizeof(*priv), GFP_KERNEL);
@@ -819,7 +819,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device
*device)
 
 	/* Sync backlight power status */
 	if (fujitsu_bl && fujitsu_bl->bl_device &&
-	    acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+	    acpi_video_get_backlight_type(false) == acpi_backlight_vendor) {
 		if (call_fext_func(fext, FUNC_BACKLIGHT, 0x2,
 				   BACKLIGHT_PARAM_POWER, 0x0) == BACKLIGHT_OFF)
 			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
diff --git a/drivers/platform/x86/ideapad-laptop.c
b/drivers/platform/x86/ideapad-laptop.c
index 3ccb7b71dfb1..deb123e7f88f 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1620,7 +1620,7 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 			dev_info(&pdev->dev, "DYTC interface is not available\n");
 	}
 
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+	if (acpi_video_get_backlight_type(false) == acpi_backlight_vendor) {
 		err = ideapad_backlight_init(priv);
 		if (err && err != -ENODEV)
 			goto backlight_failed;
diff --git a/drivers/platform/x86/intel/oaktrail.c
b/drivers/platform/x86/intel/oaktrail.c
index 1a09a75bd16d..631ae393e52e 100644
--- a/drivers/platform/x86/intel/oaktrail.c
+++ b/drivers/platform/x86/intel/oaktrail.c
@@ -330,7 +330,7 @@ static int __init oaktrail_init(void)
 		goto err_device_add;
 	}
 
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+	if (acpi_video_get_backlight_type(false) == acpi_backlight_vendor) {
 		ret = oaktrail_backlight_init();
 		if (ret)
 			goto err_backlight;
diff --git a/drivers/platform/x86/msi-laptop.c
b/drivers/platform/x86/msi-laptop.c
index 24ffc8e2d2d1..8c8cb814ae09 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -1050,7 +1050,7 @@ static int __init msi_init(void)
 	/* Register backlight stuff */
 
 	if (quirks->old_ec_model ||
-	    acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+	    acpi_video_get_backlight_type(false) == acpi_backlight_vendor) {
 		struct backlight_properties props;
 		memset(&props, 0, sizeof(struct backlight_properties));
 		props.type = BACKLIGHT_PLATFORM;
diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index fd318cdfe313..3e6f291ba14e 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -309,7 +309,7 @@ static int __init msi_wmi_init(void)
 	}
 
 	if (wmi_has_guid(MSIWMI_BIOS_GUID) &&
-	    acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+	    acpi_video_get_backlight_type(false) == acpi_backlight_vendor) {
 		err = msi_wmi_backlight_setup();
 		if (err) {
 			pr_err("Unable to setup backlight device\n");
diff --git a/drivers/platform/x86/samsung-laptop.c
b/drivers/platform/x86/samsung-laptop.c
index c187dcdf82f0..985e6ea0fabf 100644
--- a/drivers/platform/x86/samsung-laptop.c
+++ b/drivers/platform/x86/samsung-laptop.c
@@ -1660,7 +1660,7 @@ static int __init samsung_init(void)
 	if (samsung->quirks->use_native_backlight)
 		acpi_video_set_dmi_backlight_type(acpi_backlight_native);
 
-	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
+	if (acpi_video_get_backlight_type(false) != acpi_backlight_vendor)
 		samsung->handle_backlight = false;
 #endif
 
diff --git a/drivers/platform/x86/sony-laptop.c
b/drivers/platform/x86/sony-laptop.c
index d8d0c0bed5e9..ebd7738c2c44 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -3201,7 +3201,7 @@ static int sony_nc_add(struct acpi_device *device)
 			sony_nc_function_setup(device, sony_pf_device);
 	}
 
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor)
+	if (acpi_video_get_backlight_type(false) == acpi_backlight_vendor)
 		sony_nc_backlight_setup();
 
 	/* create sony_pf sysfs attributes related to the SNC device */
diff --git a/drivers/platform/x86/thinkpad_acpi.c
b/drivers/platform/x86/thinkpad_acpi.c
index e6cb4a14cdd4..411679d86308 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3547,7 +3547,7 @@ static int __init hotkey_init(struct ibm_init_struct
*iibm)
 	/* Do not issue duplicate brightness change events to
 	 * userspace. tpacpi_detect_brightness_capabilities() must have
 	 * been called before this point  */
-	if (acpi_video_get_backlight_type() != acpi_backlight_vendor) {
+	if (acpi_video_get_backlight_type(false) != acpi_backlight_vendor) {
 		pr_info("This ThinkPad has standard ACPI backlight brightness control,
supported by the ACPI video driver\n");
 		pr_notice("Disabling thinkpad-acpi brightness events by
default...\n");
 
@@ -6989,7 +6989,7 @@ static int __init brightness_init(struct ibm_init_struct
*iibm)
 		return -ENODEV;
 	}
 
-	if (acpi_video_get_backlight_type() != acpi_backlight_vendor) {
+	if (acpi_video_get_backlight_type(false) != acpi_backlight_vendor) {
 		if (brightness_enable > 1) {
 			pr_info("Standard ACPI backlight interface available, not loading
native one\n");
 			return -ENODEV;
diff --git a/drivers/platform/x86/toshiba_acpi.c
b/drivers/platform/x86/toshiba_acpi.c
index 0fc9e8b8827b..3ea6a1286f0c 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -2889,7 +2889,7 @@ static int toshiba_acpi_setup_backlight(struct
toshiba_acpi_dev *dev)
 	    dmi_check_system(toshiba_vendor_backlight_dmi))
 		acpi_video_set_dmi_backlight_type(acpi_backlight_vendor);
 
-	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
+	if (acpi_video_get_backlight_type(false) != acpi_backlight_vendor)
 		return 0;
 
 	memset(&props, 0, sizeof(props));
diff --git a/include/acpi/video.h b/include/acpi/video.h
index db8548ff03ce..e31afb93379a 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -55,7 +55,7 @@ extern int acpi_video_register(void);
 extern void acpi_video_unregister(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);
+extern enum acpi_backlight_type acpi_video_get_backlight_type(bool native);
 extern void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type);
 /*
  * Note: The value returned by acpi_video_handles_brightness_key_presses()
@@ -73,9 +73,9 @@ static inline int acpi_video_get_edid(struct acpi_device
*device, int type,
 {
 	return -ENODEV;
 }
-static inline enum acpi_backlight_type acpi_video_get_backlight_type(void)
+static inline enum acpi_backlight_type acpi_video_get_backlight_type(bool
native)
 {
-	return acpi_backlight_vendor;
+	return native ? acpi_backlight_native : acpi_backlight_vendor;
 }
 static inline void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type
type)
 {
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 02/14] drm/i915: Don't register backlight when another backlight should be used

Before this commit when we want userspace to use the acpi_video backlight
device we register both the GPU's native backlight device and
acpi_video's
firmware acpi_video# backlight device. This relies on userspace preferring
firmware type backlight devices over native ones.

Registering 2 backlight devices for a single display really is
undesirable, don't register the GPU's native backlight device when
another backlight device should be used.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/gpu/drm/i915/display/intel_backlight.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c
b/drivers/gpu/drm/i915/display/intel_backlight.c
index 98f7ea44042f..582d7f48575d 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -6,6 +6,8 @@
 #include <linux/kernel.h>
 #include <linux/pwm.h>
 
+#include <acpi/video.h>
+
 #include "intel_backlight.h"
 #include "intel_connector.h"
 #include "intel_de.h"
@@ -948,6 +950,11 @@ int intel_backlight_device_register(struct intel_connector
*connector)
 
 	WARN_ON(panel->backlight.max == 0);
 
+	if (acpi_video_get_backlight_type(true) != acpi_backlight_native) {
+		DRM_INFO("Skipping intel_backlight registration\n");
+		return 0;
+	}
+
 	memset(&props, 0, sizeof(props));
 	props.type = BACKLIGHT_RAW;
 
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 03/14] drm/amdgpu: Don't register backlight when another backlight should be used

Before this commit when we want userspace to use the acpi_video backlight
device we register both the GPU's native backlight device and
acpi_video's
firmware acpi_video# backlight device. This relies on userspace preferring
firmware type backlight devices over native ones.

Registering 2 backlight devices for a single display really is
undesirable, don't register the GPU's native backlight device when
another backlight device should be used.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/gpu/drm/Kconfig                           | 1 +
 drivers/gpu/drm/amd/amdgpu/atombios_encoders.c    | 7 +++++++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 +++++++
 3 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f1422bee3dcc..ddbeb2124df7 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -280,6 +280,7 @@ config DRM_AMDGPU
 	select HWMON
 	select BACKLIGHT_CLASS_DEVICE
 	select INTERVAL_TREE
+	select ACPI_VIDEO if ACPI && X86 && INPUT
 	help
 	  Choose this option if you have a recent AMD Radeon graphics card.
 
diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
index a92d86e12718..f9c62cd84a18 100644
--- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
+++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
@@ -26,6 +26,8 @@
 
 #include <linux/pci.h>
 
+#include <acpi/video.h>
+
 #include <drm/drm_crtc_helper.h>
 #include <drm/amdgpu_drm.h>
 #include "amdgpu.h"
@@ -186,6 +188,11 @@ void amdgpu_atombios_encoder_init_backlight(struct
amdgpu_encoder *amdgpu_encode
 	if (!(adev->mode_info.firmware_flags &
ATOM_BIOS_INFO_BL_CONTROLLED_BY_GPU))
 		return;
 
+	if (acpi_video_get_backlight_type(true) != acpi_backlight_native) {
+		DRM_INFO("Skipping amdgpu atom DIG backlight registration\n");
+		return;
+	}
+
 	pdata = kmalloc(sizeof(struct amdgpu_backlight_privdata), GFP_KERNEL);
 	if (!pdata) {
 		DRM_ERROR("Memory allocation failed\n");
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 62139ff35476..a838c7b5d942 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -83,6 +83,8 @@
 #include <drm/drm_vblank.h>
 #include <drm/drm_audio_component.h>
 
+#include <acpi/video.h>
+
 #if defined(CONFIG_DRM_AMD_DC_DCN)
 #include "ivsrcid/dcn/irqsrcs_dcn_1_0.h"
 
@@ -4079,6 +4081,11 @@ amdgpu_dm_register_backlight_device(struct
amdgpu_display_manager *dm)
 	amdgpu_dm_update_backlight_caps(dm, dm->num_of_edps);
 	dm->brightness[dm->num_of_edps] = AMDGPU_MAX_BL_LEVEL;
 
+	if (acpi_video_get_backlight_type(true) != acpi_backlight_native) {
+		DRM_INFO("Skipping amdgpu DM backlight registration\n");
+		return;
+	}
+
 	props.max_brightness = AMDGPU_MAX_BL_LEVEL;
 	props.brightness = AMDGPU_MAX_BL_LEVEL;
 	props.type = BACKLIGHT_RAW;
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 04/14] drm/radeon: Don't register backlight when another backlight should be used

Before this commit when we want userspace to use the acpi_video backlight
device we register both the GPU's native backlight device and
acpi_video's
firmware acpi_video# backlight device. This relies on userspace preferring
firmware type backlight devices over native ones.

Registering 2 backlight devices for a single display really is
undesirable, don't register the GPU's native backlight device when
another backlight device should be used.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/gpu/drm/Kconfig                         | 1 +
 drivers/gpu/drm/radeon/atombios_encoders.c      | 7 +++++++
 drivers/gpu/drm/radeon/radeon_legacy_encoders.c | 7 +++++++
 3 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index ddbeb2124df7..37205953056b 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -258,6 +258,7 @@ config DRM_RADEON
 	select HWMON
 	select BACKLIGHT_CLASS_DEVICE
 	select INTERVAL_TREE
+	select ACPI_VIDEO if ACPI && X86 && INPUT
 	help
 	  Choose this option if you have an ATI Radeon graphics card.  There
 	  are both PCI and AGP versions.  You don't need to choose this to
diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c
b/drivers/gpu/drm/radeon/atombios_encoders.c
index 70bd84b7ef2b..f82577dc25e8 100644
--- a/drivers/gpu/drm/radeon/atombios_encoders.c
+++ b/drivers/gpu/drm/radeon/atombios_encoders.c
@@ -32,6 +32,8 @@
 #include <drm/drm_file.h>
 #include <drm/radeon_drm.h>
 
+#include <acpi/video.h>
+
 #include "atom.h"
 #include "radeon_atombios.h"
 #include "radeon.h"
@@ -211,6 +213,11 @@ void radeon_atom_backlight_init(struct radeon_encoder
*radeon_encoder,
 	if (!(rdev->mode_info.firmware_flags &
ATOM_BIOS_INFO_BL_CONTROLLED_BY_GPU))
 		return;
 
+	if (acpi_video_get_backlight_type(true) != acpi_backlight_native) {
+		DRM_INFO("Skipping radeon atom DIG backlight registration\n");
+		return;
+	}
+
 	pdata = kmalloc(sizeof(struct radeon_backlight_privdata), GFP_KERNEL);
 	if (!pdata) {
 		DRM_ERROR("Memory allocation failed\n");
diff --git a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
index 7fdb77d48d6a..d2180f5c80fa 100644
--- a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
+++ b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
@@ -33,6 +33,8 @@
 #include <drm/drm_util.h>
 #include <drm/radeon_drm.h>
 
+#include <acpi/video.h>
+
 #include "radeon.h"
 #include "radeon_asic.h"
 #include "radeon_legacy_encoders.h"
@@ -389,6 +391,11 @@ void radeon_legacy_backlight_init(struct radeon_encoder
*radeon_encoder,
 		return;
 #endif
 
+	if (acpi_video_get_backlight_type(true) != acpi_backlight_native) {
+		DRM_INFO("Skipping radeon legacy LVDS backlight registration\n");
+		return;
+	}
+
 	pdata = kmalloc(sizeof(struct radeon_backlight_privdata), GFP_KERNEL);
 	if (!pdata) {
 		DRM_ERROR("Memory allocation failed\n");
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 05/14] drm/nouveau: Don't register backlight when another backlight should be used

Before this commit when we want userspace to use the acpi_video backlight
device we register both the GPU's native backlight device and
acpi_video's
firmware acpi_video# backlight device. This relies on userspace preferring
firmware type backlight devices over native ones.

Registering 2 backlight devices for a single display really is
undesirable, don't register the GPU's native backlight device when
another backlight device should be used.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_backlight.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index daf9f87477ba..f56ff797c78c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -34,6 +34,8 @@
 #include <linux/backlight.h>
 #include <linux/idr.h>
 
+#include <acpi/video.h>
+
 #include "nouveau_drv.h"
 #include "nouveau_reg.h"
 #include "nouveau_encoder.h"
@@ -404,6 +406,11 @@ nouveau_backlight_init(struct drm_connector *connector)
 		goto fail_alloc;
 	}
 
+	if (acpi_video_get_backlight_type(true) != acpi_backlight_native) {
+		NV_INFO(drm, "Skipping nv_backlight registration\n");
+		goto fail_alloc;
+	}
+
 	if (!nouveau_get_backlight_name(backlight_name, bl)) {
 		NV_ERROR(drm, "Failed to retrieve a unique name for the backlight
interface\n");
 		goto fail_alloc;
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 06/14] ACPI: video: Drop backlight_device_get_by_type() call from acpi_video_get_backlight_type()

Now that all kms drivers which register native/BACKLIGHT_RAW type backlight
devices on x86/ACPI boards call acpi_video_get_backlight_type(true), with
the native=true value getting cached, there no longer is a need to call
backlight_device_get_by_type(BACKLIGHT_RAW) to see if a native backlight
device is available.

Relying on the cached native_available value not only is simpler, it will
also work correctly in cases where then native backlight registration was
skipped because of the acpi_video_get_backlight_type() return value.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/acpi/video_detect.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index 0a06f0edd298..6caabdf189c9 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -586,8 +586,7 @@ enum acpi_backlight_type acpi_video_get_backlight_type(bool
native)
 	if (!(video_caps & ACPI_VIDEO_BACKLIGHT))
 		return acpi_backlight_vendor;
 
-	if (acpi_osi_is_win8() &&
-	    (native_available || backlight_device_get_by_type(BACKLIGHT_RAW)))
+	if (acpi_osi_is_win8() && native_available)
 		return acpi_backlight_native;
 
 	return acpi_backlight_video;
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 07/14] ACPI: video: Remove acpi_video_bus from list before tearing it down

Move the list_del removing an acpi_video_bus from video_bus_head
on teardown to before the teardown is done, to avoid code iterating
over the video_bus_head list seeing acpi_video_bus objects on there
which are (partly) torn down already.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/acpi/acpi_video.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index cebef3403620..7f48352840bb 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -2114,14 +2114,14 @@ static int acpi_video_bus_remove(struct acpi_device
*device)
 
 	video = acpi_driver_data(device);
 
-	acpi_video_bus_remove_notify_handler(video);
-	acpi_video_bus_unregister_backlight(video);
-	acpi_video_bus_put_devices(video);
-
 	mutex_lock(&video_list_lock);
 	list_del(&video->entry);
 	mutex_unlock(&video_list_lock);
 
+	acpi_video_bus_remove_notify_handler(video);
+	acpi_video_bus_unregister_backlight(video);
+	acpi_video_bus_put_devices(video);
+
 	kfree(video->attached_array);
 	kfree(video);
 
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 08/14] ACPI: video: Simplify acpi_video_unregister_backlight()

When acpi_video_register() has not run yet the video_bus_head will be
empty, so there is no need to check the register_count flag first.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/acpi/acpi_video.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 7f48352840bb..95d4868f6a8c 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -2259,14 +2259,10 @@ void acpi_video_unregister_backlight(void)
 {
 	struct acpi_video_bus *video;
 
-	mutex_lock(&register_count_mutex);
-	if (register_count) {
-		mutex_lock(&video_list_lock);
-		list_for_each_entry(video, &video_bus_head, entry)
-			acpi_video_bus_unregister_backlight(video);
-		mutex_unlock(&video_list_lock);
-	}
-	mutex_unlock(&register_count_mutex);
+	mutex_lock(&video_list_lock);
+	list_for_each_entry(video, &video_bus_head, entry)
+		acpi_video_bus_unregister_backlight(video);
+	mutex_unlock(&video_list_lock);
 }
 
 bool acpi_video_handles_brightness_key_presses(void)
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 09/14] ACPI: video: Make backlight class device registration a separate step

On x86/ACPI boards the acpi_video driver will usually initializing 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.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/acpi/acpi_video.c | 45 ++++++++++++++++++++++++++++++++++++---
 include/acpi/video.h      |  2 ++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 95d4868f6a8c..79e75dc86243 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -31,6 +31,12 @@
 #define ACPI_VIDEO_BUS_NAME		"Video Bus"
 #define ACPI_VIDEO_DEVICE_NAME		"Video Device"
 
+/*
+ * 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.
+ */
+#define ACPI_VIDEO_REGISTER_BACKLIGHT_DELAY	(8 * HZ)
+
 #define MAX_NAME_LEN	20
 
 MODULE_AUTHOR("Bruno Ducrot");
@@ -80,6 +86,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);
 
 /*
@@ -1862,8 +1871,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(false) != acpi_backlight_video)
 		return 0;
 
@@ -2089,7 +2096,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;
@@ -2128,6 +2139,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)
@@ -2238,6 +2254,17 @@ 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.
+	 */
+	schedule_delayed_work(&video_bus_register_backlight_work,
+			      ACPI_VIDEO_REGISTER_BACKLIGHT_DELAY);
+
 leave:
 	mutex_unlock(&register_count_mutex);
 	return ret;
@@ -2248,6 +2275,7 @@ void acpi_video_unregister(void)
 {
 	mutex_lock(&register_count_mutex);
 	if (register_count) {
+		cancel_delayed_work_sync(&video_bus_register_backlight_work);
 		acpi_bus_unregister_driver(&acpi_video_bus);
 		register_count = 0;
 	}
@@ -2255,6 +2283,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 e31afb93379a..b2f7dc1f354a 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(bool native);
@@ -68,6 +69,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)
 {
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 10/14] ACPI: video: Remove code to unregister acpi_video backlight when a native backlight registers

Remove the code to unregister acpi_video backlight devices when
a native backlight device gets registered later.

Now that the acpi_video backlight device registration is a separate step
which runs later, after the drm/kms driver is done setting up its own
native backlight device, it is no longer necessary to monitor for a
native (BACKLIGHT_RAW) device showing up later and to then unregister
the acpi_video backlight device(s).

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/acpi/acpi_video.c   |  2 --
 drivers/acpi/video_detect.c | 36 ------------------------------------
 2 files changed, 38 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 79e75dc86243..20a2638f0ef1 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -89,7 +89,6 @@ 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);
 
 /*
  * Indices in the _BCL method response: the first two items are special,
@@ -2345,7 +2344,6 @@ static int __init acpi_video_init(void)
 
 static void __exit acpi_video_exit(void)
 {
-	acpi_video_detect_exit();
 	acpi_video_unregister();
 }
 
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index 6caabdf189c9..3fa79584981e 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -39,10 +39,6 @@
 
 void acpi_video_unregister_backlight(void);
 
-static bool backlight_notifier_registered;
-static struct notifier_block backlight_nb;
-static struct work_struct backlight_notify_work;
-
 static enum acpi_backlight_type acpi_backlight_cmdline = acpi_backlight_undef;
 static enum acpi_backlight_type acpi_backlight_dmi = acpi_backlight_undef;
 
@@ -516,26 +512,6 @@ static const struct dmi_system_id video_detect_dmi_table[]
= {
 	{ },
 };
 
-/* This uses a workqueue to avoid various locking ordering issues */
-static void acpi_video_backlight_notify_work(struct work_struct *work)
-{
-	if (acpi_video_get_backlight_type(false) != acpi_backlight_video)
-		acpi_video_unregister_backlight();
-}
-
-static int acpi_video_backlight_notify(struct notifier_block *nb,
-				       unsigned long val, void *bd)
-{
-	struct backlight_device *backlight = bd;
-
-	/* A raw bl registering may change video -> native */
-	if (backlight->props.type == BACKLIGHT_RAW &&
-	    val == BACKLIGHT_REGISTERED)
-		schedule_work(&backlight_notify_work);
-
-	return NOTIFY_OK;
-}
-
 /*
  * Determine which type of backlight interface to use on this system,
  * First check cmdline, then dmi quirks, then do autodetect.
@@ -565,12 +541,6 @@ enum acpi_backlight_type acpi_video_get_backlight_type(bool
native)
 		acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
 				    ACPI_UINT32_MAX, find_video, NULL,
 				    &video_caps, NULL);
-		INIT_WORK(&backlight_notify_work,
-			  acpi_video_backlight_notify_work);
-		backlight_nb.notifier_call = acpi_video_backlight_notify;
-		backlight_nb.priority = 0;
-		if (backlight_register_notifier(&backlight_nb) == 0)
-			backlight_notifier_registered = true;
 		init_done = true;
 	}
 	if (native)
@@ -606,9 +576,3 @@ void acpi_video_set_dmi_backlight_type(enum
acpi_backlight_type type)
 		acpi_video_unregister_backlight();
 }
 EXPORT_SYMBOL(acpi_video_set_dmi_backlight_type);
-
-void __exit acpi_video_detect_exit(void)
-{
-	if (backlight_notifier_registered)
-		backlight_unregister_notifier(&backlight_nb);
-}
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 11/14] drm/i915: Call acpi_video_register_backlight()

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.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c
b/drivers/gpu/drm/i915/display/intel_display.c
index 85fbf59e0f58..500659c51e8d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10672,6 +10672,7 @@ void intel_display_driver_register(struct
drm_i915_private *i915)
 	/* Must be done after probing outputs */
 	intel_opregion_register(i915);
 	acpi_video_register();
+	acpi_video_register_backlight();
 
 	intel_audio_init(i915);
 
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 12/14] drm/nouveau: Register ACPI video backlight when nv_backlight registration fails

Typically the acpi_video driver will initialize before nouveau, which
used to cause /sys/class/backlight/acpi_video0 to get registered and then
nouveau would register its own nv_backlight device later. After which
the drivers/acpi/video_detect.c code unregistered the acpi_video0 device
to avoid there being 2 backlight devices.

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() when native backlight
device registration has failed / was skipped to ensure that there is a
backlight device available before the drm_device gets registered with
userspace.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_backlight.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index f56ff797c78c..0ae8793357a4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -436,6 +436,13 @@ nouveau_backlight_init(struct drm_connector *connector)
 
 fail_alloc:
 	kfree(bl);
+	/*
+	 * If we get here we have an internal panel, but no nv_backlight,
+	 * try registering an ACPI video backlight device instead.
+	 */
+	if (ret == 0)
+		acpi_video_register_backlight();
+
 	return ret;
 }
 
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 13/14] drm/amdgpu: Register ACPI video backlight when skipping amdgpu backlight registration

Typically the acpi_video driver will initialize before amdgpu, which
used to cause /sys/class/backlight/acpi_video0 to get registered and then
amdgpu would register its own amdgpu_bl# device later. After which
the drivers/acpi/video_detect.c code unregistered the acpi_video0 device
to avoid there being 2 backlight devices.

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() when amdgpu skips
registering its own backlight device because of either the firmware_flags
or the acpi_video_get_backlight_type() return value. This ensures that
if the acpi_video backlight device should be used, it will be available
before the amdgpu drm_device gets registered with userspace.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/atombios_encoders.c    | 9 +++++++--
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
index f9c62cd84a18..26f638ab7a5f 100644
--- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
+++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
@@ -186,11 +186,11 @@ void amdgpu_atombios_encoder_init_backlight(struct
amdgpu_encoder *amdgpu_encode
 		return;
 
 	if (!(adev->mode_info.firmware_flags &
ATOM_BIOS_INFO_BL_CONTROLLED_BY_GPU))
-		return;
+		goto register_acpi_backlight;
 
 	if (acpi_video_get_backlight_type(true) != acpi_backlight_native) {
 		DRM_INFO("Skipping amdgpu atom DIG backlight registration\n");
-		return;
+		goto register_acpi_backlight;
 	}
 
 	pdata = kmalloc(sizeof(struct amdgpu_backlight_privdata), GFP_KERNEL);
@@ -227,6 +227,11 @@ void amdgpu_atombios_encoder_init_backlight(struct
amdgpu_encoder *amdgpu_encode
 error:
 	kfree(pdata);
 	return;
+
+register_acpi_backlight:
+	/* Try registering an ACPI video backlight device instead. */
+	acpi_video_register_backlight();
+	return;
 }
 
 void
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a838c7b5d942..06baa4f27680 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4083,6 +4083,8 @@ amdgpu_dm_register_backlight_device(struct
amdgpu_display_manager *dm)
 
 	if (acpi_video_get_backlight_type(true) != acpi_backlight_native) {
 		DRM_INFO("Skipping amdgpu DM backlight registration\n");
+		/* Try registering an ACPI video backlight device instead. */
+		acpi_video_register_backlight();
 		return;
 	}
 
-- 
2.36.0

Hans de Goede

2022-May-17 15:23 UTC

head link

[Nouveau] [PATCH 14/14] drm/radeon: Register ACPI video backlight when skipping radeon backlight registration

Typically the acpi_video driver will initialize before radeon, which
used to cause /sys/class/backlight/acpi_video0 to get registered and then
radeon would register its own radeon_bl# device later. After which
the drivers/acpi/video_detect.c code unregistered the acpi_video0 device
to avoid there being 2 backlight devices.

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() when radeon skips
registering its own backlight device because of e.g. the firmware_flags
or the acpi_video_get_backlight_type() return value. This ensures that
if the acpi_video backlight device should be used, it will be available
before the radeon drm_device gets registered with userspace.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 drivers/gpu/drm/radeon/radeon_encoders.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_encoders.c
b/drivers/gpu/drm/radeon/radeon_encoders.c
index 46549d5179ee..c1cbebb51be1 100644
--- a/drivers/gpu/drm/radeon/radeon_encoders.c
+++ b/drivers/gpu/drm/radeon/radeon_encoders.c
@@ -30,6 +30,8 @@
 #include <drm/drm_device.h>
 #include <drm/radeon_drm.h>
 
+#include <acpi/video.h>
+
 #include "radeon.h"
 #include "radeon_atombios.h"
 #include "radeon_legacy_encoders.h"
@@ -167,7 +169,7 @@ static void radeon_encoder_add_backlight(struct
radeon_encoder *radeon_encoder,
 		return;
 
 	if (radeon_backlight == 0) {
-		return;
+		use_bl = false;
 	} else if (radeon_backlight == 1) {
 		use_bl = true;
 	} else if (radeon_backlight == -1) {
@@ -193,6 +195,13 @@ static void radeon_encoder_add_backlight(struct
radeon_encoder *radeon_encoder,
 		else
 			radeon_legacy_backlight_init(radeon_encoder, connector);
 	}
+
+	/*
+	 * If there is no native backlight device (which may happen even when
+	 * use_bl==true) try registering an ACPI video backlight device instead.
+	 */
+	if (!rdev->mode_info.bl_encoder)
+		acpi_video_register_backlight();
 }
 
 void
-- 
2.36.0

Jani Nikula

2022-May-18 08:44 UTC

head link

[Nouveau] [PATCH 00/14] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display

On Tue, 17 May 2022, Hans de Goede <hdegoede at redhat.com>
wrote:> Hi All,
>
> As mentioned in my RFC titled "drm/kms: control display brightness
through
> drm_connector properties":
> https://lore.kernel.org/dri-devel/0d188965-d809-81b5-74ce-7d30c49fee2d at
redhat.com/
>
> The first step towards this is to deal with some existing technical debt
> in backlight handling on x86/ACPI boards, specifically we need to stop
> registering multiple /sys/class/backlight devs for a single display.
I guess my question here is, how do you know it's for a *single*
display?

There are already designs out there with two flat panels, with
independent brightness controls. They're still rare and I don't think we
handle them very well. But we've got code to register multiple native
backlight interfaces, see e.g. commit 20f85ef89d94 ("drm/i915/backlight:
use unique backlight device names").

So imagine a design where one of the panels needs backlight control via
ACPI and the other via native backlight control. Granted, I don't know
if one exists, but I think it's very much in the realm of possible
things the OEMs might do. For example, have an EC PWM for primary panel
backlight, and use GPU PWM for secondary. How do you know you actually
do need to register two interfaces?

I'm fine with dealing with such cases as they arise to avoid
over-engineering up front, but I also don't want us to completely paint
ourselves in a corner either.

BR,
Jani.

>
> This series implements my RFC describing my plan for these cleanups:
> https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158 at
redhat.com/
>
> Specifically patches 1-6 implement the "Fixing kms driver
unconditionally
> register their "native" backlight dev" part.
>
> And patches 7-14 implement the "Fixing acpi_video0 getting registered
for
> a brief time" time.
>
> Note this series does not deal yet with the "Other issues" part,
I plan
> to do a follow up series for that.
>
> The changes in this series are good to have regardless of the further
> "drm/kms: control display brightness through drm_connector
properties"
> plans. So I plan to push these upstream once they are ready (once
> reviewed). Since this crosses various subsystems / touches multiple
> kms drivers my plan is to provide an immutable branch based on say
> 5.19-rc1 and then have that get merged into all the relevant trees.
>
> Please review.
>
> Regards,
>
> Hans
>
>
> Hans de Goede (14):
>   ACPI: video: Add a native function parameter to
>     acpi_video_get_backlight_type()
>   drm/i915: Don't register backlight when another backlight should be
>     used
>   drm/amdgpu: Don't register backlight when another backlight should be
>     used
>   drm/radeon: Don't register backlight when another backlight should be
>     used
>   drm/nouveau: Don't register backlight when another backlight should
be
>     used
>   ACPI: video: Drop backlight_device_get_by_type() call from
>     acpi_video_get_backlight_type()
>   ACPI: video: Remove acpi_video_bus from list before tearing it down
>   ACPI: video: Simplify acpi_video_unregister_backlight()
>   ACPI: video: Make backlight class device registration a separate step
>   ACPI: video: Remove code to unregister acpi_video backlight when a
>     native backlight registers
>   drm/i915: Call acpi_video_register_backlight()
>   drm/nouveau: Register ACPI video backlight when nv_backlight
>     registration fails
>   drm/amdgpu: Register ACPI video backlight when skipping amdgpu
>     backlight registration
>   drm/radeon: Register ACPI video backlight when skipping radeon
>     backlight registration
>
>  drivers/acpi/acpi_video.c                     | 69 ++++++++++++++-----
>  drivers/acpi/video_detect.c                   | 53 +++-----------
>  drivers/gpu/drm/Kconfig                       |  2 +
>  .../gpu/drm/amd/amdgpu/atombios_encoders.c    | 14 +++-
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  9 +++
>  .../gpu/drm/i915/display/intel_backlight.c    |  7 ++
>  drivers/gpu/drm/i915/display/intel_display.c  |  1 +
>  drivers/gpu/drm/i915/display/intel_opregion.c |  2 +-
>  drivers/gpu/drm/nouveau/nouveau_backlight.c   | 14 ++++
>  drivers/gpu/drm/radeon/atombios_encoders.c    |  7 ++
>  drivers/gpu/drm/radeon/radeon_encoders.c      | 11 ++-
>  .../gpu/drm/radeon/radeon_legacy_encoders.c   |  7 ++
>  drivers/platform/x86/acer-wmi.c               |  2 +-
>  drivers/platform/x86/asus-laptop.c            |  2 +-
>  drivers/platform/x86/asus-wmi.c               |  4 +-
>  drivers/platform/x86/compal-laptop.c          |  2 +-
>  drivers/platform/x86/dell/dell-laptop.c       |  2 +-
>  drivers/platform/x86/eeepc-laptop.c           |  2 +-
>  drivers/platform/x86/fujitsu-laptop.c         |  4 +-
>  drivers/platform/x86/ideapad-laptop.c         |  2 +-
>  drivers/platform/x86/intel/oaktrail.c         |  2 +-
>  drivers/platform/x86/msi-laptop.c             |  2 +-
>  drivers/platform/x86/msi-wmi.c                |  2 +-
>  drivers/platform/x86/samsung-laptop.c         |  2 +-
>  drivers/platform/x86/sony-laptop.c            |  2 +-
>  drivers/platform/x86/thinkpad_acpi.c          |  4 +-
>  drivers/platform/x86/toshiba_acpi.c           |  2 +-
>  include/acpi/video.h                          |  8 ++-
>  28 files changed, 156 insertions(+), 84 deletions(-)
-- 
Jani Nikula, Intel Open Source Graphics Center

Nouveau - May 2022 - [PATCH 00/14] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display

[Nouveau] [PATCH 00/14] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display

[Nouveau] [PATCH 01/14] ACPI: video: Add a native function parameter to acpi_video_get_backlight_type()

[Nouveau] [PATCH 02/14] drm/i915: Don't register backlight when another backlight should be used

[Nouveau] [PATCH 03/14] drm/amdgpu: Don't register backlight when another backlight should be used

[Nouveau] [PATCH 04/14] drm/radeon: Don't register backlight when another backlight should be used

[Nouveau] [PATCH 05/14] drm/nouveau: Don't register backlight when another backlight should be used

[Nouveau] [PATCH 06/14] ACPI: video: Drop backlight_device_get_by_type() call from acpi_video_get_backlight_type()

[Nouveau] [PATCH 07/14] ACPI: video: Remove acpi_video_bus from list before tearing it down

[Nouveau] [PATCH 08/14] ACPI: video: Simplify acpi_video_unregister_backlight()

[Nouveau] [PATCH 09/14] ACPI: video: Make backlight class device registration a separate step

[Nouveau] [PATCH 10/14] ACPI: video: Remove code to unregister acpi_video backlight when a native backlight registers

[Nouveau] [PATCH 11/14] drm/i915: Call acpi_video_register_backlight()

[Nouveau] [PATCH 12/14] drm/nouveau: Register ACPI video backlight when nv_backlight registration fails

[Nouveau] [PATCH 13/14] drm/amdgpu: Register ACPI video backlight when skipping amdgpu backlight registration

[Nouveau] [PATCH 14/14] drm/radeon: Register ACPI video backlight when skipping radeon backlight registration

[Nouveau] [PATCH 00/14] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display