Re: [PATCH v8 4/7] 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: inconsistent cross-lease state in
  drm_atomic_commit_fill_with_defaults()
- [High] drm/atomic: missing driver-private active state in
  drm_atomic_commit_fill_with_defaults()
--

commit b5521ba27187cadf5fe15d5963f98a6825dc5a6a
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 CRTCs, planes, connectors, and colorops held by a DRM lease.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7a77051096c91..8bda2871f394b 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1649,6 +1649,361 @@ bool drm_atomic_implements_create_state(struct drm_device *dev)
[ ... ]
> +static int drm_atomic_commit_fill_connector_with_defaults(struct drm_file *file_priv,
> +							  struct drm_atomic_commit *commit,
> +							  struct drm_connector *connector)
> +{
> +	struct drm_connector_state *connector_state;
> +	struct drm_crtc_state *crtc_state;
> +	int ret;
> +
> +	if (drm_lease_held(file_priv, connector->base.id)) {
[ ... ]
> +		if (!connector->state->crtc)
> +			return 0;
> +
> +		/*
> +		 * Make sure the CRTC the connector is currently bound to
> +		 * is part of the commit. The atomic helpers need that
> +		 * state: set_best_encoder() clears the encoder mask of
> +		 * that CRTC when the connector gets detached from it.
> +		 */
> +		crtc_state = drm_atomic_get_crtc_state(commit,
> +						       connector->state->crtc);
> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +
> +		/*
> +		 * The pristine state unbinds the connector from that
> +		 * CRTC, so drop it from the connector mask like
> +		 * drm_atomic_set_crtc_for_connector() would.
> +		 */
> +		crtc_state->connector_mask &= ~drm_connector_mask(connector);
> +	} else {
> +		/*
> +		 * The connector isn't part of the lease so it keeps its
> +		 * current state. If it is bound to a leased CRTC though,
> +		 * that CRTC is being reset under it and comes with an
> +		 * empty connector mask, so
> +		 * drm_atomic_add_affected_connectors() won't find the
> +		 * connector during the check. Pull it in here so that its
> +		 * routing and encoder are properly handled.
> +		 *
> +		 * The leased CRTCs are all part of the commit by now. An
> +		 * unleased one might not be yet, but it keeps its
> +		 * connector mask, so the helpers will pull the connector
> +		 * in on their own if that CRTC ends up being changed.
> +		 */
> +		if (!connector->state->crtc)
> +			return 0;
> +
> +		if (!drm_atomic_get_new_crtc_state(commit,
> +						   connector->state->crtc))
> +			return 0;
> +
> +		connector_state = drm_atomic_get_connector_state(commit,
> +								 connector);
> +		if (IS_ERR(connector_state))
> +			return PTR_ERR(connector_state);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Can cross-lease routing between leased and unleased objects leave
crtc_state->connector_mask, crtc_state->plane_mask, and crtc_state->enable
out of sync with the connector and plane states?

When an unleased connector is currently bound to a leased CRTC, the leased
CRTC gets a pristine state with enable = false and connector_mask = 0,
while the else branch here duplicates connector->state and leaves
connector_state->crtc pointing to that CRTC and best_encoder set.
update_connector_routing() only detaches a connector when
new_connector_state->crtc is cleared, so the unleased connector stays
attached to the reset CRTC with connector_mask = 0, causing
drm_atomic_helper_check_modeset() to validate or commit a zeroed mode on a
disabled CRTC or fail conflicting-encoder checks.

Conversely, when a leased connector is detached from an unleased CRTC in
the if branch and was its last connector, clearing connector_mask leaves
crtc_state->enable set to true with connector_mask = 0, which
drm_atomic_helper_check_modeset() rejects because enable != has_connectors
(unlike handle_conflicting_encoders(), which clears the mode and sets
active = false when connector_mask reaches 0).

Similarly, in drm_atomic_commit_fill_with_defaults(), the plane loop skips
unleased planes even when plane->state->crtc points to a leased CRTC being
reset.  When drm_atomic_add_affected_planes() later pulls that plane in
with its duplicated active state, drm_atomic_helper_check_plane_state()
rejects the commit because an enabled plane remains attached to a disabled
CRTC, or leaves new_crtc_state->plane_mask missing the plane if the CRTC is
re-enabled.

[ ... ]

> +int drm_atomic_commit_fill_with_defaults(struct drm_file *file_priv,
> +					 struct drm_atomic_commit *commit)
> +{
[ ... ]
> +	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_mode_copy(&crtc_state->adjusted_mode,
> +			      &crtc->state->adjusted_mode);
> +
> +		drm_atomic_commit_set_crtc_state(commit, crtc, crtc_state);
> +	}
> +
> +	drm_for_each_plane(plane, dev) {
> +		struct drm_plane_state *plane_state;
> +
> +		if (!drm_lease_held(file_priv, plane->base.id))
> +			continue;
[ ... ]
> +		plane_state = plane->funcs->atomic_create_state(plane);
> +		if (IS_ERR(plane_state))
> +			return PTR_ERR(plane_state);

[Severity: High]
Does building new states with atomic_create_state() while carrying over
best_encoder and adjusted_mode discard driver-private state and reference
counts that drivers expect to be preserved from old_state?

Drivers that implement atomic_create_state() initialize driver-subclassed
fields to an unallocated/disabled baseline there, and rely on
atomic_duplicate_state() to copy active hardware assignments and retain
references while the object is currently enabled.  Because
drm_atomic_commit_fill_connector_with_defaults() and
drm_atomic_commit_fill_with_defaults() also preserve best_encoder and
adjusted_mode to avoid a modeset when userspace re-applies the active
configuration, atomic_check sees old_crtc_state->enable ==
new_crtc_state->enable and drm_atomic_crtc_needs_modeset() == false on a
freshly created driver state:

- In vc4, vc4_crtc_create_state() sets assigned_channel to
  VC4_HVS_CHANNEL_DISABLED (-1U).  When the CRTC stays enabled,
  vc4_pv_muxing_atomic_check() skips channel assignment and
  vc4_core_clock_atomic_check() indexes hvs_new_state->fifo_state[channel]
  with -1U, writing out of bounds.

- In amdgpu_dm, amdgpu_dm_crtc_create_state() and
  amdgpu_dm_plane_drm_plane_create_state() leave dm_new_crtc_state->stream
  and dm_new_plane_state->dc_state NULL instead of retaining the references
  from old_state.  If the CRTC stays enabled without a modeset,
  dm_update_crtc_state() hits a bug check on dm_new_crtc_state->stream ==
  NULL; if the reset disables an active CRTC or plane,
  dm_update_crtc_state() and dm_update_plane_state() release
  dm_old_crtc_state->stream and dm_old_plane_state->dc_state expecting an
  extra reference from duplicate_state(), causing a double release and
  use-after-free when old_state is destroyed.

- In msm/dpu1, dpu_crtc_create_state() zeroes num_mixers, and
  dpu_crtc_atomic_check() skips dpu_crtc_assign_resources() when
  !mode_changed && !connectors_changed && !color_mgmt_changed, leaving the
  enabled CRTC with no hardware mixers assigned.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-drm-reset-state-flag-v8-0-50fd046b705f@kernel.org?part=4