Thank you for your contribution! Sashiko AI review found 4 potential issues to consider: - [High] drm/msm/a8xx: disabled slice 0 access in gen8_snapshot_mempool() - [Medium] drm/msm/a8xx: missing IFPC inhibition and SQE halt in a8xx_gpu_state_get() - [Low] drm/msm/a8xx: wrong CX_DBGC bitfield macros in gen8_cx_dbgc_debugbus_select() - [Low] drm/msm/a8xx: unguarded gen8_snapshot_cx_debugbus() in a8xx_gpu_state_get() --
commit b9d6ff67c11eafc0f39228630710e009a3fbc430 Author: Rob Clark <[email protected]> drm/msm/a8xx: devcoredump state capture support This commit adds GPU devcoredump state capture and crashdumper support for Adreno A8xx GPUs. > 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..44cf8ff75f98d > --- /dev/null > +++ b/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c > @@ -0,0 +1,1407 @@ [ ... ] > +#define NEXT_SLICE(_slice, _region, _slice_mask) \ > + ((_region == UNSLICE) ? fls(_slice_mask) : next_slice(_slice, > _slice_mask)) > + > +#define FIRST_SLICE(_region, _slice_mask) \ > + ((_region == UNSLICE) ? 0 : NEXT_SLICE(0, _region, _slice_mask)) > + > +#define FOR_EACH_SLICE(_slice, _region, _slice_mask) \ > + for (_slice = FIRST_SLICE(_region, _slice_mask); \ > + _slice < fls(_slice_mask); \ > + _slice = NEXT_SLICE(++(_slice), _region, _slice_mask)) [ ... ] > +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); > + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(to_adreno_gpu(gpu)); > + u32 slice_mask = a6xx_gpu->slice_mask; > + u32 first_slice = a8xx_get_first_slice(a6xx_gpu); > + bool ret = true; > + > + for (int i = 0; i < b->mempool_index_registers_len && ret; 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) { > + u32 pipe_id = cp_indexed_reg->pipe_id; > + > + /* Set CP_CHICKEN_DBG[StabilizeMVC] to stabilize it > while dumping */ > + a8xx_aperture_acquire_raw(gpu, > + GEN8_CP_APERTURE_REG_VAL_NS(first_slice, > pipe_id, 0, 0), > + &flags); > + gpu_rmw(gpu, REG_A8XX_CP_CHICKEN_DBG_PIPE, BIT(2), > BIT(2)); > + a8xx_aperture_release(gpu, flags); > + > + a8xx_aperture_acquire_raw(gpu, > + GEN8_CP_APERTURE_REG_VAL(j, pipe_id, 0, 0), > + &flags); > + gpu_rmw(gpu, REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE, > BIT(2), BIT(2)); > + a8xx_aperture_release(gpu, flags); > + > + ret = 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_raw(gpu, > + GEN8_CP_APERTURE_REG_VAL_NS(first_slice, > pipe_id, 0, 0), > + &flags); > + gpu_rmw(gpu, REG_A8XX_CP_CHICKEN_DBG_PIPE, BIT(2), 0); > + a8xx_aperture_release(gpu, flags); > + > + a8xx_aperture_acquire_raw(gpu, > + GEN8_CP_APERTURE_REG_VAL(j, pipe_id, 0, 0), > + &flags); > + gpu_rmw(gpu, REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE, > BIT(2), 0); > + a8xx_aperture_release(gpu, flags); [Severity: High] Can gen8_snapshot_mempool() access REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE on slice 0 when slice 0 is fused off? In b->mempool_index_registers, the CP_MEM_POOL_DBG_ADDR_PIPE entries have slice_region == UNSLICE, for which FIRST_SLICE() sets j = 0 regardless of slice_mask. Unlike REG_A8XX_CP_CHICKEN_DBG_PIPE (which uses first_slice), the RMWs to the per-slice register REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE use GEN8_CP_APERTURE_REG_VAL(j, pipe_id, 0, 0), performing an AHB access to slice 0 even when it is disabled in slice_mask, which can trigger an AHB or NoC bus error during crashstate capture. Should REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE only be modified when cp_indexed_reg->slice_region == SLICE (or using first_slice for UNSLICE)? [ ... ] > +static void gen8_cx_dbgc_debugbus_select(struct msm_gpu *gpu, u32 block_id, > u32 index) > +{ > + u32 reg; > + > + reg = A7XX_CX_DBGC_CFG_DBGBUS_SEL_A_PING_INDEX(index) | > + A7XX_CX_DBGC_CFG_DBGBUS_SEL_A_PING_BLK_SEL(block_id); [Severity: Low] Should this use A8XX_CX_DBGC_CFG_DBGBUS_SEL_A_PING_INDEX() and A8XX_CX_DBGC_CFG_DBGBUS_SEL_A_PING_BLK_SEL() instead of the A7XX macros? This patch adds the wider A8XX_CX_DBGC domain in a6xx.xml, but leaves it unused here. [ ... ] > + snapshot_sect (state, DEBUG, 4, { > + .type = SNAPSHOT_DEBUG_SLICE_MASK, > + .size = 1, > + }) { > + snapshot_sect_u32(§, a6xx_gpu->slice_mask); > + } > + gen8_snapshot_cx_debugbus(gpu, state); [Severity: Low] Should gen8_snapshot_cx_debugbus() be guarded by snapshot_debugbus, matching gen8_snapshot_debugbus() below and a6xx_get_debugbus() on A6xx/A7xx? > + /* If GX isn't on the rest of the data isn't going to be accessible */ > + if (!adreno_gpu->funcs->gx_is_on(adreno_gpu)) > + return &state->base; > + > + if (snapshot_debugbus) > + gen8_snapshot_debugbus(gpu, state); > + > + /* > + * Try to initialize the crashdumper, if we are not dumping state > + * with the SMMU stalled. The crashdumper needs memory access to > + * write out GPU state, so we need to skip this when the SMMU is > + * stalled in response to an iova fault > + */ > + bool stalled = !!(gpu_read(gpu, REG_A8XX_RBBM_MISC_STATUS) & > + A8XX_RBBM_MISC_STATUS_SMMU_STALLED_ON_FAULT); > + if (!stalled && !gpu->needs_hw_init && > + !a8xx_crashdumper_init(gpu, &_dumper)) { > + dumper = &_dumper; > + } [Severity: Medium] As reported by Alexander Koskovich on v2 [1], can GX enter IFPC after the gx_is_on() check while a8xx_gpu_state_get() is still dumping state? Unlike a6xx_gpu_state_get(), a8xx_gpu_state_get() does not halt SQE via REG_A8XX_CP_SQE_CNTL and holds no GMU OOB or keepalive vote to inhibit IFPC across gen8_snapshot_debugbus(), the 4 MB GEM BO allocation in a8xx_crashdumper_init(), and the subsequent crashdumper and GX register accesses. If in-flight work finishes during this window, the GMU collapses GX into IFPC and enables the AHB fence mid-snapshot, causing GMU fence errors and a crashdumper timeout warning in __cd_run(). [1] https://lore.kernel.org/r/FZvwtKTpIxRNzjzcqFf6bTw12mNTJvJZmg6j9lmi8MehHBt91NogIW8oQd5yFHN5DSL8yIWL8p_b1cJPAJC57ZFkoe2BOAh_Y3kwph5SV_A=@pm.me -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
