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: Tue, 05 May 2026 09:26:01 +1000
Message-ID: <review-patch5-20260501132527.522320-6-mwen@igalia.com>
In-Reply-To: <20260501132527.522320-6-mwen@igalia.com>
References: <20260501132527.522320-1-mwen@igalia.com> <20260501132527.522320-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

**Purpose:** Uses the plane's `color_mgmt_changed` flag to track when any c=
olorop property in the active pipeline changes value.

**Review:**

The approach of threading a `bool *replaced` through the set_property path =
is functional but adds some complexity. The pattern is:

```c
static int drm_atomic_colorop_set_property(..., bool *replaced)
{
    if (property =3D=3D colorop->bypass_property) {
        if (state->bypass !=3D val) {
            state->bypass =3D val;
            *replaced =3D true;
        }
    } ...
}
```

Then at the call site:
```c
bool replaced =3D false;
...
ret =3D drm_atomic_colorop_set_property(colorop, colorop_state,
                                      file_priv, prop, prop_value,
                                      &replaced);
if (!ret && replaced)
    plane_state->color_mgmt_changed =3D true;
```

**This is correct and well-designed.** It avoids setting `color_mgmt_change=
d` when the same value is written (no-change commit), which prevents unnece=
ssary driver work. The `|=3D` for pipeline changes in `drm_atomic_set_color=
op_for_plane` is also correct:

```c
state->color_mgmt_changed |=3D drm_atomic_set_colorop_for_plane(state, colo=
rop);
```

**Design note on `replaced` naming:** The name `replaced` is borrowed from =
the blob property API (`drm_property_replace_blob`), but for scalar propert=
ies it reads slightly oddly. Something like `changed` might be clearer. Thi=
s is a minor style preference.

**`drm_atomic_set_colorop_for_plane` return value change:** The function no=
w returns `bool` instead of `void` and adds an early-return optimization:

```c
if (plane_state->color_pipeline =3D=3D colorop)
    return false;
```

This is a good optimization =E2=80=94 avoids logging and state changes when=
 re-setting the same pipeline. The `#include <linux/types.h>` addition to `=
drm_atomic_uapi.h` for the `bool` return type is correct.

---
Generated by Claude Code Patch Reviewer