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

On 26/06/2026 00:28, John Harrison wrote:
> On 6/9/26 13:51, Melissa Wen wrote:
>> 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, i.e., colorop is in the chain of
>> old/new plane color pipeline property. To cover color_pipeline
>> deactivation, remove the condition for plane_state->color_pipeline.
>>
>> Signed-off-by: Melissa Wen <[email protected]>
>> ---
>>
>> v2: define a macro to walk in the color pipeline (Alex H.)
>> ---
>>   drivers/gpu/drm/drm_atomic.c | 65 +++++++++++++++++++++++++++++++-----
>>   include/drm/drm_colorop.h    |  3 ++
>>   2 files changed, 59 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 3af1b9cc9a06..464562861408 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -892,6 +892,57 @@ static int drm_atomic_plane_check(const struct 
>> drm_plane_state *old_plane_state,
>>       return 0;
>>   }
>>   +/*
>> + * This function walks old and new plane state color pipelines and 
>> adds all
>> + * colorops in use by @plane to the atomic configuration @state. 
>> This is useful
>> + * when an atomic commit needs to check all currently enabled or 
>> about to be
>> + * enabled colorop on @plane, e.g. when changing the mode. This also 
>> avoids
>> + * including colorop states that are not part of the atomic state.
>> + *
>> + * Returns:
>> + * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error 
>> is EDEADLK
>> + * then the w/w mutex code has detected a deadlock and the entire 
>> atomic
>> + * sequence must be restarted. All other errors are fatal.
>> + */
>> +static int
>> +drm_atomic_add_pipeline_colorops(struct drm_atomic_commit *state,
>> +                 struct drm_plane *plane)
>> +{
>> +    struct drm_colorop *colorop;
>> +    struct drm_colorop_state *colorop_state;
>> +    struct drm_plane_state *new_plane_state, *old_plane_state;
>> +
>> +    new_plane_state = drm_atomic_get_new_plane_state(state, plane);
>> +    old_plane_state = drm_atomic_get_old_plane_state(state, plane);
>> +
>> +    if (WARN_ON(!new_plane_state || !old_plane_state))
>> +        return -EINVAL;
>> +
>> +    drm_dbg_atomic(plane->dev,
>> +               "Adding old+new pipeline colorops for [PLANE:%d:%s]\n",
>> +               plane->base.id, plane->name);
>> +
>> +    drm_for_each_colorop_in_pipeline(colorop,
>> +                     new_plane_state->color_pipeline) {
>> +        colorop_state = drm_atomic_get_colorop_state(state, colorop);
>> +        if (IS_ERR(colorop_state))
>> +            return PTR_ERR(colorop_state);
>> +    }
>> +
>> +    /* Same color pipeline as new; no point walking old. */
>> +    if (new_plane_state->color_pipeline == 
>> old_plane_state->color_pipeline)
>> +        return 0;
>> +
>> +    drm_for_each_colorop_in_pipeline(colorop,
>> +                     old_plane_state->color_pipeline) {
>> +        colorop_state = drm_atomic_get_colorop_state(state, colorop);
>> +        if (IS_ERR(colorop_state))
>> +            return PTR_ERR(colorop_state);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static void drm_atomic_colorop_print_state(struct drm_printer *p,
>>                          const struct drm_colorop_state *state)
>>   {
>> @@ -1671,11 +1722,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;
>>       }
>>       return 0;
>>   }
>> @@ -1687,10 +1736,8 @@ EXPORT_SYMBOL(drm_atomic_add_affected_planes);
>>    * @plane: DRM plane
>>    *
>>    * This function walks the current configuration and adds all colorops
>> - * currently used by @plane to the atomic configuration @state. This 
>> is useful
>> - * when an atomic commit also needs to check all currently enabled 
>> colorop on
>> - * @plane, e.g. when changing the mode. It's also useful when 
>> re-enabling a plane
>> - * to avoid special code to force-enable all colorops.
>> + * currently used by @plane to the atomic configuration @state. It's 
>> useful
>> + * when re-enabling a plane to avoid special code to force-enable 
>> all colorops.
>>    *
>>    * Since acquiring a colorop state will always also acquire the w/w 
>> mutex of the
>>    * current plane for that colorop (if there is any) adding all the 
>> colorop states for
> Unfortunately, it is not clear from the context diff but this update 
> is for drm_atomic_add_affected_colorops(). That function is now only 
> used by drm_atomic_helper_duplicate_state(). So, potentially it could 
> be move to
> drm_atomic_helper.c and made a static local helper. Alternatively, 
> should the _duplicate_state() function also be updated to only copy 
> the states of colorops in active pipelines as well? It already only 
> does the state add call if there is a pipeline, it just doesn't filter 
> according to the pipeline. Seems like it should? In which case, this 
> blanket add code can be removed entirely.

Hi John,

Regarding making drm_atomic_add_affected_colorops() static, I will do it 
in the next version, thanks for pointing out.
About the _duplicate_state(), we actually need to duplicate all colorop 
states so that they persist in a suspend/resume cycle. I have actually 
worked on duplicating all states in the v2[1], but then I dropped the 
patch because it was colliding with _atomic_check() and for some reason 
I thought it wasn't a thing. Now that you mention it, I think the right 
thing to do is reintroduce this patch (duplicating all colorop states) 
and make duplicate_state() skip the active-colorop check().

[1] 
https://lore.kernel.org/dri-devel/[email protected]/

Melissa

>
> John.
>
>> diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
>> index b4b9e4f558ab..006278e21d49 100644
>> --- a/include/drm/drm_colorop.h
>> +++ b/include/drm/drm_colorop.h
>> @@ -459,6 +459,9 @@ static inline unsigned int 
>> drm_colorop_index(const struct drm_colorop *colorop)
>>   #define drm_for_each_colorop(colorop, dev) \
>>       list_for_each_entry(colorop, &(dev)->mode_config.colorop_list, 
>> head)
>>   +#define drm_for_each_colorop_in_pipeline(colorop, pipeline) \
>> +    for ((colorop) = (pipeline); (colorop); (colorop) = 
>> (colorop)->next)
>> +
>>   /**
>>    * drm_get_colorop_type_name - return a string for colorop type
>>    * @type: colorop type to compute name of
>