Re: [PATCH 06/10] drm/radeon: fix & improve ih ring handling

On Thu, May 24, 2012 at 09:49:10AM +0200, Christian König wrote:

From: Christian Koenig christian.koenig@amd.com

The spinlock was actually there to protect the rptr, but rptr was read outside of the locked area.

Also we don't really need a spinlock here, an atomic should to quite fine since we only need to prevent it from being reentrant.

Signed-off-by: Christian Koenig christian.koenig@amd.com

Reviewed-by: Jerome Glisse jglisse@redhat.com

---

drivers/gpu/drm/radeon/evergreen.c | 29 ++++++++++++++++------------- drivers/gpu/drm/radeon/r600.c | 30 +++++++++++++++--------------- drivers/gpu/drm/radeon/radeon.h | 3 +-- drivers/gpu/drm/radeon/radeon_device.c | 3 +-- drivers/gpu/drm/radeon/si.c | 30 ++++++++++++++++-------------- 5 files changed, 49 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c index dd3cea4..bfcb39e 100644 --- a/drivers/gpu/drm/radeon/evergreen.c +++ b/drivers/gpu/drm/radeon/evergreen.c @@ -2943,7 +2943,6 @@ int evergreen_irq_process(struct radeon_device *rdev) u32 rptr; u32 src_id, src_data; u32 ring_index;

• unsigned long flags; bool queue_hotplug = false; bool queue_hdmi = false;

@@ -2951,22 +2950,24 @@ int evergreen_irq_process(struct radeon_device *rdev) return IRQ_NONE;

wptr = evergreen_get_ih_wptr(rdev);

+restart_ih:

• /* is somebody else already processing irqs? */
• if (atomic_xchg(&rdev->ih.lock, 1))

• return IRQ_NONE;
  

• rptr = rdev->ih.rptr;
• if (rptr == wptr)

• return IRQ_NONE;
  

• DRM_DEBUG("r600_irq_process start: rptr %d, wptr %d\n", rptr, wptr);

• spin_lock_irqsave(&rdev->ih.lock, flags);
• if (rptr == wptr) {

• spin_unlock_irqrestore(&rdev->ih.lock, flags);
  

• return IRQ_NONE;
  

• }

-restart_ih: /* Order reading of wptr vs. reading of IH ring data */ rmb();

/* display interrupts */ evergreen_irq_ack(rdev);

• rdev->ih.wptr = wptr; while (rptr != wptr) { /* wptr/rptr are in bytes! */ ring_index = rptr / 4;

@@ -3265,17 +3266,19 @@ restart_ih: rptr += 16; rptr &= rdev->ih.ptr_mask; }

• /* make sure wptr hasn't changed while processing */
• wptr = evergreen_get_ih_wptr(rdev);
• if (wptr != rdev->ih.wptr)

• goto restart_ih;
  

  if (queue_hotplug) schedule_work(&rdev->hotplug_work); if (queue_hdmi) schedule_work(&rdev->audio_work); rdev->ih.rptr = rptr; WREG32(IH_RB_RPTR, rdev->ih.rptr);
• spin_unlock_irqrestore(&rdev->ih.lock, flags);

• atomic_set(&rdev->ih.lock, 0);
• /* make sure wptr hasn't changed while processing */
• wptr = evergreen_get_ih_wptr(rdev);
• if (wptr != rptr)

• goto restart_ih;
  

• return IRQ_HANDLED;

}

diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index a8d8c44..eadbb06 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -2921,7 +2921,6 @@ void r600_disable_interrupts(struct radeon_device *rdev) WREG32(IH_RB_RPTR, 0); WREG32(IH_RB_WPTR, 0); rdev->ih.enabled = false;

• rdev->ih.wptr = 0; rdev->ih.rptr = 0;

}

@@ -3373,7 +3372,6 @@ int r600_irq_process(struct radeon_device *rdev) u32 rptr; u32 src_id, src_data; u32 ring_index;

• unsigned long flags; bool queue_hotplug = false; bool queue_hdmi = false;

@@ -3385,24 +3383,24 @@ int r600_irq_process(struct radeon_device *rdev) RREG32(IH_RB_WPTR);

wptr = r600_get_ih_wptr(rdev);

• rptr = rdev->ih.rptr;

• DRM_DEBUG("r600_irq_process start: rptr %d, wptr %d\n", rptr, wptr);

• spin_lock_irqsave(&rdev->ih.lock, flags);

+restart_ih:

• /* is somebody else already processing irqs? */
• if (atomic_xchg(&rdev->ih.lock, 1))

• return IRQ_NONE;
  
  

• if (rptr == wptr) {

• spin_unlock_irqrestore(&rdev->ih.lock, flags);
  

• rptr = rdev->ih.rptr;
• if (rptr == wptr) return IRQ_NONE;

• }

-restart_ih:

• DRM_DEBUG("r600_irq_process start: rptr %d, wptr %d\n", rptr, wptr);

• /* Order reading of wptr vs. reading of IH ring data */ rmb();

  /* display interrupts */ r600_irq_ack(rdev);

• rdev->ih.wptr = wptr; while (rptr != wptr) { /* wptr/rptr are in bytes! */ ring_index = rptr / 4;

@@ -3556,17 +3554,19 @@ restart_ih: rptr += 16; rptr &= rdev->ih.ptr_mask; }

• /* make sure wptr hasn't changed while processing */
• wptr = r600_get_ih_wptr(rdev);
• if (wptr != rdev->ih.wptr)

• goto restart_ih;
  

  if (queue_hotplug) schedule_work(&rdev->hotplug_work); if (queue_hdmi) schedule_work(&rdev->audio_work); rdev->ih.rptr = rptr; WREG32(IH_RB_RPTR, rdev->ih.rptr);
• spin_unlock_irqrestore(&rdev->ih.lock, flags);

• atomic_set(&rdev->ih.lock, 0);
• /* make sure wptr hasn't changed while processing */
• wptr = r600_get_ih_wptr(rdev);
• if (wptr != rptr)

• goto restart_ih;
  

• return IRQ_HANDLED;

}

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 618df9a..8479096 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -733,11 +733,10 @@ struct r600_ih { struct radeon_bo *ring_obj; volatile uint32_t *ring; unsigned rptr;

• unsigned wptr; unsigned ring_size; uint64_t gpu_addr; uint32_t ptr_mask;
• spinlock_t lock;

• atomic_t lock; bool enabled;

};

diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index 7667184..3c563d1 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -731,8 +731,7 @@ int radeon_device_init(struct radeon_device *rdev, radeon_mutex_init(&rdev->cs_mutex); mutex_init(&rdev->ring_lock); mutex_init(&rdev->dc_hw_i2c_mutex);

• if (rdev->family >= CHIP_R600)

• spin_lock_init(&rdev->ih.lock);
  

• atomic_set(&rdev->ih.lock, 0); mutex_init(&rdev->gem.mutex); mutex_init(&rdev->pm.mutex); init_rwsem(&rdev->pm.mclk_lock);

diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c index 5ca8ef5..f969487 100644 --- a/drivers/gpu/drm/radeon/si.c +++ b/drivers/gpu/drm/radeon/si.c @@ -3095,7 +3095,6 @@ static void si_disable_interrupts(struct radeon_device *rdev) WREG32(IH_RB_RPTR, 0); WREG32(IH_RB_WPTR, 0); rdev->ih.enabled = false;

• rdev->ih.wptr = 0; rdev->ih.rptr = 0;

}

@@ -3512,29 +3511,30 @@ int si_irq_process(struct radeon_device *rdev) u32 rptr; u32 src_id, src_data, ring_id; u32 ring_index;

• unsigned long flags; bool queue_hotplug = false;

  if (!rdev->ih.enabled || rdev->shutdown) return IRQ_NONE;

  wptr = si_get_ih_wptr(rdev);

+restart_ih:

• /* is somebody else already processing irqs? */
• if (atomic_xchg(&rdev->ih.lock, 1))

• return IRQ_NONE;
  

• rptr = rdev->ih.rptr;
• if (rptr == wptr)

• return IRQ_NONE;
  

• DRM_DEBUG("si_irq_process start: rptr %d, wptr %d\n", rptr, wptr);

• spin_lock_irqsave(&rdev->ih.lock, flags);
• if (rptr == wptr) {

• spin_unlock_irqrestore(&rdev->ih.lock, flags);
  

• return IRQ_NONE;
  

• }

-restart_ih: /* Order reading of wptr vs. reading of IH ring data */ rmb();

/* display interrupts */ si_irq_ack(rdev);

• rdev->ih.wptr = wptr; while (rptr != wptr) { /* wptr/rptr are in bytes! */ ring_index = rptr / 4;

@@ -3785,15 +3785,17 @@ restart_ih: rptr += 16; rptr &= rdev->ih.ptr_mask; }

• /* make sure wptr hasn't changed while processing */
• wptr = si_get_ih_wptr(rdev);
• if (wptr != rdev->ih.wptr)

• goto restart_ih;
  

  if (queue_hotplug) schedule_work(&rdev->hotplug_work); rdev->ih.rptr = rptr; WREG32(IH_RB_RPTR, rdev->ih.rptr);
• spin_unlock_irqrestore(&rdev->ih.lock, flags);

• atomic_set(&rdev->ih.lock, 0);
• /* make sure wptr hasn't changed while processing */
• wptr = si_get_ih_wptr(rdev);
• if (wptr != rptr)

• goto restart_ih;
  

• return IRQ_HANDLED;

}

-- 1.7.9.5