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