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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2018-07-09 12:38:10)
>> Chris Wilson <chris@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@linux.intel.com>

> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx