On Don, 2012-05-31 at 22:16 +0200, Christian König wrote:
>
> So we can skip the looking.
'locking'
> diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> index 73cd0fd..52f85ba 100644
> --- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> @@ -87,16 +87,7 @@ void radeon_driver_irq_preinstall_kms(struct drm_device *dev)
>
> int radeon_driver_irq_postinstall_kms(struct drm_device *dev)
> {
> - struct radeon_device *rdev = dev->dev_private;
> - unsigned long irqflags;
> - unsigned i;
> -
> dev->max_vblank_count = 0x001fffff;
> - spin_lock_irqsave(&rdev->irq.lock, irqflags);
> - for (i = 0; i < RADEON_NUM_RINGS; i++)
> - rdev->irq.sw_int[i] = true;
> - radeon_irq_set(rdev);
> - spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> return 0;
> }
Why does this function no longer need to enable SW interrupts? If it
really doesn't, that might be material for a separate patch.
> @@ -225,25 +216,28 @@ void radeon_irq_kms_sw_irq_get(struct radeon_device *rdev, int ring)
> {
> unsigned long irqflags;
>
> - spin_lock_irqsave(&rdev->irq.lock, irqflags);
> - if (rdev->ddev->irq_enabled && (++rdev->irq.sw_refcount[ring] == 1)) {
> - rdev->irq.sw_int[ring] = true;
> + if (!rdev->ddev->irq_enabled)
> + return;
> +
> + if (atomic_inc_return(&rdev->irq.ring_int[ring]) == 1) {
> + spin_lock_irqsave(&rdev->irq.lock, irqflags);
> radeon_irq_set(rdev);
> + spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> }
> - spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> }
>
> void radeon_irq_kms_sw_irq_put(struct radeon_device *rdev, int ring)
> {
> unsigned long irqflags;
>
> - spin_lock_irqsave(&rdev->irq.lock, irqflags);
> - BUG_ON(rdev->ddev->irq_enabled && rdev->irq.sw_refcount[ring] <= 0);
> - if (rdev->ddev->irq_enabled && (--rdev->irq.sw_refcount[ring] == 0)) {
> - rdev->irq.sw_int[ring] = false;
> + if (!rdev->ddev->irq_enabled)
> + return;
> +
> + if (atomic_dec_and_test(&rdev->irq.ring_int[ring])) {
> + spin_lock_irqsave(&rdev->irq.lock, irqflags);
> radeon_irq_set(rdev);
> + spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> }
> - spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> }
>
> void radeon_irq_kms_pflip_irq_get(struct radeon_device *rdev, int crtc)
I think this might introduce a race condition:
Thread 0 Thread 1
-------- --------
atomic_inc_return() returns 1
spin_lock_irqsave()
atomic_dec_and_test()
radeon_irq_set()
=> the interrupt won't be enabled.
Maybe this explains why you couldn't remove the spinlock in patch 6?
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
