* [PATCH v7 01/12] Revert "backlight: Remove notifier"
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
@ 2026-08-31 21:05 ` Mario Limonciello
2026-08-31 22:02 ` sashiko-bot
2026-08-31 21:05 ` [PATCH v7 02/12] backlight: add kernel-internal backlight API Mario Limonciello
` (11 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Mario Limonciello @ 2026-08-31 21:05 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede, Mario Limonciello, Mario Limonciello
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 | 22 +++++++++++++++
2 files changed, 64 insertions(+)
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index fc9bb303e8c5f..4401f6294ccc8 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
+ *
+ * Unregister a notifier previously registered with
+ * backlight_register_notifier().
+ *
+ * RETURNS:
+ *
+ * 0 on success, otherwise a negative error code
+ */
+int backlight_unregister_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_unregister(&backlight_notifier, nb);
+}
+EXPORT_SYMBOL(backlight_unregister_notifier);
+
/**
* devm_backlight_device_register - register a new backlight device
* @dev: the device to register
@@ -674,6 +715,7 @@ static int __init backlight_class_init(void)
INIT_LIST_HEAD(&backlight_dev_list);
mutex_init(&backlight_dev_list_mutex);
+ BLOCKING_INIT_NOTIFIER_HEAD(&backlight_notifier);
return 0;
}
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index f29a9ef1052e7..015183d129f96 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -13,6 +13,8 @@
#include <linux/mutex.h>
#include <linux/types.h>
+struct notifier_block;
+
/**
* enum backlight_update_reason - what method was used to update backlight
*
@@ -64,6 +66,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 +408,8 @@ void devm_backlight_device_unregister(struct device *dev,
struct backlight_device *bd);
void backlight_force_update(struct backlight_device *bd,
enum backlight_update_reason reason);
+int backlight_register_notifier(struct notifier_block *nb);
+int backlight_unregister_notifier(struct notifier_block *nb);
struct backlight_device *backlight_device_get_by_name(const char *name);
struct backlight_device *backlight_device_get_by_type(enum backlight_type type);
int backlight_device_set_brightness(struct backlight_device *bd,
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 01/12] Revert "backlight: Remove notifier"
2026-08-31 21:05 ` [PATCH v7 01/12] Revert "backlight: Remove notifier" Mario Limonciello
@ 2026-08-31 22:02 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-08-31 22:02 UTC (permalink / raw)
To: Mario Limonciello; +Cc: intel-gfx, intel-xe, dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] `backlight_device_get_by_type()` does not take a reference on the returned device pointer, leading to Use-After-Free vulnerabilities in callers.
--
commit 910d8ccf8b36f6154658d6c354c95ebb55830e1d
Author: Mario Limonciello <mario.limonciello@amd.com>
Revert "backlight: Remove notifier"
This reverts commit 5461f3fd74a89757f95f351eb0bc26aafc2a2e91. The
backlight notifier support is needed in order to add backlight control
support into DRM connectors.
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index f29a9ef1052e7..015183d129f96 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
[ ... ]
> @@ -388,6 +408,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);
[Severity: High]
This is a pre-existing issue, but does backlight_device_get_by_type() need to
increment the reference count of the returned device pointer?
Currently, backlight_device_get_by_type() in
drivers/video/backlight/backlight.c returns a pointer to the backlight
device without taking a reference:
drivers/video/backlight/backlight.c:backlight_device_get_by_type() {
...
mutex_unlock(&backlight_dev_list_mutex);
return found ? bd : NULL;
}
Callers like int3406_thermal_probe() in
drivers/thermal/intel/int340x_thermal/int3406_thermal.c call this and
permanently cache the returned pointer:
bd = backlight_device_get_by_type(BACKLIGHT_RAW);
if (!bd)
return -ENODEV;
d->raw_bd = bd;
Because the reference count is not incremented (for example, via
get_device(&bd->dev)), a concurrent unbind of the raw backlight driver
will unregister and free the backlight device.
The thermal driver remains unaware, and subsequent reads or writes to the
thermal sysfs attributes (such as
/sys/class/thermal/cooling_deviceX/cur_state) will call
int3406_thermal_get_cur_state(), which dereferences the freed pointer
(d->raw_bd->props.brightness).
Could we add get_device() to backlight_device_get_by_type() and update
callers to drop the reference when done?
> int backlight_device_set_brightness(struct backlight_device *bd,
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260831210531.2334221-1-mario.limonciello@amd.com?part=1
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v7 02/12] backlight: add kernel-internal backlight API
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
2026-08-31 21:05 ` [PATCH v7 01/12] Revert "backlight: Remove notifier" Mario Limonciello
@ 2026-08-31 21:05 ` Mario Limonciello
2026-08-31 22:11 ` sashiko-bot
2026-08-31 21:05 ` [PATCH v7 03/12] drm/property: add a per-connector luminance flag Mario Limonciello
` (10 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Mario Limonciello @ 2026-08-31 21:05 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede, Mario Limonciello, David Herrmann, Marta Lofstedt,
Mario Limonciello
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 a few kernel-internal backlight helpers so we can modify
backlights from within DRM, a brightness-changed notification, and a
per-device takeover count so that legacy sysfs writes can be inhibited
(-EBUSY) while a luminance-aware DRM client is in control.
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()
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/video/backlight/backlight.c | 57 +++++++++++++++++++++++++++++
include/linux/backlight.h | 41 +++++++++++++++++++++
2 files changed, 98 insertions(+)
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 4401f6294ccc8..5735226c8b78d 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;
@@ -214,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;
@@ -514,6 +524,39 @@ 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;
+
+ dev_dbg(&bd->dev, "set brightness to %u\n", value);
+ 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
@@ -548,6 +591,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/linux/backlight.h b/include/linux/backlight.h
index 015183d129f96..18d9145198b5f 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -31,6 +31,12 @@ enum backlight_update_reason {
* @BACKLIGHT_UPDATE_SYSFS: The backlight was updated using sysfs.
*/
BACKLIGHT_UPDATE_SYSFS,
+
+ /**
+ * @BACKLIGHT_UPDATE_DRM: The backlight was updated from DRM, i.e. through
+ * a connector LUMINANCE property rather than the legacy sysfs interface.
+ */
+ BACKLIGHT_UPDATE_DRM,
};
/**
@@ -82,6 +88,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,8 +321,19 @@ 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 */
+void backlight_notify_brightness(struct backlight_device *bd);
+
/**
* backlight_update_status - force an update of the backlight device status
* @bd: the backlight device
@@ -325,6 +347,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;
}
@@ -431,6 +457,21 @@ static inline void backlight_notify_blank_all(struct device *display_dev,
{ }
#endif
+int backlight_set_brightness(struct backlight_device *bd, unsigned int value,
+ enum backlight_update_reason reason);
+
+static inline void backlight_device_ref(struct backlight_device *bd)
+{
+ if (bd)
+ get_device(&bd->dev);
+}
+
+static inline void backlight_device_unref(struct backlight_device *bd)
+{
+ if (bd)
+ put_device(&bd->dev);
+}
+
#define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
/**
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 02/12] backlight: add kernel-internal backlight API
2026-08-31 21:05 ` [PATCH v7 02/12] backlight: add kernel-internal backlight API Mario Limonciello
@ 2026-08-31 22:11 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-08-31 22:11 UTC (permalink / raw)
To: Mario Limonciello; +Cc: intel-xe, dri-devel, intel-gfx
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] DRM takeover protection is missing from bl_power_store(), allowing userspace to bypass DRM exclusivity and toggle the backlight power state.
- [Medium] backlight_set_brightness() incorrectly returns success (0) and generates a spurious event when the backlight device is unregistered or unbound (bd->ops is NULL).
- [Medium] guard(mutex)(&bd->ops_lock) unintentionally extends the lock scope over backlight_generate_event(), violating intended lock lifetime and risking lock contention.
- [Medium] backlight_notify_brightness() is called from inconsistent locking contexts, exposing registered notifier chains to AB-BA deadlocks or lockdep splats.
--
commit bb82de7554b837e30f99cb722d8f1a608b68529b
Author: Mario Limonciello <mario.limonciello@amd.com>
Subject: backlight: add kernel-internal backlight API
This commit adds kernel-internal backlight helpers to allow modifying
backlights from within DRM. It introduces a brightness-changed notification
and a per-device takeover count to inhibit legacy sysfs writes while a DRM
client is in control.
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 51d8c3501768b..eec2aeadcc4b9 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
[ ... ]
> @@ -214,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;
> +
[Severity: High]
Since drm_takeover prevents legacy sysfs writes here, does bl_power_store()
also need this check?
Without it, could userspace write to the legacy bl_power sysfs attribute to
toggle the backlight power state, completely bypassing the DRM exclusivity
constraint?
[ ... ]
> @@ -514,6 +524,39 @@ 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) {
[Severity: Medium]
If the backlight device is unregistered or unbound and bd->ops is NULL, this
skips the if block, but rc remains 0.
Could this falsely signal success to the caller and generate a spurious
uevent even though no brightness change actually occurred?
> + if (value > bd->props.max_brightness)
> + return -EINVAL;
> +
> + dev_dbg(&bd->dev, "set brightness to %u\n", value);
> + bd->props.brightness = value;
> + rc = backlight_update_status(bd);
> + }
> + if (rc == 0)
> + backlight_generate_event(bd, reason);
[Severity: Medium]
The function-scoped guard causes ops_lock to remain held during the call to
backlight_generate_event(), which in turn calls sysfs_notify() and
kobject_uevent_env().
Does this unintentionally extend the lock scope over the event generation,
departing from the convention of releasing the lock before emitting events
and potentially increasing lock contention?
> +
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(backlight_set_brightness);
[ ... ]
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 015183d129f96..18d9145198b5f 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
[ ... ]
> @@ -325,6 +347,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);
[Severity: Medium]
This emits the notifier call without guaranteeing a consistent state of
ops_lock across all call paths.
When backlight_update_status() is called from the sysfs path via
backlight_device_set_brightness(), bd->ops_lock is held. However, when
triggered by driver hotkeys, ops_lock might not be held. Does this expose
registered notifier chains to inconsistent locking contexts, risking lockdep
splats or AB-BA deadlocks if listeners acquire locks that nest with ops_lock?
> +
> return ret;
> }
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260831210531.2334221-1-mario.limonciello@amd.com?part=2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v7 03/12] drm/property: add a per-connector luminance flag
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
2026-08-31 21:05 ` [PATCH v7 01/12] Revert "backlight: Remove notifier" Mario Limonciello
2026-08-31 21:05 ` [PATCH v7 02/12] backlight: add kernel-internal backlight API Mario Limonciello
@ 2026-08-31 21:05 ` Mario Limonciello
2026-08-31 21:05 ` [PATCH v7 04/12] drm: add connector backlight (LUMINANCE) infrastructure Mario Limonciello
` (9 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Mario Limonciello @ 2026-08-31 21:05 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede, Mario Limonciello, Mario Limonciello
The upcoming per-connector LUMINANCE range property needs to accept the
value 0 (to turn the display off on DPMS-off) even when its advertised
minimum is 1. The existing check special-cased a single device-wide
property object, which does not work once every connector owns its own
LUMINANCE property.
Add a kernel-internal is_luminance flag on struct drm_property and key
the value-0 exception off it instead of a pointer comparison. The flag
is not exposed to userspace.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/gpu/drm/drm_property.c | 6 ++++++
include/drm/drm_property.h | 10 ++++++++++
2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index f38f2c5437e68..adccee10cfde4 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 the luminance property: allow 0 to turn the
+ * display off even when the normal range starts at 1.
+ */
+ if (property->is_luminance && value == 0 && property->values[1] > 0)
+ return true;
if (value < property->values[0] || value > property->values[1])
return false;
return true;
diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
index aa49b5a42bb56..8bb568a4cd485 100644
--- a/include/drm/drm_property.h
+++ b/include/drm/drm_property.h
@@ -188,6 +188,16 @@ struct drm_property {
*/
struct drm_device *dev;
+ /**
+ * @is_luminance:
+ *
+ * True for the per-connector LUMINANCE range property. Such a property
+ * additionally accepts the value 0 (to turn the display off) even when
+ * its minimum is 1. This is a kernel-internal flag and is not exposed
+ * to userspace.
+ */
+ bool is_luminance;
+
/**
* @enum_list:
*
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v7 04/12] drm: add connector backlight (LUMINANCE) infrastructure
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
` (2 preceding siblings ...)
2026-08-31 21:05 ` [PATCH v7 03/12] drm/property: add a per-connector luminance flag Mario Limonciello
@ 2026-08-31 21:05 ` Mario Limonciello
2026-08-31 22:28 ` sashiko-bot
2026-08-31 21:05 ` [PATCH v7 05/12] drm: add DRM_CLIENT_CAP_LUMINANCE Mario Limonciello
` (8 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Mario Limonciello @ 2026-08-31 21:05 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede, Mario Limonciello, David Herrmann,
Mario Limonciello
Backlight brightness is a property of a display, and thus of a DRM
connector, yet it has historically only been controllable through the
separate backlight sysfs interface. Add a generic, backend-agnostic
per-connector LUMINANCE range property so brightness can be driven
through the atomic modeset path like any other connector state.
A struct drm_backlight is embedded in every connector and initialized by
the core; drivers do not allocate it. A driver links a backend (today a
backlight_device, in the future DDC/CI, MIPI-DCS, ...) with
drm_backlight_link(), which creates the connector's LUMINANCE property
with the backend's range. The property value is staged into the atomic
connector state and only pushed to the hardware from the commit/enable
path, via a workqueue so that slow backends never stall a commit. DPMS
off drives the backlight to 0 and DPMS on restores the committed value.
The property range is per-connector (created from the backend's
max_brightness), so multiple panels no longer share and corrupt a single
device-wide range. drm_backlight_link() also carries the legacy-sysfs
takeover accounting used by the client capability added in a later patch.
The whole feature is guarded by CONFIG_DRM_BACKLIGHT (which depends on,
rather than selects, BACKLIGHT_CLASS_DEVICE) so DRM does not pull the
backlight subsystem into the kernel when it is not wanted.
Co-developed-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/gpu/drm/Kconfig | 18 +
drivers/gpu/drm/Makefile | 2 +
drivers/gpu/drm/drm_atomic_helper.c | 39 +++
drivers/gpu/drm/drm_atomic_uapi.c | 49 ++-
drivers/gpu/drm/drm_backlight.c | 515 ++++++++++++++++++++++++++++
drivers/gpu/drm/drm_connector.c | 56 +++
drivers/gpu/drm/drm_drv.c | 8 +
drivers/gpu/drm/drm_mode_config.c | 1 +
drivers/gpu/drm/drm_sysfs.c | 26 +-
include/drm/drm_atomic_helper.h | 2 +
include/drm/drm_backlight.h | 158 +++++++++
include/drm/drm_connector.h | 20 ++
12 files changed, 890 insertions(+), 4 deletions(-)
create mode 100644 drivers/gpu/drm/drm_backlight.c
create mode 100644 include/drm/drm_backlight.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 323422861e8f6..ddb6827d613f8 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -33,6 +33,24 @@ endmenu
if DRM
+config DRM_BACKLIGHT
+ bool "DRM connector backlight (luminance) support"
+ depends on DRM
+ depends on BACKLIGHT_CLASS_DEVICE
+ default DRM
+ help
+ Expose per-connector backlight brightness control through the DRM
+ connector LUMINANCE property, backed by the backlight subsystem (and,
+ in the future, other backends such as DDC/CI). This lets luminance
+ changes go through the same atomic modeset path as the rest of the
+ display state.
+
+ This depends on BACKLIGHT_CLASS_DEVICE. When that is disabled, DRM is
+ built without this support and does not pull the backlight subsystem
+ into the kernel.
+
+ If in doubt, say Y.
+
config DRM_MIPI_DBI
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index e635fcffd3790..bcb06dbb71343 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -78,6 +78,8 @@ drm-$(CONFIG_DRM_CLIENT) += \
drm_client_event.o \
drm_client_modeset.o \
drm_client_sysrq.o
+drm-$(CONFIG_DRM_BACKLIGHT) += drm_backlight.o
+drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.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_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 285aac3554dfd..3603a8cce074e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -33,6 +33,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>
@@ -1230,6 +1231,10 @@ 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);
+
+ /* Turn the backlight off before disabling the pipeline. */
+ drm_backlight_set_luminance(connector, 0);
+
drm_atomic_bridge_chain_disable(bridge, state);
drm_bridge_put(bridge);
@@ -1744,6 +1749,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);
+
+ /* Restore the backlight once the pipeline is enabled. */
+ if (connector->state)
+ drm_atomic_helper_connector_apply_luminance(connector->state);
}
}
EXPORT_SYMBOL(drm_atomic_helper_commit_encoder_bridge_enable);
@@ -1775,6 +1784,24 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
+/**
+ * drm_atomic_helper_connector_apply_luminance - apply connector luminance from atomic state
+ * @conn_state: atomic connector state to apply luminance for
+ *
+ * Updates the backlight luminance from the atomic connector state. If the
+ * connector has a linked backlight and is associated with an active CRTC,
+ * push the luminance value to hardware.
+ */
+void drm_atomic_helper_connector_apply_luminance(const struct drm_connector_state *conn_state)
+{
+ struct drm_connector *connector = conn_state->connector;
+
+ if (conn_state->crtc && conn_state->crtc->state &&
+ conn_state->crtc->state->active)
+ drm_backlight_set_luminance(connector, conn_state->luminance);
+}
+EXPORT_SYMBOL(drm_atomic_helper_connector_apply_luminance);
+
/*
* For atomic updates which touch just a single CRTC, calculate the time of the
* next vblank, and inform all the fences of the deadline.
@@ -1989,6 +2016,9 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_flip_done);
void drm_atomic_helper_commit_tail(struct drm_atomic_commit *state)
{
struct drm_device *dev = state->dev;
+ struct drm_connector *connector;
+ struct drm_connector_state *new_conn_state;
+ int i;
drm_atomic_helper_commit_modeset_disables(dev, state);
@@ -1996,6 +2026,9 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_commit *state)
drm_atomic_helper_commit_modeset_enables(dev, state);
+ for_each_new_connector_in_state(state, connector, new_conn_state, i)
+ drm_atomic_helper_connector_apply_luminance(new_conn_state);
+
drm_atomic_helper_fake_vblank(state);
drm_atomic_helper_commit_hw_done(state);
@@ -2019,11 +2052,17 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_commit *state)
{
struct drm_device *dev = state->dev;
+ struct drm_connector *connector;
+ struct drm_connector_state *new_conn_state;
+ int i;
drm_atomic_helper_commit_modeset_disables(dev, state);
drm_atomic_helper_commit_modeset_enables(dev, state);
+ for_each_new_connector_in_state(state, connector, new_conn_state, i)
+ drm_atomic_helper_connector_apply_luminance(new_conn_state);
+
drm_atomic_helper_commit_planes(dev, state,
DRM_PLANE_COMMIT_ACTIVE_ONLY);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 1050dddadb17e..5e687600ce45e 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>
@@ -962,6 +964,13 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
state->hdmi.broadcast_rgb = val;
} else if (property == connector->color_format_property) {
state->color_format = val;
+ } else if (property == connector->luminance_property) {
+ /*
+ * Only stage the value into the atomic state; the hardware is
+ * updated from the commit path (see
+ * drm_atomic_helper_connector_apply_luminance()).
+ */
+ state->luminance = val;
} else if (connector->funcs->atomic_set_property) {
return connector->funcs->atomic_set_property(connector,
state, property, val);
@@ -1049,6 +1058,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
*val = state->hdmi.broadcast_rgb;
} else if (property == connector->color_format_property) {
*val = state->color_format;
+ } else if (property == connector->luminance_property) {
+ *val = state->luminance;
} else if (connector->funcs->atomic_get_property) {
return connector->funcs->atomic_get_property(connector,
state, property, val);
@@ -1133,6 +1144,22 @@ static struct drm_pending_vblank_event *create_vblank_event(
return e;
}
+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_backlight_set_luminance(connector,
+ active ? connector->state->luminance : 0);
+ }
+ drm_connector_list_iter_end(&conn_iter);
+}
+
int drm_atomic_connector_commit_dpms(struct drm_atomic_commit *state,
struct drm_connector *connector,
int mode)
@@ -1155,9 +1182,29 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_commit *state,
if (connector->dpms == mode)
goto out;
+ crtc = connector->state ? connector->state->crtc : NULL;
+
+ /* Handle backlight brightness coordination with DPMS state changes */
+ if (old_mode != DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_OFF) {
+ /* DPMS ON -> OFF: dim all connectors driven by this CRTC. */
+ if (crtc)
+ drm_atomic_crtc_set_backlight(crtc, false);
+ else
+ drm_backlight_set_luminance(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_backlight_set_luminance(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
new file mode 100644
index 0000000000000..4251cb25079c3
--- /dev/null
+++ b/drivers/gpu/drm/drm_backlight.c
@@ -0,0 +1,515 @@
+// 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/list.h>
+#include <linux/notifier.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+
+#include <drm/drm_backlight.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_device.h>
+#include <drm/drm_property.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 a generic per-connector LUMINANCE
+ * property. The core is backend-agnostic: it does not talk to any hardware
+ * itself, it only forwards luminance requests to a backend that a driver has
+ * linked. The backend is described by a &struct drm_backlight_funcs. Today the
+ * only backend is the backlight subsystem (&struct backlight_device), linked
+ * with drm_backlight_link(), but other backends (DDC/CI, MIPI-DCS, ...) can be
+ * added by providing a different set of operations without changing the core.
+ *
+ * A &struct drm_backlight is embedded in every &struct drm_connector and
+ * initialized by the DRM core (drm_backlight_connector_init()); drivers do not
+ * allocate it. Drivers link a backend once it is available by calling
+ * drm_backlight_link(); this creates the connector's LUMINANCE property with
+ * the backend's range. Passing NULL unlinks the backend. Hardware is only ever
+ * touched from a workqueue, so slow backends never stall an atomic commit.
+ */
+
+static LIST_HEAD(drm_backlight_list);
+static DEFINE_SPINLOCK(drm_backlight_lock);
+
+/* caller must hold @drm_backlight_lock */
+static bool __drm_backlight_is_linked(struct drm_backlight *b)
+{
+ lockdep_assert_held(&drm_backlight_lock);
+ /* a backlight is live while it is on @drm_backlight_list */
+ return !list_empty(&b->list);
+}
+
+/*
+ * Return the linked &backlight_device if the current backend is the backlight
+ * subsystem, or NULL otherwise. The legacy-sysfs takeover accounting only
+ * applies to that backend.
+ */
+static const struct drm_backlight_funcs drm_backlight_bd_funcs;
+
+static struct backlight_device *drm_backlight_bd(struct drm_backlight *b)
+{
+ if (b->funcs != &drm_backlight_bd_funcs)
+ return NULL;
+ return b->backend;
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_schedule(struct drm_backlight *b)
+{
+ lockdep_assert_held(&drm_backlight_lock);
+ if (__drm_backlight_is_linked(b))
+ schedule_work(&b->work);
+}
+
+static void __drm_backlight_worker(struct work_struct *w)
+{
+ struct drm_backlight *b = container_of(w, struct drm_backlight, work);
+ static char *ep[] = { "BACKLIGHT=1", NULL };
+ const struct drm_backlight_funcs *funcs;
+ bool send_uevent;
+ unsigned int v;
+
+ scoped_guard(spinlock, &drm_backlight_lock) {
+ send_uevent = b->changed;
+ b->changed = false;
+ v = b->set_value;
+ funcs = b->funcs;
+ }
+
+ /*
+ * The backend stays valid here: drm_backlight_do_unlink() clears
+ * @backend and drops its reference only after cancel_work_sync(), so an
+ * in-flight worker always sees a live backend.
+ */
+ if (funcs && funcs->set_luminance)
+ WARN_ON(funcs->set_luminance(b, v));
+
+ if (send_uevent && b->connector->kdev)
+ kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE, ep);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_set(struct drm_backlight *b, unsigned int v)
+{
+ unsigned int max = 0;
+ bool can_disable = false;
+
+ lockdep_assert_held(&drm_backlight_lock);
+
+ if (!b->funcs || !b->funcs->get_range)
+ return;
+
+ b->funcs->get_range(b, &max, &can_disable);
+ if (!max)
+ return;
+
+ /* clamp to the backend maximum */
+ b->set_value = min(v, max);
+ __drm_backlight_schedule(b);
+}
+
+/* caller must hold @drm_backlight_lock */
+static void __drm_backlight_readback(struct drm_backlight *b, unsigned int v)
+{
+ struct drm_connector *connector = b->connector;
+ unsigned int max = 0;
+ bool can_disable = false;
+
+ lockdep_assert_held(&drm_backlight_lock);
+
+ if (!b->funcs || !b->funcs->get_range)
+ return;
+
+ b->funcs->get_range(b, &max, &can_disable);
+ if (!max)
+ return;
+
+ /*
+ * Reflect a hardware-side brightness change (firmware hotkeys, or a
+ * legacy sysfs write while not inhibited) back into the connector's
+ * committed luminance so a read-back returns the real value.
+ */
+ if (connector->state)
+ connector->state->luminance = min(v, max);
+}
+
+/*
+ * Create and attach the per-connector LUMINANCE property. Runs in process
+ * context (driver register path) with no spinlock held, since it allocates.
+ * The property's min/max are baked in for its lifetime, so it is never mutated
+ * afterwards (which would race &drm_mode_getproperty_ioctl); it is freed by
+ * drm_mode_config_cleanup().
+ */
+static int drm_backlight_create_property(struct drm_connector *connector,
+ unsigned int max, bool can_disable)
+{
+ struct drm_device *dev = connector->dev;
+ struct drm_property *prop;
+ unsigned int min = can_disable ? 0 : 1;
+
+ prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
+ "LUMINANCE", min, max);
+ if (!prop)
+ return -ENOMEM;
+
+ prop->is_luminance = true;
+ connector->luminance_property = prop;
+ drm_object_attach_property(&connector->base, prop, min);
+ if (connector->state)
+ connector->state->luminance = min;
+
+ return 0;
+}
+
+static void drm_backlight_do_unlink(struct drm_backlight *b)
+{
+ struct backlight_device *bd;
+ unsigned int clients;
+
+ /*
+ * Stop new work first, but leave @backend in place so an in-flight
+ * worker keeps a valid backend to operate on. Capture the linked
+ * backlight_device (if any) while @funcs is still set.
+ */
+ scoped_guard(spinlock, &drm_backlight_lock) {
+ if (!b->funcs)
+ return;
+ bd = drm_backlight_bd(b);
+ clients = b->luminance_clients;
+ b->funcs = NULL;
+ list_del_init(&b->list);
+ }
+
+ cancel_work_sync(&b->work);
+
+ scoped_guard(spinlock, &drm_backlight_lock) {
+ if (clients && bd)
+ atomic_sub(clients, &bd->drm_takeover);
+ b->backend = NULL;
+ }
+
+ backlight_device_unref(bd);
+}
+
+/**
+ * drm_backlight_connector_init - initialize a connector's embedded backlight
+ * @connector: connector to initialize
+ *
+ * Called by the DRM core from drm_connector_init(). Drivers never call this.
+ */
+void drm_backlight_connector_init(struct drm_connector *connector)
+{
+ struct drm_backlight *b = &connector->backlight;
+
+ b->connector = connector;
+ INIT_LIST_HEAD(&b->list);
+ INIT_WORK(&b->work, __drm_backlight_worker);
+}
+
+/**
+ * drm_backlight_connector_cleanup - tear down a connector's embedded backlight
+ * @connector: connector being cleaned up
+ *
+ * Called by the DRM core from drm_connector_cleanup(). The LUMINANCE property
+ * itself is freed by drm_mode_config_cleanup().
+ */
+void drm_backlight_connector_cleanup(struct drm_connector *connector)
+{
+ struct drm_backlight *b = &connector->backlight;
+
+ WARN_ON(__drm_backlight_is_linked(b));
+ WARN_ON(b->funcs);
+ WARN_ON(b->luminance_clients);
+}
+
+/**
+ * drm_backlight_unregister - unlink a connector's backlight on unregister
+ * @connector: connector being unregistered
+ *
+ * Called by the DRM core from drm_connector_unregister() as a safety net in
+ * case a driver did not unlink its backend itself.
+ */
+void drm_backlight_unregister(struct drm_connector *connector)
+{
+ drm_backlight_do_unlink(&connector->backlight);
+}
+
+/**
+ * drm_backlight_link - link a backlight device to a connector
+ * @connector: connector to modify
+ * @bd: backlight device to link, or NULL to unlink
+ *
+ * Establish the link between a connector's LUMINANCE property and a registered
+ * backlight_device. On the first link the connector's LUMINANCE property is
+ * created with the backend's range. Passing NULL unlinks any linked device.
+ *
+ * User-space cannot create or modify this link.
+ *
+ * Returns: 0 on success, or a negative error code if the property could not be
+ * created (in which case no backend is linked).
+ */
+int drm_backlight_link(struct drm_connector *connector,
+ struct backlight_device *bd)
+{
+ struct drm_backlight *b = &connector->backlight;
+ unsigned int max;
+ int ret;
+
+ if (!bd) {
+ drm_backlight_do_unlink(b);
+ return 0;
+ }
+
+ /* Retarget: drop any previously linked backend first. */
+ if (b->funcs)
+ drm_backlight_do_unlink(b);
+
+ max = bd->props.max_brightness;
+ if (max && !connector->luminance_property) {
+ ret = drm_backlight_create_property(connector, max, false);
+ if (ret)
+ return ret;
+ }
+
+ scoped_guard(spinlock, &drm_backlight_lock) {
+ b->funcs = &drm_backlight_bd_funcs;
+ b->backend = bd;
+ backlight_device_ref(bd);
+ list_add(&b->list, &drm_backlight_list);
+ if (b->luminance_clients)
+ atomic_add(b->luminance_clients, &bd->drm_takeover);
+ __drm_backlight_readback(b, bd->props.brightness);
+ b->changed = true;
+ __drm_backlight_set(b, bd->props.brightness);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_backlight_link);
+
+/**
+ * drm_backlight_get_device - get the backlight_device linked to a connector
+ * @connector: connector to query
+ *
+ * Returns the &backlight_device linked to @connector, or NULL if no backlight
+ * subsystem device is linked.
+ */
+struct backlight_device *drm_backlight_get_device(struct drm_connector *connector)
+{
+ guard(spinlock)(&drm_backlight_lock);
+ return drm_backlight_bd(&connector->backlight);
+}
+EXPORT_SYMBOL(drm_backlight_get_device);
+
+/**
+ * drm_backlight_inhibit_legacy - disable legacy sysfs control of the backend
+ * @connector: connector whose backlight should be inhibited
+ *
+ * Record that one more luminance-aware DRM client has taken over this
+ * connector's backlight. While any clients are recorded, writes to the linked
+ * backlight_device's legacy ``brightness`` sysfs attribute return ``-EBUSY``.
+ * The takeover follows the linked device if the link changes.
+ *
+ * Calls must be balanced with drm_backlight_uninhibit_legacy().
+ */
+void drm_backlight_inhibit_legacy(struct drm_connector *connector)
+{
+ struct drm_backlight *b = &connector->backlight;
+ struct backlight_device *bd;
+
+ guard(spinlock)(&drm_backlight_lock);
+ b->luminance_clients++;
+ bd = drm_backlight_bd(b);
+ if (bd)
+ atomic_inc(&bd->drm_takeover);
+}
+EXPORT_SYMBOL(drm_backlight_inhibit_legacy);
+
+/**
+ * drm_backlight_uninhibit_legacy - re-enable legacy sysfs control
+ * @connector: connector to uninhibit
+ *
+ * Balances a previous drm_backlight_inhibit_legacy() call.
+ */
+void drm_backlight_uninhibit_legacy(struct drm_connector *connector)
+{
+ struct drm_backlight *b = &connector->backlight;
+ struct backlight_device *bd;
+
+ guard(spinlock)(&drm_backlight_lock);
+ if (WARN_ON(b->luminance_clients == 0))
+ return;
+ b->luminance_clients--;
+ bd = drm_backlight_bd(b);
+ if (bd)
+ atomic_dec(&bd->drm_takeover);
+}
+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
+ *
+ * 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);
+ 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);
+ drm_connector_list_iter_end(&iter);
+}
+EXPORT_SYMBOL(drm_backlight_uninhibit_legacy_all);
+
+/**
+ * drm_backlight_set_luminance - request a luminance change on a connector
+ * @connector: connector to update
+ * @value: luminance value to apply
+ *
+ * Clamp @value to the backend range and schedule the hardware update. Safe to
+ * call from an atomic commit tail: the actual hardware access happens later
+ * from a workqueue.
+ */
+void drm_backlight_set_luminance(struct drm_connector *connector,
+ unsigned int value)
+{
+ guard(spinlock)(&drm_backlight_lock);
+ __drm_backlight_set(&connector->backlight, value);
+}
+EXPORT_SYMBOL(drm_backlight_set_luminance);
+
+/* backlight_device backend ------------------------------------------------- */
+
+static int drm_backlight_bd_set_luminance(struct drm_backlight *b,
+ unsigned int value)
+{
+ struct backlight_device *bd = b->backend;
+ int rc;
+
+ rc = backlight_set_brightness(bd, value, BACKLIGHT_UPDATE_DRM);
+ if (rc)
+ backlight_set_brightness(bd, U16_MAX, BACKLIGHT_UPDATE_DRM);
+
+ return rc;
+}
+
+static int drm_backlight_bd_get_luminance(struct drm_backlight *b,
+ unsigned int *value)
+{
+ struct backlight_device *bd = b->backend;
+
+ if (!bd)
+ return -ENODEV;
+ *value = bd->props.brightness;
+
+ return 0;
+}
+
+static void drm_backlight_bd_get_range(struct drm_backlight *b,
+ unsigned int *max, bool *can_disable)
+{
+ struct backlight_device *bd = b->backend;
+
+ *max = bd ? bd->props.max_brightness : 0;
+ /*
+ * A generic backlight_device gives no guarantee that a value of 0 turns
+ * the panel fully off, so keep 0 reserved for the DPMS-off sentinel and
+ * expose a 1..max range.
+ */
+ *can_disable = false;
+}
+
+static const struct drm_backlight_funcs drm_backlight_bd_funcs = {
+ .set_luminance = drm_backlight_bd_set_luminance,
+ .get_luminance = drm_backlight_bd_get_luminance,
+ .get_range = drm_backlight_bd_get_range,
+};
+
+static int drm_backlight_notify(struct notifier_block *self,
+ unsigned long event, void *data)
+{
+ struct backlight_device *bd = data;
+ struct drm_backlight *b;
+
+ switch (event) {
+ case BACKLIGHT_UNREGISTERED:
+ /*
+ * Unlink every connector using @bd. drm_backlight_do_unlink()
+ * sleeps (cancel_work_sync()), so it cannot run under the list
+ * spinlock; re-scan for the next match after each unlink.
+ */
+ for (;;) {
+ struct drm_backlight *found = NULL;
+
+ scoped_guard(spinlock, &drm_backlight_lock) {
+ list_for_each_entry(b, &drm_backlight_list, list) {
+ if (drm_backlight_bd(b) == bd) {
+ found = b;
+ break;
+ }
+ }
+ }
+ if (!found)
+ break;
+ drm_backlight_do_unlink(found);
+ }
+ break;
+ case BACKLIGHT_BRIGHTNESS_CHANGED:
+ scoped_guard(spinlock, &drm_backlight_lock) {
+ list_for_each_entry(b, &drm_backlight_list, list)
+ if (drm_backlight_bd(b) == bd)
+ __drm_backlight_readback(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 8b4baed060f3a..92ed0c1879d53 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>
@@ -316,6 +317,8 @@ static int drm_connector_init_only(struct drm_device *dev,
drm_object_attach_property(&connector->base, config->prop_crtc_id, 0);
}
+ drm_backlight_connector_init(connector);
+
connector->debugfs_entry = NULL;
out_put_type_id:
if (ret)
@@ -773,6 +776,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
struct drm_device *dev = connector->dev;
struct drm_display_mode *mode, *t;
+ drm_backlight_connector_cleanup(connector);
/* The connector should have been removed from userspace long before
* it is finally destroyed.
*/
@@ -944,6 +948,8 @@ EXPORT_SYMBOL(drm_connector_dynamic_register);
void drm_connector_unregister(struct drm_connector *connector)
{
mutex_lock(&connector->mutex);
+ drm_backlight_unregister(connector);
+
if (connector->registration_state != DRM_CONNECTOR_REGISTERED) {
mutex_unlock(&connector->mutex);
return;
@@ -1532,6 +1538,56 @@ 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, per-connector range property for controlling the backlight
+ * brightness level of the connector's display. It provides unified access
+ * to the display backlight through the atomic modeset path, replacing the
+ * legacy sysfs interface for brightness control.
+ *
+ * The property value is an unsigned integer. Its valid range is baked in
+ * when a backlight backend is linked to the connector and reflects the
+ * backend's capabilities:
+ *
+ * - Range 1-N: Normal operation for a backend that cannot guarantee a full
+ * off state. 1 is the minimum *visible* brightness and N is the backend
+ * maximum. Drivers are expected never to program a duty cycle of 0 for a
+ * value of 1.
+ * - Range 0-N: Used when the backend can also fully turn the panel off, so
+ * 0 is a normal in-range value.
+ *
+ * Value 0 is always accepted, even when the advertised range starts at 1:
+ * it is the sentinel used to turn the backlight off when the connector is
+ * powered down (DPMS off). Turning the backlight off this way may power the
+ * panel down entirely; unlike programming a legacy sysfs duty cycle of 0,
+ * this can stop vblank/pageflip events until the connector is enabled
+ * again, so luminance-aware clients must not rely on such events while the
+ * connector is off.
+ *
+ * Connectors without a linked backend do not expose this property at all.
+ *
+ * For atomic drivers the value is stored in &drm_connector_state.luminance
+ * and applied to the hardware from the atomic commit path once the
+ * connector is enabled. When DPMS transitions to OFF the backlight is set
+ * to 0; when it transitions back to ON the committed luminance is restored.
+ * Reading the property returns the last committed value (or the hardware's
+ * current state for backends that support reading brightness back).
+ *
+ * The property is created by the DRM core when a driver links a backlight
+ * backend with drm_backlight_link(); drivers do not create it directly.
+ *
+ * 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 should continue
+ * using the sysfs interface (if available).
+ *
+ * Note: This property can be set through the 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), "OLED", and "LCD".
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index c808958a2188e..c8aa6834c1f4e 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>
@@ -1250,6 +1251,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();
@@ -1273,6 +1275,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 366f6d8212425..36579e8a7c0e5 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -33,6 +33,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"
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index ef4e923a87284..06cdfe046ca89 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,33 @@ 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");
+ struct backlight_device *bd = drm_backlight_get_device(connector);
+
+ if (connector->ddc) {
+ int ret = sysfs_create_link(&connector->kdev->kobj,
+ &connector->ddc->dev.kobj, "ddc");
+ if (ret)
+ return ret;
+ }
+
+ 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 (drm_backlight_get_device(connector))
+ sysfs_remove_link(&connector->kdev->kobj, "backlight");
+
if (connector->ddc)
sysfs_remove_link(&connector->kdev->kobj, "ddc");
}
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 4cfeec70d648a..8a166ead13c55 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -116,6 +116,8 @@ void drm_atomic_helper_commit_encoder_bridge_enable(struct drm_device *dev,
void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
struct drm_atomic_commit *old_state);
+void drm_atomic_helper_connector_apply_luminance(const struct drm_connector_state *conn_state);
+
int drm_atomic_helper_prepare_planes(struct drm_device *dev,
struct drm_atomic_commit *state);
void drm_atomic_helper_unprepare_planes(struct drm_device *dev,
diff --git a/include/drm/drm_backlight.h b/include/drm/drm_backlight.h
new file mode 100644
index 0000000000000..5cc1912babed9
--- /dev/null
+++ b/include/drm/drm_backlight.h
@@ -0,0 +1,158 @@
+/* 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/list.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+
+struct backlight_device;
+struct drm_backlight;
+struct drm_connector;
+struct drm_device;
+
+/**
+ * struct drm_backlight_funcs - backend operations for a DRM backlight
+ *
+ * A DRM backlight is backend-agnostic: the core forwards luminance requests to
+ * whatever backend a driver has linked. Today the only backend is the backlight
+ * subsystem (&struct backlight_device), but the same core can drive other
+ * backends (DDC/CI, MIPI-DCS, ...) by providing a different set of these
+ * operations without any change to the core.
+ *
+ * All callbacks are invoked from process context (a workqueue), never from an
+ * atomic commit tail or while holding a spinlock, so they are allowed to sleep.
+ */
+struct drm_backlight_funcs {
+ /**
+ * @set_luminance:
+ *
+ * Push @value (already clamped to the backend range) to the hardware.
+ * Returns 0 on success or a negative error code.
+ */
+ int (*set_luminance)(struct drm_backlight *b, unsigned int value);
+
+ /**
+ * @get_luminance:
+ *
+ * Read the current hardware luminance into @value. Returns 0 on success
+ * or a negative error code. May be NULL if the backend cannot be read
+ * back.
+ */
+ int (*get_luminance)(struct drm_backlight *b, unsigned int *value);
+
+ /**
+ * @get_range:
+ *
+ * Report the backend's maximum luminance in @max and whether the
+ * backend can turn the panel off at luminance 0 in @can_disable.
+ */
+ void (*get_range)(struct drm_backlight *b, unsigned int *max,
+ bool *can_disable);
+};
+
+/**
+ * struct drm_backlight - per-connector backlight state
+ *
+ * This structure is embedded in &struct drm_connector and initialized by the
+ * DRM core; drivers never allocate it. It becomes active once a driver links a
+ * backend with drm_backlight_link().
+ */
+struct drm_backlight {
+ /** @connector: connector this backlight belongs to */
+ struct drm_connector *connector;
+ /** @funcs: backend operations, or NULL while no backend is linked */
+ const struct drm_backlight_funcs *funcs;
+ /** @backend: backend private pointer (e.g. the &backlight_device) */
+ void *backend;
+ /** @list: entry on the global list of linked DRM backlights */
+ struct list_head list;
+ /** @work: deferred hardware update and uevent */
+ struct work_struct work;
+ /** @set_value: luminance value pending application by @work */
+ unsigned int set_value;
+ /**
+ * @luminance_clients: number of luminance-aware DRM clients that have
+ * taken this backlight over. While > 0, legacy sysfs writes to the
+ * linked backend return -EBUSY.
+ */
+ unsigned int luminance_clients;
+ /** @changed: a uevent is pending for @work to emit */
+ bool changed : 1;
+};
+
+#if IS_ENABLED(CONFIG_DRM_BACKLIGHT)
+
+int drm_backlight_init(void);
+void drm_backlight_exit(void);
+
+void drm_backlight_connector_init(struct drm_connector *connector);
+void drm_backlight_connector_cleanup(struct drm_connector *connector);
+void drm_backlight_unregister(struct drm_connector *connector);
+
+int drm_backlight_link(struct drm_connector *connector,
+ struct backlight_device *bd);
+struct backlight_device *drm_backlight_get_device(struct drm_connector *connector);
+
+void drm_backlight_inhibit_legacy(struct drm_connector *connector);
+void drm_backlight_uninhibit_legacy(struct drm_connector *connector);
+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_connector *connector,
+ unsigned int value);
+
+#else /* CONFIG_DRM_BACKLIGHT */
+
+static inline int drm_backlight_init(void) { return 0; }
+static inline void drm_backlight_exit(void) {}
+
+static inline void drm_backlight_connector_init(struct drm_connector *connector) {}
+static inline void drm_backlight_connector_cleanup(struct drm_connector *connector) {}
+static inline void drm_backlight_unregister(struct drm_connector *connector) {}
+
+static inline int drm_backlight_link(struct drm_connector *connector,
+ struct backlight_device *bd)
+{
+ return 0;
+}
+
+static inline struct backlight_device *
+drm_backlight_get_device(struct drm_connector *connector)
+{
+ return NULL;
+}
+
+static inline void drm_backlight_inhibit_legacy(struct drm_connector *connector) {}
+static inline void drm_backlight_uninhibit_legacy(struct drm_connector *connector) {}
+static inline void drm_backlight_inhibit_legacy_all(struct drm_device *dev) {}
+static inline void drm_backlight_uninhibit_legacy_all(struct drm_device *dev) {}
+
+static inline void drm_backlight_set_luminance(struct drm_connector *connector,
+ unsigned int value) {}
+
+#endif /* CONFIG_DRM_BACKLIGHT */
+
+#endif /* __DRM_BACKLIGHT_H__ */
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index a0cf0268de483..0535f20e7dba5 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -28,6 +28,7 @@
#include <linux/ctype.h>
#include <linux/hdmi.h>
#include <linux/notifier.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_mode_object.h>
#include <drm/drm_util.h>
#include <drm/drm_property.h>
@@ -1290,6 +1291,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 {
@@ -2526,6 +2532,20 @@ struct drm_connector {
* @cec: CEC-related data.
*/
struct drm_connector_cec cec;
+
+ /**
+ * @backlight: DRM backlight state, embedded and initialized by the DRM
+ * core. Becomes active once a driver links a backend with
+ * drm_backlight_link().
+ */
+ struct drm_backlight backlight;
+
+ /**
+ * @luminance_property: Per-connector range property controlling the
+ * connector's backlight luminance. Created with the backend's range
+ * when a backlight is linked; NULL while no backlight is linked.
+ */
+ struct drm_property *luminance_property;
};
#define obj_to_connector(x) container_of(x, struct drm_connector, base)
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 04/12] drm: add connector backlight (LUMINANCE) infrastructure
2026-08-31 21:05 ` [PATCH v7 04/12] drm: add connector backlight (LUMINANCE) infrastructure Mario Limonciello
@ 2026-08-31 22:28 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-08-31 22:28 UTC (permalink / raw)
To: Mario Limonciello; +Cc: intel-gfx, intel-xe, dri-devel
Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [High] Use-after-free of `drm_backlight` in `drm_backlight_notify` during concurrent unregistration.
- [High] Unprotected direct modification of the active DRM atomic connector state from an asynchronous backlight event.
- [High] `drm_backlight_get_device()` returns an unreferenced pointer, leading to Use-After-Free in sysfs linking.
- [High] Legacy DPMS state handling erroneously dims all connectors on a CRTC and fails to persist the DPMS OFF state across subsequent commits.
- [High] Data race dereferencing live `connector->state` in atomic commit tail instead of the captured new state.
- [High] Leaked backlight list entry and pending work item on connector cleanup error path causes Use-After-Free.
- [High] A TOCTOU race in `drm_backlight_do_unlink` can overwrite a concurrently linked new backend pointer with NULL, leading to a NULL pointer dereference.
- [High] Race condition in `drm_backlight_do_unlink` early return can lead to use-after-free during connector teardown.
- [Medium] Asynchronous backlight transitions violate strict display pipeline sequencing and DPMS semantics.
- [Medium] The 'backlight' sysfs symlink becomes dangling or stale on dynamic backend unlink or hot-swap.
- [Medium] The `get_range` callback is invoked while holding a spinlock and from the atomic commit tail, directly violating its API contract.
- [Medium] Unconditional lockdep assertion failure in `drm_backlight_connector_cleanup`.
- [Low] The drm_backlight core is hardcoded to `backlight_device`, preventing the addition of other backends without modifying the core.
--
commit a5f154fe3d2dddb5c9740e818e4993aaff0079d8
Author: Mario Limonciello <mario.limonciello@amd.com>
Subject: drm: add connector backlight (LUMINANCE) infrastructure
This commit adds a generic per-connector LUMINANCE property to allow driving
backlight brightness through the DRM atomic modeset path. It embeds a
drm_backlight structure in every connector and links it to backend devices
like the backlight subsystem via workqueues.
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 285aac3554dfd..3603a8cce074e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
[ ... ]
> @@ -1230,6 +1231,10 @@ 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);
> +
> + /* Turn the backlight off before disabling the pipeline. */
> + drm_backlight_set_luminance(connector, 0);
> +
> drm_atomic_bridge_chain_disable(bridge, state);
> drm_bridge_put(bridge);
[Severity: Medium]
Does this sequence violate DPMS semantics and cause visual artifacts?
drm_backlight_set_luminance() schedules the backlight to turn off asynchronously
via a workqueue, but drm_atomic_bridge_chain_disable() immediately disables
the display pipeline synchronously. The pipeline might turn off before the
backlight is actually off.
[ ... ]
> @@ -1744,6 +1749,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);
> +
> + /* Restore the backlight once the pipeline is enabled. */
> + if (connector->state)
> + drm_atomic_helper_connector_apply_luminance(connector->state);
[Severity: High]
Can this cause data races by dereferencing the live connector state?
Because this runs asynchronously in the non-blocking commit tail, the live
connector->state might have already been swapped by a subsequent atomic commit.
Should this use the captured new_conn_state instead?
[ ... ]
> +void drm_atomic_helper_connector_apply_luminance(const struct drm_connector_state *conn_state)
> +{
> + struct drm_connector *connector = conn_state->connector;
> +
> + if (conn_state->crtc && conn_state->crtc->state &&
> + conn_state->crtc->state->active)
> + drm_backlight_set_luminance(connector, conn_state->luminance);
> +}
[Severity: High]
Does this unintentionally override the legacy DPMS state on subsequent commits?
By relying solely on the CRTC's active state, if a single connector is set to
DPMS OFF (but the CRTC remains active for other cloned connectors), the next
atomic commit will incorrectly restore this connector's backlight.
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 1050dddadb17e..5e687600ce45e 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
[ ... ]
> + /* 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);
[Severity: High]
Does this erroneously dim all connectors on the CRTC instead of just the
target connector?
When userspace sets one connector to DPMS OFF on a cloned output setup, this
will unintentionally turn off the backlight for all other active connectors
attached to the same CRTC.
> diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
> new file mode 100644
> index 0000000000000..4251cb25079c3
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_backlight.c
[ ... ]
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_set(struct drm_backlight *b, unsigned int v)
> +{
> + unsigned int max = 0;
> + bool can_disable = false;
> +
> + lockdep_assert_held(&drm_backlight_lock);
> +
> + if (!b->funcs || !b->funcs->get_range)
> + return;
> +
> + b->funcs->get_range(b, &max, &can_disable);
[Severity: Medium]
Can this trigger a sleeping in atomic context warning?
__drm_backlight_set() is called with the drm_backlight_lock spinlock held.
However, it invokes the get_range callback, which the API contract states
is allowed to sleep and must not be called from atomic contexts.
[ ... ]
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_readback(struct drm_backlight *b, unsigned int v)
> +{
[ ... ]
> + /*
> + * Reflect a hardware-side brightness change (firmware hotkeys, or a
> + * legacy sysfs write while not inhibited) back into the connector's
> + * committed luminance so a read-back returns the real value.
> + */
> + if (connector->state)
> + connector->state->luminance = min(v, max);
> +}
[Severity: High]
Is it safe to mutate the active DRM atomic connector state directly?
The connector->state represents the committed, immutable atomic state. Modifying
it outside of an atomic commit transaction could cause data races and torn state
when concurrent readers (like state duplication) access it.
[ ... ]
> +static void drm_backlight_do_unlink(struct drm_backlight *b)
> +{
> + struct backlight_device *bd;
> + unsigned int clients;
> +
> + /*
> + * Stop new work first, but leave @backend in place so an in-flight
> + * worker keeps a valid backend to operate on. Capture the linked
> + * backlight_device (if any) while @funcs is still set.
> + */
> + scoped_guard(spinlock, &drm_backlight_lock) {
> + if (!b->funcs)
> + return;
[Severity: High]
Can this early return lead to a use-after-free during connector teardown?
If one thread drops the lock to call cancel_work_sync(), a concurrent thread
(e.g., connector teardown) calling drm_backlight_do_unlink() will see
!b->funcs and return immediately. This second thread will proceed to free the
connector while the first thread is still executing cancel_work_sync(&b->work)
inside the freed memory block.
[Severity: Medium]
Does this missing cleanup leave a dangling sysfs symlink?
When unlinking the backend, the backlight sysfs symlink created in
drm_sysfs_connector_add_late() is not removed, which will leave a stale
symlink pointing to a freed device object.
> + bd = drm_backlight_bd(b);
> + clients = b->luminance_clients;
> + b->funcs = NULL;
> + list_del_init(&b->list);
> + }
> +
> + cancel_work_sync(&b->work);
> +
> + scoped_guard(spinlock, &drm_backlight_lock) {
> + if (clients && bd)
> + atomic_sub(clients, &bd->drm_takeover);
> + b->backend = NULL;
> + }
[Severity: High]
Can this cause a race condition resulting in a NULL pointer dereference?
After dropping the lock for cancel_work_sync(), a concurrent
drm_backlight_link() could have linked a new backend. Unconditionally executing
b->backend = NULL here will overwrite the newly linked backend, causing the next
hardware update to dereference NULL.
> +
> + backlight_device_unref(bd);
[Severity: Low]
Does this hardcoded unref violate the backend-agnostic design?
Resource cleanup is explicitly hardcoded to the backlight_device subsystem,
preventing the addition of other backends without modifying the core.
[ ... ]
> +void drm_backlight_connector_cleanup(struct drm_connector *connector)
> +{
> + struct drm_backlight *b = &connector->backlight;
> +
> + WARN_ON(__drm_backlight_is_linked(b));
[Severity: Medium]
Will this trigger an unconditional lockdep assertion failure?
__drm_backlight_is_linked() unconditionally asserts that drm_backlight_lock
is held, but it is being called here without acquiring the lock.
[Severity: High]
Does this error path leak memory and cause a use-after-free?
If a connector fails probe before being registered, drm_connector_cleanup()
is called, bypassing the unregister path. This function only warns but fails
to remove the backlight from drm_backlight_list or cancel its work item,
leaving a dangling pointer when the connector is freed.
[ ... ]
> +struct backlight_device *drm_backlight_get_device(struct drm_connector *connector)
> +{
> + guard(spinlock)(&drm_backlight_lock);
> + return drm_backlight_bd(&connector->backlight);
> +}
[Severity: High]
Can this lead to a use-after-free?
This returns a raw pointer to a reference-counted object without incrementing
the refcount. If the backlight is unregistered concurrently, callers
(like drm_sysfs_connector_add_late) will dereference a freed pointer.
[ ... ]
> +static int drm_backlight_notify(struct notifier_block *self,
> + unsigned long event, void *data)
> +{
[ ... ]
> + for (;;) {
> + struct drm_backlight *found = NULL;
> +
> + scoped_guard(spinlock, &drm_backlight_lock) {
> + list_for_each_entry(b, &drm_backlight_list, list) {
> + if (drm_backlight_bd(b) == bd) {
> + found = b;
> + break;
> + }
> + }
> + }
> + if (!found)
> + break;
> + drm_backlight_do_unlink(found);
[Severity: High]
Is the found pointer safe to dereference outside of the spinlock?
The spinlock is dropped before calling drm_backlight_do_unlink(found).
Concurrently, the connector embedding this backlight could be unregistered
and freed, leading to a use-after-free when found is dereferenced.
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index ef4e923a87284..06cdfe046ca89 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
[ ... ]
> @@ -391,15 +393,33 @@ 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");
> + struct backlight_device *bd = drm_backlight_get_device(connector);
[ ... ]
> + if (bd) {
> + int ret = sysfs_create_link(&connector->kdev->kobj,
> + &bd->dev.kobj, "backlight");
[Severity: High]
Can this cause a use-after-free when creating the sysfs link?
drm_backlight_get_device() does not take a reference on bd. If the backlight
device is concurrently unregistered and freed, dereferencing bd->dev.kobj
here will trigger a UAF.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260831210531.2334221-1-mario.limonciello@amd.com?part=4
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v7 05/12] drm: add DRM_CLIENT_CAP_LUMINANCE
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
` (3 preceding siblings ...)
2026-08-31 21:05 ` [PATCH v7 04/12] drm: add connector backlight (LUMINANCE) infrastructure Mario Limonciello
@ 2026-08-31 21:05 ` Mario Limonciello
2026-08-31 22:37 ` sashiko-bot
2026-08-31 21:05 ` [PATCH v7 06/12] drm/amd/display: Pass up errors reading actual brightness Mario Limonciello
` (7 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Mario Limonciello @ 2026-08-31 21:05 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede, Mario Limonciello, Mario Limonciello
The legacy backlight control interface can only be disabled when both the
client and the driver agree that luminance can be set during a modeset.
Add a DRIVER_CONNECTOR_LUMINANCE driver feature for the driver to
advertise support, and a DRM_CLIENT_CAP_LUMINANCE client capability for
a luminance-aware client to opt in.
When a client sets DRM_CLIENT_CAP_LUMINANCE, every 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 capability or closes its DRM
file. The takeover follows the active backlight_device when
drm_backlight_link() retargets the link.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/gpu/drm/drm_file.c | 5 +++++
drivers/gpu/drm/drm_ioctl.c | 17 +++++++++++++++++
include/drm/drm_drv.h | 7 +++++++
include/drm/drm_file.h | 8 ++++++++
include/uapi/drm/drm.h | 22 ++++++++++++++++++++++
5 files changed, 59 insertions(+)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index ec820686b3021..4d2520de7614c 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -41,6 +41,7 @@
#include <linux/slab.h>
#include <linux/vga_switcheroo.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_client_event.h>
#include <drm/drm_drv.h>
#include <drm/drm_file.h>
@@ -252,6 +253,10 @@ void drm_file_free(struct drm_file *file)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
drm_fb_release(file);
drm_property_destroy_user_blobs(dev, file);
+ if (file->supports_luminance_control) {
+ drm_backlight_uninhibit_legacy_all(dev);
+ file->supports_luminance_control = false;
+ }
}
if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 9039a39c43243..b09f4a3b9a955 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>
@@ -392,6 +394,21 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
file_priv->plane_color_pipeline = req->value;
break;
}
+ case DRM_CLIENT_CAP_LUMINANCE:
+ if (!drm_core_check_feature(dev, DRIVER_CONNECTOR_LUMINANCE))
+ return -EOPNOTSUPP;
+ 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/include/drm/drm_drv.h b/include/drm/drm_drv.h
index b23830494ed41..449160b9863f6 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -118,6 +118,13 @@ enum drm_driver_feature {
*/
DRIVER_CURSOR_HOTSPOT = BIT(9),
+ /**
+ * @DRIVER_CONNECTOR_LUMINANCE:
+ *
+ * Driver supports luminance control on a per connector basis.
+ */
+ DRIVER_CONNECTOR_LUMINANCE = BIT(10),
+
/* IMPORTANT: Below are all the legacy flags, add new ones above. */
/**
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 6ee70ad65e1fd..0bb1e53f36bec 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -248,6 +248,14 @@ struct drm_file {
*/
bool supports_virtualized_cursor_plane;
+ /**
+ * @supports_luminance_control:
+ *
+ * This client is capable of setting the luminance for connectors.
+ *
+ */
+ bool supports_luminance_control;
+
/**
* @master:
*
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index bc7ef7684099b..a3141d46d7d66 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -903,6 +903,28 @@ struct drm_get_cap {
*/
#define DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE 7
+/**
+ * DRM_CLIENT_CAP_LUMINANCE
+ *
+ * If set to 1, the client declares support for the LUMINANCE connector property
+ * and will control backlight brightness through the DRM atomic interface. This
+ * enables the kernel to expose the LUMINANCE property on connectors that have
+ * an associated backlight device.
+ *
+ * When this capability is enabled:
+ * - The LUMINANCE property becomes visible on supported connectors
+ * - Legacy sysfs writes to /sys/class/backlight/{*}/brightness will return
+ * -EBUSY to prevent conflicts with DRM-based brightness control
+ * - The client should include luminance values as part of atomic commits
+ * - Brightness changes are synchronized with display power state (DPMS)
+ *
+ * The LUMINANCE property accepts values from 0 to max_brightness, where 0 turns
+ * off the backlight, and 1 to max_brightness control the brightness level.
+ *
+ * This capability is supported starting in kernel 7.2.
+ */
+#define DRM_CLIENT_CAP_LUMINANCE 8
+
/* DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
struct drm_set_client_cap {
__u64 capability;
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 05/12] drm: add DRM_CLIENT_CAP_LUMINANCE
2026-08-31 21:05 ` [PATCH v7 05/12] drm: add DRM_CLIENT_CAP_LUMINANCE Mario Limonciello
@ 2026-08-31 22:37 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-08-31 22:37 UTC (permalink / raw)
To: Mario Limonciello; +Cc: dri-devel, intel-xe, intel-gfx
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Connector hotplugging or unplugging while DRM_CLIENT_CAP_LUMINANCE is active causes state desynchronization and triggers kernel warnings.
- [High] Concurrent calls to DRM_IOCTL_SET_CLIENT_CAP can leak `luminance_clients` reference counts, permanently inhibiting legacy sysfs backlight control (DoS).
--
commit d72da18676683e0c9579165708469f0cf974aa5b
Author: Mario Limonciello <mario.limonciello@amd.com>
drm: add DRM_CLIENT_CAP_LUMINANCE
This commit adds a DRIVER_CONNECTOR_LUMINANCE feature and a
DRM_CLIENT_CAP_LUMINANCE client capability. When a client enables this
capability, the kernel inhibits legacy sysfs backlight control by taking
over the backlight devices.
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
[ ... ]
> @@ -252,6 +253,10 @@ void drm_file_free(struct drm_file *file)
> if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> drm_fb_release(file);
> drm_property_destroy_user_blobs(dev, file);
> + if (file->supports_luminance_control) {
> + drm_backlight_uninhibit_legacy_all(dev);
> + file->supports_luminance_control = false;
> + }
> }
[Severity: High]
Will this correctly handle connectors that are hotplugged or unplugged
after the compositor sets DRM_CLIENT_CAP_LUMINANCE?
drm_backlight_uninhibit_legacy_all() iterates over the list of currently
present connectors. If a compositor enables DRM_CLIENT_CAP_LUMINANCE and a
monitor is later hotplugged via DP MST, the new connector will not have
its legacy sysfs backlight control inhibited.
When the compositor exits and drm_file_free() runs:
For newly hotplugged connectors, it will try to uninhibit them even though
they were never inhibited, triggering WARN_ON(b->luminance_clients == 0)
in drm_backlight_uninhibit_legacy().
Conversely, for connectors unplugged before the compositor exits,
drm_backlight_connector_cleanup() will fire WARN_ON(b->luminance_clients)
because the connector is destroyed while its luminance_clients count is
still 1.
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
[ ... ]
> @@ -392,6 +394,21 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
> file_priv->plane_color_pipeline = req->value;
> break;
> }
> + case DRM_CLIENT_CAP_LUMINANCE:
> + if (!drm_core_check_feature(dev, DRIVER_CONNECTOR_LUMINANCE))
> + return -EOPNOTSUPP;
> + if (!file_priv->atomic)
> + return -EINVAL;
> + if (req->value > 1)
> + return -EINVAL;
> + if (req->value == file_priv->supports_luminance_control)
> + break;
> + if (req->value)
> + drm_backlight_inhibit_legacy_all(dev);
> + else
> + drm_backlight_uninhibit_legacy_all(dev);
> + file_priv->supports_luminance_control = req->value;
> + break;
[Severity: High]
Does this check-then-act sequence lack synchronization?
If two threads share a single DRM file descriptor and concurrently invoke
DRM_IOCTL_SET_CLIENT_CAP with DRM_CLIENT_CAP_LUMINANCE=1, both threads
could evaluate the supports_luminance_control condition as false before
either updates it.
Both threads would then call drm_backlight_inhibit_legacy_all(dev), causing
a double-increment of the luminance_clients reference count. When the file
descriptor is closed, drm_file_free() will only call
drm_backlight_uninhibit_legacy_all() once, permanently leaving the legacy
sysfs backlight interface inhibited and returning -EBUSY.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260831210531.2334221-1-mario.limonciello@amd.com?part=5
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v7 06/12] drm/amd/display: Pass up errors reading actual brightness
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
` (4 preceding siblings ...)
2026-08-31 21:05 ` [PATCH v7 05/12] drm: add DRM_CLIENT_CAP_LUMINANCE Mario Limonciello
@ 2026-08-31 21:05 ` Mario Limonciello
2026-08-31 22:49 ` sashiko-bot
2026-08-31 21:05 ` [PATCH v7 07/12] drm/amd: Indicate driver supports luminance Mario Limonciello
` (6 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Mario Limonciello @ 2026-08-31 21:05 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede, Mario Limonciello, Mario Limonciello
[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>
---
.../drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c | 11 ++++++++---
.../drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h | 2 +-
.../amdgpu_dm/tests/amdgpu_dm_backlight_test.c | 4 ++--
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
index e61bbc310f33c..fb2aaf0178c2b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
@@ -363,7 +363,7 @@ int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
EXPORT_IF_KUNIT(amdgpu_dm_backlight_update_status);
STATIC_IFN_KUNIT
-u32 amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm, int bl_idx)
+int amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm, int bl_idx)
{
int ret;
struct amdgpu_dm_backlight_caps caps;
@@ -376,7 +376,7 @@ u32 amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm, int bl_idx)
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);
}
@@ -394,8 +394,13 @@ int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
{
struct amdgpu_display_manager *dm = bl_get_data(bd);
int i = amdgpu_dm_backlight_get_device_index(dm, bd);
+ int ret;
+
+ ret = amdgpu_dm_backlight_get_level(dm, i);
+ if (ret < 0)
+ return dm->brightness[i];
- return amdgpu_dm_backlight_get_level(dm, i);
+ return ret;
}
EXPORT_IF_KUNIT(amdgpu_dm_backlight_get_brightness);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
index 07b75064847c7..72f1c8d861c8c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
@@ -56,7 +56,7 @@ extern const struct attribute_group amdgpu_group;
struct dc_stream_state *dm_find_stream_with_link(struct amdgpu_display_manager *dm,
struct dc_link *link);
int amdgpu_dm_backlight_update_status(struct backlight_device *bd);
-u32 amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm, int bl_idx);
+int amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm, int bl_idx);
int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd);
ssize_t panel_power_savings_show(struct device *device,
struct device_attribute *attr,
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
index 7ca17f803f9d7..e03173d4e5bd8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
@@ -382,7 +382,7 @@ static void dm_test_backlight_get_level_pwm_error(struct kunit *test)
dm->brightness[0] = 4321;
dm->backlight_link[0] = link;
- KUNIT_EXPECT_EQ(test, amdgpu_dm_backlight_get_level(dm, 0), 4321U);
+ KUNIT_EXPECT_EQ(test, amdgpu_dm_backlight_get_level(dm, 0), -EINVAL);
}
/**
@@ -424,7 +424,7 @@ static void dm_test_backlight_get_level_aux_error(struct kunit *test)
caps->caps_valid = true;
caps->aux_support = true;
- KUNIT_EXPECT_EQ(test, amdgpu_dm_backlight_get_level(dm, 0), 6789U);
+ KUNIT_EXPECT_EQ(test, amdgpu_dm_backlight_get_level(dm, 0), -EINVAL);
}
/**
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 06/12] drm/amd/display: Pass up errors reading actual brightness
2026-08-31 21:05 ` [PATCH v7 06/12] drm/amd/display: Pass up errors reading actual brightness Mario Limonciello
@ 2026-08-31 22:49 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-08-31 22:49 UTC (permalink / raw)
To: Mario Limonciello; +Cc: intel-gfx, dri-devel, intel-xe
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incomplete refactoring of PWM error path causes KUnit test failure.
--
commit 51bad29849babac3047c98c959ac850e269def79
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/amd/display: Pass up errors reading actual brightness
This patch changes the return type of amdgpu_dm_backlight_get_level() from
an unsigned integer to a signed integer. It also updates the caller and KUnit
tests to handle negative error codes like -EINVAL on backlight read failure.
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> index b66ca60e697dc..ed1f85650e280 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
[ ... ]
> @@ -376,7 +376,7 @@ u32 amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm, int bl_idx)
> 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);
> }
>
[Severity: Medium]
Was the legacy PWM error path missed during this refactoring?
A few lines down in amdgpu_dm_backlight_get_level(), there is another error
path:
ret = dc_link_get_backlight_level(link);
if (ret == DC_ERROR_UNEXPECTED)
return dm->brightness[bl_idx];
Since this still returns the cached fallback brightness instead of -EINVAL,
could this cause unexpected behavior for the updated KUnit test?
[ ... ]
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
> index 7ca17f803f9d7..e03173d4e5bd8 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
> @@ -382,7 +382,7 @@ static void dm_test_backlight_get_level_pwm_error(struct kunit *test)
> dm->brightness[0] = 4321;
> dm->backlight_link[0] = link;
>
> - KUNIT_EXPECT_EQ(test, amdgpu_dm_backlight_get_level(dm, 0), 4321U);
> + KUNIT_EXPECT_EQ(test, amdgpu_dm_backlight_get_level(dm, 0), -EINVAL);
[Severity: Medium]
Will this test unconditionally fail?
Because the legacy error path in amdgpu_dm_backlight_get_level() wasn't
updated to return -EINVAL, it seems the function will still return the cached
fallback value (4321) when the mock function returns DC_ERROR_UNEXPECTED.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260831210531.2334221-1-mario.limonciello@amd.com?part=6
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v7 07/12] drm/amd: Indicate driver supports luminance
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
` (5 preceding siblings ...)
2026-08-31 21:05 ` [PATCH v7 06/12] drm/amd/display: Pass up errors reading actual brightness Mario Limonciello
@ 2026-08-31 21:05 ` Mario Limonciello
2026-08-31 21:05 ` [PATCH v7 08/12] drm/amd/display: use drm backlight Mario Limonciello
` (5 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Mario Limonciello @ 2026-08-31 21:05 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede, Mario Limonciello, Mario Limonciello
This will allow the connector to show luminance information for
eDP panels.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index f9f1d6cf65c5e..26ba7852e1ceb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -3124,6 +3124,7 @@ static const struct drm_driver amdgpu_kms_driver = {
DRIVER_ATOMIC |
DRIVER_GEM |
DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ |
+ DRIVER_CONNECTOR_LUMINANCE |
DRIVER_SYNCOBJ_TIMELINE,
.open = amdgpu_driver_open_kms,
.postclose = amdgpu_driver_postclose_kms,
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v7 08/12] drm/amd/display: use drm backlight
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
` (6 preceding siblings ...)
2026-08-31 21:05 ` [PATCH v7 07/12] drm/amd: Indicate driver supports luminance Mario Limonciello
@ 2026-08-31 21:05 ` Mario Limonciello
2026-08-31 23:12 ` sashiko-bot
2026-08-31 21:05 ` [PATCH v7 09/12] drm/bridge: auto-link panel backlight in bridge connector Mario Limonciello
` (4 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Mario Limonciello @ 2026-08-31 21:05 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede, Mario Limonciello, Mario Limonciello
Convert the AMD display driver to the DRM backlight infrastructure so
brightness can be controlled through the connector LUMINANCE property.
Link the registered backlight_device to the eDP connector and unlink it
on teardown; the DRM core owns the embedded backlight state and its
property, so no explicit allocation or failure handling is needed.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++--
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c | 4 ++++
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c | 2 ++
3 files changed, 9 insertions(+), 2 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 ec483276d7538..e87d1e5be58fe 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5054,6 +5054,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
if (!acrtc || drm_atomic_crtc_needs_modeset(new_crtc_state))
continue;
+ drm_connector_update_privacy_screen(new_con_state);
+ drm_atomic_helper_connector_apply_luminance(new_con_state);
+
dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
@@ -5133,8 +5136,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
&stream_update);
mutex_unlock(&dm->dc_lock);
kfree(dummy_updates);
-
- drm_connector_update_privacy_screen(new_con_state);
}
/**
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
index fb2aaf0178c2b..a60d0b0480f57 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
@@ -40,6 +40,7 @@
#include <linux/backlight.h>
#include <linux/power_supply.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_edid.h>
#include <drm/drm_utils.h>
@@ -496,6 +497,9 @@ 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. */
+ drm_backlight_link(&aconnector->base, dm->backlight_dev[aconnector->bl_idx]);
+
drm_dbg_driver(drm, "DM: Registered Backlight device: %s\n", bl_name);
}
}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
index c8a1ab8c3b169..fe4ab55f5130f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
@@ -58,6 +58,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_edid.h>
#include <drm/drm_eld.h>
#include <drm/drm_fixed.h>
@@ -1911,6 +1912,7 @@ STATIC_IFN_KUNIT void amdgpu_dm_connector_destroy(struct drm_connector *connecto
}
if (aconnector->bl_idx != -1) {
+ drm_backlight_link(&aconnector->base, NULL);
backlight_device_unregister(dm->backlight_dev[aconnector->bl_idx]);
dm->backlight_dev[aconnector->bl_idx] = NULL;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 08/12] drm/amd/display: use drm backlight
2026-08-31 21:05 ` [PATCH v7 08/12] drm/amd/display: use drm backlight Mario Limonciello
@ 2026-08-31 23:12 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-08-31 23:12 UTC (permalink / raw)
To: Mario Limonciello; +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Data race and unconditional kernel WARN_ON due to dynamic DRM property creation after connector registration.
- [Medium] Luminance property updates are silently dropped when an atomic commit includes a modeset.
Pre-existing issues:
- [High] Atomic check and commit paths dereference a NULL CRTC state if a connector property is updated without the associated CRTC being pulled into the atomic state.
--
commit 3fc3b85f7e3897f97fa8465e7158825ddd3e0dd7
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/amd/display: use drm backlight
Convert the AMD display driver to the DRM backlight infrastructure so
brightness can be controlled through the connector LUMINANCE property.
Link the registered backlight_device to the eDP connector and unlink it
on teardown; the DRM core owns the embedded backlight state and its
property, so no explicit allocation or failure handling is needed.
> 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 284aac4d96bce..b5254c775a778 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5048,6 +5048,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
> if (!acrtc || drm_atomic_crtc_needs_modeset(new_crtc_state))
[Severity: High]
This is a pre-existing issue, but does this code dereference a NULL pointer
if a connector-only property is updated?
When a connector-only property is updated, the DRM core stages the property
but does not add the CRTC to the atomic state. The drm_atomic_get_new_crtc_state
call returns NULL when the CRTC is not in the state.
If userspace submits an atomic commit that updates only the newly exposed
LUMINANCE property without modifying the CRTC, new_crtc_state will be NULL
and immediately dereferenced inside drm_atomic_crtc_needs_modeset().
> continue;
>
> + drm_connector_update_privacy_screen(new_con_state);
> + drm_atomic_helper_connector_apply_luminance(new_con_state);
[Severity: Medium]
Will this cause luminance property updates to be silently dropped when an
atomic commit includes a modeset?
The continue statement immediately above skips the newly added
drm_atomic_helper_connector_apply_luminance() call if the CRTC needs a
modeset (e.g., when changing display resolution).
Since amdgpu_dm does not use standard DRM modeset helpers, the skipped
luminance update would never be applied to the hardware, and the backlight
would remain at its previous value.
[ ... ]
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> index ed1f85650e280..0174339f0ee79 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> @@ -496,6 +497,9 @@ 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. */
> + drm_backlight_link(&aconnector->base, dm->backlight_dev[aconnector->bl_idx]);
[Severity: High]
Does this trigger a kernel WARN_ON and data race by dynamically creating the
LUMINANCE property after connector registration?
Because amdgpu_dm_register_backlight_device() is executed during the late
registration phase (amdgpu_dm_connector_late_register), the connector has
already been exposed to userspace.
The drm_backlight_link() call creates the LUMINANCE property dynamically,
but adding properties after registration is prohibited by the DRM core.
Could this also cause a data race if userspace iterates the connector's
properties array (e.g., reacting to a sysfs KOBJ_ADD uevent by issuing
DRM_IOCTL_MODE_GETCONNECTOR) locklessly while it is being reallocated?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260831210531.2334221-1-mario.limonciello@amd.com?part=8
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v7 09/12] drm/bridge: auto-link panel backlight in bridge connector
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
` (7 preceding siblings ...)
2026-08-31 21:05 ` [PATCH v7 08/12] drm/amd/display: use drm backlight Mario Limonciello
@ 2026-08-31 21:05 ` Mario Limonciello
2026-08-31 23:24 ` sashiko-bot
2026-08-31 21:05 ` [PATCH v7 10/12] drm/xe: Indicate support for luminance on the connector Mario Limonciello
` (3 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Mario Limonciello @ 2026-08-31 21:05 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede, Mario Limonciello, Mario Limonciello
Link a panel's backlight_device to the bridge connector so
luminance-aware clients can drive it through the LUMINANCE property.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/gpu/drm/bridge/panel.c | 15 +++++++++++++++
drivers/gpu/drm/display/drm_bridge_connector.c | 11 ++++++++++-
include/drm/drm_bridge.h | 1 +
3 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 02388a3de626e..36005e10494d3 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 8b54069fa53a4..d4b4e59cea51c 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>
@@ -1082,9 +1084,16 @@ 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)
+ drm_backlight_link(connector, 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 58fff047f43b6..ce1fc8695b455 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1475,6 +1475,7 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
#ifdef CONFIG_DRM_PANEL_BRIDGE
bool drm_bridge_is_panel(const struct drm_bridge *bridge);
+struct drm_panel *drm_panel_bridge_to_panel(struct drm_bridge *bridge);
struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel);
struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
u32 connector_type);
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 09/12] drm/bridge: auto-link panel backlight in bridge connector
2026-08-31 21:05 ` [PATCH v7 09/12] drm/bridge: auto-link panel backlight in bridge connector Mario Limonciello
@ 2026-08-31 23:24 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-08-31 23:24 UTC (permalink / raw)
To: Mario Limonciello; +Cc: intel-gfx, intel-xe, dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing cleanup of `drm_backlight_link()` on probe error paths causes a Use-After-Free on a global list and a reference leak.
--
commit a0dd986fcfc01d36a6bb91afe25a294e80d0f4e6
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/bridge: auto-link panel backlight in bridge connector
This commit links a panel's backlight_device to the bridge connector so
luminance-aware clients can drive it through the LUMINANCE property.
> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 8b54069fa53a4..d4b4e59cea51c 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
[ ... ]
> @@ -1082,9 +1084,16 @@ 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)
> + drm_backlight_link(connector, panel->backlight);
[Severity: High]
Does this cause a use-after-free and a reference leak on probe error paths?
During early initialization here, drm_backlight_link() links the connector's
backlight to the global list. If the driver's probe function subsequently
fails before calling drm_dev_register() (e.g., due to EPROBE_DEFER or
hardware errors), the device is never registered.
Because drm_connector_unregister() is never called in this error path, the
backlight is not unlinked. Later, when drmm managed cleanup runs
drm_connector_cleanup(), it calls drm_backlight_connector_cleanup().
Looking at drm_backlight_connector_cleanup():
void drm_backlight_connector_cleanup(struct drm_connector *connector)
{
struct drm_backlight *b = &connector->backlight;
WARN_ON(__drm_backlight_is_linked(b));
WARN_ON(b->funcs);
WARN_ON(b->luminance_clients);
}
This function only warns and does not actually unlink the backlight. The
connector is then freed, leaving a dangling pointer in the global
drm_backlight_list. Iterating this global list later (e.g., during backlight
brightness changes) could dereference the freed connector memory.
> + }
> +
> if (support_hdcp && IS_REACHABLE(CONFIG_DRM_DISPLAY_HELPER) &&
> IS_ENABLED(CONFIG_DRM_DISPLAY_HDCP_HELPER))
> drm_connector_attach_content_protection_property(connector, true);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260831210531.2334221-1-mario.limonciello@amd.com?part=9
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v7 10/12] drm/xe: Indicate support for luminance on the connector
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
` (8 preceding siblings ...)
2026-08-31 21:05 ` [PATCH v7 09/12] drm/bridge: auto-link panel backlight in bridge connector Mario Limonciello
@ 2026-08-31 21:05 ` Mario Limonciello
2026-08-31 23:30 ` sashiko-bot
2026-08-31 21:05 ` [PATCH v7 11/12] drm/i915: " Mario Limonciello
` (2 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Mario Limonciello @ 2026-08-31 21:05 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede, Mario Limonciello
This will allow the connector to show luminance information for
eDP panels.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/gpu/drm/xe/xe_device.c | 3 ++-
include/drm/drm_drv.h | 7 +++++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index ee732e5495f7d..661f76fac3f35 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -397,7 +397,8 @@ static const struct drm_driver regular_driver = {
XE_DISPLAY_DRIVER_FEATURES |
DRIVER_GEM |
DRIVER_RENDER | DRIVER_SYNCOBJ |
- DRIVER_SYNCOBJ_TIMELINE,
+ DRIVER_CONNECTOR_LUMINANCE |
+ DRIVER_SYNCOBJ_TIMELINE | DRIVER_GEM_GPUVA,
.open = xe_file_open,
.postclose = xe_file_close,
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 449160b9863f6..bda0f0bd7b16b 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -125,6 +125,13 @@ enum drm_driver_feature {
*/
DRIVER_CONNECTOR_LUMINANCE = BIT(10),
+ /**
+ * @DRIVER_GEM_GPUVA:
+ *
+ * Driver supports GEM GPU virtual addressing.
+ */
+ DRIVER_GEM_GPUVA = BIT(11),
+
/* IMPORTANT: Below are all the legacy flags, add new ones above. */
/**
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 10/12] drm/xe: Indicate support for luminance on the connector
2026-08-31 21:05 ` [PATCH v7 10/12] drm/xe: Indicate support for luminance on the connector Mario Limonciello
@ 2026-08-31 23:30 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-08-31 23:30 UTC (permalink / raw)
To: Mario Limonciello; +Cc: dri-devel, intel-xe, intel-gfx
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit silently introduces and enables a new, undocumented DRM feature flag (DRIVER_GEM_GPUVA), which is completely unrelated to connector luminance.
--
commit 2a9c8dfbbc421c84ac6987dc02eb16939b1f82f5
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/xe: Indicate support for luminance on the connector
This commit enables the connector to show luminance information for eDP panels
in the xe driver by setting the DRIVER_CONNECTOR_LUMINANCE driver feature.
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index f9791c850e520..b65bbe8fde0c3 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -397,7 +397,8 @@ static const struct drm_driver regular_driver = {
> XE_DISPLAY_DRIVER_FEATURES |
> DRIVER_GEM |
> DRIVER_RENDER | DRIVER_SYNCOBJ |
> - DRIVER_SYNCOBJ_TIMELINE,
> + DRIVER_CONNECTOR_LUMINANCE |
> + DRIVER_SYNCOBJ_TIMELINE | DRIVER_GEM_GPUVA,
[Severity: Medium]
Does this code intentionally enable DRIVER_GEM_GPUVA for the xe driver
in this patch?
The commit message only mentions adding support for luminance on the
connector, which suggests this flag might have been accidentally squashed
into this patch.
> .open = xe_file_open,
> .postclose = xe_file_close,
>
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 449160b9863f6..bda0f0bd7b16b 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -125,6 +125,13 @@ enum drm_driver_feature {
> */
> DRIVER_CONNECTOR_LUMINANCE = BIT(10),
>
> + /**
> + * @DRIVER_GEM_GPUVA:
> + *
> + * Driver supports GEM GPU virtual addressing.
> + */
> + DRIVER_GEM_GPUVA = BIT(11),
> +
[Severity: Medium]
Is the DRIVER_GEM_GPUVA flag definition meant to be included here?
It appears unrelated to the connector luminance changes described in the
commit message and might belong in a separate, dedicated patch.
> /* IMPORTANT: Below are all the legacy flags, add new ones above. */
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260831210531.2334221-1-mario.limonciello@amd.com?part=10
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v7 11/12] drm/i915: Indicate support for luminance on the connector
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
` (9 preceding siblings ...)
2026-08-31 21:05 ` [PATCH v7 10/12] drm/xe: Indicate support for luminance on the connector Mario Limonciello
@ 2026-08-31 21:05 ` Mario Limonciello
2026-08-31 21:05 ` [PATCH v7 12/12] drm/i915/display: use drm backlight Mario Limonciello
2026-09-01 0:19 ` [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
12 siblings, 0 replies; 23+ messages in thread
From: Mario Limonciello @ 2026-08-31 21:05 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede, Mario Limonciello
This will allow the connector to show luminance information for
eDP panels.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/gpu/drm/i915/i915_driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index ce6d20958320c..83cff711d28df 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1768,6 +1768,7 @@ static const struct drm_driver i915_drm_driver = {
.driver_features =
DRIVER_GEM |
DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_SYNCOBJ |
+ DRIVER_CONNECTOR_LUMINANCE |
DRIVER_SYNCOBJ_TIMELINE,
.release = i915_driver_release,
.open = i915_driver_open,
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v7 12/12] drm/i915/display: use drm backlight
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
` (10 preceding siblings ...)
2026-08-31 21:05 ` [PATCH v7 11/12] drm/i915: " Mario Limonciello
@ 2026-08-31 21:05 ` Mario Limonciello
2026-08-31 23:55 ` sashiko-bot
2026-09-01 0:19 ` [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
12 siblings, 1 reply; 23+ messages in thread
From: Mario Limonciello @ 2026-08-31 21:05 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede, Mario Limonciello
Convert the Intel display drivers (i915 and xe) to the DRM backlight
infrastructure so brightness can be controlled through the connector
LUMINANCE property. Link the backlight device to the eDP connector on
registration and unlink it on unregistration; the DRM core owns the
embedded backlight state and its property.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/gpu/drm/i915/display/intel_backlight.c | 4 ++++
drivers/gpu/drm/i915/display/intel_display.c | 7 ++++++-
drivers/gpu/drm/i915/display/intel_dp.c | 1 +
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
index b128896cb1c2d..b0cd4dc3adc00 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -9,6 +9,7 @@
#include <linux/string_helpers.h>
#include <acpi/video.h>
+#include <drm/drm_backlight.h>
#include <drm/drm_file.h>
#include <drm/drm_print.h>
#include <drm/intel/pci_config.h>
@@ -1004,6 +1005,8 @@ int intel_backlight_device_register(struct intel_connector *connector)
"[CONNECTOR:%d:%s] backlight device %s registered\n",
connector->base.base.id, connector->base.name, name);
+ drm_backlight_link(&connector->base, 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, NULL);
backlight_device_unregister(panel->backlight.device);
panel->backlight.device = NULL;
}
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 9dfd095f325f7..d53e5f4bdf57d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7496,10 +7496,12 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
struct intel_display *display = to_intel_display(state);
struct intel_uncore *uncore = to_intel_uncore(display->drm);
struct intel_crtc_state *new_crtc_state, *old_crtc_state;
+ struct drm_connector_state *new_conn_state;
+ struct drm_connector *connector;
struct intel_crtc *crtc;
struct intel_power_domain_mask put_domains[I915_MAX_PIPES] = {};
struct ref_tracker *wakeref = NULL;
- int power_async_delay;
+ int power_async_delay, i;
for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state)
intel_atomic_dsb_prepare(state, crtc);
@@ -7608,6 +7610,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
/* Now enable the clocks, plane, pipe, and connectors that we set up. */
display->modeset.funcs->commit_modeset_enables(state);
+ for_each_new_connector_in_state(&state->base, connector, new_conn_state, i)
+ drm_atomic_helper_connector_apply_luminance(new_conn_state);
+
intel_display_power_dc3co_compute(state);
/* FIXME probably need to sequence this properly */
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 113d767e62e9c..2381a80a4e671 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -43,6 +43,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>
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 12/12] drm/i915/display: use drm backlight
2026-08-31 21:05 ` [PATCH v7 12/12] drm/i915/display: use drm backlight Mario Limonciello
@ 2026-08-31 23:55 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-08-31 23:55 UTC (permalink / raw)
To: Mario Limonciello; +Cc: intel-gfx, intel-xe, dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Low] Unused include `<drm/drm_backlight.h>` in `intel_dp.c`
Pre-existing issues:
- [High] TOCTOU race in sysfs backlight update leading to NULL pointer dereference during non-blocking display disable
--
commit 6982034de97212a1664f3db93710a699b08ecdab
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/i915/display: use drm backlight
This commit converts the Intel display drivers (i915 and xe) to use the DRM
backlight infrastructure, allowing brightness control via the connector
LUMINANCE property. It links the backlight device to the eDP connector on
registration and unlinks it on unregistration.
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index b128896cb1c2d..b0cd4dc3adc00 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
[ ... ]
> @@ -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, NULL);
> backlight_device_unregister(panel->backlight.device);
> panel->backlight.device = NULL;
> }
[Severity: High]
This is a pre-existing issue, but could this cause a NULL pointer dereference
during a non-blocking display disable?
If there is a concurrent userspace write to the legacy sysfs brightness file
during a non-blocking display disable on VLV/CHV hardware, we might hit a race.
During the non-blocking commit, connection_mutex is dropped before the hardware
is disabled, allowing intel_backlight_device_update_status() to execute.
It uses the newly swapped connector->base.state (where crtc = NULL), but
panel->backlight.enabled remains true until the asynchronous commit tail
disables it.
This seems to lead to intel_panel_set_backlight() passing the state directly
to the backend without checking if conn_state->crtc is NULL:
drivers/gpu/drm/i915/display/intel_backlight.c:intel_panel_set_backlight() {
...
if (panel->backlight.enabled)
intel_panel_actually_set_backlight(conn_state, hw_level);
...
}
Which then calls into vlv_set_backlight() where the NULL crtc is dereferenced:
drivers/gpu/drm/i915/display/intel_backlight.c:vlv_set_backlight() {
...
enum pipe pipe = to_intel_crtc(conn_state->crtc)->pipe;
u32 tmp;
tmp = intel_de_read(display, VLV_BLC_PWM_CTL(pipe)) & ~BACKLIGHT_DUTY_CYCLE_MASK;
intel_de_write(display, VLV_BLC_PWM_CTL(pipe), tmp | level);
...
}
Does this require a check for conn_state->crtc being NULL before passing the
state, similar to how other backlight functions handle it?
[ ... ]
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 113d767e62e9c..2381a80a4e671 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -43,6 +43,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>
[Severity: Low]
Is this include used anywhere in this file?
It looks like <drm/drm_backlight.h> is added to intel_dp.c but no symbols from
it are utilized in this file, resulting in a dead include.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260831210531.2334221-1-mario.limonciello@amd.com?part=12
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v7 00/12] Add support for a DRM backlight capability
2026-08-31 21:05 [PATCH v7 00/12] Add support for a DRM backlight capability Mario Limonciello
` (11 preceding siblings ...)
2026-08-31 21:05 ` [PATCH v7 12/12] drm/i915/display: use drm backlight Mario Limonciello
@ 2026-09-01 0:19 ` Mario Limonciello
12 siblings, 0 replies; 23+ messages in thread
From: Mario Limonciello @ 2026-09-01 0:19 UTC (permalink / raw)
To: dri-devel, harry.wentland, Simona Vetter, Alex Deucher,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie
Cc: Xaver Hugl, amd-gfx,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
open list:INTEL DRM DISPLAY FOR XE AND I915 DRIVERS,
Hans de Goede
On 8/31/26 16:05, Mario Limonciello wrote:
> At Display Next Hackfest 2026 we reviewed progress moving brightness
> control into the DRM connector properties.
>
> There is a range LUMINANCE property that will default to 0->0.
> Once a driver attaches a backlight it will be updated to 1->max.
> If the panel supports the minimum backlight turning off the display
> the range can later be updated to 0->max instead of 1->max.
>
> The legacy sysfs interface is synchronized with the DRM connector.
> When a compositor using this feature is loaded, sysfs writes are disabled
> to prevent legacy tools from going out of sync with the compositor.
>
> This has an implementation initially for amdgpu, i915, and Xe with eDP
> connectors. It can be extended to other connectors like DP for displays
> that can be controlled via DDC as well later.
>
> The following compositors have implemented matching support:
> * Kwin: https://invent.kde.org/plasma/kwin/-/merge_requests/9298
> * Mutter: https://gitlab.gnome.org/swick/mutter/-/commits/wip/kms-luminance-prop
> * Wlroots: https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/5384
>
> v6 -> v7:
> Core rework (feedback from Maxime Ripard):
> * Make the DRM backlight core backend-agnostic. Add a struct
> drm_backlight_funcs indirection so the backlight subsystem is just
> one backend; other backends (DDC/CI, MIPI-DCS, ...) can be added
> later without touching the core.
> * Stop dynamically allocating the DRM backlight. struct drm_backlight
> is now embedded in struct drm_connector and initialized by the
> core, so drivers no longer allocate it or handle allocation
> failure.
> * Drop the "drm/amd/display: Allow backlight registration to fail"
> patch; it is no longer needed now that nothing in the alloc path
> can fail.
> * Update the LUMINANCE property only through the atomic path. Remove
> the synchronous backlight writes from the property-set and legacy
> paths; the hardware is now touched solely from the atomic
> commit/enable path (via a workqueue, so slow backends never stall a
> commit).
> * Move the inlined luminance/DPMS logic into helpers and fix the
> comment style to match kernel conventions.
>
> Per-connector property (fixes a multi-panel bug):
> * Replace the single device-wide LUMINANCE property with a
> per-connector property created from the linked backend's range.
> Previously all connectors shared one property object, so linking a
> second panel corrupted the range reported for the others.
> * Never mutate the property min/max after creation (avoids racing
> GETPROPERTY). Add a kernel-internal drm_property.is_luminance flag
> to accept value 0 (DPMS off) without a device-wide pointer
> comparison.
>
> Kconfig (feedback from Thomas Zimmermann):
> * Drop "select BACKLIGHT_CLASS_DEVICE" from DRM. Add config
> DRM_BACKLIGHT which "depends on" BACKLIGHT_CLASS_DEVICE and
> provides no-op stubs when disabled, so DRM no longer forces the
> backlight subsystem into the kernel. Builds verified with the
> option both enabled and disabled.
>
> Documentation (feedback from Hans de Goede):
> * Document the LUMINANCE range table (1-N vs 0-N), that 1 is the
> minimum visible brightness, and that 0 means the display is turned
> off (with its vblank/pageflip implications).
> * Document BACKLIGHT_UPDATE_DRM.
>
> Misc:
> * backlight: fix a copy/paste kerneldoc on
> backlight_unregister_notifier and add a struct notifier_block
> forward declaration in backlight.h.
> * Split the old "drm: link connectors to backlight devices" patch
> into "drm/property: add a per-connector luminance flag" and "drm:
> add connector backlight (LUMINANCE) infrastructure", and rework the
> capability patch into "drm: add DRM_CLIENT_CAP_LUMINANCE".
> * Drop all Tested-by tags; the series has changed substantially and
> needs to be re-tested.
>
> Mario Limonciello (12):
> Revert "backlight: Remove notifier"
> backlight: add kernel-internal backlight API
> drm/property: add a per-connector luminance flag
> drm: add connector backlight (LUMINANCE) infrastructure
> drm: add DRM_CLIENT_CAP_LUMINANCE
> drm/amd/display: Pass up errors reading actual brightness
> drm/amd: Indicate driver supports luminance
> drm/amd/display: use drm backlight
> drm/bridge: auto-link panel backlight in bridge connector
> drm/xe: Indicate support for luminance on the connector
> drm/i915: Indicate support for luminance on the connector
> drm/i915/display: use drm backlight
>
> drivers/gpu/drm/Kconfig | 18 +
> drivers/gpu/drm/Makefile | 2 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 +
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +-
> .../display/amdgpu_dm/amdgpu_dm_backlight.c | 15 +-
> .../display/amdgpu_dm/amdgpu_dm_backlight.h | 2 +-
> .../display/amdgpu_dm/amdgpu_dm_connector.c | 2 +
> .../tests/amdgpu_dm_backlight_test.c | 4 +-
> drivers/gpu/drm/bridge/panel.c | 15 +
> .../gpu/drm/display/drm_bridge_connector.c | 11 +-
> drivers/gpu/drm/drm_atomic_helper.c | 39 ++
> drivers/gpu/drm/drm_atomic_uapi.c | 49 +-
> drivers/gpu/drm/drm_backlight.c | 515 ++++++++++++++++++
> drivers/gpu/drm/drm_connector.c | 56 ++
> drivers/gpu/drm/drm_drv.c | 8 +
> drivers/gpu/drm/drm_file.c | 5 +
> drivers/gpu/drm/drm_ioctl.c | 17 +
> drivers/gpu/drm/drm_mode_config.c | 1 +
> drivers/gpu/drm/drm_property.c | 6 +
> drivers/gpu/drm/drm_sysfs.c | 26 +-
> .../gpu/drm/i915/display/intel_backlight.c | 4 +
> drivers/gpu/drm/i915/display/intel_display.c | 7 +-
> drivers/gpu/drm/i915/display/intel_dp.c | 1 +
> drivers/gpu/drm/i915/i915_driver.c | 1 +
> drivers/gpu/drm/xe/xe_device.c | 3 +-
> drivers/video/backlight/backlight.c | 99 ++++
> include/drm/drm_atomic_helper.h | 2 +
> include/drm/drm_backlight.h | 158 ++++++
> include/drm/drm_bridge.h | 1 +
> include/drm/drm_connector.h | 20 +
> include/drm/drm_drv.h | 14 +
> include/drm/drm_file.h | 8 +
> include/drm/drm_property.h | 10 +
> include/linux/backlight.h | 63 +++
> include/uapi/drm/drm.h | 22 +
> 35 files changed, 1195 insertions(+), 15 deletions(-)
> create mode 100644 drivers/gpu/drm/drm_backlight.c
> create mode 100644 include/drm/drm_backlight.h
>
I don't think everyone else received it, but there was a good share of
Sashiko generated feedback from this and a lot of it looks valid.
Don't worry about reviewing v7 of the series. I'll take into account
the feedback, re-test and post a v8 when I'm done.
^ permalink raw reply [flat|nested] 23+ messages in thread