[PATCH 0/3] don't allow changes to inactive colorops

[PATCH 0/3] don't allow changes to inactive colorops

[Melissa Wen]

This series is a follow-up of what was discussed in [1] and on #wayland
IRC channel regarding policy and userspace expectations on changes in
colorop properties and the current status of the color pipeline in which
the colorop is part of. In short, we agreed that userspace can change
properties of colorops that are currently part of an active color
pipeline or when the pipeline is switching status in the same commit.
However, userspace cannot change colorop properties of inactive color
pipeline in the expactation that it will be activated at some point in
the future.

Userspace also expects persistence of color pipeline already set, even
if it becomes inactive for a while, when activated, colorop settings
previouly set should be preserved.

In addition, I found some bugs on IGT tests when this policy is applied.
So I sent some bug fixes [2]. The rest of the series in [1] was detached
in [3] since there is no dependecy between them.

[1] https://lore.kernel.org/dri-devel/20260519211111.228303-1-mwen@igalia.com/
[2] https://lore.kernel.org/dri-devel/20260525100524.304263-1-mwen@igalia.com/
[3] https://lore.kernel.org/igt-dev/20260526140752.503380-1-mwen@igalia.com/

Melissa Wen (3):
  drm/atomic: only add states of active or transient active colorops
  drm/atomic: duplicate state of all colorops
  drm/atomic: reject colorop update from inactive color pipeline

 drivers/gpu/drm/drm_atomic.c        | 126 ++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_atomic_helper.c |   9 +-
 2 files changed, 120 insertions(+), 15 deletions(-)

-- 
2.53.0

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

[Melissa Wen]

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 <mwen@igalia.com>
---
 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) {
+		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
-- 
2.53.0

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

[Alex Hung]

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 <mwen@igalia.com>
> ---
>   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)

> +		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

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

[Jani Nikula]

On Tue, 26 May 2026, Alex Hung <alex.hung@amd.com> 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 <mwen@igalia.com>
>> ---
>>   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?

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
>

-- 
Jani Nikula, Intel

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

[Borah, Chaitanya Kumar]

On 5/29/2026 7:16 PM, Jani Nikula wrote:
> On Tue, 26 May 2026, Alex Hung <alex.hung@amd.com> 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 <mwen@igalia.com>
>>> ---
>>>    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.

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

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

[Melissa Wen]

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 <alex.hung@amd.com> 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 <mwen@igalia.com>
>>>> ---
>>>>    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

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

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

[John Harrison]

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 <alex.hung@amd.com> 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 <mwen@igalia.com>
>>>>> ---
>>>>>    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/

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

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

[Harry Wentland]

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 <alex.hung@amd.com> 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 <mwen@igalia.com>
>>>>>> ---
>>>>>>    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
>>>>>
>>>>
>>>
>>
> 

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

[John Harrison]

On 6/24/26 21:01, Harry Wentland wrote:
> 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 <alex.hung@amd.com> 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 <mwen@igalia.com>
>>>>>>> ---
>>>>>>>     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
@Harry, the patch series I linked above does the conversion. It does not 
affect the user space ABI at all, only the internal kernel operation is 
changed. I think it is better in some ways but maybe not in others. If 
you would like to take a look, any feedback would be appreciated.

Thanks,
John.


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

[PATCH 2/3] drm/atomic: duplicate state of all colorops

[Melissa Wen]

Userspace expects that colorop settings of an inactive color pipeline
persist, so that, when the color pipeline is activated again, colorops
previously set preserves their values when it was deactivated. Colorop
setup is expected to persist even during a suspend/resume. To snapshot
colorop settings correctly, duplicate state of all colorops in a given
plane, independent if color pipeline is active or not.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 51f39edc31ed..ea15e58518fd 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3752,12 +3752,9 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
 			goto free;
 		}
 
-		if (plane_state->color_pipeline) {
-			err = drm_atomic_add_affected_colorops(state, plane);
-			if (err)
-				goto free;
-		}
-
+		err = drm_atomic_add_affected_colorops(state, plane);
+		if (err)
+			goto free;
 	}
 
 	drm_connector_list_iter_begin(dev, &conn_iter);
-- 
2.53.0

[PATCH 3/3] drm/atomic: reject colorop update from inactive color pipeline

[Melissa Wen]

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.

Suggested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_atomic.c | 59 ++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 4fb3a23e862a..a0549435954b 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -865,6 +865,54 @@ 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)
+			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;
+
+	/* 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)
 {
@@ -1714,6 +1762,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;
@@ -1730,6 +1780,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) {
-- 
2.53.0

Re: [PATCH 3/3] drm/atomic: reject colorop update from inactive color pipeline

[Alex Hung]

On 5/26/26 08:17, 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.
> 
> Suggested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   drivers/gpu/drm/drm_atomic.c | 59 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 4fb3a23e862a..a0549435954b 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -865,6 +865,54 @@ 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)
> +			if (colorop == new_colorop_state->colorop)
> +				return 0;
> +		return -EINVAL;
> +	}
> +
> +	if (WARN_ON(!old_plane_state)) return -EINVAL;

return should be in a new line.

> +
> +	/* 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;
> +
> +	/* 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)
>   {
> @@ -1714,6 +1762,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;
> @@ -1730,6 +1780,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) {