| Message ID | 20161019101147.17342-8-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)); > > mutex_unlock(&gvt->lock); Might not need to hold gvt->lock when put request? > + > + i915_add_request_no_flush(rq); Why still add request in error path? > return ret; > } > > @@ -458,6 +458,8 @@ static int workload_thread(void *priv) > > complete_current_workload(gvt, ring_id); > > + i915_gem_request_put(fetch_and_zero(&workload->req)); > + > if (need_force_wake) > intel_uncore_forcewake_put(gvt->dev_priv, > FORCEWAKE_ALL); > -- > 2.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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
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. > mutex_unlock(&gvt->lock); > + > + i915_add_request_no_flush(rq); > return ret; > } > > @@ -458,6 +458,8 @@ static int workload_thread(void *priv) > > complete_current_workload(gvt, ring_id); > > + i915_gem_request_put(fetch_and_zero(&workload->req)); > + > if (need_force_wake) > intel_uncore_forcewake_put(gvt->dev_priv, > FORCEWAKE_ALL);
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
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@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
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); + + i915_add_request_no_flush(rq); return ret; } @@ -458,6 +458,8 @@ static int workload_thread(void *priv) complete_current_workload(gvt, ring_id); + i915_gem_request_put(fetch_and_zero(&workload->req)); + if (need_force_wake) intel_uncore_forcewake_put(gvt->dev_priv, FORCEWAKE_ALL);
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(-)