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

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

[Jesse Barnes]

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;
 	}
 	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;

Re: [PATCH] drm/i915: don't set unpin_work if vblank_get fails

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1645 bytes --]

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.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: don't set unpin_work if vblank_get fails

[Jesse Barnes]

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

Re: [PATCH] drm/i915: don't set unpin_work if vblank_get fails

[Jesse Barnes]

On Mon, 29 Aug 2011 09:44:52 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> 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.

With patch...

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2319f62..9cd523b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6896,11 +6896,16 @@ 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) {
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 		kfree(work);
+		drm_vblank_put(dev, intel_crtc->pipe);
 
 		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
 		return -EBUSY;
@@ -6919,10 +6924,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 +6946,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);
@@ -6954,6 +6954,8 @@ cleanup_objs:
 	intel_crtc->unpin_work = NULL;
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
+	drm_vblank_put(dev, intel_crtc->pipe);
+free_work:
 	kfree(work);
 
 	return ret;

Re: [PATCH] drm/i915: don't set unpin_work if vblank_get fails

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 240 bytes --]

On Mon, 29 Aug 2011 09:45:28 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> With patch...

It's been a couple of weeks; this patch is ready to merge, yes? Is there
a commit message somewhere?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: don't set unpin_work if vblank_get fails

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 463 bytes --]

On Mon, 19 Sep 2011 21:04:36 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Mon, 29 Aug 2011 09:45:28 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > With patch...
> 
> It's been a couple of weeks; this patch is ready to merge, yes? Is there
> a commit message somewhere?

Yeah higher up in the message... we should probably get this committed
now if people are ok with it.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx