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 <christian.koenig@amd.com>

diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index 8e40ba4a9b79db12c38096aa7d690ba4d8283dc7..765bd1f2e1ceefd5d540621ff1db90e4fc0f7d0d 100644 (file)
--- 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 aced97bb79e090d3502a2145dcbea232ca798f20..b487c69a486d443ab4602977728e87cb5edbe3d9 100644 (file)
--- 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);
-       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);
+restart_ih:
+       /* is somebody else already processing irqs? */
+       if (atomic_xchg(&rdev->ih.lock, 1))
                return IRQ_NONE;
-       }
 
-restart_ih:
+       rptr = rdev->ih.rptr;
+       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;
@@ -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 9d6c1de93591d4bb4d125ba621c454fedf133424..5ddf895a59267f34290ed61b7f31bd04d2e6ba78 100644 (file)
--- 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 7667184ab0bf5edc2a67e0277f2e179368e62927..3c563d1f9671390ada698d6b3de8fc6502b9d0fb 100644 (file)
--- 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 8868a1fa20e5f6579d2868d8f6f2c51856f78cd6..ecef972050d1b98d9deeaefa49d47ec9ec6d6983 100644 (file)
--- 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;
 }