public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
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	[thread overview]
Message-ID: <review-patch2-20260421-panthor-bo-reclaim-observability-v2-2-c9135eedfb6f@collabora.com> (raw)
In-Reply-To: <20260421-panthor-bo-reclaim-observability-v2-2-c9135eedfb6f@collabora.com>

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

  reply	other threads:[~2026-04-22 22:36 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-21 10:45 [PATCH v2 0/3] Let userspace know about swapped out panthor GEM objects Nicolas Frattaroli
2026-04-21 10:45 ` [PATCH v2 1/3] drm/fdinfo: Add "evicted" memory accounting Nicolas Frattaroli
2026-04-22 10:59   ` Steven Price
2026-04-22 22:36   ` Claude review: " Claude Code Review Bot
2026-04-21 10:45 ` [PATCH v2 2/3] drm/panthor: Implement evicted status for GEM objects Nicolas Frattaroli
2026-04-22 22:36   ` Claude Code Review Bot [this message]
2026-04-21 10:45 ` [PATCH v2 3/3] drm/panthor: Reduce padding in gems debugfs for refcount Nicolas Frattaroli
2026-04-22 22:36   ` Claude review: " Claude Code Review Bot
2026-04-22 22:36 ` 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-04-20 15:46 [PATCH 0/2] " 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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=review-patch2-20260421-panthor-bo-reclaim-observability-v2-2-c9135eedfb6f@collabora.com \
    --to=claude-review@example.com \
    --cc=dri-devel-reviews@example.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox