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: Re: [PATCH] drm/amdgpu: deduplicate ring preempt ib function
Date: Thu, 23 Apr 2026 07:52:56 +1000
Message-ID: <review-patch1-59b686c6-42f5-4cde-8199-dae64722bfd1@amd.com>
In-Reply-To: <59b686c6-42f5-4cde-8199-dae64722bfd1@amd.com>
References: <59b686c6-42f5-4cde-8199-dae64722bfd1@amd.com> <59b686c6-42f5-4cde-8199-dae64722bfd1@amd.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

**Correctness: Good.** The extracted function in `amdgpu_gfx.c` is an exact=
 copy of the removed functions from both `gfx_v11_0.c` and `gfx_v12_0.c`. T=
he function signature matches the `preempt_ib` callback in `struct amdgpu_r=
ing_funcs` (`int (*preempt_ib)(struct amdgpu_ring *ring)`), so assigning it=
 directly to `.preempt_ib` in the ring_funcs structs is correct.

**Nit =E2=80=94 trailing blank line:** The new function in `amdgpu_gfx.c` i=
s followed by two blank lines before the end of the diff:

```c
+	return r;
+}
+
+
```

Kernel coding style uses a single blank line between functions (or at end-o=
f-file). Remove the extra blank line.

**Nit =E2=80=94 alignment inconsistency:** In the new shared function, the =
`kiq_unmap_queues` call arguments use tab+tabs alignment:

```c
+	kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
+					ring->trail_fence_gpu_addr,
+					++ring->trail_seq);
```

The original code in both `gfx_v11_0.c` and `gfx_v12_0.c` aligned continuat=
ion lines to the opening parenthesis using tabs+spaces:

```c
	kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
				   ring->trail_fence_gpu_addr,
				   ++ring->trail_seq);
```

The original alignment is the more common kernel style for this codebase. S=
ince the goal is to move the code without modification, please preserve the=
 original alignment.

**Missed opportunity =E2=80=94 gfx_v10_0:** The `gfx_v10_0_ring_preempt_ib(=
)` function at `gfx_v10_0.c:8855` is functionally identical to the new shar=
ed version, with only one difference: it lacks the `adev->enable_mes` early=
-return check. Since MES is not supported on GFX10 hardware, the `enable_me=
s` guard would be harmlessly false on v10 and the shared function could be =
used there as well. This would eliminate a third copy of the same logic. Wo=
rth considering for this patch or a follow-up.

**Not in scope but worth noting =E2=80=94 gfx_v9_0:** The `gfx_v9_0` versio=
n uses a different fence emission mechanism (direct `gfx_v9_0_ring_emit_fen=
ce()` calls instead of `kiq_unmap_queues`), so it cannot be consolidated wi=
th this shared function. No action needed.

**Overall:** The patch does what it says on the tin and the logic is correc=
t. Fix the trailing blank line and the alignment nit, and consider also con=
verting `gfx_v10_0` to use the shared function since it's the same code.

---
Generated by Claude Code Patch Reviewer