Re: [PATCH v6 2/6] drm/atomic: reject colorop update from inactive color pipeline

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 5/20/2026 2:39 AM, Melissa Wen wrote:
> Only allow updates on colorops that are part of an active pipeline.
> Check if a colorop in a new state belongs to a color pipeline which was
> set as a plane color_pipeline property and therefore is an active color
> pipeline. If not, reject the atomic state. Performing this check later
> in drm_atomic_check_only() to remove the ordering dependency that would
> exist if done at the time of colorop property setting.
> 
> Suggested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   drivers/gpu/drm/drm_atomic.c | 38 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 28831a548b0c..659cf56150e5 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -812,6 +812,33 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
>   	return 0;
>   }
>   
> +/**
> + * drm_atomic_colorop_check - check new colorop state
> + * @new_colorop_state: new colorop state to check
> + *
> + * Ensure that the colorop in @new_colorop_state belongs to an active color
> + * pipeline, i.e. it's in the chain of colorops set to the color_pipeline
> + * property of a plane state.
> + *
> + * Returns: 0 on success, -EINVAL otherwise.
> + */
> +static int drm_atomic_colorop_check(const struct drm_colorop_state *new_colorop_state)
> +{
> +	struct drm_colorop *colorop, *color_pipeline;
> +	struct drm_plane_state *new_plane_state;
> +
> +	new_plane_state = drm_atomic_get_new_plane_state(new_colorop_state->state,
> +							 new_colorop_state->colorop->plane);
> +	color_pipeline = new_plane_state ? new_plane_state->color_pipeline :
> +			 new_colorop_state->colorop->plane->state->color_pipeline;
> +
> +	for (colorop = color_pipeline; colorop; colorop = colorop->next)
> +		if (colorop == new_colorop_state->colorop)
> +			return 0;
> +
> +	return -EINVAL;
> +}
> +

This causes regression in our CI[1].

I looked into it and looks like the following sequence in 
igt@kms_color_pipeline causes the error

         set_color_pipeline_bypass(plane);
         reset_colorops(colorops);
         igt_plane_set_fb(plane, NULL);
         igt_display_commit_atomic(&data->display, 0, NULL);

So this change restricts bypassing/disabling both the pipeline and a 
colorop within it in a single commit.

Also Sashiko had the following to say

"Furthermore, does this unnecessarily restrict UAPI by preventing userspace
from configuring inactive pipelines before enabling them, or from resetting
properties on a pipeline in the same commit that switches away from it?"

So this will also fail a commit which tries to change a pipeline and 
disable the colorops in an old pipeline.

That got me thinking whether the first patch[3] in the series is also 
correct, since it is quite similar to the change[4] I added, where 
colorops are only added to the state when a pipeline is active. In both 
cases, we could end up ignoring colorops that are not part of the 
currently selected pipeline.

[1] 
https://intel-gfx-ci.01.org/tree/intel-xe/xe-pw-166922v1/shard-lnl-5/igt@kms_color_pipeline@plane-ctm3x4@pipe-a-plane-2.html
[2] 
https://sashiko.dev/#/patchset/20260520073827.3395745-3-chaitanya.kumar.borah%40intel.com
[3] 
https://lore.kernel.org/dri-devel/20260519211111.228303-2-mwen@igalia.com/
[4] 
https://lore.kernel.org/dri-devel/148df44d-2456-40e3-8be6-f98b89b7ee4d@amd.com/

P.S. Can you please send the next version to intel-gfx and intel-xe too?

==
Chaitanya
>   static void drm_atomic_colorop_print_state(struct drm_printer *p,
>   					   const struct drm_colorop_state *state)
>   {
> @@ -1665,6 +1692,8 @@ int drm_atomic_check_only(struct drm_atomic_commit *state)
>   	struct drm_plane *plane;
>   	struct drm_plane_state *old_plane_state;
>   	struct drm_plane_state *new_plane_state;
> +	struct drm_colorop *colorop;
> +	struct drm_colorop_state *new_colorop_state;
>   	struct drm_crtc *crtc;
>   	struct drm_crtc_state *old_crtc_state;
>   	struct drm_crtc_state *new_crtc_state;
> @@ -1681,6 +1710,15 @@ int drm_atomic_check_only(struct drm_atomic_commit *state)
>   			requested_crtc |= drm_crtc_mask(crtc);
>   	}
>   
> +	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
> +		ret = drm_atomic_colorop_check(new_colorop_state);
> +		if (ret) {
> +			drm_dbg_atomic(dev, "[COLOROP:%d:%d] is not part of an active color pipeline.\n",
> +				       colorop->base.id, colorop->type);
> +			return ret;
> +		}
> +	}
> +
>   	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>   		ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
>   		if (ret) {