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/colorop: make lut(1/3)d_interpolation mutable
Date: Tue, 05 May 2026 09:26:01 +1000
Message-ID: <review-patch4-20260501132527.522320-5-mwen@igalia.com>
In-Reply-To: <20260501132527.522320-5-mwen@igalia.com>
References: <20260501132527.522320-1-mwen@igalia.com> <20260501132527.522320-5-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:** Moves `lut1d_interpolation` and `lut3d_interpolation` from `st=
ruct drm_colorop` (immutable HW object) to `struct drm_colorop_state` (per-=
atomic-state), making them properly mutable.

**Review:**

The state reset initialization is correctly done through `drm_object_proper=
ty_get_default_value`:

```c
if (colorop->lut1d_interpolation_property) {
    drm_object_property_get_default_value(&colorop->base,
                                          colorop->lut1d_interpolation_prop=
erty,
                                          &val);
    colorop_state->lut1d_interpolation =3D val;
}
```

**Minor concern:** The return value of `drm_object_property_get_default_val=
ue` is not checked here. Looking at the existing `curve_1d_type` pattern ju=
st above:

```c
if (colorop->curve_1d_type_property) {
    drm_object_property_get_default_value(&colorop->base,
                                          colorop->curve_1d_type_property,
                                          &val))
```

Wait =E2=80=94 in the existing code, the return value IS checked as part of=
 an `if` condition. But in the new code for interpolation properties, it is=
 NOT:

```c
if (colorop->lut1d_interpolation_property) {
    drm_object_property_get_default_value(&colorop->base,
                                          colorop->lut1d_interpolation_prop=
erty,
                                          &val);
    colorop_state->lut1d_interpolation =3D val;
}
```

Looking at the existing code for `curve_1d_type`, the pattern is:
```c
if (colorop->curve_1d_type_property) {
    if (!drm_object_property_get_default_value(&colorop->base,
                                               colorop->curve_1d_type_prope=
rty,
                                               &val))
        colorop_state->curve_1d_type =3D val;
}
```

**The new code doesn't guard the assignment with the return value check.** =
If `drm_object_property_get_default_value` fails, `val` would be stale from=
 a previous call (the `curve_1d_type` lookup). This is probably benign sinc=
e the property is guaranteed to exist when the property pointer is non-NULL=
, but it's inconsistent with the existing pattern. This should match the ex=
isting defensive style.

The removal of `colorop->lut1d_interpolation =3D interpolation` from the in=
it functions is correct since the value now lives in state.

The print_state and get/set property changes are mechanical and correct.

---
Generated by Claude Code Patch Reviewer