Skip to contentSkip to conversation

drm/amdgpu: always use legacy tlb flush on cyan_skilfish

PATCH In review Lang Yu 5 participants 10 messages Sep 14, 2023 – Sep 18, 2023

Subsystems (3)
Stable branch DRM drivers Radeon and amdgpu DRM drivers
lore
Share & tools
lore

Lang Yu

author
#0 lore raw

PATCH v2 1/1 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 <[email protected]>
Cc <[email protected]> # v5.15+
---
 drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +6 -1
index d3da13f4c80e..c6d11047169a 100644
@@ -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 */
Signature · 1 line
2.25.1
Reviewed-by on v2 1/1
#1 lore raw

[AMD Official Use Only - General]

This patch is :

Reviewed-by Yifan Zhang <[email protected]>

-----Original Message-----
From: Yu, Lang <[email protected]>
Sent: Thursday, September 14, 2023 5:24 PM
To: [email protected]
Cc: Deucher, Alexander <[email protected]>; Koenig, Christian <[email protected]>; Zhang, Yifan <[email protected]>; Yu, Lang <[email protected]>; [email protected]
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 <[email protected]>
Cc <[email protected]> # v5.15+
---
 drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +5 -1
index d3da13f4c80e..c6d11047169a 100644
@@ -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

maintainer
#2 lore raw
In reply to Lang Yu

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:

Quoted 42 lines

cyan_skilfish has problems with other flush types.

v2: fix incorrect ternary conditional operator usage.(Yifan)

Signed-off-by Lang Yu <[email protected]>
Cc <[email protected]> # v5.15+
---
  drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +6 -1
index d3da13f4c80e..c6d11047169a 100644
@@ -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 */
#3 lore raw
Quoted 3 lines · Christian König

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

Quoted 51 lines

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:

Quoted 42 lines

cyan_skilfish has problems with other flush types.

v2: fix incorrect ternary conditional operator usage.(Yifan)

Signed-off-by Lang Yu <[email protected]>
Cc <[email protected]> # v5.15+

---
  drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++-

  1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +2 -1
index d3da13f4c80e..c6d11047169a 100644
@@ -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;

drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +1 -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 */

maintainer
#4 lore raw

Am 14.09.23 um 15:59 schrieb Felix Kuehling:

Quoted 6 lines · Christian König
Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 56 lines

Regards,
  Felix

Quoted 51 lines

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:

Quoted 42 lines

cyan_skilfish has problems with other flush types.

v2: fix incorrect ternary conditional operator usage.(Yifan)

Signed-off-by Lang Yu <[email protected]>
Cc <[email protected]> # v5.15+

---
  drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++-

  1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +2 -1
index d3da13f4c80e..c6d11047169a 100644
@@ -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;

drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +1 -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 */

#5 lore raw
Quoted 10 lines · Christian König

Am 14.09.23 um 15:59 schrieb Felix Kuehling:

Quoted 6 lines · Christian König
Quoted 3 lines

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.

Quoted 8 lines

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.

Quoted 4 lines

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?

A few ideas

* Add an arbitrary delay and hope that it is longer than the FIFOs in the HW
* Execute an atomic operation to memory on some GPU engine that could act as a fence, maybe just a RELEASE_MEM on the CP to some writeback location would do the job
* If needed, RELEASE_MEM could also perform a cache flush

Regards,
  Felix

Quoted 61 lines

Regards,
Christian.

Quoted 56 lines

Regards,
  Felix

Quoted 51 lines

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:

Quoted 42 lines

cyan_skilfish has problems with other flush types.

v2: fix incorrect ternary conditional operator usage.(Yifan)

Signed-off-by Lang Yu <[email protected]>
Cc <[email protected]> # v5.15+

---
  drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++-

  1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +2 -1
index d3da13f4c80e..c6d11047169a 100644
@@ -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;

drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +1 -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 */

author
#6 lore raw
Quoted 1 line · Felix Kuehling

On 2023-09-14 10:02, Christian König wrote:

Do we still need to use legacy flush to emulate heavyweight flush if we don't use SVM? And can I push this now?

Regards,
Lang

Quoted 118 lines
Quoted 28 lines

Am 14.09.23 um 15:59 schrieb Felix Kuehling:

Quoted 13 lines · Christian König
Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 8 lines
Quoted 6 lines

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?

A few ideas

* Add an arbitrary delay and hope that it is longer than the FIFOs in the HW
* Execute an atomic operation to memory on some GPU engine that could act as a fence, maybe just a RELEASE_MEM on the CP to some writeback location would do the job
* If needed, RELEASE_MEM could also perform a cache flush

Regards,
  Felix

Quoted 68 lines

Regards,
Christian.

Quoted 63 lines

Regards,
  Felix

Quoted 58 lines

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:

Quoted 49 lines

cyan_skilfish has problems with other flush types.

v2: fix incorrect ternary conditional operator usage.(Yifan)

Signed-off-by Lang Yu <[email protected]>
Cc <[email protected]> # v5.15+

---
  drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++-

  1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +0 -0
index d3da13f4c80e..c6d11047169a 100644
@@ -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;

drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +0 -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 */

#7 lore raw

Am 15.09.23 um 10:53 schrieb Lang Yu:

Quoted 4 lines · Felix Kuehling
Quoted 1 line · Christian König

Do we still need to use legacy flush to emulate heavyweight flush if we don't use SVM? And can I push this now?

Felix needs to decide that. From what I understand the KFD needs heavyweight flushes for secure SVM operation.

If heavyweight flushes are buggy papering over that by using legacy flushes is only a mediocre workaround.

Regards,
Christian.

Quoted 112 lines

Regards,
Lang

Quoted 107 lines
Quoted 24 lines

Am 14.09.23 um 15:59 schrieb Felix Kuehling:

Quoted 12 lines · Christian König
Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 7 lines
Quoted 6 lines

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?

A few ideas

* Add an arbitrary delay and hope that it is longer than the FIFOs in the HW
* Execute an atomic operation to memory on some GPU engine that could act as a fence, maybe just a RELEASE_MEM on the CP to some writeback location would do the job
* If needed, RELEASE_MEM could also perform a cache flush

Regards,
  Felix

Quoted 63 lines

Regards,
Christian.

Quoted 60 lines

Regards,
  Felix

Quoted 56 lines

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:

Quoted 49 lines

cyan_skilfish has problems with other flush types.

v2: fix incorrect ternary conditional operator usage.(Yifan)

Signed-off-by Lang Yu <[email protected]>
Cc <[email protected]> # v5.15+
---
   drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++-
   1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +0 -0
index d3da13f4c80e..c6d11047169a 100644
@@ -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;

drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +0 -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 */

#8 lore raw
Quoted 7 lines · Christian König

Am 15.09.23 um 10:53 schrieb Lang Yu:

Quoted 4 lines · Felix Kuehling
Quoted 1 line · Christian König

Do we still need to use legacy flush to emulate heavyweight flush if we don't use SVM? And can I push this now?

Felix needs to decide that. From what I understand the KFD needs heavyweight flushes for secure SVM operation.

Yes. We need to be able to guarantee to the kernel, that the GPU will not access unmapped memory. There are two strategies in the driver to do this:

1. Preempt GPU queues (which implies a heavy-weight TLB flush in the scheduler firmware)
2. Invalidate page table entries and flush TLBs

#1 happens during MMU notifiers with XNACK off. #2 happens in MMU notifiers with XNACK on (not supported on GFX10.x) and when unified memory us munmapped. It's that last part I'm worried about. When memory is munmapped and given back to the OS, we need to be able to guarantee that the GPU won't access it any more. The same is true when GTT BOs and userptr BOs are freed. After unmapping them from the GPU page tables, we need a heavy-weight flush. I believe the same should apply to the graphics driver, but maybe that's implied through the CS and fence mechanisms that keep memory allocated while the GPU is accessing it.

A legacy flush has a slim chance of not being sufficient because memory accesses using old addresses can still be in flight in the GPU.

Quoted 2 lines

If heavyweight flushes are buggy papering over that by using legacy flushes is only a mediocre workaround.

I agree. I'd like to avoid half-baked workarounds that will cause more headaches later on. I started an internal email thread with Tony to understand the requirements for heavy-weight flushes on the affected GPUs and find a better workaround.

Regards,
  Felix

Quoted 117 lines

