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

On 26/06/2026 00:38, John Harrison wrote:
> On 6/9/26 13:51, Melissa Wen wrote:
>> Only allow updates on colorops that are part of an active pipeline, i.e.
>> check if a colorop belongs to the color pipeline of a plane in its
>> current, new or old state. If not, reject the state change of this
>> inactive colorop. 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. Userspace is allowed to change colorops of
>> an active color pipeline, or when activating or deactivating its
>> pipeline in the same commit. However, changes in inactive color pipeline
>> is not allowed.
> The last two sentences here seem to be a duplicate of the first two 
> sentences. Maybe drop them as redundant?

Sure, I think I can explain it better.
I was trying to say that we are changing this behavior on the kernel 
side to align with userspace expectations.

>
>>
>> Suggested-by: Chaitanya Kumar Borah <[email protected]>
>> Signed-off-by: Melissa Wen <[email protected]>
>> ---
>>   drivers/gpu/drm/drm_atomic.c | 60 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 60 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 464562861408..960b52624deb 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -943,6 +943,55 @@ drm_atomic_add_pipeline_colorops(struct 
>> drm_atomic_commit *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 current, old or new plane state.
>> + *
>> + * Returns: 0 on success, -EINVAL otherwise.
>> + */
>> +static int drm_atomic_colorop_check(const struct drm_colorop_state 
>> *new_colorop_state)
>> +{
>> +    struct drm_atomic_commit *state = new_colorop_state->state;
>> +    struct drm_plane *plane = new_colorop_state->colorop->plane;
>> +    struct drm_plane_state *new_plane_state, *old_plane_state;
>> +    struct drm_colorop *colorop;
>> +
>> +    new_plane_state = drm_atomic_get_new_plane_state(state, plane);
>> +    old_plane_state = drm_atomic_get_old_plane_state(state, plane);
>> +
>> +    /* No changes in the plane state. Check current-committed plane 
>> state */
>> +    if (!new_plane_state) {
>> +        for (colorop = plane->state->color_pipeline; colorop; 
>> colorop = colorop->next)
> Doesn't the first patch add a macro for wrapping this for loop?

Yes, thanks for pointing out. I'll replace all occurrences.

>
>> +            if (colorop == new_colorop_state->colorop)
>> +                return 0;
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (WARN_ON(!old_plane_state))
>> +        return -EINVAL;
>> +
>> +    /* Check if the colorop is active in the new plane state */
>> +    for (colorop = new_plane_state->color_pipeline; colorop; colorop 
>> = colorop->next)
>> +        if (colorop == new_colorop_state->colorop)
>> +            return 0;
>> +
>> +    /* Same color pipeline as new; no point walking old. Colorop 
>> isn't active */
>> +    if (new_plane_state->color_pipeline == 
>> old_plane_state->color_pipeline)
>> +        return -EINVAL;
>> +
>> +    /* Check if the colorop was active in the old plane state */
>> +    for (colorop = old_plane_state->color_pipeline; colorop; colorop 
>> = colorop->next)
>> +        if (colorop == new_colorop_state->colorop)
>> +            return 0;
> Doesn't this situation count as changing the properties of an inactive 
> colorop? And should therefore be rejected? The colorop was previously 
> in use but once the commit goes through, it will not be. So any 
> changes to its state will not actually affect the new post-commit 
> universe. Or am I missing something?

We still need it because the userspace is still allowed to change 
colorops of a pipeline A that will be disable in the same commit. I 
mean, they can change colorop settings of an active pipeline A and set 
COLOR_PIPELINE to Bypass in the same commit, and this change have to 
persist for the time this pipeline A will be reactivated, even if in 
practice those colorop changes will not be programmed in the hw because 
of deactivation. I'll add this comment to explain this contract better:

+ * Userspace is allowed to finalize colorop's settings in the same 
commit that
+ * deactivates its pipeline, and the changes should persist for when that
+ * pipeline is reactivated later. So that, changes to colorop in the 
old plane
+ * state's pipeline is accepted even though it won't drive hardware 
updates.

WDYT?

Melissa

>
> John.
>
>> +
>> +    /* Colorop is not part of an active color pipeline. */
>> +    return -EINVAL;
>> +}
>> +
>>   static void drm_atomic_colorop_print_state(struct drm_printer *p,
>>                          const struct drm_colorop_state *state)
>>   {
>> @@ -1792,6 +1841,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;
>> @@ -1808,6 +1859,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] isn't in 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) {
>