Re: [PATCH 1/3] drm/msm/a5xx: Reset the GPU when resuming it

[sashiko-bot@kernel.org]

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 <fabricioakio@gmail.com>

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/20261019-a5xx-gpu-pm-v1-0-1d0c313a1412@gmail.com?part=1