| Message ID | 20240807-a750-devcoredump-fixes-v2-0-d7483736d26d@gmail.com |
|---|---|
| Headers | show |
| Series | drm/msm: Fixes for devcoredump on a750 | expand |
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 >
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 > >
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 > >
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 > >
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 > > >
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,