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: Implement evicted status for GEM objects
Date: Thu, 23 Apr 2026 08:36:41 +1000
Message-ID: <review-patch2-20260421-panthor-bo-reclaim-observability-v2-2-c9135eedfb6f@collabora.com>
In-Reply-To: <20260421-panthor-bo-reclaim-observability-v2-2-c9135eedfb6f@collabora.com>
References: <20260421-panthor-bo-reclaim-observability-v2-0-c9135eedfb6f@collabora.com> <20260421-panthor-bo-reclaim-observability-v2-2-c9135eedfb6f@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

This is the meat of the series. It adds `atomic_t reclaimed_count` to `pant=
hor_gem_object`, increments it during eviction, and uses it to report statu=
s via both fdinfo and debugfs.

**Correctness of the eviction path ordering is good:**
```c
atomic_add_unless(&bo->reclaimed_count, 1, INT_MAX);

panthor_gem_dev_map_cleanup_locked(bo);
panthor_gem_backing_cleanup_locked(bo);
```
The counter is incremented *before* pages are cleared, which avoids a windo=
w where a concurrent lockless reader in `panthor_gem_status()` could see th=
e object as neither resident nor evicted. This is the right order.

**The status callback logic is correct:**
```c
if (drm_gem_is_imported(&bo->base) || bo->backing.pages)
    res |=3D DRM_GEM_OBJECT_RESIDENT;
else if (atomic_read(&bo->reclaimed_count))
    res |=3D DRM_GEM_OBJECT_EVICTED;
```
The `else if` correctly makes RESIDENT and EVICTED mutually exclusive, and =
the `reclaimed_count` check distinguishes evicted objects from objects that=
 were never backed. The `atomic_read()` is appropriate here since `panthor_=
gem_status()` can be called under a spinlock without holding the BO's own l=
ock.

**Debugfs evicted state flag logic:**
```c
if (drm_gem_is_imported(&bo->base))
    gem_state_flags |=3D PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED;
else if (!resident_size && reclaimed_count)
    gem_state_flags |=3D PANTHOR_DEBUGFS_GEM_STATE_FLAG_EVICTED;

if (bo->base.dma_buf)
    gem_state_flags |=3D PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED;
```
Note that `EVICTED` and `IMPORTED` are mutually exclusive via `else if`, bu=
t `EVICTED` and `EXPORTED` are not (an exported BO that gets evicted would =
show both flags). Is that the desired behavior? An exported BO could concei=
vably be evicted, so allowing both flags seems reasonable. But the fdinfo p=
ath in `panthor_gem_status()` similarly makes EVICTED exclusive with import=
ed =E2=80=94 worth confirming this is intentional for imported buffers (whi=
ch typically have backing managed by the exporter, so they shouldn't get ev=
icted anyway).

**Minor: `int reclaimed_count` local variable.** The `atomic_read()` return=
s `int`, so the local is correctly typed. However, the debugfs format uses =
`%-11d` which handles 10-digit signed ints. Since the value saturates at IN=
T_MAX (~2.1 billion, 10 digits), `%-11d` (10 digits + 1 space) is exactly r=
ight.

**No initialization needed** =E2=80=94 `panthor_gem_object` is allocated vi=
a `kzalloc` (through `drm_gem_shmem_create`), so `reclaimed_count` starts a=
t 0 implicitly. Good.

---
Generated by Claude Code Patch Reviewer