* [PATCH v6 01/10] Revert "backlight: Remove notifier"
2026-06-24 16:57 [PATCH v6 00/10] Add support for a DRM backlight capability Mario Limonciello
@ 2026-06-24 16:57 ` Mario Limonciello
2026-06-24 17:10 ` sashiko-bot
2026-06-24 16:57 ` [PATCH v6 02/10] backlight: add kernel-internal backlight API Mario Limonciello
` (9 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Mario Limonciello @ 2026-06-24 16:57 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Mario Limonciello (AMD), Simon Ser
From: "Mario Limonciello (AMD)" <superm1@kernel.org>
This reverts commit 5461f3fd74a89757f95f351eb0bc26aafc2a2e91.
The backlight notifier support is needed in order to add backlight
control support into DRM connectors.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
Tested-by: Simon Ser <contact@emersion.fr>
---
drivers/video/backlight/backlight.c | 42 +++++++++++++++++++++++++++++
include/linux/backlight.h | 20 ++++++++++++++
2 files changed, 62 insertions(+)
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index a22d0bbb6e639..ff2c2084c73a4 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -65,6 +65,7 @@
static struct list_head backlight_dev_list;
static struct mutex backlight_dev_list_mutex;
+static struct blocking_notifier_head backlight_notifier;
static const char *const backlight_types[] = {
[BACKLIGHT_RAW] = "raw",
@@ -415,6 +416,9 @@ struct backlight_device *backlight_device_register(const char *name,
list_add(&new_bd->entry, &backlight_dev_list);
mutex_unlock(&backlight_dev_list_mutex);
+ blocking_notifier_call_chain(&backlight_notifier,
+ BACKLIGHT_REGISTERED, new_bd);
+
return new_bd;
}
EXPORT_SYMBOL(backlight_device_register);
@@ -484,6 +488,9 @@ void backlight_device_unregister(struct backlight_device *bd)
mutex_unlock(&pmac_backlight_mutex);
#endif
+ blocking_notifier_call_chain(&backlight_notifier,
+ BACKLIGHT_UNREGISTERED, bd);
+
mutex_lock(&bd->ops_lock);
bd->ops = NULL;
mutex_unlock(&bd->ops_lock);
@@ -507,6 +514,40 @@ static int devm_backlight_device_match(struct device *dev, void *res,
return *r == data;
}
+/**
+ * backlight_register_notifier - get notified of backlight (un)registration
+ * @nb: notifier block with the notifier to call on backlight (un)registration
+ *
+ * Register a notifier to get notified when backlight devices get registered
+ * or unregistered.
+ *
+ * RETURNS:
+ *
+ * 0 on success, otherwise a negative error code
+ */
+int backlight_register_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_register(&backlight_notifier, nb);
+}
+EXPORT_SYMBOL(backlight_register_notifier);
+
+/**
+ * backlight_unregister_notifier - unregister a backlight notifier
+ * @nb: notifier block to unregister
+ *
+ * Register a notifier to get notified when backlight devices get registered
+ * or unregistered.
+ *
+ * RETURNS:
+ *
+ * 0 on success, otherwise a negative error code
+ */
+int backlight_unregister_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_unregister(&backlight_notifier, nb);
+}
+EXPORT_SYMBOL(backlight_unregister_notifier);
+
/**
* devm_backlight_device_register - register a new backlight device
* @dev: the device to register
@@ -674,6 +715,7 @@ static int __init backlight_class_init(void)
INIT_LIST_HEAD(&backlight_dev_list);
mutex_init(&backlight_dev_list_mutex);
+ BLOCKING_INIT_NOTIFIER_HEAD(&backlight_notifier);
return 0;
}
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index f29a9ef1052e7..d905173c7f73c 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -64,6 +64,24 @@ enum backlight_type {
BACKLIGHT_TYPE_MAX,
};
+/**
+ * enum backlight_notification - the type of notification
+ *
+ * The notifications that is used for notification sent to the receiver
+ * that registered notifications using backlight_register_notifier().
+ */
+enum backlight_notification {
+ /**
+ * @BACKLIGHT_REGISTERED: The backlight device is registered.
+ */
+ BACKLIGHT_REGISTERED,
+
+ /**
+ * @BACKLIGHT_UNREGISTERED: The backlight revice is unregistered.
+ */
+ BACKLIGHT_UNREGISTERED,
+};
+
/** enum backlight_scale - the type of scale used for brightness values
*
* The type of scale used for brightness values.
@@ -388,6 +406,8 @@ void devm_backlight_device_unregister(struct device *dev,
struct backlight_device *bd);
void backlight_force_update(struct backlight_device *bd,
enum backlight_update_reason reason);
+int backlight_register_notifier(struct notifier_block *nb);
+int backlight_unregister_notifier(struct notifier_block *nb);
struct backlight_device *backlight_device_get_by_name(const char *name);
struct backlight_device *backlight_device_get_by_type(enum backlight_type type);
int backlight_device_set_brightness(struct backlight_device *bd,
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 01/10] Revert "backlight: Remove notifier"
2026-06-24 16:57 ` [PATCH v6 01/10] Revert "backlight: Remove notifier" Mario Limonciello
@ 2026-06-24 17:10 ` sashiko-bot
0 siblings, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-06-24 17:10 UTC (permalink / raw)
To: Mario Limonciello; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [Low] Multiple copy-paste errors and typos in the newly added kerneldoc comments for the backlight notifier API.
- [Low] Missing include or forward declaration for `struct notifier_block` in `<linux/backlight.h>`.
Pre-existing issues:
- [High] Concurrent `backlight_update_status()` and `backlight_device_unregister()` can cause a Use-After-Free of `bd->ops` or execution of unmapped memory.
- [High] `backlight_device_get_by_type` returns a pointer to a `backlight_device` without incrementing its reference count, leading to a Use-After-Free.
- [High] `backlight_enable()` and `backlight_disable()` modify `bd->props.state` locklessly, causing data races and state corruption.
--
commit 692a77b3d3423be93e071e88f6a741af5c0ebe7d
Author: Mario Limonciello (AMD) <superm1@kernel.org>
Subject: Revert "backlight: Remove notifier"
This reverts a previous commit to restore backlight notifier support.
The backlight notifier support is needed in order to add backlight
control support into DRM connectors.
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index a22d0bbb6e639..ff2c2084c73a4 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
[ ... ]
> @@ -484,6 +488,9 @@ void backlight_device_unregister(struct backlight_device *bd)
> mutex_unlock(&pmac_backlight_mutex);
> #endif
>
> + blocking_notifier_call_chain(&backlight_notifier,
> + BACKLIGHT_UNREGISTERED, bd);
> +
> mutex_lock(&bd->ops_lock);
> bd->ops = NULL;
> mutex_unlock(&bd->ops_lock);
[Severity: High]
This is a pre-existing issue, but can the lockless check in
backlight_update_status() race with this teardown?
If a panel driver concurrently calls backlight_update_status(), which checks
bd->ops under the update_lock but without the ops_lock, and this function
sets bd->ops to NULL under the ops_lock:
include/linux/backlight.h:backlight_update_status() {
...
mutex_lock(&bd->update_lock);
if (bd->ops && bd->ops->update_status)
ret = bd->ops->update_status(bd);
...
}
Could it lead to a use-after-free or executing unmapped module memory if the
driver is unbound right after the check passes?
[ ... ]
> +/**
> + * backlight_unregister_notifier - unregister a backlight notifier
> + * @nb: notifier block to unregister
> + *
> + * Register a notifier to get notified when backlight devices get registered
> + * or unregistered.
[Severity: Low]
This kerneldoc description appears to be copied directly from
backlight_register_notifier().
Should this be updated to describe unregistering a notifier instead?
[ ... ]
[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
use-after-free in backlight_device_get_by_type() in the same file.
drivers/video/backlight/backlight.c:backlight_device_get_by_type() {
...
mutex_unlock(&backlight_dev_list_mutex);
return found ? bd : NULL;
}
This function returns a matched backlight_device pointer without taking a
reference count via get_device(). If the backlight device is concurrently
unregistered, the device could be freed, leaving the caller with a dangling
pointer.
Should this function increment the reference count before returning?
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index f29a9ef1052e7..d905173c7f73c 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
[ ... ]
> +enum backlight_notification {
> + /**
> + * @BACKLIGHT_REGISTERED: The backlight device is registered.
> + */
> + BACKLIGHT_REGISTERED,
> +
> + /**
> + * @BACKLIGHT_UNREGISTERED: The backlight revice is unregistered.
> + */
[Severity: Low]
Is the word revice a typo for device here?
> + BACKLIGHT_UNREGISTERED,
> +};
[ ... ]
> void backlight_force_update(struct backlight_device *bd,
> enum backlight_update_reason reason);
> +int backlight_register_notifier(struct notifier_block *nb);
> +int backlight_unregister_notifier(struct notifier_block *nb);
> struct backlight_device *backlight_device_get_by_name(const char *name);
[Severity: Low]
Does this header need a forward declaration of struct notifier_block, or an
include of linux/notifier.h?
Relying on implicit includes could cause build failures in downstream modules
that include this header.
[Severity: High]
This is a pre-existing issue, but there appears to be a potential data race
regarding state in backlight_enable() and backlight_disable().
include/linux/backlight.h:backlight_enable() {
...
bd->props.power = BACKLIGHT_POWER_ON;
bd->props.state &= ~BL_CORE_FBBLANK;
...
}
These functions modify the state locklessly. Since other backlight
functions like backlight_suspend() modify the same state while holding
the ops_lock, could a concurrent call to backlight_enable() silently
overwrite and lose bits like BL_CORE_SUSPENDED?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624165751.2014759-1-mario.limonciello@amd.com?part=1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 02/10] backlight: add kernel-internal backlight API
2026-06-24 16:57 [PATCH v6 00/10] Add support for a DRM backlight capability Mario Limonciello
2026-06-24 16:57 ` [PATCH v6 01/10] Revert "backlight: Remove notifier" Mario Limonciello
@ 2026-06-24 16:57 ` Mario Limonciello
2026-06-24 17:11 ` sashiko-bot
2026-06-24 16:57 ` [PATCH v6 03/10] drm: link connectors to backlight devices Mario Limonciello
` (8 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Mario Limonciello @ 2026-06-24 16:57 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Mario Limonciello (AMD), David Herrmann, Marta Lofstedt
From: "Mario Limonciello (AMD)" <superm1@kernel.org>
So far backlights have only been controlled via sysfs. However, sysfs is
not a proper user-space API for runtime modifications, and never was
intended to provide such. The DRM drivers are now prepared to provide
such a backlight link so user-space can control backlight via DRM
connector properties. This allows us to employ the same access-management
we use for mode-setting.
This patch adds few kernel-internal backlight helpers so we can modify
backlights from within DRM.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
V2: Marta Lofstedt <marta.lofstedt@intel.com>
- rebase
- minor edit for checkpatch warning
Signed-off-by: Marta Lofstedt <marta.lofstedt@intel.com>
V3: Mario Limonciello (AMD) <superm1@kernel.org>
- rebase
- Use guard(mutex)
V4: Mario Limonciello (AMD) <superm1@kernel.org>
- Adjust return type for backlight_set_brightness() to return errors
- Stop clamping in backlight_set_brightness()
- Drop backlight_device_lookup()
V5: Mario Limonciello (AMD) <superm1@kernel.org>
- Drop unnecessary dynamic debug message as backlight_generate_event()
sends a netlink event.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++
include/linux/backlight.h | 15 ++++++++++++++
2 files changed, 46 insertions(+)
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index ff2c2084c73a4..cd1a161ae7bc6 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -514,6 +514,37 @@ static int devm_backlight_device_match(struct device *dev, void *res,
return *r == data;
}
+/**
+ * backlight_set_brightness - set brightness on a backlight device
+ * @bd: backlight device to operate on
+ * @value: brightness value to set on the device
+ * @reason: backlight-change reason to use for notifications
+ *
+ * This is the in-kernel API equivalent of writing into the 'brightness' sysfs
+ * file. It calls into the underlying backlight driver to change the brightness
+ * value.
+ * A uevent notification is sent with the reason set to @reason.
+ * Return: 0 if successfully notified, -EINVAL for invalid values
+ */
+int backlight_set_brightness(struct backlight_device *bd, unsigned int value,
+ enum backlight_update_reason reason)
+{
+ int rc = 0;
+
+ guard(mutex)(&bd->ops_lock);
+ if (bd->ops) {
+ if (value > bd->props.max_brightness)
+ return -EINVAL;
+ bd->props.brightness = value;
+ rc = backlight_update_status(bd);
+ }
+ if (rc == 0)
+ backlight_generate_event(bd, reason);
+
+ return rc;
+}
+EXPORT_SYMBOL_GPL(backlight_set_brightness);
+
/**
* backlight_register_notifier - get notified of backlight (un)registration
* @nb: notifier block with the notifier to call on backlight (un)registration
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index d905173c7f73c..204eea9256fd7 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -429,6 +429,21 @@ static inline void backlight_notify_blank_all(struct device *display_dev,
{ }
#endif
+int backlight_set_brightness(struct backlight_device *bd, unsigned int value,
+ enum backlight_update_reason reason);
+
+static inline void backlight_device_ref(struct backlight_device *bd)
+{
+ if (bd)
+ get_device(&bd->dev);
+}
+
+static inline void backlight_device_unref(struct backlight_device *bd)
+{
+ if (bd)
+ put_device(&bd->dev);
+}
+
#define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
/**
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 02/10] backlight: add kernel-internal backlight API
2026-06-24 16:57 ` [PATCH v6 02/10] backlight: add kernel-internal backlight API Mario Limonciello
@ 2026-06-24 17:11 ` sashiko-bot
0 siblings, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-06-24 17:11 UTC (permalink / raw)
To: Mario Limonciello; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] `backlight_set_brightness()` incorrectly returns success (0) and generates an erroneous uevent when called on an unregistered or unbound backlight device (`bd->ops == NULL`).
- [Medium] The function-scoped `guard(mutex)(&bd->ops_lock)` holds the lock during `backlight_generate_event()`, violating established subsystem locking patterns and risking deadlocks.
--
commit ef4454a90178562afe2f7457649f35d82b14a03f
Author: Mario Limonciello (AMD) <superm1@kernel.org>
backlight: add kernel-internal backlight API
This patch adds kernel-internal backlight helpers to allow modifying
backlights from within DRM. This provides user-space with a way to control
backlight via DRM connector properties rather than relying solely on sysfs.
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index ff2c2084c73a4..cd1a161ae7bc6 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
[ ... ]
> +int backlight_set_brightness(struct backlight_device *bd, unsigned int value,
> + enum backlight_update_reason reason)
> +{
> + int rc = 0;
> +
> + guard(mutex)(&bd->ops_lock);
> + if (bd->ops) {
> + if (value > bd->props.max_brightness)
> + return -EINVAL;
> + bd->props.brightness = value;
> + rc = backlight_update_status(bd);
> + }
[Severity: Medium]
Does this incorrectly return success and generate a spurious event if the
backlight device is unregistered or unbound?
If bd->ops is NULL, the update block is skipped and rc remains 0. Could this
be initialized to a value like -ENXIO to correctly indicate failure instead?
> + if (rc == 0)
> + backlight_generate_event(bd, reason);
[Severity: Medium]
Should the ops_lock be released before generating the uevent?
Because guard() is used at function scope, the lock is held across
backlight_generate_event(). This means the lock is held during
kobject_uevent_env() and sysfs_notify(), which might lead to stalls or lock
inversions.
Would it be safer to use a scoped_guard() restricted exclusively to the
bd->ops operations block?
> +
> + return rc;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624165751.2014759-1-mario.limonciello@amd.com?part=2
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 03/10] drm: link connectors to backlight devices
2026-06-24 16:57 [PATCH v6 00/10] Add support for a DRM backlight capability Mario Limonciello
2026-06-24 16:57 ` [PATCH v6 01/10] Revert "backlight: Remove notifier" Mario Limonciello
2026-06-24 16:57 ` [PATCH v6 02/10] backlight: add kernel-internal backlight API Mario Limonciello
@ 2026-06-24 16:57 ` Mario Limonciello
2026-06-24 17:10 ` sashiko-bot
2026-06-24 16:57 ` [PATCH v6 04/10] DRM: Add support for client indicating support for luminance Mario Limonciello
` (7 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Mario Limonciello @ 2026-06-24 16:57 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Mario Limonciello (AMD), Dmitry Baryshkov
From: "Mario Limonciello (AMD)" <superm1@kernel.org>
This will show which connector in sysfs matches which backlight.
Tested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> # SM8150-HDK
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/gpu/drm/Kconfig | 1 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_backlight.c | 371 ++++++++++++++++++++++++++++
drivers/gpu/drm/drm_connector.c | 12 +
drivers/gpu/drm/drm_drv.c | 8 +
drivers/gpu/drm/drm_mode_config.c | 7 +
drivers/gpu/drm/drm_mode_object.c | 66 ++++-
drivers/gpu/drm/drm_sysfs.c | 28 ++-
drivers/video/backlight/backlight.c | 17 ++
include/drm/drm_backlight.h | 51 ++++
include/drm/drm_connector.h | 3 +
include/drm/drm_mode_config.h | 5 +
include/linux/backlight.h | 13 +
13 files changed, 578 insertions(+), 5 deletions(-)
create mode 100644 drivers/gpu/drm/drm_backlight.c
create mode 100644 include/drm/drm_backlight.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 323422861e8f6..d6035bbbdc83f 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -17,6 +17,7 @@ menuconfig DRM
# device and dmabuf fd. Let's make sure that is available for our userspace.
select KCMP
select VIDEO
+ select BACKLIGHT_CLASS_DEVICE
help
Kernel-level support for the Direct Rendering Infrastructure (DRI)
introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index e97faabcd7830..bf980a2ac1e6b 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -78,6 +78,7 @@ drm-$(CONFIG_DRM_CLIENT) += \
drm_client_event.o \
drm_client_modeset.o \
drm_client_sysrq.o
+drm-y += drm_backlight.o
drm-$(CONFIG_COMPAT) += drm_ioc32.o
drm-$(CONFIG_DRM_PANEL) += drm_panel.o
drm-$(CONFIG_OF) += drm_of.o
diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
new file mode 100644
index 0000000000000..b1ec470be86ca
--- /dev/null
+++ b/drivers/gpu/drm/drm_backlight.c
@@ -0,0 +1,371 @@
+// SPDX-License-Identifier: MIT
+/*
+ * DRM Backlight Helpers
+ * Copyright (c) 2014 David Herrmann
+ * Copyright (c) 2026 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/backlight.h>
+#include <linux/fs.h>
+#include <linux/list.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <drm/drm_backlight.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_device.h>
+#include <drm/drm_mode_config.h>
+
+/**
+ * DOC: Backlight Devices
+ *
+ * Backlight devices have always been managed as a separate subsystem,
+ * independent of DRM. They are usually controlled via separate hardware
+ * interfaces than the display controller, so the split works out fine.
+ * However, backlight brightness is a property of a display, and thus a
+ * property of a DRM connector. We already manage DPMS states via connector
+ * properties, so it is natural to keep brightness control at the same place.
+ *
+ * This DRM backlight interface implements generic backlight properties on
+ * connectors. It does not handle any hardware backends but simply forwards
+ * the requests to a linked backlight device. The links between connectors and
+ * backlight devices are established by DRM drivers; user-space cannot create
+ * or modify these links. A 'change' uevent is sent whenever the brightness is
+ * updated.
+ *
+ * Drivers have to call drm_backlight_alloc() after allocating a connector via
+ * drm_connector_init(). This will automatically add a backlight device to the
+ * given connector. Drivers must then link a hardware backlight by calling
+ * drm_backlight_link() with the registered backlight_device. If no link is
+ * established, the DRM backlight property reports an empty range and
+ * brightness changes are no-ops.
+ */
+
+struct drm_backlight {
+ struct list_head list;
+ struct drm_connector *connector;
+ struct backlight_device *link;
+ /*
+ * Number of luminance-aware DRM clients that have taken over this
+ * connector's backlight. While > 0, legacy sysfs writes to the
+ * linked backlight_device return -EBUSY. Protected by
+ * drm_backlight_lock.
+ */
+ unsigned int luminance_clients;
+};
+
+static LIST_HEAD(drm_backlight_list);
+static DEFINE_SPINLOCK(drm_backlight_lock);
+
+/* caller must hold @drm_backlight_lock */
+static bool __drm_backlight_is_registered(struct drm_backlight *b)
+{
+ lockdep_assert_held(&drm_backlight_lock);
+ /* a device is live if it is linked to @drm_backlight_list */
+ return !list_empty(&b->list);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
+{
+ unsigned int max, set;
+
+ lockdep_assert_held(&drm_backlight_lock);
+
+ if (!b->link)
+ return;
+
+ max = b->link->props.max_brightness;
+ if (max < 1)
+ return;
+
+ set = v;
+ if (set >= max)
+ set = max;
+}
+
+/**
+ * __drm_backlight_update_prop_range - update the luminance property range
+ * @b: backlight device
+ *
+ * Updates the luminance property range based on the linked backlight device's
+ * max_brightness. If no device is linked, sets range to 0-0 to indicate
+ * unavailability.
+ */
+static void __drm_backlight_update_prop_range(struct drm_backlight *b)
+{
+ struct drm_device *dev = b->connector->dev;
+ struct drm_property *prop = dev->mode_config.luminance_property;
+ unsigned int max = 0;
+
+ lockdep_assert_held(&drm_backlight_lock);
+
+ if (b->link && b->link->props.max_brightness > 0)
+ max = b->link->props.max_brightness;
+
+ /* Update property range to match hardware capabilities.
+ * Range of 0-0 indicates no backing device.
+ * Range of 1-max for normal operation (0 reserved for display off).
+ */
+ if (prop->values[1] != max) {
+ prop->values[0] = max ? 1 : 0;
+ prop->values[1] = max;
+ }
+}
+
+/* caller must hold @drm_backlight_lock */
+static bool __drm_backlight_link(struct drm_backlight *b,
+ struct backlight_device *bd)
+{
+ if (bd == b->link)
+ return false;
+
+ backlight_device_unref(b->link);
+ b->link = bd;
+ backlight_device_ref(b->link);
+ if (bd)
+ __drm_backlight_real_changed(b, bd->props.brightness);
+ __drm_backlight_update_prop_range(b);
+
+ return true;
+}
+
+/**
+ * drm_backlight_alloc - add backlight capability to a connector
+ * @connector: connector to add backlight to
+ *
+ * This allocates a new DRM-backlight device and attaches it to @connector.
+ * This *must* be called before registering the connector. The backlight
+ * device will be automatically registered in sync with the connector. It will
+ * also get removed once the connector is removed.
+ *
+ * No hardware backlight is linked by default. Drivers must call
+ * drm_backlight_link() to associate a registered backlight_device with the
+ * connector. User-space cannot create or modify this link.
+ *
+ * Returns: 0 on success, negative error code on failure.
+ */
+int drm_backlight_alloc(struct drm_connector *connector)
+{
+ struct drm_mode_config *config = &connector->dev->mode_config;
+ struct drm_backlight *b;
+
+ b = kzalloc_obj(*b, GFP_KERNEL);
+ if (!b)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(&b->list);
+ b->connector = connector;
+ connector->backlight = b;
+
+ drm_object_attach_property(&connector->base,
+ config->luminance_property, 0);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_backlight_alloc);
+
+void drm_backlight_free(struct drm_connector *connector)
+{
+ struct drm_backlight *b = connector->backlight;
+
+ if (!b)
+ return;
+
+ WARN_ON(__drm_backlight_is_registered(b));
+ WARN_ON(b->link);
+
+ kfree(b);
+ connector->backlight = NULL;
+}
+EXPORT_SYMBOL(drm_backlight_free);
+
+void drm_backlight_register(struct drm_backlight *b)
+{
+ if (!b)
+ return;
+
+ WARN_ON(__drm_backlight_is_registered(b));
+
+ guard(spinlock)(&drm_backlight_lock);
+ list_add(&b->list, &drm_backlight_list);
+}
+EXPORT_SYMBOL(drm_backlight_register);
+
+void drm_backlight_unregister(struct drm_backlight *b)
+{
+ if (!b)
+ return;
+
+ WARN_ON(!__drm_backlight_is_registered(b));
+
+ scoped_guard(spinlock, &drm_backlight_lock) {
+ list_del_init(&b->list);
+ __drm_backlight_link(b, NULL);
+ }
+}
+EXPORT_SYMBOL(drm_backlight_unregister);
+
+/**
+ * drm_backlight_link - link a backlight device to a DRM backlight
+ * @b: DRM backlight to modify
+ * @bd: backlight device to link, or NULL to unlink
+ *
+ * Establish the link between a DRM connector's backlight property and a
+ * registered backlight_device. Drivers must call this with the
+ * backlight_device they registered for the connector. Passing NULL unlinks
+ * any previously linked device.
+ *
+ * The caller is responsible for ensuring @bd remains valid until either it
+ * is unlinked via drm_backlight_link(b, NULL) or the connector is
+ * unregistered.
+ *
+ * Whenever a hardware backlight is linked or unlinked, a uevent with
+ * "BACKLIGHT=1" is generated on the connector.
+ */
+void drm_backlight_link(struct drm_backlight *b, struct backlight_device *bd)
+{
+ if (!b)
+ return;
+
+ guard(spinlock)(&drm_backlight_lock);
+ __drm_backlight_link(b, bd);
+}
+EXPORT_SYMBOL(drm_backlight_link);
+
+/**
+ * drm_backlight_get_device - get the backlight_device linked to a DRM backlight
+ * @b: DRM backlight
+ *
+ * Returns the &backlight_device linked to @b, or NULL if no device is linked
+ * or @b is NULL. The caller must hold the appropriate lock to prevent the
+ * link from changing while the pointer is in use.
+ */
+struct backlight_device *drm_backlight_get_device(struct drm_backlight *b)
+{
+ if (!b)
+ return NULL;
+
+ guard(spinlock)(&drm_backlight_lock);
+ return b->link;
+}
+EXPORT_SYMBOL(drm_backlight_get_device);
+
+/**
+ * drm_backlight_inhibit_legacy - disable legacy sysfs control of the linked bd
+ * @b: DRM backlight to inhibit
+ *
+ * Record that one more luminance-aware DRM client has taken over @b. While
+ * any clients are recorded, writes to the linked backlight_device's legacy
+ * ``brightness`` sysfs attribute return ``-EBUSY``. The takeover follows
+ * @b->link if the link changes.
+ *
+ * Calls must be balanced with drm_backlight_uninhibit_legacy().
+ */
+void drm_backlight_inhibit_legacy(struct drm_backlight *b)
+{
+ if (!b)
+ return;
+}
+EXPORT_SYMBOL(drm_backlight_inhibit_legacy);
+
+/**
+ * drm_backlight_uninhibit_legacy - re-enable legacy sysfs control
+ * @b: DRM backlight to uninhibit
+ *
+ * Balances a previous drm_backlight_inhibit_legacy() call. When the last
+ * luminance-aware client goes away, legacy sysfs writes are allowed again.
+ */
+void drm_backlight_uninhibit_legacy(struct drm_backlight *b)
+{
+ if (!b)
+ return;
+}
+EXPORT_SYMBOL(drm_backlight_uninhibit_legacy);
+
+/**
+ * drm_backlight_inhibit_legacy_all - inhibit legacy sysfs on every connector
+ * @dev: DRM device whose connectors should be inhibited
+ *
+ * Walks all connectors on @dev and calls drm_backlight_inhibit_legacy() on
+ * each connector that has a DRM backlight attached. Used when a client
+ * declares it is luminance-aware via DRM_CLIENT_CAP_LUMINANCE.
+ */
+void drm_backlight_inhibit_legacy_all(struct drm_device *dev)
+{
+ struct drm_connector_list_iter iter;
+ struct drm_connector *connector;
+
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter)
+ drm_backlight_inhibit_legacy(connector->backlight);
+ drm_connector_list_iter_end(&iter);
+}
+EXPORT_SYMBOL(drm_backlight_inhibit_legacy_all);
+
+/**
+ * drm_backlight_uninhibit_legacy_all - reverse drm_backlight_inhibit_legacy_all()
+ * @dev: DRM device whose connectors should be uninhibited
+ */
+void drm_backlight_uninhibit_legacy_all(struct drm_device *dev)
+{
+ struct drm_connector_list_iter iter;
+ struct drm_connector *connector;
+
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter)
+ drm_backlight_uninhibit_legacy(connector->backlight);
+ drm_connector_list_iter_end(&iter);
+}
+EXPORT_SYMBOL(drm_backlight_uninhibit_legacy_all);
+
+void drm_backlight_set_luminance(struct drm_backlight *b, unsigned int value)
+{
+ guard(spinlock)(&drm_backlight_lock);
+ __drm_backlight_real_changed(b, value);
+}
+EXPORT_SYMBOL(drm_backlight_set_luminance);
+
+static int drm_backlight_notify(struct notifier_block *self,
+ unsigned long event, void *data)
+{
+ struct backlight_device *bd = data;
+ struct drm_backlight *b;
+
+ guard(spinlock)(&drm_backlight_lock);
+
+ switch (event) {
+ case BACKLIGHT_UNREGISTERED:
+ list_for_each_entry(b, &drm_backlight_list, list)
+ if (b->link == bd)
+ __drm_backlight_link(b, NULL);
+
+ break;
+ case BACKLIGHT_BRIGHTNESS_CHANGED:
+ /* Update DRM property value when hardware backlight changes */
+ list_for_each_entry(b, &drm_backlight_list, list)
+ if (b->link == bd)
+ __drm_backlight_real_changed(b, bd->props.brightness);
+
+ break;
+ }
+
+ return 0;
+}
+
+static struct notifier_block drm_backlight_notifier = {
+ .notifier_call = drm_backlight_notify,
+};
+
+int drm_backlight_init(void)
+{
+ return backlight_register_notifier(&drm_backlight_notifier);
+}
+
+void drm_backlight_exit(void)
+{
+ backlight_unregister_notifier(&drm_backlight_notifier);
+}
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 3fa4d2082cd72..128d431f0d6b0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -21,6 +21,7 @@
*/
#include <drm/drm_auth.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_connector.h>
#include <drm/drm_drv.h>
#include <drm/drm_edid.h>
@@ -760,6 +761,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
struct drm_device *dev = connector->dev;
struct drm_display_mode *mode, *t;
+ drm_backlight_free(connector);
/* The connector should have been removed from userspace long before
* it is finally destroyed.
*/
@@ -845,6 +847,8 @@ int drm_connector_register(struct drm_connector *connector)
if (connector->registration_state != DRM_CONNECTOR_INITIALIZING)
goto unlock;
+ drm_backlight_register(connector->backlight);
+
ret = drm_sysfs_connector_add(connector);
if (ret)
goto unlock;
@@ -931,6 +935,8 @@ EXPORT_SYMBOL(drm_connector_dynamic_register);
void drm_connector_unregister(struct drm_connector *connector)
{
mutex_lock(&connector->mutex);
+ drm_backlight_unregister(connector->backlight);
+
if (connector->registration_state != DRM_CONNECTOR_REGISTERED) {
mutex_unlock(&connector->mutex);
return;
@@ -3252,10 +3258,16 @@ int drm_connector_set_obj_prop(struct drm_mode_object *obj,
{
int ret = -EINVAL;
struct drm_connector *connector = obj_to_connector(obj);
+ struct drm_mode_config *config = &connector->dev->mode_config;
/* Do DPMS ourselves */
if (property == connector->dev->mode_config.dpms_property) {
ret = (*connector->funcs->dpms)(connector, (int)value);
+ } else if (property == config->luminance_property) {
+ if (connector->backlight && connector->dpms == DRM_MODE_DPMS_ON)
+ drm_backlight_set_luminance(connector->backlight,
+ value);
+ ret = 0;
} else if (connector->funcs->set_property)
ret = connector->funcs->set_property(connector, property, value);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 675675480da49..5bf402197867a 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -42,6 +42,7 @@
#include <linux/xarray.h>
#include <drm/drm_accel.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_bridge.h>
#include <drm/drm_cache.h>
#include <drm/drm_client_event.h>
@@ -1230,6 +1231,7 @@ static void drm_core_exit(void)
drm_privacy_screen_lookup_exit();
drm_panic_exit();
accel_core_exit();
+ drm_backlight_exit();
unregister_chrdev(DRM_MAJOR, "drm");
drm_debugfs_remove_root();
drm_sysfs_destroy();
@@ -1253,6 +1255,12 @@ static int __init drm_core_init(void)
drm_debugfs_init_root();
drm_debugfs_bridge_params();
+ ret = drm_backlight_init();
+ if (ret < 0) {
+ DRM_ERROR("Cannot initialize backlight interface\n");
+ goto error;
+ }
+
ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
if (ret < 0)
goto error;
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 66f7dc37b5970..25c5d29694624 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -32,6 +32,7 @@
#include <drm/drm_print.h>
#include <drm/drm_colorop.h>
#include <linux/dma-resv.h>
+#include <drm/drm_backlight.h>
#include "drm_crtc_internal.h"
#include "drm_internal.h"
@@ -407,6 +408,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
return -ENOMEM;
dev->mode_config.size_hints_property = prop;
+ prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
+ "LUMINANCE", 0, 0);
+ if (!prop)
+ return -ENOMEM;
+ dev->mode_config.luminance_property = prop;
+
return 0;
}
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index 21fc9deda4371..a50d33af95e8c 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -30,6 +30,7 @@
#include <drm/drm_mode_object.h>
#include <drm/drm_plane.h>
#include <drm/drm_print.h>
+#include <drm/drm_backlight.h>
#include "drm_crtc_internal.h"
@@ -287,11 +288,72 @@ int drm_object_property_set_value(struct drm_mode_object *obj,
{
int i;
- WARN_ON(drm_drv_uses_atomic_modeset(property->dev) &&
- !(property->flags & DRM_MODE_PROP_IMMUTABLE));
+ /* Exempt dpms and luminance properties from the atomic warning, as these
+ * have special interdependencies managed internally in this function
+ */
+ if (obj->type == DRM_MODE_OBJECT_CONNECTOR) {
+ struct drm_connector *connector = obj_to_connector(obj);
+ struct drm_mode_config *config = &connector->dev->mode_config;
+
+ if (property != config->dpms_property &&
+ property != config->luminance_property) {
+ WARN_ON(drm_drv_uses_atomic_modeset(property->dev) &&
+ !(property->flags & DRM_MODE_PROP_IMMUTABLE));
+ }
+ } else {
+ WARN_ON(drm_drv_uses_atomic_modeset(property->dev) &&
+ !(property->flags & DRM_MODE_PROP_IMMUTABLE));
+ }
for (i = 0; i < obj->properties->count; i++) {
+ /* If properties depends on each other
+ * this is where to resolve that issue
+ */
if (obj->properties->properties[i] == property) {
+ /* Connector-specific property interdependencies */
+ if (obj->type == DRM_MODE_OBJECT_CONNECTOR) {
+ struct drm_connector *connector = obj_to_connector(obj);
+ struct drm_property *dpms_property =
+ connector->dev->mode_config.dpms_property;
+ struct drm_property *luminance_property =
+ connector->dev->mode_config.luminance_property;
+
+ if (property == dpms_property) {
+ /* When DPMS goes from ON -> OFF,
+ * set the brightness to the minimum possible
+ * to save power.
+ */
+ if (obj->properties->values[i] !=
+ DRM_MODE_DPMS_OFF &&
+ val == DRM_MODE_DPMS_OFF)
+ drm_backlight_set_luminance(
+ connector->backlight, 0);
+ /* When DPMS OFF -> ON, reset the brightness
+ * to the original level
+ */
+ else if (obj->properties->values[i] ==
+ DRM_MODE_DPMS_OFF &&
+ val != DRM_MODE_DPMS_OFF) {
+ uint64_t value;
+
+ drm_object_property_get_value(
+ obj, luminance_property,
+ &value);
+ drm_backlight_set_luminance(
+ connector->backlight, value);
+ }
+ } else if (property == luminance_property) {
+ /* Always update the property value to remember
+ * the user's desired brightness, but only update
+ * hardware when DPMS is ON.
+ */
+ obj->properties->values[i] = val;
+ if (connector->dpms == DRM_MODE_DPMS_ON)
+ drm_backlight_set_luminance(
+ connector->backlight, val);
+ return 0;
+ }
+ }
obj->properties->values[i] = val;
return 0;
}
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index ef4e923a87284..8609e1660f210 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -11,6 +11,7 @@
*/
#include <linux/acpi.h>
+#include <linux/backlight.h>
#include <linux/component.h>
#include <linux/device.h>
#include <linux/err.h>
@@ -27,6 +28,7 @@
#include <drm/drm_device.h>
#include <drm/drm_file.h>
#include <drm/drm_modes.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_print.h>
#include <drm/drm_property.h>
#include <drm/drm_sysfs.h>
@@ -391,15 +393,35 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
int drm_sysfs_connector_add_late(struct drm_connector *connector)
{
- if (connector->ddc)
- return sysfs_create_link(&connector->kdev->kobj,
- &connector->ddc->dev.kobj, "ddc");
+ if (connector->ddc) {
+ int ret = sysfs_create_link(&connector->kdev->kobj,
+ &connector->ddc->dev.kobj, "ddc");
+ if (ret)
+ return ret;
+ }
+
+ if (connector->backlight) {
+ struct backlight_device *bd = drm_backlight_get_device(connector->backlight);
+
+ if (bd) {
+ int ret = sysfs_create_link(&connector->kdev->kobj,
+ &bd->dev.kobj, "backlight");
+ if (ret) {
+ if (connector->ddc)
+ sysfs_remove_link(&connector->kdev->kobj, "ddc");
+ return ret;
+ }
+ }
+ }
return 0;
}
void drm_sysfs_connector_remove_early(struct drm_connector *connector)
{
+ if (connector->backlight && drm_backlight_get_device(connector->backlight))
+ sysfs_remove_link(&connector->kdev->kobj, "backlight");
+
if (connector->ddc)
sysfs_remove_link(&connector->kdev->kobj, "ddc");
}
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index cd1a161ae7bc6..13954c2220b7e 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -126,6 +126,9 @@ static void backlight_generate_event(struct backlight_device *bd,
case BACKLIGHT_UPDATE_HOTKEY:
envp[0] = "SOURCE=hotkey";
break;
+ case BACKLIGHT_UPDATE_DRM:
+ envp[0] = "SOURCE=drm";
+ break;
default:
envp[0] = "SOURCE=unknown";
break;
@@ -579,6 +582,20 @@ int backlight_unregister_notifier(struct notifier_block *nb)
}
EXPORT_SYMBOL(backlight_unregister_notifier);
+/**
+ * backlight_notify_brightness - notify brightness change to listeners
+ * @bd: backlight device that changed
+ *
+ * Notify registered listeners that the backlight brightness has changed.
+ * This is called automatically after successful brightness updates.
+ */
+void backlight_notify_brightness(struct backlight_device *bd)
+{
+ blocking_notifier_call_chain(&backlight_notifier,
+ BACKLIGHT_BRIGHTNESS_CHANGED, bd);
+}
+EXPORT_SYMBOL(backlight_notify_brightness);
+
/**
* devm_backlight_device_register - register a new backlight device
* @dev: the device to register
diff --git a/include/drm/drm_backlight.h b/include/drm/drm_backlight.h
new file mode 100644
index 0000000000000..e0e09e38f7c06
--- /dev/null
+++ b/include/drm/drm_backlight.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: MIT */
+#ifndef __DRM_BACKLIGHT_H__
+#define __DRM_BACKLIGHT_H__
+
+/*
+ * Copyright (c) 2014 David Herrmann <dh.herrmann at gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+
+struct backlight_device;
+struct drm_backlight;
+struct drm_connector;
+struct drm_device;
+struct drm_mode_object;
+
+int drm_backlight_init(void);
+void drm_backlight_exit(void);
+
+int drm_backlight_alloc(struct drm_connector *connector);
+void drm_backlight_free(struct drm_connector *connector);
+void drm_backlight_register(struct drm_backlight *b);
+void drm_backlight_unregister(struct drm_backlight *b);
+
+void drm_backlight_link(struct drm_backlight *b, struct backlight_device *bd);
+struct backlight_device *drm_backlight_get_device(struct drm_backlight *b);
+void drm_backlight_set_luminance(struct drm_backlight *b, unsigned int value);
+void drm_backlight_inhibit_legacy(struct drm_backlight *b);
+void drm_backlight_uninhibit_legacy(struct drm_backlight *b);
+void drm_backlight_inhibit_legacy_all(struct drm_device *dev);
+void drm_backlight_uninhibit_legacy_all(struct drm_device *dev);
+#endif /* __DRM_BACKLIGHT_H__ */
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 5ad62c207d009..d78ac1068d12e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2410,6 +2410,9 @@ struct drm_connector {
* @cec: CEC-related data.
*/
struct drm_connector_cec cec;
+
+ /* backlight link */
+ struct drm_backlight *backlight;
};
#define obj_to_connector(x) container_of(x, struct drm_connector, base)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index e584652ddf676..b6d88319d5d0f 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -852,6 +852,11 @@ struct drm_mode_config {
* the position of the output on the host's screen.
*/
struct drm_property *suggested_y_property;
+ /**
+ * @luminance_property: Default connector property to control the
+ * connector's backlight luminance.
+ */
+ struct drm_property *luminance_property;
/**
* @non_desktop_property: Optional connector property with a hint
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 204eea9256fd7..26a7281d179c1 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -29,6 +29,7 @@ enum backlight_update_reason {
* @BACKLIGHT_UPDATE_SYSFS: The backlight was updated using sysfs.
*/
BACKLIGHT_UPDATE_SYSFS,
+ BACKLIGHT_UPDATE_DRM,
};
/**
@@ -80,6 +81,11 @@ enum backlight_notification {
* @BACKLIGHT_UNREGISTERED: The backlight revice is unregistered.
*/
BACKLIGHT_UNREGISTERED,
+
+ /**
+ * @BACKLIGHT_BRIGHTNESS_CHANGED: The backlight brightness has changed.
+ */
+ BACKLIGHT_BRIGHTNESS_CHANGED,
};
/** enum backlight_scale - the type of scale used for brightness values
@@ -310,6 +316,9 @@ struct backlight_device {
int use_count;
};
+/* Forward declaration for backlight_update_status */
+void backlight_notify_brightness(struct backlight_device *bd);
+
/**
* backlight_update_status - force an update of the backlight device status
* @bd: the backlight device
@@ -323,6 +332,10 @@ static inline int backlight_update_status(struct backlight_device *bd)
ret = bd->ops->update_status(bd);
mutex_unlock(&bd->update_lock);
+ /* Notify DRM and other listeners that brightness changed */
+ if (ret == 0)
+ backlight_notify_brightness(bd);
+
return ret;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 03/10] drm: link connectors to backlight devices
2026-06-24 16:57 ` [PATCH v6 03/10] drm: link connectors to backlight devices Mario Limonciello
@ 2026-06-24 17:10 ` sashiko-bot
0 siblings, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-06-24 17:10 UTC (permalink / raw)
To: Mario Limonciello; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Missing unregistration of `drm_backlight` on connector registration error paths leads to List Corruption and Use-After-Free.
- [High] `drm_backlight_get_device()` returns an unreferenced pointer, causing a Use-After-Free vulnerability.
- [High] Calling a sleeping function (`kobject_uevent_env`) while holding a spinlock in `drm_backlight_link()`.
- [High] Out-of-band modification of atomic state in `__drm_backlight_real_changed()`.
- [High] Dangling sysfs 'backlight' symlink permanently leaked during connector unregistration.
- [High] Global shared property bounds corruption in `__drm_backlight_update_prop_range()`.
- [Medium] Unconditional lockdep assertion failure and unsafe list access in backlight registration functions.
--
commit 97672c232ba720c2f01bd5c726f55338a93bdc07
Author: Mario Limonciello (AMD) <superm1@kernel.org>
drm: link connectors to backlight devices
This commit introduces DRM backlight helpers to connect hardware backlight
devices to DRM connectors. It adds a generic LUMINANCE property and manages
the links, sysfs symlinks, and legacy sysfs inhibitions.
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_backlight.c
[ ... ]
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
> +{
[ ... ]
> + /* Update the atomic state directly.
> + * For atomic drivers, the luminance value is stored in
> + * connector->state->luminance, not in the legacy property array.
> + * We update it unconditionally to reflect the hardware state,
> + * regardless of DPMS.
> + */
> + if (connector->state)
> + connector->state->luminance = set;
[Severity: High]
Is it safe to mutate the atomic state out-of-band here?
When the hardware brightness changes, this function acts as a notifier
callback and directly assigns connector->state->luminance without
executing an atomic commit or acquiring DRM modeset locks.
Since the state object is expected to be strictly immutable during normal
operation, could this cause data races against concurrent commits modifying
or duplicating the state?
[ ... ]
> +static void __drm_backlight_update_prop_range(struct drm_backlight *b)
> +{
[ ... ]
> + /* Update property range to match hardware capabilities.
> + * Range of 0-0 indicates no backing device.
> + * Range of 1-max for normal operation.
> + * Range of 0-max means that the display would turn off at 0
> + */
> + if (prop->values[1] != max) {
> + prop->values[0] = max ? (can_disable ? 0 : 1) : 0;
> + prop->values[1] = max;
> + }
> +}
[Severity: High]
Can this corrupt the permissible brightness range for other connectors?
The LUMINANCE DRM property is created once per device in
drm_mode_create_standard_properties() and shared across all connectors on
that device.
When __drm_backlight_update_prop_range() dynamically overwrites the global
prop->values[0] and prop->values[1] array, it changes the metadata for
the shared property. If a driver exposes multiple display connectors with
backlights that have different maximum brightness ranges, it seems this would
overwrite the bounds for all of them.
[ ... ]
> +void drm_backlight_register(struct drm_backlight *b)
> +{
> + if (!b)
> + return;
> +
> + WARN_ON(__drm_backlight_is_registered(b));
> +
> + guard(spinlock)(&drm_backlight_lock);
> + list_add(&b->list, &drm_backlight_list);
> +}
[Severity: Medium]
Does this trigger an unconditional lockdep splat?
Looking at drm_backlight_register(), it calls __drm_backlight_is_registered(b)
before acquiring the drm_backlight_lock spinlock. However, the helper contains:
static bool __drm_backlight_is_registered(...)
{
lockdep_assert_held(&drm_backlight_lock);
...
}
Since the lock isn't held yet, this appears to trigger an assertion failure
and creates a data race when accessing the list. Should the lock be acquired
before checking the registration status?
[ ... ]
> +void drm_backlight_link(struct drm_backlight *b, struct backlight_device *bd)
> +{
[ ... ]
> + guard(spinlock)(&drm_backlight_lock);
> + send_uevent = __drm_backlight_link(b, bd);
> +
> + if (send_uevent)
> + kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE,
> + (char **)ep);
> +}
[Severity: High]
Could this lead to a sleep-in-atomic issue?
The guard(spinlock)(&drm_backlight_lock) acquires the spinlock for the entire
duration of the function scope. Later in the function, it calls
kobject_uevent_env(), which performs memory allocations using GFP_KERNEL and
can sleep.
Is there a way to drop the lock before sending the uevent?
[ ... ]
> +struct backlight_device *drm_backlight_get_device(struct drm_backlight *b)
> +{
> + if (!b)
> + return NULL;
> +
> + guard(spinlock)(&drm_backlight_lock);
> + return b->link;
> +}
[Severity: High]
Could this result in a use-after-free when creating the sysfs link?
In drm_backlight_get_device(), the pointer b->link is returned while briefly
holding a spinlock. However, it doesn't take a reference to the underlying
backlight_device before returning.
When drm_sysfs_connector_add_late() uses this pointer to call
sysfs_create_link(), what prevents the backlight driver from concurrently
unregistering and freeing the device before the kobj is accessed?
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
[ ... ]
> @@ -845,6 +847,8 @@ int drm_connector_register(struct drm_connector *connector)
> if (connector->registration_state != DRM_CONNECTOR_INITIALIZING)
> goto unlock;
>
> + drm_backlight_register(connector->backlight);
> +
> ret = drm_sysfs_connector_add(connector);
> if (ret)
> goto unlock;
[Severity: Critical]
Does the error path here leak the backlight registration?
If a subsequent operation like drm_sysfs_connector_add() fails, the function
aborts via goto unlock but doesn't call drm_backlight_unregister().
Later, when drm_connector_cleanup() calls drm_backlight_free(), it performs
kfree(b). Since the object was never unregistered, it remains part of
drm_backlight_list, which would lead to list corruption and a use-after-free.
Should drm_backlight_unregister() be called on the error paths?
[ ... ]
> @@ -931,6 +935,8 @@ EXPORT_SYMBOL(drm_connector_dynamic_register);
> void drm_connector_unregister(struct drm_connector *connector)
> {
> mutex_lock(&connector->mutex);
> + drm_backlight_unregister(connector->backlight);
> +
> if (connector->registration_state != DRM_CONNECTOR_REGISTERED) {
> mutex_unlock(&connector->mutex);
> return;
> }
[ ... ]
> drm_sysfs_connector_remove_early(connector);
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
[ ... ]
> void drm_sysfs_connector_remove_early(struct drm_connector *connector)
> {
> + if (connector->backlight && drm_backlight_get_device(connector->backlight))
> + sysfs_remove_link(&connector->kdev->kobj, "backlight");
> +
> if (connector->ddc)
[Severity: High]
Is the backlight sysfs symlink permanently leaked during unregistration?
In drm_connector_unregister(), drm_backlight_unregister() is called first,
which clears the backlight link.
Shortly after, drm_sysfs_connector_remove_early() is called. It checks if
drm_backlight_get_device(connector->backlight) returns a non-NULL pointer.
Because the backlight was just unregistered, this check fails and the
symlink is never removed from sysfs.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624165751.2014759-1-mario.limonciello@amd.com?part=3
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 04/10] DRM: Add support for client indicating support for luminance
2026-06-24 16:57 [PATCH v6 00/10] Add support for a DRM backlight capability Mario Limonciello
` (2 preceding siblings ...)
2026-06-24 16:57 ` [PATCH v6 03/10] drm: link connectors to backlight devices Mario Limonciello
@ 2026-06-24 16:57 ` Mario Limonciello
2026-06-24 17:12 ` sashiko-bot
2026-06-24 16:57 ` [PATCH v6 05/10] drm/amd/display: Pass up errors reading actual brightness Mario Limonciello
` (6 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Mario Limonciello @ 2026-06-24 16:57 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Mario Limonciello (AMD), Dmitry Baryshkov
From: "Mario Limonciello (AMD)" <superm1@kernel.org>
The legacy backlight control interface can only be disabled when both
the client and driver has connector support that the luminance can be
set during a modeset. Add capability for the client to register.
When a luminance-aware client sets DRM_CLIENT_CAP_LUMINANCE, each
DRM-connected backlight on the device is marked as taken over. Writes
to the legacy /sys/class/backlight/<dev>/brightness attribute then
return -EBUSY until the last luminance-aware client clears the cap or
closes its DRM file. The takeover follows the active backlight_device
when drm_backlight_link() retargets the link.
Tested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> # SM8150-HDK
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/gpu/drm/drm_atomic_helper.c | 7 +++
drivers/gpu/drm/drm_atomic_uapi.c | 59 +++++++++++++++++++-
drivers/gpu/drm/drm_backlight.c | 86 +++++++++++++++++++++++++++--
drivers/gpu/drm/drm_connector.c | 51 +++++++++++++++++
drivers/gpu/drm/drm_file.c | 5 ++
drivers/gpu/drm/drm_ioctl.c | 15 +++++
drivers/gpu/drm/drm_property.c | 6 ++
drivers/video/backlight/backlight.c | 7 +++
include/drm/drm_backlight.h | 2 +-
include/drm/drm_connector.h | 5 ++
include/drm/drm_file.h | 8 +++
include/linux/backlight.h | 15 +++++
include/uapi/drm/drm.h | 22 ++++++++
13 files changed, 280 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 51f39edc31edc..17492ff6c5579 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -32,6 +32,7 @@
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_atomic_uapi.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_blend.h>
#include <drm/drm_bridge.h>
#include <drm/drm_colorop.h>
@@ -1224,6 +1225,8 @@ drm_atomic_helper_commit_encoder_bridge_disable(struct drm_device *dev,
* it away), so we won't call disable hooks twice.
*/
bridge = drm_bridge_chain_get_first_bridge(encoder);
+ if (connector->backlight)
+ drm_backlight_set_luminance(connector->backlight, 0);
drm_atomic_bridge_chain_disable(bridge, state);
drm_bridge_put(bridge);
@@ -1738,6 +1741,10 @@ drm_atomic_helper_commit_encoder_bridge_enable(struct drm_device *dev, struct dr
drm_atomic_bridge_chain_enable(bridge, state);
drm_bridge_put(bridge);
+
+ if (connector->backlight && connector->state)
+ drm_backlight_set_luminance(connector->backlight,
+ connector->state->luminance);
}
}
EXPORT_SYMBOL(drm_atomic_helper_commit_encoder_bridge_enable);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 6441b55cc2740..ebf434bbd963b 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -30,6 +30,8 @@
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_atomic_uapi.h>
+#include <drm/drm_backlight.h>
+#include <drm/drm_connector.h>
#include <drm/drm_framebuffer.h>
#include <drm/drm_print.h>
#include <drm/drm_drv.h>
@@ -935,6 +937,14 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
state->privacy_screen_sw_state = val;
} else if (property == connector->broadcast_rgb_property) {
state->hdmi.broadcast_rgb = val;
+ } else if (property == config->luminance_property) {
+ state->luminance = val;
+ /* Update hardware backlight only when DPMS is ON.
+ * Property value is always updated to remember the user's
+ * desired brightness.
+ */
+ if (connector->dpms == DRM_MODE_DPMS_ON)
+ drm_backlight_set_luminance(connector->backlight, val);
} else if (connector->funcs->atomic_set_property) {
return connector->funcs->atomic_set_property(connector,
state, property, val);
@@ -1020,6 +1030,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
*val = state->privacy_screen_sw_state;
} else if (property == connector->broadcast_rgb_property) {
*val = state->hdmi.broadcast_rgb;
+ } else if (property == config->luminance_property) {
+ *val = state->luminance;
} else if (connector->funcs->atomic_get_property) {
return connector->funcs->atomic_get_property(connector,
state, property, val);
@@ -1104,6 +1116,31 @@ static struct drm_pending_vblank_event *create_vblank_event(
return e;
}
+static void drm_atomic_connector_set_backlight(struct drm_connector *connector,
+ unsigned int luminance)
+{
+ if (!connector->backlight)
+ return;
+
+ drm_backlight_set_luminance(connector->backlight, luminance);
+}
+
+static void drm_atomic_crtc_set_backlight(struct drm_crtc *crtc, bool active)
+{
+ struct drm_connector_list_iter conn_iter;
+ struct drm_connector *connector;
+
+ drm_connector_list_iter_begin(crtc->dev, &conn_iter);
+ drm_for_each_connector_iter(connector, &conn_iter) {
+ if (!connector->state || connector->state->crtc != crtc)
+ continue;
+
+ drm_atomic_connector_set_backlight(connector,
+ active ? connector->state->luminance : 0);
+ }
+ drm_connector_list_iter_end(&conn_iter);
+}
+
int drm_atomic_connector_commit_dpms(struct drm_atomic_commit *state,
struct drm_connector *connector,
int mode)
@@ -1126,9 +1163,29 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_commit *state,
if (connector->dpms == mode)
goto out;
+ crtc = connector->state ? connector->state->crtc : NULL;
+
+ /* Handle backlight brightness coordination with DPMS state changes */
+ if (old_mode != DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_OFF) {
+ /* DPMS ON -> OFF: dim all connectors driven by this CRTC. */
+ if (crtc)
+ drm_atomic_crtc_set_backlight(crtc, false);
+ else
+ drm_atomic_connector_set_backlight(connector, 0);
+ }
+
connector->dpms = mode;
- crtc = connector->state->crtc;
+ /* DPMS OFF -> ON: restore brightness to property value */
+ if (old_mode == DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_ON &&
+ connector->state) {
+ if (crtc)
+ drm_atomic_crtc_set_backlight(crtc, true);
+ else
+ drm_atomic_connector_set_backlight(connector,
+ connector->state->luminance);
+ }
+
if (!crtc)
goto out;
ret = drm_atomic_add_affected_connectors(state, crtc);
diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
index b1ec470be86ca..2cddf209d5088 100644
--- a/drivers/gpu/drm/drm_backlight.c
+++ b/drivers/gpu/drm/drm_backlight.c
@@ -71,6 +71,7 @@ static bool __drm_backlight_is_registered(struct drm_backlight *b)
/* caller must hold @drm_backlight_lock */
static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
{
+ struct drm_connector *connector = b->connector;
unsigned int max, set;
lockdep_assert_held(&drm_backlight_lock);
@@ -85,6 +86,15 @@ static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
set = v;
if (set >= max)
set = max;
+
+ /* Update the atomic state directly.
+ * For atomic drivers, the luminance value is stored in
+ * connector->state->luminance, not in the legacy property array.
+ * We update it unconditionally to reflect the hardware state,
+ * regardless of DPMS.
+ */
+ if (connector->state)
+ connector->state->luminance = set;
}
/**
@@ -100,18 +110,22 @@ static void __drm_backlight_update_prop_range(struct drm_backlight *b)
struct drm_device *dev = b->connector->dev;
struct drm_property *prop = dev->mode_config.luminance_property;
unsigned int max = 0;
+ bool can_disable = false;
lockdep_assert_held(&drm_backlight_lock);
- if (b->link && b->link->props.max_brightness > 0)
+ if (b->link && b->link->props.max_brightness > 0) {
max = b->link->props.max_brightness;
+ can_disable = b->link->props.can_disable;
+ }
/* Update property range to match hardware capabilities.
* Range of 0-0 indicates no backing device.
- * Range of 1-max for normal operation (0 reserved for display off).
+ * Range of 1-max for normal operation.
+ * Range of 0-max means that the display would turn off at 0
*/
if (prop->values[1] != max) {
- prop->values[0] = max ? 1 : 0;
+ prop->values[0] = max ? (can_disable ? 0 : 1) : 0;
prop->values[1] = max;
}
}
@@ -123,6 +137,16 @@ static bool __drm_backlight_link(struct drm_backlight *b,
if (bd == b->link)
return false;
+ /* Transfer any DRM legacy-sysfs takeover from the old link to the
+ * new one so the inhibit follows the active backlight_device.
+ */
+ if (b->luminance_clients) {
+ if (b->link)
+ atomic_sub(b->luminance_clients, &b->link->drm_takeover);
+ if (bd)
+ atomic_add(b->luminance_clients, &bd->drm_takeover);
+ }
+
backlight_device_unref(b->link);
b->link = bd;
backlight_device_ref(b->link);
@@ -177,6 +201,7 @@ void drm_backlight_free(struct drm_connector *connector)
WARN_ON(__drm_backlight_is_registered(b));
WARN_ON(b->link);
+ WARN_ON(b->luminance_clients);
kfree(b);
connector->backlight = NULL;
@@ -228,11 +253,18 @@ EXPORT_SYMBOL(drm_backlight_unregister);
*/
void drm_backlight_link(struct drm_backlight *b, struct backlight_device *bd)
{
+ static const char * const ep[] = { "BACKLIGHT=1", NULL };
+ bool send_uevent = false;
+
if (!b)
return;
guard(spinlock)(&drm_backlight_lock);
- __drm_backlight_link(b, bd);
+ send_uevent = __drm_backlight_link(b, bd);
+
+ if (send_uevent)
+ kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE,
+ (char **)ep);
}
EXPORT_SYMBOL(drm_backlight_link);
@@ -269,6 +301,11 @@ void drm_backlight_inhibit_legacy(struct drm_backlight *b)
{
if (!b)
return;
+
+ guard(spinlock)(&drm_backlight_lock);
+ b->luminance_clients++;
+ if (b->link)
+ atomic_inc(&b->link->drm_takeover);
}
EXPORT_SYMBOL(drm_backlight_inhibit_legacy);
@@ -283,6 +320,13 @@ void drm_backlight_uninhibit_legacy(struct drm_backlight *b)
{
if (!b)
return;
+
+ guard(spinlock)(&drm_backlight_lock);
+ if (WARN_ON(b->luminance_clients == 0))
+ return;
+ b->luminance_clients--;
+ if (b->link)
+ atomic_dec(&b->link->drm_takeover);
}
EXPORT_SYMBOL(drm_backlight_uninhibit_legacy);
@@ -324,8 +368,38 @@ EXPORT_SYMBOL(drm_backlight_uninhibit_legacy_all);
void drm_backlight_set_luminance(struct drm_backlight *b, unsigned int value)
{
- guard(spinlock)(&drm_backlight_lock);
- __drm_backlight_real_changed(b, value);
+ struct backlight_device *bd = NULL;
+ unsigned int set = 0;
+ unsigned long flags;
+ unsigned int max = 0;
+
+ spin_lock_irqsave(&drm_backlight_lock, flags);
+ if (b && b->link) {
+ struct backlight_device *link = b->link;
+
+ max = b->link->props.max_brightness;
+
+ if (max == 0)
+ goto out;
+
+ set = min(value, max);
+ if (set == link->props.brightness)
+ goto out;
+
+ bd = link;
+ backlight_device_ref(bd);
+ }
+out:
+ spin_unlock_irqrestore(&drm_backlight_lock, flags);
+
+ if (bd) {
+ int rc = backlight_set_brightness(bd, set, BACKLIGHT_UPDATE_DRM);
+
+ WARN_ON(rc);
+ if (rc)
+ backlight_set_brightness(bd, max, BACKLIGHT_UPDATE_DRM);
+ backlight_device_unref(bd);
+ }
}
EXPORT_SYMBOL(drm_backlight_set_luminance);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 128d431f0d6b0..70afe3d579f38 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1512,6 +1512,57 @@ EXPORT_SYMBOL(drm_hdmi_connector_get_output_format_name);
* Summarizing: Only set "DPMS" when the connector is known to be enabled,
* assume that a successful SETCONFIG call also sets "DPMS" to on, and
* never read back the value of "DPMS" because it can be incorrect.
+ * LUMINANCE:
+ * Atomic property for controlling the backlight brightness level of the
+ * connector's display. This property provides unified access to the display
+ * backlight, replacing the legacy sysfs interface for brightness control.
+ *
+ * The property value is an unsigned integer representing the brightness level.
+ * The valid range is dynamically determined by the capabilities of the
+ * connected backlight hardware and is exposed through the property's minimum
+ * and maximum values:
+ *
+ * - Range 0-0: No backlight device is available for this connector.
+ * - Range 1-N: Normal operation. Values from 1 to N (max_brightness) are
+ * valid brightness levels, where 1 is the minimum visible brightness and
+ * N is the maximum brightness the hardware supports.
+ * - Value 0: Special value to turn off the display backlight completely.
+ * This value is accepted even when the normal range starts at 1.
+ *
+ * The range may change during runtime if a new backlight device is linked
+ * or unlinked. The kernel will send a change uevent when this occurs.
+ *
+ * Setting LUMINANCE to 0 turns off the backlight, which may turn off the
+ * display completely depending on the hardware. Setting it to any value
+ * from 1 to N adjusts the brightness accordingly. Reading this property
+ * returns the current brightness level that was last set (or the hardware's
+ * current state for drivers that support reading actual brightness).
+ *
+ * For atomic drivers, the luminance value is stored in
+ * &drm_connector_state.luminance. The actual hardware update only occurs
+ * when the connector is active (DPMS is ON). When DPMS transitions to OFF,
+ * the kernel automatically sets luminance to 0 to turn off the backlight.
+ * When DPMS transitions back to ON, the kernel restores the previously
+ * set luminance value.
+ *
+ * This property is only available on connectors that have an associated
+ * backlight device. The property is created by calling drm_backlight_alloc()
+ * during connector initialization.
+ *
+ * Client Capability:
+ * User-space must set the DRM_CLIENT_CAP_LUMINANCE client capability
+ * to 1 before using this property. When this capability is enabled,
+ * the legacy sysfs backlight interface is inhibited to prevent
+ * conflicts between multiple clients trying to control the same
+ * backlight. This ensures that only luminance-aware clients control
+ * the backlight through the DRM atomic interface.
+ *
+ * Legacy clients that do not set this capability will not see the
+ * LUMINANCE property and should continue using the sysfs interface
+ * (if available).
+ *
+ * Note: This property can be set through MODE_ATOMIC ioctl as part of the
+ * atomic state.
* panel_type:
* Immutable enum property to indicate the type of connected panel.
* Possible values are "unknown" (default) and "OLED".
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index ec820686b3021..4d2520de7614c 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -41,6 +41,7 @@
#include <linux/slab.h>
#include <linux/vga_switcheroo.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_client_event.h>
#include <drm/drm_drv.h>
#include <drm/drm_file.h>
@@ -252,6 +253,10 @@ void drm_file_free(struct drm_file *file)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
drm_fb_release(file);
drm_property_destroy_user_blobs(dev, file);
+ if (file->supports_luminance_control) {
+ drm_backlight_uninhibit_legacy_all(dev);
+ file->supports_luminance_control = false;
+ }
}
if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index ff193155129e7..fdae36b13300a 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -28,12 +28,14 @@
* OTHER DEALINGS IN THE SOFTWARE.
*/
+#include "drm/drm.h"
#include <linux/export.h>
#include <linux/nospec.h>
#include <linux/pci.h>
#include <linux/uaccess.h>
#include <drm/drm_auth.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_crtc.h>
#include <drm/drm_drv.h>
#include <drm/drm_file.h>
@@ -380,6 +382,19 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
return -EINVAL;
file_priv->plane_color_pipeline = req->value;
break;
+ case DRM_CLIENT_CAP_LUMINANCE:
+ if (!file_priv->atomic)
+ return -EINVAL;
+ if (req->value > 1)
+ return -EINVAL;
+ if (req->value == file_priv->supports_luminance_control)
+ break;
+ if (req->value)
+ drm_backlight_inhibit_legacy_all(dev);
+ else
+ drm_backlight_uninhibit_legacy_all(dev);
+ file_priv->supports_luminance_control = req->value;
+ break;
default:
return -EINVAL;
}
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index f38f2c5437e68..4475896c963b4 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -952,6 +952,12 @@ bool drm_property_change_valid_get(struct drm_property *property,
*ref = NULL;
if (drm_property_type_is(property, DRM_MODE_PROP_RANGE)) {
+ /* Special case for luminance property: allow 0 to turn off display
+ * even when the normal range starts at 1.
+ */
+ if (property == property->dev->mode_config.luminance_property &&
+ value == 0 && property->values[1] > 0)
+ return true;
if (value < property->values[0] || value > property->values[1])
return false;
return true;
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 13954c2220b7e..40cfc2296445c 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -217,6 +217,13 @@ static ssize_t brightness_store(struct device *dev,
struct backlight_device *bd = to_backlight_device(dev);
unsigned long brightness;
+ /* A luminance-aware DRM client has taken over this backlight; the
+ * legacy sysfs interface is disabled until the last such client
+ * goes away.
+ */
+ if (atomic_read(&bd->drm_takeover) > 0)
+ return -EBUSY;
+
rc = kstrtoul(buf, 0, &brightness);
if (rc)
return rc;
diff --git a/include/drm/drm_backlight.h b/include/drm/drm_backlight.h
index e0e09e38f7c06..2af48be3aa372 100644
--- a/include/drm/drm_backlight.h
+++ b/include/drm/drm_backlight.h
@@ -43,9 +43,9 @@ void drm_backlight_unregister(struct drm_backlight *b);
void drm_backlight_link(struct drm_backlight *b, struct backlight_device *bd);
struct backlight_device *drm_backlight_get_device(struct drm_backlight *b);
-void drm_backlight_set_luminance(struct drm_backlight *b, unsigned int value);
void drm_backlight_inhibit_legacy(struct drm_backlight *b);
void drm_backlight_uninhibit_legacy(struct drm_backlight *b);
void drm_backlight_inhibit_legacy_all(struct drm_device *dev);
void drm_backlight_uninhibit_legacy_all(struct drm_device *dev);
+void drm_backlight_set_luminance(struct drm_backlight *b, unsigned int value);
#endif /* __DRM_BACKLIGHT_H__ */
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index d78ac1068d12e..5e32b6063d228 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1209,6 +1209,11 @@ struct drm_connector_state {
* @drm_atomic_helper_connector_hdmi_check().
*/
struct drm_connector_hdmi_state hdmi;
+
+ /**
+ * @luminance: Luminance for the connector
+ */
+ unsigned int luminance;
};
struct drm_connector_hdmi_audio_funcs {
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 6ee70ad65e1fd..0bb1e53f36bec 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -248,6 +248,14 @@ struct drm_file {
*/
bool supports_virtualized_cursor_plane;
+ /**
+ * @supports_luminance_control:
+ *
+ * This client is capable of setting the luminance for connectors.
+ *
+ */
+ bool supports_luminance_control;
+
/**
* @master:
*
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 26a7281d179c1..b03737ee8dacd 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -197,6 +197,13 @@ struct backlight_properties {
*/
int brightness;
+ /**
+ * @can_disable: Minimum brightness would turn off the panel.
+ *
+ * Setting minimum brightness turns off the panel.
+ */
+ bool can_disable;
+
/**
* @max_brightness: The maximum brightness value.
*
@@ -314,6 +321,14 @@ struct backlight_device {
* @use_count: The number of unblanked displays.
*/
int use_count;
+
+ /**
+ * @drm_takeover: Number of luminance-aware DRM clients that have
+ * taken over brightness control of this device. When non-zero,
+ * writes to the legacy sysfs ``brightness`` attribute return
+ * ``-EBUSY``. Managed by the DRM backlight helpers.
+ */
+ atomic_t drm_takeover;
};
/* Forward declaration for backlight_update_status */
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index bc7ef7684099b..a3141d46d7d66 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -903,6 +903,28 @@ struct drm_get_cap {
*/
#define DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE 7
+/**
+ * DRM_CLIENT_CAP_LUMINANCE
+ *
+ * If set to 1, the client declares support for the LUMINANCE connector property
+ * and will control backlight brightness through the DRM atomic interface. This
+ * enables the kernel to expose the LUMINANCE property on connectors that have
+ * an associated backlight device.
+ *
+ * When this capability is enabled:
+ * - The LUMINANCE property becomes visible on supported connectors
+ * - Legacy sysfs writes to /sys/class/backlight/{*}/brightness will return
+ * -EBUSY to prevent conflicts with DRM-based brightness control
+ * - The client should include luminance values as part of atomic commits
+ * - Brightness changes are synchronized with display power state (DPMS)
+ *
+ * The LUMINANCE property accepts values from 0 to max_brightness, where 0 turns
+ * off the backlight, and 1 to max_brightness control the brightness level.
+ *
+ * This capability is supported starting in kernel 7.2.
+ */
+#define DRM_CLIENT_CAP_LUMINANCE 8
+
/* DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
struct drm_set_client_cap {
__u64 capability;
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 04/10] DRM: Add support for client indicating support for luminance
2026-06-24 16:57 ` [PATCH v6 04/10] DRM: Add support for client indicating support for luminance Mario Limonciello
@ 2026-06-24 17:12 ` sashiko-bot
0 siblings, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-06-24 17:12 UTC (permalink / raw)
To: Mario Limonciello; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] drm_atomic_connector_set_property() synchronously modifies the physical hardware backlight, violating the DRM atomic TEST_ONLY contract.
- [High] Concurrent DRM_IOCTL_SET_CLIENT_CAP calls can leak luminance_clients references, breaking legacy sysfs backlight control.
- [High] Missing dynamic tracking of hotplugged/unplugged connectors causes unbalanced capability inhibit counters, triggering kernel WARN_ON panics.
- [High] drm_atomic_connector_commit_dpms() incorrectly modifies physical backlight state before atomic commit execution, failing to rollback on errors.
- [High] drm_backlight_link() calls a sleeping function (kobject_uevent_env) while holding a spinlock.
- [High] __drm_backlight_update_prop_range() mutates a device-wide shared property object, corrupting bounds for multi-monitor setups.
- [High] __drm_backlight_real_changed() mutates the committed atomic state directly, introducing data races and potential Use-After-Free.
- [High] A sleeping function (drm_backlight_set_luminance) is called from atomic encoder enable/disable callbacks.
- [Medium] drm_atomic_helper_commit_encoder_bridge_enable() accesses global connector->state instead of new_conn_state.
--
commit 8af77079789b2e380cebbf99913aa0db9fe6c3ef
Author: Mario Limonciello (AMD) <superm1@kernel.org>
Date: Wed Jun 24 16:57:44 2026 +0000
DRM: Add support for client indicating support for luminance
This commit introduces a DRM client capability to indicate support for a
luminance property on connectors. When the capability is enabled, the kernel
inhibits the legacy sysfs backlight control to allow the DRM client to
manage backlight brightness via atomic state.
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 51f39edc31edc..17492ff6c5579 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1224,6 +1225,8 @@ drm_atomic_helper_commit_encoder_bridge_disable(struct drm_device *dev,
> * it away), so we won't call disable hooks twice.
> */
> bridge = drm_bridge_chain_get_first_bridge(encoder);
> + if (connector->backlight)
> + drm_backlight_set_luminance(connector->backlight, 0);
[Severity: High]
Does this code call a sleeping function from atomic context?
During a DRM atomic commit, drm_atomic_helper_commit_encoder_bridge_disable()
executes in atomic context where sleeping is strictly prohibited. Calling
drm_backlight_set_luminance() from here will acquire a mutex (bd->ops_lock),
which could lead to system instability or lockdep warnings.
> drm_atomic_bridge_chain_disable(bridge, state);
> drm_bridge_put(bridge);
>
> @@ -1738,6 +1741,10 @@ drm_atomic_helper_commit_encoder_bridge_enable(struct drm_device *dev, struct dr
>
> drm_atomic_bridge_chain_enable(bridge, state);
> drm_bridge_put(bridge);
> +
> + if (connector->backlight && connector->state)
> + drm_backlight_set_luminance(connector->backlight,
> + connector->state->luminance);
[Severity: High]
Can this sleeping function safely be called from the atomic encoder enable
callback? Like the disable hook above, this runs in atomic context, but
drm_backlight_set_luminance() attempts to take a mutex.
[Severity: Medium]
Could accessing the global connector->state instead of new_conn_state cause
a race condition here?
If commit_tail runs asynchronously for non-blocking commits, a subsequent
commit could swap the state pointers, causing the hardware to be programmed
with an invalid or future brightness value.
> }
> }
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 6441b55cc2740..ebf434bbd963b 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -935,6 +937,14 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> state->privacy_screen_sw_state = val;
> } else if (property == connector->broadcast_rgb_property) {
> state->hdmi.broadcast_rgb = val;
> + } else if (property == config->luminance_property) {
> + state->luminance = val;
> + /* Update hardware backlight only when DPMS is ON.
> + * Property value is always updated to remember the user's
> + * desired brightness.
> + */
> + if (connector->dpms == DRM_MODE_DPMS_ON)
> + drm_backlight_set_luminance(connector->backlight, val);
[Severity: High]
Does this synchronously modify the physical hardware backlight?
Calling drm_backlight_set_luminance() directly in the atomic property
assignment phase appears to break the DRM atomic TEST_ONLY contract, where
TEST_ONLY or failed commits should guarantee no side effects on the hardware.
> } else if (connector->funcs->atomic_set_property) {
> return connector->funcs->atomic_set_property(connector,
> state, property, val);
> @@ -1126,9 +1163,29 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_commit *state,
> if (connector->dpms == mode)
> goto out;
>
> + crtc = connector->state ? connector->state->crtc : NULL;
> +
> + /* Handle backlight brightness coordination with DPMS state changes */
> + if (old_mode != DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_OFF) {
> + /* DPMS ON -> OFF: dim all connectors driven by this CRTC. */
> + if (crtc)
> + drm_atomic_crtc_set_backlight(crtc, false);
> + else
> + drm_atomic_connector_set_backlight(connector, 0);
> + }
[Severity: High]
Does this modify the physical backlight state before the atomic commit
is executed?
If the subsequent drm_atomic_commit() fails, it looks like the hardware
state won't be rolled back, leaving the physical backlight off but the
software state unchanged.
Also, does dimming all connectors driven by this CRTC incorrectly affect
active displays in cloned setups when only one display was targeted by DPMS?
>
> connector->dpms = mode;
> diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
> index b1ec470be86ca..2cddf209d5088 100644
> --- a/drivers/gpu/drm/drm_backlight.c
> +++ b/drivers/gpu/drm/drm_backlight.c
> @@ -71,6 +71,7 @@ static bool __drm_backlight_is_registered(struct drm_backlight *b)
> /* caller must hold @drm_backlight_lock */
> static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
> {
> + struct drm_connector *connector = b->connector;
> unsigned int max, set;
>
> lockdep_assert_held(&drm_backlight_lock);
> @@ -85,6 +86,15 @@ static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
> set = v;
> if (set >= max)
> set = max;
> +
> + /* Update the atomic state directly.
> + * For atomic drivers, the luminance value is stored in
> + * connector->state->luminance, not in the legacy property array.
> + * We update it unconditionally to reflect the hardware state,
> + * regardless of DPMS.
> + */
> + if (connector->state)
> + connector->state->luminance = set;
[Severity: High]
Could this directly mutate the committed atomic state, introducing data
races or a Use-After-Free?
A concurrent atomic commit might swap and free connector->state while this
function modifies connector->state->luminance holding only drm_backlight_lock
instead of the connection_mutex.
> }
>
> /**
> @@ -100,18 +110,22 @@ static void __drm_backlight_update_prop_range(struct drm_backlight *b)
> struct drm_device *dev = b->connector->dev;
> struct drm_property *prop = dev->mode_config.luminance_property;
> unsigned int max = 0;
> + bool can_disable = false;
>
> lockdep_assert_held(&drm_backlight_lock);
>
> - if (b->link && b->link->props.max_brightness > 0)
> + if (b->link && b->link->props.max_brightness > 0) {
> max = b->link->props.max_brightness;
> + can_disable = b->link->props.can_disable;
> + }
>
> /* Update property range to match hardware capabilities.
> * Range of 0-0 indicates no backing device.
> - * Range of 1-max for normal operation (0 reserved for display off).
> + * Range of 1-max for normal operation.
> + * Range of 0-max means that the display would turn off at 0
> */
> if (prop->values[1] != max) {
> - prop->values[0] = max ? 1 : 0;
> + prop->values[0] = max ? (can_disable ? 0 : 1) : 0;
> prop->values[1] = max;
> }
[Severity: High]
Does this mutate a globally shared device-wide property based on a single
connector's backlight bounds?
Modifying dev->mode_config.luminance_property values directly could corrupt
the acceptable bounds for other displays in a multi-monitor setup where each
display has different max_brightness limits.
> }
> @@ -228,11 +253,18 @@ EXPORT_SYMBOL(drm_backlight_unregister);
> */
> void drm_backlight_link(struct drm_backlight *b, struct backlight_device *bd)
> {
> + static const char * const ep[] = { "BACKLIGHT=1", NULL };
> + bool send_uevent = false;
> +
> if (!b)
> return;
>
> guard(spinlock)(&drm_backlight_lock);
> - __drm_backlight_link(b, bd);
> + send_uevent = __drm_backlight_link(b, bd);
> +
> + if (send_uevent)
> + kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE,
> + (char **)ep);
[Severity: High]
Can kobject_uevent_env() sleep while the drm_backlight_lock spinlock is held?
The guard(spinlock) macro holds the lock for the entire scope, and
kobject_uevent_env() dynamically allocates memory with GFP_KERNEL and may call
user-mode helpers, potentially causing a scheduling while atomic panic.
> }
> EXPORT_SYMBOL(drm_backlight_link);
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index ec820686b3021..4d2520de7614c 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -252,6 +253,10 @@ void drm_file_free(struct drm_file *file)
> if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> drm_fb_release(file);
> drm_property_destroy_user_blobs(dev, file);
> + if (file->supports_luminance_control) {
> + drm_backlight_uninhibit_legacy_all(dev);
> + file->supports_luminance_control = false;
> + }
> }
[Severity: High]
Could a lack of dynamic tracking for hotplugged or unplugged connectors
cause unbalanced inhibit counters?
If a display connector is hotplugged or unplugged after a DRM client sets
DRM_CLIENT_CAP_LUMINANCE, iterating over the current connector list on file
close will apply unmatched decrements or leak references, eventually triggering
the WARN_ON panic in drm_backlight_uninhibit_legacy().
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index ff193155129e7..fdae36b13300a 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -380,6 +382,19 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
> return -EINVAL;
> file_priv->plane_color_pipeline = req->value;
> break;
> + case DRM_CLIENT_CAP_LUMINANCE:
> + if (!file_priv->atomic)
> + return -EINVAL;
> + if (req->value > 1)
> + return -EINVAL;
> + if (req->value == file_priv->supports_luminance_control)
> + break;
> + if (req->value)
> + drm_backlight_inhibit_legacy_all(dev);
> + else
> + drm_backlight_uninhibit_legacy_all(dev);
> + file_priv->supports_luminance_control = req->value;
> + break;
[Severity: High]
Does this check-and-set logic lack locking?
Concurrent threads invoking DRM_IOCTL_SET_CLIENT_CAP with
DRM_CLIENT_CAP_LUMINANCE on the same file descriptor could bypass the check
and unbalancedly increment the inhibit counter, leaking references and
breaking legacy sysfs backlight control.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624165751.2014759-1-mario.limonciello@amd.com?part=4
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 05/10] drm/amd/display: Pass up errors reading actual brightness
2026-06-24 16:57 [PATCH v6 00/10] Add support for a DRM backlight capability Mario Limonciello
` (3 preceding siblings ...)
2026-06-24 16:57 ` [PATCH v6 04/10] DRM: Add support for client indicating support for luminance Mario Limonciello
@ 2026-06-24 16:57 ` Mario Limonciello
2026-06-24 16:57 ` [PATCH v6 06/10] drm/amd/display: Allow backlight registration to fail Mario Limonciello
` (5 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Mario Limonciello @ 2026-06-24 16:57 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Mario Limonciello (AMD), Simon Ser
From: "Mario Limonciello (AMD)" <superm1@kernel.org>
[Why]
If the DC API fails to return actual brightness when backlight control
API requests it, then the wrong value may be returned.
[How]
Change return type of amdgpu_dm_backlight_get_level() to an integer
and pass an error code up to the caller.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
Tested-by: Simon Ser <contact@emersion.fr>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
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 eb5696b5daeb7..9b3e2fc6cef5d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5492,7 +5492,7 @@ static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
return 0;
}
-static u32 amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm,
+static int amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm,
int bl_idx)
{
int ret;
@@ -5506,14 +5506,14 @@ static u32 amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm,
u32 avg, peak;
if (!dc_link_get_backlight_level_nits(link, &avg, &peak))
- return dm->brightness[bl_idx];
+ return -EINVAL;
return convert_brightness_to_user(&caps, avg);
}
ret = dc_link_get_backlight_level(link);
if (ret == DC_ERROR_UNEXPECTED)
- return dm->brightness[bl_idx];
+ return -EINVAL;
return convert_brightness_to_user(&caps, ret);
}
@@ -5521,7 +5521,7 @@ static u32 amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm,
static int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
{
struct amdgpu_display_manager *dm = bl_get_data(bd);
- int i;
+ int i, ret;
for (i = 0; i < dm->num_of_edps; i++) {
if (bd == dm->backlight_dev[i])
@@ -5529,7 +5529,12 @@ static int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
}
if (i >= AMDGPU_DM_MAX_NUM_EDP)
i = 0;
- return amdgpu_dm_backlight_get_level(dm, i);
+
+ ret = amdgpu_dm_backlight_get_level(dm, i);
+ if (ret < 0)
+ return dm->brightness[i];
+
+ return ret;
}
static const struct backlight_ops amdgpu_dm_backlight_ops = {
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v6 06/10] drm/amd/display: Allow backlight registration to fail
2026-06-24 16:57 [PATCH v6 00/10] Add support for a DRM backlight capability Mario Limonciello
` (4 preceding siblings ...)
2026-06-24 16:57 ` [PATCH v6 05/10] drm/amd/display: Pass up errors reading actual brightness Mario Limonciello
@ 2026-06-24 16:57 ` Mario Limonciello
2026-06-24 17:20 ` sashiko-bot
2026-06-24 16:57 ` [PATCH v6 07/10] drm/amd/display: use drm backlight Mario Limonciello
` (4 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Mario Limonciello @ 2026-06-24 16:57 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Mario Limonciello (AMD), Simon Ser
From: "Mario Limonciello (AMD)" <superm1@kernel.org>
[Why]
If backlight registration fails then it's ignored. This could hide
a fatal problem to a user.
[How]
Pass error codes up from backlight registration failures.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
Tested-by: Simon Ser <contact@emersion.fr>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 41 +++++++++++--------
1 file changed, 24 insertions(+), 17 deletions(-)
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 9b3e2fc6cef5d..2f87940aaea42 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5543,7 +5543,7 @@ static const struct backlight_ops amdgpu_dm_backlight_ops = {
.update_status = amdgpu_dm_backlight_update_status,
};
-static void
+static int
amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector)
{
struct drm_device *drm = aconnector->base.dev;
@@ -5554,15 +5554,16 @@ amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector)
int min, max;
int real_brightness;
int init_brightness;
+ int r;
if (aconnector->bl_idx == -1)
- return;
+ return 0;
if (!acpi_video_backlight_use_native()) {
drm_info(drm, "Skipping amdgpu DM backlight registration\n");
/* Try registering an ACPI video backlight device instead. */
acpi_video_register_backlight();
- return;
+ return 0;
}
caps = &dm->backlight_caps[aconnector->bl_idx];
@@ -5596,22 +5597,26 @@ amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector)
dm->brightness[aconnector->bl_idx] = props.brightness;
if (IS_ERR(dm->backlight_dev[aconnector->bl_idx])) {
- drm_err(drm, "DM: Backlight registration failed!\n");
+ r = PTR_ERR(dm->backlight_dev[aconnector->bl_idx]);
+ drm_err(drm, "DM: Backlight registration failed: %d\n", r);
dm->backlight_dev[aconnector->bl_idx] = NULL;
- } else {
- /*
- * dm->brightness[x] can be inconsistent just after startup until
- * ops.get_brightness is called.
- */
- real_brightness =
- amdgpu_dm_backlight_ops.get_brightness(dm->backlight_dev[aconnector->bl_idx]);
+ return r;
+ }
- if (real_brightness != init_brightness) {
- dm->actual_brightness[aconnector->bl_idx] = real_brightness;
- dm->brightness[aconnector->bl_idx] = real_brightness;
- }
- drm_dbg_driver(drm, "DM: Registered Backlight device: %s\n", bl_name);
+ /*
+ * dm->brightness[x] can be inconsistent just after startup until
+ * ops.get_brightness is called.
+ */
+ real_brightness =
+ amdgpu_dm_backlight_ops.get_brightness(dm->backlight_dev[aconnector->bl_idx]);
+
+ if (real_brightness != init_brightness) {
+ dm->actual_brightness[aconnector->bl_idx] = real_brightness;
+ dm->brightness[aconnector->bl_idx] = real_brightness;
}
+ drm_dbg_driver(drm, "DM: Registered Backlight device: %s\n", bl_name);
+
+ return 0;
}
static int initialize_plane(struct amdgpu_display_manager *dm,
@@ -8225,7 +8230,9 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector)
return r;
}
- amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
+ r = amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
+ if (r)
+ return r;
if ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) ||
(connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 06/10] drm/amd/display: Allow backlight registration to fail
2026-06-24 16:57 ` [PATCH v6 06/10] drm/amd/display: Allow backlight registration to fail Mario Limonciello
@ 2026-06-24 17:20 ` sashiko-bot
0 siblings, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-06-24 17:20 UTC (permalink / raw)
To: Mario Limonciello; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Propagating backlight registration failures causes the entire AMDGPU driver probe to fail, completely breaking all display outputs.
--
commit 290c274dbf0e6988a8d83dadd2c54c417b03e056
Author: Mario Limonciello (AMD) <superm1@kernel.org>
drm/amd/display: Allow backlight registration to fail
This commit changes backlight registration in the AMD display manager to
return error codes instead of ignoring them. This allows upstream callers
to detect when backlight initialization fails.
> 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 9b3e2fc6cef5d..2f87940aaea42 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
[ ... ]
> @@ -8225,7 +8230,9 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector)
> return r;
> }
>
> - amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
> + r = amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
> + if (r)
> + return r;
[Severity: High]
Will returning this error abort the entire GPU probe?
If backlight_device_register() fails in amdgpu_dm_register_backlight_device()
(for instance, due to a sysfs name collision, memory allocation failure, or
ACPI issue), propagating this error here causes drm_connector_register() and
ultimately drm_dev_register() to fail.
Because generic framebuffers like efifb or simpledrm are explicitly removed
by aperture_remove_conflicting_pci_devices() before drm_dev_register() is
called, does failing the PCI probe at this stage leave the system with a
dead screen and no display driver at all?
Should a non-critical backlight initialization failure be treated as fatal to
the entire display adapter?
>
> if ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) ||
> (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624165751.2014759-1-mario.limonciello@amd.com?part=6
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 07/10] drm/amd/display: use drm backlight
2026-06-24 16:57 [PATCH v6 00/10] Add support for a DRM backlight capability Mario Limonciello
` (5 preceding siblings ...)
2026-06-24 16:57 ` [PATCH v6 06/10] drm/amd/display: Allow backlight registration to fail Mario Limonciello
@ 2026-06-24 16:57 ` Mario Limonciello
2026-06-24 17:19 ` sashiko-bot
2026-06-24 16:57 ` [PATCH v6 08/10] drm/amd/display: Drop brightness caching in amdgpu_dm Mario Limonciello
` (3 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Mario Limonciello @ 2026-06-24 16:57 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Mario Limonciello (AMD), Simon Ser
From: "Mario Limonciello (AMD)" <superm1@kernel.org>
Convert AMD display driver to use the new DRM backlight infrastructure.
This allows brightness control via DRM connector properties.
Includes fixes for backlight resource lifecycle management.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
Tested-by: Simon Ser <contact@emersion.fr>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 21 +++++++++++++++++++
1 file changed, 21 insertions(+)
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 2f87940aaea42..9653cfd99c6ad 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -86,6 +86,7 @@
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_uapi.h>
#include <drm/drm_atomic_helper.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_blend.h>
#include <drm/drm_fixed.h>
#include <drm/drm_fourcc.h>
@@ -5614,6 +5615,15 @@ amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector)
dm->actual_brightness[aconnector->bl_idx] = real_brightness;
dm->brightness[aconnector->bl_idx] = real_brightness;
}
+
+ /* Link the registered backlight device to the DRM connector. If
+ * drm_backlight_alloc() failed earlier in init_helper,
+ * aconnector->base.backlight is NULL and drm_backlight_link() is a
+ * no-op.
+ */
+ drm_backlight_link(aconnector->base.backlight,
+ dm->backlight_dev[aconnector->bl_idx]);
+
drm_dbg_driver(drm, "DM: Registered Backlight device: %s\n", bl_name);
return 0;
@@ -8140,6 +8150,7 @@ static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
}
if (aconnector->bl_idx != -1) {
+ drm_backlight_link(aconnector->base.backlight, NULL);
backlight_device_unregister(dm->backlight_dev[aconnector->bl_idx]);
dm->backlight_dev[aconnector->bl_idx] = NULL;
}
@@ -8152,6 +8163,10 @@ static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
aconnector->dc_sink = NULL;
drm_dp_cec_unregister_connector(&aconnector->dm_dp_aux.aux);
+ /* DRM core will handle drm_backlight cleanup via:
+ * drm_connector_unregister() -> drm_backlight_unregister()
+ * drm_connector_cleanup() -> drm_backlight_free()
+ */
drm_connector_unregister(connector);
drm_connector_cleanup(connector);
kfree(aconnector->dm_dp_aux.aux.name);
@@ -9402,6 +9417,12 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
aconnector->base.state->max_bpc = 16;
aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc;
+ if (connector_type == DRM_MODE_CONNECTOR_eDP) {
+ int r = drm_backlight_alloc(&aconnector->base);
+
+ if (r)
+ drm_err(dm->ddev, "Failed to allocate backlight: %d\n", r);
+ }
if (connector_type == DRM_MODE_CONNECTOR_HDMIA) {
/* Content Type is currently only implemented for HDMI. */
drm_connector_attach_content_type_property(&aconnector->base);
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 07/10] drm/amd/display: use drm backlight
2026-06-24 16:57 ` [PATCH v6 07/10] drm/amd/display: use drm backlight Mario Limonciello
@ 2026-06-24 17:19 ` sashiko-bot
0 siblings, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-06-24 17:19 UTC (permalink / raw)
To: Mario Limonciello; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Redundant call to drm_connector_unregister() in the connector destroy hook triggers a kernel WARN_ON during driver unload.
- [Medium] LVDS connectors are missing the DRM backlight property allocation, leaving them without the new DRM-level brightness control capabilities.
Pre-existing issues:
- [High] Pre-existing error path Use-After-Free, Double Free, and memory leak when device initialization fails.
--
commit 1adb86b656e02ccab8cb2193136ef89ff549cddf
Author: Mario Limonciello (AMD) <superm1@kernel.org>
Subject: drm/amd/display: use drm backlight
This commit converts the AMD display driver to use the new DRM backlight
infrastructure to allow brightness control via DRM connector properties.
It also includes fixes for backlight resource lifecycle management.
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
[ ... ]
> @@ -8152,6 +8163,10 @@ static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
> aconnector->dc_sink = NULL;
>
> drm_dp_cec_unregister_connector(&aconnector->dm_dp_aux.aux);
> + /* DRM core will handle drm_backlight cleanup via:
> + * drm_connector_unregister() -> drm_backlight_unregister()
> + * drm_connector_cleanup() -> drm_backlight_free()
> + */
> drm_connector_unregister(connector);
> drm_connector_cleanup(connector);
[Severity: High]
Does this redundant unregister trigger a WARN_ON during driver unload?
When the driver is removed, drm_dev_unregister() automatically calls
drm_connector_unregister_all(), which unregisters the connector and its
newly added backlight object via drm_backlight_unregister().
Later, when the DRM device is cleaned up and the connector reference drops
to zero, amdgpu_dm_connector_destroy() is called. Because
drm_connector_unregister() calls drm_backlight_unregister() before checking
if the connector is already unregistered, it will attempt to unregister the
backlight a second time. This asserts a warning because the backlight was
already removed from the list.
[ ... ]
> @@ -9402,6 +9417,12 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
> aconnector->base.state->max_bpc = 16;
> aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc;
>
> + if (connector_type == DRM_MODE_CONNECTOR_eDP) {
> + int r = drm_backlight_alloc(&aconnector->base);
> +
> + if (r)
> + drm_err(dm->ddev, "Failed to allocate backlight: %d\n", r);
> + }
[Severity: Medium]
Are LVDS connectors meant to be excluded from the new backlight allocation?
The AMD display driver also supports backlight control for legacy LVDS
panels, but drm_backlight_alloc() is explicitly restricted to eDP here.
Since it is skipped for LVDS, the connector's backlight pointer remains
NULL, and LVDS panels will not expose the new DRM luminance properties.
[ ... ]
While reviewing these changes, I also noticed an issue in the error handling
path of amdgpu_dm_initialize_drm_device() in the same file:
> fail:
> kfree(aencoder);
> kfree(aconnector);
>
> return -EINVAL;
[Severity: High]
This is a pre-existing issue, but can this error path lead to a
use-after-free and double-free?
If a late initialization step fails after a connector is successfully
created and registered into the global drm_device connector list, the
code jumps to the fail label and frees aconnector.
This leaves a dangling pointer in the global DRM connector list. During
the subsequent error rollback via drm_mode_config_cleanup(), the DRM core
will iterate over the connector list and invoke
amdgpu_dm_connector_destroy(), which accesses the freed memory and
attempts to free it again. Additionally, the newly added backlight
allocation would be leaked here.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624165751.2014759-1-mario.limonciello@amd.com?part=7
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 08/10] drm/amd/display: Drop brightness caching in amdgpu_dm
2026-06-24 16:57 [PATCH v6 00/10] Add support for a DRM backlight capability Mario Limonciello
` (6 preceding siblings ...)
2026-06-24 16:57 ` [PATCH v6 07/10] drm/amd/display: use drm backlight Mario Limonciello
@ 2026-06-24 16:57 ` Mario Limonciello
2026-06-24 17:16 ` sashiko-bot
2026-06-24 16:57 ` [PATCH v6 09/10] drm/bridge: auto-link panel backlight in bridge connector Mario Limonciello
` (2 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Mario Limonciello @ 2026-06-24 16:57 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Mario Limonciello (AMD)
From: "Mario Limonciello (AMD)" <superm1@kernel.org>
[Why]
Brightness caching isn't necessary when the core is changing brightness
as part of the connector state.
[How]
Drop all brightness and actual brightness caching
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 72 +++----------------
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 13 ----
2 files changed, 9 insertions(+), 76 deletions(-)
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 9653cfd99c6ad..43b22ab9180e8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3798,12 +3798,6 @@ static int dm_resume(struct amdgpu_ip_block *ip_block)
mutex_unlock(&dm->dc_lock);
- /* set the backlight after a reset */
- for (i = 0; i < dm->num_of_edps; i++) {
- if (dm->backlight_dev[i])
- amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
- }
-
return 0;
}
/* Recreate dc_state - DC invalidates it when setting power state to S3. */
@@ -5388,7 +5382,7 @@ static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
struct amdgpu_dm_backlight_caps *caps;
struct dc_link *link;
u32 brightness = 0;
- bool rc = false, reallow_idle = false;
+ bool reallow_idle = false;
struct drm_connector *connector;
struct dc_stream_state *stream;
unsigned int min, max;
@@ -5399,10 +5393,8 @@ static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
if (aconnector->bl_idx != bl_idx)
continue;
- /* if connector is off, save the brightness for next time it's on */
+ /* if connector is off, DRM core will restore it next time it's on */
if (!aconnector->base.encoder) {
- dm->brightness[bl_idx] = user_brightness;
- dm->actual_brightness[bl_idx] = 0;
return;
}
}
@@ -5410,11 +5402,10 @@ static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
amdgpu_dm_update_backlight_caps(dm, bl_idx);
caps = &dm->backlight_caps[bl_idx];
- dm->brightness[bl_idx] = user_brightness;
/* update scratch register */
if (bl_idx == 0)
- amdgpu_atombios_scratch_regs_set_backlight_level(dm->adev, dm->brightness[bl_idx]);
- brightness = convert_brightness_from_user(caps, dm->brightness[bl_idx]);
+ amdgpu_atombios_scratch_regs_set_backlight_level(dm->adev, user_brightness);
+ brightness = convert_brightness_from_user(caps, user_brightness);
link = (struct dc_link *)dm->backlight_link[bl_idx];
/* Apply brightness quirk */
@@ -5440,14 +5431,14 @@ static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
}
if (caps->aux_support) {
- rc = mod_power_set_backlight_nits(dm->power_module, stream, brightness,
+ mod_power_set_backlight_nits(dm->power_module, stream, brightness,
AUX_BL_DEFAULT_TRANSITION_TIME_MS, false, true);
} else {
/* power module uses millipercent */
get_brightness_range(caps, &min, &max);
brightness = DIV_ROUND_CLOSEST(brightness * 100, (max - min)) * 1000;
- rc = mod_power_set_backlight_percent(dm->power_module, stream,
- brightness, 0, false);
+ mod_power_set_backlight_percent(dm->power_module, stream,
+ brightness, 0, false);
}
/*
@@ -5472,9 +5463,6 @@ static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
dc_allow_idle_optimizations(dm->dc, true);
mutex_unlock(&dm->dc_lock);
-
- if (rc)
- dm->actual_brightness[bl_idx] = user_brightness;
}
static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
@@ -5522,7 +5510,7 @@ static int amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm,
static int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
{
struct amdgpu_display_manager *dm = bl_get_data(bd);
- int i, ret;
+ int i;
for (i = 0; i < dm->num_of_edps; i++) {
if (bd == dm->backlight_dev[i])
@@ -5531,11 +5519,7 @@ static int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
if (i >= AMDGPU_DM_MAX_NUM_EDP)
i = 0;
- ret = amdgpu_dm_backlight_get_level(dm, i);
- if (ret < 0)
- return dm->brightness[i];
-
- return ret;
+ return amdgpu_dm_backlight_get_level(dm, i);
}
static const struct backlight_ops amdgpu_dm_backlight_ops = {
@@ -5553,8 +5537,6 @@ amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector)
struct amdgpu_dm_backlight_caps *caps;
char bl_name[16];
int min, max;
- int real_brightness;
- int init_brightness;
int r;
if (aconnector->bl_idx == -1)
@@ -5580,8 +5562,6 @@ amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector)
} else
props.brightness = props.max_brightness = MAX_BACKLIGHT_LEVEL;
- init_brightness = props.brightness;
-
if (caps->data_points && !(amdgpu_dc_debug_mask & DC_DISABLE_CUSTOM_BRIGHTNESS_CURVE)) {
drm_info(drm, "Using custom brightness curve\n");
props.scale = BACKLIGHT_SCALE_NON_LINEAR;
@@ -5595,7 +5575,6 @@ amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector)
dm->backlight_dev[aconnector->bl_idx] =
backlight_device_register(bl_name, aconnector->base.kdev, dm,
&amdgpu_dm_backlight_ops, &props);
- dm->brightness[aconnector->bl_idx] = props.brightness;
if (IS_ERR(dm->backlight_dev[aconnector->bl_idx])) {
r = PTR_ERR(dm->backlight_dev[aconnector->bl_idx]);
@@ -5604,18 +5583,6 @@ amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector)
return r;
}
- /*
- * dm->brightness[x] can be inconsistent just after startup until
- * ops.get_brightness is called.
- */
- real_brightness =
- amdgpu_dm_backlight_ops.get_brightness(dm->backlight_dev[aconnector->bl_idx]);
-
- if (real_brightness != init_brightness) {
- dm->actual_brightness[aconnector->bl_idx] = real_brightness;
- dm->brightness[aconnector->bl_idx] = real_brightness;
- }
-
/* Link the registered backlight device to the DRM connector. If
* drm_backlight_alloc() failed earlier in init_helper,
* aconnector->base.backlight is NULL and drm_backlight_link() is a
@@ -10894,7 +10861,6 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_commit *state,
bool mode_set_reset_required = false;
u32 i;
struct dc_commit_streams_params params = {dc_state->streams, dc_state->stream_count};
- bool set_backlight_level = false;
/* Disable writeback */
for_each_old_connector_in_state(state, connector, old_con_state, i) {
@@ -11016,7 +10982,6 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_commit *state,
acrtc->hw_mode = new_crtc_state->mode;
crtc->hwmode = new_crtc_state->mode;
mode_set_reset_required = true;
- set_backlight_level = true;
} else if (modereset_required(new_crtc_state)) {
drm_dbg_atomic(dev,
"Atomic commit: RESET. crtc id %d:[%p]\n",
@@ -11085,18 +11050,6 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_commit *state,
}
}
- /* During boot up and resume the DC layer will reset the panel brightness
- * to fix a flicker issue.
- * It will cause the dm->actual_brightness is not the current panel brightness
- * level. (the dm->brightness is the correct panel level)
- * So we set the backlight level with dm->brightness value after set mode
- */
- if (set_backlight_level) {
- for (i = 0; i < dm->num_of_edps; i++) {
- if (dm->backlight_dev[i])
- amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
- }
- }
}
static void dm_set_writeback(struct amdgpu_display_manager *dm,
@@ -11604,13 +11557,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
/* Update audio instances for each connector. */
amdgpu_dm_commit_audio(dev, state);
- /* restore the backlight level */
- for (i = 0; i < dm->num_of_edps; i++) {
- if (dm->backlight_dev[i] &&
- (dm->actual_brightness[i] != dm->brightness[i]))
- amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
- }
-
/*
* send vblank event on all events not handled in flip and
* mark consumed event for drm_atomic_helper_commit_hw_done
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index dd199e0b79226..1aeb223256fa7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -645,19 +645,6 @@ struct amdgpu_display_manager {
struct completion dmub_aux_transfer_done;
struct workqueue_struct *delayed_hpd_wq;
- /**
- * @brightness:
- *
- * cached backlight values.
- */
- u32 brightness[AMDGPU_DM_MAX_NUM_EDP];
- /**
- * @actual_brightness:
- *
- * last successfully applied backlight values.
- */
- u32 actual_brightness[AMDGPU_DM_MAX_NUM_EDP];
-
/**
* @aux_hpd_discon_quirk:
*
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 08/10] drm/amd/display: Drop brightness caching in amdgpu_dm
2026-06-24 16:57 ` [PATCH v6 08/10] drm/amd/display: Drop brightness caching in amdgpu_dm Mario Limonciello
@ 2026-06-24 17:16 ` sashiko-bot
0 siblings, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-06-24 17:16 UTC (permalink / raw)
To: Mario Limonciello; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] Removal of brightness caching and restoration logic causes the display to remain at incorrect brightness levels after modesets, DPMS ON, and GPU resets, and permanently drops backlight changes made while the display is off.
Pre-existing issues:
- [High] Unsafe iteration of the global DRM `connector_list` without holding `mode_config.connector_list_lock` or using the `drm_connector_list_iter` API.
- [High] Unprotected access to `dm->dc->current_state` and its streams can lead to use-after-free during concurrent atomic commits.
- [High] Missing `dc_lock` and IPS (Idle Power Save) handling in `amdgpu_dm_backlight_get_level`.
--
commit 38d7d185db9bb8880779014318e409480bde8a6d
Author: Mario Limonciello (AMD) <superm1@kernel.org>
drm/amd/display: Drop brightness caching in amdgpu_dm
This commit drops brightness and actual_brightness caching from amdgpu_dm,
relying on the DRM core to restore brightness when a connector is turned
back on.
> 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 9653cfd99c6ad..43b22ab9180e8 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
[ ... ]
> @@ -5399,10 +5393,8 @@ static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
> if (aconnector->bl_idx != bl_idx)
> continue;
>
[Severity: High]
This isn't a bug introduced by this patch, but is it safe to iterate the global
connector_list in this loop without holding mode_config.connector_list_lock or
using the drm_connector_list_iter API?
Could this lead to memory corruption if there is a concurrent DP MST hot-unplug
while a client reads or writes the sysfs backlight property?
> - /* if connector is off, save the brightness for next time it's on */
> + /* if connector is off, DRM core will restore it next time it's on */
> if (!aconnector->base.encoder) {
> - dm->brightness[bl_idx] = user_brightness;
> - dm->actual_brightness[bl_idx] = 0;
> return;
> }
> }
[Severity: High]
Is relying on the DRM core to restore the backlight sufficient here?
Since amdgpu implements a custom atomic commit tail that bypasses the standard
DRM core restoration helpers, it seems changes made while DPMS is off are
never pushed to the hardware.
> @@ -5410,11 +5402,10 @@ static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
> amdgpu_dm_update_backlight_caps(dm, bl_idx);
> caps = &dm->backlight_caps[bl_idx];
>
> - dm->brightness[bl_idx] = user_brightness;
> /* update scratch register */
> if (bl_idx == 0)
> - amdgpu_atombios_scratch_regs_set_backlight_level(dm->adev, dm->brightness[bl_idx]);
> - brightness = convert_brightness_from_user(caps, dm->brightness[bl_idx]);
> + amdgpu_atombios_scratch_regs_set_backlight_level(dm->adev, user_brightness);
> + brightness = convert_brightness_from_user(caps, user_brightness);
> link = (struct dc_link *)dm->backlight_link[bl_idx];
>
> /* Apply brightness quirk */
[Severity: High]
This is a pre-existing issue, but does the unprotected access to the stream
returned downstream from dm_find_stream_with_link() risk a use-after-free?
If a concurrent atomic commit modifies current_state and releases the old
dc_stream_state objects via dc_stream_release() before
amdgpu_dm_backlight_set_level() passes the stream downstream, the memory
might be freed.
[ ... ]
> @@ -5522,7 +5510,7 @@ static int amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm,
> static int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
> {
> struct amdgpu_display_manager *dm = bl_get_data(bd);
> - int i, ret;
> + int i;
>
> for (i = 0; i < dm->num_of_edps; i++) {
> if (bd == dm->backlight_dev[i])
> @@ -5531,11 +5519,7 @@ static int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
> if (i >= AMDGPU_DM_MAX_NUM_EDP)
> i = 0;
>
> - ret = amdgpu_dm_backlight_get_level(dm, i);
> - if (ret < 0)
> - return dm->brightness[i];
> -
> - return ret;
> + return amdgpu_dm_backlight_get_level(dm, i);
> }
[Severity: High]
This is a pre-existing issue, but does amdgpu_dm_backlight_get_level() need
to acquire dc_lock and call dc_allow_idle_optimizations() before reading
hardware registers downstream?
If the hardware is in a low-power state when a user reads the sysfs brightness
attribute, could this missing lock lead to a hardware hang or MMIO timeout?
[ ... ]
> @@ -11085,18 +11050,6 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_commit *state,
> }
> }
>
> - /* During boot up and resume the DC layer will reset the panel brightness
> - * to fix a flicker issue.
> - * It will cause the dm->actual_brightness is not the current panel brightness
> - * level. (the dm->brightness is the correct panel level)
> - * So we set the backlight level with dm->brightness value after set mode
> - */
> - if (set_backlight_level) {
> - for (i = 0; i < dm->num_of_edps; i++) {
> - if (dm->backlight_dev[i])
> - amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
> - }
> - }
> }
[Severity: High]
Without this restoration loop in amdgpu_dm_commit_streams(), does the driver
fail to correct the hardware brightness after the DC layer internally resets
it during modesets?
It appears the backlight might remain at incorrect hardware default or reset
levels upon resume or modeset.
[ ... ]
> @@ -11604,13 +11557,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
> /* Update audio instances for each connector. */
> amdgpu_dm_commit_audio(dev, state);
>
> - /* restore the backlight level */
> - for (i = 0; i < dm->num_of_edps; i++) {
> - if (dm->backlight_dev[i] &&
> - (dm->actual_brightness[i] != dm->brightness[i]))
> - amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
> - }
> -
> /*
> * send vblank event on all events not handled in flip and
> * mark consumed event for drm_atomic_helper_commit_hw_done
[Severity: High]
Does removing the explicit manual backlight restoration from
amdgpu_dm_atomic_commit_tail() cause the pending backlight level to never be
pushed to hardware when the display is enabled?
Because this custom atomic commit tail bypasses standard DRM core restoration
helpers, it seems changes made while DPMS is off are permanently lost without
this loop.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624165751.2014759-1-mario.limonciello@amd.com?part=8
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 09/10] drm/bridge: auto-link panel backlight in bridge connector
2026-06-24 16:57 [PATCH v6 00/10] Add support for a DRM backlight capability Mario Limonciello
` (7 preceding siblings ...)
2026-06-24 16:57 ` [PATCH v6 08/10] drm/amd/display: Drop brightness caching in amdgpu_dm Mario Limonciello
@ 2026-06-24 16:57 ` Mario Limonciello
2026-06-24 17:13 ` sashiko-bot
2026-06-24 16:57 ` [PATCH v6 10/10] drm/i915/display: use drm backlight Mario Limonciello
2026-06-24 18:12 ` ✗ Fi.CI.BUILD: failure for Add support for a DRM backlight capability Patchwork
10 siblings, 1 reply; 21+ messages in thread
From: Mario Limonciello @ 2026-06-24 16:57 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Mario Limonciello (AMD), Dmitry Baryshkov, Simon Ser
From: "Mario Limonciello (AMD)" <superm1@kernel.org>
The link should allow clients to change luminance property.
Tested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> # SM8150-HDK
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
Tested-by: Simon Ser <contact@emersion.fr>
---
drivers/gpu/drm/bridge/panel.c | 15 +++++++++++++++
drivers/gpu/drm/display/drm_bridge_connector.c | 15 ++++++++++++++-
include/drm/drm_bridge.h | 1 +
3 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 4978ec98a0828..2fa15278faf87 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -233,6 +233,21 @@ bool drm_bridge_is_panel(const struct drm_bridge *bridge)
}
EXPORT_SYMBOL(drm_bridge_is_panel);
+/**
+ * drm_panel_bridge_to_panel - get the drm_panel wrapped by a panel bridge
+ * @bridge: the panel bridge
+ *
+ * Returns the &drm_panel wrapped by @bridge, or NULL if @bridge is not a
+ * panel bridge.
+ */
+struct drm_panel *drm_panel_bridge_to_panel(struct drm_bridge *bridge)
+{
+ if (!drm_bridge_is_panel(bridge))
+ return NULL;
+ return drm_bridge_to_panel_bridge(bridge)->panel;
+}
+EXPORT_SYMBOL(drm_panel_bridge_to_panel);
+
/**
* drm_panel_bridge_add - Creates a &drm_bridge and &drm_connector that
* just calls the appropriate functions from &drm_panel.
diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 649969fca1413..23a3802294625 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -11,7 +11,9 @@
#include <linux/slab.h>
#include <drm/drm_atomic_state_helper.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_bridge.h>
+#include <drm/drm_panel.h>
#include <drm/drm_bridge_connector.h>
#include <drm/drm_connector.h>
#include <drm/drm_device.h>
@@ -1051,9 +1053,20 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
connector->polled = DRM_CONNECTOR_POLL_CONNECT
| DRM_CONNECTOR_POLL_DISCONNECT;
- if (panel_bridge)
+ if (panel_bridge) {
+ struct drm_panel *panel;
+
drm_panel_bridge_set_orientation(connector, panel_bridge);
+ panel = drm_panel_bridge_to_panel(panel_bridge);
+ if (panel && panel->backlight) {
+ ret = drm_backlight_alloc(connector);
+ if (!ret)
+ drm_backlight_link(connector->backlight,
+ panel->backlight);
+ }
+ }
+
if (support_hdcp && IS_REACHABLE(CONFIG_DRM_DISPLAY_HELPER) &&
IS_ENABLED(CONFIG_DRM_DISPLAY_HDCP_HELPER))
drm_connector_attach_content_protection_property(connector, true);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4ba3a5deef9a6..6311ca726b45f 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1586,6 +1586,7 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
#ifdef CONFIG_DRM_PANEL_BRIDGE
bool drm_bridge_is_panel(const struct drm_bridge *bridge);
+struct drm_panel *drm_panel_bridge_to_panel(struct drm_bridge *bridge);
struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel);
struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
u32 connector_type);
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 09/10] drm/bridge: auto-link panel backlight in bridge connector
2026-06-24 16:57 ` [PATCH v6 09/10] drm/bridge: auto-link panel backlight in bridge connector Mario Limonciello
@ 2026-06-24 17:13 ` sashiko-bot
0 siblings, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-06-24 17:13 UTC (permalink / raw)
To: Mario Limonciello; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] NULL pointer dereference when calling drm_backlight_link() on an unregistered connector
- [Medium] Unconditional overwrite of return value 'ret' ignores drm_backlight_alloc() failures
- [Medium] Reference leak and WARN_ON if drm_connector_attach_encoder() fails
- [Low] Build failure due to missing static inline fallback for drm_panel_bridge_to_panel()
Pre-existing issues:
- [High] Sleeping in atomic context when drm_backlight_link() sends a uevent while holding a spinlock
--
commit 97d3e5fef0b8faac0a3e7bb0d3351e3d883fc3e8
Author: Mario Limonciello (AMD) <superm1@kernel.org>
drm/bridge: auto-link panel backlight in bridge connector
The link should allow clients to change luminance property.
Tested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> # SM8150-HDK
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
Tested-by: Simon Ser <contact@emersion.fr>
> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 649969fca1413..23a3802294625 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
[ ... ]
> @@ -1051,9 +1053,20 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> connector->polled = DRM_CONNECTOR_POLL_CONNECT
> | DRM_CONNECTOR_POLL_DISCONNECT;
>
> - if (panel_bridge)
> + if (panel_bridge) {
> + struct drm_panel *panel;
> +
> drm_panel_bridge_set_orientation(connector, panel_bridge);
>
> + panel = drm_panel_bridge_to_panel(panel_bridge);
> + if (panel && panel->backlight) {
> + ret = drm_backlight_alloc(connector);
> + if (!ret)
> + drm_backlight_link(connector->backlight,
> + panel->backlight);
[Severity: Medium]
If drm_backlight_alloc() returns an error code like -ENOMEM, is it safe to
ignore it?
The error code stored in 'ret' appears to be unconditionally overwritten by the
subsequent call to drm_connector_attach_encoder(), silently ignoring the
allocation failure.
[Severity: High]
Can calling drm_backlight_link() here cause a kernel panic?
This links the backlight during connector initialization, before the connector
is registered to sysfs.
In drivers/gpu/drm/drm_backlight.c:drm_backlight_link():
if (send_uevent)
kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE,
(char **)ep);
If connector->kdev is NULL at this point, sending a uevent could dereference
an invalid pointer.
[Severity: High]
This is a pre-existing issue, but does drm_backlight_link() sleep while
holding a spinlock?
In drivers/gpu/drm/drm_backlight.c:drm_backlight_link():
guard(spinlock)(&drm_backlight_lock);
send_uevent = __drm_backlight_link(b, bd);
if (send_uevent)
kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE,
(char **)ep);
The lock is held across the entire function scope, including the
kobject_uevent_env() call, which can allocate memory with GFP_KERNEL and
sleep. Could this trigger a 'scheduling while atomic' BUG?
> + }
> + }
> +
> if (support_hdcp && IS_REACHABLE(CONFIG_DRM_DISPLAY_HELPER) &&
> IS_ENABLED(CONFIG_DRM_DISPLAY_HDCP_HELPER))
> drm_connector_attach_content_protection_property(connector, true);
[Severity: Medium]
If drm_connector_attach_encoder() fails just after this block, does this leak
a reference to the backlight_device and trigger a WARN_ON?
This error path bails out without unlinking the backlight that was successfully
linked above:
ret = drm_connector_attach_encoder(connector, encoder);
if (ret)
return ERR_PTR(ret);
Because the connector was never registered, the link is never released. When
drm_connector_cleanup() runs, it might hit WARN_ON(b->link) and free the
structure without dropping the reference.
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 4ba3a5deef9a6..6311ca726b45f 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
[ ... ]
> @@ -1586,6 +1586,7 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
>
> #ifdef CONFIG_DRM_PANEL_BRIDGE
> bool drm_bridge_is_panel(const struct drm_bridge *bridge);
> +struct drm_panel *drm_panel_bridge_to_panel(struct drm_bridge *bridge);
[Severity: Low]
Will this cause a build failure when CONFIG_DRM_PANEL_BRIDGE is disabled?
The function declaration lacks a static inline fallback when the config is
disabled, which might cause an implicit function declaration error when called
unconditionally in drm_bridge_connector_init().
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624165751.2014759-1-mario.limonciello@amd.com?part=9
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 10/10] drm/i915/display: use drm backlight
2026-06-24 16:57 [PATCH v6 00/10] Add support for a DRM backlight capability Mario Limonciello
` (8 preceding siblings ...)
2026-06-24 16:57 ` [PATCH v6 09/10] drm/bridge: auto-link panel backlight in bridge connector Mario Limonciello
@ 2026-06-24 16:57 ` Mario Limonciello
2026-06-24 17:19 ` sashiko-bot
2026-06-24 18:12 ` ✗ Fi.CI.BUILD: failure for Add support for a DRM backlight capability Patchwork
10 siblings, 1 reply; 21+ messages in thread
From: Mario Limonciello @ 2026-06-24 16:57 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Mario Limonciello (AMD)
From: "Mario Limonciello (AMD)" <superm1@kernel.org>
Convert Intel display drivers (i915 and xe) to use the new DRM backlight
infrastructure. This allows brightness control via DRM connector
properties.
Allocate DRM backlight for eDP connectors during initialization, link the
backlight device to the connector during registration, and unlink during
unregistration.
Assisted-by: Sonnet:4
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/gpu/drm/i915/display/intel_backlight.c | 4 ++++
drivers/gpu/drm/i915/display/intel_dp.c | 8 ++++++++
2 files changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
index b128896cb1c2d..4543c2a8f3568 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -9,6 +9,7 @@
#include <linux/string_helpers.h>
#include <acpi/video.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_file.h>
#include <drm/drm_print.h>
#include <drm/intel/pci_config.h>
@@ -1004,6 +1005,8 @@ int intel_backlight_device_register(struct intel_connector *connector)
"[CONNECTOR:%d:%s] backlight device %s registered\n",
connector->base.base.id, connector->base.name, name);
+ drm_backlight_link(connector->base.backlight, bd);
+
out:
kfree(name);
@@ -1015,6 +1018,7 @@ void intel_backlight_device_unregister(struct intel_connector *connector)
struct intel_panel *panel = &connector->panel;
if (panel->backlight.device) {
+ drm_backlight_link(connector->base.backlight, NULL);
backlight_device_unregister(panel->backlight.device);
panel->backlight.device = NULL;
}
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 0ce0c09835f6d..cd35cce329a55 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -44,6 +44,7 @@
#include <drm/display/drm_dsc_helper.h>
#include <drm/display/drm_hdmi_helper.h>
#include <drm/drm_atomic_helper.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_crtc.h>
#include <drm/drm_edid.h>
#include <drm/drm_fixed.h>
@@ -7421,6 +7422,13 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
type, &intel_dp->aux.ddc);
drm_connector_helper_add(&connector->base, &intel_dp_connector_helper_funcs);
+ if (type == DRM_MODE_CONNECTOR_eDP) {
+ int r = drm_backlight_alloc(&connector->base);
+
+ if (r)
+ drm_err(display->drm, "Failed to allocate backlight: %d\n", r);
+ }
+
if (!HAS_GMCH(display) && DISPLAY_VER(display) < 12)
connector->base.interlace_allowed = true;
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 10/10] drm/i915/display: use drm backlight
2026-06-24 16:57 ` [PATCH v6 10/10] drm/i915/display: use drm backlight Mario Limonciello
@ 2026-06-24 17:19 ` sashiko-bot
0 siblings, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-06-24 17:19 UTC (permalink / raw)
To: Mario Limonciello; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Calling kobject_uevent_env() (which can sleep) while holding the drm_backlight_lock spinlock in drm_backlight_link().
- [High] Error paths in drm_connector_register() fail to unregister the backlight device, leading to a freed pointer on the global backlight list.
--
commit 91fa46bfbc0b179001176c9331c615c49eadeb08
Author: Mario Limonciello (AMD) <superm1@kernel.org>
drm/i915/display: use drm backlight
Convert Intel display drivers (i915 and xe) to use the new DRM backlight
infrastructure. This allows brightness control via DRM connector
properties.
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index b128896cb1c2d..4543c2a8f3568 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
[ ... ]
> @@ -1004,6 +1005,8 @@ int intel_backlight_device_register(struct intel_connector *connector)
> "[CONNECTOR:%d:%s] backlight device %s registered\n",
> connector->base.base.id, connector->base.name, name);
>
> + drm_backlight_link(connector->base.backlight, bd);
> +
[Severity: High]
This is a pre-existing issue, but does calling drm_backlight_link() here
introduce a sleep-in-atomic bug?
Looking at drm_backlight_link() in drm_backlight.c, it uses a scope-based
spinlock:
guard(spinlock)(&drm_backlight_lock);
send_uevent = __drm_backlight_link(b, bd);
if (send_uevent)
kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE,
(char **)ep);
kobject_uevent_env() allocates memory with GFP_KERNEL and can sleep. Since
the spinlock guard is still active when kobject_uevent_env() is called, this
could trigger lockdep warnings or system crashes.
> out:
> kfree(name);
[ ... ]
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0ce0c09835f6d..cd35cce329a55 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
[ ... ]
> @@ -7421,6 +7422,13 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
> type, &intel_dp->aux.ddc);
> drm_connector_helper_add(&connector->base, &intel_dp_connector_helper_funcs);
>
> + if (type == DRM_MODE_CONNECTOR_eDP) {
> + int r = drm_backlight_alloc(&connector->base);
> +
[Severity: High]
This is a pre-existing issue, but does allocating the backlight here expose a
list corruption issue during connector registration failures?
If a failure occurs during drm_connector_register() (for example, in
drm_sysfs_connector_add()), the error path does not unregister the
backlight device.
Since it was never fully registered, drm_connector_cleanup() will eventually
skip calling drm_connector_unregister() but will directly call
drm_backlight_free(), which does:
kfree(b);
connector->backlight = NULL;
Because the backlight was never removed from the global drm_backlight_list,
freeing it here leaves a dangling pointer on the global list, which could
cause a use-after-free panic during later list traversals.
> + if (r)
> + drm_err(display->drm, "Failed to allocate backlight: %d\n", r);
> + }
> +
> if (!HAS_GMCH(display) && DISPLAY_VER(display) < 12)
> connector->base.interlace_allowed = true;
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624165751.2014759-1-mario.limonciello@amd.com?part=10
^ permalink raw reply [flat|nested] 21+ messages in thread
* ✗ Fi.CI.BUILD: failure for Add support for a DRM backlight capability
2026-06-24 16:57 [PATCH v6 00/10] Add support for a DRM backlight capability Mario Limonciello
` (9 preceding siblings ...)
2026-06-24 16:57 ` [PATCH v6 10/10] drm/i915/display: use drm backlight Mario Limonciello
@ 2026-06-24 18:12 ` Patchwork
10 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2026-06-24 18:12 UTC (permalink / raw)
To: Mario Limonciello; +Cc: intel-gfx
== Series Details ==
Series: Add support for a DRM backlight capability
URL : https://patchwork.freedesktop.org/series/169109/
State : failure
== Summary ==
Error: patch https://patchwork.freedesktop.org/api/1.0/series/169109/revisions/1/mbox/ not applied
Applying: Revert "backlight: Remove notifier"
Applying: backlight: add kernel-internal backlight API
Applying: drm: link connectors to backlight devices
Applying: DRM: Add support for client indicating support for luminance
error: sha1 information is lacking or useless (drivers/gpu/drm/drm_connector.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0004 DRM: Add support for client indicating support for luminance
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced
^ permalink raw reply [flat|nested] 21+ messages in thread