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) <[email protected]> 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/[email protected]?part=4
