[Intel-gfx] [PATCH] drm/i916: fix wait_for_pending_flips vs gpu hang deadlock

[Daniel Vetter]

On Sun, Sep 8, 2013 at 3:20 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> +static void i915_error_wake_up(struct drm_i915_private *dev_priv,
>> +                            bool reset_completed)
>> +{
>> +     struct intel_ring_buffer *ring;
>> +     int i;
>
> Might as well make that memory barrier explicit:
> smb_mb__after_atomic_inc();
>
> The cost of an additional mb is insignificant here.

Imo the barrier nature between waker and wakee is established enough
that we don't need to throw another barrier in here.

> Something like:
>
> /* Notify all waiters for GPU completion events that reset state has
>  * been changed, and that they need to restart their wait after
>  * checking for potential errors.
>  */
>
> I don't mind repeating ourselves as this logic impacts faraway logic in
> i915_gem and intel_display etc.
>
> /* Wakeup __wait_seqno() [dev->struct_mutex] */
>> +     for_each_ring(ring, dev_priv, i)
>> +             wake_up_all(&ring->irq_queue);
>> +
>
> /* Wakeup intel_crtc_wait_for_pending_flips() [dev->mode_config.lock] */

Actually we only care about crtc->mutex.

>> +     wake_up_all(&dev_priv->pending_flip_queue);
>> +
>
> /* Wakeup our error checking mutex after we have finished reseting the GPU,
>    i915_gem_wait_for_error() [no locks held by the waiters] */
>
>> +     if (reset_completed)
>> +             wake_up_all(&dev_priv->gpu_error.reset_queue);
>> +}
>
> With some additional comments to drive home i915_error_wake_up(),
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

Applied the additional comments with slightly adjusted colors to make
it clear that we care about the unlocking of the waiters so that the
error handler can acquire the locks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch