Re: [PATCH RFC 7/8] drm/atomic: Allow filling a commit with pristine object states

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Unchecked calls to the `atomic_create_state` hook on CRTCs, planes, and connectors will cause a kernel panic on drivers that do not implement them.
- [High] The function fails to reset private objects (`drm_private_obj`), resulting in an incomplete and inconsistent baseline state.
- [Medium] The locking order for `connection_mutex` in `drm_atomic_commit_fill_with_defaults` is inverted relative to the established DRM locking order.
--

--- Patch [7]: [PATCH RFC 7/8] drm/atomic: Allow filling a commit with pristine object states ---
commit e9e433a7ce1a444bdbe24747a11cf419bea31b2b
Author: Maxime Ripard <mripard@kernel.org>

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

This patch creates drm_atomic_commit_fill_with_defaults() to iterate over
all CRTCs, planes, connectors, and color operations in the device and insert
a fresh default state for each one. This brings the device back to a known
baseline before applying userspace property changes.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 79971d4b56a49..0c1a02d0e871d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c

[ ... ]

> @@ -1610,6 +1610,118 @@ drm_atomic_get_new_bridge_state(const struct drm_atomic_commit *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
>  
> +/**
> + * drm_atomic_commit_fill_with_defaults - populate a commit with pristine states

[ ... ]

> +int drm_atomic_commit_fill_with_defaults(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;
> +
> +	WARN_ON(!commit->acquire_ctx);
> +
> +	drm_for_each_crtc(crtc, dev) {
> +		struct drm_crtc_state *crtc_state;
> +
> +		ret = drm_modeset_lock(&crtc->mutex, commit->acquire_ctx);
> +		if (ret)
> +			return ret;
> +
> +		crtc_state = crtc->funcs->atomic_create_state(crtc);

[Severity: Critical]
Is it safe to call atomic_create_state without checking if the function pointer
exists?

Some drivers (like i915) might rely on the reset callback instead and leave
atomic_create_state unimplemented. Could this unconditional call lead to a NULL
pointer dereference here, as well as in the plane and connector loops below?

> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +
> +		ret = drm_atomic_commit_set_crtc_state(commit, crtc, crtc_state);
> +		if (ret) {
> +			crtc->funcs->atomic_destroy_state(crtc, crtc_state);
> +			return ret;
> +		}
> +	}

[ ... ]

> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		struct drm_connector_state *connector_state;
> +
> +		ret = drm_modeset_lock(&config->connection_mutex, commit->acquire_ctx);

[Severity: Medium]
Does acquiring connection_mutex here invert the standard DRM locking order?

The established canonical sequence (such as in drm_modeset_lock_all_ctx)
typically acquires connection_mutex before the CRTC and plane locks. Since the
CRTC and plane locks were already acquired in the loops above, could acquiring
connection_mutex now result in unnecessary transaction backoffs and retries?

> +		if (ret) {
> +			drm_connector_list_iter_end(&conn_iter);
> +			return ret;
> +		}
> +
> +		connector_state = connector->funcs->atomic_create_state(connector);
> +		if (IS_ERR(connector_state)) {

[ ... ]

> +	drm_for_each_colorop(colorop, dev) {
> +		struct drm_colorop_state *colorop_state;
> +
> +		colorop_state = drm_atomic_helper_colorop_create_state(colorop);
> +		if (IS_ERR(colorop_state))
> +			return PTR_ERR(colorop_state);
> +
> +		drm_modeset_lock_assert_held(&colorop->plane->mutex);
> +
> +		ret = drm_atomic_commit_set_colorop_state(commit, colorop, colorop_state);
> +		if (ret) {
> +			drm_colorop_atomic_destroy_state(colorop, colorop_state);
> +			return ret;
> +		}
> +	}
> +

[Severity: High]
Should this function also iterate over and reset private objects
(drm_private_obj) before returning?

Modern DRM features like DP MST topologies, DisplayPort tunnels, and global
bandwidth allocators maintain their state in private objects. If these are
omitted from the reset, could it leave the topology or bandwidth state
inconsistent with the newly reset hardware baseline?

> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_commit_fill_with_defaults);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-drm-reset-state-flag-v1-0-c37dc985485d@kernel.org?part=7