From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/atomic: track individual colorop updates
Date: Thu, 04 Jun 2026 12:16:44 +1000
Message-ID: <review-patch3-20260602215743.914265-4-mwen@igalia.com>
In-Reply-To: <20260602215743.914265-4-mwen@igalia.com>
References: <20260602215743.914265-1-mwen@igalia.com> <20260602215743.914265-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

**Author:** Melissa Wen

This is the largest patch and the core behavioral change. It ensures `plane=
_state->color_mgmt_changed` is set whenever any colorop property in the pla=
ne's color pipeline actually changes value.

**Changes reviewed:**

- **`drm_atomic_set_colorop_for_plane()`** now returns `bool` instead of `v=
oid`. It returns `false` (no change) when the same colorop pointer is set a=
gain, `true` when it actually changes. The early-return for no-change avoid=
s unnecessary debug logging and change tracking. Only one caller exists in-=
tree, at line 615, which uses `|=3D` to combine the result into `color_mgmt=
_changed`. Correct.

- **`drm_atomic_colorop_set_property()`** gains a `bool *replaced` output p=
arameter. Each property branch now compares old vs new value before writing=
, and only sets `*replaced =3D true` when the value actually changed:

  ```c
  if (state->bypass !=3D val) {
      state->bypass =3D val;
      *replaced =3D true;
  }
  ```

  This pattern is applied to `bypass`, `lut1d_interpolation`, `curve_1d_typ=
e`, `multiplier`, and `lut3d_interpolation`. The `data` property delegates =
to `drm_atomic_color_set_data_property()` which passes the pointer through =
to `drm_property_replace_blob_from_id()` =E2=80=94 that function already ha=
s a `bool *replaced` parameter with the correct semantics.

- **`drm_atomic_set_property()` (DRM_MODE_OBJECT_COLOROP case)**: After cal=
ling `drm_atomic_colorop_set_property()`, if `ret || !replaced` it breaks e=
arly (no plane state needed). Otherwise it fetches the plane state and sets=
 `plane_state->color_mgmt_changed |=3D replaced`. The `drm_atomic_get_plane=
_state()` call may fail (returns ERR_PTR), which is correctly handled.

- **`include/drm/drm_atomic_uapi.h`**: Adds `#include <linux/types.h>` for =
the `bool` return type, and updates the declaration. Clean.

**One minor observation:** At line 1325, `plane_state->color_mgmt_changed |=
=3D replaced;` =E2=80=94 at this point `replaced` is guaranteed `true` (the=
 `if (ret || !replaced) break;` on line 1317 already filtered the false cas=
e), so the `|=3D` is equivalent to `=3D true`. This is fine =E2=80=94 using=
 `|=3D` is defensive and matches the convention used elsewhere (e.g., line =
615).

**Verdict:** Correct and well-implemented. No issues.

---

---
Generated by Claude Code Patch Reviewer