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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/msm/a8xx: disabled slice 0 access in
  gen8_snapshot_mempool()
- [Medium] drm/msm/a8xx: swapped mux indices in
  gen8_cx_gc_us_i_0_debugbus_read()
- [Medium] drm/msm/a8xx: missing SQE halt and IFPC inhibition in
  a8xx_gpu_state_get()
--

commit 7342e242e878452594649efb772cccc770f9460e
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/a8xx: devcoredump state capture support

This commit adds devcoredump GPU 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..57c0ae1ef789a
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c
> @@ -0,0 +1,1409 @@
[ ... ]
> +#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);
> +
> +			if (!ret)
> +				break;
> +		}
> +	}

[Severity: High]
Can gen8_snapshot_mempool() access REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE on
slice 0 when slice 0 is fused off in a6xx_gpu->slice_mask?

In b->mempool_index_registers, the first two entries have
cp_indexed_reg->slice_region == UNSLICE.  For UNSLICE entries,
FIRST_SLICE() sets j = 0 regardless of slice_mask, and the loop
unconditionally programs GEN8_CP_APERTURE_REG_VAL(j, pipe_id, 0, 0) and
calls gpu_rmw() on the per-slice register
REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE.  On A8xx GEN2+ targets where slice 0
is disabled in REG_A8XX_CX_MISC_SLICE_ENABLE_FINAL, touching slice 0 over
AHB can trigger an AHB/NoC bus error and crash the system.

Should REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE only be modified when
cp_indexed_reg->slice_region == SLICE (or using first_slice)?

[ ... ]

> +static void gen8_cx_gc_us_i_0_debugbus_read(
> +	struct msm_gpu *gpu, u32 block_id, u32 index, u32 data[2])
> +{
> +	gen8_cx_dbgc_debugbus_select(gpu, block_id, index);
> +
> +	/*
> +	 * Select lower 32 bits of internal buffer data for internal mux[1:0]
> +	 * DBGC_CFG_DBGBUS_IDX.internalIndex0 = 24 (0x18)
> +	 * DBGC_CFG_DBGBUS_IDX.internalIndex1 = 25 (0x19)
> +	 */
> +	cxdbg_write(gpu, REG_A6XX_CX_DBGC_CFG_DBGBUS_IDX, 0x00001918);
> +	udelay(1);
> +
> +	data[0] = cxdbg_read(gpu, REG_A6XX_CX_DBGC_CFG_DBGBUS_TRACE_BUF3);
> +
> +	/*
> +	 * Select upper 32 bits of internal buffer data for internal mux[1:0]
> +	 * DBGC_CFG_DBGBUS_IDX.internalIndex0 = 26 (0x1a)
> +	 * DBGC_CFG_DBGBUS_IDX.internalIndex1 = 27 (0x1b)
> +	 */
> +	cxdbg_write(gpu, REG_A6XX_CX_DBGC_CFG_DBGBUS_IDX, 0x00001a1b);
> +	udelay(1);
> +
> +	data[1] = cxdbg_read(gpu, REG_A6XX_CX_DBGC_CFG_DBGBUS_TRACE_BUF3);
> +}

[Severity: Medium]
Should the second write to REG_A6XX_CX_DBGC_CFG_DBGBUS_IDX be 0x00001b1a
instead of 0x00001a1b?

internalIndex0 is in bits [7:0] and internalIndex1 is in bits [15:8], as
in 0x00001918 above.  Writing 0x00001a1b sets internalIndex0 = 0x1b and
internalIndex1 = 0x1a, swapping the two 16-bit halves of data[1] in the
captured DEBUGBUS_CX_GC_US_I_0 dump.

[ ... ]

> +struct msm_gpu_state *a8xx_gpu_state_get(struct msm_gpu *gpu)
> +{
[ ... ]
> +	/* 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 the GPU enter IFPC after
the single gx_is_on() check while a8xx_gpu_state_get() is still capturing
GX state?

Unlike a6xx_gpu_state_get(), which halts SQE by writing 3 to
REG_A6XX_CP_SQE_CNTL right after checking gx_is_on(), a8xx_gpu_state_get()
neither halts SQE via REG_A8XX_CP_SQE_CNTL nor holds an IFPC-inhibiting
GMU vote across gen8_snapshot_debugbus(), the 4 MB GEM buffer allocation
in a8xx_crashdumper_init(), and the subsequent crashdumper and GX MMIO
reads.  If in-flight work finishes during this window, the GMU powers down
GX and enables the AHB fence, causing GMU fence errors, a crashdumper poll
timeout and drm_WARN_ONCE() in __cd_run(), and an incomplete devcoredump.

[1] https://lore.kernel.org/r/FZvwtKTpIxRNzjzcqFf6bTw12mNTJvJZmg6j9lmi8MehHBt91NogIW8oQd5yFHN5DSL8yIWL8p_b1cJPAJC57ZFkoe2BOAh_Y3kwph5SV_A=@pm.me

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109142111.256630-1-robin.clark@oss.qualcomm.com?part=11