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

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

[Hans de Goede]

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@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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Hans de Goede]

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@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

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

[Jani Nikula]

On Tue, 17 May 2022, Hans de Goede <hdegoede@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@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@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

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

[Jani Nikula]

On Tue, 17 May 2022, Hans de Goede <hdegoede@redhat.com> wrote:
> 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.


> 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.
>   */

Frankly, I think the boolean native parameter here, and at the call
sites, is confusing, and the slightly different explanations in the
commit message and comment here aren't helping.

I suggest adding a separate function that the native backlight drivers
should use. I think it's more obvious all around, and easier to document
too.


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Hans de Goede]

Hi,

On 5/18/22 10:44, Jani Nikula wrote:
> On Tue, 17 May 2022, Hans de Goede <hdegoede@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@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?

On x86/ACPI devices this is all driven by acpi_video_get_backlight_type() /
by the drivers/acpi/video_detect.c code. That code already will break on
systems where there are 2 backlight controls, with one being ACPI based
and the other a native GPU PWM backlight device.

In this scenario as soon as the native GPU PWM backlight device shows up
then, if acpi_video_get_backlight_type()==native, video_detect.c will
currently unregister the acpi_video# backlight device(s) since userspace
prefers the firmware type over the native type, so for userspace to
actually honor the acpi_video_get_backlight_type()==native we need to get
the acpi_video# backlight device "out of the way" which is currently
handled by unregistering it.

Note in a way we already have a case where userspace sees 2 panels,
in hybrid laptop setups with a mux and on those systems we may see
either 2 native backlight devices; or 2 native backlight devices +
2 acpi_video backlight devices with userspace preferring the ACPI
ones.

Also note that userspace already has code to detect if the related
panel is active (iow which way the mux between the GPU and the panels
points) and then uses that backlight device. Userspace here very
much assumes a single panel though.

> 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.

Right. Note that the current code (both with and without this patchset)
already will work fine from a kernel pov as long as both panels
are either using native GPU PWM or are both using ACPI. But if we
ever get a mix then this will need special handling.

Note that all userspace code I know is currently hardcoded
to assume a single panel. Userspace already picks one preferred
device under /sys/class/backlight and ignores the rest. Actually
atm userspace must behave this way, because on x86/ACPI boards we
do often register multiple backlight devices for a single panel.

So in a way moving to registering only a single backlight device
prepares for actually having multiple panels work.

Also keep in mind that this is preparation work for making the
panel brightness a drm_connector property. When the panel uses
a backlight device other then the native GPU PWM to control the
brightness then the helper code for this needs to have a way to
select which backlight_device to use then and the non native
types are not "linked" to a specific connector so in this case
we really need there to be only 1 backlight device registered
so that the code looking up the (non native) backlight device
for the connector gets the right one and not merely the one
which happened to get registered first.

And I believe that having the panel brightness be a drm_connector
property is the way to make it possible for userspace to deal
with the multiple panels which each have a separate brightness
control case.

Regards,

Hans





> 
> BR,
> Jani.
> 
> 
>>
>> This series implements my RFC describing my plan for these cleanups:
>> https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158@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(-)
> 

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

[Hans de Goede]

Hi,

On 5/18/22 10:55, Jani Nikula wrote:
> On Tue, 17 May 2022, Hans de Goede <hdegoede@redhat.com> wrote:
>> 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.
> 
> 
>> 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.
>>   */
> 
> Frankly, I think the boolean native parameter here, and at the call
> sites, is confusing, and the slightly different explanations in the
> commit message and comment here aren't helping.

Can you elaborate the "slightly different explanations in the
commit message and comment" part a bit (so that I can fix this) ?

> I suggest adding a separate function that the native backlight drivers
> should use. I think it's more obvious all around, and easier to document
> too.

Code wise I think this would mean renaming the original and
then adding 2 wrappers, but that is fine with me. I've no real
preference either way and I'm happy with adding a new variant of
acpi_video_get_backlight_type() for the native backlight drivers
any suggestion for a name ?

