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/pagemap: Use dma-map IOVA alloc, link, and sync API for DRM pagemap
Date: Sun, 12 Apr 2026 12:09:35 +1000
Message-ID: <review-patch4-20260408201537.3580549-5-matthew.brost@intel.com>
In-Reply-To: <20260408201537.3580549-5-matthew.brost@intel.com>
References: <20260408201537.3580549-1-matthew.brost@intel.com> <20260408201537.3580549-5-matthew.brost@intel.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This is the pagemap counterpart to patch 1, following the same try/fallback pattern.

**New wrapper struct is clean:**
```c
+struct drm_pagemap_iova_state {
+	struct dma_iova_state dma_state;
+	unsigned long offset;
+};
```
This pairs the IOVA state with its offset iterator, similar to `state_offset` in `drm_gpusvm_pages` but kept as a local stack variable here rather than stored in a persistent struct. Appropriate since pagemap migration is a bounded operation.

**Same `phys` parameter issue as patch 1:**
```c
+		if (!try_alloc) {
+			dma_iova_try_alloc(dev, &state->dma_state,
+					   (npages - i) * PAGE_SIZE >=
+					   HPAGE_PMD_SIZE ?
+					   HPAGE_PMD_SIZE : 0,
+					   npages * PAGE_SIZE);
+			try_alloc = true;
+		}
```
Same comment applies -- the `HPAGE_PMD_SIZE` vs `0` conditional for the phys parameter has no effect on IOVA alignment for PAGE_SIZE-aligned transfers. Just pass `0`.

Also note: the size parameter is `npages * PAGE_SIZE` (total), but the try_alloc guard uses `(npages - i) * PAGE_SIZE` (remaining) to decide alignment. Since `try_alloc` fires on the first non-NULL page, `i` could be > 0 if leading pages are NULL. The size allocated is still `npages * PAGE_SIZE` though, so there's a slight asymmetry where the alignment check uses remaining pages but the allocation uses total pages. Not a bug but slightly inconsistent.

**Unmap path with NULL state handling:**
```c
+	if (state && dma_use_iova(&state->dma_state)) {
+		dma_iova_unlink(dev, &state->dma_state, 0, state->offset, dir, 0);
+		dma_iova_free(dev, &state->dma_state);
+		return;
+	}
```
The `state` NULL check is needed because `drm_pagemap_migrate_remote_to_local` passes `NULL` (it uses device_private mapping, not system DMA). Correct.

**Stack-local state instances:** Each call site (`drm_pagemap_migrate_range`, `drm_pagemap_evict_to_ram`, `__drm_pagemap_migrate_to_ram`) creates `struct drm_pagemap_iova_state state = {};` on the stack. The state is created, used for map, consumed in copy, then freed in unmap -- all within the same function scope. Lifetime management is correct.

---
Generated by Claude Code Patch Reviewer