Re: [PATCH v3 1/7] drm/atomic: only add states of active or transient active colorops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Incomplete refactoring: drm_atomic_helper_duplicate_state still calls drm_atomic_add_affected_colorops.

Pre-existing issues:
- [High] Architectural Flaw: Colorops are omitted from the atomic state during normal plane updates.
--

commit a131c9a42c3beaf39f56794937e21266031060d8
Author: Melissa Wen <mwen@igalia.com>

drm/atomic: only add states of active or transient active colorops

Only consider affected colorop states those that are part of an active
color pipeline or a pipeline that is about to be activated or
deactivated in the same atomic commit.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 170de30c28ae4..0d36a955c4d43 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
[ ... ]
> @@ -1591,11 +1642,9 @@ drm_atomic_add_affected_planes(struct drm_atomic_commit *state,
>  		if (IS_ERR(plane_state))
>  			return PTR_ERR(plane_state);
>  
> -		if (plane_state->color_pipeline) {
> -			ret = drm_atomic_add_affected_colorops(state, plane);
> -			if (ret)
> -				return ret;
> -		}
> +		ret = drm_atomic_add_pipeline_colorops(state, plane);
> +		if (ret)
> +			return ret;

[Severity: High]
This isn't a bug introduced by this patch, but since
drm_atomic_add_affected_planes() executes exclusively during CRTC modesets,
does this mean active colorops are never added to the atomic state during a
simple plane update like a page flip?

Drivers that iterate over state->colorops via for_each_new_colorop_in_state()
(like intel_plane_color_copy_uapi_to_hw_state() in the intel driver)
will silently fail on page flips since the hardware state blobs remain
empty. Should the color pipeline dependency tracking be anchored to the
plane's inclusion in the state, perhaps within drm_atomic_get_plane_state()?

[Severity: Medium]
Also, the commit message states the goal is to remove the condition for
plane_state->color_pipeline to handle deactivations. Did this patch miss
updating drm_atomic_helper_duplicate_state() in drm_atomic_helper.c?

It still checks if (plane_state->color_pipeline) and calls the old
drm_atomic_add_affected_colorops(), which loops over all colorops belonging
to the plane:

drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_duplicate_state() {
	...
	if (plane_state->color_pipeline) {
		err = drm_atomic_add_affected_colorops(state, plane);
		if (err)
			goto free;
	}
	...
}

Should it be updated to use the newly introduced
drm_atomic_add_pipeline_colorops() unconditionally to ensure consistent state
duplication during suspend/resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609121230.1358786-1-mwen@igalia.com?part=1