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
