On 6/9/26 13:51, Melissa Wen wrote:
Only consider affected colorop states those that are part of an active
color pipeline or a pipeline that is about to be activated or
deactivated in the same atomic commit, i.e., colorop is in the chain of
old/new plane color pipeline property. To cover color_pipeline
deactivation, remove the condition for plane_state->color_pipeline.
Signed-off-by: Melissa Wen <[email protected]>
---
v2: define a macro to walk in the color pipeline (Alex H.)
---
drivers/gpu/drm/drm_atomic.c | 65 +++++++++++++++++++++++++++++++-----
include/drm/drm_colorop.h | 3 ++
2 files changed, 59 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3af1b9cc9a06..464562861408 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -892,6 +892,57 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
return 0;
}
+/*
+ * This function walks old and new plane state color pipelines and adds all
+ * colorops in use by @plane to the atomic configuration @state. This is useful
+ * when an atomic commit needs to check all currently enabled or about to be
+ * enabled colorop on @plane, e.g. when changing the mode. This also avoids
+ * including colorop states that are not part of the atomic state.
+ *
+ * Returns:
+ * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is EDEADLK
+ * then the w/w mutex code has detected a deadlock and the entire atomic
+ * sequence must be restarted. All other errors are fatal.
+ */
+static int
+drm_atomic_add_pipeline_colorops(struct drm_atomic_commit *state,
+ struct drm_plane *plane)
+{
+ struct drm_colorop *colorop;
+ struct drm_colorop_state *colorop_state;
+ struct drm_plane_state *new_plane_state, *old_plane_state;
+
+ new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+ old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+
+ if (WARN_ON(!new_plane_state || !old_plane_state))
+ return -EINVAL;
+
+ drm_dbg_atomic(plane->dev,
+ "Adding old+new pipeline colorops for [PLANE:%d:%s]\n",
+ plane->base.id, plane->name);
+
+ drm_for_each_colorop_in_pipeline(colorop,
+ new_plane_state->color_pipeline) {
+ colorop_state = drm_atomic_get_colorop_state(state, colorop);
+ if (IS_ERR(colorop_state))
+ return PTR_ERR(colorop_state);
+ }
+
+ /* Same color pipeline as new; no point walking old. */
+ if (new_plane_state->color_pipeline == old_plane_state->color_pipeline)
+ return 0;
+
+ drm_for_each_colorop_in_pipeline(colorop,
+ old_plane_state->color_pipeline) {
+ colorop_state = drm_atomic_get_colorop_state(state, colorop);
+ if (IS_ERR(colorop_state))
+ return PTR_ERR(colorop_state);
+ }
+
+ return 0;
+}
+
static void drm_atomic_colorop_print_state(struct drm_printer *p,
const struct drm_colorop_state
*state)
{
@@ -1671,11 +1722,9 @@ drm_atomic_add_affected_planes(struct drm_atomic_commit
*state,
if (IS_ERR(plane_state))
return PTR_ERR(plane_state);
- if (plane_state->color_pipeline) {
- ret = drm_atomic_add_affected_colorops(state, plane);
- if (ret)
- return ret;
- }
+ ret = drm_atomic_add_pipeline_colorops(state, plane);
+ if (ret)
+ return ret;
}
return 0;
}
@@ -1687,10 +1736,8 @@ EXPORT_SYMBOL(drm_atomic_add_affected_planes);
* @plane: DRM plane
*
* This function walks the current configuration and adds all colorops
- * currently used by @plane to the atomic configuration @state. This is useful
- * when an atomic commit also needs to check all currently enabled colorop on
- * @plane, e.g. when changing the mode. It's also useful when re-enabling a
plane
- * to avoid special code to force-enable all colorops.
+ * currently used by @plane to the atomic configuration @state. It's useful
+ * when re-enabling a plane to avoid special code to force-enable all colorops.
*
* Since acquiring a colorop state will always also acquire the w/w mutex of
the
* current plane for that colorop (if there is any) adding all the colorop
states for
Unfortunately, it is not clear from the context diff but this update is
for drm_atomic_add_affected_colorops(). That function is now only used
by drm_atomic_helper_duplicate_state(). So, potentially it could be move to
drm_atomic_helper.c and made a static local helper. Alternatively,
should the _duplicate_state() function also be updated to only copy the
states of colorops in active pipelines as well? It already only does the
state add call if there is a pipeline, it just doesn't filter according
to the pipeline. Seems like it should? In which case, this blanket add
code can be removed entirely.
John.
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index b4b9e4f558ab..006278e21d49 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -459,6 +459,9 @@ static inline unsigned int drm_colorop_index(const struct
drm_colorop *colorop)
#define drm_for_each_colorop(colorop, dev) \
list_for_each_entry(colorop, &(dev)->mode_config.colorop_list, head)
+#define drm_for_each_colorop_in_pipeline(colorop, pipeline) \
+ for ((colorop) = (pipeline); (colorop); (colorop) = (colorop)->next)
+
/**
* drm_get_colorop_type_name - return a string for colorop type
* @type: colorop type to compute name of
