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@xxxxxxxxxxxx
> > Reviewed-by: marcheu@xxxxxxxxxxxx
>
> 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@xxxxxxxxxxxxxxxxxx>
Actually this is a regression from
commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
Author: Daniel Vetter <daniel.vetter@xxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
