[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 at 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 at amd.com>
>> Cc: stable at 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.
>
>