Re: [PATCH] drm/amdgpu: protect ring overrun

Thu, 23 Apr 2020 05:22:14 -0700 

Am 23.04.20 um 11:06 schrieb Yintian Tao:

Wait for the oldest sequence on the ring
to be signaled in order to make sure there
will be no command overrun.

v2: fix coding stype and remove abs operation
One nit pick below, with that fixed the patch is Reviewed-by: Christian König <christian.koe...@amd.com> 



Signed-off-by: Yintian Tao <yt...@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 10 +++++++++-
  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c   | 22 ++++++++++++++++++----
  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h  |  3 ++-
  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c  |  8 +++++++-
  drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c    |  1 -
  drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c     |  1 -
  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c     | 14 +++++++++++---
  drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c    |  8 +++++++-
  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c     |  8 +++++++-
  9 files changed, 61 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 7531527067df..397bd5fa77cb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -192,14 +192,22 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct 
dma_fence **f,
   * Used For polling fence.
   * Returns 0 on success, -ENOMEM on failure.
   */
-int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s)
+int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s,
+                             uint32_t timeout)
  {
        uint32_t seq;
+       signed long r = 0;
Please drop the initialization of r here. That is usually seen as rather bad style because it prevents the compiler from raising an warning when this really isn't initialized. 

Regards,
Christian.
if (!s) 
                return -EINVAL;
seq = ++ring->fence_drv.sync_seq; 
+       r = amdgpu_fence_wait_polling(ring,
+                                     seq - ring->fence_drv.num_fences_mask,
+                                     timeout);
+       if (r < 1)
+               return -ETIMEDOUT;
+
        amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr,
                               seq, 0);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c 
index a721b0e0ff69..0103acc57474 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -675,13 +675,15 @@ uint32_t amdgpu_kiq_rreg(struct amdgpu_device *adev, 
uint32_t reg)
spin_lock_irqsave(&kiq->ring_lock, flags); 
        if (amdgpu_device_wb_get(adev, &reg_val_offs)) {
-               spin_unlock_irqrestore(&kiq->ring_lock, flags);
                pr_err("critical bug! too many kiq readers\n");
-               goto failed_kiq_read;
+               goto failed_unlock;
        }
        amdgpu_ring_alloc(ring, 32);
        amdgpu_ring_emit_rreg(ring, reg, reg_val_offs);
-       amdgpu_fence_emit_polling(ring, &seq);
+       r = amdgpu_fence_emit_polling(ring, &seq, MAX_KIQ_REG_WAIT);
+       if (r)
+               goto failed_undo;
+
        amdgpu_ring_commit(ring);
        spin_unlock_irqrestore(&kiq->ring_lock, flags);
@@ -712,7 +714,13 @@ uint32_t amdgpu_kiq_rreg(struct amdgpu_device *adev, uint32_t reg) 
        amdgpu_device_wb_free(adev, reg_val_offs);
        return value;
+failed_undo: 
+       amdgpu_ring_undo(ring);
+failed_unlock:
+       spin_unlock_irqrestore(&kiq->ring_lock, flags);
  failed_kiq_read:
+       if (reg_val_offs)
+               amdgpu_device_wb_free(adev, reg_val_offs);
        pr_err("failed to read reg:%x\n", reg);
        return ~0;
  }
@@ -730,7 +738,10 @@ void amdgpu_kiq_wreg(struct amdgpu_device *adev, uint32_t 
reg, uint32_t v)
        spin_lock_irqsave(&kiq->ring_lock, flags);
        amdgpu_ring_alloc(ring, 32);
        amdgpu_ring_emit_wreg(ring, reg, v);
-       amdgpu_fence_emit_polling(ring, &seq);
+       r = amdgpu_fence_emit_polling(ring, &seq, MAX_KIQ_REG_WAIT);
+       if (r)
+               goto failed_undo;
+
        amdgpu_ring_commit(ring);
        spin_unlock_irqrestore(&kiq->ring_lock, flags);
@@ -759,6 +770,9 @@ void amdgpu_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v) return; +failed_undo: 
+       amdgpu_ring_undo(ring);
+       spin_unlock_irqrestore(&kiq->ring_lock, flags);
  failed_kiq_write:
        pr_err("failed to write reg:%x\n", reg);
  }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 137d3d2b46e8..be218754629a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -118,7 +118,8 @@ void amdgpu_fence_driver_suspend(struct amdgpu_device 
*adev);
  void amdgpu_fence_driver_resume(struct amdgpu_device *adev);
  int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **fence,
                      unsigned flags);
-int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s);
+int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s,
+                             uint32_t timeout);
  bool amdgpu_fence_process(struct amdgpu_ring *ring);
  int amdgpu_fence_wait_empty(struct amdgpu_ring *ring);
  signed long amdgpu_fence_wait_polling(struct amdgpu_ring *ring,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index 8c10084f44ef..cbbb8d02535a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -60,7 +60,10 @@ void amdgpu_virt_kiq_reg_write_reg_wait(struct amdgpu_device 
*adev,
        amdgpu_ring_alloc(ring, 32);
        amdgpu_ring_emit_reg_write_reg_wait(ring, reg0, reg1,
                                            ref, mask);
-       amdgpu_fence_emit_polling(ring, &seq);
+       r = amdgpu_fence_emit_polling(ring, &seq, MAX_KIQ_REG_WAIT);
+       if (r)
+               goto failed_undo;
+
        amdgpu_ring_commit(ring);
        spin_unlock_irqrestore(&kiq->ring_lock, flags);
@@ -82,6 +85,9 @@ void amdgpu_virt_kiq_reg_write_reg_wait(struct amdgpu_device *adev, return; +failed_undo: 
+       amdgpu_ring_undo(ring);
+       spin_unlock_irqrestore(&kiq->ring_lock, flags);
  failed_kiq:
        pr_err("failed to write reg %x wait reg %x\n", reg0, reg1);
  }
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index 8a579ce8757d..2b6d7687f4cb 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -7605,7 +7605,6 @@ static void gfx_v10_0_ring_emit_rreg(struct amdgpu_ring 
*ring, uint32_t reg,
                                     uint32_t reg_val_offs)
  {
        struct amdgpu_device *adev = ring->adev;
-       struct amdgpu_kiq *kiq = &adev->gfx.kiq;
amdgpu_ring_write(ring, PACKET3(PACKET3_COPY_DATA, 4)); 
        amdgpu_ring_write(ring, 0 |     /* src: register*/
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index 6c56cedce4c3..6ae78b9e9551 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -6393,7 +6393,6 @@ static void gfx_v8_0_ring_emit_rreg(struct amdgpu_ring 
*ring, uint32_t reg,
                                    uint32_t reg_val_offs)
  {
        struct amdgpu_device *adev = ring->adev;
-       struct amdgpu_kiq *kiq = &adev->gfx.kiq;
amdgpu_ring_write(ring, PACKET3(PACKET3_COPY_DATA, 4)); 
        amdgpu_ring_write(ring, 0 |     /* src: register*/
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index 09aa5f509bd2..be05ab221b16 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -4054,9 +4054,8 @@ static uint64_t gfx_v9_0_kiq_read_clock(struct 
amdgpu_device *adev)
spin_lock_irqsave(&kiq->ring_lock, flags); 
        if (amdgpu_device_wb_get(adev, &reg_val_offs)) {
-               spin_unlock_irqrestore(&kiq->ring_lock, flags);
                pr_err("critical bug! too many kiq readers\n");
-               goto failed_kiq_read;
+               goto failed_unlock;
        }
        amdgpu_ring_alloc(ring, 32);
        amdgpu_ring_write(ring, PACKET3(PACKET3_COPY_DATA, 4));
@@ -4070,7 +4069,10 @@ static uint64_t gfx_v9_0_kiq_read_clock(struct 
amdgpu_device *adev)
                                reg_val_offs * 4));
        amdgpu_ring_write(ring, upper_32_bits(adev->wb.gpu_addr +
                                reg_val_offs * 4));
-       amdgpu_fence_emit_polling(ring, &seq);
+       r = amdgpu_fence_emit_polling(ring, &seq, MAX_KIQ_REG_WAIT);
+       if (r)
+               goto failed_undo;
+
        amdgpu_ring_commit(ring);
        spin_unlock_irqrestore(&kiq->ring_lock, flags);
@@ -4102,7 +4104,13 @@ static uint64_t gfx_v9_0_kiq_read_clock(struct amdgpu_device *adev) 
        amdgpu_device_wb_free(adev, reg_val_offs);
        return value;
+failed_undo: 
+       amdgpu_ring_undo(ring);
+failed_unlock:
+       spin_unlock_irqrestore(&kiq->ring_lock, flags);
  failed_kiq_read:
+       if (reg_val_offs)
+               amdgpu_device_wb_free(adev, reg_val_offs);
        pr_err("failed to read gpu clock\n");
        return ~0;
  }
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
index 30b75d79efdb..b120f9160f13 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
@@ -427,7 +427,13 @@ static int gmc_v10_0_flush_gpu_tlb_pasid(struct 
amdgpu_device *adev,
                amdgpu_ring_alloc(ring, kiq->pmf->invalidate_tlbs_size + 8);
                kiq->pmf->kiq_invalidate_tlbs(ring,
                                        pasid, flush_type, all_hub);
-               amdgpu_fence_emit_polling(ring, &seq);
+               r = amdgpu_fence_emit_polling(ring, &seq, MAX_KIQ_REG_WAIT);
+               if (r) {
+                       amdgpu_ring_undo(ring);
+                       spin_unlock(&kiq->ring_lock);
+                       return -ETIME;
+               }
+
                amdgpu_ring_commit(ring);
                spin_unlock(&adev->gfx.kiq.ring_lock);
                r = amdgpu_fence_wait_polling(ring, seq, adev->usec_timeout);
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index fecdbc471983..0a6026308343 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -621,7 +621,13 @@ static int gmc_v9_0_flush_gpu_tlb_pasid(struct 
amdgpu_device *adev,
                                                      pasid, 2, all_hub);
                kiq->pmf->kiq_invalidate_tlbs(ring,
                                        pasid, flush_type, all_hub);
-               amdgpu_fence_emit_polling(ring, &seq);
+               r = amdgpu_fence_emit_polling(ring, &seq, MAX_KIQ_REG_WAIT);
+               if (r) {
+                       amdgpu_ring_undo(ring);
+                       spin_unlock(&kiq->ring_lock);
+                       return -ETIME;
+               }
+
                amdgpu_ring_commit(ring);
                spin_unlock(&adev->gfx.kiq.ring_lock);
                r = amdgpu_fence_wait_polling(ring, seq, adev->usec_timeout);


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reply via email to