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: Add affected colorops with affected planes Date: Wed, 11 Mar 2026 13:23:07 +1000 Message-ID: In-Reply-To: <20260310113238.3495981-3-chaitanya.kumar.borah@intel.com> References: <20260310113238.3495981-1-chaitanya.kumar.borah@intel.com> <20260310113238.3495981-3-chaitanya.kumar.borah@intel.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 **Verdict: Mostly good, one concern.** The change adds colorops to the atomic state when `drm_atomic_add_affected_= planes()` pulls in a plane: ```c + if (plane_state->color_pipeline) { + ret =3D drm_atomic_add_affected_colorops(state, plane); + if (ret) + return ret; + } ``` The guard on `plane_state->color_pipeline` (added in v2) is a reasonable op= timization to avoid pulling in colorop state when no pipeline is active, ke= eping the atomic state small for the common case. **Concern about which state is checked:** `plane_state` here is the return = value of `drm_atomic_get_plane_state()`, which returns the **new** (duplica= ted) plane state. Since `__drm_atomic_helper_plane_duplicate_state()` prese= rves `color_pipeline` via `memcpy()`, this correctly reflects the committed= hardware state. This is fine. **Minor concern =E2=80=94 should this be unconditional?** The `color_pipeli= ne` check means that if a plane's pipeline was *just disabled* in the same = transaction (e.g., userspace set `COLOR_PIPELINE` to NULL in this commit), = the colorops won't be pulled in. In theory this could matter if the driver = needs to see the colorop state transition from active=E2=86=92bypass. Howev= er, since `drm_atomic_add_affected_planes()` operates on the *old* crtc sta= te's plane mask (planes that were enabled before this commit), and the plan= e state here is the duplicated (pre-commit) state, `color_pipeline` should = still reflect the old active pipeline. So this should be fine in practice. **The `Reviewed-by: Uma Shankar` tag says `#v1`** =E2=80=94 this was review= ed on v1 but v2 added the `color_pipeline` guard. The tag should ideally be= re-confirmed for v2 since the logic changed (previously it would unconditi= onally add colorops, now it's conditional). **Overall:** Both patches are correct and well-scoped fixes for a real bug.= The series is suitable for merging and stable backport. --- Generated by Claude Code Patch Reviewer