[PATCH] drm/i915/gvt: Fix workload request allocation before request add

[Yang, Bin]

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 at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Zhenyu Wang <zhenyuw at 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 at intel.com>

thanks