After resume from S4 (hibernation) on gmc_v9 parts with GFXOFF (observed on Cezanne / Ryzen 7 PRO 5850U, kernel 7.1.8), KIQ-based TLB flushes start failing at the moment of the thaw and keep failing for hours of normal desktop use:
amdgpu 0000:07:00.0: failed to write reg 28b4 wait reg 28c6 amdgpu 0000:07:00.0: failed to write reg 1a6f4 wait reg 1a706 (80-140 errors/hour measured over 9+ hours; bugzilla 219492). The KIQ ring stays sched.ready throughout, so readiness does not reflect the state of the hardware in this failure mode. Two problems follow from the current code: every failed flush burns the full ~5 s KIQ retry window before erroring out (desktop-wide sluggishness), and the invalidation is then silently dropped (stale TLBs - correctness). Make the failure observable and self-healing: - amdgpu_gmc_fw_reg_write_reg_wait() returns 0/-ETIME, propagates MES errors, and counts consecutive failures per KIQ instance - gmc_v9_0_flush_gpu_tlb() falls back to a new gmc_v9_0_flush_gpu_tlb_mmio() helper (the former pre-KIQ MMIO code, now with irqsave locking since it is reachable at runtime) when the KIQ submit fails, so the invalidation is no longer dropped - after AMDGPU_KIQ_FLUSH_MAX_FAIL (3) consecutive failures the KIQ path is skipped entirely until the counter resets, so wedged systems stop paying the 5 s retry window per flush - the MMIO fallback is restricted to process context on bare metal: amdgpu_gfx_off_ctrl() is used to hold the GC block awake across the direct register access (it may sleep), and SR-IOV VFs keep proxying through KIQ as before - the counter resets on every success and in gmc_v9_0_hw_fini(); every suspend/resume cycle re-arms the KIQ path; nothing is disabled proactively Changes since v1 (addressing the sashiko-bot review): - hold GFXOFF off around runtime MMIO access (was: unguarded - could hit power-gated registers) - per-KIQ-instance failure counter instead of a global one (multi-XCC cross-talk) - invalidate_lock is now taken irqsave (the path is runtime-reachable) - VFs and interrupt context never take the runtime MMIO fallback - MES path errors are propagated instead of hardcoded success - reworded the threshold warning to not promise a fallback that gmc_v10/v11/v12 do not implement Known limitation / open question: a KIQ command that already timed out stays queued in the ring; if the ring recovers late it could emit a duplicate invalidation concurrently with a CPU MMIO flush. The command is semantically an idempotent invalidate, but reviewer input on whether the req/ack handshake needs protection here is welcome. gmc_v10/v11/v12 call sites are unchanged and compile-safe (int return used as statement). They can get the same fallback once the approach is agreed for gmc_v9. The sibling PASID path (amdgpu_gmc_flush_gpu_tlb_pasid) already has an -ETIME/MMIO split; this brings the per-VMID path in line with it. Root-cause note: with GFXOFF held off across the S4 cycle (debugfs amdgpu_gfxoff), zero errors occur across resume and 30 min of use vs ~70-140 in the control arm; the wedge forms in the S4 resume window while GFXOFF is allowed, consistent with the existing semaphore workaround comment in gmc_v9_0.c. Signed-off-by: Denis Pisarev <[email protected]> --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 + drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 2 + drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 18 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h | 2 +- drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 111 ++++++++++++++++++------ 5 files changed, 102 insertions(+), 33 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 7b09410d6..cd5d9e56e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -360,6 +360,8 @@ enum amdgpu_kiq_irq { #define MAX_KIQ_REG_WAIT 5000 /* in usecs, 5ms */ #define MAX_KIQ_REG_BAILOUT_INTERVAL 5 /* in msecs, 5ms */ #define MAX_KIQ_REG_TRY 1000 +/* consecutive KIQ TLB flush failures before falling back to MMIO */ +#define AMDGPU_KIQ_FLUSH_MAX_FAIL 3 /* * BIOS. diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h index 54c1eb9c4..e2aceb99c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h @@ -167,6 +167,8 @@ struct amdgpu_kiq { struct amdgpu_irq_src irq; const struct kiq_pm4_funcs *pmf; void *mqd_backup; + /* consecutive TLB flush reg access failures on this instance */ + atomic_t flush_failures; }; /* diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c index 5d6149ba7..49d3d6651 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c @@ -874,7 +874,7 @@ int amdgpu_gmc_flush_gpu_tlb_pasid(struct amdgpu_device *adev, uint16_t pasid, return r; } -void amdgpu_gmc_fw_reg_write_reg_wait(struct amdgpu_device *adev, +int amdgpu_gmc_fw_reg_write_reg_wait(struct amdgpu_device *adev, uint32_t reg0, uint32_t reg1, uint32_t ref, uint32_t mask, uint32_t xcc_inst) @@ -886,9 +886,8 @@ void amdgpu_gmc_fw_reg_write_reg_wait(struct amdgpu_device *adev, uint32_t seq; if (adev->mes.ring[MES_PIPE_INST(xcc_inst, 0)].sched.ready) { - amdgpu_mes_reg_write_reg_wait(adev, reg0, reg1, - ref, mask, xcc_inst); - return; + return amdgpu_mes_reg_write_reg_wait(adev, reg0, reg1, + ref, mask, xcc_inst); } spin_lock_irqsave(&kiq->ring_lock, flags); @@ -919,13 +918,20 @@ void amdgpu_gmc_fw_reg_write_reg_wait(struct amdgpu_device *adev, if (cnt > MAX_KIQ_REG_TRY) goto failed_kiq; - return; + atomic_set(&adev->gfx.kiq[xcc_inst].flush_failures, 0); + return 0; failed_undo: amdgpu_ring_undo(ring); spin_unlock_irqrestore(&kiq->ring_lock, flags); failed_kiq: - dev_err(adev->dev, "failed to write reg %x wait reg %x\n", reg0, reg1); + if (atomic_inc_return(&adev->gfx.kiq[xcc_inst].flush_failures) == + AMDGPU_KIQ_FLUSH_MAX_FAIL) + dev_warn(adev->dev, + "KIQ reg access keeps failing, MMIO fallback recommended\n"); + dev_err_ratelimited(adev->dev, + "failed to write reg %x wait reg %x\n", reg0, reg1); + return -ETIME; } /** diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h index ddb0d500e..303e0ee98 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h @@ -447,7 +447,7 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid, int amdgpu_gmc_flush_gpu_tlb_pasid(struct amdgpu_device *adev, uint16_t pasid, uint32_t flush_type, bool all_hub, uint32_t inst); -void amdgpu_gmc_fw_reg_write_reg_wait(struct amdgpu_device *adev, +int amdgpu_gmc_fw_reg_write_reg_wait(struct amdgpu_device *adev, uint32_t reg0, uint32_t reg1, uint32_t ref, uint32_t mask, uint32_t xcc_inst); diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c index 8a5c44810..11fc9085e 100644 --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c @@ -764,27 +764,28 @@ static bool gmc_v9_0_get_atc_vmid_pasid_mapping_info(struct amdgpu_device *adev, */ /** - * gmc_v9_0_flush_gpu_tlb - tlb flush with certain type + * gmc_v9_0_flush_gpu_tlb_mmio - tlb flush via direct MMIO * * @adev: amdgpu_device pointer + * @hub: vmhub to flush * @vmid: vm instance to flush * @vmhub: which hub to flush - * @flush_type: the flush type + * @inv_req: invalidation request payload * - * Flush the TLB for the requested page table using certain type. + * Direct CPU access to the invalidation engine. Callers must ensure + * the target block cannot power gate across the access (GFXOFF needs + * to be held off at runtime) and must hold no other locks. */ -static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid, - uint32_t vmhub, uint32_t flush_type) +static void gmc_v9_0_flush_gpu_tlb_mmio(struct amdgpu_device *adev, + struct amdgpu_vmhub *hub, + uint32_t vmid, uint32_t vmhub, + u32 inv_req) { bool use_semaphore = gmc_v9_0_use_invalidate_semaphore(adev, vmhub); - u32 j, inv_req, tmp, sem, req, ack, inst; const unsigned int eng = 17; - struct amdgpu_vmhub *hub; - - BUG_ON(vmhub >= AMDGPU_MAX_VMHUBS); + unsigned long flags; + u32 j, tmp, sem, req, ack, inst; - hub = &adev->vmhub[vmhub]; - inv_req = gmc_v9_0_get_invalidate_req(vmid, flush_type); sem = hub->vm_inv_eng0_sem + hub->eng_distance * eng; req = hub->vm_inv_eng0_req + hub->eng_distance * eng; ack = hub->vm_inv_eng0_ack + hub->eng_distance * eng; @@ -794,21 +795,8 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid, else inst = vmhub; - /* This is necessary for SRIOV as well as for GFXOFF to function - * properly under bare metal - */ - if (adev->gfx.kiq[inst].ring.sched.ready && - (amdgpu_sriov_runtime(adev) || !amdgpu_sriov_vf(adev))) { - uint32_t req = hub->vm_inv_eng0_req + hub->eng_distance * eng; - uint32_t ack = hub->vm_inv_eng0_ack + hub->eng_distance * eng; - - amdgpu_gmc_fw_reg_write_reg_wait(adev, req, ack, inv_req, - 1 << vmid, inst); - return; - } - /* This path is needed before KIQ/MES/GFXOFF are set up */ - spin_lock(&adev->gmc.invalidate_lock); + spin_lock_irqsave(&adev->gmc.invalidate_lock, flags); /* * It may lose gpuvm invalidate acknowldege state across power-gating @@ -870,7 +858,7 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid, WREG32_SOC15_IP_NO_KIQ(GC, sem, 0, GET_INST(GC, inst)); } - spin_unlock(&adev->gmc.invalidate_lock); + spin_unlock_irqrestore(&adev->gmc.invalidate_lock, flags); if (j < adev->usec_timeout) return; @@ -878,6 +866,70 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid, DRM_ERROR("Timeout waiting for VM flush ACK!\n"); } +/** + * gmc_v9_0_flush_gpu_tlb - tlb flush with certain type + * + * @adev: amdgpu_device pointer + * @vmid: vm instance to flush + * @vmhub: which hub to flush + * @flush_type: the flush type + * + * Flush the TLB for the requested page table using certain type. + */ +static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid, + uint32_t vmhub, uint32_t flush_type) +{ + u32 inv_req; + const unsigned int eng = 17; + struct amdgpu_vmhub *hub; + u32 inst; + + BUG_ON(vmhub >= AMDGPU_MAX_VMHUBS); + + hub = &adev->vmhub[vmhub]; + inv_req = gmc_v9_0_get_invalidate_req(vmid, flush_type); + + if (vmhub >= AMDGPU_MMHUB0(0)) + inst = 0; + else + inst = vmhub; + + /* This is necessary for SRIOV as well as for GFXOFF to function + * properly under bare metal + */ + if (adev->gfx.kiq[inst].ring.sched.ready && + (amdgpu_sriov_runtime(adev) || !amdgpu_sriov_vf(adev))) { + uint32_t req = hub->vm_inv_eng0_req + hub->eng_distance * eng; + uint32_t ack = hub->vm_inv_eng0_ack + hub->eng_distance * eng; + + if (atomic_read(&adev->gfx.kiq[inst].flush_failures) < + AMDGPU_KIQ_FLUSH_MAX_FAIL) { + if (!amdgpu_gmc_fw_reg_write_reg_wait(adev, req, ack, + inv_req, + 1 << vmid, inst)) + return; + /* KIQ submit failed; error already logged */ + } + + /* + * MMIO fallback: invalidation must not be silently dropped + * when KIQ is unresponsive. Direct register access is only + * safe in process context (amdgpu_gfx_off_ctrl may sleep) + * and on bare metal; SR-IOV VFs must keep proxying through + * KIQ. + */ + if (in_interrupt() || amdgpu_sriov_vf(adev)) + return; + + amdgpu_gfx_off_ctrl(adev, false); + gmc_v9_0_flush_gpu_tlb_mmio(adev, hub, vmid, vmhub, inv_req); + amdgpu_gfx_off_ctrl(adev, true); + return; + } + + gmc_v9_0_flush_gpu_tlb_mmio(adev, hub, vmid, vmhub, inv_req); +} + /** * gmc_v9_0_flush_gpu_tlb_pasid - tlb flush via pasid * @@ -2237,6 +2289,13 @@ static void gmc_v9_0_gart_disable(struct amdgpu_device *adev) static int gmc_v9_0_hw_fini(struct amdgpu_ip_block *ip_block) { struct amdgpu_device *adev = ip_block->adev; + int i; + + /* KIQ instances are re-initialized on the next resume; re-arm + * the MMIO fallback logic + */ + for (i = 0; i < AMDGPU_MAX_GC_INSTANCES; i++) + atomic_set(&adev->gfx.kiq[i].flush_failures, 0); gmc_v9_0_gart_disable(adev); -- 2.55.0
