From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/atomic: reject colorop update from inactive color pipeline Date: Fri, 05 Jun 2026 06:13:39 +1000 Message-ID: In-Reply-To: <20260604180457.1110110-4-mwen@igalia.com> References: <20260604180457.1110110-1-mwen@igalia.com> <20260604180457.1110110-4-mwen@igalia.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Purpose:** Add enforcement in `drm_atomic_check_only()` that rejects atte= mpts to modify colorops that don't belong to an active (old or new) color p= ipeline. **`drm_atomic_colorop_check()` function:** ```c static int drm_atomic_colorop_check(const struct drm_colorop_state *new_col= orop_state) ``` The function checks three paths: 1. No plane state in the atomic commit =E2=86=92 check the current committe= d `plane->state->color_pipeline` 2. Colorop in new plane state's pipeline =E2=86=92 allowed 3. Colorop in old plane state's pipeline (if different from new) =E2=86=92 = allowed (covers deactivation case) **Potential issue =E2=80=94 accessing `plane->state` without the atomic sta= te lock:** ```c if (!new_plane_state) { for (colorop =3D plane->state->color_pipeline; colorop; colorop =3D col= orop->next) if (colorop =3D=3D new_colorop_state->colorop) return 0; return -EINVAL; } ``` When `new_plane_state` is NULL (meaning the plane wasn't part of this atomi= c commit), the code reads `plane->state->color_pipeline` =E2=80=94 the curr= ent committed state. This is the standard pattern in DRM atomic for reading= committed state when a plane hasn't been touched by the current commit. Ho= wever, the colorop state IS in the commit (that's how we got here via `for_= each_new_colorop_in_state`), so the colorop's plane mutex should already be= held (since `drm_atomic_get_colorop_state()` acquires `colorop->plane->mut= ex`). This means `plane->state` is safe to read. **This looks correct.** **Question about the `!new_plane_state` path semantics:** If userspace modifies a colorop without also touching the plane's color_pip= eline property in the same commit, does the colorop end up in the atomic st= ate? The answer is yes =E2=80=94 if userspace sets a colorop property, `drm= _atomic_get_colorop_state()` is called during property setting, which adds = it to the state. The plane state might not be in the commit though (if only= the colorop property was touched, not the plane's color_pipeline). In that= case this function checks the committed `plane->state->color_pipeline`. If= the colorop is part of the currently-committed active pipeline, the change= is allowed. This is the correct behavior. **Not using `drm_for_each_colorop_in_pipeline` macro in this patch:** Patch 3 does manual `for` loops: ```c for (colorop =3D plane->state->color_pipeline; colorop; colorop =3D colorop= ->next) ``` rather than using the `drm_for_each_colorop_in_pipeline` macro introduced i= n Patch 1. This is inconsistent. While the macro is functionally identical,= using it would be cleaner and more consistent within the series: ```c drm_for_each_colorop_in_pipeline(colorop, plane->state->color_pipeline) { if (colorop =3D=3D new_colorop_state->colorop) return 0; } ``` **Placement in `drm_atomic_check_only()`:** ```c for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) { ret =3D drm_atomic_colorop_check(new_colorop_state); if (ret) { drm_dbg_atomic(dev, "[COLOROP:%d:%d] isn't in an active color pipel= ine.\n", colorop->base.id, colorop->type); return ret; } } ``` This is placed BEFORE the plane checks and BEFORE `config->funcs->atomic_ch= eck()`. The commit message explicitly mentions doing this check here "to re= move the ordering dependency that would exist if done at the time of coloro= p property setting." This is a good design choice =E2=80=94 it ensures user= space can set colorop properties and the color_pipeline property in any ord= er within a single atomic commit, and the validation only happens at check = time. **Debug message format:** ```c drm_dbg_atomic(dev, "[COLOROP:%d:%d] isn't in an active color pipeline.\n", colorop->base.id, colorop->type); ``` The second `%d` prints `colorop->type` as a raw integer. This is mildly les= s readable than using a type name string, but it's consistent with how othe= r DRM debug messages work and the type enum values are well-defined. Fine a= s-is. **Question: What happens when `for_each_new_colorop_in_state` iterates colo= rops that were added by `drm_atomic_add_pipeline_colorops` (from Patch 1)?** In the `drm_atomic_add_affected_planes` =E2=86=92 `drm_atomic_add_pipeline_= colorops` path, colorops from old and new pipelines are added to the state.= These will then be iterated by `for_each_new_colorop_in_state` in `drm_ato= mic_check_only()`, and `drm_atomic_colorop_check()` will verify they're in = an active pipeline =E2=80=94 which they will be, since they were added prec= isely because they're in old/new pipelines. So there's no false-positive re= jection. **Correct.** **Edge case: A colorop that appears in BOTH old and new pipelines** (i.e., = the pipeline didn't change, or the colorop is in a shared suffix). `drm_ato= mic_get_colorop_state` is idempotent =E2=80=94 calling it twice for the sam= e colorop just returns the existing state. The check will find it in the ne= w pipeline and return 0 immediately. **Correct.** Overall: **Looks good**, with one minor style suggestion to use the new `dr= m_for_each_colorop_in_pipeline` macro for consistency. --- ## Summary The series is well-designed and implements the agreed-upon UAPI contract cl= eanly. The three patches have a logical progression: narrow what gets pulle= d in for checking (patch 1), ensure full persistence for state duplication = (patch 2), and enforce the policy (patch 3). **Issues to address:** 1. **Minor (Patch 3):** The manual `for` loops walking the pipeline should = use the `drm_for_each_colorop_in_pipeline` macro introduced in Patch 1 for = consistency within the series. **Questions for the author:** 1. Are there any out-of-tree drivers or userspace clients that currently mo= dify colorops on inactive pipelines? The `-EINVAL` return from Patch 3 coul= d break them. The cover letter mentions IGT fixes were sent separately, whi= ch is good. 2. Has this been tested with the pipeline switching from one set of colorop= s to a different set in a single commit (i.e., both old and new pipelines a= re non-NULL but different)? --- Generated by Claude Code Patch Reviewer