[PATCH v3] drm/i915/gvt: scan non-privileged batch buffer for debug purpose

Zhenyu Wang zhenyuw at linux.intel.com
Tue Apr 3 07:48:18 UTC 2018


On 2018.04.02 16:35:04 +0800, Zhao Yan wrote:
> For perfomance purpose, scanning of non-privileged batch buffer is turned
> off by default. But for debugging purpose, it can be turned on via debugfs.
> After scanning, we submit the original non-privileged batch buffer into
> hardware, so that the scanning is only a peeking window of guest submitted
> commands and will not affect the execution results.
> 
> v3:
> - change vgpu->scan_nonprivbb from type bool to u32, so it is able to
>   selectively turn on/off scanning of non-privileged batch buffer on engine
>   level. e.g.
>   if vgpu->scan_nonprivbb=3, then it will scan non-privileged batch buffer
>   on engine 0 and 1.
> - in debugfs interface to set vgpu->scan_nonprivbb, print warning message
>   to warn user and explicitly tell state change in kernel log (zhenyu wang)
> v2:
> - rebase
> - update comments for start_gma_offset (henry)
> 
> Signed-off-by: Zhao Yan <yan.y.zhao at intel.com>
> Reviewed-by: Zhenyu Wang <zhenyuw at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/cmd_parser.c | 55 +++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/gvt/debugfs.c    | 63 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/gvt/gvt.h        |  1 +
>  drivers/gpu/drm/i915/gvt/scheduler.c  | 64 +++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/gvt/scheduler.h  |  1 +
>  drivers/gpu/drm/i915/gvt/trace.h      | 24 ++++++++++---
>  6 files changed, 169 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> index d85939b..a294427 100644
> --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> @@ -1604,7 +1604,8 @@ static int batch_buffer_needs_scan(struct parser_exec_state *s)
>  	if (IS_BROADWELL(gvt->dev_priv) || IS_SKYLAKE(gvt->dev_priv)
>  		|| IS_KABYLAKE(gvt->dev_priv)) {
>  		/* BDW decides privilege based on address space */
> -		if (cmd_val(s, 0) & (1 << 8))
> +		if (cmd_val(s, 0) & (1 << 8) &&
> +			!(s->vgpu->scan_nonprivbb & (1 << s->ring_id)))
>  			return 0;
>  	}
>  	return 1;
> @@ -1618,6 +1619,8 @@ static int find_bb_size(struct parser_exec_state *s, unsigned long *bb_size)
>  	bool bb_end = false;
>  	struct intel_vgpu *vgpu = s->vgpu;
>  	u32 cmd;
> +	struct intel_vgpu_mm *mm = (s->buf_addr_type == GTT_BUFFER) ?
> +		s->vgpu->gtt.ggtt_mm : s->workload->shadow_mm;
>  
>  	*bb_size = 0;
>  
> @@ -1629,18 +1632,22 @@ static int find_bb_size(struct parser_exec_state *s, unsigned long *bb_size)
>  	cmd = cmd_val(s, 0);
>  	info = get_cmd_info(s->vgpu->gvt, cmd, s->ring_id);
>  	if (info == NULL) {
> -		gvt_vgpu_err("unknown cmd 0x%x, opcode=0x%x\n",
> -				cmd, get_opcode(cmd, s->ring_id));
> +		gvt_vgpu_err("unknown cmd 0x%x, opcode=0x%x, addr_type=%s, ring %d, workload=%p\n",
> +				cmd, get_opcode(cmd, s->ring_id),
> +				(s->buf_addr_type == PPGTT_BUFFER) ?
> +				"ppgtt" : "ggtt", s->ring_id, s->workload);
>  		return -EBADRQC;
>  	}
>  	do {
> -		if (copy_gma_to_hva(s->vgpu, s->vgpu->gtt.ggtt_mm,
> +		if (copy_gma_to_hva(s->vgpu, mm,
>  				gma, gma + 4, &cmd) < 0)
>  			return -EFAULT;
>  		info = get_cmd_info(s->vgpu->gvt, cmd, s->ring_id);
>  		if (info == NULL) {
> -			gvt_vgpu_err("unknown cmd 0x%x, opcode=0x%x\n",
> -				cmd, get_opcode(cmd, s->ring_id));
> +			gvt_vgpu_err("unknown cmd 0x%x, opcode=0x%x, addr_type=%s, ring %d, workload=%p\n",
> +				cmd, get_opcode(cmd, s->ring_id),
> +				(s->buf_addr_type == PPGTT_BUFFER) ?
> +				"ppgtt" : "ggtt", s->ring_id, s->workload);
>  			return -EBADRQC;
>  		}
>  
> @@ -1666,6 +1673,9 @@ static int perform_bb_shadow(struct parser_exec_state *s)
>  	unsigned long gma = 0;
>  	unsigned long bb_size;
>  	int ret = 0;
> +	struct intel_vgpu_mm *mm = (s->buf_addr_type == GTT_BUFFER) ?
> +		s->vgpu->gtt.ggtt_mm : s->workload->shadow_mm;
> +	unsigned long gma_start_offset = 0;
>  
>  	/* get the start gm address of the batch buffer */
>  	gma = get_gma_bb_from_cmd(s, 1);
> @@ -1680,8 +1690,24 @@ static int perform_bb_shadow(struct parser_exec_state *s)
>  	if (!bb)
>  		return -ENOMEM;
>  
> +	bb->ppgtt = (s->buf_addr_type == GTT_BUFFER) ? false : true;
> +
> +	/* the gma_start_offset stores the batch buffer's start gma's
> +	 * offset relative to page boundary. so for non-privileged batch
> +	 * buffer, the shadowed gem object holds exactly the same page
> +	 * layout as original gem object. This is for the convience of
> +	 * replacing the whole non-privilged batch buffer page to this
> +	 * shadowed one in PPGTT at the same gma address. (this replacing
> +	 * action is not implemented yet now, but may be necessary in
> +	 * future).
> +	 * for prileged batch buffer, we just change start gma address to
> +	 * that of shadowed page.
> +	 */
> +	if (bb->ppgtt)
> +		gma_start_offset = gma & ~I915_GTT_PAGE_MASK;
> +
>  	bb->obj = i915_gem_object_create(s->vgpu->gvt->dev_priv,
> -					 roundup(bb_size, PAGE_SIZE));
> +			 roundup(bb_size + gma_start_offset, PAGE_SIZE));
>  	if (IS_ERR(bb->obj)) {
>  		ret = PTR_ERR(bb->obj);
>  		goto err_free_bb;
> @@ -1702,9 +1728,9 @@ static int perform_bb_shadow(struct parser_exec_state *s)
>  		bb->clflush &= ~CLFLUSH_BEFORE;
>  	}
>  
> -	ret = copy_gma_to_hva(s->vgpu, s->vgpu->gtt.ggtt_mm,
> +	ret = copy_gma_to_hva(s->vgpu, mm,
>  			      gma, gma + bb_size,
> -			      bb->va);
> +			      bb->va + gma_start_offset);
>  	if (ret < 0) {
>  		gvt_vgpu_err("fail to copy guest ring buffer\n");
>  		ret = -EFAULT;
> @@ -1730,7 +1756,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
>  	 * buffer's gma in pair. After all, we don't want to pin the shadow
>  	 * buffer here (too early).
>  	 */
> -	s->ip_va = bb->va;
> +	s->ip_va = bb->va + gma_start_offset;
>  	s->ip_gma = gma;
>  	return 0;
>  err_unmap:
> @@ -2469,15 +2495,18 @@ static int cmd_parser_exec(struct parser_exec_state *s)
>  
>  	info = get_cmd_info(s->vgpu->gvt, cmd, s->ring_id);
>  	if (info == NULL) {
> -		gvt_vgpu_err("unknown cmd 0x%x, opcode=0x%x\n",
> -				cmd, get_opcode(cmd, s->ring_id));
> +		gvt_vgpu_err("unknown cmd 0x%x, opcode=0x%x, addr_type=%s, ring %d, workload=%p\n",
> +				cmd, get_opcode(cmd, s->ring_id),
> +				(s->buf_addr_type == PPGTT_BUFFER) ?
> +				"ppgtt" : "ggtt", s->ring_id, s->workload);
>  		return -EBADRQC;
>  	}
>  
>  	s->info = info;
>  
>  	trace_gvt_command(vgpu->id, s->ring_id, s->ip_gma, s->ip_va,
> -			  cmd_length(s), s->buf_type);
> +			  cmd_length(s), s->buf_type, s->buf_addr_type,
> +			  s->workload, info->name);
>  
>  	if (info->handler) {
>  		ret = info->handler(s);
> diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c b/drivers/gpu/drm/i915/gvt/debugfs.c
> index f7d0078..70f1948 100644
> --- a/drivers/gpu/drm/i915/gvt/debugfs.c
> +++ b/drivers/gpu/drm/i915/gvt/debugfs.c
> @@ -124,6 +124,64 @@ static int vgpu_mmio_diff_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(vgpu_mmio_diff);
>  
> +static int
> +vgpu_scan_nonprivbb_get(void *data, u64 *val)
> +{
> +	struct intel_vgpu *vgpu = (struct intel_vgpu *)data;
> +	*val = vgpu->scan_nonprivbb;
> +	return 0;
> +}
> +
> +/*
> + * set/unset bit engine_id of vgpu->scan_nonprivbb to turn on/off scanning
> + * of non-privileged batch buffer. e.g.
> + * if vgpu->scan_nonprivbb=3, then it will scan non-privileged batch buffer
> + * on engine 0 and 1.
> + */
> +static int
> +vgpu_scan_nonprivbb_set(void *data, u64 val)
> +{
> +	struct intel_vgpu *vgpu = (struct intel_vgpu *)data;
> +	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> +	enum intel_engine_id id;
> +	char buf[128], *s;
> +	int len;
> +
> +	if (vgpu->scan_nonprivbb == val)
> +		return 0;
> +
> +	len = sprintf(buf,
> +		"vGPU %d currently scans non-privileged batch buffers on Engines:",
> +		vgpu->id);

Keep our log prefix "gvt: xxx"

> +
> +	s = buf + len;
> +
> +	for (id = 0; id < I915_NUM_ENGINES; id++) {
> +		struct intel_engine_cs *engine;
> +
> +		engine = dev_priv->engine[id];
> +		if (engine && (val & (1 << id))) {
> +			len = sprintf(s, "%s, ", engine->name);
> +			s += len;
> +		} else
> +			val &=  ~(1 << id) & ((1 << I915_NUM_ENGINES) - 1);
> +	}

I'm not sure it'll always be safe to assume those engine names can be contained
in this string, or use kmalloc based kasprintf, maybe simply show value is enough.

> +
> +	if (!val)
> +		sprintf(s, "None\0");

Not need to be chatty if disabled.

> +	else
> +		sprintf(s, "\0");
> +
> +	pr_warn("%s\n", buf);

For warn, maybe note "low performance expected".

> +
> +	vgpu->scan_nonprivbb = val;
> +	return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(vgpu_scan_nonprivbb_fops,
> +			vgpu_scan_nonprivbb_get, vgpu_scan_nonprivbb_set,
> +			"0x%llx\n");
> +
>  /**
>   * intel_gvt_debugfs_add_vgpu - register debugfs entries for a vGPU
>   * @vgpu: a vGPU
> @@ -151,6 +209,11 @@ int intel_gvt_debugfs_add_vgpu(struct intel_vgpu *vgpu)
>  	if (!ent)
>  		return -ENOMEM;
>  
> +	ent = debugfs_create_file("scan_nonprivbb", 0644, vgpu->debugfs,
> +				 vgpu, &vgpu_scan_nonprivbb_fops);
> +	if (!ent)
> +		return -ENOMEM;
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index efacd8a..6ec8888 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -226,6 +226,7 @@ struct intel_vgpu {
>  
>  	struct completion vblank_done;
>  
> +	u32 scan_nonprivbb;

type mismatch?

>  };
>  
>  /* validating GM healthy status*/
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
> index d71e3de..5366519 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -452,12 +452,6 @@ static int prepare_shadow_batch_buffer(struct intel_vgpu_workload *workload)
>  	int ret;
>  
>  	list_for_each_entry(bb, &workload->shadow_bb, list) {
> -		bb->vma = i915_gem_object_ggtt_pin(bb->obj, NULL, 0, 0, 0);
> -		if (IS_ERR(bb->vma)) {
> -			ret = PTR_ERR(bb->vma);
> -			goto err;
> -		}
> -
>  		/* For privilge batch buffer and not wa_ctx, the bb_start_cmd_va
>  		 * is only updated into ring_scan_buffer, not real ring address
>  		 * allocated in later copy_workload_to_ring_buffer. pls be noted
> @@ -469,25 +463,53 @@ static int prepare_shadow_batch_buffer(struct intel_vgpu_workload *workload)
>  			bb->bb_start_cmd_va = workload->shadow_ring_buffer_va
>  				+ bb->bb_offset;
>  
> -		/* relocate shadow batch buffer */
> -		bb->bb_start_cmd_va[1] = i915_ggtt_offset(bb->vma);
> -		if (gmadr_bytes == 8)
> -			bb->bb_start_cmd_va[2] = 0;
> +		if (bb->ppgtt) {
> +			/* for non-priv bb, scan&shadow is only for
> +			 * debugging purpose, so the content of shadow bb
> +			 * is the same as original bb. Therefore,
> +			 * here, rather than switch to shadow bb's gma
> +			 * address, we directly use original batch buffer's
> +			 * gma address, and send original bb to hardware
> +			 * directly
> +			 */
> +			if (bb->clflush & CLFLUSH_AFTER) {
> +				drm_clflush_virt_range(bb->va,
> +						bb->obj->base.size);
> +				bb->clflush &= ~CLFLUSH_AFTER;
> +			}
> +			i915_gem_obj_finish_shmem_access(bb->obj);
> +			bb->accessing = false;
> +
> +		} else {
> +			bb->vma = i915_gem_object_ggtt_pin(bb->obj,
> +					NULL, 0, 0, 0);
> +			if (IS_ERR(bb->vma)) {
> +				ret = PTR_ERR(bb->vma);
> +				goto err;
> +			}
>  
> -		/* No one is going to touch shadow bb from now on. */
> -		if (bb->clflush & CLFLUSH_AFTER) {
> -			drm_clflush_virt_range(bb->va, bb->obj->base.size);
> -			bb->clflush &= ~CLFLUSH_AFTER;
> -		}
> +			/* relocate shadow batch buffer */
> +			bb->bb_start_cmd_va[1] = i915_ggtt_offset(bb->vma);
> +			if (gmadr_bytes == 8)
> +				bb->bb_start_cmd_va[2] = 0;
>  
> -		ret = i915_gem_object_set_to_gtt_domain(bb->obj, false);
> -		if (ret)
> -			goto err;
> +			/* No one is going to touch shadow bb from now on. */
> +			if (bb->clflush & CLFLUSH_AFTER) {
> +				drm_clflush_virt_range(bb->va,
> +						bb->obj->base.size);
> +				bb->clflush &= ~CLFLUSH_AFTER;
> +			}
>  
> -		i915_gem_obj_finish_shmem_access(bb->obj);
> -		bb->accessing = false;
> +			ret = i915_gem_object_set_to_gtt_domain(bb->obj,
> +					false);
> +			if (ret)
> +				goto err;
>  
> -		i915_vma_move_to_active(bb->vma, workload->req, 0);
> +			i915_gem_obj_finish_shmem_access(bb->obj);
> +			bb->accessing = false;
> +
> +			i915_vma_move_to_active(bb->vma, workload->req, 0);
> +		}
>  	}
>  	return 0;
>  err:
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.h b/drivers/gpu/drm/i915/gvt/scheduler.h
> index 486ed57..6c64478 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.h
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.h
> @@ -125,6 +125,7 @@ struct intel_vgpu_shadow_bb {
>  	unsigned int clflush;
>  	bool accessing;
>  	unsigned long bb_offset;
> +	bool ppgtt;
>  };
>  
>  #define workload_q_head(vgpu, ring_id) \
> diff --git a/drivers/gpu/drm/i915/gvt/trace.h b/drivers/gpu/drm/i915/gvt/trace.h
> index 82093f1..1fd6420 100644
> --- a/drivers/gpu/drm/i915/gvt/trace.h
> +++ b/drivers/gpu/drm/i915/gvt/trace.h
> @@ -224,19 +224,25 @@
>  	TP_printk("%s", __entry->buf)
>  );
>  
> +#define GVT_CMD_STR_LEN 40
>  TRACE_EVENT(gvt_command,
> -	TP_PROTO(u8 vgpu_id, u8 ring_id, u32 ip_gma, u32 *cmd_va, u32 cmd_len,
> -		 u32 buf_type),
> +	TP_PROTO(u8 vgpu_id, u8 ring_id, u32 ip_gma, u32 *cmd_va,
> +		u32 cmd_len,  u32 buf_type, u32 buf_addr_type,
> +		void *workload, char *cmd_name),
>  
> -	TP_ARGS(vgpu_id, ring_id, ip_gma, cmd_va, cmd_len, buf_type),
> +	TP_ARGS(vgpu_id, ring_id, ip_gma, cmd_va, cmd_len, buf_type,
> +		buf_addr_type, workload, cmd_name),
>  
>  	TP_STRUCT__entry(
>  		__field(u8, vgpu_id)
>  		__field(u8, ring_id)
>  		__field(u32, ip_gma)
>  		__field(u32, buf_type)
> +		__field(u32, buf_addr_type)
>  		__field(u32, cmd_len)
> +		__field(void*, workload)
>  		__dynamic_array(u32, raw_cmd, cmd_len)
> +		__array(char, cmd_name, GVT_CMD_STR_LEN)
>  	),
>  
>  	TP_fast_assign(
> @@ -244,17 +250,25 @@
>  		__entry->ring_id = ring_id;
>  		__entry->ip_gma = ip_gma;
>  		__entry->buf_type = buf_type;
> +		__entry->buf_addr_type = buf_addr_type;
>  		__entry->cmd_len = cmd_len;
> +		__entry->workload = workload;
> +		snprintf(__entry->cmd_name, GVT_CMD_STR_LEN, "%s", cmd_name);
>  		memcpy(__get_dynamic_array(raw_cmd), cmd_va, cmd_len * sizeof(*cmd_va));
>  	),
>  
>  
> -	TP_printk("vgpu%d ring %d: buf_type %u, ip_gma %08x, raw cmd %s",
> +	TP_printk("vgpu%d ring %d: address_type %u, buf_type %u, ip_gma %08x,cmd (name=%s,len=%u,raw cmd=%s), workload=%p\n",
>  		__entry->vgpu_id,
>  		__entry->ring_id,
> +		__entry->buf_addr_type,
>  		__entry->buf_type,
>  		__entry->ip_gma,
> -		__print_array(__get_dynamic_array(raw_cmd), __entry->cmd_len, 4))
> +		__entry->cmd_name,
> +		__entry->cmd_len,
> +		__print_array(__get_dynamic_array(raw_cmd),
> +			__entry->cmd_len, 4),
> +		__entry->workload)
>  );
>  
>  #define GVT_TEMP_STR_LEN 10
> -- 
> 1.9.1
> 
> _______________________________________________
> 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/20180403/90798a92/attachment.sig>


More information about the intel-gvt-dev mailing list