Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/msm/a6xx: unsynchronized ifpc_counter_last access in hw_init() - [Medium] drm/msm/a6xx: lost IFPC counts on suspend and pre-hw_init() resume sample --
commit dc6d6671e40dbc56d3fc3f26cdac4aed610caf63 Author: Konrad Dybcio <[email protected]> drm/msm/a6xx: Expose GMU IFPC entry count in debugfs This commit adds a read-only ifpc_count debugfs file for Adreno GPUs with ADRENO_QUIRK_IFPC to report the total number of GMU IFPC entries. It initializes ifpc_counter_last from the GMU XOCLK_4 power counter during hw_init() and accumulates deltas into ifpc_count from a6xx_gpu_busy() and a8xx_gpu_busy(). > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 48d3ee4d55d9a..e7c3da5d3ac64 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -1338,6 +1338,11 @@ static int hw_init(struct msm_gpu *gpu) > GENMASK(7, 0), 0x4); > } > > + if (adreno_gpu->info->quirks & ADRENO_QUIRK_IFPC) { > + a6xx_gpu->ifpc_counter_last = > + gmu_read(&a6xx_gpu->gmu, > REG_A6XX_GMU_CX_GMU_POWER_COUNTER_XOCLK_4_L); > + } [Severity: Medium] Can this write to a6xx_gpu->ifpc_counter_last in hw_init() (and the matching write in a8xx_gpu.c:hw_init()) race with a6xx_ifpc_count_update()? During runtime resume (for example in msm_gpu_submit(), adreno_load_gpu(), or a6xx_recover()), pm_runtime_get_sync() runs adreno_runtime_resume() -> a6xx_gmu_pm_resume() before the caller invokes msm_gpu_hw_init() -> hw_init(): a6xx_gmu_pm_resume() a6xx_gmu_resume() msm_devfreq_resume() df->suspended = false; devfreq_resume_device() When msm_gpu_hw_init() subsequently runs, hw_init() holds gpu->lock and a6xx_gpu->gmu.lock, whereas msm_devfreq_get_dev_status() calls a6xx_gpu_busy() / a8xx_gpu_busy() -> a6xx_ifpc_count_update() under gpu->devfreq.lock without holding gpu->lock or gmu.lock. If devfreq polls while hw_init() is executing, could a6xx_ifpc_count_update() race with hw_init() updating a6xx_gpu->ifpc_counter_last and corrupt a6xx_gpu->ifpc_count? > + > /* Select CP0 to always count cycles */ > gpu_write(gpu, REG_A6XX_CP_PERFCTR_CP_SEL(0), PERF_CP_ALWAYS_COUNT); > > @@ -2334,6 +2339,17 @@ static void a6xx_destroy(struct msm_gpu *gpu) > kfree(a6xx_gpu); > } > > +void a6xx_ifpc_count_update(struct a6xx_gpu *a6xx_gpu, u32 cur) > +{ > + /* A counter below the last sample means the GMU restarted it */ > + if (cur >= a6xx_gpu->ifpc_counter_last) > + a6xx_gpu->ifpc_count += cur - a6xx_gpu->ifpc_counter_last; > + else > + a6xx_gpu->ifpc_count += cur; > + > + a6xx_gpu->ifpc_counter_last = cur; > +} [Severity: Medium] Does this accounting lose IFPC entries across GMU suspend/resume cycles and misorder the initial sample on resume? In a6xx_gmu_pm_suspend(), msm_devfreq_suspend() stops devfreq without calling gpu_busy(), and a6xx_gmu_stop() powers down the GMU CX domain without reading REG_A6XX_GMU_CX_GMU_POWER_COUNTER_XOCLK_4_L (or the A8xx equivalent). Any IFPC entries that occurred after the last periodic devfreq poll, such as the GPU entering IFPC after retiring its last submission before autosuspend fires, are discarded when the GMU power counter resets on shutdown. In addition, on resume, a6xx_gmu_pm_resume() calls msm_devfreq_resume() -> gpu->funcs->gpu_busy() -> a6xx_ifpc_count_update() before msm_gpu_hw_init() -> hw_init() has selected countable 0x4 on XOCLK_4 or initialized a6xx_gpu->ifpc_counter_last. When hw_init() then overwrites a6xx_gpu->ifpc_counter_last directly with gmu_read(), any unaccumulated delta is overwritten rather than folded into a6xx_gpu->ifpc_count. Should XOCLK_4_L be sampled before stopping the GMU in a6xx_gmu_pm_suspend(), and should counter initialization happen before msm_devfreq_resume() samples gpu_busy()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
