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

["Michel Dänzer" <michel@daenzer.net>]

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