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: Track the number of mmap on a BO
Date: Tue, 10 Mar 2026 12:15:42 +1000
Message-ID: <review-patch8-20260309151119.290217-9-boris.brezillon@collabora.com>
In-Reply-To: <20260309151119.290217-9-boris.brezillon@collabora.com>
References: <20260309151119.290217-1-boris.brezillon@collabora.com> <20260309151119.290217-9-boris.brezillon@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

Tracks mmap count using `refcount_t` to determine reclaimability ordering. =
The pattern used is:

```c
if (!refcount_inc_not_zero(&bo->cmap.mmap_count)) {
    dma_resv_lock(obj->resv, NULL);
    if (!refcount_inc_not_zero(&bo->cmap.mmap_count))
        refcount_set(&bo->cmap.mmap_count, 1);
    dma_resv_unlock(obj->resv);
}
```

This double-check pattern uses the resv lock as the serialization mechanism=
 for the 0=E2=86=921 transition. The `refcount_set` on the 0=E2=86=921 path=
 is intentional because `refcount_inc` from zero triggers a WARN with `CONF=
IG_REFCOUNT_FULL`. The outer `refcount_inc_not_zero` is a fast path to avoi=
d the lock for the common N=E2=86=92N+1 case.

The `panthor_gem_vm_close` uses a similar pattern for the 1=E2=86=920 trans=
ition:

```c
if (refcount_dec_not_one(&bo->cmap.mmap_count))
    goto out;

dma_resv_lock(bo->base.resv, NULL);
if (refcount_dec_and_test(&bo->cmap.mmap_count)) {
    /* Nothing to do, pages are reclaimed lazily. */
}
dma_resv_unlock(bo->base.resv);
```

**Potential issue**: `refcount_dec_not_one()` returns true when the count w=
as decremented (i.e., went from N>1 to N-1). If it returns false (count was=
 1), the code takes the lock and calls `refcount_dec_and_test()`. But betwe=
en `refcount_dec_not_one()` returning false and the lock being acquired, an=
other thread could have incremented the refcount (via mmap). In that case `=
refcount_dec_and_test()` would decrement but not reach zero, which is fine =
=E2=80=94 but the lock was acquired unnecessarily. The correctness holds be=
cause the resv lock serializes the critical state transitions.

One nit: `panthor_gem_vm_open` calls `drm_gem_is_imported()` to skip the mm=
ap_count increment for imported BOs. But `panthor_gem_mmap` redirects to `d=
ma_buf_mmap` for imported BOs and returns early, so the mmap_count would ne=
ver have been incremented in the first place. The check is defensive but sl=
ightly misleading.

No blocking issues.

---
Generated by Claude Code Patch Reviewer