Re: [PATCH RFC v2 09/14] drm/msm: initialize LPAC ring

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/msm: adreno_gpu_init() enables LPAC without checking lpac_pdev
- [Medium] drm/msm/a6xx: shadow RPTR size and clear mismatch on preempt init fallback
- [Medium] drm/msm/a6xx: unhalted LPAC SQE in a6xx_recover() and a6xx_gpu_state_get()
- [Medium] drm/msm/a6xx: missing LPAC GX registers in A7xx IFPC/pwrup reglists
--

commit 35ad03732804997c43b27fa187ec0be66e34e69c
Author: Anna Maniscalco <anna.maniscalco2000@gmail.com>

drm/msm: initialize LPAC ring

This commit programs the hardware registers required to start the LPAC ring
and sends the CP initialization sequence. It also updates ringbuffer
management, fence retirement, and scheduler suspend/resume paths to account
for the live LPAC ring.

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> index 3e6f409d13a2a..5ee00eead14c8 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> @@ -1394,6 +1394,7 @@ static const u32 a750_ifpc_reglist_regs[] = {
>  	REG_A6XX_SP_NC_MODE_CNTL,
>  	REG_A6XX_CP_DBG_ECO_CNTL,
>  	REG_A6XX_CP_PROTECT_CNTL,
> +	REG_A6XX_CP_LPAC_PROTECT_CNTL,

[Severity: Medium]
Should the other GX domain registers programmed for LPAC in hw_init() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c also be added to the A7xx IFPC and
power-up restore lists?

Specifically, hw_init() configures these registers when gpu->lpac_rb is
present:

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:hw_init() {
	if (gpu->lpac_rb)
		gpu_rmw(gpu, REG_A6XX_UCHE_DEBUG_CNTL_1, BIT(30), BIT(30));
	...
	if (gpu->lpac_rb) {
		gpu_write(gpu, REG_A7XX_CP_LPAC_CHICKEN_DBG, 0x1);

		gpu_write(gpu, REG_A7XX_SP_CHICKEN_BITS_2, BIT(4));
		gpu_write(gpu, REG_A7XX_SP_LPAC_CHICKEN_BITS_2, BIT(4));
	}
}

On IFPC-capable A7xx targets such as Adreno 750, the GX power domain
collapses across IFPC without re-running hw_init(), and the CP only restores
registers recorded in a750_ifpc_reglist_regs and a7xx_pwrup_reglist_regs by
a7xx_patch_pwrup_reglist(). Because REG_A6XX_UCHE_DEBUG_CNTL_1,
REG_A7XX_CP_LPAC_CHICKEN_DBG, REG_A7XX_SP_CHICKEN_BITS_2, and
REG_A7XX_SP_LPAC_CHICKEN_BITS_2 are not in those lists, won't their LPAC
settings revert to reset defaults after the first IFPC cycle?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 8b3bb2fd433ba..e7bc12de650ca 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c

[ ... ]

> @@ -1096,7 +1162,7 @@ static int a6xx_ucode_load(struct msm_gpu *gpu)
>  	if ((adreno_gpu->base.hw_apriv || a6xx_gpu->has_whereami) &&
>  	    !a6xx_gpu->shadow_bo) {
>  		a6xx_gpu->shadow = msm_gem_kernel_new(gpu->dev,
> -						      sizeof(u32) * gpu->nr_rings,
> +						      sizeof(u32) * (gpu->nr_rings + !!gpu->lpac_rb),
>  						      MSM_BO_WC | MSM_BO_MAP_PRIV,
>  						      gpu->vm, &a6xx_gpu->shadow_bo,
>  						      &a6xx_gpu->shadow_iova);

[ ... ]

> @@ -1451,7 +1533,10 @@ static int hw_init(struct msm_gpu *gpu)
>  	if (a6xx_gpu->shadow_bo) {
>  		gpu_write64(gpu, REG_A6XX_CP_RB_RPTR_ADDR,
>  			shadowptr(a6xx_gpu, gpu->rb[0]));
> -		for (unsigned int i = 0; i < gpu->nr_rings; i++)
> +		if (gpu->lpac_rb)
> +			gpu_write64(gpu, REG_A7XX_CP_LPAC_RB_RPTR_ADDR,
> +				shadowptr(a6xx_gpu, gpu->lpac_rb));
> +		for (unsigned int i = 0; i < (gpu->nr_rings + !!gpu->lpac_rb); i++)
>  			a6xx_gpu->shadow[i] = 0;
>  	}

[Severity: Medium]
Can a6xx_gpu->shadow[] be indexed past gpu->nr_rings + !!gpu->lpac_rb if
a6xx_preempt_init() falls back to a single ring?

During initialization in a6xx_gpu_init() -> adreno_gpu_init() ->
msm_gpu_init(), gpu->lpac_rb is created with ring->id set to nr_rings (4 when
preemption is enabled):

drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_init() {
	if (config->has_lpac) {
		gpu->lpac_rb = msm_ringbuffer_new(gpu, nr_rings, memptrs, memptrs_iova);
}

After msm_gpu_init() returns, a6xx_gpu_init() calls a6xx_preempt_init(),
which falls back to gpu->nr_rings = 1 on allocation failure without updating
gpu->lpac_rb->id:

drivers/gpu/drm/msm/adreno/a6xx_preempt.c:a6xx_preempt_init() {
fail:
	/*
	 * On failure to init preemption, fall back to a single
	 * ring (ie. no preemption)
	 */
	a6xx_preempt_fini(gpu);
	gpu->nr_rings = 1;
}

When adreno_load_gpu() then calls a6xx_ucode_load() and hw_init(),
a6xx_gpu->shadow is sized for only 2 entries (gpu->nr_rings + !!gpu->lpac_rb)
and hw_init() only zeroes indices 0 and 1. However, shadowptr(a6xx_gpu,
gpu->lpac_rb) and a6xx_get_rptr() still index a6xx_gpu->shadow[ring->id] at
index 4:

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a6xx_get_rptr() {
	if (adreno_gpu->base.hw_apriv || a6xx_gpu->has_whereami)
		return a6xx_gpu->shadow[ring->id];
}

On subsequent hw_init() calls during runtime resume or GPU recovery, index 4
is not zeroed and retains its stale RPTR value (8 from the 8-dword CP_ME_INIT
packet in lpac_cp_init()), which causes adreno_idle() to see get_rptr() ==
wptr and return before the LPAC CP has drained CP_ME_INIT.

>  
> @@ -1469,9 +1554,17 @@ static int hw_init(struct msm_gpu *gpu)
>  	for (i = 0; i < gpu->nr_rings; i++)
>  		gpu->rb[i]->cur_ctx_seqno = 0;
>  
> +	if (gpu->lpac_rb)
> +		gpu->lpac_rb->cur_ctx_seqno = 0;
> +
>  	/* Enable the SQE_to start the CP engine */
>  	gpu_write(gpu, REG_A6XX_CP_SQE_CNTL, 1);
>  
> +	/* Enable the LPAC SQE_to start the CP engine */
> +	//TODO is this needed? Doesn't fw do this at init?
> +	if (gpu->lpac_rb)
> +		gpu_write(gpu, REG_A6XX_CP_LPAC_SQE_CNTL, 1);

[Severity: Medium]
Now that REG_A6XX_CP_LPAC_SQE_CNTL is enabled here, should a6xx_recover() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c and a6xx_gpu_state_get() in
drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c also halt the LPAC SQE by writing
3 to REG_A6XX_CP_LPAC_SQE_CNTL when gpu->lpac_rb is present?

Currently, a6xx_recover() and a6xx_gpu_state_get() only halt
REG_A6XX_CP_SQE_CNTL:

drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c:a6xx_gpu_state_get() {
	/* Halt SQE first */
	gpu_write(gpu, REG_A6XX_CP_SQE_CNTL, 3);
}

Leaving the LPAC SQE running while a6xx_gpu_state_get() reads indexed LPAC CP
registers (such as CP_LPAC_DRAW_STATE, CP_LPAC_ROQ_DBG, CP_SQE_AC_UCODE_DBG,
CP_SQE_AC_STAT, and CP_LPAC_FIFO_DBG) and executes the crashdumper, or while
a6xx_recover() prepares for reset, could result in inconsistent crash dump
state or ongoing LPAC CP activity during recovery.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 4389559e1d172..085e1b9044bc7 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c

[ ... ]

> @@ -1253,6 +1261,8 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  
>  	adreno_gpu_config.nr_rings = nr_rings;
>  
> +	adreno_gpu_config.has_lpac = !!(config->info->quirks & ADRENO_QUIRK_LPAC);

[Severity: High]
What happens on a target with ADRENO_QUIRK_LPAC when booting with an older
Device Tree that lacks the qcom,lpac child node?

Earlier in adreno_gpu_init(), gpu->lpac_pdev is only populated if the
qcom,lpac child node exists:

drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_gpu_init() {
	lpac_node = of_get_compatible_child(pdev->dev.of_node, "qcom,lpac");
	if (lpac_node)
		gpu->lpac_pdev = of_find_device_by_node(lpac_node);
}

When gpu->lpac_pdev is NULL, adreno_iommu_create_vm() passes NULL for
lpac_dev to msm_iommu_gpu_new(), so iommu->lpac_domain is never allocated or
attached to SMMU context bank 1.

If adreno_gpu_config.has_lpac is set based solely on ADRENO_QUIRK_LPAC
without checking gpu->lpac_pdev (and adreno_hw_init() likewise calls
qcom_scm_set_gpu_smmu_lpac_aperture(1) based only on ADRENO_QUIRK_LPAC),
msm_gpu_init() will still create gpu->lpac_rb, hw_init() will initialize the
LPAC ring, and msm_submitqueue_create() will allow userspace
MSM_SUBMITQUEUE_LPAC submissions that switch to context bank 1 without an
attached IOMMU domain.

Should adreno_gpu_config.has_lpac require gpu->lpac_pdev != NULL, and should
adreno_hw_init() check gpu->lpac_rb before calling
qcom_scm_set_gpu_smmu_lpac_aperture(1)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-descriptive-name-lpac-upstream-v2-0-a21a5c850961@gmail.com?part=9