[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: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.
-Chris