[Intel-gfx] [PATCH 07/15] drm/i915: Hold a reference on the request for its fence chain

[Chris Wilson]

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 at chris-wilson.co.uk>
> 
> <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