mbox series

[v2,0/3] drm/msm: Fixes for devcoredump on a750

Message ID 20240807-a750-devcoredump-fixes-v2-0-d7483736d26d@gmail.com
Headers show
Series drm/msm: Fixes for devcoredump on a750 | expand

Message

Connor Abbott Aug. 7, 2024, 12:34 p.m. UTC
Unfortunately the first time around I made the mistake of not checking
the devcoredump file closely enough to make sure it had the right
contents. This makes sure we're actually using the a750 register lists
on a750.

Signed-off-by: Connor Abbott <cwabbott0@gmail.com>
---
Changes in v2:
- Add last commit fixing an indexed register name
- Link to v1: https://lore.kernel.org/r/20240509-a750-devcoredump-fixes-v1-0-60cc99e2147d@gmail.com

---
Connor Abbott (3):
      drm/msm: Use a7xx family directly in gpu_state
      drm/msm: Dump correct dbgahb clusters on a750
      drm/msm: Fix CP_BV_DRAW_STATE_ADDR name

 drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c        | 46 +++++++++++-----------
 .../gpu/drm/msm/adreno/adreno_gen7_9_0_snapshot.h  |  2 +-
 2 files changed, 25 insertions(+), 23 deletions(-)
---
base-commit: 5acf1f91d74433cbfffd9df962b6e45f5d3ef253
change-id: 20240509-a750-devcoredump-fixes-e59bb5563b1a

Best regards,

Comments

Akhil P Oommen Aug. 12, 2024, 6:08 a.m. UTC | #1
On Wed, Aug 07, 2024 at 01:34:27PM +0100, Connor Abbott wrote:
> With a7xx, we need to import a new header for each new generation and
> switch to a different list of registers, instead of making
> backwards-compatible changes. Using the helpers inadvertently made a750
> use the a740 list of registers, instead use the family directly to fix
> this.

This won't scale. What about other gpus in the same generation but has a
different register list? You don't see that issue currently because
there are no support for lower tier a7x GPUs yet.

I think we should move to a "snapshot block list" like in the downstream
driver if you want to simplify the whole logic. Otherwise, we should
leave the chipid check as it is and just fix up a750 configurations.

-Akhil

> 
> Fixes: f3f8207d8aed ("drm/msm: Add devcoredump support for a750")
> Signed-off-by: Connor Abbott <cwabbott0@gmail.com>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 41 ++++++++++++++---------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> index 77146d30bcaa..c641ee7dec78 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> @@ -390,18 +390,18 @@ static void a7xx_get_debugbus_blocks(struct msm_gpu *gpu,
>  	const u32 *debugbus_blocks, *gbif_debugbus_blocks;
>  	int i;
>  
> -	if (adreno_is_a730(adreno_gpu)) {
> +	if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
>  		debugbus_blocks = gen7_0_0_debugbus_blocks;
>  		debugbus_blocks_count = ARRAY_SIZE(gen7_0_0_debugbus_blocks);
>  		gbif_debugbus_blocks = a7xx_gbif_debugbus_blocks;
>  		gbif_debugbus_blocks_count = ARRAY_SIZE(a7xx_gbif_debugbus_blocks);
> -	} else if (adreno_is_a740_family(adreno_gpu)) {
> +	} else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
>  		debugbus_blocks = gen7_2_0_debugbus_blocks;
>  		debugbus_blocks_count = ARRAY_SIZE(gen7_2_0_debugbus_blocks);
>  		gbif_debugbus_blocks = a7xx_gbif_debugbus_blocks;
>  		gbif_debugbus_blocks_count = ARRAY_SIZE(a7xx_gbif_debugbus_blocks);
>  	} else {
> -		BUG_ON(!adreno_is_a750(adreno_gpu));
> +		BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
>  		debugbus_blocks = gen7_9_0_debugbus_blocks;
>  		debugbus_blocks_count = ARRAY_SIZE(gen7_9_0_debugbus_blocks);
>  		gbif_debugbus_blocks = gen7_9_0_gbif_debugbus_blocks;
> @@ -511,7 +511,7 @@ static void a6xx_get_debugbus(struct msm_gpu *gpu,
>  		const struct a6xx_debugbus_block *cx_debugbus_blocks;
>  
>  		if (adreno_is_a7xx(adreno_gpu)) {
> -			BUG_ON(!(adreno_is_a730(adreno_gpu) || adreno_is_a740_family(adreno_gpu)));
> +			BUG_ON(adreno_gpu->info->family > ADRENO_7XX_GEN3);
>  			cx_debugbus_blocks = a7xx_cx_debugbus_blocks;
>  			nr_cx_debugbus_blocks = ARRAY_SIZE(a7xx_cx_debugbus_blocks);
>  		} else {
> @@ -662,11 +662,11 @@ static void a7xx_get_dbgahb_clusters(struct msm_gpu *gpu,
>  	const struct gen7_sptp_cluster_registers *dbgahb_clusters;
>  	unsigned dbgahb_clusters_size;
>  
> -	if (adreno_is_a730(adreno_gpu)) {
> +	if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
>  		dbgahb_clusters = gen7_0_0_sptp_clusters;
>  		dbgahb_clusters_size = ARRAY_SIZE(gen7_0_0_sptp_clusters);
>  	} else {
> -		BUG_ON(!adreno_is_a740_family(adreno_gpu));
> +		BUG_ON(adreno_gpu->info->family > ADRENO_7XX_GEN3);
>  		dbgahb_clusters = gen7_2_0_sptp_clusters;
>  		dbgahb_clusters_size = ARRAY_SIZE(gen7_2_0_sptp_clusters);
>  	}
> @@ -820,14 +820,14 @@ static void a7xx_get_clusters(struct msm_gpu *gpu,
>  	const struct gen7_cluster_registers *clusters;
>  	unsigned clusters_size;
>  
> -	if (adreno_is_a730(adreno_gpu)) {
> +	if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
>  		clusters = gen7_0_0_clusters;
>  		clusters_size = ARRAY_SIZE(gen7_0_0_clusters);
> -	} else if (adreno_is_a740_family(adreno_gpu)) {
> +	} else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
>  		clusters = gen7_2_0_clusters;
>  		clusters_size = ARRAY_SIZE(gen7_2_0_clusters);
>  	} else {
> -		BUG_ON(!adreno_is_a750(adreno_gpu));
> +		BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
>  		clusters = gen7_9_0_clusters;
>  		clusters_size = ARRAY_SIZE(gen7_9_0_clusters);
>  	}
> @@ -895,7 +895,7 @@ static void a7xx_get_shader_block(struct msm_gpu *gpu,
>  	if (WARN_ON(datasize > A6XX_CD_DATA_SIZE))
>  		return;
>  
> -	if (adreno_is_a730(adreno_gpu)) {
> +	if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
>  		gpu_rmw(gpu, REG_A7XX_SP_DBG_CNTL, GENMASK(1, 0), 3);
>  	}
>  
> @@ -925,7 +925,7 @@ static void a7xx_get_shader_block(struct msm_gpu *gpu,
>  		datasize);
>  
>  out:
> -	if (adreno_is_a730(adreno_gpu)) {
> +	if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
>  		gpu_rmw(gpu, REG_A7XX_SP_DBG_CNTL, GENMASK(1, 0), 0);
>  	}
>  }
> @@ -958,14 +958,14 @@ static void a7xx_get_shaders(struct msm_gpu *gpu,
>  	unsigned num_shader_blocks;
>  	int i;
>  
> -	if (adreno_is_a730(adreno_gpu)) {
> +	if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
>  		shader_blocks = gen7_0_0_shader_blocks;
>  		num_shader_blocks = ARRAY_SIZE(gen7_0_0_shader_blocks);
> -	} else if (adreno_is_a740_family(adreno_gpu)) {
> +	} else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
>  		shader_blocks = gen7_2_0_shader_blocks;
>  		num_shader_blocks = ARRAY_SIZE(gen7_2_0_shader_blocks);
>  	} else {
> -		BUG_ON(!adreno_is_a750(adreno_gpu));
> +		BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
>  		shader_blocks = gen7_9_0_shader_blocks;
>  		num_shader_blocks = ARRAY_SIZE(gen7_9_0_shader_blocks);
>  	}
> @@ -1350,14 +1350,14 @@ static void a7xx_get_registers(struct msm_gpu *gpu,
>  	const u32 *pre_crashdumper_regs;
>  	const struct gen7_reg_list *reglist;
>  
> -	if (adreno_is_a730(adreno_gpu)) {
> +	if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
>  		reglist = gen7_0_0_reg_list;
>  		pre_crashdumper_regs = gen7_0_0_pre_crashdumper_gpu_registers;
> -	} else if (adreno_is_a740_family(adreno_gpu)) {
> +	} else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
>  		reglist = gen7_2_0_reg_list;
>  		pre_crashdumper_regs = gen7_0_0_pre_crashdumper_gpu_registers;
>  	} else {
> -		BUG_ON(!adreno_is_a750(adreno_gpu));
> +		BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
>  		reglist = gen7_9_0_reg_list;
>  		pre_crashdumper_regs = gen7_9_0_pre_crashdumper_gpu_registers;
>  	}
> @@ -1407,8 +1407,7 @@ static void a7xx_get_post_crashdumper_registers(struct msm_gpu *gpu,
>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>  	const u32 *regs;
>  
> -	BUG_ON(!(adreno_is_a730(adreno_gpu) || adreno_is_a740_family(adreno_gpu) ||
> -		 adreno_is_a750(adreno_gpu)));
> +	BUG_ON(adreno_gpu->info->family > ADRENO_7XX_GEN3);
>  	regs = gen7_0_0_post_crashdumper_registers;
>  
>  	a7xx_get_ahb_gpu_registers(gpu,
> @@ -1514,11 +1513,11 @@ static void a7xx_get_indexed_registers(struct msm_gpu *gpu,
>  	const struct a6xx_indexed_registers *indexed_regs;
>  	int i, indexed_count, mempool_count;
>  
> -	if (adreno_is_a730(adreno_gpu) || adreno_is_a740_family(adreno_gpu)) {
> +	if (adreno_gpu->info->family <= ADRENO_7XX_GEN2) {
>  		indexed_regs = a7xx_indexed_reglist;
>  		indexed_count = ARRAY_SIZE(a7xx_indexed_reglist);
>  	} else {
> -		BUG_ON(!adreno_is_a750(adreno_gpu));
> +		BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
>  		indexed_regs = gen7_9_0_cp_indexed_reg_list;
>  		indexed_count = ARRAY_SIZE(gen7_9_0_cp_indexed_reg_list);
>  	}
> 
> -- 
> 2.31.1
>
Akhil P Oommen Aug. 12, 2024, 6:22 a.m. UTC | #2
On Wed, Aug 07, 2024 at 01:34:29PM +0100, Connor Abbott wrote:
> This was missed because we weren't using the a750-specific indexed regs.
> 
> Fixes: f3f8207d8aed ("drm/msm: Add devcoredump support for a750")
> Signed-off-by: Connor Abbott <cwabbott0@gmail.com>

Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>

-Akhil

> ---
>  drivers/gpu/drm/msm/adreno/adreno_gen7_9_0_snapshot.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gen7_9_0_snapshot.h b/drivers/gpu/drm/msm/adreno/adreno_gen7_9_0_snapshot.h
> index 260d66eccfec..9a327d543f27 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gen7_9_0_snapshot.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gen7_9_0_snapshot.h
> @@ -1303,7 +1303,7 @@ static struct a6xx_indexed_registers gen7_9_0_cp_indexed_reg_list[] = {
>  		REG_A6XX_CP_ROQ_DBG_DATA, 0x00800},
>  	{ "CP_UCODE_DBG_DATA", REG_A6XX_CP_SQE_UCODE_DBG_ADDR,
>  		REG_A6XX_CP_SQE_UCODE_DBG_DATA, 0x08000},
> -	{ "CP_BV_SQE_STAT_ADDR", REG_A7XX_CP_BV_DRAW_STATE_ADDR,
> +	{ "CP_BV_DRAW_STATE_ADDR", REG_A7XX_CP_BV_DRAW_STATE_ADDR,
>  		REG_A7XX_CP_BV_DRAW_STATE_DATA, 0x00200},
>  	{ "CP_BV_ROQ_DBG_ADDR", REG_A7XX_CP_BV_ROQ_DBG_ADDR,
>  		REG_A7XX_CP_BV_ROQ_DBG_DATA, 0x00800},
> 
> -- 
> 2.31.1
> 
>
Rob Clark Aug. 12, 2024, 4:21 p.m. UTC | #3
On Sun, Aug 11, 2024 at 11:09 PM Akhil P Oommen
<quic_akhilpo@quicinc.com> wrote:
>
> On Wed, Aug 07, 2024 at 01:34:27PM +0100, Connor Abbott wrote:
> > With a7xx, we need to import a new header for each new generation and
> > switch to a different list of registers, instead of making
> > backwards-compatible changes. Using the helpers inadvertently made a750
> > use the a740 list of registers, instead use the family directly to fix
> > this.
>
> This won't scale. What about other gpus in the same generation but has a
> different register list? You don't see that issue currently because
> there are no support for lower tier a7x GPUs yet.
>
> I think we should move to a "snapshot block list" like in the downstream
> driver if you want to simplify the whole logic. Otherwise, we should
> leave the chipid check as it is and just fix up a750 configurations.

Or maybe just move some of this to the device catalogue?

BR,
-R

> -Akhil
>
> >
> > Fixes: f3f8207d8aed ("drm/msm: Add devcoredump support for a750")
> > Signed-off-by: Connor Abbott <cwabbott0@gmail.com>
> > ---
> >  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 41 ++++++++++++++---------------
> >  1 file changed, 20 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > index 77146d30bcaa..c641ee7dec78 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > @@ -390,18 +390,18 @@ static void a7xx_get_debugbus_blocks(struct msm_gpu *gpu,
> >       const u32 *debugbus_blocks, *gbif_debugbus_blocks;
> >       int i;
> >
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               debugbus_blocks = gen7_0_0_debugbus_blocks;
> >               debugbus_blocks_count = ARRAY_SIZE(gen7_0_0_debugbus_blocks);
> >               gbif_debugbus_blocks = a7xx_gbif_debugbus_blocks;
> >               gbif_debugbus_blocks_count = ARRAY_SIZE(a7xx_gbif_debugbus_blocks);
> > -     } else if (adreno_is_a740_family(adreno_gpu)) {
> > +     } else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
> >               debugbus_blocks = gen7_2_0_debugbus_blocks;
> >               debugbus_blocks_count = ARRAY_SIZE(gen7_2_0_debugbus_blocks);
> >               gbif_debugbus_blocks = a7xx_gbif_debugbus_blocks;
> >               gbif_debugbus_blocks_count = ARRAY_SIZE(a7xx_gbif_debugbus_blocks);
> >       } else {
> > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> >               debugbus_blocks = gen7_9_0_debugbus_blocks;
> >               debugbus_blocks_count = ARRAY_SIZE(gen7_9_0_debugbus_blocks);
> >               gbif_debugbus_blocks = gen7_9_0_gbif_debugbus_blocks;
> > @@ -511,7 +511,7 @@ static void a6xx_get_debugbus(struct msm_gpu *gpu,
> >               const struct a6xx_debugbus_block *cx_debugbus_blocks;
> >
> >               if (adreno_is_a7xx(adreno_gpu)) {
> > -                     BUG_ON(!(adreno_is_a730(adreno_gpu) || adreno_is_a740_family(adreno_gpu)));
> > +                     BUG_ON(adreno_gpu->info->family > ADRENO_7XX_GEN3);
> >                       cx_debugbus_blocks = a7xx_cx_debugbus_blocks;
> >                       nr_cx_debugbus_blocks = ARRAY_SIZE(a7xx_cx_debugbus_blocks);
> >               } else {
> > @@ -662,11 +662,11 @@ static void a7xx_get_dbgahb_clusters(struct msm_gpu *gpu,
> >       const struct gen7_sptp_cluster_registers *dbgahb_clusters;
> >       unsigned dbgahb_clusters_size;
> >
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               dbgahb_clusters = gen7_0_0_sptp_clusters;
> >               dbgahb_clusters_size = ARRAY_SIZE(gen7_0_0_sptp_clusters);
> >       } else {
> > -             BUG_ON(!adreno_is_a740_family(adreno_gpu));
> > +             BUG_ON(adreno_gpu->info->family > ADRENO_7XX_GEN3);
> >               dbgahb_clusters = gen7_2_0_sptp_clusters;
> >               dbgahb_clusters_size = ARRAY_SIZE(gen7_2_0_sptp_clusters);
> >       }
> > @@ -820,14 +820,14 @@ static void a7xx_get_clusters(struct msm_gpu *gpu,
> >       const struct gen7_cluster_registers *clusters;
> >       unsigned clusters_size;
> >
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               clusters = gen7_0_0_clusters;
> >               clusters_size = ARRAY_SIZE(gen7_0_0_clusters);
> > -     } else if (adreno_is_a740_family(adreno_gpu)) {
> > +     } else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
> >               clusters = gen7_2_0_clusters;
> >               clusters_size = ARRAY_SIZE(gen7_2_0_clusters);
> >       } else {
> > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> >               clusters = gen7_9_0_clusters;
> >               clusters_size = ARRAY_SIZE(gen7_9_0_clusters);
> >       }
> > @@ -895,7 +895,7 @@ static void a7xx_get_shader_block(struct msm_gpu *gpu,
> >       if (WARN_ON(datasize > A6XX_CD_DATA_SIZE))
> >               return;
> >
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               gpu_rmw(gpu, REG_A7XX_SP_DBG_CNTL, GENMASK(1, 0), 3);
> >       }
> >
> > @@ -925,7 +925,7 @@ static void a7xx_get_shader_block(struct msm_gpu *gpu,
> >               datasize);
> >
> >  out:
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               gpu_rmw(gpu, REG_A7XX_SP_DBG_CNTL, GENMASK(1, 0), 0);
> >       }
> >  }
> > @@ -958,14 +958,14 @@ static void a7xx_get_shaders(struct msm_gpu *gpu,
> >       unsigned num_shader_blocks;
> >       int i;
> >
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               shader_blocks = gen7_0_0_shader_blocks;
> >               num_shader_blocks = ARRAY_SIZE(gen7_0_0_shader_blocks);
> > -     } else if (adreno_is_a740_family(adreno_gpu)) {
> > +     } else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
> >               shader_blocks = gen7_2_0_shader_blocks;
> >               num_shader_blocks = ARRAY_SIZE(gen7_2_0_shader_blocks);
> >       } else {
> > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> >               shader_blocks = gen7_9_0_shader_blocks;
> >               num_shader_blocks = ARRAY_SIZE(gen7_9_0_shader_blocks);
> >       }
> > @@ -1350,14 +1350,14 @@ static void a7xx_get_registers(struct msm_gpu *gpu,
> >       const u32 *pre_crashdumper_regs;
> >       const struct gen7_reg_list *reglist;
> >
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               reglist = gen7_0_0_reg_list;
> >               pre_crashdumper_regs = gen7_0_0_pre_crashdumper_gpu_registers;
> > -     } else if (adreno_is_a740_family(adreno_gpu)) {
> > +     } else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
> >               reglist = gen7_2_0_reg_list;
> >               pre_crashdumper_regs = gen7_0_0_pre_crashdumper_gpu_registers;
> >       } else {
> > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> >               reglist = gen7_9_0_reg_list;
> >               pre_crashdumper_regs = gen7_9_0_pre_crashdumper_gpu_registers;
> >       }
> > @@ -1407,8 +1407,7 @@ static void a7xx_get_post_crashdumper_registers(struct msm_gpu *gpu,
> >       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> >       const u32 *regs;
> >
> > -     BUG_ON(!(adreno_is_a730(adreno_gpu) || adreno_is_a740_family(adreno_gpu) ||
> > -              adreno_is_a750(adreno_gpu)));
> > +     BUG_ON(adreno_gpu->info->family > ADRENO_7XX_GEN3);
> >       regs = gen7_0_0_post_crashdumper_registers;
> >
> >       a7xx_get_ahb_gpu_registers(gpu,
> > @@ -1514,11 +1513,11 @@ static void a7xx_get_indexed_registers(struct msm_gpu *gpu,
> >       const struct a6xx_indexed_registers *indexed_regs;
> >       int i, indexed_count, mempool_count;
> >
> > -     if (adreno_is_a730(adreno_gpu) || adreno_is_a740_family(adreno_gpu)) {
> > +     if (adreno_gpu->info->family <= ADRENO_7XX_GEN2) {
> >               indexed_regs = a7xx_indexed_reglist;
> >               indexed_count = ARRAY_SIZE(a7xx_indexed_reglist);
> >       } else {
> > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> >               indexed_regs = gen7_9_0_cp_indexed_reg_list;
> >               indexed_count = ARRAY_SIZE(gen7_9_0_cp_indexed_reg_list);
> >       }
> >
> > --
> > 2.31.1
> >
Connor Abbott Aug. 12, 2024, 6:25 p.m. UTC | #4
On Mon, Aug 12, 2024 at 7:09 AM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>
> On Wed, Aug 07, 2024 at 01:34:27PM +0100, Connor Abbott wrote:
> > With a7xx, we need to import a new header for each new generation and
> > switch to a different list of registers, instead of making
> > backwards-compatible changes. Using the helpers inadvertently made a750
> > use the a740 list of registers, instead use the family directly to fix
> > this.
>
> This won't scale. What about other gpus in the same generation but has a
> different register list? You don't see that issue currently because
> there are no support for lower tier a7x GPUs yet.

GPUs in the same generation always have the same register list. e.g.
gen7_4_0 has the same register list as gen7_0_0. kgsl has already
moved onto gen8 which redoes everything again and will require a
separate codepath, they only have one more gen7 register list compared
to us, and I doubt they'll add many more. So the kgsl approach would
be pointless over-engineering.

Connor

>
> I think we should move to a "snapshot block list" like in the downstream
> driver if you want to simplify the whole logic. Otherwise, we should
> leave the chipid check as it is and just fix up a750 configurations.
>
> -Akhil
>
> >
> > Fixes: f3f8207d8aed ("drm/msm: Add devcoredump support for a750")
> > Signed-off-by: Connor Abbott <cwabbott0@gmail.com>
> > ---
> >  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 41 ++++++++++++++---------------
> >  1 file changed, 20 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > index 77146d30bcaa..c641ee7dec78 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > @@ -390,18 +390,18 @@ static void a7xx_get_debugbus_blocks(struct msm_gpu *gpu,
> >       const u32 *debugbus_blocks, *gbif_debugbus_blocks;
> >       int i;
> >
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               debugbus_blocks = gen7_0_0_debugbus_blocks;
> >               debugbus_blocks_count = ARRAY_SIZE(gen7_0_0_debugbus_blocks);
> >               gbif_debugbus_blocks = a7xx_gbif_debugbus_blocks;
> >               gbif_debugbus_blocks_count = ARRAY_SIZE(a7xx_gbif_debugbus_blocks);
> > -     } else if (adreno_is_a740_family(adreno_gpu)) {
> > +     } else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
> >               debugbus_blocks = gen7_2_0_debugbus_blocks;
> >               debugbus_blocks_count = ARRAY_SIZE(gen7_2_0_debugbus_blocks);
> >               gbif_debugbus_blocks = a7xx_gbif_debugbus_blocks;
> >               gbif_debugbus_blocks_count = ARRAY_SIZE(a7xx_gbif_debugbus_blocks);
> >       } else {
> > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> >               debugbus_blocks = gen7_9_0_debugbus_blocks;
> >               debugbus_blocks_count = ARRAY_SIZE(gen7_9_0_debugbus_blocks);
> >               gbif_debugbus_blocks = gen7_9_0_gbif_debugbus_blocks;
> > @@ -511,7 +511,7 @@ static void a6xx_get_debugbus(struct msm_gpu *gpu,
> >               const struct a6xx_debugbus_block *cx_debugbus_blocks;
> >
> >               if (adreno_is_a7xx(adreno_gpu)) {
> > -                     BUG_ON(!(adreno_is_a730(adreno_gpu) || adreno_is_a740_family(adreno_gpu)));
> > +                     BUG_ON(adreno_gpu->info->family > ADRENO_7XX_GEN3);
> >                       cx_debugbus_blocks = a7xx_cx_debugbus_blocks;
> >                       nr_cx_debugbus_blocks = ARRAY_SIZE(a7xx_cx_debugbus_blocks);
> >               } else {
> > @@ -662,11 +662,11 @@ static void a7xx_get_dbgahb_clusters(struct msm_gpu *gpu,
> >       const struct gen7_sptp_cluster_registers *dbgahb_clusters;
> >       unsigned dbgahb_clusters_size;
> >
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               dbgahb_clusters = gen7_0_0_sptp_clusters;
> >               dbgahb_clusters_size = ARRAY_SIZE(gen7_0_0_sptp_clusters);
> >       } else {
> > -             BUG_ON(!adreno_is_a740_family(adreno_gpu));
> > +             BUG_ON(adreno_gpu->info->family > ADRENO_7XX_GEN3);
> >               dbgahb_clusters = gen7_2_0_sptp_clusters;
> >               dbgahb_clusters_size = ARRAY_SIZE(gen7_2_0_sptp_clusters);
> >       }
> > @@ -820,14 +820,14 @@ static void a7xx_get_clusters(struct msm_gpu *gpu,
> >       const struct gen7_cluster_registers *clusters;
> >       unsigned clusters_size;
> >
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               clusters = gen7_0_0_clusters;
> >               clusters_size = ARRAY_SIZE(gen7_0_0_clusters);
> > -     } else if (adreno_is_a740_family(adreno_gpu)) {
> > +     } else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
> >               clusters = gen7_2_0_clusters;
> >               clusters_size = ARRAY_SIZE(gen7_2_0_clusters);
> >       } else {
> > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> >               clusters = gen7_9_0_clusters;
> >               clusters_size = ARRAY_SIZE(gen7_9_0_clusters);
> >       }
> > @@ -895,7 +895,7 @@ static void a7xx_get_shader_block(struct msm_gpu *gpu,
> >       if (WARN_ON(datasize > A6XX_CD_DATA_SIZE))
> >               return;
> >
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               gpu_rmw(gpu, REG_A7XX_SP_DBG_CNTL, GENMASK(1, 0), 3);
> >       }
> >
> > @@ -925,7 +925,7 @@ static void a7xx_get_shader_block(struct msm_gpu *gpu,
> >               datasize);
> >
> >  out:
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               gpu_rmw(gpu, REG_A7XX_SP_DBG_CNTL, GENMASK(1, 0), 0);
> >       }
> >  }
> > @@ -958,14 +958,14 @@ static void a7xx_get_shaders(struct msm_gpu *gpu,
> >       unsigned num_shader_blocks;
> >       int i;
> >
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               shader_blocks = gen7_0_0_shader_blocks;
> >               num_shader_blocks = ARRAY_SIZE(gen7_0_0_shader_blocks);
> > -     } else if (adreno_is_a740_family(adreno_gpu)) {
> > +     } else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
> >               shader_blocks = gen7_2_0_shader_blocks;
> >               num_shader_blocks = ARRAY_SIZE(gen7_2_0_shader_blocks);
> >       } else {
> > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> >               shader_blocks = gen7_9_0_shader_blocks;
> >               num_shader_blocks = ARRAY_SIZE(gen7_9_0_shader_blocks);
> >       }
> > @@ -1350,14 +1350,14 @@ static void a7xx_get_registers(struct msm_gpu *gpu,
> >       const u32 *pre_crashdumper_regs;
> >       const struct gen7_reg_list *reglist;
> >
> > -     if (adreno_is_a730(adreno_gpu)) {
> > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> >               reglist = gen7_0_0_reg_list;
> >               pre_crashdumper_regs = gen7_0_0_pre_crashdumper_gpu_registers;
> > -     } else if (adreno_is_a740_family(adreno_gpu)) {
> > +     } else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
> >               reglist = gen7_2_0_reg_list;
> >               pre_crashdumper_regs = gen7_0_0_pre_crashdumper_gpu_registers;
> >       } else {
> > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> >               reglist = gen7_9_0_reg_list;
> >               pre_crashdumper_regs = gen7_9_0_pre_crashdumper_gpu_registers;
> >       }
> > @@ -1407,8 +1407,7 @@ static void a7xx_get_post_crashdumper_registers(struct msm_gpu *gpu,
> >       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> >       const u32 *regs;
> >
> > -     BUG_ON(!(adreno_is_a730(adreno_gpu) || adreno_is_a740_family(adreno_gpu) ||
> > -              adreno_is_a750(adreno_gpu)));
> > +     BUG_ON(adreno_gpu->info->family > ADRENO_7XX_GEN3);
> >       regs = gen7_0_0_post_crashdumper_registers;
> >
> >       a7xx_get_ahb_gpu_registers(gpu,
> > @@ -1514,11 +1513,11 @@ static void a7xx_get_indexed_registers(struct msm_gpu *gpu,
> >       const struct a6xx_indexed_registers *indexed_regs;
> >       int i, indexed_count, mempool_count;
> >
> > -     if (adreno_is_a730(adreno_gpu) || adreno_is_a740_family(adreno_gpu)) {
> > +     if (adreno_gpu->info->family <= ADRENO_7XX_GEN2) {
> >               indexed_regs = a7xx_indexed_reglist;
> >               indexed_count = ARRAY_SIZE(a7xx_indexed_reglist);
> >       } else {
> > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> >               indexed_regs = gen7_9_0_cp_indexed_reg_list;
> >               indexed_count = ARRAY_SIZE(gen7_9_0_cp_indexed_reg_list);
> >       }
> >
> > --
> > 2.31.1
> >
Akhil P Oommen Aug. 12, 2024, 8:01 p.m. UTC | #5
On Mon, Aug 12, 2024 at 07:25:14PM +0100, Connor Abbott wrote:
> On Mon, Aug 12, 2024 at 7:09 AM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
> >
> > On Wed, Aug 07, 2024 at 01:34:27PM +0100, Connor Abbott wrote:
> > > With a7xx, we need to import a new header for each new generation and
> > > switch to a different list of registers, instead of making
> > > backwards-compatible changes. Using the helpers inadvertently made a750
> > > use the a740 list of registers, instead use the family directly to fix
> > > this.
> >
> > This won't scale. What about other gpus in the same generation but has a
> > different register list? You don't see that issue currently because
> > there are no support for lower tier a7x GPUs yet.
> 
> GPUs in the same generation always have the same register list. e.g.
> gen7_4_0 has the same register list as gen7_0_0. kgsl has already
> moved onto gen8 which redoes everything again and will require a
> separate codepath, they only have one more gen7 register list compared
> to us, and I doubt they'll add many more. So the kgsl approach would
> be pointless over-engineering.

https://git.codelinaro.org/clo/la/platform/vendor/qcom/opensource/graphics-kernel/-/tree/gfx-kernel.lnx.1.0.r48-rel?ref_type=heads

Not sure if there is another more recent public facing kgsl code than this
one, but at least this lists 2 more snapshot headers we will have to
consider in future. And there are other a7x GPUs and a8x (even though a8x
should be a separate HWL, it is good to have a similar code structure).

I am not saying you should do the extra engineering at this point, but I don't
think we should move things in the other direction.

-Akhil

> 
> Connor
> 
> >
> > I think we should move to a "snapshot block list" like in the downstream
> > driver if you want to simplify the whole logic. Otherwise, we should
> > leave the chipid check as it is and just fix up a750 configurations.
> >
> > -Akhil
> >
> > >
> > > Fixes: f3f8207d8aed ("drm/msm: Add devcoredump support for a750")
> > > Signed-off-by: Connor Abbott <cwabbott0@gmail.com>
> > > ---
> > >  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 41 ++++++++++++++---------------
> > >  1 file changed, 20 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > > index 77146d30bcaa..c641ee7dec78 100644
> > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > > @@ -390,18 +390,18 @@ static void a7xx_get_debugbus_blocks(struct msm_gpu *gpu,
> > >       const u32 *debugbus_blocks, *gbif_debugbus_blocks;
> > >       int i;
> > >
> > > -     if (adreno_is_a730(adreno_gpu)) {
> > > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> > >               debugbus_blocks = gen7_0_0_debugbus_blocks;
> > >               debugbus_blocks_count = ARRAY_SIZE(gen7_0_0_debugbus_blocks);
> > >               gbif_debugbus_blocks = a7xx_gbif_debugbus_blocks;
> > >               gbif_debugbus_blocks_count = ARRAY_SIZE(a7xx_gbif_debugbus_blocks);
> > > -     } else if (adreno_is_a740_family(adreno_gpu)) {
> > > +     } else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
> > >               debugbus_blocks = gen7_2_0_debugbus_blocks;
> > >               debugbus_blocks_count = ARRAY_SIZE(gen7_2_0_debugbus_blocks);
> > >               gbif_debugbus_blocks = a7xx_gbif_debugbus_blocks;
> > >               gbif_debugbus_blocks_count = ARRAY_SIZE(a7xx_gbif_debugbus_blocks);
> > >       } else {
> > > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> > >               debugbus_blocks = gen7_9_0_debugbus_blocks;
> > >               debugbus_blocks_count = ARRAY_SIZE(gen7_9_0_debugbus_blocks);
> > >               gbif_debugbus_blocks = gen7_9_0_gbif_debugbus_blocks;
> > > @@ -511,7 +511,7 @@ static void a6xx_get_debugbus(struct msm_gpu *gpu,
> > >               const struct a6xx_debugbus_block *cx_debugbus_blocks;
> > >
> > >               if (adreno_is_a7xx(adreno_gpu)) {
> > > -                     BUG_ON(!(adreno_is_a730(adreno_gpu) || adreno_is_a740_family(adreno_gpu)));
> > > +                     BUG_ON(adreno_gpu->info->family > ADRENO_7XX_GEN3);
> > >                       cx_debugbus_blocks = a7xx_cx_debugbus_blocks;
> > >                       nr_cx_debugbus_blocks = ARRAY_SIZE(a7xx_cx_debugbus_blocks);
> > >               } else {
> > > @@ -662,11 +662,11 @@ static void a7xx_get_dbgahb_clusters(struct msm_gpu *gpu,
> > >       const struct gen7_sptp_cluster_registers *dbgahb_clusters;
> > >       unsigned dbgahb_clusters_size;
> > >
> > > -     if (adreno_is_a730(adreno_gpu)) {
> > > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> > >               dbgahb_clusters = gen7_0_0_sptp_clusters;
> > >               dbgahb_clusters_size = ARRAY_SIZE(gen7_0_0_sptp_clusters);
> > >       } else {
> > > -             BUG_ON(!adreno_is_a740_family(adreno_gpu));
> > > +             BUG_ON(adreno_gpu->info->family > ADRENO_7XX_GEN3);
> > >               dbgahb_clusters = gen7_2_0_sptp_clusters;
> > >               dbgahb_clusters_size = ARRAY_SIZE(gen7_2_0_sptp_clusters);
> > >       }
> > > @@ -820,14 +820,14 @@ static void a7xx_get_clusters(struct msm_gpu *gpu,
> > >       const struct gen7_cluster_registers *clusters;
> > >       unsigned clusters_size;
> > >
> > > -     if (adreno_is_a730(adreno_gpu)) {
> > > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> > >               clusters = gen7_0_0_clusters;
> > >               clusters_size = ARRAY_SIZE(gen7_0_0_clusters);
> > > -     } else if (adreno_is_a740_family(adreno_gpu)) {
> > > +     } else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
> > >               clusters = gen7_2_0_clusters;
> > >               clusters_size = ARRAY_SIZE(gen7_2_0_clusters);
> > >       } else {
> > > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> > >               clusters = gen7_9_0_clusters;
> > >               clusters_size = ARRAY_SIZE(gen7_9_0_clusters);
> > >       }
> > > @@ -895,7 +895,7 @@ static void a7xx_get_shader_block(struct msm_gpu *gpu,
> > >       if (WARN_ON(datasize > A6XX_CD_DATA_SIZE))
> > >               return;
> > >
> > > -     if (adreno_is_a730(adreno_gpu)) {
> > > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> > >               gpu_rmw(gpu, REG_A7XX_SP_DBG_CNTL, GENMASK(1, 0), 3);
> > >       }
> > >
> > > @@ -925,7 +925,7 @@ static void a7xx_get_shader_block(struct msm_gpu *gpu,
> > >               datasize);
> > >
> > >  out:
> > > -     if (adreno_is_a730(adreno_gpu)) {
> > > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> > >               gpu_rmw(gpu, REG_A7XX_SP_DBG_CNTL, GENMASK(1, 0), 0);
> > >       }
> > >  }
> > > @@ -958,14 +958,14 @@ static void a7xx_get_shaders(struct msm_gpu *gpu,
> > >       unsigned num_shader_blocks;
> > >       int i;
> > >
> > > -     if (adreno_is_a730(adreno_gpu)) {
> > > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> > >               shader_blocks = gen7_0_0_shader_blocks;
> > >               num_shader_blocks = ARRAY_SIZE(gen7_0_0_shader_blocks);
> > > -     } else if (adreno_is_a740_family(adreno_gpu)) {
> > > +     } else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
> > >               shader_blocks = gen7_2_0_shader_blocks;
> > >               num_shader_blocks = ARRAY_SIZE(gen7_2_0_shader_blocks);
> > >       } else {
> > > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> > >               shader_blocks = gen7_9_0_shader_blocks;
> > >               num_shader_blocks = ARRAY_SIZE(gen7_9_0_shader_blocks);
> > >       }
> > > @@ -1350,14 +1350,14 @@ static void a7xx_get_registers(struct msm_gpu *gpu,
> > >       const u32 *pre_crashdumper_regs;
> > >       const struct gen7_reg_list *reglist;
> > >
> > > -     if (adreno_is_a730(adreno_gpu)) {
> > > +     if (adreno_gpu->info->family == ADRENO_7XX_GEN1) {
> > >               reglist = gen7_0_0_reg_list;
> > >               pre_crashdumper_regs = gen7_0_0_pre_crashdumper_gpu_registers;
> > > -     } else if (adreno_is_a740_family(adreno_gpu)) {
> > > +     } else if (adreno_gpu->info->family == ADRENO_7XX_GEN2) {
> > >               reglist = gen7_2_0_reg_list;
> > >               pre_crashdumper_regs = gen7_0_0_pre_crashdumper_gpu_registers;
> > >       } else {
> > > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> > >               reglist = gen7_9_0_reg_list;
> > >               pre_crashdumper_regs = gen7_9_0_pre_crashdumper_gpu_registers;
> > >       }
> > > @@ -1407,8 +1407,7 @@ static void a7xx_get_post_crashdumper_registers(struct msm_gpu *gpu,
> > >       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> > >       const u32 *regs;
> > >
> > > -     BUG_ON(!(adreno_is_a730(adreno_gpu) || adreno_is_a740_family(adreno_gpu) ||
> > > -              adreno_is_a750(adreno_gpu)));
> > > +     BUG_ON(adreno_gpu->info->family > ADRENO_7XX_GEN3);
> > >       regs = gen7_0_0_post_crashdumper_registers;
> > >
> > >       a7xx_get_ahb_gpu_registers(gpu,
> > > @@ -1514,11 +1513,11 @@ static void a7xx_get_indexed_registers(struct msm_gpu *gpu,
> > >       const struct a6xx_indexed_registers *indexed_regs;
> > >       int i, indexed_count, mempool_count;
> > >
> > > -     if (adreno_is_a730(adreno_gpu) || adreno_is_a740_family(adreno_gpu)) {
> > > +     if (adreno_gpu->info->family <= ADRENO_7XX_GEN2) {
> > >               indexed_regs = a7xx_indexed_reglist;
> > >               indexed_count = ARRAY_SIZE(a7xx_indexed_reglist);
> > >       } else {
> > > -             BUG_ON(!adreno_is_a750(adreno_gpu));
> > > +             BUG_ON(adreno_gpu->info->family != ADRENO_7XX_GEN3);
> > >               indexed_regs = gen7_9_0_cp_indexed_reg_list;
> > >               indexed_count = ARRAY_SIZE(gen7_9_0_cp_indexed_reg_list);
> > >       }
> > >
> > > --
> > > 2.31.1
> > >