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@xxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
