From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/panfrost: Make reset sequence deal with an active HWPerf session Date: Wed, 27 May 2026 15:22:49 +1000 Message-ID: In-Reply-To: <20260526-claude-fixes-v1-2-16e92eaa4949@collabora.com> References: <20260526-claude-fixes-v1-0-16e92eaa4949@collabora.com> <20260526-claude-fixes-v1-2-16e92eaa4949@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Status: Reasonable approach with one ordering concern.** The design is sound: during a GPU reset, if perfcnt is active, do the `panfrost_mmu_as_put()` before `panfrost_mmu_reset()` zeroes the refcounts, and set a flag so the later disable path skips the (now-redundant) put. This prevents the WARN at `panfrost_mmu.c:336`: ```c atomic_dec(&mmu->as_count); WARN_ON(atomic_read(&mmu->as_count) < 0); ``` **Refactoring of GPU register writes** into `panfrost_perfcnt_gpu_disable()` is clean and eliminates duplication across init, fini, disable, and the new reset path. One note: the original `panfrost_perfcnt_disable_locked` wrote the enable registers first and the CFG mode-off last, while `panfrost_perfcnt_gpu_disable` writes CFG mode-off first: ```c +static void panfrost_perfcnt_gpu_disable(struct panfrost_device *pfdev) +{ + gpu_write(pfdev, GPU_PERFCNT_CFG, + GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF)); + gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0x0); ``` This order reversal is fine for disable (clearing config then enables is no worse than the reverse), but it's worth noting intentionally. **Ordering concern**: `panfrost_perfcnt_reset()` is called after `panfrost_gpu_soft_reset()` but *before* `panfrost_gpu_power_on()`: ```c panfrost_gpu_soft_reset(pfdev); panfrost_perfcnt_reset(pfdev); // <-- here panfrost_gpu_power_on(pfdev); ``` Inside `panfrost_perfcnt_reset()`, `panfrost_perfcnt_gpu_disable()` writes to GPU registers and `panfrost_mmu_as_put()` may write to MMU registers -- all on a soft-reset GPU that hasn't been powered back on. The register writes are likely harmless (they'll be overwritten by the subsequent `panfrost_mmu_reset`), but it would be cleaner to skip the `panfrost_perfcnt_gpu_disable()` call here since the GPU was just soft-reset and the registers are already in their default state. Only the `panfrost_mmu_as_put()` and the flag-set are needed in the reset path. **EAGAIN semantics**: Returning `-EAGAIN` from `panfrost_ioctl_perfcnt_dump` is a reasonable choice, signaling the user to retry after a disable/enable cycle. The comment explains this well. --- --- Generated by Claude Code Patch Reviewer