[PATCH v4 00/11] drm/atomic: don't allow changes to inactive colorops & other fixes

[PATCH v4 00/11] drm/atomic: don't allow changes to inactive colorops & other fixes

[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 (new version) [2]. The rest of the series in
[1] was detached in [3] and already applied. However, after a bad merge
conflict resolution the colorop-update track was removed from AMD and
this series needs it back to make the AMD part work correctly. I've
already resubmitted it [4].

I also tried to address some Sashiko's complaints on pre-existent issues
that affects the stability of this series, but not all since I want to
keep a healthy scope for reviews. AMD fixes are in this series because
of their scope, but they can be detached and applied whenever it's
convenient.

[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).

[v3] https://lore.kernel.org/dri-devel/20260609121230.1358786-1-mwen@igalia.com/
Changes:
- make drm_atomic_add_affected_colorops static and move to
  drm_atomic_helper.c (John H.)
- skip check when just duplicating state for suspend/resume persistence.
- [re-add] drm/atomic: duplicate state of all colorops to preserve all
  colorop status in a suspend/resume
- rewite commit message and better explain what's considered an active
  colorop (John H.)
- [new] drm/atomic: check if an active colorop has a blob if its type
  requires one
- drop the ternary and add just a warn_on since both current callers
  iterate planes already in the atomic state in AMD's active pipeline
  check (John H.)
- explain the reason to use commited colorop in AMD's active pipeline
  check (John H.)
- [new] drm/amd/display: don't ignore failure on blend colorop setup
- [new] drm/amd/display: distinguish colorop setup error from no colorop support

[1] https://lore.kernel.org/dri-devel/20260519211111.228303-1-mwen@igalia.com/
[2] https://lore.kernel.org/igt-dev/20260811143558.141813-1-mwen@igalia.com
[3] https://lore.kernel.org/dri-devel/20260609110420.1298352-1-mwen@igalia.com/
[4] https://lore.kernel.org/dri-devel/20260807115712.22423-1-mwen@igalia.com/

Melissa Wen (11):
  drm/atomic: only add states of active or transient active colorops
  drm/atomic: reject colorop update from inactive color pipeline
  drm/atomic: duplicate state of all colorops
  drm/atomic: check if an active colorop has a blob if its type requires one
  drm/amd/display: only check colorops of an active color pipeline
  drm/amd/display: truly bypass plane colorop 3x4 matrix and hdr mult
  drm/amd/display: make shaper bypass mode cleaner
  drm/amd/display: make blnd bypass mode clearer
  drm/amd/display: don't ignore failure on blend colorop setup
  drm/amd/display: allow individual colorop changes
  drm/amd/display: distinguish colorop setup error from no colorop support

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  33 ++-
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 206 +++++++-----------
 drivers/gpu/drm/drm_atomic.c                  | 199 ++++++++++++-----
 drivers/gpu/drm/drm_atomic_helper.c           |  52 ++++-
 include/drm/drm_atomic.h                      |   3 -
 include/drm/drm_colorop.h                     |   3 +
 6 files changed, 304 insertions(+), 192 deletions(-)

-- 
2.53.0

[PATCH v4 01/11] 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.
Make drm_atomic_add_affected_colorops() static and move to
drm_atomic_helper.c since it's only used by
drm_atomic_helper_duplicate_state() now.

Acked-by: Harry Wentland <harry.wentland@amd.com> #v3
Signed-off-by: Melissa Wen <mwen@igalia.com>

---

v2: define a macro to walk in the color pipeline (Alex H.)
v4: make drm_atomic_add_affected_colorops static and move to drm_atomic_helper.c (John H.)
---
 drivers/gpu/drm/drm_atomic.c        | 105 ++++++++++++++--------------
 drivers/gpu/drm/drm_atomic_helper.c |  43 ++++++++++++
 include/drm/drm_atomic.h            |   3 -
 include/drm/drm_colorop.h           |   3 +
 4 files changed, 100 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index e5c8ef06caed..f00df28e2051 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,62 +1722,14 @@ 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;
 }
 EXPORT_SYMBOL(drm_atomic_add_affected_planes);
 
