* [PATCH] drm/i915: don't set unpin_work if vblank_get fails
@ 2011-08-22 18:05 Jesse Barnes
2011-08-22 18:45 ` Keith Packard
2011-08-29 16:44 ` Jesse Barnes
0 siblings, 2 replies; 6+ messages in thread
From: Jesse Barnes @ 2011-08-22 18:05 UTC (permalink / raw)
To: intel-gfx
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;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915: don't set unpin_work if vblank_get fails
2011-08-22 18:05 [PATCH] drm/i915: don't set unpin_work if vblank_get fails Jesse Barnes
@ 2011-08-22 18:45 ` Keith Packard
2011-08-29 16:44 ` Jesse Barnes
1 sibling, 0 replies; 6+ messages in thread
From: Keith Packard @ 2011-08-22 18:45 UTC (permalink / raw)
To: Jesse Barnes, intel-gfx
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915: don't set unpin_work if vblank_get fails
2011-08-22 18:05 [PATCH] drm/i915: don't set unpin_work if vblank_get fails Jesse Barnes
2011-08-22 18:45 ` Keith Packard
@ 2011-08-29 16:44 ` Jesse Barnes
2011-08-29 16:45 ` Jesse Barnes
1 sibling, 1 reply; 6+ messages in thread
From: Jesse Barnes @ 2011-08-29 16:44 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: don't set unpin_work if vblank_get fails
2011-08-29 16:44 ` Jesse Barnes
@ 2011-08-29 16:45 ` Jesse Barnes
2011-09-20 4:04 ` Keith Packard
0 siblings, 1 reply; 6+ messages in thread
From: Jesse Barnes @ 2011-08-29 16:45 UTC (permalink / raw)
Cc: intel-gfx
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;
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-11-11 20:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-22 18:05 [PATCH] drm/i915: don't set unpin_work if vblank_get fails Jesse Barnes
2011-08-22 18:45 ` Keith Packard
2011-08-29 16:44 ` Jesse Barnes
2011-08-29 16:45 ` Jesse Barnes
2011-09-20 4:04 ` Keith Packard
2011-11-11 20:36 ` Jesse Barnes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).