On 6/24/26 21:01, Harry Wentland wrote:
On 2026-06-09 13:23, John Harrison wrote:
On 6/3/26 04:27, Melissa Wen wrote:
On 01/06/2026 11:24, Borah, Chaitanya Kumar wrote:
On 5/29/2026 7:16 PM, Jani Nikula wrote:
On Tue, 26 May 2026, Alex Hung <[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
"there is no struct drm_color_pipeline to hold a list_head" <-- I think this is
the real reason. It is possible to convert to use a proper list structure, but the result is
slightly messy. I had a quick go at it to see how messy:
https://patchwork.freedesktop.org/series/168200/
Yeah, Melissa and Chaitanya pretty much described why they work the way they
do. I'm not sure it makes sense to replace the mechanism with lists and any
attempt to do so should make sure not to break userspace ABI. I'm not opposed
to improvements either if anyone finds a solution that makes everyone's lives
easier.
Harry
@Harry, the patch series I linked above does the conversion. It does not
affect the user space ABI at all, only the internal kernel operation is
changed. I think it is better in some ways but maybe not in others. If
you would like to take a look, any feedback would be appreciated.
Thanks,
John.
John.
Harry, others can chime in.
==
Chaitanya
BR,
Jani.
+ colorop_state = drm_atomic_get_colorop_state(state, colorop);
+ if (IS_ERR(colorop_state))
+ return PTR_ERR(colorop_state);
+ }
+
+ /* Same color pipeline as new; no point walking old. */
+ if (new_plane_state->color_pipeline == old_plane_state->color_pipeline)
+ return 0;
+
+ for (colorop = old_plane_state->color_pipeline;
+ colorop;
+ colorop = colorop->next) {
+ colorop_state = drm_atomic_get_colorop_state(state, colorop);
+ if (IS_ERR(colorop_state))
+ return PTR_ERR(colorop_state);
+ }
+
+ return 0;
+}
+
static void drm_atomic_colorop_print_state(struct drm_printer *p,
const struct drm_colorop_state *state)
{
@@ -1591,11 +1644,9 @@ drm_atomic_add_affected_planes(struct drm_atomic_commit
*state,
if (IS_ERR(plane_state))
return PTR_ERR(plane_state);
- if (plane_state->color_pipeline) {
- ret = drm_atomic_add_affected_colorops(state, plane);
- if (ret)
- return ret;
- }
+ ret = drm_atomic_add_pipeline_colorops(state, plane);
+ if (ret)
+ return ret;
}
return 0;
}
@@ -1607,10 +1658,8 @@ EXPORT_SYMBOL(drm_atomic_add_affected_planes);
* @plane: DRM plane
*
* This function walks the current configuration and adds all colorops
- * currently used by @plane to the atomic configuration @state. This is useful
- * when an atomic commit also needs to check all currently enabled colorop on
- * @plane, e.g. when changing the mode. It's also useful when re-enabling a
plane
- * to avoid special code to force-enable all colorops.
+ * currently used by @plane to the atomic configuration @state. It's useful
+ * when re-enabling a plane to avoid special code to force-enable all colorops.
*
* Since acquiring a colorop state will always also acquire the w/w mutex
of the
* current plane for that colorop (if there is any) adding all the colorop
states for
