From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: dmem: get region once per driver Date: Sat, 16 May 2026 12:42:33 +1000 Message-ID: In-Reply-To: <20260512215156.4083082-9-cascardo@igalia.com> References: <20260512215156.4083082-1-cascardo@igalia.com> <20260512215156.4083082-9-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 Clean refactoring. The `cg_region` is obtained once in the per-driver fixture and passed to each test function, avoiding redundant `get_region_name` calls. The `free(cg_region)` in the cleanup fixture is safe even if `igt_require_f` skipped earlier (since IGT skips the cleanup fixture too on skip), and `free(NULL)` is a no-op regardless. The function signature change from `(int fd, unsigned int flags, const struct igt_dmem_driver *drv)` to `(int fd, char *cg_region, unsigned int flags, const struct igt_dmem_driver *drv)` is fine. The `cg_region` parameter could be `const char *` since the tests don't modify it, but this is minor. --- **Summary:** The series architecture is good. The main issues requiring attention are the inverted assertion in `test_current` (patch 6) which will cause test failure on Xe, the BO handle leak on bind failure in Xe (patch 1), and the question of whether amdgpu's GTT fallback actually tests anything meaningful for the eviction path (patch 5). Patches 2-4, 7-8 are clean. --- Generated by Claude Code Patch Reviewer