Re: [PATCH 11/12] drm/msm/a8xx: Switch to generated protect regs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm/a8xx: wrong protect table pakala_protect used for Kaanapali
- [High] drm/msm/a8xx: missing CP_PROTECT_GLOBAL(56..62) in a850_ifpc_reglist_regs
--

commit 552d3a60893e50e06127da749c3d585e911f2b2e
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/a8xx: Switch to generated protect regs

This commit replaces the hardcoded protect register tables in
a8xx_catalog.c with the generated protect tables from the per-target
catalog headers.

> diff --git a/drivers/gpu/drm/msm/adreno/a8xx_catalog.c b/drivers/gpu/drm/msm/adreno/a8xx_catalog.c
> index bc79013165c13..5f5f71d86df93 100644
> --- a/drivers/gpu/drm/msm/adreno/a8xx_catalog.c
> +++ b/drivers/gpu/drm/msm/adreno/a8xx_catalog.c

[ ... ]

> @@ -1305,7 +1026,7 @@ static const struct adreno_info a8xx_gpus[] = {
>  			  ADRENO_QUIRK_IFPC,
>  		.funcs = &a8xx_gpu_funcs,
>  		.a6xx = &(const struct a6xx_info) {
> -			.protect = &a840_protect,
> +			.protect = &pakala_protect,
>  			.nonctxt_reglist = kaanapali_nonctxt_regs,
>  			.pwrup_reglist = &a840_pwrup_reglist,
>  			.dyn_pwrup_reglist = &a840_dyn_pwrup_reglist,

[Severity: High]
Should the Kaanapali (Adreno 840, 0x44050a01) entry in a8xx_gpus[] use
&kaanapali_protect instead of &pakala_protect?

__build_asserts() checks kaanapali_protect, which is defined in
drivers/gpu/drm/msm/adreno/catalog/adreno_kaanapali.h, but kaanapali_protect
is left unused in a8xx_gpus[].

When hw_init() -> a8xx_set_cp_protect() programs the protection registers
from pakala_protect_regs on Kaanapali, several Kaanapali-specific spans from
kaanapali_protect_regs (including 0x00775..0x0077f, 0x00c07..0x00c0f,
0x03d00..0x05cff, 0x0a82e, 0x0aed0..0x0b17f, and 0x1fd80..0x1ffff) are left
unprotected against unprivileged userspace command streams, while
0x00000..0x00007 and 0x27c02..0x27c05 are protected instead.

[ ... ]

> @@ -1481,7 +1202,7 @@ static const struct adreno_info a8xx_gpus[] = {
>  			  ADRENO_QUIRK_IFPC,
>  		.funcs = &a8xx_gpu_funcs,
>  		.a6xx = &(const struct a6xx_info) {
> -			.protect = &a850_protect,
> +			.protect = &hawi_protect,
>  			.nonctxt_reglist = hawi_nonctxt_regs,
>  			.pwrup_reglist = &a850_pwrup_reglist,
>  			.dyn_pwrup_reglist = &a850_dyn_pwrup_reglist,

[Severity: High]
Does switching Hawi (Adreno 850, 0x44051401) to hawi_protect without
updating a850_ifpc_reglist_regs cause REG_A8XX_CP_PROTECT_GLOBAL(56) through
REG_A8XX_CP_PROTECT_GLOBAL(62) to be lost across inter-frame power collapse?

Previously, a850_protect_regs had 57 entries, and a850_ifpc_reglist_regs
omitted REG_A8XX_CP_PROTECT_GLOBAL(56) through
REG_A8XX_CP_PROTECT_GLOBAL(62):

drivers/gpu/drm/msm/adreno/a8xx_catalog.c:
static const u32 a850_ifpc_reglist_regs[] = {
	...
	REG_A8XX_CP_PROTECT_GLOBAL(54),
	REG_A8XX_CP_PROTECT_GLOBAL(55),
	REG_A8XX_CP_PROTECT_GLOBAL(63),
};

In drivers/gpu/drm/msm/adreno/catalog/adreno_hawi.h, hawi_protect_regs has
64 entries, so hw_init() -> a8xx_set_cp_protect() populates all 64
REG_A8XX_CP_PROTECT_GLOBAL(0..63) registers:

drivers/gpu/drm/msm/adreno/catalog/adreno_hawi.h:
static const u32 hawi_protect_regs[] __maybe_unused = {
	...
	A6XX_PROTECT_NORDWR(0x1ac00, 0x1fff),
	A6XX_PROTECT_NORDWR(0x1f400, 0x0477),
	A6XX_PROTECT_NORDWR(0x1f930, 0x032f),
	A6XX_PROTECT_NORDWR(0x1fd80, 0x1fff),
	A6XX_PROTECT_NORDWR(0x27800, 0x007f),
	A6XX_PROTECT_RDONLY(0x27880, 0x0001),
	A6XX_PROTECT_NORDWR(0x27882, 0x037d),
	A6XX_PROTECT_NORDWR(0x27c06, 0x0000),
};
DECLARE_ADRENO_PROTECT(hawi_protect, 64);

Because Hawi sets ADRENO_QUIRK_IFPC and uses a850_ifpc_reglist in
a8xx_patch_pwrup_reglist(), those 7 protection registers (covering ranges in
0x1ac00..0x27bff) will not be restored after inter-frame power collapse.

> @@ -1516,6 +1237,10 @@ DECLARE_ADRENO_GPULIST(a8xx);
>  
>  static inline __always_unused void __build_asserts(void)
>  {
> -	BUILD_BUG_ON(a810_protect.count > a810_protect.count_max);
> -	BUILD_BUG_ON(a830_protect.count > a830_protect.count_max);
> +	BUILD_BUG_ON(glymur_protect.count > glymur_protect.count_max);
> +	BUILD_BUG_ON(hawi_protect.count > hawi_protect.count_max);
> +	BUILD_BUG_ON(kaanapali_protect.count > kaanapali_protect.count_max);
> +	BUILD_BUG_ON(maili_protect.count > maili_protect.count_max);
> +	BUILD_BUG_ON(milos_protect.count > milos_protect.count_max);
> +	BUILD_BUG_ON(pakala_protect.count > pakala_protect.count_max);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020204728.28844-1-robin.clark@oss.qualcomm.com?part=11