From c20dc3698dc7ecf053e2bf77299ae5982c0c2c45 Mon Sep 17 00:00:00 2001 From: Christian Koenig Date: Wed, 16 May 2012 21:45:24 +0200 Subject: [PATCH] drm/radeon: fix & improve ih ring handling v3 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. v2: Keep the spinlock.... v3: Back to an atomic again after finding & fixing the real bug. Signed-off-by: Christian Koenig --- drivers/gpu/drm/radeon/evergreen.c | 26 +++++++++++------------ drivers/gpu/drm/radeon/r600.c | 29 ++++++++++++-------------- drivers/gpu/drm/radeon/radeon.h | 3 +-- drivers/gpu/drm/radeon/radeon_device.c | 3 +-- drivers/gpu/drm/radeon/si.c | 27 ++++++++++++------------ 5 files changed, 41 insertions(+), 47 deletions(-) diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c index 8e40ba4a9b7..765bd1f2e1c 100644 --- a/drivers/gpu/drm/radeon/evergreen.c +++ b/drivers/gpu/drm/radeon/evergreen.c @@ -2676,7 +2676,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; @@ -2684,22 +2683,21 @@ 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; 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; @@ -2998,17 +2996,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 aced97bb79e..b487c69a486 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -2858,7 +2858,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; } @@ -3310,7 +3309,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; @@ -3322,24 +3320,21 @@ int r600_irq_process(struct radeon_device *rdev) RREG32(IH_RB_WPTR); wptr = r600_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; 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 */ r600_irq_ack(rdev); - rdev->ih.wptr = wptr; while (rptr != wptr) { /* wptr/rptr are in bytes! */ ring_index = rptr / 4; @@ -3493,17 +3488,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 9d6c1de9359..5ddf895a592 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -738,11 +738,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 7667184ab0b..3c563d1f967 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 8868a1fa20e..ecef972050d 100644 --- a/drivers/gpu/drm/radeon/si.c +++ b/drivers/gpu/drm/radeon/si.c @@ -2942,7 +2942,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; } @@ -3359,29 +3358,27 @@ 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; 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; @@ -3632,15 +3629,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; }