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

[Melissa Wen <mwen@igalia.com>]

On 21/05/2026 13:00, Borah, Chaitanya Kumar wrote:
>
>
> 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.

Oops, cc'ing everyone.

"
I wonder if userspace resetting colorops to disable a pipeline or 
configuring colorops before enabling the color pipeline is an expected 
behavior.

For resetting properties, I think I can solve it by taking into account 
old and new state to collect the active colorops, not only the new state.
But if configuring colorops before activate a color pipeline is 
expected, there is no need to have patches 1 and 2, since setting an 
inactive colorop have to be allowed. In that case, the solution is just 
drop both patches from the series.
"

Melissa

>
> 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) {
>