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, 07 May 2026 13:05:38 +1000
Message-ID: <review-patch5-20260506192633.16066-6-mwen@igalia.com>
In-Reply-To: <20260506192633.16066-6-mwen@igalia.com>
References: <20260506192633.16066-1-mwen@igalia.com> <20260506192633.16066-6-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

**Assessment: Good, one design observation**

This is the core patch that wires up `color_mgmt_changed` tracking. The app=
roach is clean:

1. `drm_atomic_set_colorop_for_plane()` now returns `bool` (true if pipelin=
e changed) and the caller uses `|=3D` to accumulate into `plane_state->colo=
r_mgmt_changed`.

2. `drm_atomic_colorop_set_property()` and `drm_atomic_color_set_data_prope=
rty()` gain a `bool *replaced` output parameter to signal when a property v=
alue actually changed.

3. In `drm_atomic_set_property()`, after setting a colorop property:
```c
if (!ret && replaced)
    plane_state->color_mgmt_changed =3D true;
```

The change detection in `drm_atomic_colorop_set_property()` correctly check=
s each property individually:
```c
if (state->bypass !=3D val) {
    state->bypass =3D val;
    *replaced =3D true;
}
```

**Design observation**: The `replaced` variable is initialized to `false` b=
ut only set to `true` =E2=80=94 it's never checked before being returned. F=
or `drm_atomic_color_set_data_property()`, the `replaced` pointer is forwar=
ded directly to `drm_property_replace_blob_from_id()`, which handles blob c=
omparison internally. This is correct because `drm_property_replace_blob_fr=
om_id()` already sets `*replaced` to indicate whether the blob pointer chan=
ged.

**Observation about `drm_atomic_set_colorop_for_plane()`**: The early retur=
n `if (plane_state->color_pipeline =3D=3D colorop) return false` is a nice =
optimization =E2=80=94 it avoids the debug prints for no-op pipeline select=
ions. However, this changes behavior slightly: previously, the debug messag=
e would fire even for redundant sets. This is fine since it's debug-only.

The `#include <linux/types.h>` in `drm_atomic_uapi.h` for `bool` is correct=
 and matches the v2 changelog note about fixing the MSM driver build.

---
Generated by Claude Code Patch Reviewer