Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Enabling perfcnt introduces a long-lived cycle counter reference that is forcefully and incorrectly cleared by a GPU soft reset, permanently breaking cycle counting. Pre-existing issues: - [High] Concurrently submitting a job during a GPU reset causes an unbalanced `panfrost_cycle_counter_put()`, permanently corrupting the cycle counter reference count. -- commit c238efd4d96ef26a59cfb145cb471bac9ab43fe6 Author: Boris Brezillon <[email protected]> drm/panfrost: Enable cycle counting when perfcnt is enabled If we don't do that, we end up with invalid timestamps in the block headers. > diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > index 7020c0192e189..63861b4c7d953 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c [ ... ] > @@ -166,6 +167,7 @@ static int panfrost_perfcnt_enable_locked(struct > panfrost_device *pfdev, > /* The BO ref is retained by the mapping. */ > drm_gem_object_put(&bo->base); > > + panfrost_cycle_counter_get(pfdev); [Severity: High] Does this long-lived cycle counter reference interact correctly with GPU soft resets? Looking at panfrost_gpu_soft_reset(), it seems to assume only in-flight jobs hold the cycle counter reference, and forcefully zeroes it if it's non-zero: drivers/gpu/drm/panfrost/panfrost_gpu.c:panfrost_gpu_soft_reset() { ... if (drm_WARN_ON(&pfdev->base, atomic_read(&pfdev->cycle_counter.use_count) != 0)) atomic_set(&pfdev->cycle_counter.use_count, 0); ... } If a reset occurs while perfcnt is active, the use_count is zeroed. Later, when panfrost_perfcnt_disable_locked() is called, it will call panfrost_cycle_counter_put(). Because the count is already 0, the internal atomic_add_unless() will decrement it to -1: drivers/gpu/drm/panfrost/panfrost_gpu.c:panfrost_cycle_counter_put() { ... if (atomic_add_unless(&pfdev->cycle_counter.use_count, -1, 1)) return; ... } Once it reaches -1, future calls to panfrost_cycle_counter_get() will use atomic_inc_not_zero() to increment it from -1 to 0, returning early without starting the hardware cycle counter. Can this cause cycle counting to permanently stop working? > perfcnt->user = user; > > return 0; [ ... ] > @@ -207,6 +209,7 @@ static int panfrost_perfcnt_disable_locked(struct > panfrost_device *pfdev, > panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu); > panfrost_gem_mapping_put(perfcnt->mapping); > perfcnt->mapping = NULL; > + panfrost_cycle_counter_put(pfdev); [Severity: High] This isn't a bug introduced by this patch, but while reviewing the cycle counter refcounting, another issue was found in panfrost_job.c where the reference count can also underflow. If a job is submitted during a GPU reset, panfrost_job_hw_submit() enqueues the job but skips calling panfrost_cycle_counter_get() because the reset is pending: drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_job_hw_submit() { subslot = panfrost_enqueue_job(pfdev, js, job); /* Don't queue the job if a reset is in progress */ if (!atomic_read(&pfdev->reset.pending)) { job->is_profiled = pfdev->profile_mode; if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT || job->is_profiled) panfrost_cycle_counter_get(pfdev); ... } Shortly after, panfrost_reset() iterates over the scheduled jobs and unconditionally calls panfrost_cycle_counter_put() if the job requires it: drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_reset() { ... for (j = 0; j < ARRAY_SIZE(pfdev->jobs[0]) && pfdev->jobs[i][j]; j++) { if (pfdev->jobs[i][j]->requirements & PANFROST_JD_REQ_CYCLE_COUNT || pfdev->jobs[i][j]->is_profiled) panfrost_cycle_counter_put(pfdev->jobs[i][j]->pfdev); ... } Could this create a scenario where a put is called without a prior get, underflowing use_count to -1 in the same manner and permanently breaking the cycle counter? > pm_runtime_put_autosuspend(pfdev->base.dev); > > return 0; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260617-panfrost-perfcnt-timestamp-fix-v1-1-f215f44b5...@collabora.com?part=1
