cyan_skilfish has problems with other flush types.
v2: fix incorrect ternary conditional operator usage.(Yifan)
Signed-off-by: Lang Yu Lang.Yu@amd.com Cc: stable@vger.kernel.org # v5.15+ --- drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c index d3da13f4c80e..c6d11047169a 100644 --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c @@ -236,7 +236,8 @@ static void gmc_v10_0_flush_vm_hub(struct amdgpu_device *adev, uint32_t vmid, { bool use_semaphore = gmc_v10_0_use_invalidate_semaphore(adev, vmhub); struct amdgpu_vmhub *hub = &adev->vmhub[vmhub]; - u32 inv_req = hub->vmhub_funcs->get_invalidate_req(vmid, flush_type); + u32 inv_req = hub->vmhub_funcs->get_invalidate_req(vmid, + (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0); u32 tmp; /* Use register 17 for GART */ const unsigned int eng = 17; @@ -331,6 +332,8 @@ static void gmc_v10_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
int r;
+ flush_type = (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0; + /* flush hdp cache */ adev->hdp.funcs->flush_hdp(adev, NULL);
@@ -426,6 +429,8 @@ static int gmc_v10_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev, struct amdgpu_ring *ring = &adev->gfx.kiq[0].ring; struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
+ flush_type = (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0; + if (amdgpu_emu_mode == 0 && ring->sched.ready) { spin_lock(&adev->gfx.kiq[0].ring_lock); /* 2 dwords flush + 8 dwords fence */
[AMD Official Use Only - General]
This patch is :
Reviewed-by: Yifan Zhang yifan1.zhang@amd.com
-----Original Message----- From: Yu, Lang Lang.Yu@amd.com Sent: Thursday, September 14, 2023 5:24 PM To: amd-gfx@lists.freedesktop.org Cc: Deucher, Alexander Alexander.Deucher@amd.com; Koenig, Christian Christian.Koenig@amd.com; Zhang, Yifan Yifan1.Zhang@amd.com; Yu, Lang Lang.Yu@amd.com; stable@vger.kernel.org Subject: [PATCH v2] drm/amdgpu: always use legacy tlb flush on cyan_skilfish
cyan_skilfish has problems with other flush types.
v2: fix incorrect ternary conditional operator usage.(Yifan)
Signed-off-by: Lang Yu Lang.Yu@amd.com Cc: stable@vger.kernel.org # v5.15+ --- drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c index d3da13f4c80e..c6d11047169a 100644 --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c @@ -236,7 +236,8 @@ static void gmc_v10_0_flush_vm_hub(struct amdgpu_device *adev, uint32_t vmid, { bool use_semaphore = gmc_v10_0_use_invalidate_semaphore(adev, vmhub); struct amdgpu_vmhub *hub = &adev->vmhub[vmhub]; - u32 inv_req = hub->vmhub_funcs->get_invalidate_req(vmid, flush_type); + u32 inv_req = hub->vmhub_funcs->get_invalidate_req(vmid, + (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0); u32 tmp; /* Use register 17 for GART */ const unsigned int eng = 17; @@ -331,6 +332,8 @@ static void gmc_v10_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
int r;
+ flush_type = (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : +0; + /* flush hdp cache */ adev->hdp.funcs->flush_hdp(adev, NULL);
@@ -426,6 +429,8 @@ static int gmc_v10_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev, struct amdgpu_ring *ring = &adev->gfx.kiq[0].ring; struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
+ flush_type = (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : +0; + if (amdgpu_emu_mode == 0 && ring->sched.ready) { spin_lock(&adev->gfx.kiq[0].ring_lock); /* 2 dwords flush + 8 dwords fence */ -- 2.25.1
Is a single legacy flush sufficient to emulate an heavyweight flush as well?
On previous generations we needed to issue at least two legacy flushes for this.
And please don't push before getting an rb from Felix as well.
Regards, Christian.
Am 14.09.23 um 11:23 schrieb Lang Yu:
cyan_skilfish has problems with other flush types.
v2: fix incorrect ternary conditional operator usage.(Yifan)
Signed-off-by: Lang Yu Lang.Yu@amd.com Cc: stable@vger.kernel.org # v5.15+
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c index d3da13f4c80e..c6d11047169a 100644 --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c @@ -236,7 +236,8 @@ static void gmc_v10_0_flush_vm_hub(struct amdgpu_device *adev, uint32_t vmid, { bool use_semaphore = gmc_v10_0_use_invalidate_semaphore(adev, vmhub); struct amdgpu_vmhub *hub = &adev->vmhub[vmhub];
- u32 inv_req = hub->vmhub_funcs->get_invalidate_req(vmid, flush_type);
- u32 inv_req = hub->vmhub_funcs->get_invalidate_req(vmid,
(adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0);@@ -331,6 +332,8 @@ static void gmc_v10_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid, int r;
- flush_type = (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0;
- /* flush hdp cache */ adev->hdp.funcs->flush_hdp(adev, NULL);
@@ -426,6 +429,8 @@ static int gmc_v10_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev, struct amdgpu_ring *ring = &adev->gfx.kiq[0].ring; struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
- flush_type = (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0;
- if (amdgpu_emu_mode == 0 && ring->sched.ready) { spin_lock(&adev->gfx.kiq[0].ring_lock); /* 2 dwords flush + 8 dwords fence */
On 2023-09-14 9:39, Christian König wrote:
Is a single legacy flush sufficient to emulate an heavyweight flush as well?
On previous generations we needed to issue at least two legacy flushes for this.
I assume you are referring to the Vega20 XGMI workaround. That is a very different issue. Because PTEs would be cached in L2, we had to always use a heavy-weight flush that would also flush the L2 cache as well, and follow that with another legacy flush to deal with race conditions where stale PTEs could be re-fetched from L2 before the L2 flush was complete.
A heavy-weight flush guarantees that there are no more possible memory accesses using the old PTEs. With physically addressed caches on GFXv9 that includes a cache flush because the address translation happened before putting data into the cache. I think the address translation and cache architecture works differently on GFXv10. So maybe the cache-flush is not required here.
But even then a legacy flush probably allows for in-flight memory accesses with old physical addresses to complete after the TLB flush. So there is a small risk of memory corruption that was assumed to not be accessed by the GPU any more. Or when using IOMMU device isolation it would result in IOMMU faults if the DMA mappings are invalidated slightly too early.
Regards, Felix
And please don't push before getting an rb from Felix as well.
Regards, Christian.
Am 14.09.23 um 11:23 schrieb Lang Yu:
cyan_skilfish has problems with other flush types.
v2: fix incorrect ternary conditional operator usage.(Yifan)
Signed-off-by: Lang Yu Lang.Yu@amd.com Cc: stable@vger.kernel.org # v5.15+
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c index d3da13f4c80e..c6d11047169a 100644 --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c @@ -236,7 +236,8 @@ static void gmc_v10_0_flush_vm_hub(struct amdgpu_device *adev, uint32_t vmid, { bool use_semaphore = gmc_v10_0_use_invalidate_semaphore(adev, vmhub); struct amdgpu_vmhub *hub = &adev->vmhub[vmhub]; - u32 inv_req = hub->vmhub_funcs->get_invalidate_req(vmid, flush_type); + u32 inv_req = hub->vmhub_funcs->get_invalidate_req(vmid, + (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0); u32 tmp; /* Use register 17 for GART */ const unsigned int eng = 17; @@ -331,6 +332,8 @@ static void gmc_v10_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid, int r; + flush_type = (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0;
/* flush hdp cache */ adev->hdp.funcs->flush_hdp(adev, NULL); @@ -426,6 +429,8 @@ static int gmc_v10_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev, struct amdgpu_ring *ring = &adev->gfx.kiq[0].ring; struct amdgpu_kiq *kiq = &adev->gfx.kiq[0]; + flush_type = (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0;
if (amdgpu_emu_mode == 0 && ring->sched.ready) { spin_lock(&adev->gfx.kiq[0].ring_lock); /* 2 dwords flush + 8 dwords fence */
Am 14.09.23 um 15:59 schrieb Felix Kuehling:
On 2023-09-14 9:39, Christian König wrote:
Is a single legacy flush sufficient to emulate an heavyweight flush as well?
On previous generations we needed to issue at least two legacy flushes for this.
I assume you are referring to the Vega20 XGMI workaround. That is a very different issue. Because PTEs would be cached in L2, we had to always use a heavy-weight flush that would also flush the L2 cache as well, and follow that with another legacy flush to deal with race conditions where stale PTEs could be re-fetched from L2 before the L2 flush was complete.
No, we also have another (badly documented) workaround which issues a legacy flush before each heavy weight on some hw generations. See the my TLB flush cleanup patches.
A heavy-weight flush guarantees that there are no more possible memory accesses using the old PTEs. With physically addressed caches on GFXv9 that includes a cache flush because the address translation happened before putting data into the cache. I think the address translation and cache architecture works differently on GFXv10. So maybe the cache-flush is not required here.
But even then a legacy flush probably allows for in-flight memory accesses with old physical addresses to complete after the TLB flush. So there is a small risk of memory corruption that was assumed to not be accessed by the GPU any more. Or when using IOMMU device isolation it would result in IOMMU faults if the DMA mappings are invalidated slightly too early.
Mhm, that's quite bad. Any idea how to avoid that?
Regards, Christian.
Regards, Felix
And please don't push before getting an rb from Felix as well.
Regards, Christian.
Am 14.09.23 um 11:23 schrieb Lang Yu:
cyan_skilfish has problems with other flush types.
v2: fix incorrect ternary conditional operator usage.(Yifan)
Signed-off-by: Lang Yu Lang.Yu@amd.com Cc: stable@vger.kernel.org # v5.15+
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c index d3da13f4c80e..c6d11047169a 100644 --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c @@ -236,7 +236,8 @@ static void gmc_v10_0_flush_vm_hub(struct amdgpu_device *adev, uint32_t vmid, { bool use_semaphore = gmc_v10_0_use_invalidate_semaphore(adev, vmhub); struct amdgpu_vmhub *hub = &adev->vmhub[vmhub]; - u32 inv_req = hub->vmhub_funcs->get_invalidate_req(vmid, flush_type); + u32 inv_req = hub->vmhub_funcs->get_invalidate_req(vmid, + (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0); u32 tmp; /* Use register 17 for GART */ const unsigned int eng = 17; @@ -331,6 +332,8 @@ static void gmc_v10_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid, int r; + flush_type = (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0;
/* flush hdp cache */ adev->hdp.funcs->flush_hdp(adev, NULL); @@ -426,6 +429,8 @@ static int gmc_v10_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev, struct amdgpu_ring *ring = &adev->gfx.kiq[0].ring; struct amdgpu_kiq *kiq = &adev->gfx.kiq[0]; + flush_type = (adev->asic_type != CHIP_CYAN_SKILLFISH) ? flush_type : 0;
if (amdgpu_emu_mode == 0 && ring->sched.ready) { spin_lock(&adev->gfx.kiq[0].ring_lock); /* 2 dwords flush + 8 dwords fence */
linux-stable-mirror@lists.linaro.org
-
Christian König -
Felix Kuehling -
Lang Yu -
Zhang, Yifan