[v2] drm/i915/gvt: Fix crash after request->hw_context change
diff mbox

Message ID 20180521081752.31056-1-zhenyuw@linux.intel.com
State New
Headers show

Commit Message

Zhenyu Wang May 21, 2018, 8:17 a.m. UTC
When we do shadowing, workload's request might not be allocated yet,
so we still require shadow context's object. And when complete workload,
delay to zero workload's request pointer after used for update guest context.

v2: Move request alloc earlier as already try to track shadow status
depending on request state, which also facilitate to use request->hw_context
for target engine context reference.

Fixes: 1fc44d9b1afb ("drm/i915: Store a pointer to intel_context in i915_request")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>
Cc: Weinan Li <weinan.z.li@intel.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/gvt/scheduler.c | 52 +++++++++-------------------
 1 file changed, 16 insertions(+), 36 deletions(-)

Comments

Chris Wilson May 21, 2018, 9:54 a.m. UTC | #1
Quoting Zhenyu Wang (2018-05-21 09:17:52)
> When we do shadowing, workload's request might not be allocated yet,
> so we still require shadow context's object. And when complete workload,
> delay to zero workload's request pointer after used for update guest context.
> 
> v2: Move request alloc earlier as already try to track shadow status
> depending on request state, which also facilitate to use request->hw_context
> for target engine context reference.
> 
> Fixes: 1fc44d9b1afb ("drm/i915: Store a pointer to intel_context in i915_request")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> Cc: Weinan Li <weinan.z.li@intel.com>
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>

Looks like it's applying the principles we discussed to avoid
chasing gem_context to get at the pinned intel_context,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks for the fixup!
-Chris
Chris Wilson May 21, 2018, 10:12 a.m. UTC | #2
Quoting Chris Wilson (2018-05-21 10:54:57)
> Quoting Zhenyu Wang (2018-05-21 09:17:52)
> > When we do shadowing, workload's request might not be allocated yet,
> > so we still require shadow context's object. And when complete workload,
> > delay to zero workload's request pointer after used for update guest context.
> > 
> > v2: Move request alloc earlier as already try to track shadow status
> > depending on request state, which also facilitate to use request->hw_context
> > for target engine context reference.
> > 
> > Fixes: 1fc44d9b1afb ("drm/i915: Store a pointer to intel_context in i915_request")
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Zhi Wang <zhi.a.wang@intel.com>
> > Cc: Weinan Li <weinan.z.li@intel.com>
> > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> 
> Looks like it's applying the principles we discussed to avoid
> chasing gem_context to get at the pinned intel_context,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Thanks for the fixup!

And pushed, many thanks again.
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
index e1760030dda1..7f5e01df95ee 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -348,6 +348,7 @@  int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload)
 	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
 	struct intel_engine_cs *engine = dev_priv->engine[workload->ring_id];
 	struct intel_context *ce;
+	struct i915_request *rq;
 	int ret;
 
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
@@ -386,46 +387,26 @@  int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload)
 			goto err_shadow;
 	}
 
-	ret = populate_shadow_context(workload);
-	if (ret)
-		goto err_shadow;
-
-	return 0;
-
-err_shadow:
-	release_shadow_wa_ctx(&workload->wa_ctx);
-err_unpin:
-	intel_context_unpin(ce);
-	return ret;
-}
-
-static int intel_gvt_generate_request(struct intel_vgpu_workload *workload)
-{
-	int ring_id = workload->ring_id;
-	struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
-	struct i915_request *rq;
-	struct intel_vgpu *vgpu = workload->vgpu;
-	struct intel_vgpu_submission *s = &vgpu->submission;
-	struct i915_gem_context *shadow_ctx = s->shadow_ctx;
-	int ret;
-
-	rq = i915_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
+	rq = i915_request_alloc(engine, shadow_ctx);
 	if (IS_ERR(rq)) {
 		gvt_vgpu_err("fail to allocate gem request\n");
 		ret = PTR_ERR(rq);
-		goto err_unpin;
+		goto err_shadow;
 	}
-
-	gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq);
-
 	workload->req = i915_request_get(rq);
-	ret = copy_workload_to_ring_buffer(workload);
+
+	ret = populate_shadow_context(workload);
 	if (ret)
-		goto err_unpin;
-	return 0;
+		goto err_req;
 
-err_unpin:
+	return 0;
+err_req:
+	rq = fetch_and_zero(&workload->req);
+	i915_request_put(rq);
+err_shadow:
 	release_shadow_wa_ctx(&workload->wa_ctx);
+err_unpin:
+	intel_context_unpin(ce);
 	return ret;
 }
 
@@ -609,7 +590,7 @@  static int prepare_workload(struct intel_vgpu_workload *workload)
 		goto err_unpin_mm;
 	}
 
-	ret = intel_gvt_generate_request(workload);
+	ret = copy_workload_to_ring_buffer(workload);
 	if (ret) {
 		gvt_vgpu_err("fail to generate request\n");
 		goto err_unpin_mm;
@@ -823,7 +804,7 @@  static void complete_current_workload(struct intel_gvt *gvt, int ring_id)
 		scheduler->current_workload[ring_id];
 	struct intel_vgpu *vgpu = workload->vgpu;
 	struct intel_vgpu_submission *s = &vgpu->submission;
-	struct i915_request *rq;
+	struct i915_request *rq = workload->req;
 	int event;
 
 	mutex_lock(&gvt->lock);
@@ -832,7 +813,6 @@  static void complete_current_workload(struct intel_gvt *gvt, int ring_id)
 	 * switch to make sure request is completed.
 	 * For the workload w/o request, directly complete the workload.
 	 */
-	rq = fetch_and_zero(&workload->req);
 	if (rq) {
 		wait_event(workload->shadow_ctx_status_wq,
 			   !atomic_read(&workload->shadow_ctx_active));
@@ -863,7 +843,7 @@  static void complete_current_workload(struct intel_gvt *gvt, int ring_id)
 		intel_context_unpin(rq->hw_context);
 		mutex_unlock(&rq->i915->drm.struct_mutex);
 
-		i915_request_put(rq);
+		i915_request_put(fetch_and_zero(&workload->req));
 	}
 
 	gvt_dbg_sched("ring id %d complete workload %p status %d\n",