Re: [PATCH 02/11] drm/i915: Hold the spinlock whilst resetting unpin_work along error path

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Thu, 27 May 2010 13:18:13 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Delay taking the mutex until we need to and ensure that we hold the
> spinlock when resetting unpin_work on the error path. Also defer the
> debugging print messages until after we have released the spinlock.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Kristian Høgsberg <krh@bitplanet.net>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   20 ++++++++++++--------
>  1 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cfac4dd..1845a06 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4667,8 +4667,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	if (work == NULL)
>  		return -ENOMEM;
>  
> -	mutex_lock(&dev->struct_mutex);
> -
>  	work->event = event;
>  	work->dev = crtc->dev;
>  	intel_fb = to_intel_framebuffer(crtc->fb);
> @@ -4678,10 +4676,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	/* We borrow the event spin lock for protecting unpin_work */
>  	spin_lock_irqsave(&dev->event_lock, flags);
>  	if (intel_crtc->unpin_work) {
> -		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
>  		spin_unlock_irqrestore(&dev->event_lock, flags);
>  		kfree(work);
> -		mutex_unlock(&dev->struct_mutex);
> +
> +		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
>  		return -EBUSY;
>  	}
>  	intel_crtc->unpin_work = work;
> @@ -4690,13 +4688,19 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	intel_fb = to_intel_framebuffer(fb);
>  	obj = intel_fb->obj;
>  
> +	mutex_lock(&dev->struct_mutex);
>  	ret = intel_pin_and_fence_fb_obj(dev, obj);
>  	if (ret != 0) {
> -		DRM_DEBUG_DRIVER("flip queue: %p pin & fence failed\n",
> -			  to_intel_bo(obj));
> -		kfree(work);
> -		intel_crtc->unpin_work = NULL;
>  		mutex_unlock(&dev->struct_mutex);
> +
> +		spin_lock_irqsave(&dev->event_lock, flags);
> +		intel_crtc->unpin_work = NULL;
> +		spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +		kfree(work);
> +
> +		DRM_DEBUG_DRIVER("flip queue: %p pin & fence failed\n",
> +				 to_intel_bo(obj));
>  		return ret;
>  	}

The mutex change looks like a good cleanup, and the spin lock addition
is a good fix.  Thanks.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx