Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] drm/msm: LPAC enabled on gen70900 without checking lpac_pdev or SW fuse - [High] drm/msm: a6xx_active_ring() and suspend ignore lpac_rb when LPAC is enabled - [High] drm/msm: wrong runtime PM device checked for LPAC in msm_iommu_tlb_flush_all() - [High] drm/msm: failed parent-to-child device link skips LPAC SMMU TLB flushes - [Medium] drm/msm: LPAC not enabled on gen70900 due to enable_lpac defaulting to 0 - [Medium] drm/msm: mismatched always-on counter reads for LPAC in a7xx_submit() - [Medium] drm/msm/adreno: missing LPAC GX registers in A750 IFPC restore lists --
commit 80cc6b84cce3401809bd7592d96a060ab6cb7da3 Author: Anna Maniscalco <[email protected]> drm/msm: enable LPAC on gen70900 This commit adds ADRENO_QUIRK_LPAC to the gen70900 (Adreno 750) GPU entry in a7xx_gpus[] so that LPAC is enabled on targets known to support it. > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > index 5ee00eead14c8..cd1bdb54429d3 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > @@ -1591,7 +1591,8 @@ static const struct adreno_info a7xx_gpus[] = { > .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT | > ADRENO_QUIRK_HAS_HW_APRIV | > ADRENO_QUIRK_PREEMPTION | > - ADRENO_QUIRK_IFPC, > + ADRENO_QUIRK_IFPC | > + ADRENO_QUIRK_LPAC, [Severity: Medium] Does setting ADRENO_QUIRK_LPAC here actually enable LPAC by default on gen70900 when enable_lpac is not explicitly passed by the user? In drivers/gpu/drm/msm/adreno/adreno_device.c, enable_lpac is initialized to 0 instead of -1: int enable_lpac = 0; MODULE_PARM_DESC(enable_lpac, "Enable LPAC (A7xx+ only) (1=on , 0=disable, -1=auto (default))"); module_param(enable_lpac, int, 0600); When adreno_gpu_init() in drivers/gpu/drm/msm/adreno/adreno_gpu.c sets: adreno_gpu_config.has_lpac = !!(config->info->quirks & ADRENO_QUIRK_LPAC); msm_gpu_init() in drivers/gpu/drm/msm/msm_gpu.c allocates an extra msm_rbmemptrs slot for config->has_lpac, but then skips creating gpu->lpac_rb because enable_lpac is 0: if (config->has_lpac && enable_lpac != 0) { gpu->lpac_rb = msm_ringbuffer_new(gpu, nr_rings, memptrs, memptrs_iova); Meanwhile, adreno_hw_init() in drivers/gpu/drm/msm/adreno/adreno_gpu.c only checks the quirk rather than gpu->lpac_rb: if (info->quirks & ADRENO_QUIRK_LPAC) { ret = qcom_scm_set_gpu_smmu_lpac_aperture(1); so qcom_scm_set_gpu_smmu_lpac_aperture(1) is still called during hardware initialization even when LPAC is disabled. Additionally, because config->has_lpac only checks ADRENO_QUIRK_LPAC, passing enable_lpac=1 cannot force-enable LPAC on A7xx+ targets that do not set the quirk. Should enable_lpac be initialized to -1, and should adreno_hw_init() check gpu->lpac_rb instead of ADRENO_QUIRK_LPAC? [Severity: High] What happens when booting a kernel with ADRENO_QUIRK_LPAC enabled on an older SM8650 (gen70900) device tree that lacks the qcom,lpac child node, or on a device where firmware fuses off LPAC? In adreno_gpu_init() in drivers/gpu/drm/msm/adreno/adreno_gpu.c, gpu->lpac_pdev is looked up via of_get_compatible_child(pdev->dev.of_node, "qcom,lpac"). On an older DTB without that child node, gpu->lpac_pdev is NULL, and adreno_iommu_create_vm() passes NULL for lpac_dev: mmu = msm_iommu_gpu_new(&pdev->dev, gpu->lpac_pdev ? &gpu->lpac_pdev->dev : NULL, gpu, quirks); so iommu->lpac_domain (context bank 1) is never allocated and SID 1 remains attached to context bank 0. However, adreno_gpu_init() sets adreno_gpu_config.has_lpac solely from ADRENO_QUIRK_LPAC without checking gpu->lpac_pdev, and adreno_hw_init() unconditionally calls qcom_scm_set_gpu_smmu_lpac_aperture(1). When gpu->lpac_rb is created and userspace submits to MSM_SUBMITQUEUE_LPAC, a7xx_submit() -> a6xx_set_pagetable() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c executes: /* qcom-arm-smmu always maps asid and ctxbank 1:1 */ ctxbank = asid = is_lpac; and emits CP_SMMU_TABLE_UPDATE targeting uninitialized context bank 1 while SID 1 still translates through context bank 0. Similarly, in a6xx_gmu_secure_init() in drivers/gpu/drm/msm/adreno/a6xx_gmu.c, REG_A7XX_CX_MISC_SW_FUSE_VALUE is read after qcom_scm_gpu_init_regs() to check A7XX_CX_MISC_SW_FUSE_VALUE_RAYTRACING, but A7XX_CX_MISC_SW_FUSE_VALUE_LPAC is not checked before hw_init() writes REG_A6XX_CP_LPAC_SQE_CNTL and runs lpac_cp_init(), which can trigger a7xx_sw_fuse_violation_irq() if firmware disables the LPAC software fuse. Should LPAC enablement verify both that gpu->lpac_pdev is present and that A7XX_CX_MISC_SW_FUSE_VALUE_LPAC is set? [Severity: High] With ADRENO_QUIRK_LPAC enabled on gen70900 and gpu->lpac_rb exposed to userspace, how do hangcheck, fault recovery, and suspend handle the LPAC ring? In drivers/gpu/drm/msm/adreno/a6xx_gpu.c, a6xx_active_ring() always returns a6xx_gpu->cur_ring (one of gpu->rb[0..gpu->nr_rings - 1]) and never returns gpu->lpac_rb: static struct msm_ringbuffer *a6xx_active_ring(struct msm_gpu *gpu) { struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); return a6xx_gpu->cur_ring; } In drivers/gpu/drm/msm/msm_gpu.c, hangcheck_handler() only checks gpu->funcs->active_ring(gpu) for fence progress, so a hung submission on gpu->lpac_rb while the GFX rings are idle will not be detected by software hangcheck. Furthermore, if hardware fault detection (a6xx_fault_detect_irq()) or a7xx_sw_fuse_violation_irq() triggers on an LPAC job, a6xx_fault_detect_irq() deletes gpu->hangcheck_timer, masks all GPU interrupts by writing 0 to REG_A6XX_RBBM_INT_0_MASK, and queues gpu->recover_work. In recover_worker() in drivers/gpu/drm/msm/msm_gpu.c: struct msm_ringbuffer *cur_ring = gpu->funcs->active_ring(gpu); ... submit = find_submit(cur_ring, cur_ring->memptrs->fence + 1); if (!submit) goto out_unlock; If cur_ring is idle when LPAC faults, recover_worker() exits to out_unlock without calling gpu->funcs->recover(gpu) or restoring REG_A6XX_RBBM_INT_0_MASK, leaving GPU interrupts masked and the GPU wedged. If cur_ring has a concurrent GFX submission, recover_worker() blames that GFX submit, marks its VM unusable, advances cur_ring->memptrs->fence while leaving gpu->lpac_rb->memptrs->fence unadvanced: for (i = 0; i < gpu->nr_rings + !!gpu->lpac_rb; i++) { struct msm_ringbuffer *ring = i < gpu->nr_rings ? gpu->rb[i] : gpu->lpac_rb; uint32_t fence = ring->memptrs->fence; if (ring == cur_ring) ring->memptrs->fence = ++fence; msm_update_fence(ring->fctx, fence); } and then replays the hung LPAC submission. Also, in a6xx_gmu_pm_suspend() and a6xx_pm_suspend() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c, the loop clearing a6xx_gpu->shadow[] only iterates up to gpu->nr_rings: if (a6xx_gpu->shadow_bo) for (i = 0; i < gpu->nr_rings; i++) a6xx_gpu->shadow[i] = 0; leaving a6xx_gpu->shadow[gpu->nr_rings] for gpu->lpac_rb stale across suspend. Could hangcheck_handler(), recover_worker(), and the suspend shadow-clearing loops be updated to account for gpu->lpac_rb? [Severity: High] When LPAC submissions on gen70900 switch context bank 1 (ASID 1) to per-process TTBR0 pagetables via a6xx_set_pagetable(), will TLB invalidations on context bank 1 be skipped when those pagetables are unmapped or destroyed? In msm_iommu_tlb_flush_all() and msm_iommu_tlb_flush_walk() in drivers/gpu/drm/msm/msm_iommu.c, the primary context bank flushes check pm_runtime_get_if_in_use(pagetable->iommu_dev), whereas the LPAC flushes check pagetable->parent->lpac_dev: if (pagetable->parent->lpac_dev && pm_runtime_get_if_in_use(pagetable->parent->lpac_dev)) { adreno_smmu = dev_get_drvdata(pagetable->parent->lpac_dev); pagetable->tlb->tlb_flush_all((void *)adreno_smmu->cookie); pm_runtime_put_autosuspend(pagetable->parent->lpac_dev); } However, in adreno_gpu_init() in drivers/gpu/drm/msm/adreno/adreno_gpu.c: if (gpu->lpac_pdev) { pm_runtime_enable(&gpu->lpac_pdev->dev); if (!device_link_add(&pdev->dev, &gpu->lpac_pdev->dev, DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER)) dev_err(&gpu->lpac_pdev->dev, "failed to link to lpac device\n"); } Because gpu->lpac_pdev is populated from the child node under the GPU node in arch/arm64/boot/dts/qcom/sm8650.dtsi via of_platform_populate(..., &pdev->dev), &pdev->dev is the parent of &gpu->lpac_pdev->dev. Calling device_link_add() with the parent (&pdev->dev) as consumer and its child (&gpu->lpac_pdev->dev) as supplier fails in device_is_dependent() because device_is_ancestor(consumer, supplier) is true. As a result, lpac_dev has no runtime PM link, no bound driver, and no autosuspend configuration, so it remains permanently in RPM_SUSPENDED with usage_count == 0. Even if lpac_dev were suspended while pagetable->iommu_dev is active, the SMMU hardware still retains cached TLB entries for context bank 1. Because pm_runtime_get_if_in_use(pagetable->parent->lpac_dev) always returns 0, tlb_flush_all() and tlb_flush_walk() are never called for context bank 1 before page table pages are freed in msm_iommu_pagetable_destroy() or __arm_lpae_unmap(). Should msm_iommu_tlb_flush_all() and msm_iommu_tlb_flush_walk() check pagetable->iommu_dev (or shouldn't the runtime PM handling for lpac_dev be fixed so context bank 1 TLBs are invalidated before freeing page tables)? [Severity: Medium] When LPAC is enabled on gen70900 alongside ADRENO_QUIRK_IFPC, are the LPAC GX registers restored after an Inter-Frame Power Collapse (IFPC) cycle? During hw_init() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c, when gpu->lpac_rb is present, the driver programs several GX-domain registers: 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)); } In drivers/gpu/drm/msm/adreno/a6xx_catalog.c, a750_ifpc_reglist_regs includes REG_A6XX_CP_LPAC_PROTECT_CNTL, but 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 absent from both a750_ifpc_reglist_regs and a7xx_pwrup_reglist_regs, so their values will be lost when the GX domain collapses during IFPC. Should those four registers be added to the A750 IFPC / power-up register lists? > .funcs = &a7xx_gpu_funcs, [Severity: Medium] When gen70900 uses a7xx_gpu_funcs and submits LPAC jobs through a7xx_submit() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c, does alwayson_end read from the wrong counter for LPAC? At the start of a7xx_submit(), alwayson_start is sampled from cp_always_on_counter when is_lpac is true: if (is_lpac) get_stats_counter(ring, cp_always_on_counter, rbmemptr_stats(ring, index, alwayson_start)); else get_stats_counter(ring, cp_always_on_context, rbmemptr_stats(ring, index, alwayson_start)); However, at the end of a7xx_submit(), alwayson_end is unconditionally sampled from cp_always_on_context: get_stats_counter(ring, rbbm_perfctr_cp0, rbmemptr_stats(ring, index, cpcycles_end)); get_stats_counter(ring, cp_always_on_context, rbmemptr_stats(ring, index, alwayson_end)); When retire_submit() in drivers/gpu/drm/msm/msm_gpu.c computes: elapsed = (stats->alwayson_end - stats->alwayson_start) * 10000; it subtracts CP_ALWAYS_ON_COUNTER from CP_ALWAYS_ON_CONTEXT, corrupting the elapsed time, clock estimation, trace_msm_gpu_submit_retired(), and ctx->elapsed_ns for LPAC submissions (note also that a7xx_submit() has an unused local variable timestamp_iova). Should the alwayson_end sample in a7xx_submit() also branch on is_lpac to read cp_always_on_counter? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261029-descriptive-name-lpac-upstream-v2-0-a21a5c850...@gmail.com?part=14
