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

[Chris Wilson]

On Wed, Oct 19, 2016 at 06:32:54PM +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));
> >  
> >  	mutex_unlock(&gvt->lock);
> 
> Might not need to hold gvt->lock when put request?

I was just updating the current worklod->req = NULL which was under the
lock, using the same code as later. You can drop i915_gem_request_put(rq)
afterwards which is what I did at first, before deciding using the same
style for both was nicer.

> > +
> > +	i915_add_request_no_flush(rq);
> 
> Why still add request in error path?

The request may have changed global state which is now associated with
the request. You have to pass it back upon completion, whether or not
you have added your own workload to the request.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre