[Intel-gfx] [PATCH] Avoid i915 flip unpin/HPD event handler deadlock.

[Daniel Vetter]

On Sat, Aug 31, 2013 at 10:15:25AM +0100, Chris Wilson wrote:
> On Fri, Aug 30, 2013 at 06:30:55PM -0700, Stuart Abercrombie wrote:
> > Both of these were taking the mode_config mutex but executed from the
> > same work queue.  If intel_crtc_page_flip happened to flush a work queue
> > containing an HPD event handler work item, deadlock resulted, since the
> > mutex required by the HPD handler was taken before the flush.  Instead
> > use a separate work queue for the flip unpin work.
> > 
> > Signed-off-by: sabercrombie at chromium.org
> > Reviewed-by: marcheu at chromium.org
> 
> It would be possible to rearrange the flip to drop the lock around the
> flush (which is a regression from the kernel/workqueue.c refacting...).
> However, this looks much simpler. In the long run being strict on
> calling flush_workqueue() unlocked is likely to be safer though.
> 
> Reviewed-by; Chris Wilson <chris at chris-wilson.co.uk>

Actually this is a regression from

commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
Author: Daniel Vetter <daniel.vetter at ffwll.ch>
Date:   Tue Oct 23 18:23:34 2012 +0000

    drm: run the hpd irq event code directly

and the fix is a bit simpler.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c      | 21 ++++++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> >  drivers/gpu/drm/i915/intel_display.c |  4 ++--
> >  3 files changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 4f129bb..9215360 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1558,6 +1558,22 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  		goto out_mtrrfree;
> >  	}
> >  
> > +	/* intel_crtc_page_flip runs with the mode_config mutex having been
> > +	 * taken in the DRM layer.  It synchronously waits for pending unpin
> > +	 * work items while holding this mutex.  Therefore this queue cannot
> > +	 * contain work items that take this mutex, such as HPD event
> > +	 * handling, or we deadlock.  There is also no reason for flipping to
> > +	 * wait on such events.  Therefore put flip unpinning in its own
> > +	 * work queue.
> > +	 */
> > +	dev_priv->flip_unpin_wq = alloc_ordered_workqueue("i915", 0);
> > +	if (dev_priv->flip_unpin_wq == NULL) {
> > +		DRM_ERROR("Failed to create flip unpin workqueue.\n");
> > +		destroy_workqueue(dev_priv->wq);
> > +		ret = -ENOMEM;
> > +		goto out_mtrrfree;
> > +	}
> > +
> >  	/* This must be called before any calls to HAS_PCH_* */
> >  	intel_detect_pch(dev);
> >  
> > @@ -1628,6 +1644,7 @@ out_gem_unload:
> >  	intel_teardown_gmbus(dev);
> >  	intel_teardown_mchbar(dev);
> >  	destroy_workqueue(dev_priv->wq);
> > +	destroy_workqueue(dev_priv->flip_unpin_wq);
> 
> To be consistent, flip_wq then wq. In case we get ordering issues later.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch