[PATCH 1/6] drm/i915/gvt: Separate cmd scan from request allocation

Zhenyu Wang zhenyuw at linux.intel.com
Fri Aug 11 08:49:09 UTC 2017


You need to specify each series version when sending with "-v" option helper. Thx

On 2017.08.11 16:25:32 +0800, fred gao wrote:
> Currently i915 request structure and shadow ring buffer are allocated
> before command scan, so it will have to restore to previous states once
> any error happens afterwards in the long dispatch_workload path.
> 
> This patch is to introduce a reserved ring buffer created at the beginning
> of vGPU initialization. Workload will be coped to this reserved buffer and
> be scanned first, the i915 request and shadow ring buffer are only
> allocated after the result of scan is successful.
> 
> To balance the memory usage and buffer alloc time, the coming bigger ring
> buffer will be reallocated and kept until more bigger buffer is coming.
> 
> v2:
> - use kmalloc for the smaller ring buffer, realloc if required. (Zhenyu)
> 
> v3:
> - remove the dynamically allocated ring buffer. (Zhenyu)
> 
> v4:
> - code style polish.
> - kfree previous allocated buffer once kmalloc failed. (Zhenyu)
> 
> Signed-off-by: fred gao <fred.gao at intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/cmd_parser.c | 31 ++++++++-----
>  drivers/gpu/drm/i915/gvt/execlist.c   | 30 ++++++++++++
>  drivers/gpu/drm/i915/gvt/gvt.h        |  3 ++
>  drivers/gpu/drm/i915/gvt/scheduler.c  | 86 +++++++++++++++++++++++------------
>  4 files changed, 110 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> index 72b97ce..e53efc0 100644
> --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> @@ -2603,7 +2603,8 @@ static int shadow_workload_ring_buffer(struct intel_vgpu_workload *workload)
>  {
>  	struct intel_vgpu *vgpu = workload->vgpu;
>  	unsigned long gma_head, gma_tail, gma_top, guest_rb_size;
> -	u32 *cs;
> +	void *shadow_ring_buffer_va;
> +	int ring_id = workload->ring_id;
>  	int ret;
>  
>  	guest_rb_size = _RING_CTL_BUF_SIZE(workload->rb_ctl);
> @@ -2616,34 +2617,42 @@ static int shadow_workload_ring_buffer(struct intel_vgpu_workload *workload)
>  	gma_tail = workload->rb_start + workload->rb_tail;
>  	gma_top = workload->rb_start + guest_rb_size;
>  
> -	/* allocate shadow ring buffer */
> -	cs = intel_ring_begin(workload->req, workload->rb_len / sizeof(u32));
> -	if (IS_ERR(cs))
> -		return PTR_ERR(cs);
> +	if (workload->rb_len > vgpu->reserve_ring_buffer_size[ring_id]) {
> +		void *va = vgpu->reserve_ring_buffer_va[ring_id];
> +		/* realloc the new ring buffer if needed */
> +		vgpu->reserve_ring_buffer_va[ring_id] =
> +			krealloc(va, workload->rb_len, GFP_KERNEL);
> +		if (!vgpu->reserve_ring_buffer_va[ring_id]) {
> +			gvt_vgpu_err("fail to alloc reserve ring buffer\n");
> +			return -ENOMEM;
> +		}
> +		vgpu->reserve_ring_buffer_size[ring_id] = workload->rb_len;
> +	}
> +
> +	shadow_ring_buffer_va = vgpu->reserve_ring_buffer_va[ring_id];
>  
>  	/* get shadow ring buffer va */
> -	workload->shadow_ring_buffer_va = cs;
> +	workload->shadow_ring_buffer_va = shadow_ring_buffer_va;
>  
>  	/* head > tail --> copy head <-> top */
>  	if (gma_head > gma_tail) {
>  		ret = copy_gma_to_hva(vgpu, vgpu->gtt.ggtt_mm,
> -				      gma_head, gma_top, cs);
> +				      gma_head, gma_top, shadow_ring_buffer_va);
>  		if (ret < 0) {
>  			gvt_vgpu_err("fail to copy guest ring buffer\n");
>  			return ret;
>  		}
> -		cs += ret / sizeof(u32);
> +		shadow_ring_buffer_va += ret;
>  		gma_head = workload->rb_start;
>  	}
>  
>  	/* copy head or start <-> tail */
> -	ret = copy_gma_to_hva(vgpu, vgpu->gtt.ggtt_mm, gma_head, gma_tail, cs);
> +	ret = copy_gma_to_hva(vgpu, vgpu->gtt.ggtt_mm, gma_head, gma_tail,
> +				shadow_ring_buffer_va);
>  	if (ret < 0) {
>  		gvt_vgpu_err("fail to copy guest ring buffer\n");
>  		return ret;
>  	}
> -	cs += ret / sizeof(u32);
> -	intel_ring_advance(workload->req, cs);
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/gvt/execlist.c b/drivers/gpu/drm/i915/gvt/execlist.c
> index 91b4300..1e2c277 100644
> --- a/drivers/gpu/drm/i915/gvt/execlist.c
> +++ b/drivers/gpu/drm/i915/gvt/execlist.c
> @@ -820,10 +820,21 @@ static void clean_workloads(struct intel_vgpu *vgpu, unsigned long engine_mask)
>  
>  void intel_vgpu_clean_execlist(struct intel_vgpu *vgpu)
>  {
> +	enum intel_engine_id i;
> +	struct intel_engine_cs *engine;
> +
>  	clean_workloads(vgpu, ALL_ENGINES);
>  	kmem_cache_destroy(vgpu->workloads);
> +
> +	for_each_engine(engine, vgpu->gvt->dev_priv, i) {
> +		kfree(vgpu->reserve_ring_buffer_va[i]);
> +		vgpu->reserve_ring_buffer_va[i] = NULL;
> +		vgpu->reserve_ring_buffer_size[i] = 0;
> +	}
> +
>  }
>  
> +#define RESERVE_RING_BUFFER_SIZE		((1 * PAGE_SIZE)/8)
>  int intel_vgpu_init_execlist(struct intel_vgpu *vgpu)
>  {
>  	enum intel_engine_id i;
> @@ -843,7 +854,26 @@ int intel_vgpu_init_execlist(struct intel_vgpu *vgpu)
>  	if (!vgpu->workloads)
>  		return -ENOMEM;
>  
> +	/* each ring has a shadow ring buffer until vgpu destroyed */
> +	for_each_engine(engine, vgpu->gvt->dev_priv, i) {
> +		vgpu->reserve_ring_buffer_va[i] =
> +			kmalloc(RESERVE_RING_BUFFER_SIZE, GFP_KERNEL);
> +		if (!vgpu->reserve_ring_buffer_va[i]) {
> +			gvt_vgpu_err("fail to alloc reserve ring buffer\n");
> +			goto out;
> +		}
> +		vgpu->reserve_ring_buffer_size[i] = RESERVE_RING_BUFFER_SIZE;
> +	}
>  	return 0;
> +out:
> +	for_each_engine(engine, vgpu->gvt->dev_priv, i) {
> +		if (vgpu->reserve_ring_buffer_size[i]) {
> +			kfree(vgpu->reserve_ring_buffer_va[i]);
> +			vgpu->reserve_ring_buffer_va[i] = NULL;
> +			vgpu->reserve_ring_buffer_size[i] = 0;
> +		}
> +	}
> +	return -ENOMEM;
>  }
>  
>  void intel_vgpu_reset_execlist(struct intel_vgpu *vgpu,
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index 44b719e..8960544 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -165,6 +165,9 @@ struct intel_vgpu {
>  	struct list_head workload_q_head[I915_NUM_ENGINES];
>  	struct kmem_cache *workloads;
>  	atomic_t running_workload_num;
> +	/* 1/2K for each reserve ring buffer */
> +	void *reserve_ring_buffer_va[I915_NUM_ENGINES];
> +	int reserve_ring_buffer_size[I915_NUM_ENGINES];
>  	DECLARE_BITMAP(tlb_handle_pending, I915_NUM_ENGINES);
>  	struct i915_gem_context *shadow_ctx;
>  	DECLARE_BITMAP(shadow_ctx_desc_updated, I915_NUM_ENGINES);
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
> index 391800d..43022d3 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -201,6 +201,34 @@ static void shadow_context_descriptor_update(struct i915_gem_context *ctx,
>  	ce->lrc_desc = desc;
>  }
>  
> +static int copy_workload_to_ring_buffer(struct intel_vgpu_workload *workload)
> +{
> +	struct intel_vgpu *vgpu = workload->vgpu;
> +	void *shadow_ring_buffer_va;
> +	u32 *cs;
> +
> +	/* allocate shadow ring buffer */
> +	cs = intel_ring_begin(workload->req, workload->rb_len / sizeof(u32));
> +	if (IS_ERR(cs)) {
> +		gvt_vgpu_err("fail to alloc size =%ld shadow  ring buffer\n",
> +			workload->rb_len);
> +		return PTR_ERR(cs);
> +	}
> +
> +	shadow_ring_buffer_va = workload->shadow_ring_buffer_va;
> +
> +	/* get shadow ring buffer va */
> +	workload->shadow_ring_buffer_va = cs;
> +
> +	memcpy(cs, shadow_ring_buffer_va,
> +			workload->rb_len);
> +
> +	cs += workload->rb_len / sizeof(u32);
> +	intel_ring_advance(workload->req, cs);
> +
> +	return 0;
> +}
> +
>  /**
>   * intel_gvt_scan_and_shadow_workload - audit the workload by scanning and
>   * shadow it as well, include ringbuffer,wa_ctx and ctx.
> @@ -214,8 +242,10 @@ int intel_gvt_scan_and_shadow_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 intel_engine_cs *engine = dev_priv->engine[ring_id];
>  	struct drm_i915_gem_request *rq;
>  	struct intel_vgpu *vgpu = workload->vgpu;
> +	struct intel_ring *ring;
>  	int ret;
>  
>  	lockdep_assert_held(&dev_priv->drm.struct_mutex);
> @@ -231,17 +261,6 @@ int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload)
>  		shadow_context_descriptor_update(shadow_ctx,
>  					dev_priv->engine[ring_id]);
>  
> -	rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
> -	if (IS_ERR(rq)) {
> -		gvt_vgpu_err("fail to allocate gem request\n");
> -		ret = PTR_ERR(rq);
> -		goto out;
> -	}
> -
> -	gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq);
> -
> -	workload->req = i915_gem_request_get(rq);
> -
>  	ret = intel_gvt_scan_and_shadow_ringbuffer(workload);
>  	if (ret)
>  		goto out;
> @@ -253,10 +272,37 @@ int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload)
>  			goto out;
>  	}
>  
> +	/* pin shadow context by gvt even the shadow context will be pinned
> +	 * when i915 alloc request. That is because gvt will update the guest
> +	 * context from shadow context when workload is completed, and at that
> +	 * moment, i915 may already unpined the shadow context to make the
> +	 * shadow_ctx pages invalid. So gvt need to pin itself. After update
> +	 * the guest context, gvt can unpin the shadow_ctx safely.
> +	 */
> +	ring = engine->context_pin(engine, shadow_ctx);
> +	if (IS_ERR(ring)) {
> +		ret = PTR_ERR(ring);
> +		gvt_vgpu_err("fail to pin shadow context\n");
> +		goto out;
> +	}
> +
>  	ret = populate_shadow_context(workload);
>  	if (ret)
>  		goto out;
>  
> +	rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
> +	if (IS_ERR(rq)) {
> +		gvt_vgpu_err("fail to allocate gem request\n");
> +		ret = PTR_ERR(rq);
> +		goto out;
> +	}
> +
> +	gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq);
> +
> +	workload->req = i915_gem_request_get(rq);
> +	ret = copy_workload_to_ring_buffer(workload);
> +	if (ret)
> +		goto out;
>  	workload->shadowed = true;
>  
>  out:
> @@ -266,11 +312,7 @@ int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload)
>  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 intel_engine_cs *engine = dev_priv->engine[ring_id];
> -	struct intel_vgpu *vgpu = workload->vgpu;
> -	struct intel_ring *ring;
>  	int ret = 0;
>  
>  	gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
> @@ -288,20 +330,6 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
>  			goto out;
>  	}
>  
> -	/* pin shadow context by gvt even the shadow context will be pinned
> -	 * when i915 alloc request. That is because gvt will update the guest
> -	 * context from shadow context when workload is completed, and at that
> -	 * moment, i915 may already unpined the shadow context to make the
> -	 * shadow_ctx pages invalid. So gvt need to pin itself. After update
> -	 * the guest context, gvt can unpin the shadow_ctx safely.
> -	 */
> -	ring = engine->context_pin(engine, shadow_ctx);
> -	if (IS_ERR(ring)) {
> -		ret = PTR_ERR(ring);
> -		gvt_vgpu_err("fail to pin shadow context\n");
> -		goto out;
> -	}
> -
>  out:
>  	if (ret)
>  		workload->status = ret;
> -- 
> 2.7.4
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20170811/6c986db4/attachment-0001.sig>


More information about the intel-gvt-dev mailing list