On Fri, Nov 25, 2016 at 12:31:03PM +0200, Joonas Lahtinen wrote:
> On pe, 2016-11-25 at 09:30 +0000, Chris Wilson wrote:
> > Currently, we have an active reference for the request until it is
> > retired. Though it cannot be retired before it has been executed by
> > hardware, the request may be completed before we have finished
> > processing the execute fence, i.e. we may continue to process that fence
> > as we free the request.
> >
> > Fixes: 5590af3e115a ("drm/i915: Drive request submission through fence callbacks")
> > Fixes: 23902e49c999 ("drm/i915: Split request submit/execute phase into two")
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>
> <SNIP>
>
> > @@ -551,8 +569,10 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
> > req->timeline->fence_context,
> > __timeline_get_seqno(req->timeline->common));
> >
> > - i915_sw_fence_init(&req->submit, submit_notify);
> > - i915_sw_fence_init(&req->execute, execute_notify);
> > + /* We bump the ref for the fence chain */
> > + i915_sw_fence_init(&i915_gem_request_get(req)->submit, submit_notify);
> > + i915_sw_fence_init(&i915_gem_request_get(req)->execute, execute_notify);
>
> I think having the gem_request_get in a separate line before fence_init
> and remove the comment. Make the code speak for itself instead of
> camouflaging it. Now it looks much like a type conversion helper.
I don't like the kref_init(); kref_get(); kref_get(); but that's the
interface we have, so yes I was trying to hide it as I think it is ugly.
And also because it pairs with the submit_notify, i.e. we pass a new
ref to sw_fence_init that it deals with. Hmm, still favouring the
all-in-one.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
