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: dmem: add test for current/max
Date: Sat, 16 May 2026 12:42:33 +1000
Message-ID: <review-patch6-20260512215156.4083082-7-cascardo@igalia.com>
In-Reply-To: <20260512215156.4083082-7-cascardo@igalia.com>
References: <20260512215156.4083082-1-cascardo@igalia.com> <20260512215156.4083082-7-cascardo@igalia.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Bug: Inverted assertion at max=0 test.**

```c
igt_cgroup_dmem_set_max(cg, cg_region, 0);

n_bo = allocate_vram(drv, ctx, fd, max_bo, BO_SIZE);
igt_assert_f(n_bo != -ENOMEM, "VRAM allocation succeeded despite max set to 0\n");
```

`allocate_vram` returns negative error on failure, or positive n_bo count on success. With `max=0`, Xe should reject the allocation with `-ENOMEM`. The assertion `n_bo != -ENOMEM` is **false** when allocation correctly fails, so the test will fire `igt_assert_f` and abort with the message "VRAM allocation succeeded despite max set to 0" -- but the allocation actually *failed*, which is the expected behavior.

This should probably be:
```c
igt_assert_f(n_bo <= 0, "VRAM allocation succeeded despite max set to 0\n");
```

Or, since amdgpu may succeed by falling back to GTT:
```c
/* Xe: allocation should fail; amdgpu: may succeed via GTT fallback */
```
with appropriate per-driver logic.

**Issue: stale context reuse across allocate/free cycles.** The test does:
1. `drv->init(&ctx, ...)` with `max_bo = cg_max / BO_SIZE`
2. `allocate_vram(... max_bo ...)` -- uses handles[0..n_bo-1]
3. `drv->free_vram(ctx, n_bo, BO_SIZE)` -- frees those BOs
4. `allocate_vram(... max_bo ...)` -- reuses handles[0..n_bo-1] (OK)
5. `drv->free_vram(ctx, n_bo, BO_SIZE)` -- frees again
6. `allocate_vram(... max_bo ...)` -- third round

For Xe, each `allocate_vram` call increments `xe_ctx->addr` but `free_vram` doesn't reset it (as noted in patch 1 review). After three rounds, we've consumed `~3 * 4GiB = 12GiB` of VA space starting from `BIND_BASE` (4 GiB). This works but is fragile.

**Nit: `sleep(1)` after free.** This is a heuristic wait for the kernel to update cgroup accounting. It's common in IGT but fragile -- consider using a polling helper like the existing `wait_for_usage_drop` if available.

---
Generated by Claude Code Patch Reviewer