* [PATCH v5 01/11] Revert "backlight: Remove notifier"
2026-05-31 11:48 [PATCH v5 00/11] Add support for a DRM backlight capability Mario Limonciello (AMD)
@ 2026-05-31 11:48 ` Mario Limonciello (AMD)
2026-06-04 4:54 ` Claude review: " Claude Code Review Bot
2026-05-31 11:48 ` [PATCH v5 02/11] backlight: add kernel-internal backlight API Mario Limonciello (AMD)
` (10 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Mario Limonciello (AMD) @ 2026-05-31 11:48 UTC (permalink / raw)
To: dri-devel; +Cc: amd-gfx, Mario Limonciello (AMD)
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>
---
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 a22d0bbb6e63..ff2c2084c73a 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 f29a9ef1052e..d905173c7f73 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.54.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: Revert "backlight: Remove notifier"
2026-05-31 11:48 ` [PATCH v5 01/11] Revert "backlight: Remove notifier" Mario Limonciello (AMD)
@ 2026-06-04 4:54 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 4:54 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Status: Looks fine**
Straightforward revert restoring the blocking notifier infrastructure. Minor nit: the docstring for `backlight_unregister_notifier` still says "Register a notifier" (copy-paste from register):
```c
+/**
+ * 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.
```
Should say "Unregister a notifier..."
Also, `BLOCKING_INIT_NOTIFIER_HEAD` is called in `backlight_class_init()` but `backlight_notifier` is file-scope static — it could use a static initializer (`BLOCKING_NOTIFIER_INIT`) instead, avoiding the runtime init.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 02/11] backlight: add kernel-internal backlight API
2026-05-31 11:48 [PATCH v5 00/11] Add support for a DRM backlight capability Mario Limonciello (AMD)
2026-05-31 11:48 ` [PATCH v5 01/11] Revert "backlight: Remove notifier" Mario Limonciello (AMD)
@ 2026-05-31 11:48 ` Mario Limonciello (AMD)
2026-06-04 4:54 ` Claude review: " Claude Code Review Bot
2026-05-31 11:49 ` [PATCH v5 03/11] drm: link connectors to backlight devices Mario Limonciello (AMD)
` (9 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Mario Limonciello (AMD) @ 2026-05-31 11:48 UTC (permalink / raw)
To: dri-devel
Cc: amd-gfx, Mario Limonciello (AMD), David Herrmann, Marta Lofstedt
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 ff2c2084c73a..cd1a161ae7bc 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 d905173c7f73..204eea9256fd 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.54.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: backlight: add kernel-internal backlight API
2026-05-31 11:48 ` [PATCH v5 02/11] backlight: add kernel-internal backlight API Mario Limonciello (AMD)
@ 2026-06-04 4:54 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 4:54 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Concern: Double notification in backlight_set_brightness**
`backlight_set_brightness` calls `backlight_update_status(bd)` and then `backlight_generate_event()`. But in patch 3, `backlight_update_status` is modified to also call `backlight_notify_brightness()`. This means a single `backlight_set_brightness` call will:
1. Call `backlight_update_status` → which calls `backlight_notify_brightness` (notifier chain)
2. Then call `backlight_generate_event` (uevent)
3. The notifier from step 1 fires `__drm_backlight_real_changed` which is supposed to update the DRM property
The `backlight_generate_event` call here is arguably correct (it sends a uevent), but the interaction with the notifier added in patch 3 creates a loop risk: DRM sets brightness → `backlight_set_brightness` → `backlight_update_status` → `backlight_notify_brightness` → DRM notifier → `__drm_backlight_real_changed`. The `__drm_backlight_real_changed` function updates `connector->state->luminance` under spinlock, which should be safe, but it's worth verifying there's no re-entrant path.
**Concern: ops_lock held across backlight_generate_event**
```c
+ 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);
```
`backlight_generate_event` calls `kobject_uevent_env` which can sleep and allocate. It's called with `ops_lock` held (via `guard`). While `ops_lock` is a mutex so sleeping is OK, holding it longer than necessary across a uevent broadcast isn't ideal. But it's not a correctness bug.
**Minor**: `backlight_set_brightness` sends a uevent even when `bd->ops` is NULL (rc stays 0 in that case). Should it return early if ops is NULL?
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 03/11] drm: link connectors to backlight devices
2026-05-31 11:48 [PATCH v5 00/11] Add support for a DRM backlight capability Mario Limonciello (AMD)
2026-05-31 11:48 ` [PATCH v5 01/11] Revert "backlight: Remove notifier" Mario Limonciello (AMD)
2026-05-31 11:48 ` [PATCH v5 02/11] backlight: add kernel-internal backlight API Mario Limonciello (AMD)
@ 2026-05-31 11:49 ` Mario Limonciello (AMD)
2026-06-04 4:54 ` Claude review: " Claude Code Review Bot
2026-05-31 11:49 ` [PATCH v5 04/11] DRM: Add support for client indicating support for luminance Mario Limonciello (AMD)
` (8 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Mario Limonciello (AMD) @ 2026-05-31 11:49 UTC (permalink / raw)
To: dri-devel; +Cc: amd-gfx, Mario Limonciello (AMD), Dmitry Baryshkov
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>
---
v5:
* Add tag
* Changed the property range from 0-max to 1-max when a backlight
device is linked
* Move prototype to this patch
* Drop worker
---
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 8f5a8d3012e4..8c00f534d50b 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 e97faabcd783..bf980a2ac1e6 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 000000000000..b1ec470be86c
--- /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 47dc53c4a738..09bb790782f1 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;
@@ -3257,10 +3263,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 eb6cd5d7dac1..387ed0a83253 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>
@@ -1235,6 +1236,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();
@@ -1258,6 +1260,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 66f7dc37b597..25c5d2969462 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 2d943a610b88..e6f0027d70cb 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 ef4e923a8728..8609e1660f21 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 cd1a161ae7bc..13954c2220b7 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 000000000000..e0e09e38f7c0
--- /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 f83f28cae207..10daf088b8f1 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 687c0ee163d2..b340dc93f814 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 204eea9256fd..26a7281d179c 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.54.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm: link connectors to backlight devices
2026-05-31 11:49 ` [PATCH v5 03/11] drm: link connectors to backlight devices Mario Limonciello (AMD)
@ 2026-06-04 4:54 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 4:54 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the core patch and has the most issues.
**Bug: Spinlock used in sleeping context**
`drm_backlight_lock` is defined as:
```c
+static DEFINE_SPINLOCK(drm_backlight_lock);
```
But `__drm_backlight_link` is called under this spinlock, and it calls:
- `backlight_device_unref(b->link)` → `put_device()` which can trigger device cleanup (sleeping)
- `backlight_device_ref(b->link)` → `get_device()` (this is fine)
And in the notifier callback `drm_backlight_notify`, the spinlock is held while iterating and calling `__drm_backlight_link(b, NULL)`, which again calls `put_device`.
This should be a mutex, not a spinlock, or the ref/unref should be deferred outside the lock.
**Bug: Global property range shared across connectors**
```c
+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;
+ ...
+ if (prop->values[1] != max) {
+ prop->values[0] = max ? 1 : 0;
+ prop->values[1] = max;
+ }
```
The `luminance_property` is a single property object shared by all connectors on the device. Modifying `prop->values[0]` and `prop->values[1]` affects the range for ALL connectors, not just the one being linked. If two eDP panels have different `max_brightness`, the last one linked wins. This is a design problem — each connector should have its own property instance, or a per-connector range mechanism is needed.
**Bug: drm_backlight_get_device returns pointer without refcount**
```c
+struct backlight_device *drm_backlight_get_device(struct drm_backlight *b)
+{
+ if (!b)
+ return NULL;
+
+ guard(spinlock)(&drm_backlight_lock);
+ return b->link;
+}
```
The pointer is returned after the spinlock is released. The caller in `drm_sysfs_connector_add_late` uses it to create a sysfs link:
```c
+ struct backlight_device *bd = drm_backlight_get_device(connector->backlight);
+ if (bd) {
+ int ret = sysfs_create_link(...)
```
Between `drm_backlight_get_device` returning and `sysfs_create_link`, the link could be changed and `bd` could be freed. The function should take a reference, or the caller should hold a lock.
**Issue: `__drm_backlight_real_changed` is a no-op in this patch**
```c
+static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
+{
+ unsigned int max, set;
+ ...
+ set = v;
+ if (set >= max)
+ set = max;
+}
```
`set` is computed but never used. The function does nothing except check bounds. It's only completed in patch 4 where `connector->state->luminance = set` is added. This makes the intermediate state confusing and means patch 3 alone doesn't actually do anything useful with brightness values.
**Issue: select BACKLIGHT_CLASS_DEVICE unconditionally**
```c
+ select BACKLIGHT_CLASS_DEVICE
```
This forces all DRM builds to pull in backlight support. This may be undesirable for embedded/server builds that never use backlight. Consider making it conditional or using `depends on`.
**Issue: WARN_ON outside spinlock in drm_backlight_unregister**
```c
+void drm_backlight_unregister(struct drm_backlight *b)
+{
+ ...
+ WARN_ON(!__drm_backlight_is_registered(b));
+
+ scoped_guard(spinlock, &drm_backlight_lock) {
```
`__drm_backlight_is_registered` asserts `lockdep_assert_held(&drm_backlight_lock)`, but it's called outside the spinlock guard. This will trigger a lockdep warning. Similarly in `drm_backlight_register`:
```c
+ WARN_ON(__drm_backlight_is_registered(b));
+ guard(spinlock)(&drm_backlight_lock);
```
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 04/11] DRM: Add support for client indicating support for luminance
2026-05-31 11:48 [PATCH v5 00/11] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (2 preceding siblings ...)
2026-05-31 11:49 ` [PATCH v5 03/11] drm: link connectors to backlight devices Mario Limonciello (AMD)
@ 2026-05-31 11:49 ` Mario Limonciello (AMD)
2026-06-04 4:54 ` Claude review: " Claude Code Review Bot
2026-05-31 11:49 ` [PATCH v5 05/11] drm/amd/display: Pass up errors reading actual brightness Mario Limonciello (AMD)
` (7 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Mario Limonciello (AMD) @ 2026-05-31 11:49 UTC (permalink / raw)
To: dri-devel; +Cc: amd-gfx, Mario Limonciello (AMD), Dmitry Baryshkov
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>
---
v5:
* Add tag
* Added special validation handling for the luminance property
* Allows value 0 to be set even when the range starts at 1
* This enables DPMS to set luminance to 0 to turn off the display
* Add documentation
* Fix a compile error
* Drop the driver capability - no longer needed! (yay)
v4:
* Update unit for luminance
* Disable backlight of other connectors on same CRTC
* handle CRTC disconnect
* Make DRM_CLIENT_CAP_LUMINANCE actually inhibit legacy sysfs writes
with -EBUSY
f-luminance
---
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 34ed8e5d93d7..bf85e98f1dff 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>
@@ -1225,6 +1226,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);
@@ -1737,6 +1740,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 5bd5bf6661df..64cd0830beb7 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_state *state,
struct drm_connector *connector,
int mode)
@@ -1126,9 +1163,29 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *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 b1ec470be86c..2cddf209d508 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 09bb790782f1..224661587241 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 ec820686b302..4d2520de7614 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 ff193155129e..fdae36b13300 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 f38f2c5437e6..4475896c963b 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 13954c2220b7..40cfc2296445 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 e0e09e38f7c0..2af48be3aa37 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 10daf088b8f1..dcb7dd0bdf44 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 6ee70ad65e1f..0bb1e53f36be 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 26a7281d179c..b03737ee8dac 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 27cc159c1d27..1b17fd2d3b2c 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -921,6 +921,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.54.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: DRM: Add support for client indicating support for luminance
2026-05-31 11:49 ` [PATCH v5 04/11] DRM: Add support for client indicating support for luminance Mario Limonciello (AMD)
@ 2026-06-04 4:54 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 4:54 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Issue: Modifying connector->state without holding atomic lock**
```c
+ if (connector->state)
+ connector->state->luminance = set;
```
This is in `__drm_backlight_real_changed`, called under the spinlock. Writing to `connector->state->luminance` without holding the atomic modeset lock is a race with any concurrent atomic commit that might be swapping connector states.
**Concern: DPMS handling in drm_atomic_connector_commit_dpms**
The patch modifies this function significantly, but the base code has changed — upstream uses `struct drm_atomic_commit *state` while the patch shows `struct drm_atomic_state *state`. This indicates a base mismatch that will need rebasing.
The DPMS backlight coordination logic is complex with both legacy and atomic paths:
```c
+ crtc = connector->state ? connector->state->crtc : NULL;
+
+ if (old_mode != DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_OFF) {
+ if (crtc)
+ drm_atomic_crtc_set_backlight(crtc, false);
```
This null-checks `connector->state` (good, fixing a potential NULL deref in the original), but the original code unconditionally did `crtc = connector->state->crtc`. So this is also a bugfix, but it should be a separate patch.
**Issue: `drm_backlight_set_luminance` WARN_ON on failure then fallback**
```c
+ int rc = backlight_set_brightness(bd, set, BACKLIGHT_UPDATE_DRM);
+ WARN_ON(rc);
+ if (rc)
+ backlight_set_brightness(bd, max, BACKLIGHT_UPDATE_DRM);
```
`WARN_ON` in normal operation is too aggressive. If the hardware is busy or the panel is off, the brightness set can legitimately fail. Using `WARN_ON` will spam dmesg and could be flagged by CI. The fallback to max brightness on failure is also questionable — if setting `set` failed, why would setting `max` succeed? And setting max on failure seems wrong — the user asked for a specific brightness.
**Issue: drm_takeover atomic vs spinlock**
`drm_takeover` is an `atomic_t` on `backlight_device`, modified under `drm_backlight_lock` spinlock:
```c
+ if (b->link)
+ atomic_inc(&b->link->drm_takeover);
```
But it's read without the spinlock in `brightness_store`:
```c
+ if (atomic_read(&bd->drm_takeover) > 0)
+ return -EBUSY;
```
This is fine for the intended purpose (advisory check, TOCTOU is acceptable here), but the use of both a spinlock and atomics is redundant — if the atomic is sufficient for the read side, the spinlock protection on the write side is unnecessary overhead. Pick one synchronization mechanism.
**Leftover text in changelog**: Line `f-luminance` appears to be a stray fragment in the commit message changelog area.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 05/11] drm/amd/display: Pass up errors reading actual brightness
2026-05-31 11:48 [PATCH v5 00/11] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (3 preceding siblings ...)
2026-05-31 11:49 ` [PATCH v5 04/11] DRM: Add support for client indicating support for luminance Mario Limonciello (AMD)
@ 2026-05-31 11:49 ` Mario Limonciello (AMD)
2026-06-04 4:54 ` Claude review: " Claude Code Review Bot
2026-05-31 11:49 ` [PATCH v5 06/11] drm/amd/display: Allow backlight registration to fail Mario Limonciello (AMD)
` (6 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Mario Limonciello (AMD) @ 2026-05-31 11:49 UTC (permalink / raw)
To: dri-devel; +Cc: amd-gfx, Mario Limonciello (AMD)
[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>
---
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 d99617c9bbb9..de9b29ad6cab 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5427,7 +5427,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;
@@ -5441,14 +5441,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);
}
@@ -5456,7 +5456,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])
@@ -5464,7 +5464,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.54.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm/amd/display: Pass up errors reading actual brightness
2026-05-31 11:49 ` [PATCH v5 05/11] drm/amd/display: Pass up errors reading actual brightness Mario Limonciello (AMD)
@ 2026-06-04 4:54 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 4:54 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Status: Looks fine, minor concern**
Clean refactor of error handling. The change from returning cached `dm->brightness[bl_idx]` to returning `-EINVAL` on failure is correct, and the caller in `amdgpu_dm_backlight_get_brightness` now handles the error by falling back to the cached value.
However, note that patch 9 later removes the cached `dm->brightness` array entirely. After patch 9, `amdgpu_dm_backlight_get_level` returns `-EINVAL` on failure and `amdgpu_dm_backlight_get_brightness` will propagate that negative value directly to the backlight core as the `get_brightness` callback return. The backlight core interprets negative returns as errors, so this should be fine, but it means the fallback added here is immediately removed two patches later. Consider squashing or reordering.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 06/11] drm/amd/display: Allow backlight registration to fail
2026-05-31 11:48 [PATCH v5 00/11] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (4 preceding siblings ...)
2026-05-31 11:49 ` [PATCH v5 05/11] drm/amd/display: Pass up errors reading actual brightness Mario Limonciello (AMD)
@ 2026-05-31 11:49 ` Mario Limonciello (AMD)
2026-06-04 4:54 ` Claude review: " Claude Code Review Bot
2026-05-31 11:49 ` [PATCH v5 07/11] drm/amd/display: Move backlight tracing out of the dc lock Mario Limonciello (AMD)
` (5 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Mario Limonciello (AMD) @ 2026-05-31 11:49 UTC (permalink / raw)
To: dri-devel; +Cc: amd-gfx, Mario Limonciello (AMD)
[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>
---
.../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 de9b29ad6cab..430b7d3d8930 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5478,7 +5478,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;
@@ -5489,15 +5489,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];
@@ -5531,22 +5532,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,
@@ -8078,7 +8083,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.54.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm/amd/display: Allow backlight registration to fail
2026-05-31 11:49 ` [PATCH v5 06/11] drm/amd/display: Allow backlight registration to fail Mario Limonciello (AMD)
@ 2026-06-04 4:54 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 4:54 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Concern: Failing connector late_register on backlight failure**
```c
- amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
+ r = amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
+ if (r)
+ return r;
```
If backlight registration fails, the entire connector late_register fails. This will prevent the connector from being usable at all, even though the display itself works fine — only backlight control is broken. This seems too aggressive. A warning + continue would be more appropriate for a non-critical feature like backlight.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 07/11] drm/amd/display: Move backlight tracing out of the dc lock
2026-05-31 11:48 [PATCH v5 00/11] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (5 preceding siblings ...)
2026-05-31 11:49 ` [PATCH v5 06/11] drm/amd/display: Allow backlight registration to fail Mario Limonciello (AMD)
@ 2026-05-31 11:49 ` Mario Limonciello (AMD)
2026-06-04 4:54 ` Claude review: " Claude Code Review Bot
2026-05-31 11:49 ` [PATCH v5 08/11] drm/amd/display: use drm backlight Mario Limonciello (AMD)
` (4 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Mario Limonciello (AMD) @ 2026-05-31 11:49 UTC (permalink / raw)
To: dri-devel; +Cc: amd-gfx, Mario Limonciello (AMD)
The backlight tracing doesn't need to be done with DC lock held as
it's using information that is fed into the function.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 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 430b7d3d8930..f6610902eee4 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5371,13 +5371,6 @@ static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
if (caps->brightness_mask)
brightness |= caps->brightness_mask;
- /* Change brightness based on AUX property */
- mutex_lock(&dm->dc_lock);
- if (dm->dc->caps.ips_support && dm->dc->ctx->dmub_srv->idle_allowed) {
- dc_allow_idle_optimizations(dm->dc, false);
- reallow_idle = true;
- }
-
if (trace_amdgpu_dm_brightness_enabled()) {
trace_amdgpu_dm_brightness(__builtin_return_address(0),
user_brightness,
@@ -5386,6 +5379,13 @@ static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
power_supply_is_system_supplied() > 0);
}
+ /* Change brightness based on AUX property */
+ mutex_lock(&dm->dc_lock);
+ if (dm->dc->caps.ips_support && dm->dc->ctx->dmub_srv->idle_allowed) {
+ dc_allow_idle_optimizations(dm->dc, false);
+ reallow_idle = true;
+ }
+
if (caps->aux_support) {
rc = dc_link_set_backlight_level_nits(link, true, brightness,
AUX_BL_DEFAULT_TRANSITION_TIME_MS);
--
2.54.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 08/11] drm/amd/display: use drm backlight
2026-05-31 11:48 [PATCH v5 00/11] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (6 preceding siblings ...)
2026-05-31 11:49 ` [PATCH v5 07/11] drm/amd/display: Move backlight tracing out of the dc lock Mario Limonciello (AMD)
@ 2026-05-31 11:49 ` Mario Limonciello (AMD)
2026-06-04 4:54 ` Claude review: " Claude Code Review Bot
2026-05-31 11:49 ` [PATCH v5 09/11] drm/amd/display: Drop brightness caching in amdgpu_dm Mario Limonciello (AMD)
` (3 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Mario Limonciello (AMD) @ 2026-05-31 11:49 UTC (permalink / raw)
To: dri-devel; +Cc: amd-gfx, Mario Limonciello (AMD)
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>
---
.../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 f6610902eee4..55c3838f7ec8 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>
@@ -5549,6 +5550,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;
@@ -7993,6 +8003,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;
}
@@ -8005,6 +8016,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);
@@ -9254,6 +9269,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.54.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm/amd/display: use drm backlight
2026-05-31 11:49 ` [PATCH v5 08/11] drm/amd/display: use drm backlight Mario Limonciello (AMD)
@ 2026-06-04 4:54 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 4:54 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Status: Mostly good**
Links the backlight device to the DRM connector. The alloc happens in `init_helper` and the link in `register_backlight_device`, which is the right ordering.
**Minor nit**: Missing blank line before the `if (connector_type == DRM_MODE_CONNECTOR_HDMIA)`:
```c
+ if (r)
+ drm_err(dm->ddev, "Failed to allocate backlight: %d\n", r);
+ }
if (connector_type == DRM_MODE_CONNECTOR_HDMIA) {
```
Should have a blank line between the closing brace and the next if statement.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 09/11] drm/amd/display: Drop brightness caching in amdgpu_dm
2026-05-31 11:48 [PATCH v5 00/11] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (7 preceding siblings ...)
2026-05-31 11:49 ` [PATCH v5 08/11] drm/amd/display: use drm backlight Mario Limonciello (AMD)
@ 2026-05-31 11:49 ` Mario Limonciello (AMD)
2026-06-04 4:54 ` Claude review: " Claude Code Review Bot
2026-05-31 11:49 ` [PATCH v5 10/11] drm/bridge: auto-link panel backlight in bridge connector Mario Limonciello (AMD)
` (2 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Mario Limonciello (AMD) @ 2026-05-31 11:49 UTC (permalink / raw)
To: dri-devel; +Cc: amd-gfx, Mario Limonciello (AMD)
[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 | 64 ++-----------------
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 13 ----
2 files changed, 5 insertions(+), 72 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 55c3838f7ec8..62161d7f7994 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3732,12 +3732,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. */
@@ -5350,10 +5344,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;
}
}
@@ -5361,11 +5353,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 */
@@ -5407,9 +5398,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)
@@ -5457,7 +5445,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])
@@ -5466,11 +5454,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 = {
@@ -5488,8 +5472,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)
@@ -5515,8 +5497,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;
@@ -5530,7 +5510,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]);
@@ -5539,18 +5518,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
@@ -10665,7 +10632,6 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_state *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) {
@@ -10785,7 +10751,6 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_state *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",
@@ -10838,18 +10803,6 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_state *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,
@@ -11355,13 +11308,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *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 1ab066859d9e..952bfea0d8dc 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -648,19 +648,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.54.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm/amd/display: Drop brightness caching in amdgpu_dm
2026-05-31 11:49 ` [PATCH v5 09/11] drm/amd/display: Drop brightness caching in amdgpu_dm Mario Limonciello (AMD)
@ 2026-06-04 4:54 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 4:54 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Concern: Get_brightness now returns errors directly**
After this patch, `amdgpu_dm_backlight_get_brightness` returns `amdgpu_dm_backlight_get_level(dm, i)` which can return `-EINVAL`. The backlight core's `get_brightness` callback should handle this, but verify the backlight sysfs `actual_brightness` attribute handler deals with negative returns correctly.
**Concern: Resume path no longer restores backlight**
The patch removes:
```c
- /* 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]);
- }
```
The comment says "DRM core will restore it next time it's on", but this depends on the DRM core atomic commit path restoring `connector->state->luminance` during resume. Is this guaranteed? The `drm_atomic_helper_commit_encoder_bridge_enable` path does restore it, but only if the full modeset path is taken during resume. If the DC layer does a shortcut resume (the `if (dc_enable_dmub_notifications(adev->dm.dc))` early return path at the top of `dm_resume`), no modeset happens and backlight won't be restored.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 10/11] drm/bridge: auto-link panel backlight in bridge connector
2026-05-31 11:48 [PATCH v5 00/11] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (8 preceding siblings ...)
2026-05-31 11:49 ` [PATCH v5 09/11] drm/amd/display: Drop brightness caching in amdgpu_dm Mario Limonciello (AMD)
@ 2026-05-31 11:49 ` Mario Limonciello (AMD)
2026-06-04 4:54 ` Claude review: " Claude Code Review Bot
2026-05-31 11:49 ` [PATCH v5 11/11] drm/i915/display: use drm backlight Mario Limonciello (AMD)
2026-06-04 4:54 ` Claude review: Add support for a DRM backlight capability Claude Code Review Bot
11 siblings, 1 reply; 24+ messages in thread
From: Mario Limonciello (AMD) @ 2026-05-31 11:49 UTC (permalink / raw)
To: dri-devel; +Cc: amd-gfx, Mario Limonciello (AMD), Dmitry Baryshkov
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>
---
v5:
* Add tag
---
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 184a8b7049a7..1dbd3ba75903 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 39cc18f78eda..a1d54f7bac6a 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>
@@ -1048,9 +1050,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 a8d67bd9ee50..6cc2a22b2dec 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1558,6 +1558,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.54.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm/bridge: auto-link panel backlight in bridge connector
2026-05-31 11:49 ` [PATCH v5 10/11] drm/bridge: auto-link panel backlight in bridge connector Mario Limonciello (AMD)
@ 2026-06-04 4:54 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 4:54 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Status: Looks good**
Clean integration. The `drm_panel_bridge_to_panel` helper is properly guarded. Linking happens during `drm_bridge_connector_init` which is the right time.
One question: if `drm_backlight_alloc` succeeds but `panel->backlight` is NULL (panel has no backlight), the connector gets a backlight property with 0-0 range permanently. Is that the desired behavior? It means every panel bridge connector will have a LUMINANCE property even if no backlight exists.
Wait, looking more carefully — the alloc only happens if `panel && panel->backlight`:
```c
+ if (panel && panel->backlight) {
+ ret = drm_backlight_alloc(connector);
```
This is correct.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 11/11] drm/i915/display: use drm backlight
2026-05-31 11:48 [PATCH v5 00/11] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (9 preceding siblings ...)
2026-05-31 11:49 ` [PATCH v5 10/11] drm/bridge: auto-link panel backlight in bridge connector Mario Limonciello (AMD)
@ 2026-05-31 11:49 ` Mario Limonciello (AMD)
2026-06-04 4:54 ` Claude review: " Claude Code Review Bot
2026-06-04 4:54 ` Claude review: Add support for a DRM backlight capability Claude Code Review Bot
11 siblings, 1 reply; 24+ messages in thread
From: Mario Limonciello (AMD) @ 2026-05-31 11:49 UTC (permalink / raw)
To: dri-devel; +Cc: amd-gfx, Mario Limonciello (AMD)
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>
---
v5:
* New patch
---
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 34e95f05936e..99ae39dfb7bb 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>
@@ -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 6ef2a0043cda..109f85dc6b89 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>
@@ -7222,6 +7223,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.54.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm/i915/display: use drm backlight
2026-05-31 11:49 ` [PATCH v5 11/11] drm/i915/display: use drm backlight Mario Limonciello (AMD)
@ 2026-06-04 4:54 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 4:54 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Status: Mostly good**
Clean integration following the same pattern as amdgpu.
**Concern**: The `Assisted-by: Sonnet:4` tag in the commit message should probably be `Assisted-by: Claude Sonnet 4 <noreply@anthropic.com>` or similar proper format, or dropped if the Intel maintainers don't want AI-assisted tags.
**Minor**: Same missing blank line issue as patch 8 between the backlight alloc block and the next if statement in `intel_dp_init_connector`.
---
**Summary of critical issues to address before merge:**
1. Spinlock vs sleeping calls in `drm_backlight.c` — needs to be a mutex or refcounting deferred
2. Global property range clobbering across connectors — needs per-connector property instances
3. `drm_backlight_get_device` returns unprotected pointer — needs refcount or different API
4. lockdep assertions called outside lock in register/unregister
5. `WARN_ON` on brightness set failure is too aggressive for production
6. Resume backlight restore path may be broken after cache removal
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* Claude review: Add support for a DRM backlight capability
2026-05-31 11:48 [PATCH v5 00/11] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (10 preceding siblings ...)
2026-05-31 11:49 ` [PATCH v5 11/11] drm/i915/display: use drm backlight Mario Limonciello (AMD)
@ 2026-06-04 4:54 ` Claude Code Review Bot
11 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 4:54 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: Add support for a DRM backlight capability
Author: "Mario Limonciello (AMD)" <superm1@kernel.org>
Patches: 12
Reviewed: 2026-06-04T14:54:30.211806
---
This is v5 of a significant series by Mario Limonciello adding DRM connector-level backlight control via a new `LUMINANCE` property, replacing the legacy sysfs-only interface. The design is solid: connectors get an atomic property, a new `DRM_CLIENT_CAP_LUMINANCE` client capability inhibits legacy sysfs writes when a compositor takes over, and the backlight notifier is restored to allow cross-subsystem coordination.
The series has working compositor support in KWin, Mutter, and wlroots, which is a strong signal. The overall architecture of a `drm_backlight` intermediary that links DRM connectors to `backlight_device` instances is clean. Driver integration for amdgpu, i915/xe, and bridge connectors is provided.
However, there are several issues ranging from potential locking bugs to incomplete implementations and UAPI concerns that should be addressed before merge.
**Key concerns:**
1. **Spinlock + sleeping calls**: `drm_backlight_lock` is a spinlock, but several paths under it call functions that may sleep (kobject_uevent_env, backlight notifier calls).
2. **Per-device vs global property range**: The luminance property is global to the DRM device but the range is updated per-connector link, meaning multi-eDP setups will clobber each other's ranges.
3. **Incomplete inhibit implementation** in patch 3 (stubs), then filled in patch 4 — the intermediate state is confusing for bisection.
4. **Notification loop risk**: `backlight_update_status` now calls `backlight_notify_brightness` which notifies DRM, which may call back into backlight.
5. **drm_atomic_connector_commit_dpms signature change** — the upstream function takes `struct drm_atomic_commit *state`, but the patch diff shows `struct drm_atomic_state *state`, suggesting this may be based on an older tree.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread