| Message ID | 20110822110531.57d85c32@jbarnes-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 --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;
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>