Re: [PATCH v6 04/10] DRM: Add support for client indicating support for luminance

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] drm_atomic_connector_set_property() synchronously modifies the physical hardware backlight, violating the DRM atomic TEST_ONLY contract.
- [High] Concurrent DRM_IOCTL_SET_CLIENT_CAP calls can leak luminance_clients references, breaking legacy sysfs backlight control.
- [High] Missing dynamic tracking of hotplugged/unplugged connectors causes unbalanced capability inhibit counters, triggering kernel WARN_ON panics.
- [High] drm_atomic_connector_commit_dpms() incorrectly modifies physical backlight state before atomic commit execution, failing to rollback on errors.
- [High] drm_backlight_link() calls a sleeping function (kobject_uevent_env) while holding a spinlock.
- [High] __drm_backlight_update_prop_range() mutates a device-wide shared property object, corrupting bounds for multi-monitor setups.
- [High] __drm_backlight_real_changed() mutates the committed atomic state directly, introducing data races and potential Use-After-Free.
- [High] A sleeping function (drm_backlight_set_luminance) is called from atomic encoder enable/disable callbacks.
- [Medium] drm_atomic_helper_commit_encoder_bridge_enable() accesses global connector->state instead of new_conn_state.
--

commit 8af77079789b2e380cebbf99913aa0db9fe6c3ef
Author: Mario Limonciello (AMD) <superm1@kernel.org>
Date:   Wed Jun 24 16:57:44 2026 +0000

    DRM: Add support for client indicating support for luminance

This commit introduces a DRM client capability to indicate support for a
luminance property on connectors. When the capability is enabled, the kernel
inhibits the legacy sysfs backlight control to allow the DRM client to
manage backlight brightness via atomic state.

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 51f39edc31edc..17492ff6c5579 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1224,6 +1225,8 @@ drm_atomic_helper_commit_encoder_bridge_disable(struct drm_device *dev,
>  		 * it away), so we won't call disable hooks twice.
>  		 */
>  		bridge = drm_bridge_chain_get_first_bridge(encoder);
> +		if (connector->backlight)
> +			drm_backlight_set_luminance(connector->backlight, 0);

[Severity: High]
Does this code call a sleeping function from atomic context?
During a DRM atomic commit, drm_atomic_helper_commit_encoder_bridge_disable()
executes in atomic context where sleeping is strictly prohibited. Calling
drm_backlight_set_luminance() from here will acquire a mutex (bd->ops_lock),
which could lead to system instability or lockdep warnings.

>  		drm_atomic_bridge_chain_disable(bridge, state);
>  		drm_bridge_put(bridge);
>  
> @@ -1738,6 +1741,10 @@ drm_atomic_helper_commit_encoder_bridge_enable(struct drm_device *dev, struct dr
>  
>  		drm_atomic_bridge_chain_enable(bridge, state);
>  		drm_bridge_put(bridge);
> +
> +		if (connector->backlight && connector->state)
> +			drm_backlight_set_luminance(connector->backlight,
> +						    connector->state->luminance);

[Severity: High]
Can this sleeping function safely be called from the atomic encoder enable
callback? Like the disable hook above, this runs in atomic context, but
drm_backlight_set_luminance() attempts to take a mutex.

[Severity: Medium]
Could accessing the global connector->state instead of new_conn_state cause
a race condition here?
If commit_tail runs asynchronously for non-blocking commits, a subsequent
commit could swap the state pointers, causing the hardware to be programmed
with an invalid or future brightness value.

>  	}
>  }
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 6441b55cc2740..ebf434bbd963b 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -935,6 +937,14 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		state->privacy_screen_sw_state = val;
>  	} else if (property == connector->broadcast_rgb_property) {
>  		state->hdmi.broadcast_rgb = val;
> +	} else if (property == config->luminance_property) {
> +		state->luminance = val;
> +		/* Update hardware backlight only when DPMS is ON.
> +		 * Property value is always updated to remember the user's
> +		 * desired brightness.
> +		 */
> +		if (connector->dpms == DRM_MODE_DPMS_ON)
> +			drm_backlight_set_luminance(connector->backlight, val);

[Severity: High]
Does this synchronously modify the physical hardware backlight?
Calling drm_backlight_set_luminance() directly in the atomic property
assignment phase appears to break the DRM atomic TEST_ONLY contract, where
TEST_ONLY or failed commits should guarantee no side effects on the hardware.

>  	} else if (connector->funcs->atomic_set_property) {
>  		return connector->funcs->atomic_set_property(connector,
>  				state, property, val);
> @@ -1126,9 +1163,29 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_commit *state,
>  	if (connector->dpms == mode)
>  		goto out;
>  
> +	crtc = connector->state ? connector->state->crtc : NULL;
> +
> +	/* Handle backlight brightness coordination with DPMS state changes */
> +	if (old_mode != DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_OFF) {
> +		/* DPMS ON -> OFF: dim all connectors driven by this CRTC. */
> +		if (crtc)
> +			drm_atomic_crtc_set_backlight(crtc, false);
> +		else
> +			drm_atomic_connector_set_backlight(connector, 0);
> +	}

[Severity: High]
Does this modify the physical backlight state before the atomic commit
is executed?
If the subsequent drm_atomic_commit() fails, it looks like the hardware
state won't be rolled back, leaving the physical backlight off but the
software state unchanged. 

Also, does dimming all connectors driven by this CRTC incorrectly affect
active displays in cloned setups when only one display was targeted by DPMS?

>  
>  	connector->dpms = mode;
> diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
> index b1ec470be86ca..2cddf209d5088 100644
> --- a/drivers/gpu/drm/drm_backlight.c
> +++ b/drivers/gpu/drm/drm_backlight.c
> @@ -71,6 +71,7 @@ static bool __drm_backlight_is_registered(struct drm_backlight *b)
>  /* caller must hold @drm_backlight_lock */
>  static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
>  {
> +	struct drm_connector *connector = b->connector;
>  	unsigned int max, set;
>  
>  	lockdep_assert_held(&drm_backlight_lock);
> @@ -85,6 +86,15 @@ static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v)
>  	set = v;
>  	if (set >= max)
>  		set = max;
> +
> +	/* Update the atomic state directly.
> +	 * For atomic drivers, the luminance value is stored in
> +	 * connector->state->luminance, not in the legacy property array.
> +	 * We update it unconditionally to reflect the hardware state,
> +	 * regardless of DPMS.
> +	 */
> +	if (connector->state)
> +		connector->state->luminance = set;

[Severity: High]
Could this directly mutate the committed atomic state, introducing data
races or a Use-After-Free?
A concurrent atomic commit might swap and free connector->state while this
function modifies connector->state->luminance holding only drm_backlight_lock
instead of the connection_mutex.

>  }
>  
>  /**
> @@ -100,18 +110,22 @@ static void __drm_backlight_update_prop_range(struct drm_backlight *b)
>  	struct drm_device *dev = b->connector->dev;
>  	struct drm_property *prop = dev->mode_config.luminance_property;
>  	unsigned int max = 0;
> +	bool can_disable = false;
>  
>  	lockdep_assert_held(&drm_backlight_lock);
>  
> -	if (b->link && b->link->props.max_brightness > 0)
> +	if (b->link && b->link->props.max_brightness > 0) {
>  		max = b->link->props.max_brightness;
> +		can_disable = b->link->props.can_disable;
> +	}
>  
>  	/* Update property range to match hardware capabilities.
>  	 * Range of 0-0 indicates no backing device.
> -	 * Range of 1-max for normal operation (0 reserved for display off).
> +	 * Range of 1-max for normal operation.
> +	 * Range of 0-max means that the display would turn off at 0
>  	 */
>  	if (prop->values[1] != max) {
> -		prop->values[0] = max ? 1 : 0;
> +		prop->values[0] = max ? (can_disable ? 0 : 1) : 0;
>  		prop->values[1] = max;
>  	}

