Re: [PATCH 3/8] drm/msm/a6xx: Expose GMU IFPC entry count in debugfs

[sashiko-bot@kernel.org]

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 <konrad.dybcio@oss.qualcomm.com>

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/20261030-topic-a6xx_ifpc-v1-0-6992841a9a32@oss.qualcomm.com?part=3