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