Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] drm/msm/a8xx: wrong GX power check and early GX read in a8xx_gpu_state_get() - [High] drm/msm/a8xx: off-by-one slice ID and ignored error in gen8_snapshot_mempool() - [Medium] drm/msm/a8xx: unrestored DBGC config after gen8_debugbus_internal_read_setup() - [Medium] drm/msm/a8xx: hardcoded context 0 in gen8_snapshot_dbgahb_regs() SP_READ_SEL - [Medium] drm/msm/a8xx: broken host aperture setup in legacy AHB snapshot helpers - [Low] drm/msm/a8xx: 32-bit GENMASK(63, 44) overflow in CD_WRITE() and CD_READ() --
commit 2b161a7f68dabf614abb5d253ffd0443bf998e9b Author: Rob Clark <[email protected]> drm/msm/a8xx: devcoredump state capture support This commit adds devcoredump GPU state capture and crashdumper support for Adreno A8xx (Gen8) GPUs, along with snapshot section helpers in adreno_snapshot.h. > diff --git a/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c > b/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c > new file mode 100644 > index 0000000000000..e4eb2f335a890 > --- /dev/null > +++ b/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c > @@ -0,0 +1,1253 @@ [ ... ] > +static void gen8_legacy_snapshot_registers(struct msm_gpu *gpu, > + struct adreno_gpu_state *state, > + const struct gen8_reg_list *regs, > + u32 slice_id) > +{ > + int count; > + > + /* Figure out how many registers we are going to dump */ > + count = adreno_snapshot_regs_count(regs->regs); > + > + snapshot_sect (state, MVC_V3, 4 * count, { > + .ctxt_id = 0, > + .cluster_id = CLUSTER_NONE, > + .pipe_id = PIPE_NONE, > + .location_id = UINT_MAX, > + .sp_id = UINT_MAX, > + .usptp_id = UINT_MAX, > + .slice_id = HEADER_SLICE_ID(regs->slice_region, slice_id), > + }) { > + unsigned long flags; > + > + if (regs->sel) > + gpu_write(gpu, regs->sel->host_reg, regs->sel->val); > + > + if (regs->slice_region) > + a8xx_aperture_acquire_slice(gpu, PIPE_NONE, slice_id, > false, &flags); > + > + /* Make sure the previous writes are posted before reading */ > + mb(); > + > + for (const u32 *ptr = regs->regs; ptr[0] != UINT_MAX; ptr += 2) > { > + snapshot_mvc_regs_v3_subheader(§, ptr); > + > + for (int k = ptr[0]; k <= ptr[1]; k++) { > + snapshot_sect_u32(§, reg_read(gpu, k)); > + } > + } > + > + if (regs->slice_region) > + a8xx_aperture_release(gpu, flags); > + } > +} [ ... ] > +static bool gen8_snapshot_mempool(struct msm_gpu *gpu, > + struct adreno_gpu_state *state, > + struct adreno_crashdumper *dumper) > +{ > + const struct gen8_snapshot_block_list *b = block_list(gpu); > + u32 slice_mask = to_a6xx_gpu(to_adreno_gpu(gpu))->slice_mask; > + u32 first_slice = ffs(slice_mask); > + > + for (int i = 0; i < b->mempool_index_registers_len; i++) { > + const struct gen8_cp_indexed_reg *cp_indexed_reg = > + &b->mempool_index_registers[i]; > + unsigned long flags; > + u32 j; > + > + FOR_EACH_SLICE (j, cp_indexed_reg->slice_region, slice_mask) { > + > + /* TODO: Remove hardcoded values */ > + /* Set CP_CHICKEN_DBG[StabilizeMVC] to stabilize it > while dumping */ > + a8xx_aperture_acquire_slice(gpu, > cp_indexed_reg->pipe_id, first_slice, false, &flags); > + gpu_rmw(gpu, REG_A8XX_CP_CHICKEN_DBG_PIPE, BIT(2), > BIT(2)); > + a8xx_aperture_release(gpu, flags); > + > + a8xx_aperture_acquire_slice(gpu, > cp_indexed_reg->pipe_id, j, true, &flags); > + gpu_rmw(gpu, REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE, > BIT(2), BIT(2)); > + a8xx_aperture_release(gpu, flags); > + > + gen8_snapshot_indexed_registers(gpu, state, dumper, > + cp_indexed_reg->addr, > + cp_indexed_reg->data, 0, > + cp_indexed_reg->size, > + cp_indexed_reg->pipe_id, > + HEADER_SLICE_ID(cp_indexed_reg->slice_region, > j)); > + > + /* Reset CP_CHICKEN_DBG[StabilizeMVC] once we are done > */ > + a8xx_aperture_acquire_slice(gpu, > cp_indexed_reg->pipe_id, first_slice, false, &flags); > + gpu_rmw(gpu, REG_A8XX_CP_CHICKEN_DBG_PIPE, BIT(2), 0); > + a8xx_aperture_release(gpu, flags); > + > + a8xx_aperture_acquire_slice(gpu, > cp_indexed_reg->pipe_id, j, true, &flags); > + gpu_rmw(gpu, REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE, > BIT(2), 0); > + a8xx_aperture_release(gpu, flags); > + } > + } > + > + /* Clear aperture register */ > + a8xx_aperture_clear(gpu); > + > + return true; > +} [Severity: High] In drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c, when a8xx_gpu_state_get() calls gen8_snapshot_mempool(), first_slice is computed as ffs(slice_mask). Because ffs() returns a 1-based bit index (unlike a8xx_get_first_slice() in drivers/gpu/drm/msm/adreno/a8xx_gpu.c, which uses ffs(a6xx_gpu->slice_mask) - 1), passing first_slice to a8xx_aperture_acquire_slice() before modifying REG_A8XX_CP_CHICKEN_DBG_PIPE programs an off-by-one slice ID. On a 1-slice GPU such as Milos (where slice_mask is 0x1) or when slice ffs(slice_mask) is fused off in slice_mask, this selects a non-existent or harvested slice. In addition, for UNSLICE entries in b->mempool_index_registers, FOR_EACH_SLICE sets j = 0 regardless of slice_mask, and gen8_snapshot_mempool() still programs the aperture for slice j = 0 with use_slice = true to modify REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE even if slice 0 is fused off. Finally, gen8_snapshot_mempool() ignores the return value of gen8_snapshot_indexed_registers() and always returns true. If the crashdumper times out during mempool capture, a8xx_gpu_state_get() will not bail out via goto out and will continue issuing register accesses. Should first_slice be ffs(slice_mask) - 1, should REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE only be accessed when cp_indexed_reg->slice_region == SLICE, and should gen8_snapshot_mempool() propagate failures from gen8_snapshot_indexed_registers()? > +static void gen8_legacy_snapshot_mvc(struct msm_gpu *gpu, > + struct adreno_gpu_state *state, > + const struct gen8_cluster_registers > *cluster, > + u32 slice_id) > +{ > + int count; > + > + /* Figure out how many registers we are going to dump */ > + count = adreno_snapshot_regs_count(cluster->regs); > + > + snapshot_sect (state, MVC_V3, 4 * count, { > + .ctxt_id = (cluster->context_id == STATE_FORCE_CTXT_1) ? 1 : 0, > + .cluster_id = cluster->cluster_id, > + .pipe_id = cluster->pipe_id, > + .location_id = UINT_MAX, > + .sp_id = UINT_MAX, > + .usptp_id = UINT_MAX, > + .slice_id = HEADER_SLICE_ID(cluster->slice_region, slice_id), > + }) { > + unsigned long flags; > + > + /* > + * Set the AHB control for the Host to read from the > + * cluster/context for this iteration. > + */ > + a8xx_aperture_acquire_slice(gpu, cluster->pipe_id, slice_id, > true, &flags); > + > + if (cluster->sel) > + gpu_write(gpu, cluster->sel->host_reg, > cluster->sel->val); > + > + /* Make sure the previous writes are posted before reading */ > + mb(); > + > + for (const u32 *ptr = cluster->regs; ptr[0] != UINT_MAX; ptr += > 2) { > + snapshot_mvc_regs_v3_subheader(§, ptr); > + > + for (int k = ptr[0]; k <= ptr[1]; k++) { > + snapshot_sect_u32(§, reg_read(gpu, k)); > + } > + } > + > + a8xx_aperture_release(gpu, flags); > + } > +} [Severity: Medium] Does the host aperture programming in gen8_legacy_snapshot_mvc() and gen8_legacy_snapshot_registers() in drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c match what the hardware expects when dumping registers over AHB (for b->pre_crashdumper_regs and when dumper is NULL, such as during an SMMU stall fault)? 1. In gen8_legacy_snapshot_mvc(), calling a8xx_aperture_acquire_slice() only programs PIPEID, SLICEID, and USESLICEID in REG_A8XX_CP_APERTURE_CNTL_HOST, leaving CLUSTERID and CONTEXTID3D as 0 (whereas the crashdumper path in gen8_snapshot_mvc_regs() writes cluster->cluster_id and cluster->context_id via GEN8_CP_APERTURE_REG_VAL()). All cluster and context-banked MVC registers read on the CPU fallback path will therefore read cluster 0 and context 0. 2. In gen8_legacy_snapshot_registers(): if (regs->sel) gpu_write(gpu, regs->sel->host_reg, regs->sel->val); if (regs->slice_region) a8xx_aperture_acquire_slice(gpu, PIPE_NONE, slice_id, false, &flags); regs->sel->host_reg is written before acquiring the aperture lock or setting the slice, a8xx_aperture_acquire_slice() is called with use_slice = false instead of true for SLICE regions (omitting A8XX_CP_APERTURE_CNTL_HOST_USESLICEID), and when a subsequent region has regs->slice_region == UNSLICE, the aperture is not cleared or updated, leaving the previous slice selected. 3. Because a8xx_aperture_release() only drops a6xx_gpu->aperture_lock without clearing REG_A8XX_CP_APERTURE_CNTL_HOST, and the out: path in a8xx_gpu_state_get() does not call a8xx_aperture_clear(gpu), a CPU-fallback state capture during a non-reset SMMU stall fault leaves REG_A8XX_CP_APERTURE_CNTL_HOST and a6xx_gpu->cached_aperture dirty for subsequent host register accesses. [ ... ] > +static void gen8_legacy_snapshot_cluster_dbgahb( > + struct msm_gpu *gpu, struct adreno_gpu_state *state, > + const struct gen8_sptp_cluster_registers *cluster, > + u32 sp, u32 usptp, u32 slice_id) > +{ > + int count; > + > + /* Figure out how many registers we are going to dump */ > + count = adreno_snapshot_regs_count(cluster->regs); > + > + snapshot_sect (state, MVC_V3, 4 * count, { > + .ctxt_id = cluster->context_id, > + .cluster_id = cluster->cluster_id, > + .pipe_id = cluster->pipe_id, > + .location_id = cluster->location_id, > + .sp_id = sp, > + .usptp_id = usptp, > + .slice_id = HEADER_SLICE_ID(cluster->slice_region, slice_id), > + }) { > + u32 read_sel = GEN8_SP_READ_SEL_VAL( > + 0, slice_id, cluster->location_id, cluster->pipe_id, > + cluster->statetype, usptp, sp); > + > + gpu_write(gpu, REG_A7XX_SP_READ_SEL, read_sel); [ ... ] > +static bool gen8_snapshot_dbgahb_regs(struct msm_gpu *gpu, > + struct adreno_gpu_state *state, > + struct adreno_crashdumper *dumper) > +{ [ ... ] > + /* Build the crash script */ > + struct cd_script p = CD_INIT(dumper); > + > + u32 read_sel = GEN8_SP_READ_SEL_VAL( > + 0, slice_id, > cluster->location_id, > + cluster->pipe_id, > cluster->statetype, > + usptp, sp); > + > + /* Program the aperture */ > + CD_WRITE(&p, REG_A7XX_SP_READ_SEL, > read_sel); [Severity: Medium] In drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c, both gen8_legacy_snapshot_cluster_dbgahb() and gen8_snapshot_dbgahb_regs() record .ctxt_id = cluster->context_id in the MVC_V3 section header, but pass a hardcoded 0 instead of cluster->context_id as the first argument (_contextid) to GEN8_SP_READ_SEL_VAL() when programming REG_A7XX_SP_READ_SEL. For multi-context SPTP cluster entries in the A8xx snapshot tables (where cluster->context_id ranges from 0 to 15), won't this repeatedly dump context 0 for every context section? [ ... ] > +static void gen8_debugbus_internal_read_setup(struct msm_gpu *gpu) > +{ > + /* Clear the DBGC logic/events */ > + gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_CNTLM, 0x70000000); > + > + /* Enable the internal buffer mode and program to tail mode */ > + gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_OPL, 0x28000000); > + > + /* Program the logic unit 0 to high when the side data is 1 */ > + gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_BYTEL_0, 0x0000000c); > + gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_MASKL_0, 0x000000f0); > + > + /* Enable the DBGC logic/events */ > + gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_CNTLM, 0x07000000); > +} [ ... ] > +static void gen8_snapshot_cx_dbgc_debugbus_block( > + struct msm_gpu *gpu, uint32_t block_id, size_t count, > + struct adreno_snapshot_sect *sect) > +{ > + /* This block requires an out of order access due to a HW limitation. > + * (QCTDD12240539) > + */ > + if (block_id == DEBUGBUS_CX_GC_US_I_0) { > + static u32 data[GEN8_DEBUGBUS_BLOCK_SIZE * 2]; > + gen8_debugbus_internal_read_setup(gpu); > + for (int i = 0; i < GEN8_DEBUGBUS_BLOCK_IDX_1; i++) > + gen8_cx_gc_us_i_0_debugbus_read(gpu, block_id, i, > &data[i * 2]); > + for (int i = GEN8_DEBUGBUS_BLOCK_IDX_2; i < > GEN8_DEBUGBUS_BLOCK_SIZE; i++) > + gen8_cx_gc_us_i_0_debugbus_read(gpu, block_id, i, > &data[i * 2]); > + for (int i = GEN8_DEBUGBUS_BLOCK_IDX_1; i < > GEN8_DEBUGBUS_BLOCK_IDX_2; i++) > + gen8_cx_gc_us_i_0_debugbus_read(gpu, block_id, i, > &data[i * 2]); > + snapshot_sect_arr(sect, data); > + } else { > + for (int i = 0; i < count; i++) { > + u32 data[2]; > + gen8_cx_debug_bus_read(gpu, block_id, i, data); > + snapshot_sect_arr(sect, data); > + } > + } > +} [Severity: Medium] In drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c, gen8_get_debugbus() programs the initial debugbus configuration into REG_A6XX_DBGC_CFG_DBGBUS_CNTLT, REG_A6XX_DBGC_CFG_DBGBUS_CNTLM, REG_A6XX_DBGC_CFG_DBGBUS_IVTL_0..3, REG_A6XX_DBGC_CFG_DBGBUS_BYTEL_0..1, and REG_A6XX_DBGC_CFG_DBGBUS_MASKL_0..3 before iterating through b->debugbus_blocks[] and b->gbif_debugbus_blocks[]. When gen8_snapshot_cx_dbgc_debugbus_block() encounters DEBUGBUS_CX_GC_US_I_0 (at index 2 of b->debugbus_blocks[] in the A8xx snapshot tables), it calls gen8_debugbus_internal_read_setup(), which overwrites REG_A6XX_DBGC_CFG_DBGBUS_CNTLM, REG_A6XX_DBGC_CFG_DBGBUS_OPL, REG_A6XX_DBGC_CFG_DBGBUS_BYTEL_0, and REG_A6XX_DBGC_CFG_DBGBUS_MASKL_0 for internal buffer mode. Because those registers are not restored after reading DEBUGBUS_CX_GC_US_I_0, won't all subsequent blocks in b->debugbus_blocks[] and b->gbif_debugbus_blocks[] be read with the internal buffer mode configuration still active? [ ... ] > +struct msm_gpu_state *a8xx_gpu_state_get(struct msm_gpu *gpu) > +{ > + const struct gen8_snapshot_block_list *b = block_list(gpu); > + struct adreno_crashdumper _dumper = { 0 }, *dumper = NULL; > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); > + struct adreno_gpu_state *state; > + // TODO need to get status from SMMU on some gen8.. > + bool stalled = !!(gpu_read(gpu, REG_A8XX_RBBM_MISC_STATUS) & > + A8XX_RBBM_MISC_STATUS_SMMU_STALLED_ON_FAULT); > + > + if (!b) > + return NULL; > + > + state = kzalloc_obj(*state); > + if (!state) > + return ERR_PTR(-ENOMEM); > + > + INIT_LIST_HEAD(&state->objs); > + > + /* Get the generic state from the adreno core */ > + adreno_gpu_state_get(gpu, &state->base); > + > + adreno_snapshot_registers_v2(gpu, state, b->gmu_cx_unsliced_regs); > + > + a8xx_snapshot_gmu_bo(state, &a6xx_gpu->gmu.log, SNAPSHOT_GMU_MEM_LOG); > + a8xx_snapshot_gmu_bo(state, &a6xx_gpu->gmu.hfi, SNAPSHOT_GMU_MEM_HFI); > + a8xx_snapshot_gmu_bo(state, &a6xx_gpu->gmu.debug, > SNAPSHOT_GMU_MEM_DEBUG); > + > + a8xx_snapshot_gmu_hfi_history(gpu, state); > + > + /* If GX isn't on the rest of the data isn't going to be accessible */ > + if (!a6xx_gmu_gx_is_on(adreno_gpu)) > + return &state->base; [Severity: High] Can this cause unclocked or unpowered register accesses in a8xx_gpu_state_get() in drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c when state capture is triggered via debugfs or fault/hang recovery while GX (or GMU MX on A850) is powered off? First, at the top of a8xx_gpu_state_get(), REG_A8XX_RBBM_MISC_STATUS is read before checking whether GX is powered on, even though stalled is only used after the GX power check. Second, the GX power check calls a6xx_gmu_gx_is_on(adreno_gpu) instead of adreno_gpu->funcs->gx_is_on(adreno_gpu) (which points to a8xx_gmu_gx_is_on()). In drivers/gpu/drm/msm/adreno/a6xx_gmu.c: bool a8xx_gmu_gx_is_on(struct adreno_gpu *adreno_gpu) { ... if (adreno_is_a850_family(adreno_gpu)) { val = a6xx_cx_misc_read(a6xx_gpu, REG_A8XX_CX_MISC_GFX_PWR_CLK_STATUS); return !(val & (A8XX_CX_MISC_GFX_PWR_CLK_STATUS_GX_GDSC_POWER_OFF | A8XX_CX_MISC_GFX_PWR_CLK_STATUS_GX_CLK_OFF)); } val = gmu_read(gmu, REG_A8XX_GMU_PWR_CLK_STATUS); return !(val & (A8XX_GMU_PWR_CLK_STATUS_GX_HM_GDSC_POWER_OFF | A8XX_GMU_PWR_CLK_STATUS_GX_HM_CLK_OFF)); } Calling a6xx_gmu_gx_is_on() reads REG_A6XX_GMU_SPTPRAC_PWR_CLK_STATUS (an A6xx/A7xx register that is not valid on A8xx and accesses GMU MMIO even when GMU MX is off on A850) and checks the wrong bits, allowing a8xx_gpu_state_get() to proceed with GX register accesses when GX is powered off. [ ... ] > diff --git a/drivers/gpu/drm/msm/adreno/adreno_snapshot.h > b/drivers/gpu/drm/msm/adreno/adreno_snapshot.h > index f9956e0bc1d5f..e8ff102a628b7 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_snapshot.h > +++ b/drivers/gpu/drm/msm/adreno/adreno_snapshot.h [ ... ] > +static inline void CD_WRITE(struct cd_script *p, u32 reg, u32 val) > +{ > + *(p->in++) = val; > + *(p->in++) = FIELD_PREP(GENMASK(63, 44), reg) | BIT(21) | BIT(0); > + __cd_check_overflow(p); > +} > + > +static inline void CD_READ(struct cd_script *p, u32 reg, u32 dwords) > +{ > + *(p->in++) = p->out; > + *(p->in++) = FIELD_PREP(GENMASK(63, 44), reg) | dwords; > + p->out += sizeof(u32) * dwords; > + __cd_check_overflow(p); > +} [Severity: Low] In drivers/gpu/drm/msm/adreno/adreno_snapshot.h, CD_WRITE() and CD_READ() use GENMASK(63, 44) to build a 64-bit value for u64 *in. On 32-bit architectures (such as 32-bit ARM builds with CONFIG_DRM_MSM and CONFIG_DRM_MSM_GPU_STATE enabled), GENMASK() uses unsigned long (32-bit), which overflows the shift count and triggers a BUILD_BUG_ON_MSG() failure in FIELD_PREP(). Should these use GENMASK_ULL(63, 44) instead? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
