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/amdgpu: clamp user gartsize against device capacity
Date: Sat, 16 May 2026 10:27:15 +1000
Message-ID: <review-patch1-20260514194937.35649-1-en22ue@gmail.com>
In-Reply-To: <20260514194937.35649-1-en22ue@gmail.com>
References: <20260514194937.35649-1-en22ue@gmail.com> <20260514194937.35649-1-en22ue@gmail.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

**1. Patch does not apply =E2=80=94 based on stale tree (blocking)**

The patch modifies `gmc_v11_0.c` expecting the old inline pattern:
```c
-	/* set the gart size */
-	if (amdgpu_gart_size =3D=3D -1)
-		adev->gmc.gart_size =3D 512ULL << 20;
-	else
-		adev->gmc.gart_size =3D (u64)amdgpu_gart_size << 20;
```
This code was already refactored into `amdgpu_gmc_set_gart_size()` and repl=
aced with:
```c
	amdgpu_gmc_set_gart_size(adev, SZ_512M);
```
The patch must be rebased onto the current tree.

**2. Should modify the existing helper, not add a new one (design)**

The validation logic belongs inside the existing `amdgpu_gmc_set_gart_size(=
)` at `amdgpu_gmc.c:283`. This would:
- Fix all gmc backends (v6=E2=80=93v12) automatically instead of requiring =
per-backend wiring
- Avoid a parallel API that does almost the same thing
- Eliminate the need for the "follow-up patches" the commit message mentions

**3. Missing `smu_prv_buffer_size` accounting (bug)**

The existing `amdgpu_gmc_set_gart_size()` adds `adev->pm.smu_prv_buffer_siz=
e` to the default GART size:
```c
	adev->gmc.gart_size =3D default_size + adev->pm.smu_prv_buffer_size;
```
The new `amdgpu_gmc_validate_gart_size()` returns `(u64)default_mb << 20` w=
ithout this addition. This is a functional regression in the auto-default p=
ath.

**4. Falls back to default instead of clamping (questionable policy)**

When the user requests too much, the function falls back to the hard-coded =
default rather than clamping to the computed maximum:
```c
	if (want_bytes > max_bytes) {
		...
		return (u64)default_mb << 20;
	}
```
The function name says "clamp" and the commit message says "clamping," but =
it doesn't actually clamp =E2=80=94 it resets to default. If the user asked=
 for 64 GiB and the maximum is 65536 MiB (64 GiB), they might reasonably ex=
pect the maximum to be used, not a fall-back to 512 MiB. The user explicitl=
y set a large value for a reason (e.g., compute workloads); clamping to `ma=
x_bytes` would better honor their intent.

**5. `user_mb` type is `int` but format specifier uses `%u` (minor)**

```c
	int user_mb, u32 default_mb)
	...
		dev_warn(adev->dev,
			 "amdgpu.gartsize=3D%u MiB exceeds device capacity "
```
`user_mb` is `int` but printed with `%u`. Although negative values are hand=
led by the `=3D=3D -1` check, a negative value other than -1 would print as=
 a large unsigned number. Use `%d` for consistency with the type.

**6. Kernel style: avoid splitting quoted strings across lines (minor)**

```c
		dev_warn(adev->dev,
			 "amdgpu.gartsize=3D%u MiB exceeds device capacity "
			 "(real_vram=3D%llu MiB, max sensible=3D%llu MiB); "
			 "clamping to default %u MiB\n",
```
Kernel convention (per `checkpatch.pl`) prefers keeping format strings as a=
 single searchable string even if it exceeds 80 columns, so that `grep` can=
 find the message in the source. Consider keeping it on one line.

**7. The 1/8 VRAM limit for page table is reasonable but arbitrary (nit)**

The constraint caps the page-table BO at `real_vram_size / 8`. The math is:
- page_table_bytes =3D gart_bytes / 4096 * 8 =3D gart_bytes / 512
- constraint: gart_bytes / 512 <=3D real_vram / 8
- therefore: gart_bytes <=3D real_vram * 64

For 1 GiB VRAM: max gart =3D 64 GiB =3D 65536 MiB, which seems generous. Th=
e 1/8 reservation is not justified in the code or commit message =E2=80=94 =
a brief rationale for why 1/8 was chosen would be helpful, especially since=
 the page table only needs to be sized to fit, and the real constraint is t=
hat it must be =E2=89=A4 real_vram_size (not 1/8 of it). A tighter bound (e=
.g., 1/4 or even 1/2) could also be justified if actual VRAM pressure from =
other pinned BOs is a concern.

**Summary:** The problem is valid and worth fixing, but this patch needs to=
 be rebased and reworked to modify `amdgpu_gmc_set_gart_size()` directly. T=
hat would be a smaller, simpler change that fixes all backends at once.

---
Generated by Claude Code Patch Reviewer