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

[Chris Wilson]

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>

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