Re: [PATCH RFT 8/8] drm/msm/a6xx: Wire up IFPC on A615-family GPUs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm/a6xx: missing revn 618 on sm7150 and revn 615 in adreno_is_a615_family()
- [Medium] drm/msm/a6xx: QoS register mismatch between a640_pwrup_reglist and hw_init()
--

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

drm/msm/a6xx: Wire up IFPC on A615-family GPUs

This commit enables ADRENO_QUIRK_IFPC and wires up a630_ifpc_reglist and
a640_pwrup_reglist for the A615-family entries in a6xx_gpus[].

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> index 2d59f7735c907..257771d4eb4d5 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> @@ -1012,7 +1012,8 @@ static const struct adreno_info a6xx_gpus[] = {
>  			[ADRENO_FW_GMU] = "a630_gmu.bin",
>  		},
>  		.gmem = SZ_512K,
> -		.quirks = ADRENO_QUIRK_4GB_VA,
> +		.quirks = ADRENO_QUIRK_4GB_VA |
> +			  ADRENO_QUIRK_IFPC,
>  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>  		.funcs = &a6xx_gpu_funcs,
>  		.zapfw = "a615_zap.mdt",
> @@ -1021,6 +1022,8 @@ static const struct adreno_info a6xx_gpus[] = {
>  			.protect = &a630_protect,
>  			.gmu_cgc_mode = 0x00000222,
>  			.prim_fifo_threshold = 0x0018000,
> +			.ifpc_reglist = &a630_ifpc_reglist,
> +			.pwrup_reglist = &a640_pwrup_reglist,

[Severity: Medium]
Should the A615-family entries in a6xx_gpus[] either use a630_pwrup_reglist
or have hw_init() updated to program REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL for
this family?

In drivers/gpu/drm/msm/adreno/a6xx_gpu.c:hw_init(), all six A615-family
entries have .family = ADRENO_6XX_GEN1 and fall into the final else branch,
which writes 0x3 to REG_A6XX_RBBM_VBIF_CLIENT_QOS_CNTL and leaves
REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL unprogrammed:

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:hw_init() {
	...
	if (adreno_is_a610_family(adreno_gpu) ||
	    adreno_is_a640_family(adreno_gpu) ||
	    adreno_is_a650_family(adreno_gpu)) {
		gpu_write(gpu, REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL, 0x3);
	} else if (adreno_is_a7xx(adreno_gpu)) {
		gpu_write(gpu, REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL, 0x2120212);
	} else {
		gpu_write(gpu, REG_A6XX_RBBM_VBIF_CLIENT_QOS_CNTL, 0x3);
	}
	...
}

Later in hw_init(), a6xx_patch_pwrup_reglist() snapshots the current values
of the registers listed in info->a6xx->pwrup_reglist via gpu_read() so the CP
can restore them after each IFPC GX power collapse:

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a6xx_patch_pwrup_reglist() {
	...
	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;
	}
	...
}

Because a640_pwrup_reglist_regs lists REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL
rather than REG_A6XX_RBBM_VBIF_CLIENT_QOS_CNTL (which is the only difference
between a640_pwrup_reglist and a630_pwrup_reglist), won't every IFPC cycle
lose the programmed REG_A6XX_RBBM_VBIF_CLIENT_QOS_CNTL value and replay the
uninitialized REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL register instead?

>  		},
>  		.speedbins = ADRENO_SPEEDBINS(
>  			/*
> @@ -1044,7 +1047,8 @@ static const struct adreno_info a6xx_gpus[] = {
>  		.gmem = SZ_512K,
>  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>  		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
> -			  ADRENO_QUIRK_4GB_VA,
> +			  ADRENO_QUIRK_4GB_VA |
> +			  ADRENO_QUIRK_IFPC,

[Severity: High]
Does enabling ADRENO_QUIRK_IFPC on the qcom,sm7150 (0x06010800) and A615
(0x06010500) entries cause them to run IFPC with A630 PDC and GMU clock
gating settings instead of A618/A615-family settings?

The qcom,sm7150 entry in drivers/gpu/drm/msm/adreno/a6xx_catalog.c omits
.revn = 618, unlike the generic 0x06010800 entry right below it. In addition,
adreno_is_a615_family() in drivers/gpu/drm/msm/adreno/adreno_gpu.h does not
check for revn 615:

drivers/gpu/drm/msm/adreno/adreno_gpu.h:
/* TODO: 615/616 */
static inline int adreno_is_a615_family(const struct adreno_gpu *gpu)
{
	return adreno_is_a618(gpu) ||
	       adreno_is_a619(gpu);
}

Because adreno_is_a618() checks adreno_is_revn(gpu, 618), both
adreno_is_a618() and adreno_is_a615_family() evaluate to false on
qcom,sm7150. In drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_rpmh_init(),
this causes the driver to program A630 PDC sleep/wakeup values instead of
A618 values:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_rpmh_init() {
	...
	else if (adreno_is_a618(adreno_gpu) || adreno_is_a640_family(adreno_gpu))
		pdc_address_offset = 0x30090;
	else if (adreno_is_a619(adreno_gpu))
		pdc_address_offset = 0x300a0;
	else
		pdc_address_offset = 0x30080;
	...
	if (adreno_is_a618(adreno_gpu) || adreno_is_a619(adreno_gpu) ||
			adreno_is_a650_family(adreno_gpu))
		pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_DATA + 4, 0x2);
	else
		pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_DATA + 4, 0x3);
	...
}

With ADRENO_QUIRK_IFPC setting gmu->idle_level = GMU_IDLE_STATE_IFPC, the GMU
will execute those PDC sequences on every IFPC power collapse. Missing revn
618 on qcom,sm7150 and revn 615 in adreno_is_a615_family() also causes
a6xx_set_hwcg() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c to program A630 GMU
CGC delay and hysteresis values (0x10111 / 0x5555) instead of A615-family
values (0x111 / 0x555), and causes a6xx_hfi_send_bw_table() to skip
a618_build_bw_table() on qcom,sm7150.

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