Re: [PATCH 10/12] drm/msm/a8xx: Switch to generated nonctx regs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/msm/a8xx: a8xx_nonctxt_config() clobbers LLCC SCID in GBIF_SCACHE_CNTL0
- [Medium] drm/msm/a8xx: nonctxt_reglist clobbers CX GBIF config in a8xx_nonctxt_config()
--

commit 966abf648e13ba76b4165851da9f6e5975e5c769
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/a8xx: Switch to generated nonctx regs

This commit removes the hand-maintained non-context register tables in
drivers/gpu/drm/msm/adreno/a8xx_catalog.c and switches the A8xx GPU catalog
entries to use the generated non-context register lists from the catalog
headers.

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

[ ... ]

> @@ -1556,7 +1216,7 @@ static const struct adreno_info a8xx_gpus[] = {
>  		.funcs = &a8xx_gpu_funcs,
>  		.a6xx = &(const struct a6xx_info) {
>  			.protect = &a830_protect,
> -			.nonctxt_reglist = a830_nonctxt_regs,
> +			.nonctxt_reglist = pakala_nonctxt_regs,
>  			.pwrup_reglist = &a830_pwrup_reglist,
>  			.dyn_pwrup_reglist = &a830_dyn_pwrup_reglist,
>  			.ifpc_reglist = &a830_ifpc_reglist,

[Severity: Medium]
Will switching .nonctxt_reglist in a8xx_gpus to pakala_nonctxt_regs (and the
other generated *_nonctxt_regs tables) cause a8xx_nonctxt_config() to clobber
the LLCC slice IDs in REG_A6XX_GBIF_SCACHE_CNTL0?

In drivers/gpu/drm/msm/adreno/catalog/adreno_pakala.h (as well as the
glymur_nonctxt_regs, kaanapali_nonctxt_regs, milos_nonctxt_regs,
maili_nonctxt_regs, and hawi_nonctxt_regs tables), the generated list
includes:

	{ 0x00003c01 /* GBIF_SCACHE_CNTL0                   */, 0x00000100, BIT(PIPE_NONE) },

On initial GPU load (adreno_load_gpu()), job submission after runtime suspend
(msm_gpu_submit()), or recovery (a8xx_recover()), pm_runtime_get_sync() runs
adreno_runtime_resume() -> a6xx_gmu_pm_resume() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c:

	gpu->needs_hw_init = true;
	...
	if (adreno_is_a8xx(adreno_gpu))
		a8xx_llc_activate(a6xx_gpu);

In drivers/gpu/drm/msm/adreno/a8xx_gpu.c, a8xx_llc_activate() programs the
system cache slice ID into REG_A6XX_GBIF_SCACHE_CNTL0 (0x3c01):

	gpu_write(gpu, REG_A6XX_GBIF_SCACHE_CNTL0,
		  FIELD_PREP(GENMASK(27, 22), gpu_scid) |
		  FIELD_PREP(GENMASK(21, 16), gpu_scid) |
		  FIELD_PREP(GENMASK(15, 10), gpu_scid) |
		  BIT(8));

Immediately afterwards, msm_gpu_hw_init() sees gpu->needs_hw_init == true and
calls a8xx_hw_init() -> hw_init() -> a8xx_nonctxt_config(), which executes:

	gpu_write(gpu, regs[i].offset, regs[i].value);

Because 0x00003c01 is in info->nonctxt_reglist with value 0x00000100,
wouldn't this overwrite REG_A6XX_GBIF_SCACHE_CNTL0 and zero out the gpu_scid
fields in bits [27:10] on every hardware init?

> @@ -1601,7 +1261,7 @@ static const struct adreno_info a8xx_gpus[] = {
>  		.funcs = &a8xx_gpu_funcs,
>  		.a6xx = &(const struct a6xx_info) {
>  			.protect = &x285_protect,
> -			.nonctxt_reglist = x285_nonctxt_regs,
> +			.nonctxt_reglist = glymur_nonctxt_regs,
>  			.pwrup_reglist = &a840_pwrup_reglist,
>  			.dyn_pwrup_reglist = &x285_dyn_pwrup_reglist,
>  			.ifpc_reglist = &a840_ifpc_reglist,
> @@ -1646,7 +1306,7 @@ static const struct adreno_info a8xx_gpus[] = {
>  		.funcs = &a8xx_gpu_funcs,
>  		.a6xx = &(const struct a6xx_info) {
>  			.protect = &a840_protect,
> -			.nonctxt_reglist = a840_nonctxt_regs,
> +			.nonctxt_reglist = kaanapali_nonctxt_regs,
>  			.pwrup_reglist = &a840_pwrup_reglist,
>  			.dyn_pwrup_reglist = &a840_dyn_pwrup_reglist,
>  			.ifpc_reglist = &a840_ifpc_reglist,
> @@ -1692,7 +1352,7 @@ static const struct adreno_info a8xx_gpus[] = {
>  		.zapfw = "gen80300_zap.mbn",
>  		.a6xx = &(const struct a6xx_info) {
>  			.protect = &a810_protect,
> -			.nonctxt_reglist = a810_nonctxt_regs,
> +			.nonctxt_reglist = milos_nonctxt_regs,
>  			.pwrup_reglist = &a810_pwrup_reglist,
>  			.dyn_pwrup_reglist = &a810_dyn_pwrup_reglist,
>  			.ifpc_reglist = &a810_ifpc_reglist,
> @@ -1735,7 +1395,7 @@ static const struct adreno_info a8xx_gpus[] = {
>  		.funcs = &a8xx_gpu_funcs,
>  		.a6xx = &(const struct a6xx_info) {
>  			.protect = &x285_protect,
> -			.nonctxt_reglist = x285_nonctxt_regs,
> +			.nonctxt_reglist = glymur_nonctxt_regs,
>  			.pwrup_reglist = &a840_pwrup_reglist,
>  			.dyn_pwrup_reglist = &x285_dyn_pwrup_reglist,
>  			.ifpc_reglist = &a840_ifpc_reglist,
> @@ -1778,7 +1438,7 @@ static const struct adreno_info a8xx_gpus[] = {
>  		.funcs = &a8xx_gpu_funcs,
>  		.a6xx = &(const struct a6xx_info) {
>  			.protect = &a850_protect,
> -			.nonctxt_reglist = a845_nonctxt_regs,
> +			.nonctxt_reglist = maili_nonctxt_regs,
>  			.pwrup_reglist = &a850_pwrup_reglist,
>  			.dyn_pwrup_reglist = &a850_dyn_pwrup_reglist,
>  			.ifpc_reglist = &a850_ifpc_reglist,
> @@ -1822,7 +1482,7 @@ static const struct adreno_info a8xx_gpus[] = {
>  		.funcs = &a8xx_gpu_funcs,
>  		.a6xx = &(const struct a6xx_info) {
>  			.protect = &a850_protect,
> -			.nonctxt_reglist = a850_nonctxt_regs,
> +			.nonctxt_reglist = hawi_nonctxt_regs,
>  			.pwrup_reglist = &a850_pwrup_reglist,
>  			.dyn_pwrup_reglist = &a850_dyn_pwrup_reglist,
>  			.ifpc_reglist = &a850_ifpc_reglist,

[Severity: Medium]
Should the CX GBIF registers (0x3c00..0x3c45) in the generated *_nonctxt_regs
tables also be excluded or handled separately in a8xx_nonctxt_config()?

Unlike the previous hand-maintained nonctxt_regs arrays in
drivers/gpu/drm/msm/adreno/a8xx_catalog.c, all six generated *_nonctxt_regs
tables include CX GBIF entries:

	{ 0x00003c00 /* GBIF_CX_CONFIG                      */, 0x20023020, BIT(PIPE_NONE) },
	{ 0x00003c01 /* GBIF_SCACHE_CNTL0                   */, 0x00000100, BIT(PIPE_NONE) },
	{ 0x00003c03 /* GBIF_QSB_SIDE0                      */, 0x00071e20, BIT(PIPE_NONE) },
	{ 0x00003c04 /* GBIF_QSB_SIDE1                      */, 0x00071e20, BIT(PIPE_NONE) },
	{ 0x00003c05 /* GBIF_QSB_SIDE2                      */, 0x00071e20, BIT(PIPE_NONE) },
	{ 0x00003c06 /* GBIF_QSB_SIDE3                      */, 0x00071e20, BIT(PIPE_NONE) },
	{ 0x00003c45 /* GBIF_HALT                           */, 0x00000008, BIT(PIPE_NONE) },

During resume, a6xx_gmu_pm_resume() -> a6xx_gmu_resume() ->
a6xx_gmu_fw_start() configures gbif_cx and boots the GMU (which enables clock
gating in REG_A6XX_GBIF_CX_CONFIG BIT(0), as noted in a8xx_set_hwcg() in
drivers/gpu/drm/msm/adreno/a8xx_gpu.c):

	/*
	 * GMU enables clk gating in GBIF during boot up. So,
	 * override that here when hwcg feature is disabled
	 */
	gpu_rmw(gpu, REG_A6XX_GBIF_CX_CONFIG, BIT(0), 0);

Then in drivers/gpu/drm/msm/adreno/a8xx_gpu.c, hw_init() explicitly clears
REG_A6XX_GBIF_HALT before calling a8xx_nonctxt_config():

	/* Clear GBIF halt in case GX domain was not collapsed */
	gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
	gpu_read(gpu, REG_A6XX_GBIF_HALT);
	...
	a8xx_nonctxt_config(gpu);

When a8xx_nonctxt_config() subsequently writes the generated nonctxt_reglist,
could writing 0x20023020 to REG_A6XX_GBIF_CX_CONFIG (0x3c00) clear BIT(0)
that the GMU enabled on boot, and could writing 0x00000008 to
REG_A6XX_GBIF_HALT (0x3c45) undo the earlier clear in hw_init() in addition
to overwriting REG_A6XX_GBIF_SCACHE_CNTL0 (0x3c01)?

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