On Thu, May 24, 2012 at 09:49:10AM +0200, Christian K?nig wrote:
> From: Christian Koenig <christian.koenig at 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 at amd.com>
Reviewed-by: Jerome Glisse <jglisse at 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
>
