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: Implement evicted status for GEM objects Date: Mon, 25 May 2026 20:03:23 +1000 Message-ID: In-Reply-To: <20260521-panthor-bo-reclaim-observability-v5-1-49313994da55@collabora.com> References: <20260521-panthor-bo-reclaim-observability-v5-0-49313994da55@collabora.com> <20260521-panthor-bo-reclaim-observability-v5-1-49313994da55@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 **Commit message vs. diff mismatch:** The commit message says: > Use this new member to then set the appropriate DRM_GEM_OBJECT_EVICTED st= atus flag for fdinfo. But this patch does not touch fdinfo or reference `DRM_GEM_OBJECT_EVICTED` = anywhere in the diff. The fdinfo patch was dropped in v5 (as noted in the c= over letter), but the commit message wasn't updated to reflect that. This s= hould be fixed before merging. **Code review:** The `atomic_add_unless(&bo->reclaimed_count, 1, INT_MAX)` at line 690 (in t= he patched file) is correct =E2=80=94 it atomically increments unless alrea= dy at INT_MAX, preventing wraparound. The placement before `panthor_gem_bac= king_cleanup_locked` is logical since you want to record that eviction happ= ened before the backing is torn down. ```c + atomic_add_unless(&bo->reclaimed_count, 1, INT_MAX); + panthor_gem_dev_map_cleanup_locked(bo); panthor_gem_backing_cleanup_locked(bo); ``` The new `reclaimed_count` field in the header is well-documented: ```c + /** + * @reclaimed_count: How many times object has been evicted to swap. + * The count saturates at %INT_MAX and will never wrap around to 0. + */ + atomic_t reclaimed_count; ``` No explicit initialization is needed since `panthor_gem_object` is allocate= d with `kzalloc` (via `drm_gem_object_init`), which zeros the atomic. The evicted flag logic is correct: ```c + else if (!resident_size && reclaimed_count) + gem_state_flags |=3D PANTHOR_DEBUGFS_GEM_STATE_FLAG_EVICTED; ``` This correctly distinguishes "evicted and not yet faulted back in" (`!resid= ent_size && reclaimed_count > 0`) from "newly allocated but not yet backed"= (`!resident_size && reclaimed_count =3D=3D 0`). Using `else if` after the = imported check is also correct =E2=80=94 an imported BO wouldn't be evicted= by panthor's shrinker. The format string change adds the new `%-11d` column for `reclaimed_count`: ```c - seq_printf(m, "%-32s%-16d%-16d%-16zd%-16zd0x%-16lx", + seq_printf(m, "%-32s%-16d%-16d%-11d%-16zd%-16zd0x%-16lx", ``` This uses width 11 for the evictions column, which is consistent with what = patch 2 later sets for refcount. For an `int` (max 10 digits for INT_MAX=3D= 2147483647), 11 chars (10 digits + 1 separator) is sufficient. **Minor note:** The `reclaimed_count` field is read without holding any loc= k in the debugfs path (`atomic_read`), while it's incremented in `panthor_g= em_evict_locked` under `dma_resv` and `gpuva.lock`. This is fine =E2=80=94 = `atomic_read` provides a consistent snapshot of the counter and exact preci= sion isn't required for debugfs output. --- Generated by Claude Code Patch Reviewer