Re: [PATCH 07/12] drm/i915/gvt: Hold a reference on the request

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 07/12] drm/i915/gvt: Hold a reference on the request
Date: Thu, 20 Oct 2016 07:52:18 +0100	[thread overview]
Message-ID: <20161020065218.GE19173@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <20161020002200.656623zjtjyddw44@zhen-hp.sh.intel.com>

On Thu, Oct 20, 2016 at 08:22:00AM +0800, Zhenyu Wang wrote:
> On 2016.10.19 11:11:42 +0100, Chris Wilson wrote:
> > The workload took a pointer to the request, and even waited upon,
> > without holding a reference on the request. Take that reference
> > explicitly and fix up the error path following request allocation that
> > missed flushing the request.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/gvt/scheduler.c | 24 +++++++++++++-----------
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
> > index b15cdf5978a9..224f19ae61ab 100644
> > --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> > +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> > @@ -163,6 +163,7 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
> >  	int ring_id = workload->ring_id;
> >  	struct i915_gem_context *shadow_ctx = workload->vgpu->shadow_ctx;
> >  	struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
> > +	struct drm_i915_gem_request *rq;
> >  	int ret;
> >  
> >  	gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
> > @@ -171,17 +172,16 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
> >  	shadow_ctx->desc_template = workload->ctx_desc.addressing_mode <<
> >  				    GEN8_CTX_ADDRESSING_MODE_SHIFT;
> >  
> > -	workload->req = i915_gem_request_alloc(dev_priv->engine[ring_id],
> > -					       shadow_ctx);
> > -	if (IS_ERR_OR_NULL(workload->req)) {
> > +	rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
> > +	if (IS_ERR(rq)) {
> >  		gvt_err("fail to allocate gem request\n");
> > -		workload->status = PTR_ERR(workload->req);
> > -		workload->req = NULL;
> > +		workload->status = PTR_ERR(rq);
> >  		return workload->status;
> >  	}
> >  
> > -	gvt_dbg_sched("ring id %d get i915 gem request %p\n",
> > -			ring_id, workload->req);
> > +	gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq);
> > +
> > +	workload->req = i915_gem_request_get(rq);
> >  
> >  	mutex_lock(&gvt->lock);
> >  
> > @@ -208,16 +208,16 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
> >  	gvt_dbg_sched("ring id %d submit workload to i915 %p\n",
> >  			ring_id, workload->req);
> >  
> > -	i915_add_request_no_flush(workload->req);
> > -
> > +	i915_add_request_no_flush(rq);
> >  	workload->dispatched = true;
> >  	return 0;
> >  err:
> >  	workload->status = ret;
> > -	if (workload->req)
> > -		workload->req = NULL;
> > +	i915_gem_request_put(fetch_and_zero(&workload->req));
> >
> 
> Looks we don't need put here as in error path from dispatch_workload()
> we will go with below put path too in main thread.

If we clear the request pointer, then we need the put. But yes, we don't
necessarily need to clear the pointer on error for the caller, as the
caller doesn't distinguish the error path and the no-op request can be
handled identically to a real request.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx