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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/msm/a8xx: disabled slice 0 access in gen8_snapshot_mempool()
- [Medium] drm/msm/a8xx: 9-bit PING_BLK_SEL mask truncates Slice 3 debugbus IDs
- [Medium] drm/msm/a8xx: un-cleared CP_APERTURE_CNTL_HOST in a8xx_gpu_state_get()
- [Medium] drm/msm/a8xx: snapshot_debugbus ignored in a8xx_gpu_state_get()
- [Low] drm/msm/a8xx: missing linux/bitfield.h include in adreno_snapshot.h and a8xx_gpu_state.c
--

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

drm/msm/a8xx: devcoredump state capture support

This commit adds GPU state capture and devcoredump support for Adreno A8xx
GPUs, including register, indexed register, mempool, MVC cluster, shader,
debugbus, and GMU state dumping.

> 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..0692e21da59c8
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c
> @@ -0,0 +1,1403 @@

[ ... ]

> +#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 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) {
> +			u32 val = GEN8_CP_APERTURE_REG_VAL(slice_id, 0, 0, 0);
> +			a8xx_aperture_acquire_raw(gpu, val, &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);
> +	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]
As reported by Sashiko on v5 [1], on A8xx GEN2+ GPUs with partial slice
configurations where slice 0 is disabled in a6xx_gpu->slice_mask, can
gen8_snapshot_mempool() access a fused-off slice and trigger an AHB/NoC bus
error?

In b->mempool_index_registers, the first two entries have slice_region set to
UNSLICE. For UNSLICE entries, FIRST_SLICE(UNSLICE, slice_mask) sets j = 0
regardless of a6xx_gpu->slice_mask.

While the REG_A8XX_CP_CHICKEN_DBG_PIPE update uses first_slice, the
subsequent update to the per-slice register REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE
uses GEN8_CP_APERTURE_REG_VAL(j, pipe_id, 0, 0), which selects slice 0 when
j = 0 even if slice 0 is not present in slice_mask.

Should FIRST_SLICE() start at next_slice(0, _slice_mask), or should
REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE only be programmed when
cp_indexed_reg->slice_region == SLICE?

[ ... ]

> +static void gen8_dbgc_debugbus_select(struct msm_gpu *gpu, u32 block_id, u32 index)
> +{
> +	u32 reg;
> +
> +	reg = A7XX_DBGC_CFG_DBGBUS_SEL_D_PING_INDEX(index) |
> +		A7XX_DBGC_CFG_DBGBUS_SEL_D_PING_BLK_SEL(block_id);

[Severity: Medium]
Does A7XX_DBGC_CFG_DBGBUS_SEL_D_PING_BLK_SEL(block_id) truncate Slice 3
debugbus block IDs on 4-slice A8xx GPUs such as Glymur?

In drivers/gpu/drm/msm/adreno/adreno_gen8_snapshot.h and
drivers/gpu/drm/msm/adreno/catalog/adreno_glymur.h, the Slice 3 debugbus
blocks (DEBUGBUS_CP_GC_S_3_I_0 through DEBUGBUS_VFD_GC_S_3_I_1) have block
IDs 512 through 593, which require 10 bits.

However, DBGC_CFG_DBGBUS_SEL_D in drivers/gpu/drm/msm/registers/adreno/a6xx.xml
defines PING_BLK_SEL as bits 24:16 (9 bits, max 511):

	<reg32 offset="0x0603" name="DBGC_CFG_DBGBUS_SEL_D" variants="A7XX-">
		<bitfield high="7" low="0" name="PING_INDEX"/>
		<bitfield high="24" low="16" name="PING_BLK_SEL"/>
	</reg32>

When block_id is 512..593, A7XX_DBGC_CFG_DBGBUS_SEL_D_PING_BLK_SEL(block_id)
masks off bit 9 and selects unslice debugbus blocks 0..81 instead.

[ ... ]

> +	gen8_snapshot_cx_debugbus(gpu, state);
> +
> +	/* 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;
> +
> +	gen8_snapshot_debugbus(gpu, state);

[Severity: Medium]
Should gen8_snapshot_cx_debugbus() and gen8_snapshot_debugbus() in
a8xx_gpu_state_get() be guarded by the snapshot_debugbus module parameter,
similar to a6xx_gpu_state_get()?

Currently these run unconditionally even when snapshot_debugbus is false by
default, which includes debugbus data in devcoredumps without opt-in and adds
over 100 ms of udelay(1) waits while holding gpu->lock (including during SMMU
stall-on-fault page fault captures).

[ ... ]

> +out:
> +	gpu_write(gpu, REG_A8XX_RBBM_SNAPSHOT_STATUS, 0x0);
> +
> +	if (dumper)
> +		msm_gem_kernel_put(dumper->bo, gpu->vm);
> +
> +	state->gpu_initialized = !gpu->needs_hw_init;
> +
> +	return  &state->base;
> +}

[Severity: Medium]
As reported by Sashiko on v5 [1], should a8xx_gpu_state_get() call
a8xx_aperture_clear(gpu) at the out label before returning?

When dumper is NULL (such as during SMMU stall-on-fault devcoredump capture
via adreno_fault_handler() -> msm_gpu_fault_crashstate_capture(), where the
GPU continues running without a reset) or when an earlier step jumps to out
before gen8_snapshot_mempool() clears the aperture, gen8_snapshot_mvc_regs(),
gen8_snapshot_cp_indexed_regs(), and gen8_legacy_snapshot_registers() leave
REG_A8XX_CP_APERTURE_CNTL_HOST and a6xx_gpu->cached_aperture programmed with
A8XX_CP_APERTURE_CNTL_HOST_USESLICEID and non-zero slice, pipe, cluster, and
context IDs.

Leaving A8XX_CP_APERTURE_CNTL_HOST_USESLICEID set causes subsequent host
writes that expect broadcast mode, such as
REG_A8XX_RBBM_SLICE_PERFCTR_FLUSH_HOST_CMD in a8xx_perfcntr_flush(), to only
reach a single slice, and leaves a6xx_gpu->cached_aperture out of sync with
the hardware reset value across IFPC.

Also, in gen8_legacy_snapshot_registers(), regs->sel->host_reg is written
before a8xx_aperture_acquire_raw() acquires a6xx_gpu->aperture_lock and
selects the target slice. Should the regs->sel->host_reg write happen after
acquiring the aperture, matching gen8_legacy_snapshot_mvc() and the
crashdumper path in gen8_reglist_snapshot()?

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_snapshot.h b/drivers/gpu/drm/msm/adreno/adreno_snapshot.h
> index f9956e0bc1d5f..1c56be7249f76 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_U64(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_U64(63, 44), reg) | dwords;
> +	p->out += sizeof(u32) * dwords;
> +	__cd_check_overflow(p);
> +}

[Severity: Low]
As reported by kernel test robot on v5 [2], adreno_snapshot.h and
a8xx_gpu_state.c use FIELD_PREP() and FIELD_GET() (in CD_WRITE(), CD_READ(),
gen8_dbgc_side_debug_bus_read(), gen8_cx_side_debug_bus_read(), and
a8xx_gpu_state_get()) without including linux/bitfield.h.

Can this cause implicit function declaration build failures on architectures
where linux/bitfield.h is not pulled in transitively?

[1] https://lore.kernel.org/r/sashiko-outbox-194286@kernel.org
[2] https://lore.kernel.org/r/202611040652.1Wy2glVj-lkp@intel.com

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