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 <[email protected]> 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-a21a5c850...@gmail.com?part=9
