[Intel-gfx] [PATCH 1/3] drm/i915: Provide a timeout to i915_gem_wait_for_idle()

[Chris Wilson]

Quoting Mika Kuoppala (2018-07-09 12:52:51)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > Quoting Mika Kuoppala (2018-07-09 12:38:10)
> >> Chris Wilson <chris at chris-wilson.co.uk> writes:
> >> > -int i915_gem_wait_for_idle(struct drm_i915_private *i915, unsigned int flags)
> >> > +int i915_gem_wait_for_idle(struct drm_i915_private *i915,
> >> > +                        unsigned int flags, long timeout)
> >> >  {
> >> >       GEM_TRACE("flags=%x (%s)\n",
> >> >                 flags, flags & I915_WAIT_LOCKED ? "locked" : "unlocked");
> >> 
> >> Did you omit enhancing the trace on purpose?
> >
> > Didn't even consider it. I personally only use it as a breadcrumb in the
> > trace.
> >  
> >> Eventually i915_request_wait will assert for silly timeout values, but
> >> should we add assertion here too as wait_for_timeline will
> >> return what we put, for nonactive timelines?
> >> 
> >> > @@ -3798,9 +3801,9 @@ int i915_gem_wait_for_idle(struct drm_i915_private *i915, unsigned int flags)
> >> >               lockdep_assert_held(&i915->drm.struct_mutex);
> >> >  
> >> >               list_for_each_entry(tl, &i915->gt.timelines, link) {
> >> > -                     err = wait_for_timeline(tl, flags);
> >> > -                     if (err)
> >> > -                             return err;
> >> > +                     timeout = wait_for_timeline(tl, flags, timeout);
> >> > +                     if (timeout < 0)
> >> > +                             return timeout;
> >> >               }
> >> >  
> >> >               err = wait_for_engines(i915);
> >> 
> >> To truely serve the caller, the remaining timeout could be passed to
> >> wait_for_engines too, but that can be followup.
> >
> > I don't think so. The timeout we employ there is specific to the CSB
> > delay and we need to employ that irrespective of the caller timeout.
> > At this point, we have successfully waited for all requests, and now
> > just have to drain any HW queues -- imo, we have completed the task the
> > caller set out for us (requests completed within the timeout). And now
> > onto the followup retirement optimisations.
> 
> Agreed, my mistake. At that point all the requests are already done.
> If the engine hw flush timeouts, that is catastrophic, regardless
> of caller preferences.
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>

Added the timeout info to the GEM_TRACE and pushed. Thanks for the review,
-Chris