Regards,

Hans

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

[Jani Nikula]

On Wed, 18 May 2022, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 5/18/22 10:44, Jani Nikula wrote:
>> On Tue, 17 May 2022, Hans de Goede <hdegoede@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@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?
>
> On x86/ACPI devices this is all driven by acpi_video_get_backlight_type() /
> by the drivers/acpi/video_detect.c code. That code already will break on
> systems where there are 2 backlight controls, with one being ACPI based
> and the other a native GPU PWM backlight device.
>
> In this scenario as soon as the native GPU PWM backlight device shows up
> then, if acpi_video_get_backlight_type()==native, video_detect.c will
> currently unregister the acpi_video# backlight device(s) since userspace
> prefers the firmware type over the native type, so for userspace to
> actually honor the acpi_video_get_backlight_type()==native we need to get
> the acpi_video# backlight device "out of the way" which is currently
> handled by unregistering it.
>
> Note in a way we already have a case where userspace sees 2 panels,
> in hybrid laptop setups with a mux and on those systems we may see
> either 2 native backlight devices; or 2 native backlight devices +
> 2 acpi_video backlight devices with userspace preferring the ACPI
> ones.
>
> Also note that userspace already has code to detect if the related
> panel is active (iow which way the mux between the GPU and the panels
> points) and then uses that backlight device. Userspace here very
> much assumes a single panel though.
>
>> 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.
>
> Right. Note that the current code (both with and without this patchset)
> already will work fine from a kernel pov as long as both panels
> are either using native GPU PWM or are both using ACPI. But if we
> ever get a mix then this will need special handling.
>
> Note that all userspace code I know is currently hardcoded
> to assume a single panel. Userspace already picks one preferred
> device under /sys/class/backlight and ignores the rest. Actually
> atm userspace must behave this way, because on x86/ACPI boards we
> do often register multiple backlight devices for a single panel.
>
> So in a way moving to registering only a single backlight device
> prepares for actually having multiple panels work.
>
> Also keep in mind that this is preparation work for making the
> panel brightness a drm_connector property. When the panel uses
> a backlight device other then the native GPU PWM to control the
> brightness then the helper code for this needs to have a way to
> select which backlight_device to use then and the non native
> types are not "linked" to a specific connector so in this case
> we really need there to be only 1 backlight device registered
> so that the code looking up the (non native) backlight device
> for the connector gets the right one and not merely the one
> which happened to get registered first.
>
> And I believe that having the panel brightness be a drm_connector
> property is the way to make it possible for userspace to deal
> with the multiple panels which each have a separate brightness
> control case.

Agreed.

Thanks for the explanations and recording them here.

BR,
Jani.

>
> Regards,
>
> Hans
>
>
>
>
>
>> 
>> BR,
>> Jani.
>> 
>> 
>>>
>>> This series implements my RFC describing my plan for these cleanups:
>>> https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158@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

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

[Lyude Paul]

Reviewed-by: Lyude Paul <lyude@redhat.com>

Also, ack on this being pushed to drm-misc, along with any other patches I r-b

On Tue, 2022-05-17 at 17:23 +0200, Hans de Goede wrote:
> 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@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;

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

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

[Lyude Paul]

On Tue, 2022-05-17 at 17:23 +0200, Hans de Goede wrote:
> 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@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();

Assuming we don't need to return the value of acpi_video_register_backlight()
here:

Reviewed-by: Lyude Paul <lyude@redhat.com>

> +
>         return ret;
>  }
>  

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

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

[Jani Nikula]

On Wed, 18 May 2022, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 5/18/22 10:55, Jani Nikula wrote:
>> On Tue, 17 May 2022, Hans de Goede <hdegoede@redhat.com> wrote:
>>> 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.

Regarding the question below, this is the part that throws me off.

>>>
>>> Note that all current callers are updated to pass false for the new
>>> parameter, so this change in itself causes no functional changes.
>> 
>> 
>>> 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.
>>>   */
>> 
>> Frankly, I think the boolean native parameter here, and at the call
>> sites, is confusing, and the slightly different explanations in the
>> commit message and comment here aren't helping.
>
> Can you elaborate the "slightly different explanations in the
> commit message and comment" part a bit (so that I can fix this) ?
>
>> I suggest adding a separate function that the native backlight drivers
>> should use. I think it's more obvious all around, and easier to document
>> too.
>
> Code wise I think this would mean renaming the original and
> then adding 2 wrappers, but that is fine with me. I've no real
> preference either way and I'm happy with adding a new variant of
> acpi_video_get_backlight_type() for the native backlight drivers
> any suggestion for a name ?

Alternatively, do the native backlight drivers have any need for the
actual backlight type information from acpi? They only need to be able
to ask if they should register themselves, right?

I understand this sounds like bikeshedding, but I'm trying to avoid
duplicating the conditions in the drivers where a single predicate
function call could be sufficient, and the complexity could be hidden in
acpi.

	if (!acpi_video_backlight_use_native())
		return;

Perhaps all the drivers/platform/x86/* backlight drivers could use:

	if (acpi_video_backlight_use_vendor())
		...

You can still use the native parameter etc. internally, but just hide
the details from everyone else, and, hopefully, make it harder for them
to do silly things?

BR,
Jani.


>
> Regards,
>
> Hans
>

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Daniel Dadap]

On 5/17/22 10:23, Hans de Goede wrote:
> 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.


If I'm understanding this correctly, it seems we will want to call 
acpi_video_register_backlight() from the NVIDIA proprietary driver in a 
fallback path in case the driver's own GPU-controlled backlight handler 
either should not be used, or fails to register. That sounds reasonable 
enough, but I'm not sure what should be done about drivers like 
nvidia-wmi-ec-backlight, which are independent of the GPU hardware, and 
wouldn't be part of the acpi_video driver, either. There are a number of 
other similar vendor-y/platform-y type backlight drivers in 
drivers/video/backlight and drivers/platform/x86 that I think would be 
in a similar situation.

 From a quick skim of the ACPI video driver, it seems that perhaps 
nvidia-wmi-ec-backlight is missing a call to 
acpi_video_set_dmi_backlight_type(), perhaps with the 
acpi_backlight_vendor value? But I'm not familiar enough with this code 
to be sure that nobody will be checking acpi_video_get_backlight_type() 
before nvidia-wmi-ec-backlight loads. I'll take a closer look to try to 
convince myself that it makes sense.


> 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.


It sounds like maybe everything should be fine as long as 
nvidia-wmi-ec-backlight (and other vendor-y/platform-y type drivers) 
gets everything set up before the delayed work which calls 
acpi_video_register_backlight()? But then is it really necessary to 
explicitly call acpi_video_register_backlight() from the DRM drivers if 
it's going to be called later if no GPU driver registered a backlight 
handler, anyway? Then we'd just need to make sure that the iGPU and dGPU 
drivers won't attempt to register a backlight handler on systems where a 
vendor-y/platform-y driver is supposed to handle the backlight instead, 
which sounds like it has the potential to be quite messy.

Recall that on at least one system, both amdgpu and the NVIDIA 
proprietary driver registered a handler even when it shouldn't: 
https://patchwork.kernel.org/project/platform-driver-x86/patch/20220316203325.2242536-1-ddadap@nvidia.com/ 
- I didn't have direct access to this system, but the fact that the 
NVIDIA driver registered a handler was almost certainly a bug in either 
the driver or the system's firmware (on other systems with the same type 
of backlight hardware, NVIDIA does not register a handler), and I 
imagine the same is true of the amdgpu driver. In all likelihood nouveau 
would have probably tried to register one too; I am not certain whether 
the person who reported the issue to me had tested with nouveau. I'm not 
convinced that the GPU drivers can reliably determine whether or not 
they are supposed to register, but maybe cases where they aren't, such 
as the system mentioned above, are supposed to be handled in a quirks 
table somewhere.


> Signed-off-by: Hans de Goede <hdegoede@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)
>   {

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

[Daniel Dadap]

[-- Attachment #1: Type: text/plain, Size: 11700 bytes --]

On 5/20/22 16:41, Daniel Dadap wrote:
>
> On 5/17/22 10:23, Hans de Goede wrote:
>> 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.
>
>
> If I'm understanding this correctly, it seems we will want to call 
> acpi_video_register_backlight() from the NVIDIA proprietary driver in 
> a fallback path in case the driver's own GPU-controlled backlight 
> handler either should not be used, or fails to register. That sounds 
> reasonable enough, but I'm not sure what should be done about drivers 
> like nvidia-wmi-ec-backlight, which are independent of the GPU 
> hardware, and wouldn't be part of the acpi_video driver, either. There 
> are a number of other similar vendor-y/platform-y type backlight 
> drivers in drivers/video/backlight and drivers/platform/x86 that I 
> think would be in a similar situation.
>
> From a quick skim of the ACPI video driver, it seems that perhaps 
> nvidia-wmi-ec-backlight is missing a call to 
> acpi_video_set_dmi_backlight_type(), perhaps with the 
> acpi_backlight_vendor value? But I'm not familiar enough with this 
> code to be sure that nobody will be checking 
> acpi_video_get_backlight_type() before nvidia-wmi-ec-backlight loads. 
> I'll take a closer look to try to convince myself that it makes sense.
>
>
>> 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.
>
>
> It sounds like maybe everything should be fine as long as 
> nvidia-wmi-ec-backlight (and other vendor-y/platform-y type drivers) 
> gets everything set up before the delayed work which calls 
> acpi_video_register_backlight()? But then is it really necessary to 
> explicitly call acpi_video_register_backlight() from the DRM drivers 
> if it's going to be called later if no GPU driver registered a 
> backlight handler, anyway? Then we'd just need to make sure that the 
> iGPU and dGPU drivers won't attempt to register a backlight handler on 
> systems where a vendor-y/platform-y driver is supposed to handle the 
> backlight instead, which sounds like it has the potential to be quite 
> messy.
>

Ah, I see you addressed this concern in your RFC (sorry for missing 
that, earlier):

> The above only takes native vs acpi_video backlight issues into
> account, there are also a couple of other scenarios which may
> lead to multiple backlight-devices getting registered:
>
> 1) Apple laptops using the apple_gmux driver
> 2) Nvidia laptops using the (new) nvidia-wmi-ec-backlight driver
> 3) drivers/platform/x86 drivers calling acpi_video_set_dmi_backlight_type()
>     to override the normal acpi_video_get_backlight_type() heuristics after
>     the native/acpi_video drivers have already loaded
>
> The plan for 1) + 2) is to extend the acpi_backlight_type enum with
> acpi_backlight_gmux and acpi_backlight_nvidia_wmi_ec values and to add
> detection of these 2 to acpi_video_get_backlight_type().

Is there a reason these shouldn't have the same, generic, type, rather 
than separate, driver-specific ones? I don't foresee any situation where 
a system would need to use these two particular drivers simultaneously. 
Multiple DRM drivers can coexist on the same system, and even though the 
goal here is to remove the existing "multiple backlight interfaces for 
the same panel" situation, there shouldn't be any reason why more than 
one DRM driver couldn't register backlight handlers at the same time, so 
long as they are driving distinct panels. If we don't need separate 
backlight types for individual DRM drivers, why should we have them for 
apple_gmux and nvidia_wmi_ec_backlight?

I still think that relying on the GPU drivers to correctly know whether 
they should register their backlight handlers when a platform-level 
device is supposed to register one instead might be easier said than 
done, especially on systems where the same panel could potentially be 
driven by more than one GPU (but not at the same time).

> Recall that on at least one system, both amdgpu and the NVIDIA 
> proprietary driver registered a handler even when it shouldn't: 
> https://patchwork.kernel.org/project/platform-driver-x86/patch/20220316203325.2242536-1-ddadap@nvidia.com/ 
> - I didn't have direct access to this system, but the fact that the 
> NVIDIA driver registered a handler was almost certainly a bug in 
> either the driver or the system's firmware (on other systems with the 
> same type of backlight hardware, NVIDIA does not register a handler), 
> and I imagine the same is true of the amdgpu driver. In all likelihood 
> nouveau would have probably tried to register one too; I am not 
> certain whether the person who reported the issue to me had tested 
> with nouveau. I'm not convinced that the GPU drivers can reliably 
> determine whether or not they are supposed to register, but maybe 
> cases where they aren't, such as the system mentioned above, are 
> supposed to be handled in a quirks table somewhere.
>
>
>> Signed-off-by: Hans de Goede <hdegoede@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)
>>   {

[-- Attachment #2: Type: text/html, Size: 17357 bytes --]

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

[Hans de Goede]

Hi,

On 5/24/22 01:25, Daniel Dadap wrote:
> On 5/20/22 16:41, Daniel Dadap wrote:
>>
>> On 5/17/22 10:23, Hans de Goede wrote:
>>> 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.
>>
>>
>> If I'm understanding this correctly, it seems we will want to call acpi_video_register_backlight() from the NVIDIA proprietary driver in a fallback path in case the driver's own GPU-controlled backlight handler either should not be used, or fails to register. That sounds reasonable enough, but I'm not sure what should be done about drivers like nvidia-wmi-ec-backlight, which are independent of the GPU hardware, and wouldn't be part of the acpi_video driver, either. There are a number of other similar vendor-y/platform-y type backlight drivers in drivers/video/backlight and drivers/platform/x86 that I think would be in a similar situation.
>>
>> From a quick skim of the ACPI video driver, it seems that perhaps nvidia-wmi-ec-backlight is missing a call to acpi_video_set_dmi_backlight_type(), perhaps with the acpi_backlight_vendor value? But I'm not familiar enough with this code to be sure that nobody will be checking acpi_video_get_backlight_type() before nvidia-wmi-ec-backlight loads. I'll take a closer look to try to convince myself that it makes sense.
>>
>>
>>> 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.
>>
>>
>> It sounds like maybe everything should be fine as long as nvidia-wmi-ec-backlight (and other vendor-y/platform-y type drivers) gets everything set up before the delayed work which calls acpi_video_register_backlight()? But then is it really necessary to explicitly call acpi_video_register_backlight() from the DRM drivers if it's going to be called later if no GPU driver registered a backlight handler, anyway? Then we'd just need to make sure that the iGPU and dGPU drivers won't attempt to register a backlight handler on systems where a vendor-y/platform-y driver is supposed to handle the backlight instead, which sounds like it has the potential to be quite messy.
>>
> 
> Ah, I see you addressed this concern in your RFC (sorry for missing that, earlier):
> 
>> The above only takes native vs acpi_video backlight issues into
>> account, there are also a couple of other scenarios which may
>> lead to multiple backlight-devices getting registered:
>>
>> 1) Apple laptops using the apple_gmux driver
>> 2) Nvidia laptops using the (new) nvidia-wmi-ec-backlight driver
>> 3) drivers/platform/x86 drivers calling acpi_video_set_dmi_backlight_type()
>>    to override the normal acpi_video_get_backlight_type() heuristics after
>>    the native/acpi_video drivers have already loaded
>>
>> The plan for 1) + 2) is to extend the acpi_backlight_type enum with
>> acpi_backlight_gmux and acpi_backlight_nvidia_wmi_ec values and to add
>> detection of these 2 to acpi_video_get_backlight_type().
> 
> Is there a reason these shouldn't have the same, generic, type, rather than separate, driver-specific ones?

In case it is not clear, this needs to be separate from "vendor" because vendor is
meant for the old (often pre windows XP) vendor specific BIOS interfaces used
by the likes of dell-laptopo, thinkpad_acpi, etc. And acpi_video_get_backlight_type()
returns vendor when it cannot find any other types, iow it is the type of
last resort.

So vendor is the fallback where as nvidia_wmi_ec and apple_gmux both
must take precedence over anything else if detected.

As for why not have a single type for nvidia_wmi_ec and apple_gmux,
these will have 2 separate detection helper functions, so it seems
cleaner to me to use 2 separate types to match this.
Most drivers check for type != my-type, so an extra type does not hurt.

> I don't foresee any situation where a system would need to use these two particular drivers simultaneously.

Agreed.

> Multiple DRM drivers can coexist on the same system, and even though the goal here is to remove the existing "multiple backlight interfaces for the same panel" situation, there shouldn't be any reason why more than one DRM driver couldn't register backlight handlers at the same time, so long as they are driving distinct panels. If we don't need separate backlight types for individual DRM drivers, why should we have them for apple_gmux and nvidia_wmi_ec_backlight?

I don't think we need them, but as said since they use separate detection
methods, it just feels cleaner.

The drivers/acpi/video_detect.c code at the moment has the following detection
methods:

1. Check if any GPU drivers have *told* it that the GPU driver will register
a GPU native backlight device for the panel (either direct PWM driving or
over DP aux, etc.)

2. Check if the ACPI tables have the ACPI video backlight control bits

3. None of the above, assume vendor.

For nvidia-wmi-ec and apple-gmux 2 separate detection helper functions +
steps will get added and as said it just feels cleaner to have 2
separate types to match.

> I still think that relying on the GPU drivers to correctly know whether they should register their backlight handlers when a platform-level device is supposed to register one instead might be easier said than done, especially on systems where the same panel could potentially be driven by more than one GPU (but not at the same time).

ATM the GPU drivers unconditionally register their native
backlight device if they believe (e.g. the video bios tables say so)
they can control the backlight.

And then in the case of e.g. windows XP era laptops, where often
the EC was still used, the GPU drivers atm rely on acpi_video also
registering a backlight device and userspace then preferring that one.

IOW atm the native GPU drivers rely on userspace ignoring their
backlight device if another one is present.

The adding of something like e.g.:

	if (acpi_video_get_backlight_type(true) != acpi_backlight_native) {
		NV_INFO(drm, "Skipping nv_backlight registration\n");
		goto fail_alloc;
	}

To the code-paths doing the backlight-device registration, skipping
tthe registration just replaces the "lets hope userspace ignores this
if necessary" with outright skipping the registration of the sysfs
backlight class device in the cases where before we would hope
userspace behaves as expected.

Also note that this just skips the registration of the sysfs class
device, any prep work is still done, so as to not cause any behavior
changes from the pov of which GPU registers get poked.

>> Recall that on at least one system, both amdgpu and the NVIDIA proprietary driver registered a handler even when it shouldn't: https://patchwork.kernel.org/project/platform-driver-x86/patch/20220316203325.2242536-1-ddadap@nvidia.com/ - I didn't have direct access to this system, but the fact that the NVIDIA driver registered a handler was almost certainly a bug in either the driver or the system's firmware (on other systems with the same type of backlight hardware, NVIDIA does not register a handler), and I imagine the same is true of the amdgpu driver. In all likelihood nouveau would have probably tried to register one too; I am not certain whether the person who reported the issue to me had tested with nouveau. I'm not convinced that the GPU drivers can reliably determine whether or not they are supposed to register, but maybe cases where they aren't, such as the system mentioned above, are supposed to be handled in a quirks table somewhere.

Right video_detect.c already has a DMI table for this which
overrides the value returned by acpi_video_get_backlight_type().

Although in this specific case it seems we may want
acpi_video_get_backlight_type() to return native when
called from the amdgpu driver and none when called from
the nvidia/nouveau driver ?   That is not supported atm, but if
necessary it seems reasonable to:

1. Add a "const char *driver_name" to acpi_video_get_backlight_type()
and maybe agree on nouveau and nvidia to pass the same value.

2. Extend the quirk mechanism to allow using the driver_name to
return different results to different drivers.

Regards,

Hans




>>
>>
>>> Signed-off-by: Hans de Goede <hdegoede@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)
>>>   {

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

[Daniel Vetter]

On Wed, May 18, 2022 at 01:12:33PM +0300, Jani Nikula wrote:
> On Wed, 18 May 2022, Hans de Goede <hdegoede@redhat.com> wrote:
> > Hi,
> >
> > On 5/18/22 10:44, Jani Nikula wrote:
> >> On Tue, 17 May 2022, Hans de Goede <hdegoede@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@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?
> >
> > On x86/ACPI devices this is all driven by acpi_video_get_backlight_type() /
> > by the drivers/acpi/video_detect.c code. That code already will break on
> > systems where there are 2 backlight controls, with one being ACPI based
> > and the other a native GPU PWM backlight device.
> >
> > In this scenario as soon as the native GPU PWM backlight device shows up
> > then, if acpi_video_get_backlight_type()==native, video_detect.c will
> > currently unregister the acpi_video# backlight device(s) since userspace
> > prefers the firmware type over the native type, so for userspace to
> > actually honor the acpi_video_get_backlight_type()==native we need to get
> > the acpi_video# backlight device "out of the way" which is currently
> > handled by unregistering it.
> >
> > Note in a way we already have a case where userspace sees 2 panels,
> > in hybrid laptop setups with a mux and on those systems we may see
> > either 2 native backlight devices; or 2 native backlight devices +
> > 2 acpi_video backlight devices with userspace preferring the ACPI
> > ones.
> >
> > Also note that userspace already has code to detect if the related
> > panel is active (iow which way the mux between the GPU and the panels
> > points) and then uses that backlight device. Userspace here very
> > much assumes a single panel though.
> >
> >> 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.
> >
> > Right. Note that the current code (both with and without this patchset)
> > already will work fine from a kernel pov as long as both panels
> > are either using native GPU PWM or are both using ACPI. But if we
> > ever get a mix then this will need special handling.
> >
> > Note that all userspace code I know is currently hardcoded
> > to assume a single panel. Userspace already picks one preferred
> > device under /sys/class/backlight and ignores the rest. Actually
> > atm userspace must behave this way, because on x86/ACPI boards we
> > do often register multiple backlight devices for a single panel.
> >
> > So in a way moving to registering only a single backlight device
> > prepares for actually having multiple panels work.
> >
> > Also keep in mind that this is preparation work for making the
> > panel brightness a drm_connector property. When the panel uses
> > a backlight device other then the native GPU PWM to control the
> > brightness then the helper code for this needs to have a way to
> > select which backlight_device to use then and the non native
> > types are not "linked" to a specific connector so in this case
> > we really need there to be only 1 backlight device registered
> > so that the code looking up the (non native) backlight device
> > for the connector gets the right one and not merely the one
> > which happened to get registered first.
> >
> > And I believe that having the panel brightness be a drm_connector
> > property is the way to make it possible for userspace to deal
> > with the multiple panels which each have a separate brightness
> > control case.
> 
> Agreed.
> 
> Thanks for the explanations and recording them here.

Can we stuff a summary of all the things we've discussed here into
Documentation/gpu/todo.rst so that we have this recorded somewhere more
permanently? Also good place to make sure everyone who was part of these
discussions has a chance to ack the overall plan as part of merging such a
patch.

Just feels like this is big&complex enough to justify a todo entry with
what's going on and what's still to be done.
-Daniel


> 
> BR,
> Jani.
> 
> >
> > Regards,
> >
> > Hans
> >
> >
> >
> >
> >
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >>>
> >>> This series implements my RFC describing my plan for these cleanups:
> >>> https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158@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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Dadap]

On 5/18/22 03:44, Jani Nikula wrote:
> On Tue, 17 May 2022, Hans de Goede <hdegoede@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@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").


This is one of my concerns as well. There are a small number of (mostly 
somewhat old) designs with more than one internal panel. Since the 
intent here is to tie the new backlight UAPI to DRM connectors, I think 
it should remain possible to address each panel individually (the goal 
is to stop having multiple backlight handlers, all of which control the 
same display, not to stop having multiple backlight handlers in 
general). Where I think it might get tricky is when the same display 
might be driven by one GPU or another at different times, for example, 
with a switchable mux. Notebook designs which use the 
nvidia-wmi-ec-backlight driver will use the same backlight handler for 
the display regardless of which GPU is currently driving it, but I 
believe there are other designs where the same panel might have 
backlight controlled by either the iGPU or the dGPU, depending on the 
mux state.


> 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?


The "how do you know" question is one I am wondering as well. I need to 
check with some of our backlight experts here at NVIDIA to figure out 
how exactly reliably we are able to make this determination.


> 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@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(-)

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

[Hans de Goede]

Hi Lyude,

Thank you for the reviews.

On 5/18/22 19:39, Lyude Paul wrote:
> On Tue, 2022-05-17 at 17:23 +0200, Hans de Goede wrote:
>> 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@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();
> 
> Assuming we don't need to return the value of acpi_video_register_backlight()
> here:

The function return type is void, so no return value to check :)

> 
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> 
>> +
>>         return ret;
>>  }
>>  
> 

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

[Hans de Goede]

Hi,

On 5/19/22 11:02, Jani Nikula wrote:
> On Wed, 18 May 2022, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 5/18/22 10:55, Jani Nikula wrote:
>>> On Tue, 17 May 2022, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> 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.
> 
> Regarding the question below, this is the part that throws me off.
> 
>>>>
>>>> Note that all current callers are updated to pass false for the new
>>>> parameter, so this change in itself causes no functional changes.
>>>
>>>
>>>> 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.
>>>>   */
>>>
>>> Frankly, I think the boolean native parameter here, and at the call
>>> sites, is confusing, and the slightly different explanations in the
>>> commit message and comment here aren't helping.
>>
>> Can you elaborate the "slightly different explanations in the
>> commit message and comment" part a bit (so that I can fix this) ?
>>
>>> I suggest adding a separate function that the native backlight drivers
>>> should use. I think it's more obvious all around, and easier to document
>>> too.
>>
>> Code wise I think this would mean renaming the original and
>> then adding 2 wrappers, but that is fine with me. I've no real
>> preference either way and I'm happy with adding a new variant of
>> acpi_video_get_backlight_type() for the native backlight drivers
>> any suggestion for a name ?
> 
> Alternatively, do the native backlight drivers have any need for the
> actual backlight type information from acpi? They only need to be able
> to ask if they should register themselves, right?
> 
> I understand this sounds like bikeshedding, but I'm trying to avoid
> duplicating the conditions in the drivers where a single predicate
> function call could be sufficient, and the complexity could be hidden in
> acpi.
> 
> 	if (!acpi_video_backlight_use_native())
> 		return;

acpi_video_backlight_use_native() sounds good, I like I will change
this for v2. This also removes churn in all the other
acpi_video_get_backlight_type() callers.

> Perhaps all the drivers/platform/x86/* backlight drivers could use:
> 
> 	if (acpi_video_backlight_use_vendor())
> 		...

Hmm, as part of the ractoring there also will be new apple_gmux
and nvidia_wmi_ec types. I'm not sure about adding seperate functions
for all of those vs get_type() != foo. I like get_type != foo because
it makes clear that there will also be another caller somewhere
where get_type == foo and that that one will rbe the one which
actually gets to register its backlight.

> You can still use the native parameter etc. internally, but just hide
> the details from everyone else, and, hopefully, make it harder for them
> to do silly things?

Ack.

Regards,

Hans