Re: [PATCH v2 06/14] drm/i915: Protect the request->global_seqno with the engine->timeline lock

[Chris Wilson <chris@chris-wilson.co.uk>]

On Fri, Feb 17, 2017 at 02:43:05PM +0000, Tvrtko Ursulin wrote:
> 
> On 14/02/2017 09:54, Chris Wilson wrote:
> >@@ -947,7 +948,11 @@ bool __i915_spin_request(const struct drm_i915_gem_request *req,
> >
> > 	timeout_us += local_clock_us(&cpu);
> > 	do {
> >-		if (__i915_gem_request_completed(req))
> >+		if (seqno != i915_gem_request_global_seqno(req))
> >+			break;
> 
> You don't want to keep spinning for the allotted timeslice after the
> seqno transitions from zero to something and if is the currently
> executing seqno?

The intent was that we only spin for the active (on hw) request. If it
was removed from the execution queue, it is unlikely(?) to be put back
as the next request.

Yes, I'd love to spin out the timeslice, used to give some nice boosts
to some synmarks that kept hitting readbacks. Hopefully, they've been
fixed now to use gpu pipelines!

> >@@ -1073,7 +1081,8 @@ long i915_wait_request(struct drm_i915_gem_request *req,
> >
> > 		timeout = io_schedule_timeout(timeout);
> >
> >-		if (intel_wait_complete(&wait))
> >+		if (intel_wait_complete(&wait) &&
> >+		    i915_gem_request_global_seqno(req) == wait.seqno)
> > 			break;
> 
> Hm, the second part of the conditional sounds like it is always
> true. Since we know seqno is not zero, given that the first part of
> the wait has completed, so it has to be the expected one at this
> point. Otherwise a GEM_BUG_ON that it is different. Or I missed
> something?

Yes. We are preparing for global_seqno changing mid-wait due to request
reordering
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx