On Mon, Mar 30, 2015 at 02:52:26PM +0100, Tvrtko Ursulin wrote:
> >+static void
> >+__i915_gem_request_retire__upto(struct drm_i915_gem_request *rq)
>
> It is a bit annoying (for readability) that it can be rq, req and request.
Nonsense they are all rq and struct i915_request. Or once have been and
so will again. /prophecy
> >+err:
> >+ for (i = 0; i < n; i++) {
> >+ if (ret == 0) {
> >+ int ring = requests[i]->ring->id;
> >+ if (obj->last_read_req[ring] == requests[i])
> >+ i915_gem_object_retire__read(obj, ring);
> >+ if (obj->last_write_req == requests[i])
> >+ i915_gem_object_retire__write(obj);
>
> Above four lines seem to be identical functionality to similar four
> in __i915_gem_object_sync.
Yes. Extracting them ended up looking worse (imo).
> Also, _retire__read will do _retire__write if there is one on the
> same ring. And here by definition they are since it is the same
> request, no?
No. It's subtle but here is the bug I pointed out from before. Once we
drop the lock, we no longer can make assumptions about the state of obj.
> >@@ -2698,16 +2747,13 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
> > if (!i915_gem_request_completed(request, true))
> > break;
> >
> >- trace_i915_gem_request_retire(request);
> >-
> > /* We know the GPU must have read the request to have
> > * sent us the seqno + interrupt, so use the position
> > * of tail of the request to update the last known position
> > * of the GPU head.
> > */
> > request->ringbuf->last_retired_head = request->postfix;
> >-
> >- i915_gem_free_request(request);
> >+ i915_gem_request_retire(request);
> > }
>
> This loop could also use __i915_gem_request_retire__upto if it found
> the first completed request first. Not sure how much code would that
> save but would maube be more readable, a little bit more self
> documenting.
Actually this loop here should be pushed back to the engine (as part of
later patches). After that transformation, using i915_gem_request_retire()
is even clearer. But _retire__upto does become the main way in which we
retire requests (having killed off retire_requests_ring in favour of
explict wait/poll+retire).
> So far it all looks reasonable to me, but apart from the comments
> above, I want to do another pass anyway.
There's a few more changes afoot as well (minor ones concerning
retire__upto and unexporting retire_requests_rig).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
