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

[Zhenyu Wang]

On 2016.10.20 07:52:18 +0100, Chris Wilson wrote:
> 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 at 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.

Would you refresh this one? So I'd send out next pull request with this.

thanks

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20161020/e3ee7d58/attachment.sig>