Re: [PATCH] drm/i915: Fix __wait_seqno to use true infinite timeouts

[Rodrigo Vivi <rodrigo.vivi@gmail.com>]

applied to experimental drm-intel-collector.

drivers/gpu/drm/i915/i915_gem.c: In function ‘__wait_seqno’:
drivers/gpu/drm/i915/i915_gem.c:1033:20: warning: ‘timeout_jiffies’ may be used
+uninitialized in this function



On Tue, Aug 6, 2013 at 10:03 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Rather than continue to fix up the timeouts to work around the interface
> impedence in wait_event_*(), open code the combination of
> wait_event[_interruptible][_timeout]. And note the code size reduction,
> and dare say readability?, in doing so..
>
> Food for thought.
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 82 ++++++++++++++++++++---------------------
>  1 file changed, 39 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 99ba622..8adbce9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1088,23 +1088,18 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
>                         bool interruptible, struct timespec *timeout)
>  {
>         drm_i915_private_t *dev_priv = ring->dev->dev_private;
> -       struct timespec before, now, wait_time={1,0};
> -       unsigned long timeout_jiffies;
> -       long end;
> -       bool wait_forever = true;
> +       struct timespec before, now;
> +       DEFINE_WAIT(__wait);
> +       long timeout_jiffies;
>         int ret;
>
>         if (i915_seqno_passed(ring->get_seqno(ring, true), seqno))
>                 return 0;
>
> -       trace_i915_gem_request_wait_begin(ring, seqno);
> -
> -       if (timeout != NULL) {
> -               wait_time = *timeout;
> -               wait_forever = false;
> -       }
> +       if (timeout)
> +               timeout_jiffies = timespec_to_jiffies_timeout(timeout);
>
> -       timeout_jiffies = timespec_to_jiffies_timeout(&wait_time);
> +       trace_i915_gem_request_wait_begin(ring, seqno);
>
>         if (WARN_ON(!ring->irq_get(ring)))
>                 return -ENODEV;
> @@ -1112,36 +1107,47 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
>         /* Record current time in case interrupted by signal, or wedged * */
>         getrawmonotonic(&before);
>
> -#define EXIT_COND \
> -       (i915_seqno_passed(ring->get_seqno(ring, false), seqno) || \
> -        i915_reset_in_progress(&dev_priv->gpu_error) || \
> -        reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
> -       do {
> -               if (interruptible)
> -                       end = wait_event_interruptible_timeout(ring->irq_queue,
> -                                                              EXIT_COND,
> -                                                              timeout_jiffies);
> -               else
> -                       end = wait_event_timeout(ring->irq_queue, EXIT_COND,
> -                                                timeout_jiffies);
> +       for (;;) {
> +               prepare_to_wait(&ring->irq_queue, &__wait,
> +                               interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
>
>                 /* We need to check whether any gpu reset happened in between
>                  * the caller grabbing the seqno and now ... */
> -               if (reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
> -                       end = -EAGAIN;
> +               if (reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter)) {
> +                       /* ... but upgrade the -EGAIN to an -EIO if the gpu
> +                        * is truely gone. */
> +                       ret = i915_gem_check_wedge(&dev_priv->gpu_error, interruptible);
> +                       if (ret == 0)
> +                               ret = -EAGAIN;
> +                       break;
> +               }
>
> -               /* ... but upgrade the -EGAIN to an -EIO if the gpu is truely
> -                * gone. */
> -               ret = i915_gem_check_wedge(&dev_priv->gpu_error, interruptible);
> -               if (ret)
> -                       end = ret;
> -       } while (end == 0 && wait_forever);
> +               if (i915_seqno_passed(ring->get_seqno(ring, false), seqno)) {
> +                       ret = 0;
> +                       break;
> +               }
> +
> +               if (interruptible && signal_pending(current)) {
> +                       ret = -ERESTARTSYS;
> +                       break;
> +               }
> +
> +               if (timeout) {
> +                       if (timeout_jiffies == 0) {
> +                               ret = -ETIME;
> +                               break;
> +                       }
> +
> +                       timeout_jiffies = schedule_timeout(timeout_jiffies);
> +               } else
> +                       schedule();
> +       }
> +       finish_wait(&ring->irq_queue, &__wait);
>
>         getrawmonotonic(&now);
>
>         ring->irq_put(ring);
>         trace_i915_gem_request_wait_end(ring, seqno);
> -#undef EXIT_COND
>
>         if (timeout) {
>                 struct timespec sleep_time = timespec_sub(now, before);
> @@ -1150,17 +1156,7 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
>                         set_normalized_timespec(timeout, 0, 0);
>         }
>
> -       switch (end) {
> -       case -EIO:
> -       case -EAGAIN: /* Wedged */
> -       case -ERESTARTSYS: /* Signal */
> -               return (int)end;
> -       case 0: /* Timeout */
> -               return -ETIME;
> -       default: /* Completed */
> -               WARN_ON(end < 0); /* We're not aware of other errors */
> -               return 0;
> -       }
> +       return ret;
>  }
>
>  /**
> --
> 1.8.4.rc1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br