From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/panthor: Add a GEM shrinker Date: Tue, 10 Mar 2026 12:15:42 +1000 Message-ID: In-Reply-To: <20260309151119.290217-10-boris.brezillon@collabora.com> References: <20260309151119.290217-1-boris.brezillon@collabora.com> <20260309151119.290217-10-boris.brezillon@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review The main event. Several observations: 1. **`panthor_gem_evaluate_reclaim_state_locked`**: Good hierarchy of recla= imability 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 modifi= ed in `panthor_gem_update_reclaim_state_locked()` without holding `reclaim.= lock`, but read in `panthor_gem_shrinker_count()` also without the lock. Si= nce it's a `long` (word-sized), reads and writes are atomic on most archite= ctures, 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 `pan= thor_vm_lock_region()` fails for one VMA (after successfully evicting other= s), the function returns an error but some VMAs are already evicted. The ca= ller (`panthor_gem_try_evict_no_resv_wait`) then returns false (eviction fa= iled), but the BO is in a partially-evicted state. The GPU will see an inco= nsistent address space. This seems like it could cause GPU faults on the al= ready-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 he= ld, but that's not guaranteed for shared BOs. 4. **`panthor_mmu_reclaim_priv_bos` VM list management**: The check `vm->re= claim.lru_node.prev =3D=3D &vms` to determine if the VM is still in the tem= porary 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 mo= ved by `panthor_vm_active` (which moves to `ptdev->reclaim.vms`) or `pantho= r_vm_free` (which does `list_del_init`). Since the `reclaim.lock` is held w= hen checking, and concurrent moves also hold `reclaim.lock`, this is actual= ly safe. But the `prev =3D=3D &vms` pattern is still fragile =E2=80=94 usin= g `list_empty()` after `list_del_init()` or a dedicated flag would be clean= er. 5. **`panthor_vm_bo_free` changes**: The patch removes the `panthor_gem_unp= in(bo)` call and adds reclaim state update. But the unpin is now done in `p= anthor_vm_cleanup_op_ctx` via the `op_ctx->map.bo` path. This is important = =E2=80=94 the pin acquired in `panthor_vm_prepare_map_op_ctx` is now releas= ed 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_ge= m_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 explai= ns the lock drop between selection and remapping is needed because PT alloc= ation (in `panthor_vm_op_ctx_prealloc_pts`) can't happen under `op_lock` (t= aken in DMA signalling path). The double-check with the `evicted` flag is t= he correct approach to handle the race. 8. **`panthor_vm_unlock_region` called unconditionally in `remap_evicted_vm= a`**: After `panthor_vm_lock_region()` potentially fails: ```c ret =3D panthor_vm_lock_region(vm, ...); if (!ret) { ret =3D panthor_vm_map_pages(vm, ...); } ... panthor_vm_unlock_region(vm); ``` If `panthor_vm_lock_region()` fails, `panthor_vm_unlock_region()` is sti= ll called. Looking at the implementation, `panthor_vm_unlock_region` just r= esets `vm->locked_region` =E2=80=94 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 aroun= d the scan is a nice touch for lockdep annotation. But `panthor_gem_shrinke= r_scan` is not static =E2=80=94 it's used here and as a callback. The funct= ion being called directly (not through `shrinker->scan_objects`) means it w= on't go through any shrinker framework wrapper. This is fine for testing pu= rposes. 10. **`can_block` check**: The condition `current_is_kswapd() || (sc->gfp_m= ask & __GFP_RECLAIM)` seems redundant =E2=80=94 `__GFP_DIRECT_RECLAIM` is a= lready checked (and is a subset of `__GFP_RECLAIM`). If `__GFP_DIRECT_RECLA= IM` 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_REC= LAIM`. --- Generated by Claude Code Patch Reviewer