Re: [PATCH 4/8] drm/msm/a6xx: Add A6xx IFPC support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm/a6xx: missing IFPC CP_SET_MARKER packets in a6xx_submit()
- [Medium] drm/msm/a6xx: perfcntr SELECT regs lost on IFPC in a6xx_perfcntr_configure()
--

commit bfad38c351383c6a537cd26f28ef21f13067df68
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

drm/msm/a6xx: Add A6xx IFPC support

This commit adds A6xx-specific IFPC plumbing, including GMU WFI
configuration, power-up register list patching, CP_ME_INIT register list
programming, and skipping the A7xx HFI IFPC enable message on A6xx.

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index e7c3da5d3ac64..8ea9ab5b98861 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c

[ ... ]

> @@ -878,6 +887,43 @@ static void a7xx_patch_pwrup_reglist(struct msm_gpu *gpu)
>  	a6xx_gpu->dynamic_sel_reglist_offset = dyn_pwrup_reglist_count;
>  }
>  
> +static void a6xx_patch_pwrup_reglist(struct msm_gpu *gpu)
> +{
> +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> +	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> +	const struct adreno_reglist_list *reglist;
> +	void *ptr = a6xx_gpu->pwrup_reglist_ptr;
> +	struct cpu_gpu_lock *lock = ptr;
> +	u32 *dest = (u32 *)&lock->regs[0];
> +	int i;
> +
> +	lock->gpu_req = lock->cpu_req = lock->turn = 0;
> +	lock->list_length = 0;
> +
> +	/* Static IFPC-only registers */
> +	reglist = adreno_gpu->info->a6xx->ifpc_reglist;
> +	if (reglist) {
> +		for (i = 0; i < reglist->count; i++) {
> +			*dest++ = reglist->regs[i];
> +			*dest++ = gpu_read(gpu, reglist->regs[i]);
> +		}
> +		lock->list_length += reglist->count * 2;
> +	}
> +
> +	/* The CP sees IFPC-only and IFPC + preemption as one list from list_offset */
> +	lock->list_offset = lock->list_length;
> +
> +	/* Static IFPC + preemption registers */
> +	reglist = adreno_gpu->info->a6xx->pwrup_reglist;
> +	if (reglist) {
> +		for (i = 0; i < reglist->count; i++) {
> +			*dest++ = reglist->regs[i];
> +			*dest++ = gpu_read(gpu, reglist->regs[i]);
> +		}
> +		lock->list_length += reglist->count * 2;
> +	}
> +}

[Severity: Medium]
When a performance counter stream is configured via
DRM_IOCTL_MSM_PERFCNTR_CONFIG, msm_gpu_sysprof_no_ifpc() allows the GPU to
enter IFPC between samples under the assumption that a6xx_perfcntr_configure()
appends the configured SELECT registers to pwrup_reglist_ptr.

However, a6xx_perfcntr_configure() only populates pwrup_reglist_ptr when
info->family >= ADRENO_7XX_GEN1:

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a6xx_perfcntr_configure() {
	if (to_adreno_gpu(gpu)->info->family >= ADRENO_7XX_GEN1) {
		WARN_ON(!a6xx_gpu->pwrup_reglist_emitted);

		struct cpu_gpu_lock *lock = a6xx_gpu->pwrup_reglist_ptr;
		int off = (2 * lock->ifpc_list_len) +
			  (2 * lock->preemption_list_len) +
			  (3 * a6xx_gpu->dynamic_sel_reglist_offset);
...

On A6xx, a6xx_patch_pwrup_reglist() uses the A6xx cpu_gpu_lock layout
(lock->list_length and lock->list_offset with register/value pairs) without
recording the base static offset in dynamic_sel_reglist_offset, and
a6xx_perfcntr_configure() leaves reglist as NULL.

Will entering IFPC on A6xx while a performance counter stream is active lose
the GX-domain performance counter SELECT registers when the GX power domain
collapses?

[ ... ]

> @@ -1485,8 +1564,12 @@ static int hw_init(struct msm_gpu *gpu)
>  	/* Enable the SQE_to start the CP engine */
>  	gpu_write(gpu, REG_A6XX_CP_SQE_CNTL, 1);
>  
> -	if (adreno_is_a7xx(adreno_gpu) && !a6xx_gpu->pwrup_reglist_emitted) {
> -		a7xx_patch_pwrup_reglist(gpu);
> +	if (!a6xx_gpu->pwrup_reglist_emitted) {
> +		if (adreno_is_a7xx(adreno_gpu))
> +			a7xx_patch_pwrup_reglist(gpu);
> +		else
> +			a6xx_patch_pwrup_reglist(gpu);
> +
>  		a6xx_gpu->pwrup_reglist_emitted = true;
>  	}

[Severity: High]
In a7xx_submit(), the driver emits CP_SET_MARKER packets around command
submissions to disable IFPC before indirect buffers execute and re-enable it
after completion:

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a7xx_submit() {
	...
	OUT_PKT7(ring, CP_SET_MARKER, 1);
	OUT_RING(ring, 0x101); /* IFPC disable */
	...
	OUT_PKT7(ring, CP_SET_MARKER, 1);
	OUT_RING(ring, 0x100); /* IFPC enable */
}

However, a6xx_submit() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c does not emit
these CP_SET_MARKER packets before the indirect buffer loop or after the
CACHE_FLUSH_TS event write.

Should a6xx_submit() also emit CP_SET_MARKER(0x101) and CP_SET_MARKER(0x100)
so that REG_A6XX_CP_CP2GMU_STATUS properly tracks IFPC eligibility across
submissions on A6xx?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-topic-a6xx_ifpc-v1-0-6992841a9a32@oss.qualcomm.com?part=4