On 2026-06-09 13:23, John Harrison wrote:
> On 6/3/26 04:27, Melissa Wen wrote:
>> On 01/06/2026 11:24, Borah, Chaitanya Kumar wrote:
>>> On 5/29/2026 7:16 PM, Jani Nikula wrote:
>>>> On Tue, 26 May 2026, Alex Hung <[email protected]> wrote:
>>>>> On 5/26/26 08:17, 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]>
>>>>>> ---
>>>>>> drivers/gpu/drm/drm_atomic.c | 67 +++++++++++++++++++++++++++++++-----
>>>>>> 1 file changed, 58 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>>>> index 170de30c28ae..4fb3a23e862a 100644
>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>>> @@ -812,6 +812,59 @@ 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);
>>>>>> +
>>>>>> + for (colorop = new_plane_state->color_pipeline;
>>>>>> + colorop;
>>>>>> + colorop = colorop->next) {
>>>>>
>>>>> This for-loop is used 5 times in this patchset. How about a macro in
>>>>> drm_colorop.h?
>>>>>
>>>>> #define drm_for_each_colorop_in_pipeline(colorop, pipeline) \
>>>>> for ((colorop) = (pipeline); (colorop); (colorop) = (colorop)->next)
>>>>
>>>> Is there a reason struct drm_colorop reinvents lists and doesn't have
>>>> struct list_head node?
>>>>
>>>
>>> I believe that's because the "next" colorop is exposed as a property (of
>>> the current colorop) to userspace. Since the chain is already described by
>>> the property, a struct list_head would be redundant.
>>
>> Also, each color pipeline is an immutable chain of colorops where the
>> sequence and position matter: once the chain is built, colorops are never
>> added, removed, replaced or walked in reverse. It's a forward-only chain
>> that ends when next == NULL, and it directly matches userspace mapping.
>> Another point to take into account is that there is no struct
>> drm_color_pipeline to hold a list_head yet, since each color pipeline is
>> identified by the first colorop element in the chain. Maybe we will want a
>> container to link a given pre-blend color pipeline to a specific post-blend
>> color pipeline for example, but linking pre- to post-blend color pipelines
>> is something we are still not clear about.
>>
>> Melissa
>>
> "there is no struct drm_color_pipeline to hold a list_head" <-- I think this
> is the real reason. It is possible to convert to use a proper list structure,
> but the result is slightly messy. I had a quick go at it to see how messy:
> https://patchwork.freedesktop.org/series/168200/
>
Yeah, Melissa and Chaitanya pretty much described why they work the way they
do. I'm not sure it makes sense to replace the mechanism with lists and any
attempt to do so should make sure not to break userspace ABI. I'm not opposed
to improvements either if anyone finds a solution that makes everyone's lives
easier.
Harry
> John.
>
>>>
>>> Harry, others can chime in.
>>>
>>> ==
>>> Chaitanya
>>>
>>>> BR,
>>>> Jani.
>>>>
>>>>>
>>>>>> + 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;
>>>>>> +
>>>>>> + for (colorop = old_plane_state->color_pipeline;
>>>>>> + colorop;
>>>>>> + colorop = colorop->next) {
>>>>>> + 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)
>>>>>> {
>>>>>> @@ -1591,11 +1644,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;
>>>>>> }
>>>>>> @@ -1607,10 +1658,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
>>>>>
>>>>
>>>
>>
>
