Re: [PATCH v4 10/13] 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 issue(s) to consider:
- [High] drm/atomic: deadlock on NULL acquire_ctx in drm_atomic_commit_fill_with_defaults
- [Medium] drm/atomic: lock inversion in drm_atomic_commit_fill_with_defaults
--

commit 955de6594c45c6a520389d6b2f5a51cb5ce03d61
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 iterate over all
CRTCs, planes, connectors, and color operations in the device. It inserts a
fresh default state for each one into the atomic commit. This is intended to
be used with the upcoming DRM_MODE_ATOMIC_RESET flag to bring the device
back to a known baseline.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7a77051096c91..b039edaa3271f 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c

[ ... ]

> +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;
> +
> +	drm_WARN_ON(dev, !commit->acquire_ctx);
> +
> +	/*
> +	 * Private objects are ignored because none have userspace
> +	 * properties we might want to reset. atomic_check

[ ... ]

> +	drm_for_each_plane(plane, dev) {
> +		struct drm_plane_state *plane_state;
> +
> +		ret = drm_modeset_lock(&plane->mutex, commit->acquire_ctx);

[Severity: Medium]
Does the locking order starting here create a lock inversion against standard
paths?

The standard DRM top-down locking hierarchy typically locks the
connection_mutex first, then CRTCs, and finally planes (as seen in
drm_modeset_lock_all_ctx).

However, drm_atomic_commit_fill_with_defaults() iterates and locks objects
in the reverse order (planes -> CRTCs -> connection_mutex). If userspace
invokes the DRM_MODE_ATOMIC_RESET flag while concurrent atomic operations
are running, could this inefficient locking cause ww_mutex to abort
transactions with -EDEADLK and retry, leading to unnecessary contention and
performance penalties?

[ ... ]

> +	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: High]
Can this loop cause a hard deadlock if commit->acquire_ctx is NULL?

Earlier in the function, drm_WARN_ON(dev, !commit->acquire_ctx) detects a
missing acquire context, but execution is allowed to continue. When we reach
this loop, drm_modeset_lock() is called for every connector.

If commit->acquire_ctx is NULL, the first iteration will successfully acquire
the underlying mutex, but the second iteration will attempt to recursively
lock the same connection_mutex without a ww_mutex context and block
indefinitely. Should the drm_WARN_ON() above be accompanied by an early
return to prevent this unrecoverable multi-connector deadlock?

> +		if (ret) {
> +			drm_connector_list_iter_end(&conn_iter);
> +			return ret;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-drm-reset-state-flag-v4-0-5ad106370f05@kernel.org?part=10