public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* Claude review: drm/panthor: Implement evicted status for GEM objects
  2026-04-21 10:45 ` [PATCH v2 2/3] drm/panthor: Implement evicted status for " Nicolas Frattaroli
@ 2026-04-22 22:36   ` Claude Code Review Bot
  0 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 22:36 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This is the meat of the series. It adds `atomic_t reclaimed_count` to `panthor_gem_object`, increments it during eviction, and uses it to report status 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 window where a concurrent lockless reader in `panthor_gem_status()` could see the 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 |= DRM_GEM_OBJECT_RESIDENT;
else if (atomic_read(&bo->reclaimed_count))
    res |= 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 lock.

**Debugfs evicted state flag logic:**
```c
if (drm_gem_is_imported(&bo->base))
    gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED;
else if (!resident_size && 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 that `EVICTED` and `IMPORTED` are mutually exclusive via `else if`, but `EVICTED` and `EXPORTED` are not (an exported BO that gets evicted would show both flags). Is that the desired behavior? An exported BO could conceivably be evicted, so allowing both flags seems reasonable. But the fdinfo path in `panthor_gem_status()` similarly makes EVICTED exclusive with imported — worth confirming this is intentional for imported buffers (which typically have backing managed by the exporter, so they shouldn't get evicted anyway).

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

**No initialization needed** — `panthor_gem_object` is allocated via `kzalloc` (through `drm_gem_shmem_create`), so `reclaimed_count` starts at 0 implicitly. Good.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Claude review: drm/panthor: Implement evicted status for GEM objects
  2026-04-20 15:47 ` [PATCH 2/2] drm/panthor: Implement evicted status for " Nicolas Frattaroli
@ 2026-04-22 23:29   ` Claude Code Review Bot
  0 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 23:29 UTC (permalink / raw)
  To: dri-devel-reviews

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v5 0/2] Let userspace know about swapped out panthor GEM objects
@ 2026-05-21 13:51 Nicolas Frattaroli
  2026-05-21 13:51 ` [PATCH v5 1/2] drm/panthor: Implement evicted status for " Nicolas Frattaroli
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Nicolas Frattaroli @ 2026-05-21 13:51 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Boris Brezillon, Steven Price, Liviu Dudau
  Cc: dri-devel, linux-kernel, kernel, Nicolas Frattaroli

Panthor has recently gained a GEM shrinker. It allows evicting memory
that backs unused GEM objects to swap.

In this series, Panthor's gems debugfs is extended so that information
on evicted pages can be gathered by users.

---
Changes in v5:
- Drop fdinfo patch to unblock series
- Don't set the flag introduced in fdinfo patch in panthor
- Link to v4: https://patch.msgid.link/20260520-panthor-bo-reclaim-observability-v4-0-a47ab61cb80d@collabora.com

Changes in v4:
- Change "evicted" memory type documentation to no longer explicitly
  mention swap
- Link to v3: https://patch.msgid.link/20260423-panthor-bo-reclaim-observability-v3-0-60af32164a4f@collabora.com

Changes in v3:
- Add documentation for new "evicted" memory type in fdinfo
- Link to v2: https://patch.msgid.link/20260421-panthor-bo-reclaim-observability-v2-0-c9135eedfb6f@collabora.com

Changes in v2:
- Change reclaimed_count to saturate at INT_MAX
- Add "evictions" column to panthor gems debugfs which prints
  reclaimed_count
- Add a patch to reduce the padding of one panthor gems debugfs column a
  bit
- Link to v1: https://patch.msgid.link/20260420-panthor-bo-reclaim-observability-v1-0-a4d1a36ee84f@collabora.com

To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Boris Brezillon <boris.brezillon@collabora.com>
To: Steven Price <steven.price@arm.com>
To: Liviu Dudau <liviu.dudau@arm.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel@collabora.com
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

---
Nicolas Frattaroli (2):
      drm/panthor: Implement evicted status for GEM objects
      drm/panthor: Reduce padding in gems debugfs for refcount

 drivers/gpu/drm/panthor/panthor_gem.c | 16 ++++++++++++----
 drivers/gpu/drm/panthor/panthor_gem.h | 10 ++++++++++
 2 files changed, 22 insertions(+), 4 deletions(-)
---
base-commit: 69c95e4c529297c25503e60acba757fba24fdc95
change-id: 20260420-panthor-bo-reclaim-observability-970679c9533c

Best regards,
--  
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v5 1/2] drm/panthor: Implement evicted status for GEM objects
  2026-05-21 13:51 [PATCH v5 0/2] Let userspace know about swapped out panthor GEM objects Nicolas Frattaroli
@ 2026-05-21 13:51 ` Nicolas Frattaroli
  2026-05-21 14:44   ` Nicolas Frattaroli
  2026-05-25 10:03   ` Claude review: " Claude Code Review Bot
  2026-05-21 13:51 ` [PATCH v5 2/2] drm/panthor: Reduce padding in gems debugfs for refcount Nicolas Frattaroli
  2026-05-25 10:03 ` Claude review: Let userspace know about swapped out panthor GEM objects Claude Code Review Bot
  2 siblings, 2 replies; 10+ messages in thread
From: Nicolas Frattaroli @ 2026-05-21 13:51 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Boris Brezillon, Steven Price, Liviu Dudau
  Cc: dri-devel, linux-kernel, kernel, Nicolas Frattaroli

For fdinfo to be able to fill its evicted counter with data, panthor
needs to keep track of whether a GEM object has ever been reclaimed.
Just checking whether the pages are resident isn't enough, as newly
allocated objects also won't be resident.

Do this with a new atomic_t member on panthor_gem_object. It's increased
when an object gets evicted by the shrinker, and saturates at INT_MAX.
This means that once an object has been evicted at least once, its
reclaim counter will never return to 0.

Due to this, it's possible to distinguish evicted non-resident pages
from newly allocated non-resident pages by checking whether
reclaimed_count is != 0

Use this new member to then set the appropriate DRM_GEM_OBJECT_EVICTED
status flag for fdinfo.

Also add a new column and status flag to the panthor gems debugfs: the
column is the number of times an object has been evicted, whereas the
flag indicates whether it currently is evicted.

Reviewed-by: Steven Price <steven.price@arm.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_gem.c | 16 ++++++++++++----
 drivers/gpu/drm/panthor/panthor_gem.h | 10 ++++++++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
index 13295d7a593d..25bb62a48276 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.c
+++ b/drivers/gpu/drm/panthor/panthor_gem.c
@@ -687,6 +687,8 @@ static void panthor_gem_evict_locked(struct panthor_gem_object *bo)
 	if (drm_WARN_ON_ONCE(bo->base.dev, !bo->backing.pages))
 		return;
 
+	atomic_add_unless(&bo->reclaimed_count, 1, INT_MAX);
+
 	panthor_gem_dev_map_cleanup_locked(bo);
 	panthor_gem_backing_cleanup_locked(bo);
 	panthor_gem_update_reclaim_state_locked(bo, NULL);
@@ -1595,6 +1597,7 @@ static void panthor_gem_debugfs_print_flag_names(struct seq_file *m)
 	static const char * const gem_state_flags_names[] = {
 		[PANTHOR_DEBUGFS_GEM_STATE_IMPORTED_BIT] = "imported",
 		[PANTHOR_DEBUGFS_GEM_STATE_EXPORTED_BIT] = "exported",
+		[PANTHOR_DEBUGFS_GEM_STATE_EVICTED_BIT] = "evicted",
 	};
 
 	static const char * const gem_usage_flags_names[] = {
@@ -1625,6 +1628,7 @@ static void panthor_gem_debugfs_bo_print(struct panthor_gem_object *bo,
 {
 	enum panthor_gem_reclaim_state reclaim_state = bo->reclaim_state;
 	unsigned int refcount = kref_read(&bo->base.refcount);
+	int reclaimed_count = atomic_read(&bo->reclaimed_count);
 	char creator_info[32] = {};
 	size_t resident_size;
 	u32 gem_usage_flags = bo->debugfs.flags;
@@ -1638,16 +1642,20 @@ static void panthor_gem_debugfs_bo_print(struct panthor_gem_object *bo,
 
 	snprintf(creator_info, sizeof(creator_info),
 		 "%s/%d", bo->debugfs.creator.process_name, bo->debugfs.creator.tgid);
-	seq_printf(m, "%-32s%-16d%-16d%-16zd%-16zd0x%-16lx",
+	seq_printf(m, "%-32s%-16d%-16d%-11d%-16zd%-16zd0x%-16lx",
 		   creator_info,
 		   bo->base.name,
 		   refcount,
+		   reclaimed_count,
 		   bo->base.size,
 		   resident_size,
 		   drm_vma_node_start(&bo->base.vma_node));
 
 	if (drm_gem_is_imported(&bo->base))
 		gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED;
+	else if (!resident_size && 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;
 
@@ -1671,8 +1679,8 @@ static void panthor_gem_debugfs_print_bos(struct panthor_device *ptdev,
 
 	panthor_gem_debugfs_print_flag_names(m);
 
-	seq_puts(m, "created-by                      global-name     refcount        size            resident-size   file-offset       state      usage       label\n");
-	seq_puts(m, "----------------------------------------------------------------------------------------------------------------------------------------------\n");
+	seq_puts(m, "created-by                      global-name     refcount        evictions  size            resident-size   file-offset       state      usage       label\n");
+	seq_puts(m, "---------------------------------------------------------------------------------------------------------------------------------------------------------\n");
 
 	scoped_guard(mutex, &ptdev->gems.lock) {
 		list_for_each_entry(bo, &ptdev->gems.node, debugfs.node) {
@@ -1680,7 +1688,7 @@ static void panthor_gem_debugfs_print_bos(struct panthor_device *ptdev,
 		}
 	}
 
-	seq_puts(m, "==============================================================================================================================================\n");
+	seq_puts(m, "=========================================================================================================================================================\n");
 	seq_printf(m, "Total size: %zd, Total resident: %zd, Total reclaimable: %zd\n",
 		   totals.size, totals.resident, totals.reclaimable);
 }
diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h
index ae0491d0b121..56d63137b4eb 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.h
+++ b/drivers/gpu/drm/panthor/panthor_gem.h
@@ -19,12 +19,16 @@ struct panthor_vm;
 enum panthor_debugfs_gem_state_flags {
 	PANTHOR_DEBUGFS_GEM_STATE_IMPORTED_BIT = 0,
 	PANTHOR_DEBUGFS_GEM_STATE_EXPORTED_BIT = 1,
+	PANTHOR_DEBUGFS_GEM_STATE_EVICTED_BIT = 2,
 
 	/** @PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED: GEM BO is PRIME imported. */
 	PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED = BIT(PANTHOR_DEBUGFS_GEM_STATE_IMPORTED_BIT),
 
 	/** @PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED: GEM BO is PRIME exported. */
 	PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED = BIT(PANTHOR_DEBUGFS_GEM_STATE_EXPORTED_BIT),
+
+	/** @PANTHOR_DEBUGFS_GEM_STATE_FLAG_EVICTED: GEM BO is evicted to swap. */
+	PANTHOR_DEBUGFS_GEM_STATE_FLAG_EVICTED = BIT(PANTHOR_DEBUGFS_GEM_STATE_EVICTED_BIT),
 };
 
 enum panthor_debugfs_gem_usage_flags {
@@ -172,6 +176,12 @@ struct panthor_gem_object {
 	/** @reclaim_state: Cached reclaim state */
 	enum panthor_gem_reclaim_state reclaim_state;
 
+	/**
+	 * @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;
+
 	/**
 	 * @exclusive_vm_root_gem: Root GEM of the exclusive VM this GEM object
 	 * is attached to.

-- 
2.54.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 2/2] drm/panthor: Reduce padding in gems debugfs for refcount
  2026-05-21 13:51 [PATCH v5 0/2] Let userspace know about swapped out panthor GEM objects Nicolas Frattaroli
  2026-05-21 13:51 ` [PATCH v5 1/2] drm/panthor: Implement evicted status for " Nicolas Frattaroli
@ 2026-05-21 13:51 ` Nicolas Frattaroli
  2026-05-25 10:03   ` Claude review: " Claude Code Review Bot
  2026-05-25 10:03 ` Claude review: Let userspace know about swapped out panthor GEM objects Claude Code Review Bot
  2 siblings, 1 reply; 10+ messages in thread
From: Nicolas Frattaroli @ 2026-05-21 13:51 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Boris Brezillon, Steven Price, Liviu Dudau
  Cc: dri-devel, linux-kernel, kernel, Nicolas Frattaroli

The "gems" debugfs file is getting a little too wide for comfort. While
a lot of this is unavoidable due to the theoretical upper limits of
numbers here (e.g. size needs to be 16 chars because 2**48-1 in decimal
is 15 digits, plus one space for separation), the refcount column has a
decent 5 characters to be saved, as it can only ever contain a 10-digit
decimal number.

Reduce the refcount column's width to 11, which fulfils this requirement
with an additional space for separation.

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_gem.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
index 25bb62a48276..ac9b9854c729 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.c
+++ b/drivers/gpu/drm/panthor/panthor_gem.c
@@ -1642,7 +1642,7 @@ static void panthor_gem_debugfs_bo_print(struct panthor_gem_object *bo,
 
 	snprintf(creator_info, sizeof(creator_info),
 		 "%s/%d", bo->debugfs.creator.process_name, bo->debugfs.creator.tgid);
-	seq_printf(m, "%-32s%-16d%-16d%-11d%-16zd%-16zd0x%-16lx",
+	seq_printf(m, "%-32s%-16d%-11d%-11d%-16zd%-16zd0x%-16lx",
 		   creator_info,
 		   bo->base.name,
 		   refcount,
@@ -1679,8 +1679,8 @@ static void panthor_gem_debugfs_print_bos(struct panthor_device *ptdev,
 
 	panthor_gem_debugfs_print_flag_names(m);
 
-	seq_puts(m, "created-by                      global-name     refcount        evictions  size            resident-size   file-offset       state      usage       label\n");
-	seq_puts(m, "---------------------------------------------------------------------------------------------------------------------------------------------------------\n");
+	seq_puts(m, "created-by                      global-name     refcount   evictions  size            resident-size   file-offset       state      usage       label\n");
+	seq_puts(m, "----------------------------------------------------------------------------------------------------------------------------------------------------\n");
 
 	scoped_guard(mutex, &ptdev->gems.lock) {
 		list_for_each_entry(bo, &ptdev->gems.node, debugfs.node) {
@@ -1688,7 +1688,7 @@ static void panthor_gem_debugfs_print_bos(struct panthor_device *ptdev,
 		}
 	}
 
-	seq_puts(m, "=========================================================================================================================================================\n");
+	seq_puts(m, "====================================================================================================================================================\n");
 	seq_printf(m, "Total size: %zd, Total resident: %zd, Total reclaimable: %zd\n",
 		   totals.size, totals.resident, totals.reclaimable);
 }

-- 
2.54.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 1/2] drm/panthor: Implement evicted status for GEM objects
  2026-05-21 13:51 ` [PATCH v5 1/2] drm/panthor: Implement evicted status for " Nicolas Frattaroli
@ 2026-05-21 14:44   ` Nicolas Frattaroli
  2026-05-25 10:03   ` Claude review: " Claude Code Review Bot
  1 sibling, 0 replies; 10+ messages in thread
From: Nicolas Frattaroli @ 2026-05-21 14:44 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Boris Brezillon, Steven Price, Liviu Dudau
  Cc: dri-devel, linux-kernel, kernel

On Thursday, 21 May 2026 15:51:15 Central European Summer Time Nicolas Frattaroli wrote:
> For fdinfo to be able to fill its evicted counter with data, panthor
> needs to keep track of whether a GEM object has ever been reclaimed.
> Just checking whether the pages are resident isn't enough, as newly
> allocated objects also won't be resident.
> 
> Do this with a new atomic_t member on panthor_gem_object. It's increased
> when an object gets evicted by the shrinker, and saturates at INT_MAX.
> This means that once an object has been evicted at least once, its
> reclaim counter will never return to 0.
> 
> Due to this, it's possible to distinguish evicted non-resident pages
> from newly allocated non-resident pages by checking whether
> reclaimed_count is != 0
> 
> Use this new member to then set the appropriate DRM_GEM_OBJECT_EVICTED
> status flag for fdinfo.
> 

Oops, feel free to drop this paragraph in the commit message when
applying.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Claude review: Let userspace know about swapped out panthor GEM objects
  2026-05-21 13:51 [PATCH v5 0/2] Let userspace know about swapped out panthor GEM objects Nicolas Frattaroli
  2026-05-21 13:51 ` [PATCH v5 1/2] drm/panthor: Implement evicted status for " Nicolas Frattaroli
  2026-05-21 13:51 ` [PATCH v5 2/2] drm/panthor: Reduce padding in gems debugfs for refcount Nicolas Frattaroli
@ 2026-05-25 10:03 ` Claude Code Review Bot
  2 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 10:03 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: Let userspace know about swapped out panthor GEM objects
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Patches: 4
Reviewed: 2026-05-25T20:03:23.030343

---

This is a clean, small 2-patch series (v5) that adds eviction observability to panthor's GEM debugfs. The changes are well-scoped: patch 1 adds a `reclaimed_count` atomic to track evictions with proper saturation, exposes it via debugfs, and adds an "evicted" state flag; patch 2 tightens column widths. Both patches carry Reviewed-by tags from Steven Price and Boris Brezillon.

The implementation is straightforward and correct. The use of `atomic_add_unless` for saturation at INT_MAX is appropriate. The ordering of the patches is slightly unusual — the column width reduction in patch 2 is applied to widths that were already set in patch 1 — but this is a minor aesthetic issue in sequencing, not a correctness problem. The cover letter mentions dropping an fdinfo patch from v4 to unblock the series, and the commit message for patch 1 still references fdinfo and `DRM_GEM_OBJECT_EVICTED` which no longer appears in the diff; this is a minor commit message hygiene issue.

**No blocking issues found.** This series looks ready to merge.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Claude review: drm/panthor: Implement evicted status for GEM objects
  2026-05-21 13:51 ` [PATCH v5 1/2] drm/panthor: Implement evicted status for " Nicolas Frattaroli
  2026-05-21 14:44   ` Nicolas Frattaroli
@ 2026-05-25 10:03   ` Claude Code Review Bot
  1 sibling, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 10:03 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Commit message vs. diff mismatch:**

The commit message says:
> Use this new member to then set the appropriate DRM_GEM_OBJECT_EVICTED status 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 cover letter), but the commit message wasn't updated to reflect that. This should be fixed before merging.

**Code review:**

The `atomic_add_unless(&bo->reclaimed_count, 1, INT_MAX)` at line 690 (in the patched file) is correct — it atomically increments unless already at INT_MAX, preventing wraparound. The placement before `panthor_gem_backing_cleanup_locked` is logical since you want to record that eviction happened 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 allocated 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 |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_EVICTED;
```

This correctly distinguishes "evicted and not yet faulted back in" (`!resident_size && reclaimed_count > 0`) from "newly allocated but not yet backed" (`!resident_size && reclaimed_count == 0`). Using `else if` after the imported check is also correct — 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=2147483647), 11 chars (10 digits + 1 separator) is sufficient.

**Minor note:** The `reclaimed_count` field is read without holding any lock in the debugfs path (`atomic_read`), while it's incremented in `panthor_gem_evict_locked` under `dma_resv` and `gpuva.lock`. This is fine — `atomic_read` provides a consistent snapshot of the counter and exact precision isn't required for debugfs output.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Claude review: drm/panthor: Reduce padding in gems debugfs for refcount
  2026-05-21 13:51 ` [PATCH v5 2/2] drm/panthor: Reduce padding in gems debugfs for refcount Nicolas Frattaroli
