Re: [PATCH 3/6] drm/i915: Wake up pending_flip_queue as part of reset handling

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jan 29, 2013 at 06:13:35PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Someone may be waiting for a flip that will never complete due to a GPU
> reset. Wake up all such waiters when the hang is first detected, and
> after the reset processing has finished.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 13bb8d3..8b1146b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -915,6 +915,8 @@ static void i915_error_work_func(struct work_struct *work)
>  		for_each_ring(ring, dev_priv, i)
>  			wake_up_all(&ring->irq_queue);
>  
> +		wake_up_all(&dev_priv->pending_flip_queue);
> +
>  		wake_up_all(&dev_priv->gpu_error.reset_queue);
>  	}
>  }
> @@ -1540,6 +1542,8 @@ void i915_handle_error(struct drm_device *dev, bool wedged)
>  		 */
>  		for_each_ring(ring, dev_priv, i)
>  			wake_up_all(&ring->irq_queue);
> +
> +		wake_up_all(&dev_priv->pending_flip_queue);

I don't quite follow why we need this one here. The wake up for the ring
irq queues is to get people off the dev->struct_mutex lock, which the
reset handler needs to acquire to do its job. But process stalling for
pageflips to complete already can't hold dev->struct_mutex, since
otherwise they'd block out the pageflip completion works and so would
deadlock already.

Is there another reason why we need to kick waiters before the reset has
completed that I'm missing here? One potential issue would be clarifying
what exactly happens when the gpu hangs while pageflipping, i.e. whether
the kernel should go out of it's way and re-issue the pageflip if it died
meanwhile. But even that could be scheduled from a workqueue I think. And
even if that'd require us to kick waiters of the pending flip queue I'd
prefer to only add it once we really need it (plus a big comment
explaining the tricks).
-Daniel

>  	}
>  
>  	queue_work(dev_priv->wq, &dev_priv->gpu_error.work);
> -- 
> 1.7.12.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@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