On 21/05/2026 13:00, Borah, Chaitanya Kumar wrote:
On 5/20/2026 2:39 AM, Melissa Wen wrote:
Only allow updates on colorops that are part of an active pipeline.
Check if a colorop in a new state belongs to a color pipeline which was
set as a plane color_pipeline property and therefore is an active color
pipeline. If not, reject the atomic state. Performing this check later
in drm_atomic_check_only() to remove the ordering dependency that would
exist if done at the time of colorop property setting.
Suggested-by: Chaitanya Kumar Borah <[email protected]>
Signed-off-by: Melissa Wen <[email protected]>
---
drivers/gpu/drm/drm_atomic.c | 38 ++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 28831a548b0c..659cf56150e5 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -812,6 +812,33 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_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 a plane state.
+ *
+ * Returns: 0 on success, -EINVAL otherwise.
+ */
+static int drm_atomic_colorop_check(const struct drm_colorop_state
*new_colorop_state)
+{
+ struct drm_colorop *colorop, *color_pipeline;
+ struct drm_plane_state *new_plane_state;
+
+ new_plane_state =
drm_atomic_get_new_plane_state(new_colorop_state->state,
+ new_colorop_state->colorop->plane);
+ color_pipeline = new_plane_state ?
new_plane_state->color_pipeline :
+ new_colorop_state->colorop->plane->state->color_pipeline;
+
+ for (colorop = color_pipeline; colorop; colorop = colorop->next)
+ if (colorop == new_colorop_state->colorop)
+ return 0;
+
+ return -EINVAL;
+}
+
This causes regression in our CI[1].
I looked into it and looks like the following sequence in
igt@kms_color_pipeline causes the error
set_color_pipeline_bypass(plane);
reset_colorops(colorops);
igt_plane_set_fb(plane, NULL);
igt_display_commit_atomic(&data->display, 0, NULL);
So this change restricts bypassing/disabling both the pipeline and a
colorop within it in a single commit.
Also Sashiko had the following to say
"Furthermore, does this unnecessarily restrict UAPI by preventing
userspace
from configuring inactive pipelines before enabling them, or from
resetting
properties on a pipeline in the same commit that switches away from it?"
So this will also fail a commit which tries to change a pipeline and
disable the colorops in an old pipeline.
I wonder if userspace resetting colorops to disable a pipeline or
configuring colorops before enabling the color pipeline is an expected
behavior.
For resetting properties, I think I can solve it by taking into account
old and new state to collect the active colorops, not only the new state.
But if configuring colorops before activate a color pipeline is
expected, there is no need to have patches 1 and 2, since setting an
inactive colorop have to be allowed. In that case, the solution is just
drop both patches from the series.
Melissa
That got me thinking whether the first patch[3] in the series is also
correct, since it is quite similar to the change[4] I added, where
colorops are only added to the state when a pipeline is active. In
both cases, we could end up ignoring colorops that are not part of the
currently selected pipeline.
[1]
https://intel-gfx-ci.01.org/tree/intel-xe/xe-pw-166922v1/shard-lnl-5/igt@kms_color_pipeline@[email protected]
[2]
https://sashiko.dev/#/patchset/20260520073827.3395745-3-chaitanya.kumar.borah%40intel.com
[3]
https://lore.kernel.org/dri-devel/[email protected]/
[4]
https://lore.kernel.org/dri-devel/[email protected]/
P.S. Can you please send the next version to intel-gfx and intel-xe too?
==
Chaitanya
static void drm_atomic_colorop_print_state(struct drm_printer *p,
const struct drm_colorop_state *state)
{
@@ -1665,6 +1692,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;
@@ -1681,6 +1710,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] is not part of 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) {
