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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing lock acquisition for `colorop->plane->mutex` before accessing and modifying colorop state.
- [Low] Grammar error in code comment ('there' instead of 'their').
--

--- Patch [8]: [PATCH RFC v2 8/9] drm/atomic: Allow filling a commit with pristine object states ---
commit be70167a2a5eebe655b84ef35f7fed5d15989a8f
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 pristine initial states for all display objects. It
uses atomic_create_state() hooks to provide a fresh default state for
reset operations.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index d8251447e44a5..d5ac10dd3148e 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)
> +{
[ ... ]
> +	/*
> +	 * Private objects are ignored because none have userspace
> +	 * properties we might want to reset. atomic_check
> +	 * implementations will derive or infer there private obj state
                                                 ^^^^^
[Severity: Low]
This isn't a bug, but there's a grammatical error here. Should "there" be
"their"?

> +	 * from the state that will end up being committed anyway.
> +	 */
> +	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);

[Severity: High]
Does this loop need to acquire the lock before asserting it is held?

Unlike the loops for planes, CRTCs, and connectors below which explicitly
call drm_modeset_lock(), this colorop loop calls
drm_modeset_lock_assert_held() on colorop->plane->mutex without acquiring
it first. 

Will this cause a lockdep assertion failure and leave colorop->state
unprotected against concurrent modifications?

> +		if (ret) {
> +			drm_colorop_atomic_destroy_state(colorop, colorop_state);
> +			return ret;
> +		}
> +	}
> +
> +	drm_for_each_plane(plane, dev) {
> +		struct drm_plane_state *plane_state;
> +
> +		ret = drm_modeset_lock(&plane->mutex, commit->acquire_ctx);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8