Claude review: drm/panthor: Implement evicted status for GEM objects

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

Patch Review

**Summary:** Adds an `atomic_t reclaimed_count` field to `panthor_gem_object`, increments it in the evict path, and uses it to report `DRM_GEM_OBJECT_EVICTED` via the `.status` callback and the debugfs gems interface.

**Review:**

**Issue 1 (design): The wrap-around avoidance is over-engineered for a boolean need.**

```c
	/* Don't ever wrap around as far as 0, jump from INT_MIN to 1 */
	if (!atomic_inc_unless_negative(&bo->reclaimed_count))
		atomic_set(&bo->reclaimed_count, 1);
```

The only consumer of `reclaimed_count` checks `!= 0`:
```c
	else if (atomic_read(&bo->reclaimed_count))
		res |= DRM_GEM_OBJECT_EVICTED;
```

If the sole purpose is "has this BO ever been evicted", a simple `bool` or even a flag bit would suffice. The commit message justifies the counter by saying "It's possible to distinguish evicted non-resident pages from newly allocated non-resident pages by checking whether reclaimed_count is != 0" — but that's the same as a boolean.

If there's a genuine use case for knowing *how many times* a BO was evicted (e.g., for future debugfs stats), the counter makes sense. But in that case, the wrap-around logic is wrong — after wrap-around, the count is no longer meaningful. And `atomic_inc_unless_negative` is not a cheap operation (it's a cmpxchg loop), unlike a simple `atomic_inc` or `atomic_set`.

Suggestion: Either use a simpler `bool evicted` (or a flag bit) if you only need the boolean, or use a plain `atomic_inc` if you want a counter (wrap-around after 2^31 evictions is not a real-world concern, and the count being "wrong" after wrap is no worse than clamping at 1).

**Issue 2 (atomics concern): `atomic_set` after failed `atomic_inc_unless_negative` is not atomic with respect to concurrent reads.**

```c
	if (!atomic_inc_unless_negative(&bo->reclaimed_count))
		atomic_set(&bo->reclaimed_count, 1);
```

If two threads hit this simultaneously when `reclaimed_count` is `INT_MIN`, both could see `atomic_inc_unless_negative` fail (since `INT_MIN` is negative), and both call `atomic_set(&bo->reclaimed_count, 1)`. That's fine — the result is still 1. But there's a window where a concurrent `atomic_read` from `panthor_gem_status` could observe 0 if the set hasn't completed. In practice, this is harmless since:
- Eviction holds a lock (`panthor_gem_evict_locked`)
- The status read is racy by design (documented in `drm_gem_object_funcs.status`)

But it would be cleaner to just use `atomic_inc` and accept that the counter can wrap, or use a simple flag.

**Issue 3 (logic): The `else if` chain in `panthor_gem_status` means imported objects can never be reported as evicted.**

```c
	if (drm_gem_is_imported(&bo->base) || bo->backing.pages)
		res |= DRM_GEM_OBJECT_RESIDENT;
	else if (atomic_read(&bo->reclaimed_count))
		res |= DRM_GEM_OBJECT_EVICTED;
```

Since imported objects always get `RESIDENT`, they can never get `EVICTED`. This is probably correct (imported objects shouldn't be evicted by the panthor shrinker), but worth confirming — does the panthor shrinker skip imported BOs? If an imported BO were somehow evicted, the status would incorrectly show it as `RESIDENT`.

**Issue 4 (debugfs): Same `else if` pattern with different condition for debugfs.**

```c
	if (drm_gem_is_imported(&bo->base))
		gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED;
	else if (!resident_size && atomic_read(&bo->reclaimed_count))
		gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_EVICTED;

	if (bo->base.dma_buf)
		gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED;
```

Note the `else if` means an imported BO will never show as evicted in debugfs either. An exported BO *can* show as evicted (since the exported check uses a separate `if`), which is inconsistent with the imported BO treatment. If an exported BO is evicted, it will show both "exported" and "evicted" flags in debugfs, which seems correct. But an imported+evicted BO (if possible) would only show "imported". This is a minor consistency concern.

**Issue 5 (missing initialization): `reclaimed_count` is not explicitly initialized.**

The new `atomic_t reclaimed_count` field in `panthor_gem_object` is not explicitly initialized to 0 in any allocation path. This relies on the GEM object being allocated with `kzalloc` (via shmem helpers), which zero-initializes the entire struct. This works correctly for `atomic_t` since `ATOMIC_INIT(0)` is just `{ 0 }`, but it's worth noting that this is an implicit dependency on zero-initialization.

**Issue 6 (nit): Comment style.**

```c
	/* Don't ever wrap around as far as 0, jump from INT_MIN to 1 */
```

This is a reasonable comment given the non-obvious wrap-around handling, though it could be simplified if the approach itself is simplified.

**Positive aspects:**
- The debugfs flag naming table is correctly extended
- The `else if` logic in `panthor_gem_status` correctly makes `EVICTED` mutually exclusive with `RESIDENT`
- The commit message is well-written and clearly explains the rationale

---

**Summary:** The series is architecturally sound and fills a real observability gap. The main feedback is: (1) consider whether `obj->size` is more appropriate than `add_size` for the evicted counter in patch 1, (2) simplify the `reclaimed_count` to either a plain boolean flag or a simple `atomic_inc` counter in patch 2, and (3) add documentation about `EVICTED`/`RESIDENT` mutual exclusivity to the enum comments.

---
Generated by Claude Code Patch Reviewer