diff mbox

drm/i915: don't set unpin_work if vblank_get fails

Message ID 20110822110531.57d85c32@jbarnes-desktop (mailing list archive)
State New, archived
Headers show

Commit Message

Jesse Barnes Aug. 22, 2011, 6:05 p.m. UTC
This fixes a race where we may try to finish a page flip and decrement
the refcount even if our vblank_get failed and we ended up with a
spurious flip pending interrupt.

Fixes https://bugs.freedesktop.org/show_bug.cgi?id=34211.

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

Comments

Keith Packard Aug. 22, 2011, 6:45 p.m. UTC | #1
On Mon, 22 Aug 2011 11:05:31 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> This fixes a race where we may try to finish a page flip and decrement
> the refcount even if our vblank_get failed and we ended up with a
> spurious flip pending interrupt.
> 
> Fixes https://bugs.freedesktop.org/show_bug.cgi?id=34211.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2319f62..0910537 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6896,6 +6896,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	work->old_fb_obj = intel_fb->obj;
>  	INIT_WORK(&work->work, intel_unpin_work_fn);
>  
> +	ret = drm_vblank_get(dev, intel_crtc->pipe);
> +	if (ret)
> +		goto free_work;
> +
>  	/* We borrow the event spin lock for protecting unpin_work */
>  	spin_lock_irqsave(&dev->event_lock, flags);
>  	if (intel_crtc->unpin_work) {
> @@ -6906,6 +6910,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  		return -EBUSY;

You'll need a drm_vblank_put above this return.

>  	}
>  	intel_crtc->unpin_work = work;
> +	/*
> +	 * Past this point, if we fail we'll let the flip completion code
> +	 * clean up the vblank refcount and pin work.  It'll be a spurious
> +	 * completion, but we handle that case.
> +	 */

I don't see how this is going to happen reliably; the hardware will have
to generate a suitable interrupt, which on IRL and later will have to be
an actual page flip interrupt.
Jesse Barnes Aug. 29, 2011, 4:44 p.m. UTC | #2
On Mon, 22 Aug 2011 11:05:31 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> This fixes a race where we may try to finish a page flip and decrement
> the refcount even if our vblank_get failed and we ended up with a
> spurious flip pending interrupt.
> 
> Fixes https://bugs.freedesktop.org/show_bug.cgi?id=34211.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Updated patch.  Though now I've lost track of the outstanding
issues...  IIRC we still have another race here, but it's unrelated to
the one in the bug report.

Jesse
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2319f62..0910537 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6896,6 +6896,10 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	work->old_fb_obj = intel_fb->obj;
 	INIT_WORK(&work->work, intel_unpin_work_fn);
 
+	ret = drm_vblank_get(dev, intel_crtc->pipe);
+	if (ret)
+		goto free_work;
+
 	/* We borrow the event spin lock for protecting unpin_work */
 	spin_lock_irqsave(&dev->event_lock, flags);
 	if (intel_crtc->unpin_work) {
@@ -6906,6 +6910,11 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		return -EBUSY;
 	}
 	intel_crtc->unpin_work = work;
+	/*
+	 * Past this point, if we fail we'll let the flip completion code
+	 * clean up the vblank refcount and pin work.  It'll be a spurious
+	 * completion, but we handle that case.
+	 */
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
 	intel_fb = to_intel_framebuffer(fb);
@@ -6919,10 +6928,6 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 
 	crtc->fb = fb;
 
-	ret = drm_vblank_get(dev, intel_crtc->pipe);
-	if (ret)
-		goto cleanup_objs;
-
 	work->pending_flip_obj = obj;
 
 	work->enable_stall_check = true;
@@ -6945,7 +6950,6 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 
 cleanup_pending:
 	atomic_sub(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip);
-cleanup_objs:
 	drm_gem_object_unreference(&work->old_fb_obj->base);
 	drm_gem_object_unreference(&obj->base);
 	mutex_unlock(&dev->struct_mutex);
@@ -6953,7 +6957,7 @@  cleanup_objs:
 	spin_lock_irqsave(&dev->event_lock, flags);
 	intel_crtc->unpin_work = NULL;
 	spin_unlock_irqrestore(&dev->event_lock, flags);
-
+free_work:
 	kfree(work);
 
 	return ret;