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: Wed, 25 Mar 2026 07:51:14 +1000
Message-ID: <review-patch1-20260323131942.494217-2-mwen@igalia.com>
In-Reply-To: <20260323131942.494217-2-mwen@igalia.com>
References: <20260323131942.494217-1-mwen@igalia.com> <20260323131942.494217-2-mwen@igalia.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Overall:** Solid patch. The approach of threading a `bool *replaced` through the colorop property-set path and using it to conditionally set `plane_state->color_mgmt_changed` is clean and consistent.

**Observations:**

1. **`drm_atomic_set_colorop_for_plane` return value change** (lines 213-233): Changing from `void` to `bool` and adding the early-return short-circuit is good. The usage at line 241:
   ```c
   state->color_mgmt_changed |= drm_atomic_set_colorop_for_plane(state, colorop);
   ```
   Correctly uses `|=` to avoid clearing a previously-set flag. Good.

2. **Interpolation properties intentionally not tracked** (lines 282, 295): The `lut1d_interpolation` and `lut3d_interpolation` properties are set directly on the `colorop` object (not `state`) and don't set `*replaced`. The cover letter mentions these are read-only properties and intentionally excluded. This is correct - they describe hardware capabilities rather than userspace-settable state.

3. **`replaced` naming convention**: The `bool *replaced` out-parameter is functional but the name is slightly misleading for scalar properties like `bypass` or `multiplier` where nothing is being "replaced" (that term fits blob properties better). Something like `changed` would be more natural. Minor style nit, not worth a respin.

4. **Plane state fetch on colorop change** (lines 324-329): In `drm_atomic_set_property`, when a colorop value changes, the code fetches the plane state via `drm_atomic_get_plane_state()`. This is correct - it ensures the plane is part of the atomic commit and its state is properly tracked. The error handling with `PTR_ERR` is also correct.

5. **`#include <linux/types.h>`** (line 341): Added to provide `bool` for the header. This is the right fix for the build issue the kernel bot reported on MSM.

6. **Missing tracking for `drm_atomic_colorop_set_property` unknown property path** (lines 302-306): If the property is unknown, the function returns `-EINVAL` and `*replaced` remains `false`. The caller at line 321 checks `if (ret || !replaced)` and breaks. This correctly avoids setting `color_mgmt_changed` on error. Good.

7. **Thread safety consideration**: The `replaced` variable is stack-local in `drm_atomic_set_property` (line 310), so no concurrency concerns. Fine.

---
Generated by Claude Code Patch Reviewer