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

[PATCH v3 0/7] 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 bug fixes to kms_colorop and kms_properties to behave
according to this contract [2]. The rest of the series in [1] was
detached in [3] since there is no dependency between them.

[v1] https://lore.kernel.org/dri-devel/20260526142940.504911-1-mwen@igalia.com/
Changes:
- define a macro to walk in the color pipeline (Alex H.)
- fix checkpatch warning (Alex H.)
[v2] https://lore.kernel.org/dri-devel/20260604180457.1110110-1-mwen@igalia.com/
Changes:
- [Drop] drm/atomic: duplicate state of all colorops
  If inactive colorops state are duplicated on resume, the commit will be
  rejected. 
- [New] Four new patches to make AMD driver match the policy of colorop
  updates only for colorops in active color pipelines plus individual
  colorop updates. It also tries to untangle COLOR_PIPELINE = Bypass from
  colorop BYPASS prop = true. I think patches 3-5 can be cherry-picked and
  applied if it looks correct for AMD, I just included them here for
  context (for example, Sashiko reported an issue in the previous version
  of this series).

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


Melissa

Melissa Wen (7):
  drm/atomic: only add states of active or transient active colorops
  drm/atomic: reject colorop update from inactive color pipeline
  drm/amd/display: don't check colorop status if its in an inactive
    pipeline
  drm/amd/display: truly bypass plane colorop 3x4 matrix and hdr mult
  drm/amd/display: make shaper bypass mode cleaner
  drm/amd/display: fix bnld colorop bypass mode
  drm/amd/display: allow individual colorop changes

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  31 ++-
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 178 +++++++-----------
 drivers/gpu/drm/drm_atomic.c                  | 125 +++++++++++-
 include/drm/drm_colorop.h                     |   3 +
 4 files changed, 207 insertions(+), 130 deletions(-)

-- 
2.53.0

[PATCH v3 1/7] 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>
---

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Incomplete refactoring: drm_atomic_helper_duplicate_state still calls drm_atomic_add_affected_colorops.

Pre-existing issues:
- [High] Architectural Flaw: Colorops are omitted from the atomic state during normal plane updates.
--

commit a131c9a42c3beaf39f56794937e21266031060d8
Author: Melissa Wen <mwen@igalia.com>

drm/atomic: only add states of active or transient active colorops

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.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 170de30c28ae4..0d36a955c4d43 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
[ ... ]
> @@ -1591,11 +1642,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;

[Severity: High]
This isn't a bug introduced by this patch, but since
drm_atomic_add_affected_planes() executes exclusively during CRTC modesets,
does this mean active colorops are never added to the atomic state during a
simple plane update like a page flip?

Drivers that iterate over state->colorops via for_each_new_colorop_in_state()
(like intel_plane_color_copy_uapi_to_hw_state() in the intel driver)
will silently fail on page flips since the hardware state blobs remain
empty. Should the color pipeline dependency tracking be anchored to the
plane's inclusion in the state, perhaps within drm_atomic_get_plane_state()?

[Severity: Medium]
Also, the commit message states the goal is to remove the condition for
plane_state->color_pipeline to handle deactivations. Did this patch miss
updating drm_atomic_helper_duplicate_state() in drm_atomic_helper.c?

It still checks if (plane_state->color_pipeline) and calls the old
drm_atomic_add_affected_colorops(), which loops over all colorops belonging
to the plane:

drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_duplicate_state() {
	...
	if (plane_state->color_pipeline) {
		err = drm_atomic_add_affected_colorops(state, plane);
		if (err)
			goto free;
	}
	...
}

Should it be updated to use the newly introduced
drm_atomic_add_pipeline_colorops() unconditionally to ensure consistent state
duplication during suspend/resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609121230.1358786-1-mwen@igalia.com?part=1

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

[John Harrison]

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

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

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

[Melissa Wen]

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 <mwen@igalia.com>
>> ---
>>
>> 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/20260604180457.1110110-3-mwen@igalia.com/

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
>

[PATCH v3 2/7] 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 | 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)
+			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)
 {
@@ -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) {
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm_atomic_colorop_check() allows modifying colorops without adding the parent plane to the atomic commit, bypassing hardware updates.
--

commit c38eca82c086d2e818d8283fed72b37dc3abbab9
Author: Melissa Wen <mwen@igalia.com>

drm/atomic: reject colorop update from inactive color pipeline

This commit rejects updates on colorops that are not part of an active
pipeline. It adds drm_atomic_colorop_check() to verify the color pipeline
of a plane in its current, new or old state.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 0d36a955c4d43..ec3c6e4e5a062 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c

[ ... ]

> @@ -863,6 +863,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)
> +			if (colorop == new_colorop_state->colorop)
> +				return 0;

[Severity: High]
If a colorop property is updated but the parent plane isn't explicitly
modified, new_plane_state will be NULL and this returns 0.

Since drm_colorop does not have its own atomic_update callback, hardware
programming relies on the plane's commit paths iterating over planes in
the atomic state.

If the plane is missing from the atomic state, will the hardware ever be
reprogrammed with the new colorop parameters, or does this result in a
silent failure where only the software state is updated?

Should the affected plane be actively pulled into the atomic state
(e.g., via drm_atomic_get_plane_state()) whenever a colorop is modified?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609121230.1358786-1-mwen@igalia.com?part=2

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

[John Harrison]

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?

>
> Suggested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   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?

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

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

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

[Melissa Wen]

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 <chaitanya.kumar.borah@intel.com>
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>> ---
>>   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) {
>

[PATCH v3 3/7] drm/amd/display: don't check colorop status if its in an inactive pipeline

[Melissa Wen]

If colorop BYPASS property is true, but the colorop isn't part of an
active/transient active color pipeline, this colorop status should not
be taken into account when checking if a plane color pipeline is
actually active. For example, if the userspace doesn't explicitly set a
colorop obj to bypass but deactivates its color pipeline by setting
plane COLOR_PIPELINE to bypass, it means that colorop is inactive
regardless of its BYPASS property status.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: d3a549f4df78 ("drm/amd/display: Use overlay cursor when color pipeline is active")
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 31 +++++++++++++------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index ba7f98a87808..2edec3e1b838 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -12590,9 +12590,9 @@ static int add_affected_mst_dsc_crtcs(struct drm_atomic_commit *state, struct dr
  * @use_old: if true, inspect the old colorop states; otherwise the new ones
  *
  * A color pipeline may be selected (color_pipeline != NULL) but still is
- * inactive if every colorop in the chain is bypassed.  Only return
- * true when at least one colorop has bypass == false, meaning the cursor
- * would be subjected to the transformation in native mode.
+ * inactive if every colorop in the chain is bypassed. Only return true when at
+ * least one active colorop has bypass == false, meaning the cursor would be
+ * subjected to the transformation in native mode.
  *
  * Return: true if the pipeline modifies pixels, false otherwise.
  */
@@ -12600,18 +12600,29 @@ static bool dm_plane_color_pipeline_active(struct drm_atomic_commit *state,
 					   struct drm_plane *plane,
 					   bool use_old)
 {
-	struct drm_colorop *colorop;
-	struct drm_colorop_state *old_colorop_state, *new_colorop_state;
-	int i;
+	struct drm_plane_state *plane_state = use_old ?
+					      drm_atomic_get_old_plane_state(state, plane) :
+					      drm_atomic_get_new_plane_state(state, plane);
+	struct drm_colorop *colorop, *pipeline;
+	struct drm_colorop_state *cstate;
 
-	for_each_oldnew_colorop_in_state(state, colorop, old_colorop_state, new_colorop_state, i) {
-		struct drm_colorop_state *cstate = use_old ? old_colorop_state : new_colorop_state;
+	pipeline = plane_state ? plane_state->color_pipeline :
+				 plane->state->color_pipeline;
 
-		if (cstate->colorop->plane != plane)
-			continue;
+	if (!pipeline)
+		return false;
+
+	drm_for_each_colorop_in_pipeline(colorop, pipeline) {
+		cstate = use_old ?
+			 drm_atomic_get_old_colorop_state(state, colorop) :
+			 drm_atomic_get_new_colorop_state(state, colorop);
+
+		if (!cstate)
+			cstate = colorop->state;
 		if (!cstate->bypass)
 			return true;
 	}
+
 	return false;
 }
 
-- 
2.53.0

Re: [PATCH v3 3/7] drm/amd/display: don't check colorop status if its in an inactive pipeline

[John Harrison]

On 6/9/26 13:51, Melissa Wen wrote:
> If colorop BYPASS property is true, but the colorop isn't part of an
Should this say 'is false'?

John.

> active/transient active color pipeline, this colorop status should not
> be taken into account when checking if a plane color pipeline is
> actually active. For example, if the userspace doesn't explicitly set a
> colorop obj to bypass but deactivates its color pipeline by setting
> plane COLOR_PIPELINE to bypass, it means that colorop is inactive
> regardless of its BYPASS property status.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Fixes: d3a549f4df78 ("drm/amd/display: Use overlay cursor when color pipeline is active")
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 31 +++++++++++++------
>   1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index ba7f98a87808..2edec3e1b838 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -12590,9 +12590,9 @@ static int add_affected_mst_dsc_crtcs(struct drm_atomic_commit *state, struct dr
>    * @use_old: if true, inspect the old colorop states; otherwise the new ones
>    *
>    * A color pipeline may be selected (color_pipeline != NULL) but still is
> - * inactive if every colorop in the chain is bypassed.  Only return
> - * true when at least one colorop has bypass == false, meaning the cursor
> - * would be subjected to the transformation in native mode.
> + * inactive if every colorop in the chain is bypassed. Only return true when at
> + * least one active colorop has bypass == false, meaning the cursor would be
> + * subjected to the transformation in native mode.
>    *
>    * Return: true if the pipeline modifies pixels, false otherwise.
>    */
> @@ -12600,18 +12600,29 @@ static bool dm_plane_color_pipeline_active(struct drm_atomic_commit *state,
>   					   struct drm_plane *plane,
>   					   bool use_old)
>   {
> -	struct drm_colorop *colorop;
> -	struct drm_colorop_state *old_colorop_state, *new_colorop_state;
> -	int i;
> +	struct drm_plane_state *plane_state = use_old ?
> +					      drm_atomic_get_old_plane_state(state, plane) :
> +					      drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_colorop *colorop, *pipeline;
> +	struct drm_colorop_state *cstate;
>   
> -	for_each_oldnew_colorop_in_state(state, colorop, old_colorop_state, new_colorop_state, i) {
> -		struct drm_colorop_state *cstate = use_old ? old_colorop_state : new_colorop_state;
> +	pipeline = plane_state ? plane_state->color_pipeline :
> +				 plane->state->color_pipeline;
Why would plane_state be null? And if it is, why is it correct to use 
plane->state rather than the old or new state as requested by the 
use_old flag? Seems like there should be a comment to explain this.

>   
> -		if (cstate->colorop->plane != plane)
> -			continue;
> +	if (!pipeline)
> +		return false;
> +
> +	drm_for_each_colorop_in_pipeline(colorop, pipeline) {
> +		cstate = use_old ?
> +			 drm_atomic_get_old_colorop_state(state, colorop) :
> +			 drm_atomic_get_new_colorop_state(state, colorop);
> +
> +		if (!cstate)
> +			cstate = colorop->state;
Same question as above. Why would there not be a old/new state and if 
there isn't, why is it correct to use the current state when a check 
against the old/new state was explicitly requested?

John.

>   		if (!cstate->bypass)
>   			return true;
>   	}
> +
>   	return false;
>   }
>   

Re: [PATCH v3 3/7] drm/amd/display: don't check colorop status if its in an inactive pipeline

[Melissa Wen]

On 26/06/2026 00:52, John Harrison wrote:
> On 6/9/26 13:51, Melissa Wen wrote:
>> If colorop BYPASS property is true, but the colorop isn't part of an
> Should this say 'is false'?

Oops, you are right. I'll fix it in the next version.

>
> John.
>
>> active/transient active color pipeline, this colorop status should not
>> be taken into account when checking if a plane color pipeline is
>> actually active. For example, if the userspace doesn't explicitly set a
>> colorop obj to bypass but deactivates its color pipeline by setting
>> plane COLOR_PIPELINE to bypass, it means that colorop is inactive
>> regardless of its BYPASS property status.
>>
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Fixes: d3a549f4df78 ("drm/amd/display: Use overlay cursor when color 
>> pipeline is active")
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 31 +++++++++++++------
>>   1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index ba7f98a87808..2edec3e1b838 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -12590,9 +12590,9 @@ static int add_affected_mst_dsc_crtcs(struct 
>> drm_atomic_commit *state, struct dr
>>    * @use_old: if true, inspect the old colorop states; otherwise the 
>> new ones
>>    *
>>    * A color pipeline may be selected (color_pipeline != NULL) but 
>> still is
>> - * inactive if every colorop in the chain is bypassed.  Only return
>> - * true when at least one colorop has bypass == false, meaning the 
>> cursor
>> - * would be subjected to the transformation in native mode.
>> + * inactive if every colorop in the chain is bypassed. Only return 
>> true when at
>> + * least one active colorop has bypass == false, meaning the cursor 
>> would be
>> + * subjected to the transformation in native mode.
>>    *
>>    * Return: true if the pipeline modifies pixels, false otherwise.
>>    */
>> @@ -12600,18 +12600,29 @@ static bool 
>> dm_plane_color_pipeline_active(struct drm_atomic_commit *state,
>>                          struct drm_plane *plane,
>>                          bool use_old)
>>   {
>> -    struct drm_colorop *colorop;
>> -    struct drm_colorop_state *old_colorop_state, *new_colorop_state;
>> -    int i;
>> +    struct drm_plane_state *plane_state = use_old ?
>> +                          drm_atomic_get_old_plane_state(state, 
>> plane) :
>> +                          drm_atomic_get_new_plane_state(state, plane);
>> +    struct drm_colorop *colorop, *pipeline;
>> +    struct drm_colorop_state *cstate;
>>   -    for_each_oldnew_colorop_in_state(state, colorop, 
>> old_colorop_state, new_colorop_state, i) {
>> -        struct drm_colorop_state *cstate = use_old ? 
>> old_colorop_state : new_colorop_state;
>> +    pipeline = plane_state ? plane_state->color_pipeline :
>> +                 plane->state->color_pipeline;
> Why would plane_state be null? And if it is, why is it correct to use 
> plane->state rather than the old or new state as requested by the 
> use_old flag? Seems like there should be a comment to explain this.

Right, about the plane_state, it was a defensive approach for the case 
that we have a colorop change but no changes on plane that make its 
state part of an atomic commit and in this case, there is no difference 
between old/new state, it's just the committed state.
However following the callers of this function, currently plane_state is 
never NULL, so now I think a WARN_ON is enough, instead of handling an 
unreachable case.
>
>
>>   -        if (cstate->colorop->plane != plane)
>> -            continue;
>> +    if (!pipeline)
>> +        return false;
>> +
>> +    drm_for_each_colorop_in_pipeline(colorop, pipeline) {
>> +        cstate = use_old ?
>> +             drm_atomic_get_old_colorop_state(state, colorop) :
>> +             drm_atomic_get_new_colorop_state(state, colorop);
>> +
>> +        if (!cstate)
>> +            cstate = colorop->state;
> Same question as above. Why would there not be a old/new state and if 
> there isn't, why is it correct to use the current state when a check 
> against the old/new state was explicitly requested?

Here is a bit different, there is a situation in which just one or two 
colorops of a given color pipeline changes, but others are not touched. 
In this case only colorops that changed will have a old/new states, but 
others only have the committed state for validation.
To state that a given color pipeline can modify pixels or not, we also 
need to check the commited/current state of all colorop in the selected 
colorop pipeline, not only those that were changed in this atomic commit.
I'll add a comment explaining it better.

Thanks,

Melissa

>
> John.
>
>>           if (!cstate->bypass)
>>               return true;
>>       }
>> +
>>       return false;
>>   }
>

[PATCH v3 4/7] drm/amd/display: truly bypass plane colorop 3x4 matrix and hdr mult

[Melissa Wen]

Latent issue as the driver is currently just skipping programming 3x4
matrix and hdr multiplier blocks on bypass. Reset to default values if
the bypass property is set true.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c    | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index fa6883ae4dfb..84ff136183ee 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -1542,7 +1542,13 @@ __set_dm_plane_colorop_3x4_matrix(struct drm_plane_state *plane_state,
 		}
 	}
 
-	if (colorop_state && !colorop_state->bypass && colorop->type == DRM_COLOROP_CTM_3X4) {
+	if (colorop_state && colorop->type == DRM_COLOROP_CTM_3X4) {
+		if (colorop_state->bypass) {
+			dc_plane_state->gamut_remap_matrix.enable_remap = false;
+			dc_plane_state->input_csc_color_matrix.enable_adjustment = false;
+			return 0;
+		}
+
 		drm_dbg(dev, "3x4 matrix colorop with ID: %d\n", colorop->base.id);
 		blob = colorop_state->data;
 		if (blob->length == sizeof(struct drm_color_ctm_3x4)) {
@@ -1581,9 +1587,13 @@ __set_dm_plane_colorop_multiplier(struct drm_plane_state *plane_state,
 		}
 	}
 
-	if (colorop_state && !colorop_state->bypass && colorop->type == DRM_COLOROP_MULTIPLIER) {
-		drm_dbg(dev, "Multiplier colorop with ID: %d\n", colorop->base.id);
-		dc_plane_state->hdr_mult = amdgpu_dm_fixpt_from_s3132(colorop_state->multiplier);
+	if (colorop_state && colorop->type == DRM_COLOROP_MULTIPLIER) {
+		if (colorop_state->bypass) {
+			dc_plane_state->hdr_mult = dc_fixpt_one;
+		} else {
+			drm_dbg(dev, "Multiplier colorop with ID: %d\n", colorop->base.id);
+			dc_plane_state->hdr_mult = amdgpu_dm_fixpt_from_s3132(colorop_state->multiplier);
+		}
 	}
 
 	return 0;
-- 
2.53.0

Re: [PATCH v3 4/7] drm/amd/display: truly bypass plane colorop 3x4 matrix and hdr mult

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Dereference of `blob->length` when `blob` is NULL in `__set_dm_plane_colorop_3x4_matrix`.
--

commit e38d376afd75d8310bd23096e9f3d253c0d99c70
Author: Melissa Wen <mwen@igalia.com>

drm/amd/display: truly bypass plane colorop 3x4 matrix and hdr mult

This commit addresses a latent issue where the driver skips programming the
3x4 matrix and hdr multiplier blocks on bypass. It updates the driver to
explicitly reset these blocks to their default values if the bypass
property is set.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index 86086d10c5435..13ad7ac1e35e4 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> @@ -1579,7 +1579,13 @@ __set_dm_plane_colorop_3x4_matrix(struct drm_plane_state *plane_state,
>  		}
>  	}
>  
> -	if (colorop_state && !colorop_state->bypass && colorop->type == DRM_COLOROP_CTM_3X4) {
> +	if (colorop_state && colorop->type == DRM_COLOROP_CTM_3X4) {
> +		if (colorop_state->bypass) {
> +			dc_plane_state->gamut_remap_matrix.enable_remap = false;
> +			dc_plane_state->input_csc_color_matrix.enable_adjustment = false;
> +			return 0;
> +		}
> +
>  		drm_dbg(dev, "3x4 matrix colorop with ID: %d\n", colorop->base.id);
>  		blob = colorop_state->data;
>  		if (blob->length == sizeof(struct drm_color_ctm_3x4)) {

[Severity: High]
This isn't a bug introduced by this patch, but can blob be NULL here when
colorop_state->bypass is false?

Looking at __set_dm_plane_colorop_3x4_matrix(), the DRM API allows userspace
to unset the data property (for instance, by passing 0 for the blob ID).
This would result in colorop_state->data being NULL. 

If userspace sets bypass to false while providing no data blob, would the
unconditional dereference of blob->length lead to a NULL pointer dereference
and a kernel crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609121230.1358786-1-mwen@igalia.com?part=4

[PATCH v3 5/7] drm/amd/display: make shaper bypass mode cleaner

[Melissa Wen]

Start shaper tranfer function setup in bypass mode, i.e. tf->type ==
TF_TYPE_BYPASS and let the helper checks set it to a different mode
according to userspace request.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index 84ff136183ee..2f51009b2978 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -1611,10 +1611,11 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
 	struct dc_transfer_func *tf = &dc_plane_state->in_shaper_func;
 	const struct drm_color_lut32 *shaper_lut;
 	struct drm_device *dev = colorop->dev;
-	bool enabled = false;
 	u32 shaper_size;
 	int i = 0, ret = 0;
 
+	tf->type = TF_TYPE_BYPASS;
+
 	/* 1D Curve - SHAPER TF */
 	old_colorop = colorop;
 	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
@@ -1633,7 +1634,6 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
 		ret = __set_output_tf(tf, 0, 0, false);
 		if (ret)
 			return ret;
-		enabled = true;
 	}
 
 	/* 1D LUT - SHAPER LUT */
@@ -1665,13 +1665,9 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
 			ret = __set_output_tf_32(tf, shaper_lut, shaper_size, false);
 			if (ret)
 				return ret;
-			enabled = true;
 		}
 	}
 
-	if (!enabled)
-		tf->type = TF_TYPE_BYPASS;
-
 	return 0;
 }
 
-- 
2.53.0

Re: [PATCH v3 5/7] drm/amd/display: make shaper bypass mode cleaner

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Removing the `enabled` fallback leaves the shaper transfer function in a corrupted state if an invalid 1D LUT size is provided.

Pre-existing issues:
- [High] Atomic check silently ignores invalid colorop LUT sizes and allocation failures in the blend transfer function.
--

commit 7d44b36a8843f15370d407ecab07d171e3f7d506
Author: Melissa Wen <mwen@igalia.com>

drm/amd/display: make shaper bypass mode cleaner

Start shaper tranfer function setup in bypass mode, i.e. tf->type ==
TF_TYPE_BYPASS and let the helper checks set it to a different mode
according to userspace request.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index 13ad7ac1e35e4..4651241433296 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> @@ -1648,10 +1648,11 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
>  	struct dc_transfer_func *tf = &dc_plane_state->in_shaper_func;
>  	const struct drm_color_lut32 *shaper_lut;
>  	struct drm_device *dev = colorop->dev;
> -	bool enabled = false;
>  	u32 shaper_size;
>  	int i = 0, ret = 0;
>  
> +	tf->type = TF_TYPE_BYPASS;
> +
>  	/* 1D Curve - SHAPER TF */
>  	old_colorop = colorop;
>  	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {

[ ... ]

> @@ -1702,13 +1702,9 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
>  			ret = __set_output_tf_32(tf, shaper_lut, shaper_size, false);
>  			if (ret)
>  				return ret;
> -			enabled = true;
>  		}
>  	}
>  
> -	if (!enabled)
> -		tf->type = TF_TYPE_BYPASS;
> -
>  	return 0;
>  }

[Severity: High]
Does removing the enabled fallback leave the shaper transfer function in a
corrupted state if an invalid 1D LUT size is provided?

If userspace sets bypass = false but provides an invalid LUT size or unsets
the data blob (size = 0), the check for shaper_size == colorop->size fails
and __set_output_tf_32() is skipped in __set_dm_plane_colorop_shaper().

Since tf->type was already set to TF_TYPE_DISTRIBUTED_POINTS earlier in the
function, and we no longer fall back to TF_TYPE_BYPASS when enabled is false,
the function returns 0. Could this cause the atomic commit to succeed while
programming the hardware with an uninitialized transfer function?

[Severity: High]
This is a pre-existing issue, but a similar problem exists in
__set_dm_plane_colorop_blend().

Does the atomic check silently ignore invalid colorop LUT sizes and allocation
failures in the blend transfer function?

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c:__set_dm_plane_colorop_blend() {
	...
		/* Custom LUT size must be the same as supported size */
		if (blend_size == colorop->size)
			__set_input_tf_32(NULL, tf, blend_lut, blend_size);
	}

	return 0;
}

If the size check fails, __set_input_tf_32() is skipped entirely, but the
function returns 0. Also, the return value of __set_input_tf_32() is ignored,
meaning allocation failures are not propagated. Could this allow the hardware
to be programmed with an uninitialized or partially initialized transfer
function?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609121230.1358786-1-mwen@igalia.com?part=5

[PATCH v3 6/7] drm/amd/display: fix bnld colorop bypass mode

[Melissa Wen]

If userspace set blnd colorop to bypass, AMD driver just skips blnd
transfer function configuration. Currently, this is not an issue since
dc plane state is a reset/default state, but it's not fully correct and
doesn't mirror shaper tf helper. Make bypass mode setup clear by
initially set tf->type as BYPASS and let the helper change its type
according to userspace requests.

Fixes: 52da8325996b ("drm/amd/display: Add support for sRGB EOTF in BLND block")
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index 2f51009b2978..2dc3951c8e39 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -1773,6 +1773,8 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
 	uint32_t blend_size = 0;
 	int i = 0;
 
+	tf->type = TF_TYPE_BYPASS;
+
 	/* 1D Curve - BLND TF */
 	old_colorop = colorop;
 	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
-- 
2.53.0

Re: [PATCH v3 6/7] drm/amd/display: fix bnld colorop bypass mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Silent failure and invalid hardware state when an active 1D LUT has an invalid data blob size.
- [High] The return value of `__set_input_tf_32()` is ignored, masking potential memory allocation failures.
- [Medium] The validation logic for the 1D LUT colorop incorrectly checks the `curve_1d_type` property, which is unused and logically invalid for LUTs.
--

commit 43bb295360cf4259492eaea0a93a4e70f5f5599c
Author: Melissa Wen <mwen@igalia.com>
Subject: drm/amd/display: fix bnld colorop bypass mode

If userspace set blnd colorop to bypass, AMD driver skips blnd transfer
function configuration. This patch clarifies the bypass mode setup by initially
setting the tf type to bypass.

Fixes: 52da8325996b ("drm/amd/display: Add support for sRGB EOTF in BLND block")

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index 4651241433296..2d91d88a45742 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> @@ -1811,6 +1811,8 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
>  	uint32_t blend_size = 0;
>  	int i = 0;
>  
> +	tf->type = TF_TYPE_BYPASS;
> +
>  	/* 1D Curve - BLND TF */

[ ... ]

> 	if (!colorop_state->bypass && colorop->type == DRM_COLOROP_1D_CURVE &&
> 	    (BIT(colorop_state->curve_1d_type) & amdgpu_dm_supported_blnd_tfs)) {
> 		drm_dbg(dev, "Blend TF colorop with ID: %d\n", colorop->base.id);
> 		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
> 		tf->tf = default_tf = amdgpu_colorop_tf_to_dc_tf(colorop_state->curve_1d_type);
> 		tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
> 		__set_input_tf_32(NULL, tf, blend_lut, blend_size);

[Severity: High]
This is a pre-existing issue, but does this mask potential memory allocation
failures?

The return value of __set_input_tf_32() is ignored here, so if it returns an
error, the atomic commit proceeds with incompletely programmed hardware state.

> 	}
> 
> 	/* 1D Curve - BLND LUT */

[ ... ]

> 	if (!colorop_state->bypass && colorop->type == DRM_COLOROP_1D_LUT &&
> 	    (BIT(colorop_state->curve_1d_type) & amdgpu_dm_supported_blnd_tfs)) {

[Severity: Medium]
This isn't a bug introduced by this patch, but does this improperly validate
the 1D LUT state?

The validation logic incorrectly checks the curve_1d_type property, which is
unused and logically invalid for LUTs. The condition passes by coincidence
because 0 corresponds to DRM_COLOROP_1D_CURVE_SRGB_EOTF, which happens to be
supported by the BLND block.

> 		drm_dbg(dev, "Blend LUT colorop with ID: %d\n", colorop->base.id);
> 		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
> 		tf->tf = default_tf;
> 		tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
> 		blend_lut = __extract_blob_lut32(colorop_state->data, &blend_size);
> 		blend_size = blend_lut != NULL ? blend_size : 0;
> 
> 		/* Custom LUT size must be the same as supported size */
> 		if (blend_size == colorop->size)
> 			__set_input_tf_32(NULL, tf, blend_lut, blend_size);

[Severity: High]
This is a pre-existing issue, but could this lead to an invalid hardware
state if the sizes do not match?

If a 1D LUT colorop is active but the user provides an improperly sized or
missing data blob, the setup function __set_input_tf_32() is skipped.

However, tf->type is already set to TF_TYPE_DISTRIBUTED_POINTS and the
function returns success (0) instead of an error. This leaves the transfer
function logically active but with uninitialized point data. A similar issue
appears to exist in __set_dm_plane_colorop_shaper() as well.

> 	}
> 
> 	return 0;
> }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609121230.1358786-1-mwen@igalia.com?part=6

[PATCH v3 7/7] drm/amd/display: allow individual colorop changes

[Melissa Wen]

Every AMD colorop helper requires new colorop state to update a single
active colorop, i.e. if the userspace modifies a single property of a
colorop, but doesn't resubmit the whole color pipeline, the driver
rejects the atomic commit, instead of just restore colorop settings from
committed state. Change all colorop helpers to get the committed state
if there's no new state for a given colorop. It keeps walking in the
active color pipeline and update a color block if the related colorop
changed.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 182 +++++++-----------
 1 file changed, 65 insertions(+), 117 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index 2dc3951c8e39..08cbe3b862d4 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -1497,24 +1497,13 @@ __set_dm_plane_colorop_degamma(struct drm_plane_state *plane_state,
 			       struct dc_plane_state *dc_plane_state,
 			       struct drm_colorop *colorop)
 {
-	struct drm_colorop *old_colorop;
-	struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
+	struct drm_colorop_state *colorop_state;
 	struct drm_atomic_commit *state = plane_state->state;
-	int i = 0;
-
-	old_colorop = colorop;
 
 	/* 1st op: 1d curve - degamma */
-	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
-		if (new_colorop_state->colorop == old_colorop &&
-		    (BIT(new_colorop_state->curve_1d_type) & amdgpu_dm_supported_degam_tfs)) {
-			colorop_state = new_colorop_state;
-			break;
-		}
-	}
-
+	colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
 	if (!colorop_state)
-		return -EINVAL;
+		colorop_state = colorop->state;
 
 	return __set_colorop_in_tf_1d_curve(dc_plane_state, colorop_state);
 }
@@ -1524,43 +1513,37 @@ __set_dm_plane_colorop_3x4_matrix(struct drm_plane_state *plane_state,
 				  struct dc_plane_state *dc_plane_state,
 				  struct drm_colorop *colorop)
 {
-	struct drm_colorop *old_colorop;
-	struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
+	struct drm_colorop_state *colorop_state;
 	struct drm_atomic_commit *state = plane_state->state;
 	const struct drm_device *dev = colorop->dev;
 	const struct drm_property_blob *blob;
 	struct drm_color_ctm_3x4 *ctm = NULL;
-	int i = 0;
 
 	/* 3x4 matrix */
-	old_colorop = colorop;
-	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
-		if (new_colorop_state->colorop == old_colorop &&
-		    new_colorop_state->colorop->type == DRM_COLOROP_CTM_3X4) {
-			colorop_state = new_colorop_state;
-			break;
-		}
+	colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
+	if (!colorop_state)
+		colorop_state = colorop->state;
+
+	if (colorop_state->colorop->type != DRM_COLOROP_CTM_3X4)
+		return -EINVAL;
+
+	if (colorop_state->bypass) {
+		dc_plane_state->gamut_remap_matrix.enable_remap = false;
+		dc_plane_state->input_csc_color_matrix.enable_adjustment = false;
+		return 0;
 	}
 
-	if (colorop_state && colorop->type == DRM_COLOROP_CTM_3X4) {
-		if (colorop_state->bypass) {
-			dc_plane_state->gamut_remap_matrix.enable_remap = false;
-			dc_plane_state->input_csc_color_matrix.enable_adjustment = false;
-			return 0;
-		}
-
-		drm_dbg(dev, "3x4 matrix colorop with ID: %d\n", colorop->base.id);
-		blob = colorop_state->data;
-		if (blob->length == sizeof(struct drm_color_ctm_3x4)) {
-			ctm = (struct drm_color_ctm_3x4 *) blob->data;
-			__drm_ctm_3x4_to_dc_matrix(ctm, dc_plane_state->gamut_remap_matrix.matrix);
-			dc_plane_state->gamut_remap_matrix.enable_remap = true;
-			dc_plane_state->input_csc_color_matrix.enable_adjustment = false;
-		} else {
-			drm_warn(dev, "blob->length (%zu) isn't equal to drm_color_ctm_3x4 (%zu)\n",
-				 blob->length, sizeof(struct drm_color_ctm_3x4));
-			return -EINVAL;
-		}
+	drm_dbg(dev, "3x4 matrix colorop with ID: %d\n", colorop->base.id);
+	blob = colorop_state->data;
+	if (blob->length == sizeof(struct drm_color_ctm_3x4)) {
+		ctm = (struct drm_color_ctm_3x4 *) blob->data;
+		__drm_ctm_3x4_to_dc_matrix(ctm, dc_plane_state->gamut_remap_matrix.matrix);
+		dc_plane_state->gamut_remap_matrix.enable_remap = true;
+		dc_plane_state->input_csc_color_matrix.enable_adjustment = false;
+	} else {
+		drm_warn(dev, "blob->length (%zu) isn't equal to drm_color_ctm_3x4 (%zu)\n",
+			 blob->length, sizeof(struct drm_color_ctm_3x4));
+		return -EINVAL;
 	}
 
 	return 0;
@@ -1571,29 +1554,23 @@ __set_dm_plane_colorop_multiplier(struct drm_plane_state *plane_state,
 				  struct dc_plane_state *dc_plane_state,
 				  struct drm_colorop *colorop)
 {
-	struct drm_colorop *old_colorop;
-	struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
+	struct drm_colorop_state *colorop_state;
 	struct drm_atomic_commit *state = plane_state->state;
 	const struct drm_device *dev = colorop->dev;
-	int i = 0;
 
 	/* Multiplier */
-	old_colorop = colorop;
-	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
-		if (new_colorop_state->colorop == old_colorop &&
-		    new_colorop_state->colorop->type == DRM_COLOROP_MULTIPLIER) {
-			colorop_state = new_colorop_state;
-			break;
-		}
-	}
+	colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
+	if (!colorop_state)
+		colorop_state = colorop->state;
 
-	if (colorop_state && colorop->type == DRM_COLOROP_MULTIPLIER) {
-		if (colorop_state->bypass) {
-			dc_plane_state->hdr_mult = dc_fixpt_one;
-		} else {
-			drm_dbg(dev, "Multiplier colorop with ID: %d\n", colorop->base.id);
-			dc_plane_state->hdr_mult = amdgpu_dm_fixpt_from_s3132(colorop_state->multiplier);
-		}
+	if (colorop_state->colorop->type != DRM_COLOROP_MULTIPLIER)
+		return -EINVAL;
+
+	if (colorop_state->bypass) {
+		dc_plane_state->hdr_mult = dc_fixpt_one;
+	} else {
+		drm_dbg(dev, "Multiplier colorop with ID: %d\n", colorop->base.id);
+		dc_plane_state->hdr_mult = amdgpu_dm_fixpt_from_s3132(colorop_state->multiplier);
 	}
 
 	return 0;
@@ -1604,29 +1581,23 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
 			      struct dc_plane_state *dc_plane_state,
 			      struct drm_colorop *colorop)
 {
-	struct drm_colorop *old_colorop;
-	struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
+	struct drm_colorop_state *colorop_state;
 	struct drm_atomic_commit *state = plane_state->state;
 	enum dc_transfer_func_predefined default_tf = TRANSFER_FUNCTION_LINEAR;
 	struct dc_transfer_func *tf = &dc_plane_state->in_shaper_func;
 	const struct drm_color_lut32 *shaper_lut;
 	struct drm_device *dev = colorop->dev;
 	u32 shaper_size;
-	int i = 0, ret = 0;
+	int ret = 0;
 
 	tf->type = TF_TYPE_BYPASS;
 
 	/* 1D Curve - SHAPER TF */
-	old_colorop = colorop;
-	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
-		if (new_colorop_state->colorop == old_colorop &&
-		    (BIT(new_colorop_state->curve_1d_type) & amdgpu_dm_supported_shaper_tfs)) {
-			colorop_state = new_colorop_state;
-			break;
-		}
-	}
+	colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
+	if (!colorop_state)
+		colorop_state = colorop->state;
 
-	if (colorop_state && !colorop_state->bypass && colorop->type == DRM_COLOROP_1D_CURVE) {
+	if (!colorop_state->bypass && colorop->type == DRM_COLOROP_1D_CURVE) {
 		drm_dbg(dev, "Shaper TF colorop with ID: %d\n", colorop->base.id);
 		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
 		tf->tf = default_tf = amdgpu_colorop_tf_to_dc_tf(colorop_state->curve_1d_type);
@@ -1637,22 +1608,17 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
 	}
 
 	/* 1D LUT - SHAPER LUT */
-	colorop = old_colorop->next;
+	colorop = colorop->next;
 	if (!colorop) {
 		drm_dbg(dev, "no Shaper LUT colorop found\n");
 		return -EINVAL;
 	}
 
-	old_colorop = colorop;
-	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
-		if (new_colorop_state->colorop == old_colorop &&
-		    new_colorop_state->colorop->type == DRM_COLOROP_1D_LUT) {
-			colorop_state = new_colorop_state;
-			break;
-		}
-	}
+	colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
+	if (!colorop_state)
+		colorop_state = colorop->state;
 
-	if (colorop_state && !colorop_state->bypass && colorop->type == DRM_COLOROP_1D_LUT) {
+	if (!colorop_state->bypass && colorop->type == DRM_COLOROP_1D_LUT) {
 		drm_dbg(dev, "Shaper LUT colorop with ID: %d\n", colorop->base.id);
 		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
 		tf->tf = default_tf;
@@ -1707,8 +1673,7 @@ __set_dm_plane_colorop_3dlut(struct drm_plane_state *plane_state,
 			     struct dc_plane_state *dc_plane_state,
 			     struct drm_colorop *colorop)
 {
-	struct drm_colorop *old_colorop;
-	struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
+	struct drm_colorop_state *colorop_state;
 	struct dc_transfer_func *tf = &dc_plane_state->in_shaper_func;
 	struct drm_atomic_commit *state = plane_state->state;
 	const struct amdgpu_device *adev = drm_to_adev(colorop->dev);
@@ -1716,19 +1681,14 @@ __set_dm_plane_colorop_3dlut(struct drm_plane_state *plane_state,
 	const struct drm_device *dev = colorop->dev;
 	const struct drm_color_lut32 *lut3d;
 	uint32_t lut3d_size;
-	int i = 0, ret = 0;
+	int ret = 0;
 
 	/* 3D LUT */
-	old_colorop = colorop;
-	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
-		if (new_colorop_state->colorop == old_colorop &&
-		    new_colorop_state->colorop->type == DRM_COLOROP_3D_LUT) {
-			colorop_state = new_colorop_state;
-			break;
-		}
-	}
+	colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
+	if (!colorop_state)
+		colorop_state = colorop->state;
 
-	if (colorop_state && !colorop_state->bypass && colorop->type == DRM_COLOROP_3D_LUT) {
+	if (!colorop_state->bypass && colorop->type == DRM_COLOROP_3D_LUT) {
 		if (!has_3dlut) {
 			drm_dbg(dev, "3D LUT is not supported by hardware\n");
 			return -EINVAL;
@@ -1763,29 +1723,22 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
 			     struct dc_plane_state *dc_plane_state,
 			     struct drm_colorop *colorop)
 {
-	struct drm_colorop *old_colorop;
-	struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
+	struct drm_colorop_state *colorop_state;
 	struct drm_atomic_commit *state = plane_state->state;
 	enum dc_transfer_func_predefined default_tf = TRANSFER_FUNCTION_LINEAR;
 	struct dc_transfer_func *tf = &dc_plane_state->blend_tf;
 	const struct drm_color_lut32 *blend_lut = NULL;
 	struct drm_device *dev = colorop->dev;
 	uint32_t blend_size = 0;
-	int i = 0;
 
 	tf->type = TF_TYPE_BYPASS;
 
 	/* 1D Curve - BLND TF */
-	old_colorop = colorop;
-	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
-		if (new_colorop_state->colorop == old_colorop &&
-		    (BIT(new_colorop_state->curve_1d_type) & amdgpu_dm_supported_blnd_tfs)) {
-			colorop_state = new_colorop_state;
-			break;
-		}
-	}
+	colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
+	if (!colorop_state)
+		colorop_state = colorop->state;
 
-	if (colorop_state && !colorop_state->bypass && colorop->type == DRM_COLOROP_1D_CURVE &&
+	if (!colorop_state->bypass && colorop->type == DRM_COLOROP_1D_CURVE &&
 	    (BIT(colorop_state->curve_1d_type) & amdgpu_dm_supported_blnd_tfs)) {
 		drm_dbg(dev, "Blend TF colorop with ID: %d\n", colorop->base.id);
 		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
@@ -1795,22 +1748,17 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
 	}
 
 	/* 1D Curve - BLND LUT */
-	colorop = old_colorop->next;
+	colorop = colorop->next;
 	if (!colorop) {
 		drm_dbg(dev, "no Blend LUT colorop found\n");
 		return -EINVAL;
 	}
 
-	old_colorop = colorop;
-	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
-		if (new_colorop_state->colorop == old_colorop &&
-		    new_colorop_state->colorop->type == DRM_COLOROP_1D_LUT) {
-			colorop_state = new_colorop_state;
-			break;
-		}
-	}
+	colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
+	if (!colorop_state)
+		colorop_state = colorop->state;
 
-	if (colorop_state && !colorop_state->bypass && colorop->type == DRM_COLOROP_1D_LUT &&
+	if (!colorop_state->bypass && colorop->type == DRM_COLOROP_1D_LUT &&
 	    (BIT(colorop_state->curve_1d_type) & amdgpu_dm_supported_blnd_tfs)) {
 		drm_dbg(dev, "Blend LUT colorop with ID: %d\n", colorop->base.id);
 		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
-- 
2.53.0

Re: [PATCH v3 7/7] drm/amd/display: allow individual colorop changes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Potential NULL pointer dereference when accessing `blob->length` in `__set_dm_plane_colorop_3x4_matrix` if the data blob is NULL.
--

commit f99935be74382989d52703e3315aeec4d0efd059
Author: Melissa Wen <mwen@igalia.com>

drm/amd/display: allow individual colorop changes

Change all colorop helpers to get the committed state if there's no new
state for a given colorop. This allows userspace to modify a single
property of a colorop without resubmitting the whole color pipeline by
falling back to colorop settings from the committed state.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index 2d91d88a45742..51bd937153d41 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c

[ ... ]

> @@ -1561,43 +1550,37 @@ __set_dm_plane_colorop_3x4_matrix(struct drm_plane_state *plane_state,

[ ... ]

> +	if (colorop_state->bypass) {
> +		dc_plane_state->gamut_remap_matrix.enable_remap = false;
> +		dc_plane_state->input_csc_color_matrix.enable_adjustment = false;
> +		return 0;
> +	}
> +
> +	drm_dbg(dev, "3x4 matrix colorop with ID: %d\n", colorop->base.id);
> +	blob = colorop_state->data;
> +	if (blob->length == sizeof(struct drm_color_ctm_3x4)) {

[Severity: Critical]
This is a pre-existing issue, but could this lead to a NULL pointer 
dereference if the data blob is missing?

If userspace configures a 3x4 matrix colorop with bypass set to false but
doesn't provide a valid data blob, the DRM core might leave colorop_state->data
as NULL. Dereferencing blob->length here without checking if blob is valid
would then crash the system. 

Should __set_dm_plane_colorop_3x4_matrix() explicitly check if blob is not NULL
before attempting to access its length?

> +		ctm = (struct drm_color_ctm_3x4 *) blob->data;
> +		__drm_ctm_3x4_to_dc_matrix(ctm, dc_plane_state->gamut_remap_matrix.matrix);
> +		dc_plane_state->gamut_remap_matrix.enable_remap = true;
> +		dc_plane_state->input_csc_color_matrix.enable_adjustment = false;
> +	} else {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609121230.1358786-1-mwen@igalia.com?part=7

Re: [PATCH v3 0/7] don't allow changes to inactive colorops

[Harry Wentland]

On 2026-06-09 07:51, Melissa Wen wrote:
> 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 bug fixes to kms_colorop and kms_properties to behave
> according to this contract [2]. The rest of the series in [1] was
> detached in [3] since there is no dependency between them.
> 
> [v1] https://lore.kernel.org/dri-devel/20260526142940.504911-1-mwen@igalia.com/
> Changes:
> - define a macro to walk in the color pipeline (Alex H.)
> - fix checkpatch warning (Alex H.)
> [v2] https://lore.kernel.org/dri-devel/20260604180457.1110110-1-mwen@igalia.com/
> Changes:
> - [Drop] drm/atomic: duplicate state of all colorops
>   If inactive colorops state are duplicated on resume, the commit will be
>   rejected. 
> - [New] Four new patches to make AMD driver match the policy of colorop
>   updates only for colorops in active color pipelines plus individual
>   colorop updates. It also tries to untangle COLOR_PIPELINE = Bypass from
>   colorop BYPASS prop = true. I think patches 3-5 can be cherry-picked and
>   applied if it looks correct for AMD, I just included them here for
>   context (for example, Sashiko reported an issue in the previous version
>   of this series).
> 
> [1] https://lore.kernel.org/dri-devel/20260519211111.228303-1-mwen@igalia.com/
> [2] https://lore.kernel.org/igt-dev/20260602211259.898147-1-mwen@igalia.com
> [3] https://lore.kernel.org/igt-dev/20260526140752.503380-1-mwen@igalia.com/
> 

I went through the series a week ago before being getting into the weeds
of other-important-work-tm. It looked fine to me then. Since I'll be off
for the next two weeks I wanted to make sure to at least provide my

Acked-by: Harry Wentland <harry.wentland@amd.com>

for the series. If you don't have RBs by mid-July I'll be happy to have
a more thorough review with the aim of giving my RBs.

Harry

> 
> Melissa
> 
> Melissa Wen (7):
>   drm/atomic: only add states of active or transient active colorops
>   drm/atomic: reject colorop update from inactive color pipeline
>   drm/amd/display: don't check colorop status if its in an inactive
>     pipeline
>   drm/amd/display: truly bypass plane colorop 3x4 matrix and hdr mult
>   drm/amd/display: make shaper bypass mode cleaner
>   drm/amd/display: fix bnld colorop bypass mode
>   drm/amd/display: allow individual colorop changes
> 
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  31 ++-
>  .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 178 +++++++-----------
>  drivers/gpu/drm/drm_atomic.c                  | 125 +++++++++++-
>  include/drm/drm_colorop.h                     |   3 +
>  4 files changed, 207 insertions(+), 130 deletions(-)
>