-/**
- * drm_atomic_add_affected_colorops - add colorops for plane
- * @state: atomic state
- * @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.
- *
- * 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
- * a plane will not reduce parallelism of atomic updates.
- *
- * 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.
- */
-int
-drm_atomic_add_affected_colorops(struct drm_atomic_commit *state,
-				 struct drm_plane *plane)
-{
-	struct drm_colorop *colorop;
-	struct drm_colorop_state *colorop_state;
-
-	WARN_ON(!drm_atomic_get_new_plane_state(state, plane));
-
-	drm_dbg_atomic(plane->dev,
-		       "Adding all current colorops for [PLANE:%d:%s] to %p\n",
-		       plane->base.id, plane->name, state);
-
-	drm_for_each_colorop(colorop, plane->dev) {
-		if (colorop->plane != plane)
-			continue;
-
-		colorop_state = drm_atomic_get_colorop_state(state, colorop);
-		if (IS_ERR(colorop_state))
-			return PTR_ERR(colorop_state);
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL(drm_atomic_add_affected_colorops);
-
 /**
  * drm_atomic_check_only - check whether a given config would work
  * @state: atomic configuration to check
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 285aac3554df..917fd0594259 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3697,6 +3697,49 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_atomic_helper_shutdown);
 
+/*
+ * drm_atomic_add_affected_colorops - add colorops for plane
+ * @state: atomic state
+ * @plane: DRM plane
+ *
+ * This function walks the current configuration and adds 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
+ * a plane will not reduce parallelism of atomic updates.
+ *
+ * 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_affected_colorops(struct drm_atomic_commit *state,
+				 struct drm_plane *plane)
+{
+	struct drm_colorop *colorop;
+	struct drm_colorop_state *colorop_state;
+
+	WARN_ON(!drm_atomic_get_new_plane_state(state, plane));
+
+	drm_dbg_atomic(plane->dev,
+		       "Adding all current colorops for [PLANE:%d:%s] to %p\n",
+		       plane->base.id, plane->name, state);
+
+	drm_for_each_colorop(colorop, plane->dev) {
+		if (colorop->plane != plane)
+			continue;
+
+		colorop_state = drm_atomic_get_colorop_state(state, colorop);
+		if (IS_ERR(colorop_state))
+			return PTR_ERR(colorop_state);
+	}
+
+	return 0;
+}
+
 /**
  * drm_atomic_helper_duplicate_state - duplicate an atomic state object
  * @dev: DRM device
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 88087910ab1a..0597041bde04 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -921,9 +921,6 @@ drm_atomic_add_affected_connectors(struct drm_atomic_commit *state,
 int __must_check
 drm_atomic_add_affected_planes(struct drm_atomic_commit *state,
 			       struct drm_crtc *crtc);
-int __must_check
-drm_atomic_add_affected_colorops(struct drm_atomic_commit *state,
-				 struct drm_plane *plane);
 
 int __must_check drm_atomic_check_only(struct drm_atomic_commit *state);
 int __must_check drm_atomic_commit(struct drm_atomic_commit *state);
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 224fae40ed2b..568dfcc4dd63 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -457,6 +457,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

[PATCH v4 02/11] drm/atomic: reject colorop update from inactive color pipeline

[Melissa Wen]

A colorop can only be changed while it is part of an active color
pipeline, or in the same commit that activates or deactivates that
pipeline. Enforce this contract by checking that the colorop belongs to
the color pipeline of a plane in its current, new or old state, and
rejecting the state change otherwise. Do the check in
drm_atomic_check_only() rather than when the colorop property is set, so
it doesn't depend on the order userspace sets properties within a
commit.

Link: https://lore.kernel.org/dri-devel/20260519211111.228303-1-mwen@igalia.com/
Suggested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Acked-by: Harry Wentland <harry.wentland@amd.com> #v3
Signed-off-by: Melissa Wen <mwen@igalia.com>

---
v4:
- skip check when just duplicating state for suspend/resume persistence
- rewrite commit message and better explain the change (John H.)
---
 drivers/gpu/drm/drm_atomic.c | 76 ++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f00df28e2051..86e4348cad58 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -943,6 +943,71 @@ 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.
+ *
+ * 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 changes to colorop in the old plane
+ * state's pipeline are accepted even though it won't drive hardware updates.
+ *
+ * Skip this check for duplicated state, since all colorop states must persist
+ * in suspend/resume regardless of whether it belongs to an active color
+ * pipeline or not.
+ *
+ * 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;
+
+	if (state->duplicated)
+		return 0;
+
+	/* Not a plane colorop */
+	if (!plane)
+		return 0;
+
+	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) {
+		drm_for_each_colorop_in_pipeline(colorop, plane->state->color_pipeline)
+			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 */
+	drm_for_each_colorop_in_pipeline(colorop, new_plane_state->color_pipeline)
+		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 */
+	drm_for_each_colorop_in_pipeline(colorop, old_plane_state->color_pipeline)
+		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)
 {
@@ -1748,6 +1813,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;
@@ -1764,6 +1831,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

[PATCH v4 03/11] 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, preserves
the values they had 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, regardless
of whether color pipeline is active. Depends on skipping
drm_atomic_colorop_check() for duplicated state done in previous commit.

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 917fd0594259..11c67bb808f9 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3801,12 +3801,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 v4 04/11] drm/atomic: check if an active colorop has a blob if its type requires one

[Melissa Wen]

If colorop TYPE requires a data blob, userspace have to set a blob
whenever enables this colorop, i.e. when setting this colorop bypass
property to false.

Fixes: e5719e7f1900 ("drm/colorop: Add 3x4 CTM type")
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 86e4348cad58..7b9d52cf87d0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -947,8 +947,13 @@ drm_atomic_add_pipeline_colorops(struct drm_atomic_commit *state,
  * 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
+ * Check that a colorop whose TYPE requires a data blob has one when it's
+ * enabled, i.e. userspace can't clear (or never set) the DATA property while
+ * taking the colorop out of bypass, since drivers would have nothing to
+ * program.
+ *
+ * Also 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.
  *
  * Userspace is allowed to finalize colorop's settings in the same commit that
@@ -972,6 +977,19 @@ static int drm_atomic_colorop_check(const struct drm_colorop_state *new_colorop_
 	if (state->duplicated)
 		return 0;
 
+	/*
+	 * Reject if colorop TYPE requires a DATA but set bypass to false and
+	 * no blob submitted
+	 */
+	if (new_colorop_state->colorop->data_property &&
+	    !new_colorop_state->bypass && !new_colorop_state->data) {
+		drm_dbg_atomic(new_colorop_state->colorop->dev,
+			       "[COLOROP:%d:%d] enabled without a DATA blob\n",
+			       new_colorop_state->colorop->base.id,
+			       new_colorop_state->colorop->type);
+		return -EINVAL;
+	}
+
 	/* Not a plane colorop */
 	if (!plane)
 		return 0;
-- 
2.53.0

[PATCH v4 05/11] drm/amd/display: only check colorops of an active color pipeline

[Melissa Wen]

dm_plane_color_pipeline_active() iterates every colorop state in the
atomic commit, so colorops of a pipeline that userspace deactivated via
plane COLOR_PIPELINE are still taken into account, even though their
BYPASS property is irrelevant once the pipeline is off. Walk the color
pipeline of the plane state under evaluation instead, falling back to
the committed colorop state when a colorop isn't in the atomic commit.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: d3a549f4df78 ("drm/amd/display: Use overlay cursor when color pipeline is active")
Acked-by: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>

---

v4:
- drop the ternary and add just a warn_on since both current callers
  iterate planes already in the atomic state. (John H.)
- explain the reason to use commited colorop (John H.)
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 ++++++++++++++-----
 1 file changed, 24 insertions(+), 9 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 d0e612371c8f..384541b9ac9c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6551,9 +6551,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 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.
  */
@@ -6561,18 +6561,33 @@ static bool dm_plane_color_pipeline_active(struct drm_atomic_commit *state,
 					   struct drm_plane *plane,
 					   bool use_old)
 {
+	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;
-	struct drm_colorop_state *old_colorop_state, *new_colorop_state;
-	int i;
+	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;
+	if (drm_WARN_ON(plane->dev, !plane_state))
+		return false;
 
-		if (cstate->colorop->plane != plane)
-			continue;
+	/*
+	 * A commit may change only some colorops of a pipeline, and only those
+	 * have old and new states here. Telling whether the pipeline modifies
+	 * pixels requires every colorop of the selected pipeline, so fall back
+	 * to the committed state of the untouched ones; it's both their old
+	 * and new state.
+	 */
+	drm_for_each_colorop_in_pipeline(colorop, plane_state->color_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

[PATCH v4 06/11] 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.

Acked-by: Harry Wentland <harry.wentland@amd.com>
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 357c7c5c85cf..450a1469d0fd 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
@@ -1595,7 +1595,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)) {
@@ -1634,9 +1640,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

[PATCH v4 07/11] drm/amd/display: make shaper bypass mode cleaner

[Melissa Wen]

Start shaper transfer 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. It's aligned with current blend setup.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_color.c   | 15 +++++----------
 1 file changed, 5 insertions(+), 10 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 450a1469d0fd..ca9e43e81edf 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
@@ -1666,10 +1666,12 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
 	struct dc_transfer_func *tf = &dc_plane_state->cm.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;
+	dc_plane_state->cm.flags.bits.shaper_enable = 0;
+
 	/* 1D Curve - SHAPER TF: find state */
 	old_colorop = colorop;
 	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
@@ -1703,7 +1705,7 @@ __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;
+		dc_plane_state->cm.flags.bits.shaper_enable = 1;
 	}
 
 	if (lut_state && !lut_state->bypass) {
@@ -1719,17 +1721,10 @@ __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;
+			dc_plane_state->cm.flags.bits.shaper_enable = 1;
 		}
 	}
 
-	if (!enabled) {
-		tf->type = TF_TYPE_BYPASS;
-		dc_plane_state->cm.flags.bits.shaper_enable = 0;
-	} else {
-		dc_plane_state->cm.flags.bits.shaper_enable = 1;
-	}
-
 	return 0;
 }
 
-- 
2.53.0

[PATCH v4 08/11] drm/amd/display: make blnd bypass mode clearer

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

Acked-by: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c | 1 +
 1 file changed, 1 insertion(+)

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 ca9e43e81edf..ad67106c6435 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
@@ -1837,6 +1837,7 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
 	uint32_t blend_size = 0;
 	int i = 0;
 
+	tf->type = TF_TYPE_BYPASS;
 	dc_plane_state->cm.flags.bits.blend_enable = 0;
 
 	/* 1D Curve - BLND TF: find state */
-- 
2.53.0

[PATCH v4 09/11] drm/amd/display: don't ignore failure on blend colorop setup

[Melissa Wen]

__set_input_tf_32() can fail on ENOMEM and let the blend transfer
function setup in an unstable state. Check its return value and only
enable blend if transfer function was successfully configured.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: 5ed78b44e4e6 ("drm/amd/display: add shaper and blend colorops for 1D Curve Custom LUT")
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_color.c   | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 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 ad67106c6435..c528daefac5e 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
@@ -1835,7 +1835,7 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
 	const struct drm_color_lut32 *blend_lut = NULL;
 	struct drm_device *dev = colorop->dev;
 	uint32_t blend_size = 0;
-	int i = 0;
+	int i = 0, ret;
 
 	tf->type = TF_TYPE_BYPASS;
 	dc_plane_state->cm.flags.bits.blend_enable = 0;
@@ -1870,8 +1870,10 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
 		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
 		tf->tf = default_tf = amdgpu_colorop_tf_to_dc_tf(tf_state->curve_1d_type);
 		tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
+		ret = __set_input_tf_32(NULL, tf, blend_lut, blend_size);
+		if (ret)
+			return ret;
 		dc_plane_state->cm.flags.bits.blend_enable = 1;
-		__set_input_tf_32(NULL, tf, blend_lut, blend_size);
 	}
 
 	if (lut_state && !lut_state->bypass) {
@@ -1879,13 +1881,16 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
 		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
 		tf->tf = default_tf;
 		tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
-		dc_plane_state->cm.flags.bits.blend_enable = 1;
 		blend_lut = __extract_blob_lut32(lut_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 == lut_colorop->size)
-			__set_input_tf_32(NULL, tf, blend_lut, blend_size);
+		if (blend_size == lut_colorop->size) {
+			ret = __set_input_tf_32(NULL, tf, blend_lut, blend_size);
+			if (ret)
+				return ret;
+			dc_plane_state->cm.flags.bits.blend_enable = 1;
+		}
 	}
 
 	return 0;
-- 
2.53.0

[PATCH v4 10/11] 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
silently falls back to the legacy color path, 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.

Fixes: 9ba25915efba ("drm/amd/display: Add support for sRGB EOTF in DEGAM block")
Acked-by: Harry Wentland <harry.wentland@amd.com> #v3
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 183 +++++++-----------
 1 file changed, 66 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 c528daefac5e..f6a2af5d2e96 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
@@ -1550,24 +1550,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);
 }
@@ -1577,43 +1566,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;
@@ -1624,29 +1607,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;
@@ -1657,8 +1634,6 @@ __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 *new_colorop_state;
 	struct drm_colorop_state *tf_state = NULL, *lut_state = NULL;
 	struct drm_atomic_commit *state = plane_state->state;
 	struct drm_colorop *lut_colorop;
@@ -1667,38 +1642,29 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
 	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;
 	dc_plane_state->cm.flags.bits.shaper_enable = 0;
 
 	/* 1D Curve - SHAPER TF: find state */
-	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)) {
-			tf_state = new_colorop_state;
-			break;
-		}
-	}
+	tf_state = drm_atomic_get_new_colorop_state(state, colorop);
+	if (!tf_state)
+		tf_state = colorop->state;
 
 	/* 1D LUT - SHAPER LUT: find state */
-	lut_colorop = old_colorop->next;
+	lut_colorop = colorop->next;
 	if (!lut_colorop) {
 		drm_dbg(dev, "no Shaper LUT colorop found\n");
 		return -EINVAL;
 	}
 
-	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
-		if (new_colorop_state->colorop == lut_colorop &&
-		    new_colorop_state->colorop->type == DRM_COLOROP_1D_LUT) {
-			lut_state = new_colorop_state;
-			break;
-		}
-	}
+	lut_state = drm_atomic_get_new_colorop_state(state, lut_colorop);
+	if (!lut_state)
+		lut_state = lut_colorop->state;
 
-	if (tf_state && !tf_state->bypass) {
-		drm_dbg(dev, "Shaper TF colorop with ID: %d\n", old_colorop->base.id);
+	if (!tf_state->bypass) {
+		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(tf_state->curve_1d_type);
 		tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
@@ -1708,7 +1674,7 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
 		dc_plane_state->cm.flags.bits.shaper_enable = 1;
 	}
 
-	if (lut_state && !lut_state->bypass) {
+	if (!lut_state->bypass) {
 		drm_dbg(dev, "Shaper LUT colorop with ID: %d\n", lut_colorop->base.id);
 		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
 		tf->tf = default_tf;
@@ -1765,8 +1731,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->cm.shaper_func;
 	struct drm_atomic_commit *state = plane_state->state;
 	const struct amdgpu_device *adev = drm_to_adev(colorop->dev);
@@ -1774,19 +1739,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;
@@ -1825,8 +1785,6 @@ __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 *new_colorop_state;
 	struct drm_colorop_state *tf_state = NULL, *lut_state = NULL;
 	struct drm_atomic_commit *state = plane_state->state;
 	struct drm_colorop *lut_colorop;
@@ -1835,38 +1793,29 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
 	const struct drm_color_lut32 *blend_lut = NULL;
 	struct drm_device *dev = colorop->dev;
 	uint32_t blend_size = 0;
-	int i = 0, ret;
+	int ret;
 
 	tf->type = TF_TYPE_BYPASS;
 	dc_plane_state->cm.flags.bits.blend_enable = 0;
 
 	/* 1D Curve - BLND TF: find state */
-	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)) {
-			tf_state = new_colorop_state;
-			break;
-		}
-	}
+	tf_state = drm_atomic_get_new_colorop_state(state, colorop);
+	if (!tf_state)
+		tf_state = colorop->state;
 
 	/* 1D LUT - BLND LUT: find state */
-	lut_colorop = old_colorop->next;
+	lut_colorop = colorop->next;
 	if (!lut_colorop) {
 		drm_dbg(dev, "no Blend LUT colorop found\n");
 		return -EINVAL;
 	}
 
-	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
-		if (new_colorop_state->colorop == lut_colorop &&
-		    new_colorop_state->colorop->type == DRM_COLOROP_1D_LUT) {
-			lut_state = new_colorop_state;
-			break;
-		}
-	}
+	lut_state = drm_atomic_get_new_colorop_state(state, lut_colorop);
+	if (!lut_state)
+		lut_state = lut_colorop->state;
 
-	if (tf_state && !tf_state->bypass) {
-		drm_dbg(dev, "Blend TF colorop with ID: %d\n", old_colorop->base.id);
+	if (!tf_state->bypass) {
+		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(tf_state->curve_1d_type);
 		tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
@@ -1876,7 +1825,7 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
 		dc_plane_state->cm.flags.bits.blend_enable = 1;
 	}
 
-	if (lut_state && !lut_state->bypass) {
+	if (!lut_state->bypass) {
 		drm_dbg(dev, "Blend LUT colorop with ID: %d\n", lut_colorop->base.id);
 		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
 		tf->tf = default_tf;
-- 
2.53.0

[PATCH v4 11/11] drm/amd/display: distinguish colorop setup error from no colorop support

[Melissa Wen]

amdgpu_dm_plane_set_colorop_properties() returns -EINVAL both when the
plane has no color pipeline selected, where falling back to the legacy
color properties is correct, and when programming an active pipeline
fails, so the caller treats every failure as the former and silently
programs the plane from the legacy properties, leaving it in a mixed
state and userspace with no error. Check if plane_state->color_pipeline
is set instead, so the legacy path is only taken when no pipeline is set
and any other failure is propagated out of the atomic check.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c | 8 ++++----
 1 file changed, 4 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 f6a2af5d2e96..f2731989499c 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
@@ -1906,10 +1906,10 @@ amdgpu_dm_plane_set_colorop_properties(struct drm_plane_state *plane_state,
 	bool has_3dlut = adev->dm.dc->caps.color.dpp.hw_3d_lut || adev->dm.dc->caps.color.mpc.preblend;
 	int ret;
 
-	/* 1D Curve - DEGAM TF */
-	if (!colorop)
+	if (drm_WARN_ON(dev, !colorop))
 		return -EINVAL;
 
+	/* 1D Curve - DEGAM TF */
 	ret = __set_dm_plane_colorop_degamma(plane_state, dc_plane_state, colorop);
 	if (ret)
 		return ret;
@@ -2080,8 +2080,8 @@ int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
 		dc_plane_state->input_csc_color_matrix.enable_adjustment = false;
 	}
 
-	if (!amdgpu_dm_plane_set_colorop_properties(plane_state, dc_plane_state))
-		return 0;
+	if (plane_state->color_pipeline)
+		return amdgpu_dm_plane_set_colorop_properties(plane_state, dc_plane_state);
 
 	return amdgpu_dm_plane_set_color_properties(plane_state, dc_plane_state);
 }
-- 
2.53.0

✗ Fi.CI.BUILD: failure for drm/atomic: don't allow changes to inactive colorops & other fixes

[Patchwork]

== Series Details ==

Series: drm/atomic: don't allow changes to inactive colorops & other fixes
URL   : https://patchwork.freedesktop.org/series/172028/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/172028/revisions/1/mbox/ not applied
Applying: drm/atomic: only add states of active or transient active colorops
Applying: drm/atomic: reject colorop update from inactive color pipeline
Applying: drm/atomic: duplicate state of all colorops
Applying: drm/atomic: check if an active colorop has a blob if its type requires one
Applying: drm/amd/display: only check colorops of an active color pipeline
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 drm/amd/display: only check colorops of an active color pipeline
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced

Re: [PATCH v4 01/11] drm/atomic: only add states of active or transient active colorops

[Harry Wentland]

On 2026-08-11 12:45, 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.
> Make drm_atomic_add_affected_colorops() static and move to
> drm_atomic_helper.c since it's only used by
> drm_atomic_helper_duplicate_state() now.
> 

There is an amdgpu_dm bit in drm-misc-next now that needs it. Would it 
be possible to treat it the same as drm_atomic_add_affected_planes?

There's a chance that Patch 3 in this series fixes it but it'll be a 
while since I'll have a chance to try it.

Harry

> Acked-by: Harry Wentland <harry.wentland@amd.com> #v3
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> 
> ---
> 
> v2: define a macro to walk in the color pipeline (Alex H.)
> v4: make drm_atomic_add_affected_colorops static and move to drm_atomic_helper.c (John H.)
> ---
>   drivers/gpu/drm/drm_atomic.c        | 105 ++++++++++++++--------------
>   drivers/gpu/drm/drm_atomic_helper.c |  43 ++++++++++++
>   include/drm/drm_atomic.h            |   3 -
>   include/drm/drm_colorop.h           |   3 +
>   4 files changed, 100 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index e5c8ef06caed..f00df28e2051 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,62 +1722,14 @@ 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;
>   }
>   EXPORT_SYMBOL(drm_atomic_add_affected_planes);
>   
> -/**
> - * drm_atomic_add_affected_colorops - add colorops for plane
> - * @state: atomic state
> - * @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.
> - *
> - * 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
> - * a plane will not reduce parallelism of atomic updates.
> - *
> - * 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.
> - */
> -int
> -drm_atomic_add_affected_colorops(struct drm_atomic_commit *state,
> -				 struct drm_plane *plane)
> -{
> -	struct drm_colorop *colorop;
> -	struct drm_colorop_state *colorop_state;
> -
> -	WARN_ON(!drm_atomic_get_new_plane_state(state, plane));
> -
> -	drm_dbg_atomic(plane->dev,
> -		       "Adding all current colorops for [PLANE:%d:%s] to %p\n",
> -		       plane->base.id, plane->name, state);
> -
> -	drm_for_each_colorop(colorop, plane->dev) {
> -		if (colorop->plane != plane)
> -			continue;
> -
> -		colorop_state = drm_atomic_get_colorop_state(state, colorop);
> -		if (IS_ERR(colorop_state))
> -			return PTR_ERR(colorop_state);
> -	}
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(drm_atomic_add_affected_colorops);
> -
>   /**
>    * drm_atomic_check_only - check whether a given config would work
>    * @state: atomic configuration to check
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 285aac3554df..917fd0594259 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3697,6 +3697,49 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
>   }
>   EXPORT_SYMBOL(drm_atomic_helper_shutdown);
>   
> +/*
> + * drm_atomic_add_affected_colorops - add colorops for plane
> + * @state: atomic state
> + * @plane: DRM plane
> + *
> + * This function walks the current configuration and adds 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
> + * a plane will not reduce parallelism of atomic updates.
> + *
> + * 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_affected_colorops(struct drm_atomic_commit *state,
> +				 struct drm_plane *plane)
> +{
> +	struct drm_colorop *colorop;
> +	struct drm_colorop_state *colorop_state;
> +
> +	WARN_ON(!drm_atomic_get_new_plane_state(state, plane));
> +
> +	drm_dbg_atomic(plane->dev,
> +		       "Adding all current colorops for [PLANE:%d:%s] to %p\n",
> +		       plane->base.id, plane->name, state);
> +
> +	drm_for_each_colorop(colorop, plane->dev) {
> +		if (colorop->plane != plane)
> +			continue;
> +
> +		colorop_state = drm_atomic_get_colorop_state(state, colorop);
> +		if (IS_ERR(colorop_state))
> +			return PTR_ERR(colorop_state);
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * drm_atomic_helper_duplicate_state - duplicate an atomic state object
>    * @dev: DRM device
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 88087910ab1a..0597041bde04 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -921,9 +921,6 @@ drm_atomic_add_affected_connectors(struct drm_atomic_commit *state,
>   int __must_check
>   drm_atomic_add_affected_planes(struct drm_atomic_commit *state,
>   			       struct drm_crtc *crtc);
> -int __must_check
> -drm_atomic_add_affected_colorops(struct drm_atomic_commit *state,
> -				 struct drm_plane *plane);
>   
>   int __must_check drm_atomic_check_only(struct drm_atomic_commit *state);
>   int __must_check drm_atomic_commit(struct drm_atomic_commit *state);
> diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
> index 224fae40ed2b..568dfcc4dd63 100644
> --- a/include/drm/drm_colorop.h
> +++ b/include/drm/drm_colorop.h
> @@ -457,6 +457,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 v4 02/11] drm/atomic: reject colorop update from inactive color pipeline

[Harry Wentland]

On 2026-08-11 12:45, Melissa Wen wrote:
> A colorop can only be changed while it is part of an active color
> pipeline, or in the same commit that activates or deactivates that
> pipeline. Enforce this contract by checking that the colorop belongs to
> the color pipeline of a plane in its current, new or old state, and
> rejecting the state change otherwise. Do the check in
> drm_atomic_check_only() rather than when the colorop property is set, so
> it doesn't depend on the order userspace sets properties within a
> commit.
> 
> Link: https://lore.kernel.org/dri-devel/20260519211111.228303-1-mwen@igalia.com/
> Suggested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> Acked-by: Harry Wentland <harry.wentland@amd.com> #v3
> Signed-off-by: Melissa Wen <mwen@igalia.com>

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

Harry

> 
> ---
> v4:
> - skip check when just duplicating state for suspend/resume persistence
> - rewrite commit message and better explain the change (John H.)
> ---
>   drivers/gpu/drm/drm_atomic.c | 76 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f00df28e2051..86e4348cad58 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -943,6 +943,71 @@ 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.
> + *
> + * 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 changes to colorop in the old plane
> + * state's pipeline are accepted even though it won't drive hardware updates.
> + *
> + * Skip this check for duplicated state, since all colorop states must persist
> + * in suspend/resume regardless of whether it belongs to an active color
> + * pipeline or not.
> + *
> + * 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;
> +
> +	if (state->duplicated)
> +		return 0;
> +
> +	/* Not a plane colorop */
> +	if (!plane)
> +		return 0;
> +
> +	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) {
> +		drm_for_each_colorop_in_pipeline(colorop, plane->state->color_pipeline)
> +			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 */
> +	drm_for_each_colorop_in_pipeline(colorop, new_plane_state->color_pipeline)
> +		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 */
> +	drm_for_each_colorop_in_pipeline(colorop, old_plane_state->color_pipeline)
> +		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)
>   {
> @@ -1748,6 +1813,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;
> @@ -1764,6 +1831,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 v4 03/11] drm/atomic: duplicate state of all colorops

[Harry Wentland]

On 2026-08-11 12:45, Melissa Wen wrote:
> Userspace expects that colorop settings of an inactive color pipeline
> persist, so that, when the color pipeline is activated again, preserves
> the values they had 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, regardless
> of whether color pipeline is active. Depends on skipping
> drm_atomic_colorop_check() for duplicated state done in previous commit.
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>

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

Harry

> ---
>   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 917fd0594259..11c67bb808f9 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3801,12 +3801,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);