[Severity: High]
Does this mutate a globally shared device-wide property based on a single
connector's backlight bounds?
Modifying dev->mode_config.luminance_property values directly could corrupt
the acceptable bounds for other displays in a multi-monitor setup where each
display has different max_brightness limits.

>  }
> @@ -228,11 +253,18 @@ EXPORT_SYMBOL(drm_backlight_unregister);
>   */
>  void drm_backlight_link(struct drm_backlight *b, struct backlight_device *bd)
>  {
> +	static const char * const ep[] = { "BACKLIGHT=1", NULL };
> +	bool send_uevent = false;
> +
>  	if (!b)
>  		return;
>  
>  	guard(spinlock)(&drm_backlight_lock);
> -	__drm_backlight_link(b, bd);
> +	send_uevent = __drm_backlight_link(b, bd);
> +
> +	if (send_uevent)
> +		kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE,
> +				   (char **)ep);

[Severity: High]
Can kobject_uevent_env() sleep while the drm_backlight_lock spinlock is held?
The guard(spinlock) macro holds the lock for the entire scope, and
kobject_uevent_env() dynamically allocates memory with GFP_KERNEL and may call
user-mode helpers, potentially causing a scheduling while atomic panic.

>  }
>  EXPORT_SYMBOL(drm_backlight_link);
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index ec820686b3021..4d2520de7614c 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -252,6 +253,10 @@ void drm_file_free(struct drm_file *file)
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		drm_fb_release(file);
>  		drm_property_destroy_user_blobs(dev, file);
> +		if (file->supports_luminance_control) {
> +			drm_backlight_uninhibit_legacy_all(dev);
> +			file->supports_luminance_control = false;
> +		}
>  	}

[Severity: High]
Could a lack of dynamic tracking for hotplugged or unplugged connectors
cause unbalanced inhibit counters?
If a display connector is hotplugged or unplugged after a DRM client sets
DRM_CLIENT_CAP_LUMINANCE, iterating over the current connector list on file
close will apply unmatched decrements or leak references, eventually triggering
the WARN_ON panic in drm_backlight_uninhibit_legacy().

> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index ff193155129e7..fdae36b13300a 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -380,6 +382,19 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  			return -EINVAL;
>  		file_priv->plane_color_pipeline = req->value;
>  		break;
> +	case DRM_CLIENT_CAP_LUMINANCE:
> +		if (!file_priv->atomic)
> +			return -EINVAL;
> +		if (req->value > 1)
> +			return -EINVAL;
> +		if (req->value == file_priv->supports_luminance_control)
> +			break;
> +		if (req->value)
> +			drm_backlight_inhibit_legacy_all(dev);
> +		else
> +			drm_backlight_uninhibit_legacy_all(dev);
> +		file_priv->supports_luminance_control = req->value;
> +		break;

[Severity: High]
Does this check-and-set logic lack locking?
Concurrent threads invoking DRM_IOCTL_SET_CLIENT_CAP with
DRM_CLIENT_CAP_LUMINANCE on the same file descriptor could bypass the check
and unbalancedly increment the inhibit counter, leaking references and
breaking legacy sysfs backlight control.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624165751.2014759-1-mario.limonciello@amd.com?part=4