On 26/06/2026 00:38, John Harrison wrote:
On 6/9/26 13:51, Melissa Wen wrote:
Only allow updates on colorops that are part of an active pipeline, i.e.
check if a colorop belongs to the color pipeline of a plane in its
current, new or old state. If not, reject the state change of this
inactive colorop. 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. Userspace is allowed to change colorops of
an active color pipeline, or when activating or deactivating its
pipeline in the same commit. However, changes in inactive color pipeline
is not allowed.
The last two sentences here seem to be a duplicate of the first two
sentences. Maybe drop them as redundant?
Sure, I think I can explain it better.
I was trying to say that we are changing this behavior on the kernel
side to align with userspace expectations.
Suggested-by: Chaitanya Kumar Borah <[email protected]>
Signed-off-by: Melissa Wen <[email protected]>
---
drivers/gpu/drm/drm_atomic.c | 60 ++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 464562861408..960b52624deb 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -943,6 +943,55 @@ drm_atomic_add_pipeline_colorops(struct
drm_atomic_commit *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 current, old or new plane state.
+ *
+ * Returns: 0 on success, -EINVAL otherwise.
+ */
+static int drm_atomic_colorop_check(const struct drm_colorop_state
*new_colorop_state)
+{
+ struct drm_atomic_commit *state = new_colorop_state->state;
+ struct drm_plane *plane = new_colorop_state->colorop->plane;
+ struct drm_plane_state *new_plane_state, *old_plane_state;
+ struct drm_colorop *colorop;
+
+ new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+ old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+
+ /* No changes in the plane state. Check current-committed plane
state */
+ if (!new_plane_state) {
+ for (colorop = plane->state->color_pipeline; colorop;
colorop = colorop->next)
Doesn't the first patch add a macro for wrapping this for loop?
Yes, thanks for pointing out. I'll replace all occurrences.
+ if (colorop == new_colorop_state->colorop)
+ return 0;
+ return -EINVAL;
+ }
+
+ if (WARN_ON(!old_plane_state))
+ return -EINVAL;
+
+ /* Check if the colorop is active in the new plane state */
+ for (colorop = new_plane_state->color_pipeline; colorop; colorop
= colorop->next)
+ if (colorop == new_colorop_state->colorop)
+ return 0;
+
+ /* Same color pipeline as new; no point walking old. Colorop
isn't active */
+ if (new_plane_state->color_pipeline ==
old_plane_state->color_pipeline)
+ return -EINVAL;
+
+ /* Check if the colorop was active in the old plane state */
+ for (colorop = old_plane_state->color_pipeline; colorop; colorop
= colorop->next)
+ if (colorop == new_colorop_state->colorop)
+ return 0;
Doesn't this situation count as changing the properties of an inactive
colorop? And should therefore be rejected? The colorop was previously
in use but once the commit goes through, it will not be. So any
changes to its state will not actually affect the new post-commit
universe. Or am I missing something?
We still need it because the userspace is still allowed to change
colorops of a pipeline A that will be disable in the same commit. I
mean, they can change colorop settings of an active pipeline A and set
COLOR_PIPELINE to Bypass in the same commit, and this change have to
persist for the time this pipeline A will be reactivated, even if in
practice those colorop changes will not be programmed in the hw because
of deactivation. I'll add this comment to explain this contract better:
+ * Userspace is allowed to finalize colorop's settings in the same
commit that
+ * deactivates its pipeline, and the changes should persist for when that
+ * pipeline is reactivated later. So that, changes to colorop in the
old plane
+ * state's pipeline is accepted even though it won't drive hardware
updates.
WDYT?
Melissa
John.
+
+ /* Colorop is not part of an active color pipeline. */
+ return -EINVAL;
+}
+
static void drm_atomic_colorop_print_state(struct drm_printer *p,
const struct drm_colorop_state *state)
{
@@ -1792,6 +1841,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;
@@ -1808,6 +1859,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] isn't in 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) {
