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.
Oops, cc'ing everyone.
"
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
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.
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) {
