Claude review: drm/panthor: Add a GEM shrinker

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

The main event. Several observations:

1. **`panthor_gem_evaluate_reclaim_state_locked`**: Good hierarchy of reclaimability states. The ordering (UNUSED < MMAPPED < GPU_MAPPED_PRIVATE < GPU_MAPPED_SHARED < UNRECLAIMABLE) makes sense.

2. **`gpu_mapped_count` race**: `ptdev->reclaim.gpu_mapped_count` is modified in `panthor_gem_update_reclaim_state_locked()` without holding `reclaim.lock`, but read in `panthor_gem_shrinker_count()` also without the lock. Since it's a `long` (word-sized), reads and writes are atomic on most architectures, but this isn't guaranteed by the C standard. Consider using `atomic_long_t` for correctness.

3. **`panthor_gem_try_evict_no_resv_wait`**: The trylock cascade for vm_bo resv locks is correctly handled with rollback in `out_unlock`. However:

   - **Partial eviction in `panthor_vm_evict_bo_mappings_locked`**: If `panthor_vm_lock_region()` fails for one VMA (after successfully evicting others), the function returns an error but some VMAs are already evicted. The caller (`panthor_gem_try_evict_no_resv_wait`) then returns false (eviction failed), but the BO is in a partially-evicted state. The GPU will see an inconsistent address space. This seems like it could cause GPU faults on the already-unmapped ranges before the BO is fully restored. Is this acceptable? The assumption might be that the VM won't be used while the resv lock is held, but that's not guaranteed for shared BOs.

4. **`panthor_mmu_reclaim_priv_bos` VM list management**: The check `vm->reclaim.lru_node.prev == &vms` to determine if the VM is still in the temporary list is fragile. If a concurrent operation moves the node to `ptdev->reclaim.vms` between the `mutex_unlock` and `mutex_lock`, this check would fail incorrectly. However, looking more carefully, the node can only be moved by `panthor_vm_active` (which moves to `ptdev->reclaim.vms`) or `panthor_vm_free` (which does `list_del_init`). Since the `reclaim.lock` is held when checking, and concurrent moves also hold `reclaim.lock`, this is actually safe. But the `prev == &vms` pattern is still fragile — using `list_empty()` after `list_del_init()` or a dedicated flag would be cleaner.

5. **`panthor_vm_bo_free` changes**: The patch removes the `panthor_gem_unpin(bo)` call and adds reclaim state update. But the unpin is now done in `panthor_vm_cleanup_op_ctx` via the `op_ctx->map.bo` path. This is important — the pin acquired in `panthor_vm_prepare_map_op_ctx` is now released when the op_ctx is cleaned up, not when the vm_bo is freed.

6. **FW BO pinning**: Patch adds explicit `panthor_gem_pin()` / `panthor_gem_unpin()` for FW BOs in `panthor_kernel_bo_create()` / `panthor_kernel_bo_destroy()`. This ensures firmware memory is never reclaimed. Makes sense.

7. **`remap_evicted_vma` / `select_evicted_vma` dance**: The comment explains the lock drop between selection and remapping is needed because PT allocation (in `panthor_vm_op_ctx_prealloc_pts`) can't happen under `op_lock` (taken in DMA signalling path). The double-check with the `evicted` flag is the correct approach to handle the race.

8. **`panthor_vm_unlock_region` called unconditionally in `remap_evicted_vma`**: After `panthor_vm_lock_region()` potentially fails:
   ```c
   ret = panthor_vm_lock_region(vm, ...);
   if (!ret) {
       ret = panthor_vm_map_pages(vm, ...);
   }
   ...
   panthor_vm_unlock_region(vm);
   ```
   If `panthor_vm_lock_region()` fails, `panthor_vm_unlock_region()` is still called. Looking at the implementation, `panthor_vm_unlock_region` just resets `vm->locked_region` — this should be harmless but is slightly sloppy. If `lock_region` failed, there's nothing to unlock.

9. **`shrink_set` debugfs**: The `fs_reclaim_acquire/release` wrapper around the scan is a nice touch for lockdep annotation. But `panthor_gem_shrinker_scan` is not static — it's used here and as a callback. The function being called directly (not through `shrinker->scan_objects`) means it won't go through any shrinker framework wrapper. This is fine for testing purposes.

10. **`can_block` check**: The condition `current_is_kswapd() || (sc->gfp_mask & __GFP_RECLAIM)` seems redundant — `__GFP_DIRECT_RECLAIM` is already checked (and is a subset of `__GFP_RECLAIM`). If `__GFP_DIRECT_RECLAIM` is set, `__GFP_RECLAIM` is also set. The `current_is_kswapd()` check is the meaningful part here, since kswapd can block even with just `__GFP_RECLAIM`.

---
Generated by Claude Code Patch Reviewer