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/msm/a6xx: Allow IFPC with perfcntr stream Date: Sat, 16 May 2026 10:50:11 +1000 Message-ID: In-Reply-To: <20260514134052.361771-17-robin.clark@oss.qualcomm.com> References: <20260514134052.361771-1-robin.clark@oss.qualcomm.com> <20260514134052.361771-17-robin.clark@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review The final patch that ties it together. Changes `sysprof_setup` to accept a `force_on` parameter, and uses it in `sample_worker` to temporarily inhibit IFPC only while reading counters. **Issue: IFPC inhibit in `sample_worker` without pm_runtime protection.** The `sample_worker` calls `gpu->funcs->sysprof_setup(gpu, true)` but doesn't hold a pm_runtime reference. The hrtimer and sample_work are cancelled in the suspend path, but there's a window where the worker could be scheduled just before suspend. However, since `sample_worker` is a kthread_work queued on `gpu->worker`, and the cancel happens in `msm_perfcntr_suspend_locked()` via `kthread_cancel_work_sync()`, this should be safe - the cancel_work_sync waits for it to finish. **Issue: `msm_gpu_sysprof_no_ifpc` no longer considers perfcntr stream.** After patch 16: ```c static inline bool msm_gpu_sysprof_no_ifpc(struct msm_gpu *gpu) { return refcount_read(&gpu->sysprof_active) > 1; } ``` This means `msm_gpu_sysprof_no_ifpc()` only reflects legacy userspace SYSPROF param, not global counter streaming. IFPC is now managed via the `force_on` parameter to `sysprof_setup()` during sampling. This is the intended behavior - IFPC stays enabled between samples, only inhibited during the read window. The `hw_init` paths that call `msm_gpu_sysprof_no_ifpc()` would correctly only disable IFPC for legacy SYSPROF users, not for the new perfcntr stream. This is correct and well-designed. **`msm_gpu_sysprof_no_perfcntr_zap` still checks for stream.** This ensures that context-switch perfcntr clearing is disabled whenever a global stream is active, even though IFPC is allowed. This is correct - you don't want context switches clearing counters that are being globally sampled. --- **Summary of actionable items:** 1. **Patch 13 (ioctl):** Roll back `allocated_counters` on error paths to avoid corrupting global state 2. **Patch 13 (read):** Consider handling circular buffer wrap-around in a single read call 3. **Patch 13 (includes):** Use angle brackets for system headers (``, ``) 4. **Patch 10:** Consider `kzalloc`/`kfree` instead of `devm_kzalloc`/`devm_kfree` since lifetime is manually managed 5. **Patch 15:** Add bounds checking on reglist writes in `perfcntr_select()` 6. **Patch 10:** Fix typo "correspnding" -> "corresponding" --- Generated by Claude Code Patch Reviewer