@ 2026-05-25 10:03   ` Claude Code Review Bot
  0 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 10:03 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This is a purely cosmetic patch that reduces the refcount column width from 16 to 11 characters:

```c
-	seq_printf(m, "%-32s%-16d%-11d%-11d%-16zd%-16zd0x%-16lx",
+	seq_printf(m, "%-32s%-16d%-11d%-11d%-16zd%-16zd0x%-16lx",
```

Wait — looking more carefully at the diff: in the base from patch 1, the format string was `%-16d` for refcount, and this patch changes it to `%-11d`. The header/separator lines are updated consistently to match the narrower column.

The rationale is sound: `kref_read` returns an `unsigned int`, but realistically refcount values are small. The commit message says it "can only ever contain a 10-digit decimal number" (UINT_MAX is 4294967295, 10 digits), so 11 chars (10 + separator) is correct.

The header, separator, and footer lines are all consistently shortened by 5 characters. Looks correct.

Already has `Reviewed-by: Steven Price <steven.price@arm.com>`.

**No issues with this patch.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Claude review: drm/panthor: Implement evicted status for GEM objects
  2026-05-20 13:04 ` [PATCH v4 2/3] drm/panthor: Implement evicted status for " Nicolas Frattaroli
@ 2026-05-25 11:52   ` Claude Code Review Bot
  0 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 11:52 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Overall:** Well-designed implementation using `atomic_t` for the reclaimed counter with INT_MAX saturation.

**`atomic_add_unless` for saturation:** Correct approach.

```c
atomic_add_unless(&bo->reclaimed_count, 1, INT_MAX);
```

This atomically increments unless the value is already INT_MAX, providing clean saturation semantics. Placed correctly before the actual eviction cleanup.

**Status callback logic:**

```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;
```

The `else if` correctly makes RESIDENT and EVICTED mutually exclusive. An object that is swapped back in (`backing.pages` non-NULL again) will show as RESIDENT, not EVICTED — which is the correct current-state semantics. The `reclaimed_count` provides the historical record. The distinction between "never had pages" (reclaimed_count == 0, neither RESIDENT nor EVICTED) and "had pages but were evicted" (reclaimed_count > 0, EVICTED) is exactly the gap this series fills.

**Race between `backing.pages` and `reclaimed_count`:** These are read without holding locks in the status callback (called under `file->table_lock` spinlock, not dma_resv). A concurrent shrinker could evict the object between reading `backing.pages` and `reclaimed_count`. This is acceptable — fdinfo is inherently racy and will be correct on the next poll.

**Zero-initialization:** `reclaimed_count` is implicitly zero-initialized via `kzalloc_obj(*bo)` in `panthor_gem_alloc_object()`, so newly allocated objects correctly start with count 0.

**debugfs state flag:**

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

The `else if` is correct — imported BOs are always considered resident by panthor and are not managed by the shrinker, so they can never be in the evicted state.

**Format string update:** The new `%-11d` for `reclaimed_count` is correct (INT_MAX = 2147483647, 10 digits + 1 space = 11 chars). The column header "evictions" (9 chars) fits within the 11-char field.

**No issues with:** enum definitions, kerneldoc for `reclaimed_count`, debugfs flag name table update.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-05-25 11:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-21 13:51 [PATCH v5 0/2] Let userspace know about swapped out panthor GEM objects Nicolas Frattaroli
2026-05-21 13:51 ` [PATCH v5 1/2] drm/panthor: Implement evicted status for " Nicolas Frattaroli
2026-05-21 14:44   ` Nicolas Frattaroli
2026-05-25 10:03   ` Claude review: " Claude Code Review Bot
2026-05-21 13:51 ` [PATCH v5 2/2] drm/panthor: Reduce padding in gems debugfs for refcount Nicolas Frattaroli
2026-05-25 10:03   ` Claude review: " Claude Code Review Bot
2026-05-25 10:03 ` Claude review: Let userspace know about swapped out panthor GEM objects Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-05-20 13:04 [PATCH v4 0/3] " Nicolas Frattaroli
2026-05-20 13:04 ` [PATCH v4 2/3] drm/panthor: Implement evicted status for " Nicolas Frattaroli
2026-05-25 11:52   ` Claude review: " Claude Code Review Bot
2026-04-21 10:45 [PATCH v2 0/3] Let userspace know about swapped out panthor " Nicolas Frattaroli
2026-04-21 10:45 ` [PATCH v2 2/3] drm/panthor: Implement evicted status for " Nicolas Frattaroli
2026-04-22 22:36   ` Claude review: " Claude Code Review Bot
2026-04-20 15:46 [PATCH 0/2] Let userspace know about swapped out panthor " Nicolas Frattaroli
2026-04-20 15:47 ` [PATCH 2/2] drm/panthor: Implement evicted status for " Nicolas Frattaroli
2026-04-22 23:29   ` Claude review: " Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox