[PATCH] drm/radeon: fix race condition in radeon_crtc_page_flip

[PATCH] drm/radeon: fix race condition in radeon_crtc_page_flip

[Christian König]

From: Christian König <christian.koenig@amd.com>

radeon_crtc_handle_flip can be called concurrently and if
we set the unpin_work to early try to flip an unpinned BO or
worse.

Signed-off-by: Christian König <christian.koenig@amd.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/radeon/radeon_display.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 356b733..cf22741 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -393,17 +393,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
 
 	INIT_WORK(&work->work, radeon_unpin_work_func);
 
-	/* We borrow the event spin lock for protecting unpin_work */
-	spin_lock_irqsave(&dev->event_lock, flags);
-	if (radeon_crtc->unpin_work) {
-		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
-		r = -EBUSY;
-		goto unlock_free;
-	}
-	radeon_crtc->unpin_work = work;
-	radeon_crtc->deferred_flip_completion = 0;
-	spin_unlock_irqrestore(&dev->event_lock, flags);
-
 	/* pin the new buffer */
 	DRM_DEBUG_DRIVER("flip-ioctl() cur_fbo = %p, cur_bbo = %p\n",
 			 work->old_rbo, rbo);
@@ -461,10 +450,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
 		base &= ~7;
 	}
 
-	spin_lock_irqsave(&dev->event_lock, flags);
-	work->new_crtc_base = base;
-	spin_unlock_irqrestore(&dev->event_lock, flags);
-
 	/* update crtc fb */
 	crtc->primary->fb = fb;
 
@@ -477,6 +462,22 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
 	/* set the proper interrupt */
 	radeon_pre_page_flip(rdev, radeon_crtc->crtc_id);
 
+	/* We borrow the event spin lock for protecting unpin_work */
+	spin_lock_irqsave(&dev->event_lock, flags);
+	if (radeon_crtc->unpin_work) {
+		spin_unlock_irqrestore(&dev->event_lock, flags);
+		radeon_post_page_flip(rdev, radeon_crtc->crtc_id);
+		drm_vblank_put(dev, radeon_crtc->crtc_id);
+
+		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
+		r = -EBUSY;
+		goto pflip_cleanup1;
+	}
+	radeon_crtc->unpin_work = work;
+	radeon_crtc->deferred_flip_completion = 0;
+	work->new_crtc_base = base;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
 	return 0;
 
 pflip_cleanup1:
-- 
1.9.1

Re: [PATCH] drm/radeon: fix race condition in radeon_crtc_page_flip

[Michel Dänzer]

This patch only applies to 3.15, right?


On 19.06.2014 02:11, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
> 
> radeon_crtc_handle_flip can be called concurrently and if
> we set the unpin_work to early try to flip an unpinned BO or
> worse.

Spelling: 'too early'

Maybe something like:

radeon_crtc_handle_flip can be called concurrently, and if
we set the unpin_work too early, it may try to flip an unpinned BO or
worse.


> Signed-off-by: Christian König <christian.koenig@amd.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/radeon/radeon_display.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index 356b733..cf22741 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -393,17 +393,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
>  
>  	INIT_WORK(&work->work, radeon_unpin_work_func);
>  
> -	/* We borrow the event spin lock for protecting unpin_work */
> -	spin_lock_irqsave(&dev->event_lock, flags);
> -	if (radeon_crtc->unpin_work) {
> -		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
> -		r = -EBUSY;
> -		goto unlock_free;
> -	}
> -	radeon_crtc->unpin_work = work;
> -	radeon_crtc->deferred_flip_completion = 0;
> -	spin_unlock_irqrestore(&dev->event_lock, flags);
> -
>  	/* pin the new buffer */
>  	DRM_DEBUG_DRIVER("flip-ioctl() cur_fbo = %p, cur_bbo = %p\n",
>  			 work->old_rbo, rbo);
> @@ -461,10 +450,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
>  		base &= ~7;
>  	}
>  
> -	spin_lock_irqsave(&dev->event_lock, flags);
> -	work->new_crtc_base = base;
> -	spin_unlock_irqrestore(&dev->event_lock, flags);
> -
>  	/* update crtc fb */
>  	crtc->primary->fb = fb;
>  
> @@ -477,6 +462,22 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
>  	/* set the proper interrupt */
>  	radeon_pre_page_flip(rdev, radeon_crtc->crtc_id);
>  
> +	/* We borrow the event spin lock for protecting unpin_work */
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	if (radeon_crtc->unpin_work) {
> +		spin_unlock_irqrestore(&dev->event_lock, flags);
> +		radeon_post_page_flip(rdev, radeon_crtc->crtc_id);
> +		drm_vblank_put(dev, radeon_crtc->crtc_id);
> +
> +		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
> +		r = -EBUSY;
> +		goto pflip_cleanup1;
> +	}
> +	radeon_crtc->unpin_work = work;
> +	radeon_crtc->deferred_flip_completion = 0;
> +	work->new_crtc_base = base;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +

This introduces a path where crtc->primary->fb is updated, but then we
return -EBUSY.


It also introduces a warning:

drivers/gpu/drm/radeon/radeon_display.c: In function ‘radeon_crtc_page_flip’:
drivers/gpu/drm/radeon/radeon_display.c:496:1: warning: label ‘unlock_free’ defined but not used [-Wunused-label]
 unlock_free:
 ^


Apart from that, looks good.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: fix race condition in radeon_crtc_page_flip

[Christian König]

Am 19.06.2014 05:49, schrieb Michel Dänzer:
> This patch only applies to 3.15, right?

Yes correct. For 3.16 we have the reworked flip which I think is still a 
good idea to keep for now.

I've addresses your comments and send out a v2 to the list CCing you, 
please review.

Thanks,
Christian.

>
>
> On 19.06.2014 02:11, Christian König wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> radeon_crtc_handle_flip can be called concurrently and if
>> we set the unpin_work to early try to flip an unpinned BO or
>> worse.
> Spelling: 'too early'
>
> Maybe something like:
>
> radeon_crtc_handle_flip can be called concurrently, and if
> we set the unpin_work too early, it may try to flip an unpinned BO or
> worse.
>
>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Cc: stable@vger.kernel.org
>> ---
>>   drivers/gpu/drm/radeon/radeon_display.c | 31 ++++++++++++++++---------------
>>   1 file changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
>> index 356b733..cf22741 100644
>> --- a/drivers/gpu/drm/radeon/radeon_display.c
>> +++ b/drivers/gpu/drm/radeon/radeon_display.c
>> @@ -393,17 +393,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
>>   
>>   	INIT_WORK(&work->work, radeon_unpin_work_func);
>>   
>> -	/* We borrow the event spin lock for protecting unpin_work */
>> -	spin_lock_irqsave(&dev->event_lock, flags);
>> -	if (radeon_crtc->unpin_work) {
>> -		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
>> -		r = -EBUSY;
>> -		goto unlock_free;
>> -	}
>> -	radeon_crtc->unpin_work = work;
>> -	radeon_crtc->deferred_flip_completion = 0;
>> -	spin_unlock_irqrestore(&dev->event_lock, flags);
>> -
>>   	/* pin the new buffer */
>>   	DRM_DEBUG_DRIVER("flip-ioctl() cur_fbo = %p, cur_bbo = %p\n",
>>   			 work->old_rbo, rbo);
>> @@ -461,10 +450,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
>>   		base &= ~7;
>>   	}
>>   
>> -	spin_lock_irqsave(&dev->event_lock, flags);
>> -	work->new_crtc_base = base;
>> -	spin_unlock_irqrestore(&dev->event_lock, flags);
>> -
>>   	/* update crtc fb */
>>   	crtc->primary->fb = fb;
>>   
>> @@ -477,6 +462,22 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
>>   	/* set the proper interrupt */
>>   	radeon_pre_page_flip(rdev, radeon_crtc->crtc_id);
>>   
>> +	/* We borrow the event spin lock for protecting unpin_work */
>> +	spin_lock_irqsave(&dev->event_lock, flags);
>> +	if (radeon_crtc->unpin_work) {
>> +		spin_unlock_irqrestore(&dev->event_lock, flags);
>> +		radeon_post_page_flip(rdev, radeon_crtc->crtc_id);
>> +		drm_vblank_put(dev, radeon_crtc->crtc_id);
>> +
>> +		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
>> +		r = -EBUSY;
>> +		goto pflip_cleanup1;
>> +	}
>> +	radeon_crtc->unpin_work = work;
>> +	radeon_crtc->deferred_flip_completion = 0;
>> +	work->new_crtc_base = base;
>> +	spin_unlock_irqrestore(&dev->event_lock, flags);
>> +
> This introduces a path where crtc->primary->fb is updated, but then we
> return -EBUSY.
>
>
> It also introduces a warning:
>
> drivers/gpu/drm/radeon/radeon_display.c: In function ‘radeon_crtc_page_flip’:
> drivers/gpu/drm/radeon/radeon_display.c:496:1: warning: label ‘unlock_free’ defined but not used [-Wunused-label]
>   unlock_free:
>   ^
>
>
> Apart from that, looks good.
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel