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/amd/display: Fix mem_type change detection for async flips
Date: Fri, 05 Jun 2026 06:42:09 +1000
Message-ID: <review-patch1-20260604064424.3154796-1-matthew.schwartz@linux.dev>
In-Reply-To: <20260604064424.3154796-1-matthew.schwartz@linux.dev>
References: <20260604064424.3154796-1-matthew.schwartz@linux.dev> <20260604064424.3154796-1-matthew.schwartz@linux.dev>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Bug analysis: Correct.** The existing code at `amdgpu_dm.c:12785-12786`:

```c
new_plane_state =3D drm_atomic_get_plane_state(state, plane);
old_plane_state =3D drm_atomic_get_plane_state(state, plane);
```

`drm_atomic_get_plane_state()` is declared `__must_check` and returns an `E=
RR_PTR` on failure or the (new) plane state pointer on success. Since both =
calls pass the same `(state, plane)`, the second call finds the plane alrea=
dy added to the atomic state and returns the same `new_state` pointer. So `=
old_plane_state =3D=3D new_plane_state` always, and the mem_type comparison=
 is a no-op.

**Fix analysis: Correct.** The replacement:

```c
new_plane_state =3D drm_atomic_get_new_plane_state(state, plane);
old_plane_state =3D drm_atomic_get_old_plane_state(state, plane);
```

These are simple inline lookups (confirmed in `drm_atomic.h:808-828`) that =
return `state->planes[idx].old_state` and `.new_state` respectively, or NUL=
L if the plane is not part of the commit. This correctly retrieves distinct=
 old/new states for comparison.

**NULL handling: Correct.** The change from `IS_ERR()` to a NULL check with=
 `continue`:

```c
if (!old_plane_state || !new_plane_state)
    continue;
```

This is correct because `drm_atomic_get_old/new_plane_state()` returns NULL=
 (not ERR_PTR) when the plane isn't in the commit. Using `continue` rather =
than `return false` is the right semantic =E2=80=94 a plane not participati=
ng in this commit can't have a mem_type change, so we skip it and check the=
 remaining planes. The old code's `return false` was also defensively accep=
table (since that means "no change detected"), but `continue` is more preci=
se and avoids short-circuiting the loop.

**Iteration scope note (minor).** The loop uses `crtc_state->plane_mask`, w=
hich includes all planes currently associated with the CRTC =E2=80=94 inclu=
ding planes not being modified in this commit. For those planes, `drm_atomi=
c_get_new_plane_state()` will return NULL, and the `continue` correctly ski=
ps them. This is a strict improvement over the old code, which would have *=
added* every plane in the mask to the atomic state as a side effect of call=
ing `drm_atomic_get_plane_state()` =E2=80=94 unnecessarily pulling in unrel=
ated planes during atomic check.

**Commit message quality: Good.** The `[Why]`/`[How]` structure clearly exp=
lains both the root cause and the fix. The `Fixes:` tag references the corr=
ect introducing commit. The description of the real-world symptom (corrupti=
on under gamescope with DCN 3.0.1 when buffers migrate between VRAM and GTT=
) is helpful.

**No issues found.** The patch is clean, correct, and minimal.

---
Generated by Claude Code Patch Reviewer