drm/i915/gvt: Fix workload request allocation before request add

Submitted by Zhenyu Wang on Dec. 25, 2018, 2:22 a.m.

Details

Message ID 20181225022214.7051-1-zhenyuw@linux.intel.com
State New
Series "drm/i915/gvt: Fix workload request allocation before request add"
Headers show

Commit Message

Zhenyu Wang Dec. 25, 2018, 2:22 a.m.
In commit 6bb2a2af8b1b ("drm/i915/gvt: Fix crash after request->hw_context change"),
forgot to handle workload scan path in ELSP handler case which was to
optimize scanning earlier instead of in gvt submission thread, so request
alloc and add was splitted then which is against right process.

This trys to do a partial revert of that commit which still has workload
request alloc helper and make sure shadow state population is handled after
request alloc for target state buffer.

Fixes: 6bb2a2af8b1b ("drm/i915/gvt: Fix crash after request->hw_context change")
Cc: Bin Yang <bin.yang@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/gvt/scheduler.c | 65 ++++++++++++++++++++--------
 drivers/gpu/drm/i915/gvt/scheduler.h |  1 +
 2 files changed, 47 insertions(+), 19 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
index 1ad8c5e1455d..21633154086e 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -356,6 +356,33 @@  static int set_context_ppgtt_from_shadow(struct intel_vgpu_workload *workload,
 	return 0;
 }
 
+static int
+intel_gvt_workload_req_alloc(struct intel_vgpu_workload *workload)
+{
+	struct intel_vgpu *vgpu = workload->vgpu;
+	struct intel_vgpu_submission *s = &vgpu->submission;
+	struct i915_gem_context *shadow_ctx = s->shadow_ctx;
+	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+	struct intel_engine_cs *engine = dev_priv->engine[workload->ring_id];
+	struct i915_request *rq;
+	int ret = 0;
+
+	lockdep_assert_held(&dev_priv->drm.struct_mutex);
+
+	if (workload->req)
+		goto out;
+
+	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 out;
+	}
+	workload->req = i915_request_get(rq);
+out:
+	return ret;
+}
+
 /**
  * intel_gvt_scan_and_shadow_workload - audit the workload by scanning and
  * shadow it as well, include ringbuffer,wa_ctx and ctx.
@@ -372,12 +399,11 @@  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);
 
-	if (workload->req)
+	if (workload->shadow)
 		return 0;
 
 	ret = set_context_ppgtt_from_shadow(workload, shadow_ctx);
@@ -417,22 +443,8 @@  int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload)
 			goto err_shadow;
 	}
 
-	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_shadow;
-	}
-	workload->req = i915_request_get(rq);
-
-	ret = populate_shadow_context(workload);
-	if (ret)
-		goto err_req;
-
+	workload->shadow = true;
 	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:
@@ -671,15 +683,30 @@  static int dispatch_workload(struct intel_vgpu_workload *workload)
 	mutex_lock(&vgpu->vgpu_lock);
 	mutex_lock(&dev_priv->drm.struct_mutex);
 
+	ret = intel_gvt_workload_req_alloc(workload);
+	if (ret)
+		goto out;
+
 	ret = intel_gvt_scan_and_shadow_workload(workload);
 	if (ret)
 		goto out;
 
-	ret = prepare_workload(workload);
+	ret = populate_shadow_context(workload);
+	if (ret) {
+		release_shadow_wa_ctx(&workload->wa_ctx);
+		goto out;
+	}
 
+	ret = prepare_workload(workload);
 out:
-	if (ret)
+	if (ret) {
+		if (workload->req) {
+			struct i915_request *rq;
+			rq = fetch_and_zero(&workload->req);
+			i915_request_put(rq);
+		}
 		workload->status = ret;
+	}
 
 	if (!IS_ERR_OR_NULL(workload->req)) {
 		gvt_dbg_sched("ring id %d submit workload to i915 %p\n",
diff --git a/drivers/gpu/drm/i915/gvt/scheduler.h b/drivers/gpu/drm/i915/gvt/scheduler.h
index ca5529d0e48e..2065cba59aab 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.h
+++ b/drivers/gpu/drm/i915/gvt/scheduler.h
@@ -83,6 +83,7 @@  struct intel_vgpu_workload {
 	struct i915_request *req;
 	/* if this workload has been dispatched to i915? */
 	bool dispatched;
+	bool shadow;      /* if workload has done shadow of guest request */
 	int status;
 
 	struct intel_vgpu_mm *shadow_mm;

Comments

Yang, Bin Dec. 25, 2018, 6:24 a.m.
On Tue, 2018-12-25 at 10:22 +0800, Zhenyu Wang wrote:
> In commit 6bb2a2af8b1b ("drm/i915/gvt: Fix crash after request->hw_context change"),

> forgot to handle workload scan path in ELSP handler case which was to

> optimize scanning earlier instead of in gvt submission thread, so request

> alloc and add was splitted then which is against right process.

> 

> This trys to do a partial revert of that commit which still has workload

> request alloc helper and make sure shadow state population is handled after

> request alloc for target state buffer.

> 

> Fixes: 6bb2a2af8b1b ("drm/i915/gvt: Fix crash after request->hw_context change")

> Cc: Bin Yang <bin.yang@intel.com>

> Cc: Chris Wilson <chris@chris-wilson.co.uk>

> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>

> ---

>  drivers/gpu/drm/i915/gvt/scheduler.c | 65 ++++++++++++++++++++--------

>  drivers/gpu/drm/i915/gvt/scheduler.h |  1 +

>  2 files changed, 47 insertions(+), 19 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c

> index 1ad8c5e1455d..21633154086e 100644

> --- a/drivers/gpu/drm/i915/gvt/scheduler.c

> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c

> @@ -356,6 +356,33 @@ static int set_context_ppgtt_from_shadow(struct intel_vgpu_workload *workload,

>  	return 0;

>  }

>  

> +static int

> +intel_gvt_workload_req_alloc(struct intel_vgpu_workload *workload)

> +{

> +	struct intel_vgpu *vgpu = workload->vgpu;

> +	struct intel_vgpu_submission *s = &vgpu->submission;

> +	struct i915_gem_context *shadow_ctx = s->shadow_ctx;

> +	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;

> +	struct intel_engine_cs *engine = dev_priv->engine[workload->ring_id];

> +	struct i915_request *rq;

> +	int ret = 0;

> +

> +	lockdep_assert_held(&dev_priv->drm.struct_mutex);

> +

> +	if (workload->req)

> +		goto out;

> +

> +	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 out;

> +	}

> +	workload->req = i915_request_get(rq);

> +out:

> +	return ret;

> +}

> +

>  /**

>   * intel_gvt_scan_and_shadow_workload - audit the workload by scanning and

>   * shadow it as well, include ringbuffer,wa_ctx and ctx.

> @@ -372,12 +399,11 @@ 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);

>  

> -	if (workload->req)

> +	if (workload->shadow)

>  		return 0;

>  

>  	ret = set_context_ppgtt_from_shadow(workload, shadow_ctx);

> @@ -417,22 +443,8 @@ int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload)

>  			goto err_shadow;

>  	}

>  

> -	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_shadow;

> -	}

> -	workload->req = i915_request_get(rq);

> -

> -	ret = populate_shadow_context(workload);

> -	if (ret)

> -		goto err_req;

> -

> +	workload->shadow = true;

>  	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:

> @@ -671,15 +683,30 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)

>  	mutex_lock(&vgpu->vgpu_lock);

>  	mutex_lock(&dev_priv->drm.struct_mutex);

>  

> +	ret = intel_gvt_workload_req_alloc(workload);

> +	if (ret)

> +		goto out;

> +

>  	ret = intel_gvt_scan_and_shadow_workload(workload);

>  	if (ret)

>  		goto out;

>  

> -	ret = prepare_workload(workload);

> +	ret = populate_shadow_context(workload);

> +	if (ret) {

> +		release_shadow_wa_ctx(&workload->wa_ctx);

> +		goto out;

> +	}

>  

> +	ret = prepare_workload(workload);

>  out:

> -	if (ret)

> +	if (ret) {

> +		if (workload->req) {

> +			struct i915_request *rq;

> +			rq = fetch_and_zero(&workload->req);

> +			i915_request_put(rq);

> +		}

>  		workload->status = ret;

> +	}

>  

>  	if (!IS_ERR_OR_NULL(workload->req)) {

>  		gvt_dbg_sched("ring id %d submit workload to i915 %p\n",

> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.h b/drivers/gpu/drm/i915/gvt/scheduler.h

> index ca5529d0e48e..2065cba59aab 100644

> --- a/drivers/gpu/drm/i915/gvt/scheduler.h

> +++ b/drivers/gpu/drm/i915/gvt/scheduler.h

> @@ -83,6 +83,7 @@ struct intel_vgpu_workload {

>  	struct i915_request *req;

>  	/* if this workload has been dispatched to i915? */

>  	bool dispatched;

> +	bool shadow;      /* if workload has done shadow of guest request */

>  	int status;

>  

>  	struct intel_vgpu_mm *shadow_mm;



Tested on Apollo Lake with Clearlinux + ACRN, works fine.

Tested-by: Bin Yang <bin.yang@intel.com>


thanks
lab_gvt-test@intel.com Dec. 28, 2018, 4:56 p.m.