On 01/06/2026 11:24, Borah, Chaitanya Kumar wrote:
On 5/29/2026 7:16 PM, Jani Nikula wrote:
On Tue, 26 May 2026, Alex Hung <[email protected]> wrote:
On 5/26/26 08:17, Melissa Wen wrote:
Only consider affected colorop states those that are part of an active
color pipeline or a pipeline that is about to be activated or
deactivated in the same atomic commit, i.e., colorop is in the
chain of
old/new plane color pipeline property. To cover color_pipeline
deactivation, remove the condition for plane_state->color_pipeline.
Signed-off-by: Melissa Wen <[email protected]>
---
drivers/gpu/drm/drm_atomic.c | 67
+++++++++++++++++++++++++++++++-----
1 file changed, 58 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c
b/drivers/gpu/drm/drm_atomic.c
index 170de30c28ae..4fb3a23e862a 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -812,6 +812,59 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
return 0;
}
+/*
+ * This function walks old and new plane state color pipelines and
adds all
+ * colorops in use by @plane to the atomic configuration @state.
This is useful
+ * when an atomic commit needs to check all currently enabled or
about to be
+ * enabled colorop on @plane, e.g. when changing the mode. This
also avoids
+ * including colorop states that are not part of the atomic state.
+ *
+ * Returns:
+ * 0 on success or can fail with -EDEADLK or -ENOMEM. When the
error is EDEADLK
+ * then the w/w mutex code has detected a deadlock and the entire
atomic
+ * sequence must be restarted. All other errors are fatal.
+ */
+static int
+drm_atomic_add_pipeline_colorops(struct drm_atomic_commit *state,
+ struct drm_plane *plane)
+{
+ struct drm_colorop *colorop;
+ struct drm_colorop_state *colorop_state;
+ struct drm_plane_state *new_plane_state, *old_plane_state;
+
+ new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+ old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+
+ if (WARN_ON(!new_plane_state || !old_plane_state))
+ return -EINVAL;
+
+ drm_dbg_atomic(plane->dev,
+ "Adding old+new pipeline colorops for
[PLANE:%d:%s]\n",
+ plane->base.id, plane->name);
+
+ for (colorop = new_plane_state->color_pipeline;
+ colorop;
+ colorop = colorop->next) {
This for-loop is used 5 times in this patchset. How about a macro in
drm_colorop.h?
#define drm_for_each_colorop_in_pipeline(colorop, pipeline) \
for ((colorop) = (pipeline); (colorop); (colorop) =
(colorop)->next)
Is there a reason struct drm_colorop reinvents lists and doesn't have
struct list_head node?
I believe that's because the "next" colorop is exposed as a property
(of the current colorop) to userspace. Since the chain is already
described by the property, a struct list_head would be redundant.
Also, each color pipeline is an immutable chain of colorops where the
sequence and position matter: once the chain is built, colorops are
never added, removed, replaced or walked in reverse. It's a forward-only
chain that ends when next == NULL, and it directly matches userspace
mapping. Another point to take into account is that there is no struct
drm_color_pipeline to hold a list_head yet, since each color pipeline is
identified by the first colorop element in the chain. Maybe we will want
a container to link a given pre-blend color pipeline to a specific
post-blend color pipeline for example, but linking pre- to post-blend
color pipelines is something we are still not clear about.
Melissa
Harry, others can chime in.
==
Chaitanya
BR,
Jani.
+ colorop_state = drm_atomic_get_colorop_state(state, colorop);
+ if (IS_ERR(colorop_state))
+ return PTR_ERR(colorop_state);
+ }
+
+ /* Same color pipeline as new; no point walking old. */
+ if (new_plane_state->color_pipeline ==
old_plane_state->color_pipeline)
+ return 0;
+
+ for (colorop = old_plane_state->color_pipeline;
+ colorop;
+ colorop = colorop->next) {
+ colorop_state = drm_atomic_get_colorop_state(state, colorop);
+ if (IS_ERR(colorop_state))
+ return PTR_ERR(colorop_state);
+ }
+
+ return 0;
+}
+
static void drm_atomic_colorop_print_state(struct drm_printer *p,
const struct drm_colorop_state *state)
{
@@ -1591,11 +1644,9 @@ drm_atomic_add_affected_planes(struct
drm_atomic_commit *state,
if (IS_ERR(plane_state))
return PTR_ERR(plane_state);
- if (plane_state->color_pipeline) {
- ret = drm_atomic_add_affected_colorops(state, plane);
- if (ret)
- return ret;
- }
+ ret = drm_atomic_add_pipeline_colorops(state, plane);
+ if (ret)
+ return ret;
}
return 0;
}
@@ -1607,10 +1658,8 @@ EXPORT_SYMBOL(drm_atomic_add_affected_planes);
* @plane: DRM plane
*
* This function walks the current configuration and adds all
colorops
- * currently used by @plane to the atomic configuration @state.
This is useful
- * when an atomic commit also needs to check all currently enabled
colorop on
- * @plane, e.g. when changing the mode. It's also useful when
re-enabling a plane
- * to avoid special code to force-enable all colorops.
+ * currently used by @plane to the atomic configuration @state.
It's useful
+ * when re-enabling a plane to avoid special code to force-enable
all colorops.
*
* Since acquiring a colorop state will always also acquire the
w/w mutex of the
* current plane for that colorop (if there is any) adding all
the colorop states for
