Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/msm/a5xx: soft reset in a5xx_pm_resume() underflows devfreq busy_cycles --
commit 19124e467b5360709bf4e69d8b941ff7d1098156 Author: Fabricio Akio <[email protected]> drm/msm/a5xx: Reset the GPU when resuming it This patch adds a soft reset via REG_A5XX_RBBM_SW_RESET_CMD in a5xx_pm_resume() so that the GPU always resumes from a reset state even when the GX power domain remained on during suspend. > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > index 16c7335c4b1f8..dd0d731df138c 100644 > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > @@ -1367,6 +1367,18 @@ static int a5xx_pm_resume(struct msm_gpu *gpu) > if (ret) > return ret; > > + /* > + * The GX power domain is not necessarily switched off while we are > + * suspended: on SDM630, SDM660 and MSM8998 it follows the GPU's SMMU > + * rather than the GPU. If it stayed on, the CP still holds its old > + * ringbuffer read pointer and would execute stale ringbuffer contents > + * once hw_init restarts it. Soft reset the GPU so that it always > + * resumes from its reset state. > + */ > + gpu_write(gpu, REG_A5XX_RBBM_SW_RESET_CMD, 1); > + gpu_read(gpu, REG_A5XX_RBBM_SW_RESET_CMD); > + gpu_write(gpu, REG_A5XX_RBBM_SW_RESET_CMD, 0); [Severity: Medium] Can pulsing REG_A5XX_RBBM_SW_RESET_CMD after msm_gpu_pm_resume() underflow df->busy_cycles in msm_devfreq_get_dev_status() when the GX power domain remains powered across suspend? Earlier in a5xx_pm_resume(), msm_gpu_pm_resume() runs and calls msm_devfreq_resume() -> a5xx_gpu_busy(): drivers/gpu/drm/msm/msm_gpu_devfreq.c:msm_devfreq_resume() { ... mutex_lock(&df->lock); df->busy_cycles = gpu->funcs->gpu_busy(gpu, &sample_rate); df->time = ktime_get(); df->suspended = false; mutex_unlock(&df->lock); devfreq_resume_device(df->devfreq); } drivers/gpu/drm/msm/adreno/a5xx_gpu.c:a5xx_gpu_busy() { ... busy_cycles = gpu_read64(gpu, REG_A5XX_RBBM_PERFCTR_RBBM_0_LO); *out_sample_rate = clk_get_rate(gpu->core_clk); return busy_cycles; } When GX stays on during suspend (such as on SDM630, SDM660, and MSM8998), REG_A5XX_RBBM_PERFCTR_RBBM_0_LO retains its non-zero pre-suspend counter value when msm_devfreq_resume() samples it into df->busy_cycles. Immediately after msm_gpu_pm_resume() returns, a5xx_pm_resume() writes REG_A5XX_RBBM_SW_RESET_CMD, which resets the RBBM registers including REG_A5XX_RBBM_PERFCTR_RBBM_0_LO back to 0 while devfreq polling is already active. On the next devfreq poll in msm_devfreq_get_dev_status(): drivers/gpu/drm/msm/msm_gpu_devfreq.c:msm_devfreq_get_dev_status() { ... busy_cycles = gpu->funcs->gpu_busy(gpu, &sample_rate); busy_time = busy_cycles - df->busy_cycles; df->busy_cycles = busy_cycles; mutex_unlock(&df->lock); busy_time *= USEC_PER_SEC; busy_time = div64_ul(busy_time, sample_rate); if (WARN_ON(busy_time > ~0LU)) busy_time = ~0LU; ... } Wouldn't busy_cycles - df->busy_cycles underflow the 64-bit unsigned value, causing a bogus maximum busy_time spike (and triggering the WARN_ON on 32-bit builds)? > + > /* Adreno 505, 506, 508, 509, 510, 512 needs manual RBBM sus/res > control */ > if (!(adreno_is_a530(adreno_gpu) || adreno_is_a540(adreno_gpu))) { > /* Halt the sp_input_clk at HM level */ [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
