Quoting Mika Kuoppala (2018-07-09 12:52:51)
> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
>
> > Quoting Mika Kuoppala (2018-07-09 12:38:10)
> >> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
Added the timeout info to the GEM_TRACE and pushed. Thanks for the review,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
