From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: igt_dmem_driver: add amdgpu support Date: Sat, 16 May 2026 12:42:32 +1000 Message-ID: In-Reply-To: <20260512215156.4083082-6-cascardo@igalia.com> References: <20260512215156.4083082-1-cascardo@igalia.com> <20260512215156.4083082-6-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 **Issue: `amdgpu_dmem_ctx.fd` field is declared but never assigned.** ```c struct amdgpu_dmem_ctx { int fd; /* <-- never assigned */ amdgpu_device_handle device; amdgpu_bo_handle *handles; }; ``` In `amdgpu_dmem_init`, the struct is `malloc`'d (not zeroed) and `actx->fd` is never set. It isn't read anywhere either, so it's dead code. Either remove the field or assign it. **Design concern: fill_vram assertion relaxation.** The change: ```c igt_assert_f(err == -ENOMEM || err == -ENOSPC || err == 0, "Expected -ENOMEM,-ENOSPC or success, got %d (%s)\n", err, strerror(-err)); ``` Accepting `err == 0` means `fill_vram` can return having allocated *all* `max_bo` BOs without ever hitting the cgroup limit. For amdgpu, the VRAM domain is a preference, not a constraint -- the allocator falls back to GTT. This means `test_write_eviction` on amdgpu may not actually test eviction at all, since no cgroup error is triggered. The commit message acknowledges this ("That can still be tracked with dmem.current"), but the test's core logic (fill until cgroup rejects, then lower max and watch usage drop) depends on the fill actually hitting the limit. Consider using `AMDGPU_GEM_CREATE_NO_CPU_ACCESS` or another flag to force VRAM-only placement so the test actually exercises the cgroup path. Otherwise the amdgpu variant of `write_eviction` is essentially untested. **Nit:** `#include "lib/amdgpu/amd_memory.h"` -- the `lib/` prefix in the include path is unusual for IGT; most files use paths relative to the lib directory (e.g., `#include "amdgpu/amd_memory.h"`). This works because of how meson sets include directories, but it's inconsistent with the rest of the codebase. --- Generated by Claude Code Patch Reviewer