Regards,
Christian.

Quoted 112 lines

Regards,
Lang

Quoted 107 lines
Quoted 24 lines

Am 14.09.23 um 15:59 schrieb Felix Kuehling:

Quoted 12 lines · Christian König
Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 7 lines
Quoted 6 lines

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?

A few ideas

  * Add an arbitrary delay and hope that it is longer than the FIFOs in the HW
  * Execute an atomic operation to memory on some GPU engine that could act as a fence, maybe just a RELEASE_MEM on the CP to some writeback location would do the job
  * If needed, RELEASE_MEM could also perform a cache flush

Regards,
   Felix

Quoted 63 lines

Regards,
Christian.

Quoted 60 lines

Regards,
   Felix

Quoted 56 lines

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:

Quoted 49 lines

cyan_skilfish has problems with other flush types.

v2: fix incorrect ternary conditional operator usage.(Yifan)

Signed-off-by Lang Yu <[email protected]>
Cc <[email protected]> # v5.15+

---
   drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++-

   1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +0 -0
index d3da13f4c80e..c6d11047169a 100644
@@ -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;

drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +0 -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 */

#9 lore raw

Am 15.09.23 um 16:49 schrieb Felix Kuehling:

Quoted 17 lines · Christian König
Quoted 7 lines

Am 15.09.23 um 10:53 schrieb Lang Yu:

Quoted 4 lines · Felix Kuehling
Quoted 1 line · Christian König

Do we still need to use legacy flush to emulate heavyweight flush if we don't use SVM? And can I push this now?

Felix needs to decide that. From what I understand the KFD needs heavyweight flushes for secure SVM operation.

Yes. We need to be able to guarantee to the kernel, that the GPU will not access unmapped memory. There are two strategies in the driver to do this:

1. Preempt GPU queues (which implies a heavy-weight TLB flush in the scheduler firmware)
2. Invalidate page table entries and flush TLBs

#1 happens during MMU notifiers with XNACK off. #2 happens in MMU notifiers with XNACK on (not supported on GFX10.x) and when unified memory us munmapped. It's that last part I'm worried about. When memory is munmapped and given back to the OS, we need to be able to guarantee that the GPU won't access it any more. The same is true when GTT BOs and userptr BOs are freed. After unmapping them from the GPU page tables, we need a heavy-weight flush. I believe the same should apply to the graphics driver, but maybe that's implied through the CS and fence mechanisms that keep memory allocated while the GPU is accessing it.

Quoted 8 lines

A legacy flush has a slim chance of not being sufficient because memory accesses using old addresses can still be in flight in the GPU.

Quoted 2 lines

If heavyweight flushes are buggy papering over that by using legacy flushes is only a mediocre workaround.

I agree. I'd like to avoid half-baked workarounds that will cause more headaches later on. I started an internal email thread with Tony to understand the requirements for heavy-weight flushes on the affected GPUs and find a better workaround.

Thanks, then this patch should be put on hold until that stuff is cleared up.

Regards,
Christian.

Quoted 121 lines

Regards,
  Felix

Quoted 117 lines

Regards,
Christian.

Quoted 112 lines

Regards,
Lang

Quoted 107 lines
Quoted 24 lines

Am 14.09.23 um 15:59 schrieb Felix Kuehling:

Quoted 12 lines · Christian König
Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 7 lines
Quoted 6 lines

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?

A few ideas

  * Add an arbitrary delay and hope that it is longer than the FIFOs in the HW
  * Execute an atomic operation to memory on some GPU engine that could act as a fence, maybe just a RELEASE_MEM on the CP to some writeback location would do the job
  * If needed, RELEASE_MEM could also perform a cache flush

Regards,
   Felix

Quoted 63 lines

Regards,
Christian.

Quoted 60 lines

Regards,
   Felix

Quoted 56 lines

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:

Quoted 49 lines

cyan_skilfish has problems with other flush types.

v2: fix incorrect ternary conditional operator usage.(Yifan)

Signed-off-by Lang Yu <[email protected]>
Cc <[email protected]> # v5.15+

---
   drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 7 ++++++-

   1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +0 -0
index d3da13f4c80e..c6d11047169a 100644
@@ -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;

drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c +0 -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 */