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/colorop: Preserve bypass value in duplicate_state() Date: Wed, 11 Mar 2026 13:23:06 +1000 Message-ID: In-Reply-To: <20260310113238.3495981-2-chaitanya.kumar.borah@intel.com> References: <20260310113238.3495981-1-chaitanya.kumar.borah@intel.com> <20260310113238.3495981-2-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: Looks good.** The fix removes the unconditional `state->bypass =3D true` assignment after= `memcpy()` in `__drm_atomic_helper_colorop_duplicate_state()`: ```c if (state->data) drm_property_blob_get(state->data); - - state->bypass =3D true; ``` This is clearly correct. The `memcpy()` on line 465 already copies the enti= re state including `bypass`. Overwriting it to `true` afterwards defeats th= e purpose of duplicating the current hardware state =E2=80=94 exactly the s= ame pattern used for plane and CRTC state duplication, where no fields are = unconditionally reset to non-hardware values after the copy. As the cover letter notes, this is also a problem for colorops where bypass= is immutably false =E2=80=94 the forced `true` would be contradicting the = hardware capabilities. One question: was there an original reason this line existed? Looking at th= e commit message for `8c5ea1745f4c` ("drm/colorop: Add BYPASS property") mi= ght reveal if this was intentional to ensure bypass-by-default semantics fo= r userspace-initiated commits. If so, the change is still correct (userspac= e should set bypass explicitly), but it's worth understanding the original = intent to ensure no regressions. --- Generated by Claude Code Patch Reviewer