Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/atomic: broken lease handling in drm_atomic_commit_fill_with_defaults() - [Medium] drm/atomic: missing color_mgmt_changed in drm_atomic_commit_fill_with_defaults() --
commit 5d2d0c35315849450f4b89a700b8964027903618 Author: Maxime Ripard <[email protected]> drm/atomic: Allow filling a commit with pristine object states This commit introduces drm_atomic_commit_fill_with_defaults() to populate an atomic commit with freshly created default states for CRTCs, planes, connectors, and color operations using their atomic_create_state() hooks. > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 7a77051096c91..c3c38695814fc 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1649,6 +1649,148 @@ bool drm_atomic_implements_create_state(struct > drm_device *dev) > } > EXPORT_SYMBOL(drm_atomic_implements_create_state); > [ ... ] > +int drm_atomic_commit_fill_with_defaults(struct drm_file *file_priv, > + struct drm_atomic_commit *commit) > +{ > + struct drm_device *dev = commit->dev; > + struct drm_mode_config *config = &dev->mode_config; > + struct drm_crtc *crtc; > + struct drm_plane *plane; > + struct drm_connector *connector; > + struct drm_connector_list_iter conn_iter; > + struct drm_colorop *colorop; > + int ret; > + > + drm_WARN_ON(dev, !commit->acquire_ctx); > + > + /* > + * Private objects are ignored because none have userspace > + * properties we might want to reset. atomic_check > + * implementations will derive or infer their private obj state > + * from the state that will end up being committed anyway. > + */ > + drm_for_each_colorop(colorop, dev) { > + struct drm_colorop_state *colorop_state; > + > + if (!drm_lease_held(file_priv, colorop->plane->base.id)) > + continue; > + > + ret = drm_modeset_lock(&colorop->plane->mutex, > commit->acquire_ctx); > + if (ret) > + return ret; > + > + colorop_state = drm_atomic_helper_colorop_create_state(colorop); > + if (IS_ERR(colorop_state)) > + return PTR_ERR(colorop_state); > + > + drm_atomic_commit_set_colorop_state(commit, colorop, > colorop_state); > + } > + > + drm_for_each_plane(plane, dev) { > + struct drm_plane_state *plane_state; > + > + if (!drm_lease_held(file_priv, plane->base.id)) > + continue; > + > + ret = drm_modeset_lock(&plane->mutex, commit->acquire_ctx); > + if (ret) > + return ret; > + > + 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); > + } > + > + drm_for_each_crtc(crtc, dev) { > + struct drm_crtc_state *crtc_state; > + > + if (!drm_lease_held(file_priv, crtc->base.id)) > + continue; > + > + ret = drm_modeset_lock(&crtc->mutex, commit->acquire_ctx); > + if (ret) > + return ret; > + > + 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); > + } [Severity: Medium] When drm_atomic_commit_fill_with_defaults() replaces the colorop, plane, and CRTC states with newly created default states, both crtc_state->color_mgmt_changed and plane_state->color_mgmt_changed are initialized to false, with crtc_state->{degamma_lut,ctm,gamma_lut} and plane_state->color_pipeline set to NULL and colorop_state set to defaults. In drivers/gpu/drm/drm_atomic_uapi.c, drm_atomic_crtc_set_property(), drm_atomic_plane_set_property(), drm_atomic_set_colorop_for_plane(), and drm_atomic_colorop_set_property() only set color_mgmt_changed when a property write changes the value already stored in the new 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; ... } drivers/gpu/drm/drm_atomic_uapi.c:drm_atomic_set_colorop_for_plane() { ... /* Color pipeline didn't change */ if (plane_state->color_pipeline == colorop) return false; ... } Neither drm_atomic_check_only() nor drm_atomic_helper_check_modeset() and drm_atomic_helper_check_planes() compares old_state against new_state to set color_mgmt_changed. If crtc->state previously had a non-NULL degamma_lut, ctm, or gamma_lut (or plane->state and colorop->state had an active color pipeline or non-default colorop settings), and a reset commit either omits those properties to leave them at defaults or explicitly sets them to 0 / default (which matches the already-pristine new_state), won't color_mgmt_changed remain false on both crtc_state and plane_state? When a reset commit keeps the same mode and routing on an active CRTC so drm_atomic_crtc_needs_modeset() is false, drivers that gate color updates on color_mgmt_changed || drm_atomic_crtc_needs_modeset() (such as amdgpu_dm, i915, msm/dpu1, tidss, and rockchip vop2) will skip reprogramming hardware LUTs, CTMs, and colorops. In addition, drivers that gate hardware LUT or CTM updates strictly on crtc_state->color_mgmt_changed (such as mtk_crtc_atomic_flush(), vc4_hvs_atomic_flush(), omap_crtc_atomic_flush(), malidp_atomic_commit_update_gamma(), malidp_atomic_commit_update_coloradj(), d71_improc_update(), and armada_drm_crtc_atomic_begin()) will skip resetting hardware color management even across a full modeset. Should drm_atomic_commit_fill_with_defaults() compare the pristine states against crtc->state, plane->state, and colorop->state and set color_mgmt_changed accordingly? > + > + drm_connector_list_iter_begin(dev, &conn_iter); > + drm_for_each_connector_iter(connector, &conn_iter) { > + struct drm_connector_state *connector_state; > + > + if (!drm_lease_held(file_priv, connector->base.id)) > + continue; > + > + ret = drm_modeset_lock(&config->connection_mutex, > commit->acquire_ctx); > + if (ret) { > + drm_connector_list_iter_end(&conn_iter); > + return ret; > + } > + > + /* > + * drm_atomic_implements_create_state() already checked > + * this, but connectors can be hotplugged between that > + * lockless check and the iteration here. > + */ > + if (!connector->funcs->atomic_create_state) { > + drm_connector_list_iter_end(&conn_iter); > + return -EOPNOTSUPP; > + } > + > + 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; > + } > + > + ret = drm_atomic_commit_set_connector_state(commit, connector, > connector_state); > + if (ret) { > + connector->funcs->atomic_destroy_state(connector, > connector_state); > + drm_connector_list_iter_end(&conn_iter); > + return ret; > + } > + } > + drm_connector_list_iter_end(&conn_iter); [Severity: High] Can this per-object lease filtering cause NULL pointer dereferences and broken atomic state when a lease is used or revoked? First, drm_atomic_commit_fill_with_defaults() calls drm_lease_held() in four separate loops (for colorops, planes, CRTCs, and connectors), taking and releasing dev->mode_config.idr_mutex on each call. Concurrent lease revocation via drm_mode_revoke_lease_ioctl() or drm_lease_revoke() only acquires dev->mode_config.idr_mutex (without any drm_modeset_lock) and clears master->leases. If a lease is revoked after the plane loop adds a leased plane (with old_plane_state->crtc != NULL and pristine new_plane_state->crtc == NULL) but before the CRTC loop checks old_plane_state->crtc, that CRTC is skipped and never added to the commit. During drm_atomic_commit(), drm_atomic_helper_plane_changed() hits WARN_ON(!crtc_state), drm_atomic_plane_check() and drm_atomic_helper_check_plane_state() succeed because new_plane_state->fb is NULL, and drm_atomic_helper_setup_commit() -> crtc_or_fake_commit() dereferences a NULL new_crtc_state: drivers/gpu/drm/drm_atomic_helper.c:crtc_or_fake_commit() { if (crtc) { struct drm_crtc_state *new_crtc_state; new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); return new_crtc_state->commit; } ... } Second, drm_mode_create_lease_ioctl() -> fill_object_idr() -> validate_lease() does not verify that leased planes or connectors are not currently bound to an unleased CRTC, nor does drm_lease_held() prevent the lessor (where master->lessor == NULL) from configuring cross-lease routing. Unlike drm_atomic_get_plane_state() and drm_atomic_get_connector_state(), which pull old_state->crtc into the commit via drm_atomic_get_crtc_state(), drm_atomic_commit_fill_with_defaults() resets leased planes and connectors to crtc = NULL while skipping unleased CRTCs. If old_connector_state->crtc is not in the commit, update_connector_routing() dereferences a NULL crtc_state: 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; } ... } Similarly, if old_plane_state->crtc is not in the commit, crtc_or_fake_commit() dereferences a NULL new_crtc_state. Third, when universal_planes is enabled, a lease on a CRTC does not have to include every plane (such as overlay or cursor planes) or cloned connector currently active on that CRTC. Resetting the leased CRTC to a pristine state (enable = false, plane_mask = 0, connector_mask = 0) while skipping the unleased plane or connector causes drm_atomic_helper_check_modeset() -> drm_atomic_add_affected_planes() to duplicate the unleased plane's active state (with crtc = leased_crtc and fb != NULL). That causes drm_atomic_helper_check_plane_state() to fail with -EINVAL if the CRTC remains disabled, or leaves new_crtc_state->plane_mask missing the unleased plane if the CRTC is re-enabled in the same commit. Meanwhile, drm_atomic_add_affected_connectors() inspects new_crtc_state->connector_mask (which is 0), failing to pull in the unleased connector and leaving connector->state->crtc and best_encoder dangling on the reset CRTC. > + > + return 0; > +} > +EXPORT_SYMBOL(drm_atomic_commit_fill_with_defaults); [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
