Re: [PATCH v10 3/9] drm/atomic: Allow filling a commit with pristine object states

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/atomic: lost driver state in
  drm_atomic_commit_fill_with_defaults()
- [Medium] drm/atomic: race with lease revoke in
  drm_atomic_commit_fill_with_defaults()
--

commit 5078d7ac25f2348a94f9d3158195c6d51b1f515d
Author: Maxime Ripard <mripard@kernel.org>

drm/atomic: Allow filling a commit with pristine object states

This commit adds drm_atomic_commit_fill_with_defaults() to populate an atomic
commit with pristine object states created via atomic_create_state() for all
leased CRTCs, planes, connectors, and colorops.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7a77051096c91..4fcce3bb0aa11 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1649,6 +1649,291 @@ bool drm_atomic_implements_create_state(struct drm_device *dev)

[ ... ]

> +int drm_atomic_commit_fill_with_defaults(struct drm_file *file_priv,
> +					 struct drm_atomic_commit *commit)
> +{

[ ... ]

> +	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;
> +
> +		/*
> +		 * The pristine state detaches the plane from the CRTC it
> +		 * is enabled on, which the atomic helpers can only sort
> +		 * out with that CRTC in the commit. Resetting a plane
> +		 * enabled on a CRTC that isn't leased would thus require
> +		 * touching an object the lease doesn't cover, so report it
> +		 * like any other access outside of the lease.
> +		 */
> +		if (plane->state->crtc &&
> +		    !drm_lease_held(file_priv, plane->state->crtc->base.id)) {
> +			drm_dbg_atomic(dev,
> +				       "[PLANE:%d:%s] is enabled on [CRTC:%d:%s], which isn't leased\n",
> +				       plane->base.id, plane->name,
> +				       plane->state->crtc->base.id,
> +				       plane->state->crtc->name);
> +			return -ENOENT;
> +		}
> +
> +		plane_state = plane->funcs->atomic_create_state(plane);
> +		if (IS_ERR(plane_state))
> +			return PTR_ERR(plane_state);
> +
> +		/*
> +		 * Same thing for the planes: report a color pipeline change
> +		 * compared to the current state.
> +		 */
> +		plane_state->color_mgmt_changed =
> +			plane->state->color_pipeline != plane_state->color_pipeline;
> +
> +		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;

[Severity: Medium]
Can a concurrent lease revocation between the plane, CRTC, and connector
loops leave a split pipeline in commit and cause a NULL pointer dereference?

drm_lease_held() takes and drops mode_config.idr_mutex on each call, while
drm_lease_revoke() clears master->leases under idr_mutex without holding any
modeset locks:

CPU0 (lessee atomic reset)        CPU1 (lessor revoke)
drm_atomic_commit_fill_with_defaults()
  drm_for_each_plane()
    drm_lease_held(plane) -> true
    drm_lease_held(plane->state->crtc) -> true
    drm_atomic_commit_set_plane_state()
      /* old->crtc = crtc, new->crtc = NULL */
                                  drm_mode_revoke_lease_ioctl()
                                    drm_lease_revoke()
                                      idr_destroy(&master->leases)
  drm_for_each_crtc()
    drm_lease_held(crtc) -> false (skipped)
  drm_for_each_connector_iter()
    drm_lease_held(conn) -> false
    drm_lease_held(conn->state->crtc) -> false
  return 0
...
drm_atomic_helper_setup_commit()
  crtc_or_fake_commit(state, old_plane_state->crtc)
    new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc)
    return new_crtc_state->commit;  <-- NULL pointer dereference

Similarly, if the lease is revoked between the CRTC loop and the connector
loop, a leased CRTC is added to commit with a disabled pristine state while
the connector bound to it is skipped because drm_lease_held() returns false
for both the connector and connector->state->crtc, leaving the connector
bound to a disabled CRTC.

> +		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_mode_copy(&crtc_state->adjusted_mode,
> +			      &crtc->state->adjusted_mode);

[ ... ]

> +		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;
> +		}
> +
> +		/*
> +		 * best_encoder is a routing information maintained by the
> +		 * atomic helpers rather than a userspace property, and it
> +		 * is only carried over to a new state when that state is a
> +		 * duplicate of the current one. Since
> +		 * update_connector_routing() uses it to detect the commits
> +		 * that keep the connector routing unchanged, carry the
> +		 * encoder currently in use over so that a reset on which
> +		 * userspace re-applies the exact same configuration doesn't
> +		 * require a modeset.
> +		 */
> +		connector_state->best_encoder = connector->state->best_encoder;

[Severity: High]
Does carrying over adjusted_mode and best_encoder onto pristine states created
via atomic_create_state() break drivers that rely on persistent driver-private
state when a commit does not trigger a modeset?

When userspace re-applies the active mode and connector routing on top of a
reset, mode_changed and connectors_changed remain false, so drivers skip
modeset recomputation even though atomic_create_state() zeroed their private
state subclass fields and drm_private_obj states were not reset.  For drivers
where drm_atomic_implements_create_state() returns true without dedicated
state carry-over handling:

- In msm/mdp5, mdp5_plane_create_state() zeroes mdp5_state->hwpipe, so
  mdp5_plane_atomic_check_with_state() never releases the previous hwpipe in
  mdp5_global_state->hwpipe.hwpipe_to_plane[], permanently leaking hardware
  pipes.  If a CRTC stays active with no visible planes,
  mdp5_crtc_atomic_check() returns early before mdp5_crtc_setup_pipeline(),
  leaving mdp5_cstate->pipeline.mixer NULL while mdp5_cstate->ctl is set and
  causing a NULL pointer dereference in blend_setup() during
  mdp5_crtc_atomic_flush():

drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c:blend_setup() {
    ...
	struct mdp5_hw_mixer *mixer = mdp5_cstate->pipeline.mixer;
	uint32_t lm = mixer->lm;
    ...
}

- In nouveau dispnv50, nv50_wndw_atomic_check_acquire() skips rebuilding
  asyw->image when the framebuffer is unchanged and no modeset is requested,
  so curs507a_acquire() sees a zeroed asyw->image.w and rejects cursor
  commits with -EINVAL, while primary/overlay planes lose asyw->ntfy.handle
  and asyw->xlut.handle.  nv50_head_atomic_check() also skips
  nv50_head_atomic_check_mode() when mode_changed and connectors_changed are
  false, leaving asyh->mode, asyh->view, and asyh->or zeroed, and resetting
  connector properties via nouveau_conn_atomic_create_state() leaves
  asyc->set.mask at 0 so scaler, dither, and procamp hardware state is not
  reprogrammed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-drm-reset-state-flag-v10-0-cefaa1ed056d@kernel.org?part=3