On Sun, Sep 8, 2013 at 3:20 PM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html