Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] drm/atomic: DRM_MODE_ATOMIC_RESET with page-flip event fails on inactive CRTC - [High] drm/atomic: NULL crtc_state dereference on DRM_MODE_ATOMIC_RESET in DRM lease - [Medium] drm/ioctl: drm_getcap() reports DRM_CAP_ATOMIC_RESET on non-atomic drivers - [Medium] drm/atomic: DRM_MODE_ATOMIC_RESET clears best_encoder and forces full modeset - [Medium] drm/atomic: DRM_MODE_ATOMIC_RESET fails to set color_mgmt_changed on reset --
commit 421841f24ef240ac1eae965a30846086114bc3d7 Author: Sebastian Wick <[email protected]> drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag This commit introduces the DRM_MODE_ATOMIC_RESET flag for atomic commits and the DRM_CAP_ATOMIC_RESET capability so userspace can reset all KMS object states to their defaults before applying the properties in the request. > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c > b/drivers/gpu/drm/drm_atomic_uapi.c > index 1eebcf6f5e05b..9ff1ddecb8ede 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c [ ... ] > @@ -1688,6 +1695,17 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > fence_state = NULL; > num_fences = 0; > > + if (arg->flags & DRM_MODE_ATOMIC_RESET) { > + if (!drm_atomic_implements_create_state(dev)) { > + ret = -EOPNOTSUPP; > + goto out; > + } > + > + ret = drm_atomic_commit_fill_with_defaults(file_priv, state); > + if (ret) > + goto out; > + } [Severity: High] What happens when userspace combines DRM_MODE_ATOMIC_RESET with DRM_MODE_PAGE_FLIP_EVENT on a device where at least one crtc is currently off and remains off after the commit? In drivers/gpu/drm/drm_atomic_uapi.c, drm_mode_atomic_ioctl() calls drm_atomic_commit_fill_with_defaults(file_priv, state), which populates state->crtcs with a default drm_crtc_state (where enable and active are false) for every crtc on the device or lease. Any crtc not explicitly enabled by properties in the request remains in state->crtcs with new_crtc_state->active set to false. Later in drm_mode_atomic_ioctl(), prepare_signaling() iterates over every crtc in state and attaches a vblank event whenever DRM_MODE_PAGE_FLIP_EVENT is set: drivers/gpu/drm/drm_atomic_uapi.c:prepare_signaling() { ... for_each_new_crtc_in_state(state, crtc, crtc_state, i) { s32 __user *fence_ptr; fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc); if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) { struct drm_pending_vblank_event *e; e = create_vblank_event(crtc, arg->user_data); if (!e) return -ENOMEM; crtc_state->event = e; } ... } When drm_atomic_commit() or drm_atomic_nonblocking_commit() subsequently runs drm_atomic_check_only() -> drm_atomic_crtc_check() in drivers/gpu/drm/drm_atomic.c, any crtc that was off and remains off causes the entire commit to fail with -EINVAL: drivers/gpu/drm/drm_atomic.c:drm_atomic_crtc_check() { ... if (new_crtc_state->event && !new_crtc_state->active && !old_crtc_state->active) { drm_dbg_atomic(crtc->dev, "[CRTC:%d:%s] requesting event but off\n", crtc->base.id, crtc->name); return -EINVAL; } ... } This also means a preceding DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_RESET commit succeeds while the subsequent commit with DRM_MODE_PAGE_FLIP_EVENT fails. Should prepare_signaling() avoid attaching vblank events to inactive crtcs that were only added by drm_atomic_commit_fill_with_defaults()? [Severity: High] Can calling drm_atomic_commit_fill_with_defaults() from drm_mode_atomic_ioctl() trigger a NULL crtc_state dereference in update_connector_routing() when used by a drm lessee? In drivers/gpu/drm/drm_atomic.c, drm_atomic_commit_fill_with_defaults() filters crtcs, planes, and connectors independently via drm_lease_held(): drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() { ... drm_for_each_crtc(crtc, dev) { struct drm_crtc_state *crtc_state; if (!drm_lease_held(file_priv, crtc->base.id)) continue; ... drm_for_each_connector_iter(connector, &conn_iter) { struct drm_connector_state *connector_state; if (!drm_lease_held(file_priv, connector->base.id)) continue; ... } Unlike drm_atomic_get_connector_state(), which pulls the connector's previously bound crtc into the commit via drm_atomic_get_crtc_state(), drm_atomic_commit_fill_with_defaults() resets a leased connector to new_connector_state->crtc = NULL while skipping old_connector_state->crtc if that crtc is not held by the lease. When drm_atomic_check_only() -> drm_atomic_helper_check_modeset() -> update_connector_routing() runs in drivers/gpu/drm/drm_atomic_helper.c, drm_atomic_get_new_crtc_state(state, old_connector_state->crtc) returns NULL and is immediately dereferenced: drivers/gpu/drm/drm_atomic_helper.c:update_connector_routing() { ... if (old_connector_state->crtc != new_connector_state->crtc) { if (old_connector_state->crtc) { crtc_state = drm_atomic_get_new_crtc_state(state, old_connector_state->crtc); crtc_state->connectors_changed = true; } ... } Conversely, if a leased crtc currently has an unleased plane or connector bound to it, resetting the leased crtc zeroes its plane_mask, connector_mask, and encoder_mask without detaching the unleased object. [Severity: Medium] Will resetting connector state via atomic_create_state() in drm_atomic_commit_fill_with_defaults() force a full modeset even when an active connector remains bound to the same crtc and mode? In drivers/gpu/drm/drm_atomic.c, drm_atomic_commit_fill_with_defaults() creates each connector state via connector->funcs->atomic_create_state(), which leaves new_connector_state->best_encoder as NULL (whereas atomic_duplicate_state() preserves old_connector_state->best_encoder): drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() { ... connector_state = connector->funcs->atomic_create_state(connector); if (IS_ERR(connector_state)) { drm_connector_list_iter_end(&conn_iter); ret = PTR_ERR(connector_state); return ret; } ... } When userspace re-applies the same crtc to the connector in the reset commit, new_connector_state->best_encoder is still NULL when drm_atomic_helper_check_modeset() -> update_connector_routing() runs in drivers/gpu/drm/drm_atomic_helper.c: drivers/gpu/drm/drm_atomic_helper.c:update_connector_routing() { ... if (new_encoder == new_connector_state->best_encoder) { set_best_encoder(state, new_connector_state, new_encoder); ... return 0; } steal_encoder(state, new_encoder); set_best_encoder(state, new_connector_state, new_encoder); crtc_state->connectors_changed = true; ... } Because update_connector_routing() compares new_encoder against new_connector_state->best_encoder rather than old_connector_state->best_encoder, the check always fails and sets crtc_state->connectors_changed = true. Does this cause drm_atomic_crtc_needs_modeset() to return true unconditionally on active crtcs, rejecting reset commits that omit DRM_MODE_ATOMIC_ALLOW_MODESET with -EINVAL? [Severity: Medium] When DRM_MODE_ATOMIC_RESET clears previously active crtc color management blobs (degamma_lut, ctm, gamma_lut) or a plane color_pipeline, how does color_mgmt_changed get set on the new state? In drivers/gpu/drm/drm_atomic.c, drm_atomic_commit_fill_with_defaults() allocates fresh plane and crtc states via atomic_create_state(), which initializes degamma_lut, ctm, gamma_lut, and color_pipeline to NULL and color_mgmt_changed to false: drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() { ... plane_state = plane->funcs->atomic_create_state(plane); if (IS_ERR(plane_state)) return PTR_ERR(plane_state); drm_atomic_commit_set_plane_state(commit, plane, plane_state); ... crtc_state = crtc->funcs->atomic_create_state(crtc); if (IS_ERR(crtc_state)) return PTR_ERR(crtc_state); drm_atomic_commit_set_crtc_state(commit, crtc, crtc_state); ... } In drivers/gpu/drm/drm_atomic_uapi.c, drm_atomic_crtc_set_property() and drm_atomic_plane_set_property() only update color_mgmt_changed when a property change modifies the in-progress state: drivers/gpu/drm/drm_atomic_uapi.c:drm_atomic_crtc_set_property() { ... ret = drm_property_replace_blob_from_id(dev, &state->degamma_lut, val, elem_size * lut_size, -1, elem_size, &replaced); state->color_mgmt_changed |= replaced; return ret; ... } drivers/gpu/drm/drm_atomic_uapi.c:drm_atomic_plane_set_property() { ... state->color_mgmt_changed |= drm_atomic_set_colorop_for_plane(state, colorop); ... } If an active crtc or plane had a non-NULL lut, ctm, or color_pipeline in old_crtc_state or old_plane_state and the reset commit leaves those properties at their default NULL values (or explicitly writes 0), color_mgmt_changed remains false. Won't drivers that gate hardware lut, ctm, or color pipeline updates on color_mgmt_changed skip reprogramming the hardware and leave stale color transforms active? > for (i = 0; i < arg->count_objs; i++) { > uint32_t obj_id, count_props; > struct drm_mode_object *obj; > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 0dbf04d4aa9e0..e589d93871e27 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -305,6 +305,9 @@ static int drm_getcap(struct drm_device *dev, void *data, > struct drm_file *file_ > req->value = drm_core_check_feature(dev, DRIVER_ATOMIC) && > dev->mode_config.async_page_flip; > break; > + case DRM_CAP_ATOMIC_RESET: > + req->value = drm_atomic_implements_create_state(dev); > + break; > default: > return -EINVAL; > } [Severity: Medium] Should DRM_CAP_ATOMIC_RESET in drm_getcap() also check drm_core_check_feature(dev, DRIVER_ATOMIC) like DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP does right above it? In drivers/gpu/drm/drm_ioctl.c, this switch in drm_getcap() only checks drm_core_check_feature(dev, DRIVER_MODESET), and drm_atomic_implements_create_state() in drivers/gpu/drm/drm_atomic.c only checks whether objects in plane_list, crtc_list, and connector_list have atomic_create_state hooks. On KMS drivers where DRIVER_ATOMIC is not set (for example, when a driver clears DRIVER_ATOMIC dynamically or when the mode object lists are empty), drm_getcap() will report DRM_CAP_ATOMIC_RESET = 1 even though DRM_IOCTL_MODE_ATOMIC fails with -EOPNOTSUPP. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