Re: [PATCH v4 04/11] drm/atomic: check if an active colorop has a blob if its type requires one

[Harry Wentland]

On 2026-08-11 12:45, Melissa Wen wrote:
> If colorop TYPE requires a data blob, userspace have to set a blob
> whenever enables this colorop, i.e. when setting this colorop bypass
> property to false.
> 
> Fixes: e5719e7f1900 ("drm/colorop: Add 3x4 CTM type")
> Signed-off-by: Melissa Wen <mwen@igalia.com>

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

Harry

> ---
>   drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 86e4348cad58..7b9d52cf87d0 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -947,8 +947,13 @@ drm_atomic_add_pipeline_colorops(struct drm_atomic_commit *state,
>    * 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
> + * Check that a colorop whose TYPE requires a data blob has one when it's
> + * enabled, i.e. userspace can't clear (or never set) the DATA property while
> + * taking the colorop out of bypass, since drivers would have nothing to
> + * program.
> + *
> + * Also 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.
>    *
>    * Userspace is allowed to finalize colorop's settings in the same commit that
> @@ -972,6 +977,19 @@ static int drm_atomic_colorop_check(const struct drm_colorop_state *new_colorop_
>   	if (state->duplicated)
>   		return 0;
>   
> +	/*
> +	 * Reject if colorop TYPE requires a DATA but set bypass to false and
> +	 * no blob submitted
> +	 */
> +	if (new_colorop_state->colorop->data_property &&
> +	    !new_colorop_state->bypass && !new_colorop_state->data) {
> +		drm_dbg_atomic(new_colorop_state->colorop->dev,
> +			       "[COLOROP:%d:%d] enabled without a DATA blob\n",
> +			       new_colorop_state->colorop->base.id,
> +			       new_colorop_state->colorop->type);
> +		return -EINVAL;
> +	}
> +
>   	/* Not a plane colorop */
>   	if (!plane)
>   		return 0;

Re: [PATCH v4 05/11] drm/amd/display: only check colorops of an active color pipeline

[Harry Wentland]

On 2026-08-11 12:45, Melissa Wen wrote:
> dm_plane_color_pipeline_active() iterates every colorop state in the
> atomic commit, so colorops of a pipeline that userspace deactivated via
> plane COLOR_PIPELINE are still taken into account, even though their
> BYPASS property is irrelevant once the pipeline is off. Walk the color
> pipeline of the plane state under evaluation instead, falling back to
> the committed colorop state when a colorop isn't in the atomic commit.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Fixes: d3a549f4df78 ("drm/amd/display: Use overlay cursor when color pipeline is active")
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> 

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

Harry

> ---
> 
> v4:
> - drop the ternary and add just a warn_on since both current callers
>    iterate planes already in the atomic state. (John H.)
> - explain the reason to use commited colorop (John H.)
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 ++++++++++++++-----
>   1 file changed, 24 insertions(+), 9 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 d0e612371c8f..384541b9ac9c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6551,9 +6551,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 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.
>    */
> @@ -6561,18 +6561,33 @@ static bool dm_plane_color_pipeline_active(struct drm_atomic_commit *state,
>   					   struct drm_plane *plane,
>   					   bool use_old)
>   {
> +	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;
> -	struct drm_colorop_state *old_colorop_state, *new_colorop_state;
> -	int i;
> +	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;
> +	if (drm_WARN_ON(plane->dev, !plane_state))
> +		return false;
>   
> -		if (cstate->colorop->plane != plane)
> -			continue;
> +	/*
> +	 * A commit may change only some colorops of a pipeline, and only those
> +	 * have old and new states here. Telling whether the pipeline modifies
> +	 * pixels requires every colorop of the selected pipeline, so fall back
> +	 * to the committed state of the untouched ones; it's both their old
> +	 * and new state.
> +	 */
> +	drm_for_each_colorop_in_pipeline(colorop, plane_state->color_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;
>   }
>   

Re: [PATCH v4 06/11] drm/amd/display: truly bypass plane colorop 3x4 matrix and hdr mult

[Harry Wentland]

On 2026-08-11 12:45, Melissa Wen wrote:
> 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.
> 
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>

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

Harry

> ---
>   .../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 357c7c5c85cf..450a1469d0fd 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
> @@ -1595,7 +1595,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)) {
> @@ -1634,9 +1640,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;

Re: [PATCH v4 07/11] drm/amd/display: make shaper bypass mode cleaner

[Harry Wentland]

On 2026-08-11 12:45, Melissa Wen wrote:
> Start shaper transfer 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. It's aligned with current blend setup.
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>

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

Harry

> ---
>   .../drm/amd/display/amdgpu_dm/amdgpu_dm_color.c   | 15 +++++----------
>   1 file changed, 5 insertions(+), 10 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 450a1469d0fd..ca9e43e81edf 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
> @@ -1666,10 +1666,12 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
>   	struct dc_transfer_func *tf = &dc_plane_state->cm.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;
> +	dc_plane_state->cm.flags.bits.shaper_enable = 0;
> +
>   	/* 1D Curve - SHAPER TF: find state */
>   	old_colorop = colorop;
>   	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
> @@ -1703,7 +1705,7 @@ __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;
> +		dc_plane_state->cm.flags.bits.shaper_enable = 1;
>   	}
>   
>   	if (lut_state && !lut_state->bypass) {
> @@ -1719,17 +1721,10 @@ __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;
> +			dc_plane_state->cm.flags.bits.shaper_enable = 1;
>   		}
>   	}
>   
> -	if (!enabled) {
> -		tf->type = TF_TYPE_BYPASS;
> -		dc_plane_state->cm.flags.bits.shaper_enable = 0;
> -	} else {
> -		dc_plane_state->cm.flags.bits.shaper_enable = 1;
> -	}
> -
>   	return 0;
>   }
>   

Re: [PATCH v4 08/11] drm/amd/display: make blnd bypass mode clearer

[Harry Wentland]

On 2026-08-11 12:45, Melissa Wen wrote:
> 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.
> 
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>

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

Harry

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> 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 ca9e43e81edf..ad67106c6435 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
> @@ -1837,6 +1837,7 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
>   	uint32_t blend_size = 0;
>   	int i = 0;
>   
> +	tf->type = TF_TYPE_BYPASS;
>   	dc_plane_state->cm.flags.bits.blend_enable = 0;
>   
>   	/* 1D Curve - BLND TF: find state */

Re: [PATCH v4 09/11] drm/amd/display: don't ignore failure on blend colorop setup

[Harry Wentland]

On 2026-08-11 12:45, Melissa Wen wrote:
> __set_input_tf_32() can fail on ENOMEM and let the blend transfer
> function setup in an unstable state. Check its return value and only
> enable blend if transfer function was successfully configured.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Fixes: 5ed78b44e4e6 ("drm/amd/display: add shaper and blend colorops for 1D Curve Custom LUT")
> Signed-off-by: Melissa Wen <mwen@igalia.com>

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

Harry

> ---
>   .../drm/amd/display/amdgpu_dm/amdgpu_dm_color.c   | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 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 ad67106c6435..c528daefac5e 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
> @@ -1835,7 +1835,7 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
>   	const struct drm_color_lut32 *blend_lut = NULL;
>   	struct drm_device *dev = colorop->dev;
>   	uint32_t blend_size = 0;
> -	int i = 0;
> +	int i = 0, ret;
>   
>   	tf->type = TF_TYPE_BYPASS;
>   	dc_plane_state->cm.flags.bits.blend_enable = 0;
> @@ -1870,8 +1870,10 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
>   		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
>   		tf->tf = default_tf = amdgpu_colorop_tf_to_dc_tf(tf_state->curve_1d_type);
>   		tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
> +		ret = __set_input_tf_32(NULL, tf, blend_lut, blend_size);
> +		if (ret)
> +			return ret;
>   		dc_plane_state->cm.flags.bits.blend_enable = 1;
> -		__set_input_tf_32(NULL, tf, blend_lut, blend_size);
>   	}
>   
>   	if (lut_state && !lut_state->bypass) {
> @@ -1879,13 +1881,16 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
>   		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
>   		tf->tf = default_tf;
>   		tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
> -		dc_plane_state->cm.flags.bits.blend_enable = 1;
>   		blend_lut = __extract_blob_lut32(lut_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 == lut_colorop->size)
> -			__set_input_tf_32(NULL, tf, blend_lut, blend_size);
> +		if (blend_size == lut_colorop->size) {
> +			ret = __set_input_tf_32(NULL, tf, blend_lut, blend_size);
> +			if (ret)
> +				return ret;
> +			dc_plane_state->cm.flags.bits.blend_enable = 1;
> +		}
>   	}
>   
>   	return 0;

Re: [PATCH v4 11/11] drm/amd/display: distinguish colorop setup error from no colorop support

[Harry Wentland]

On 2026-08-11 12:46, Melissa Wen wrote:
> amdgpu_dm_plane_set_colorop_properties() returns -EINVAL both when the
> plane has no color pipeline selected, where falling back to the legacy
> color properties is correct, and when programming an active pipeline
> fails, so the caller treats every failure as the former and silently
> programs the plane from the legacy properties, leaving it in a mixed
> state and userspace with no error. Check if plane_state->color_pipeline
> is set instead, so the legacy path is only taken when no pipeline is set
> and any other failure is propagated out of the atomic check.
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>

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

Harry

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c | 8 ++++----
>   1 file changed, 4 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 f6a2af5d2e96..f2731989499c 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
> @@ -1906,10 +1906,10 @@ amdgpu_dm_plane_set_colorop_properties(struct drm_plane_state *plane_state,
>   	bool has_3dlut = adev->dm.dc->caps.color.dpp.hw_3d_lut || adev->dm.dc->caps.color.mpc.preblend;
>   	int ret;
>   
> -	/* 1D Curve - DEGAM TF */
> -	if (!colorop)
> +	if (drm_WARN_ON(dev, !colorop))
>   		return -EINVAL;
>   
> +	/* 1D Curve - DEGAM TF */
>   	ret = __set_dm_plane_colorop_degamma(plane_state, dc_plane_state, colorop);
>   	if (ret)
>   		return ret;
> @@ -2080,8 +2080,8 @@ int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
>   		dc_plane_state->input_csc_color_matrix.enable_adjustment = false;
>   	}
>   
> -	if (!amdgpu_dm_plane_set_colorop_properties(plane_state, dc_plane_state))
> -		return 0;
> +	if (plane_state->color_pipeline)
> +		return amdgpu_dm_plane_set_colorop_properties(plane_state, dc_plane_state);
>   
>   	return amdgpu_dm_plane_set_color_properties(plane_state, dc_plane_state);
>   }

Re: [PATCH v4 10/11] drm/amd/display: allow individual colorop changes

[Harry Wentland]

On 2026-08-11 12:45, Melissa Wen wrote:
> 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
> silently falls back to the legacy color path, 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.
> 

This probably needs an update for __set_dm_plane_colorop_fixed_matrix as 
well.

> Fixes: 9ba25915efba ("drm/amd/display: Add support for sRGB EOTF in DEGAM block")
> Acked-by: Harry Wentland <harry.wentland@amd.com> #v3
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 183 +++++++-----------
>   1 file changed, 66 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 c528daefac5e..f6a2af5d2e96 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
> @@ -1550,24 +1550,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);
>   }
> @@ -1577,43 +1566,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;
> @@ -1624,29 +1607,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;

dm_test_colorop_multiplier_no_match needs to be updated to now expect
-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;
> @@ -1657,8 +1634,6 @@ __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 *new_colorop_state;
>   	struct drm_colorop_state *tf_state = NULL, *lut_state = NULL;
>   	struct drm_atomic_commit *state = plane_state->state;
>   	struct drm_colorop *lut_colorop;
> @@ -1667,38 +1642,29 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
>   	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;
>   	dc_plane_state->cm.flags.bits.shaper_enable = 0;
>   
>   	/* 1D Curve - SHAPER TF: find state */
> -	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)) {
> -			tf_state = new_colorop_state;
> -			break;
> -		}
> -	}
> +	tf_state = drm_atomic_get_new_colorop_state(state, colorop);
> +	if (!tf_state)
> +		tf_state = colorop->state;
>   
>   	/* 1D LUT - SHAPER LUT: find state */
> -	lut_colorop = old_colorop->next;
> +	lut_colorop = colorop->next;
>   	if (!lut_colorop) {
>   		drm_dbg(dev, "no Shaper LUT colorop found\n");
>   		return -EINVAL;
>   	}
>   
> -	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
> -		if (new_colorop_state->colorop == lut_colorop &&
> -		    new_colorop_state->colorop->type == DRM_COLOROP_1D_LUT) {

We control the pipeline creation, so this should never not be 
DRM_COLOROP_1D_LUT but it might make sense to still check that for 
sanity, in case someone goes and messes with pipeline creation.

Same for the blend LUT below.

> -			lut_state = new_colorop_state;
> -			break;
> -		}
> -	}
> +	lut_state = drm_atomic_get_new_colorop_state(state, lut_colorop);
> +	if (!lut_state)
> +		lut_state = lut_colorop->state;
>   
> -	if (tf_state && !tf_state->bypass) {
> -		drm_dbg(dev, "Shaper TF colorop with ID: %d\n", old_colorop->base.id);
> +	if (!tf_state->bypass) {
> +		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(tf_state->curve_1d_type);
>   		tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
> @@ -1708,7 +1674,7 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
>   		dc_plane_state->cm.flags.bits.shaper_enable = 1;
>   	}
>   
> -	if (lut_state && !lut_state->bypass) {
> +	if (!lut_state->bypass) {
>   		drm_dbg(dev, "Shaper LUT colorop with ID: %d\n", lut_colorop->base.id);
>   		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
>   		tf->tf = default_tf;
> @@ -1765,8 +1731,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->cm.shaper_func;
>   	struct drm_atomic_commit *state = plane_state->state;
>   	const struct amdgpu_device *adev = drm_to_adev(colorop->dev);
> @@ -1774,19 +1739,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) {

An issue that existed before is that we silently treated a colorop in 
this position that wasn't DRM_COLOROP_3D_LUT as valid and simply set 
lut3d_enable = 0 below. It would be better if we explicitly check for 
the type here as well and return -EINVAL if it's not a DRM_COLOROP_3D_LUT.

Harry

>   		if (!has_3dlut) {
>   			drm_dbg(dev, "3D LUT is not supported by hardware\n");
>   			return -EINVAL;
> @@ -1825,8 +1785,6 @@ __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 *new_colorop_state;
>   	struct drm_colorop_state *tf_state = NULL, *lut_state = NULL;
>   	struct drm_atomic_commit *state = plane_state->state;
>   	struct drm_colorop *lut_colorop;
> @@ -1835,38 +1793,29 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
>   	const struct drm_color_lut32 *blend_lut = NULL;
>   	struct drm_device *dev = colorop->dev;
>   	uint32_t blend_size = 0;
> -	int i = 0, ret;
> +	int ret;
>   
>   	tf->type = TF_TYPE_BYPASS;
>   	dc_plane_state->cm.flags.bits.blend_enable = 0;
>   
>   	/* 1D Curve - BLND TF: find state */
> -	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)) {
> -			tf_state = new_colorop_state;
> -			break;
> -		}
> -	}
> +	tf_state = drm_atomic_get_new_colorop_state(state, colorop);
> +	if (!tf_state)
> +		tf_state = colorop->state;
>   
>   	/* 1D LUT - BLND LUT: find state */
> -	lut_colorop = old_colorop->next;
> +	lut_colorop = colorop->next;
>   	if (!lut_colorop) {
>   		drm_dbg(dev, "no Blend LUT colorop found\n");
>   		return -EINVAL;
>   	}
>   
> -	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
> -		if (new_colorop_state->colorop == lut_colorop &&
> -		    new_colorop_state->colorop->type == DRM_COLOROP_1D_LUT) {
> -			lut_state = new_colorop_state;
> -			break;
> -		}
> -	}
> +	lut_state = drm_atomic_get_new_colorop_state(state, lut_colorop);
> +	if (!lut_state)
> +		lut_state = lut_colorop->state;
>   
> -	if (tf_state && !tf_state->bypass) {
> -		drm_dbg(dev, "Blend TF colorop with ID: %d\n", old_colorop->base.id);
> +	if (!tf_state->bypass) {
> +		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(tf_state->curve_1d_type);
>   		tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
> @@ -1876,7 +1825,7 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
>   		dc_plane_state->cm.flags.bits.blend_enable = 1;
>   	}
>   
> -	if (lut_state && !lut_state->bypass) {
> +	if (!lut_state->bypass) {
>   		drm_dbg(dev, "Blend LUT colorop with ID: %d\n", lut_colorop->base.id);
>   		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
>   		tf->tf = default_tf;