[Intel-gfx] [PATCH 17/49] drm/i915: Implement inter-engine read-read optimisations

[Chris Wilson]

On Mon, Mar 30, 2015 at 03:45:04PM +0100, Tvrtko Ursulin wrote:
> 
> On 03/30/2015 03:09 PM, Chris Wilson wrote:
> >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
> 
> rq is least spread in the codebase, and even the worst option of the
> three since it sounds like a queue of some sort.

It's much clearer than req, matches equivalent implementations in
userspace ;-) rq is the local variable, and request the verbose version
for structure memers.

> >>>+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).
> 
> It would be a single function call taking object and request, how
> can it be worse? Should be more readable with a good name.

I wanted i915_gem_object_retire_request. However,  done.
 
> >>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.
> 
> You mean request might have disappeared from last_read_req but is
> still on last_write_req? But how, since if it was retired that
> shouldn't be possible.

Just that last_write_req != last_read_req either before or after the
wait (and doubly so after the wait, which is where we had the previous
bug).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre