On 2020-01-20 12:47 p.m., shaoyunl wrote:
comments in line . On 2020-01-17 8:37 p.m., Felix Kuehling wrote:Using a heavy-weight TLB flush once is not sufficient. Concurrent memory accesses in the same TLB cache line can re-populate TLB entries from stale texture cache (TC) entries while the heavy-weight TLB flush is in progress. To fix this race condition, perform another TLB flush after the heavy-weight one, when TC is known to be clean. Move the workaround into the low-level TLB flushing functions. This way they apply to amdgpu as well, and KIQ-based TLB flush only needs to synchronize once. CC: shaoyun.liu@xxxxxxx Signed-off-by: Felix Kuehling <Felix.Kuehling@xxxxxxx> --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 6 +- drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 68 +++++++++++++++++----- 2 files changed, 53 insertions(+), 21 deletions(-)diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.cindex 8609287620ea..5325f6b455f6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c@@ -647,13 +647,9 @@ int amdgpu_amdkfd_flush_gpu_tlb_vmid(struct kgd_dev *kgd, uint16_t vmid) int amdgpu_amdkfd_flush_gpu_tlb_pasid(struct kgd_dev *kgd, uint16_t pasid){ struct amdgpu_device *adev = (struct amdgpu_device *)kgd; - uint32_t flush_type = 0; + const uint32_t flush_type = 0; bool all_hub = false; - if (adev->gmc.xgmi.num_physical_nodes && - adev->asic_type == CHIP_VEGA20) - flush_type = 2; - if (adev->family == AMDGPU_FAMILY_AI) all_hub = true;diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.cindex 90216abf14a4..e2a5e852bdb0 100644 --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c@@ -476,13 +476,26 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,{bool use_semaphore = gmc_v9_0_use_invalidate_semaphore(adev, vmhub);const unsigned eng = 17; - u32 j, inv_req, tmp; + u32 j, inv_req, inv_req2, tmp; struct amdgpu_vmhub *hub; BUG_ON(vmhub >= adev->num_vmhubs); hub = &adev->vmhub[vmhub]; - inv_req = gmc_v9_0_get_invalidate_req(vmid, flush_type); + if (adev->gmc.xgmi.num_physical_nodes && + adev->asic_type == CHIP_VEGA20) { + /* Vega20+XGMI caches PTEs in TC and TLB. Add a + * heavy-weight TLB flush (type 2), which flushes + * both. Due to a race condition with concurrent + * memory accesses using the same TLB cache line, we + * still need a second TLB flush after this. + */ + inv_req = gmc_v9_0_get_invalidate_req(vmid, 2); + inv_req2 = gmc_v9_0_get_invalidate_req(vmid, flush_type);[shaoyunl] For the send invalidation in this situation ,can we use 0 for the flush type directly ? I think no matter what's the input flush_type for this function , heavy-weight + legacy invalidation should be enough for all of them .
I'm not sure that's true. In the case of the race condition, there was some concurrent memory access during the first heavy-weight invalidation. If that is now flushed in the second invalidation, and a heavy-weight invalidation was requested, we should also flush any TC cache lines associated with that access. So hard-coding flush_type 0 here is probably not safe for all cases.
Regards, Felix
+ } else { + inv_req = gmc_v9_0_get_invalidate_req(vmid, flush_type); + inv_req2 = 0; + } /* This is necessary for a HW workaround under SRIOV as well * as GFXOFF under bare metal@@ -521,21 +534,27 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid, DRM_ERROR("Timeout waiting for sem acquire in VM flush!\n");} - WREG32_NO_KIQ(hub->vm_inv_eng0_req + eng, inv_req); + do { + WREG32_NO_KIQ(hub->vm_inv_eng0_req + eng, inv_req); - /* - * Issue a dummy read to wait for the ACK register to be cleared - * to avoid a false ACK due to the new fast GRBM interface. - */ - if (vmhub == AMDGPU_GFXHUB_0) - RREG32_NO_KIQ(hub->vm_inv_eng0_req + eng); + /* + * Issue a dummy read to wait for the ACK register to + * be cleared to avoid a false ACK due to the new fast + * GRBM interface. + */ + if (vmhub == AMDGPU_GFXHUB_0) + RREG32_NO_KIQ(hub->vm_inv_eng0_req + eng); - for (j = 0; j < adev->usec_timeout; j++) { - tmp = RREG32_NO_KIQ(hub->vm_inv_eng0_ack + eng); - if (tmp & (1 << vmid)) - break; - udelay(1); - } + for (j = 0; j < adev->usec_timeout; j++) { + tmp = RREG32_NO_KIQ(hub->vm_inv_eng0_ack + eng); + if (tmp & (1 << vmid)) + break; + udelay(1); + } + + inv_req = inv_req2; + inv_req2 = 0; + } while (inv_req);/* TODO: It needs to continue working on debugging with semaphore for GFXHUB as well. */if (use_semaphore)@@ -577,9 +596,26 @@ static int gmc_v9_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev,return -EIO; if (ring->sched.ready) { + /* Vega20+XGMI caches PTEs in TC and TLB. Add a + * heavy-weight TLB flush (type 2), which flushes + * both. Due to a race condition with concurrent + * memory accesses using the same TLB cache line, we + * still need a second TLB flush after this. + */ + bool vega20_xgmi_wa = (adev->gmc.xgmi.num_physical_nodes && + adev->asic_type == CHIP_VEGA20); + /* 2 dwords flush + 8 dwords fence */ + unsigned int ndw = kiq->pmf->invalidate_tlbs_size + 8; + + if (vega20_xgmi_wa) + ndw += kiq->pmf->invalidate_tlbs_size; + spin_lock(&adev->gfx.kiq.ring_lock); /* 2 dwords flush + 8 dwords fence */ - amdgpu_ring_alloc(ring, kiq->pmf->invalidate_tlbs_size + 8); + amdgpu_ring_alloc(ring, ndw); + if (vega20_xgmi_wa) + kiq->pmf->kiq_invalidate_tlbs(ring, + pasid, 2, all_hub); kiq->pmf->kiq_invalidate_tlbs(ring, pasid, flush_type, all_hub); amdgpu_fence_emit_polling(ring, &seq);
_______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
