Re: [PATCH v2 13/21] drm/msm/adreno: Introduce A8x GPU Support

[Akhil P Oommen <akhilpo@oss.qualcomm.com>]

On 11/13/2025 3:45 PM, Konrad Dybcio wrote:
> On 11/10/25 5:37 PM, Akhil P Oommen wrote:
>> A8x is the next generation of Adreno GPUs, featuring a significant
>> hardware design change. A major update to the design is the introduction
>> of Slice architecture. Slices are sort of mini-GPUs within the GPU which
>> are more independent in processing Graphics and compute workloads. Also,
>> in addition to the BV and BR pipe we saw in A7x, CP has more concurrency
>> with additional pipes.
>>
>> From a software interface perspective, these changes have a significant
>> impact on the KMD side. First, the GPU register space has been extensively
>> reorganized. Second, to avoid  a register space explosion caused by the
>> new slice architecture and additional pipes, many registers are now
>> virtualized, instead of duplicated as in A7x. KMD must configure an
>> aperture register with the appropriate slice and pipe ID before accessing
>> these virtualized registers.
>>
>> This patch adds only a skeleton support for the A8x family. An A8x GPU
>> support will be added in an upcoming patch.
>>
>> Signed-off-by: Akhil P Oommen <akhilpo@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> +static void a8xx_aperture_slice_set(struct msm_gpu *gpu, enum adreno_pipe pipe, u32 slice)
>> +{
>> +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> +	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>> +	u32 val;
>> +
>> +	val = A8XX_CP_APERTURE_CNTL_HOST_PIPEID(pipe) | A8XX_CP_APERTURE_CNTL_HOST_SLICEID(slice);
> 
> There's also a BIT(23) value here which is seemingly never set, but
> perhaps may come in useful for the bigger GPU
> 
>> +
>> +	if (a6xx_gpu->cached_aperture == val)
>> +		return;
>> +
>> +	gpu_write(gpu, REG_A8XX_CP_APERTURE_CNTL_HOST, val);
>> +
>> +	a6xx_gpu->cached_aperture = val;
>> +}
>> +
>> +static void a8xx_aperture_aquire(struct msm_gpu *gpu, enum adreno_pipe pipe, unsigned long *flags)
> 
> "acquire"

Oops!

> 
>> +{
>> +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> +	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>> +
>> +	spin_lock_irqsave(&a6xx_gpu->aperture_lock, *flags);
>> +
>> +	a8xx_aperture_slice_set(gpu, pipe, 0);
> 
> Maybe we can add "unsigned long flags[MAX_NUM_SLICES]" to a6xx_gpu
> to make the API a little more ergonomic.. but maybe that's too much
> IDK
> 
> [...]
> 
>> +	a6xx_gpu->slice_mask = a6xx_llc_read(a6xx_gpu,
>> +			REG_A8XX_CX_MISC_SLICE_ENABLE_FINAL) & GENMASK(3, 0);
> 
> Please define that field in the XML
This should be more clear in the next rev

> 
> [...]
> 
>> +}
>> +
>> +static u32 a8xx_get_first_slice(struct a6xx_gpu *a6xx_gpu)
>> +{
>> +	return ffs(a6xx_gpu->slice_mask) - 1;
>> +}
>> +
>> +static inline bool _a8xx_check_idle(struct msm_gpu *gpu)
>> +{
>> +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> +	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>> +
>> +	/* Check that the GMU is idle */
>> +	if (!a6xx_gmu_isidle(&a6xx_gpu->gmu))
>> +		return false;
>> +
>> +	/* Check that the CX master is idle */
>> +	if (gpu_read(gpu, REG_A8XX_RBBM_STATUS) &
>> +			~A8XX_RBBM_STATUS_CP_AHB_BUSY_CX_MASTER)
>> +		return false;
>> +
>> +	return !(gpu_read(gpu, REG_A8XX_RBBM_INT_0_STATUS) &
>> +		A6XX_RBBM_INT_0_MASK_RBBM_HANG_DETECT);
> 
> Odd parenthesis-alignment (couple times in the file), checkpatch
> usually mumbles at that
Not sure about the issue here, but I do run b4 prep --check and didn't
see any complaints.

> 
> [...]
> 
>> +
>> +void a8xx_flush(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
>> +{
>> +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> +	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>> +	uint32_t wptr;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&ring->preempt_lock, flags);
>> +
>> +	/* Copy the shadow to the actual register */
>> +	ring->cur = ring->next;
>> +
>> +	/* Make sure to wrap wptr if we need to */
>> +	wptr = get_wptr(ring);
>> +
>> +	/* Update HW if this is the current ring and we are not in preempt*/
>> +	if (!a6xx_in_preempt(a6xx_gpu)) {
>> +		if (a6xx_gpu->cur_ring == ring)
>> +			gpu_write(gpu, REG_A6XX_CP_RB_WPTR, wptr);
> 
> I think this should use _fenced too, but I guess the preempt detail
> is just a harmless copypasta
I have reused a6xx_flush here in next rev.

> 
> [...]
> 
>> +static void a8xx_set_hwcg(struct msm_gpu *gpu, bool state)
>> +{
>> +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> +	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>> +	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>> +	u32 val;
>> +
>> +	gmu_write(gmu, REG_A6XX_GPU_GMU_AO_GMU_CGC_MODE_CNTL,
>> +			state ? adreno_gpu->info->a6xx->gmu_cgc_mode : 0);
>> +	gmu_write(gmu, REG_A6XX_GPU_GMU_AO_GMU_CGC_DELAY_CNTL,
>> +			state ? 0x110111 : 0);
> 
> a840 sets this, a830 sets 0x10111, please confirm which way x2 skews
This is correct for X2.

> 
>> +	gmu_write(gmu, REG_A6XX_GPU_GMU_AO_GMU_CGC_HYST_CNTL,
>> +			state ? 0x55555 : 0);
>> +
>> +	gpu_write(gpu, REG_A8XX_RBBM_CLOCK_CNTL_GLOBAL, 1);
>> +	gpu_write(gpu, REG_A8XX_RBBM_CGC_GLOBAL_LOAD_CMD, state ? 1 : 0);
> 
> !!state
> 
> [...]
> 
>> +static void a8xx_nonctxt_config(struct msm_gpu *gpu, u32 *gmem_protect)
>> +{
>> +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> +	const struct a6xx_info *info = adreno_gpu->info->a6xx;
>> +	const struct adreno_reglist_pipe *regs = info->nonctxt_reglist;
>> +	unsigned int pipe_id, i;
>> +	unsigned long flags;
>> +
>> +	for (pipe_id = PIPE_NONE; pipe_id <= PIPE_DDE_BV; pipe_id++) {
>> +		/* We don't have support for LPAC yet */
>> +		if (pipe_id == PIPE_LPAC)
>> +			continue;
> 
> This seems arbitrary - one because there are no defines targetting PIPE_LPAC
> specifcally in the reg lists you shared and two because it would almost
> certainly not hurt to configure these registers and otherwise not power up
> the LPAC pipeline
> 
>> +
>> +		a8xx_aperture_aquire(gpu, pipe_id, &flags);
>> +
>> +		for (i = 0; regs[i].offset; i++) {
>> +			if (!(BIT(pipe_id) & regs[i].pipe))
>> +				continue;
>> +
>> +			if (regs[i].offset == REG_A8XX_RB_GC_GMEM_PROTECT)
>> +				*gmem_protect = regs[i].value;
>> +
>> +			gpu_write(gpu, regs[i].offset, regs[i].value);
>> +		}
>> +
>> +		a8xx_aperture_release(gpu, flags);
>> +	}
>> +
>> +	a8xx_aperture_clear(gpu);
>> +}
>> +
>> +static int a8xx_cp_init(struct msm_gpu *gpu)
>> +{
>> +	struct msm_ringbuffer *ring = gpu->rb[0];
>> +	u32 mask;
>> +
>> +	/* Disable concurrent binning before sending CP init */
>> +	OUT_PKT7(ring, CP_THREAD_CONTROL, 1);
>> +	OUT_RING(ring, BIT(27));
>> +
>> +	OUT_PKT7(ring, CP_ME_INIT, 4);
>> +
>> +	/* Use multiple HW contexts */
>> +	mask = BIT(0);
>> +
>> +	/* Enable error detection */
>> +	mask |= BIT(1);
>> +
>> +	/* Set default reset state */
>> +	mask |= BIT(3);
>> +
>> +	/* Disable save/restore of performance counters across preemption */
>> +	mask |= BIT(6);
>> +
>> +	OUT_RING(ring, mask);
>> +
>> +	/* Enable multiple hardware contexts */
>> +	OUT_RING(ring, 0x00000003);
>> +
>> +	/* Enable error detection */
>> +	OUT_RING(ring, 0x20000000);
>> +
>> +	/* Operation mode mask */
>> +	OUT_RING(ring, 0x00000002);
> 
> Should we include the pwrup reglist from the get-go too? I don't think
> you used the ones you declared in patch 15 (or at least my ctrl-f can't
> find the use of it)

yeah. I should remove that for now.

> 
> [...]
> 
>> +#define A8XX_CP_INTERRUPT_STATUS_MASK_PIPE \
>> +	(A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_CSFRBWRAP | \
>> +	 A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_CSFIB1WRAP | \
>> +	 A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_CSFIB2WRAP | \
>> +	 A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_CSFIB3WRAP | \
>> +	 A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_CSFSDSWRAP | \
>> +	 A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_CSFMRBWRAP | \
>> +	 A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_CSFVSDWRAP | \
>> +	 A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_OPCODEERROR | \
>> +	 A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_VSDPARITYERROR | \
>> +	 A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_REGISTERPROTECTIONERROR | \
>> +	 A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_ILLEGALINSTRUCTION | \
>> +	 A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_SMMUFAULT | \
>> +	 A8XX_CP_INTERRUPT_STATUS_MASK_PIPE_VBIFRESP | \
> 
> kgsl also enables VBIFRESTP(TYPE/READ/LIENT)

Ack

> 
> [...]
> 
>> +	/* Setup GMEM Range in UCHE */
>> +	gmem_range_min = SZ_64M;
> 
> this doesn't seem to ever change, you can inline it

fwiw, this documents the magic number. ;)

-Akhil

> 
> [...]
> 
>> +static void a8xx_dump(struct msm_gpu *gpu)
>> +{
>> +	DRM_DEV_INFO(&gpu->pdev->dev, "status:   %08x\n",
>> +			gpu_read(gpu, REG_A8XX_RBBM_STATUS));
> 
> This can be a single line
> 
> Konrad