Re: [PATCH v7 11/15] drm/msm/a8xx: devcoredump state capture support

[sashiko-bot@kernel.org]

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 <robin.clark@oss.qualcomm.com>

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(&sect, 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/20261106232220.47602-1-robin.clark@oss.qualcomm.com?part=11