[PATCH 08/10] drm/radeon: replace pflip and sw_int counters with atomics

[Michel Dänzer]

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