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/panthor: Add a GEM shrinker
Date: Tue, 31 Mar 2026 17:24:47 +1000
Message-ID: <review-patch9-20260330094848.2169422-10-boris.brezillon@collabora.com>
In-Reply-To: <20260330094848.2169422-10-boris.brezillon@collabora.com>
References: <20260330094848.2169422-1-boris.brezillon@collabora.com> <20260330094848.2169422-10-boris.brezillon@collabora.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 main event. Several points:

1. **`panthor_gem_free_object` without resv lock**: The comment explains that the last ref can be dropped from the shrinker path where resv can't be taken. The `bo_assert_locked_or_gone` helper checks `kref_read() == 0` to allow lockless access during destruction. This is correct because at `refcount==0` no other code path can access the object.

2. **`panthor_gem_try_evict_no_resv_wait` lock ordering**: The function comments explain the lock order inversion between `vm_resv -> bo_resv -> bo_gpuva` (normal path) vs `bo_resv -> bo_gpuva -> vm_resv` (reclaim path). Using `trylock` throughout is the right approach. The comment about unmapping CPU pages before taking `bo_gpuva` lock to avoid `i_mmap_rwsem` ordering issues is correct.

3. **`panthor_vm_evict_bo_mappings_locked` partial eviction**: When `panthor_vm_lock_region` fails mid-eviction, `drm_gpuvm_bo_evict(vm_bo, true)` has already been called. The comment explains this is safe because validation will repopulate all evicted VMAs. However:
```c
ret = panthor_vm_lock_region(vm, va->va.addr, va->va.range);
if (ret)
    break;
panthor_vm_unmap_pages(vm, va->va.addr, va->va.range);
panthor_vm_unlock_region(vm);
```
If `panthor_vm_lock_region` fails, we break without calling `panthor_vm_unlock_region`. Looking at the kernel tree, `panthor_vm_lock_region` only sets `locked_region` on success, so not calling unlock is correct (no region was locked).

4. **`select_evicted_vma` / `remap_evicted_vma` race**: The lock is released between selection and remapping to allow PT allocation (which can't happen under `op_lock` since it's in a DMA-signalling path). The re-verification via both pointer equality AND the `evicted` bit is sound - if the VMA was freed and reallocated at the same address, the evicted bit would be false (new VMAs start non-evicted).

5. **`panthor_mmu_reclaim_priv_bos` VM list management**: The splice/re-insert logic looks correct. VMs with remaining reclaimable BOs are preserved in LRU order. The `kref_get_unless_zero` check handles VMs being freed concurrently.

6. **`panthor_gem_shrinker_count` raciness**: The comment acknowledges the race on `gpu_mapped_count` and compares to MSM. This is standard practice for shrinker count functions - the value is a hint. Fine.

7. **`panthor_gem_shrinker_scan` retry logic**:
```c
if (!freed && remaining > 0 &&
    atomic_inc_return(&ptdev->reclaim.retry_count) < 2)
    return 0;
```
Returning 0 (not `SHRINK_STOP`) when there are remaining objects but we couldn't free any tells the shrinker to try again. After 2 failed attempts, it falls through to `SHRINK_STOP`. This seems reasonable though the threshold of 2 is somewhat arbitrary.

8. **`can_block` check**:
```c
return (sc->gfp_mask & __GFP_DIRECT_RECLAIM) ||
       ((sc->gfp_mask & __GFP_KSWAPD_RECLAIM) && current_is_kswapd());
```
This is correct - `__GFP_KSWAPD_RECLAIM` alone doesn't mean we can block (it just means kswapd has been woken), but if we're actually running *in* kswapd, we can.

9. **`panthor_vm_bo_free` change**: Previously called `panthor_gem_unpin(bo)`, now calls `panthor_gem_update_reclaim_state_locked`. The pin is now managed via `op_ctx->map.bo` (added ref + pin in prepare, dropped in cleanup). The state update in `panthor_vm_bo_free` ensures the BO moves to the correct LRU when the last vm_bo is freed.

10. **`panthor_kernel_bo_create` pinning for FW VM**: FW buffers are now explicitly pinned to prevent reclaim. This is correct - firmware memory must not be evicted.

11. **Patch 9 changes `panthor_gem_any_fault`** to handle `FAULT_FLAG_VMA_LOCK`:
```c
if (vmf->flags & FAULT_FLAG_VMA_LOCK)
    vma_end_read(vmf->vma);
else
    mmap_read_unlock(vmf->vma->vm_mm);
```
This is needed because with per-VMA locks (a relatively recent kernel feature), the fault handler may be called with only the VMA lock held rather than the mmap read lock. Good attention to detail.

12. **`blocking_page_setup` in patch 9** now calls `panthor_gem_update_reclaim_state_locked` after page setup succeeds. This ensures newly faulted-in pages are properly tracked. The `gpuva.lock` is taken inside the `resv` lock, consistent with the declared lock ordering.

13. **`debugfs shrink` interface**: The `shrink_set` function wraps the scan in `fs_reclaim_acquire/release` to simulate reclaim context for lockdep. The format string `"0x%08llx\n"` for the get is using hex for a page count - a decimal format might be more user-friendly for debugging, but this is minor.

Overall, patch 9 is complex but well-commented and the locking considerations are thoroughly documented. The series is in good shape for v6.

---
Generated by Claude Code Patch Reviewer