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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
