Intel-GFX Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] drm/i915: Add perf property support for context HW id
@ 2017-07-19  5:39 Zhenyu Wang
  2017-07-19  5:39 ` [PATCH v2 2/2] drm/i915/gvt: expose vGPU context hw id Zhenyu Wang
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Zhenyu Wang @ 2017-07-19  5:39 UTC (permalink / raw)
  To: intel-gfx; +Cc: intel-gvt-dev, Jiao Pengyuan

In order to support profiling for special context e.g vGPU context,
we can expose vGPU context hw id and enable i915 perf property to
get target context for profiling. This adds new perf property to
assign target context with hw id.

Jiao Pengyuan has helped to fix context reference bug in original code.

v2:
- new function for context lookup with hw id
- always require priviledged op
- fix error return value

Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jiao Pengyuan <pengyuan.jiao@intel.com>
Cc: Niu Bing <bing.niu@intel.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 83 ++++++++++++++++++++++++++++++++--------
 include/uapi/drm/i915_drm.h      |  5 +++
 2 files changed, 72 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 96682fd86f82..a80a3959bc3b 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -334,7 +334,9 @@ static struct i915_oa_format gen8_plus_oa_formats[I915_OA_FORMAT_MAX] = {
  * struct perf_open_properties - for validated properties given to open a stream
  * @sample_flags: `DRM_I915_PERF_PROP_SAMPLE_*` properties are tracked as flags
  * @single_context: Whether a single or all gpu contexts should be monitored
+ * @single_context_hw: Set for special context with hw id e.g vGPU
  * @ctx_handle: A gem ctx handle for use with @single_context
+ * @ctx_hw_id: A ctx hw id for use with @single_context_hw
  * @metrics_set: An ID for an OA unit metric set advertised via sysfs
  * @oa_format: An OA unit HW report format
  * @oa_periodic: Whether to enable periodic OA unit sampling
@@ -348,7 +350,9 @@ struct perf_open_properties {
 	u32 sample_flags;
 
 	u64 single_context:1;
+	u64 single_context_hw:1;
 	u64 ctx_handle;
+	u64 ctx_hw_id;
 
 	/* OA sampling state */
 	int metrics_set;
@@ -2482,6 +2486,29 @@ static const struct file_operations fops = {
 	.unlocked_ioctl	= i915_perf_ioctl,
 };
 
+static struct i915_gem_context *
+lookup_context_hw_id(struct drm_i915_private *dev_priv, unsigned int hw_id)
+{
+	struct i915_gem_context *ctx;
+	int ret;
+
+	ret = i915_mutex_lock_interruptible(&dev_priv->drm);
+	if (ret)
+		return ERR_PTR(ret);
+
+	list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
+		if (!i915_gem_context_is_default(ctx))
+			continue;
+
+		if (ctx->hw_id == hw_id) {
+			ret = 1;
+			i915_gem_context_get(ctx);
+			break;
+		}
+	}
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+	return ret ? ctx : NULL;
+}
 
 /**
  * i915_perf_open_ioctl_locked - DRM ioctl() for userspace to open a stream FD
@@ -2531,24 +2558,35 @@ i915_perf_open_ioctl_locked(struct drm_i915_private *dev_priv,
 			ret = -ENOENT;
 			goto err;
 		}
+
+		/*
+		 * On Haswell the OA unit supports clock gating off for a specific
+		 * context and in this mode there's no visibility of metrics for the
+		 * rest of the system, which we consider acceptable for a
+		 * non-privileged client.
+		 *
+		 * For Gen8+ the OA unit no longer supports clock gating off for a
+		 * specific context and the kernel can't securely stop the counters
+		 * from updating as system-wide / global values. Even though we can
+		 * filter reports based on the included context ID we can't block
+		 * clients from seeing the raw / global counter values via
+		 * MI_REPORT_PERF_COUNT commands and so consider it a privileged op to
+		 * enable the OA unit by default.
+		 */
+		if (IS_HASWELL(dev_priv) && specific_ctx)
+			privileged_op = false;
 	}
 
-	/*
-	 * On Haswell the OA unit supports clock gating off for a specific
-	 * context and in this mode there's no visibility of metrics for the
-	 * rest of the system, which we consider acceptable for a
-	 * non-privileged client.
-	 *
-	 * For Gen8+ the OA unit no longer supports clock gating off for a
-	 * specific context and the kernel can't securely stop the counters
-	 * from updating as system-wide / global values. Even though we can
-	 * filter reports based on the included context ID we can't block
-	 * clients from seeing the raw / global counter values via
-	 * MI_REPORT_PERF_COUNT commands and so consider it a privileged op to
-	 * enable the OA unit by default.
-	 */
-	if (IS_HASWELL(dev_priv) && specific_ctx)
-		privileged_op = false;
+	if (props->single_context_hw) {
+		unsigned int hw_id = props->ctx_hw_id;
+
+		specific_ctx = lookup_context_hw_id(dev_priv, hw_id);
+		if (IS_ERR_OR_NULL(specific_ctx)) {
+			DRM_DEBUG("Failed to look up HW context id.\n");
+			ret = -ENOENT;
+			goto err;
+		}
+	}
 
 	/* Similar to perf's kernel.perf_paranoid_cpu sysctl option
 	 * we check a dev.i915.perf_stream_paranoid sysctl option
@@ -2686,6 +2724,14 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv,
 			props->single_context = 1;
 			props->ctx_handle = value;
 			break;
+		case DRM_I915_PERF_PROP_CTX_HW_ID:
+			if (value >= MAX_CONTEXT_HW_ID) {
+				DRM_DEBUG("Invalid OA HW context ID\n");
+				return -EINVAL;
+			}
+			props->single_context_hw = 1;
+			props->ctx_hw_id = value;
+			break;
 		case DRM_I915_PERF_PROP_SAMPLE_OA:
 			props->sample_flags |= SAMPLE_OA_REPORT;
 			break;
@@ -2757,6 +2803,11 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv,
 		uprop += 2;
 	}
 
+	if (props->single_context && props->single_context_hw) {
+		DRM_DEBUG("Assign context handler and HW id simultaneously\n");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7ccbd6a2bbe0..ddafa556e290 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1381,6 +1381,11 @@ enum drm_i915_perf_property_id {
 	 */
 	DRM_I915_PERF_PROP_OA_EXPONENT,
 
+	/**
+	 * The value specifies ctx with hw_id
+	 */
+	DRM_I915_PERF_PROP_CTX_HW_ID,
+
 	DRM_I915_PERF_PROP_MAX /* non-ABI */
 };
 
-- 
2.13.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/2] drm/i915/gvt: expose vGPU context hw id
  2017-07-19  5:39 [PATCH v2 1/2] drm/i915: Add perf property support for context HW id Zhenyu Wang
@ 2017-07-19  5:39 ` Zhenyu Wang
  2017-07-19  6:10 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/2] drm/i915: Add perf property support for context HW id Patchwork
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Zhenyu Wang @ 2017-07-19  5:39 UTC (permalink / raw)
  To: intel-gfx; +Cc: intel-gvt-dev, Jiao Pengyuan

This exposes vGPU context hw id in mdev sysfs which is used to
do vGPU based profiling. Retrieved vGPU context hw id can be set
through i915 perf ioctl to set profiling for target vGPU.

Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jiao Pengyuan <pengyuan.jiao@intel.com>
Cc: Niu Bing <bing.niu@intel.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/gvt/kvmgt.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index fd0c85f9ef3c..83e88c70272a 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1170,10 +1170,27 @@ vgpu_id_show(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "\n");
 }
 
+static ssize_t
+hw_id_show(struct device *dev, struct device_attribute *attr,
+	   char *buf)
+{
+	struct mdev_device *mdev = mdev_from_dev(dev);
+
+	if (mdev) {
+		struct intel_vgpu *vgpu = (struct intel_vgpu *)
+			mdev_get_drvdata(mdev);
+		return sprintf(buf, "%u\n",
+			       vgpu->shadow_ctx->hw_id);
+	}
+	return sprintf(buf, "\n");
+}
+
 static DEVICE_ATTR_RO(vgpu_id);
+static DEVICE_ATTR_RO(hw_id);
 
 static struct attribute *intel_vgpu_attrs[] = {
 	&dev_attr_vgpu_id.attr,
+	&dev_attr_hw_id.attr,
 	NULL
 };
 
-- 
2.13.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [v2,1/2] drm/i915: Add perf property support for context HW id
  2017-07-19  5:39 [PATCH v2 1/2] drm/i915: Add perf property support for context HW id Zhenyu Wang
  2017-07-19  5:39 ` [PATCH v2 2/2] drm/i915/gvt: expose vGPU context hw id Zhenyu Wang
@ 2017-07-19  6:10 ` Patchwork
  2017-07-21 10:50 ` [PATCH v2 1/2] " Lionel Landwerlin
  2017-07-21 11:04 ` Chris Wilson
  3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2017-07-19  6:10 UTC (permalink / raw)
  To: Zhenyu Wang; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v2,1/2] drm/i915: Add perf property support for context HW id
URL   : https://patchwork.freedesktop.org/series/27547/
State : success

== Summary ==

Series 27547v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/27547/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) k.org#196399
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-byt-n2820) fdo#101705

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
k.org#196399 https://bugzilla.kernel.org/show_bug.cgi?id=196399
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:436s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:427s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:354s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:529s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:511s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:486s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:494s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:601s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:437s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:410s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:416s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:485s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:470s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7560u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:572s
fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:575s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:558s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:455s
fi-skl-6700hq    total:279  pass:262  dwarn:0   dfail:0   fail:0   skip:17  time:581s
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:464s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:483s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:435s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:472s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:545s
fi-snb-2600      total:279  pass:249  dwarn:0   dfail:0   fail:1   skip:29  time:404s

10de1e17faaab452782e5a1baffd1b30a639a261 drm-tip: 2017y-07m-18d-10h-08m-42s UTC integration manifest
3cb4c1c drm/i915/gvt: expose vGPU context hw id
ae89000 drm/i915: Add perf property support for context HW id

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5229/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] drm/i915: Add perf property support for context HW id
  2017-07-19  5:39 [PATCH v2 1/2] drm/i915: Add perf property support for context HW id Zhenyu Wang
  2017-07-19  5:39 ` [PATCH v2 2/2] drm/i915/gvt: expose vGPU context hw id Zhenyu Wang
  2017-07-19  6:10 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/2] drm/i915: Add perf property support for context HW id Patchwork
@ 2017-07-21 10:50 ` Lionel Landwerlin
  2017-07-21 11:04 ` Chris Wilson
  3 siblings, 0 replies; 8+ messages in thread
From: Lionel Landwerlin @ 2017-07-21 10:50 UTC (permalink / raw)
  To: Zhenyu Wang, intel-gfx; +Cc: intel-gvt-dev, Jiao Pengyuan

Just a small nit down there.

On 19/07/17 06:39, Zhenyu Wang wrote:
> In order to support profiling for special context e.g vGPU context,
> we can expose vGPU context hw id and enable i915 perf property to
> get target context for profiling. This adds new perf property to
> assign target context with hw id.
>
> Jiao Pengyuan has helped to fix context reference bug in original code.
>
> v2:
> - new function for context lookup with hw id
> - always require priviledged op
> - fix error return value
>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jiao Pengyuan <pengyuan.jiao@intel.com>
> Cc: Niu Bing <bing.niu@intel.com>
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_perf.c | 83 ++++++++++++++++++++++++++++++++--------
>   include/uapi/drm/i915_drm.h      |  5 +++
>   2 files changed, 72 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 96682fd86f82..a80a3959bc3b 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -334,7 +334,9 @@ static struct i915_oa_format gen8_plus_oa_formats[I915_OA_FORMAT_MAX] = {
>    * struct perf_open_properties - for validated properties given to open a stream
>    * @sample_flags: `DRM_I915_PERF_PROP_SAMPLE_*` properties are tracked as flags
>    * @single_context: Whether a single or all gpu contexts should be monitored
> + * @single_context_hw: Set for special context with hw id e.g vGPU
>    * @ctx_handle: A gem ctx handle for use with @single_context
> + * @ctx_hw_id: A ctx hw id for use with @single_context_hw
>    * @metrics_set: An ID for an OA unit metric set advertised via sysfs
>    * @oa_format: An OA unit HW report format
>    * @oa_periodic: Whether to enable periodic OA unit sampling
> @@ -348,7 +350,9 @@ struct perf_open_properties {
>   	u32 sample_flags;
>   
>   	u64 single_context:1;
> +	u64 single_context_hw:1;
>   	u64 ctx_handle;
> +	u64 ctx_hw_id;
>   
>   	/* OA sampling state */
>   	int metrics_set;
> @@ -2482,6 +2486,29 @@ static const struct file_operations fops = {
>   	.unlocked_ioctl	= i915_perf_ioctl,
>   };
>   
> +static struct i915_gem_context *
> +lookup_context_hw_id(struct drm_i915_private *dev_priv, unsigned int hw_id)
> +{
> +	struct i915_gem_context *ctx;
> +	int ret;
> +
> +	ret = i915_mutex_lock_interruptible(&dev_priv->drm);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
> +		if (!i915_gem_context_is_default(ctx))
> +			continue;
> +
> +		if (ctx->hw_id == hw_id) {
> +			ret = 1;
> +			i915_gem_context_get(ctx);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
> +	return ret ? ctx : NULL;

I think you should replace NULL by ERR_PTR(-ENOENT) here, that would 
simplify the call to lookup_context_hw_id().

> +}
>   
>   /**
>    * i915_perf_open_ioctl_locked - DRM ioctl() for userspace to open a stream FD
> @@ -2531,24 +2558,35 @@ i915_perf_open_ioctl_locked(struct drm_i915_private *dev_priv,
>   			ret = -ENOENT;
>   			goto err;
>   		}
> +
> +		/*
> +		 * On Haswell the OA unit supports clock gating off for a specific
> +		 * context and in this mode there's no visibility of metrics for the
> +		 * rest of the system, which we consider acceptable for a
> +		 * non-privileged client.
> +		 *
> +		 * For Gen8+ the OA unit no longer supports clock gating off for a
> +		 * specific context and the kernel can't securely stop the counters
> +		 * from updating as system-wide / global values. Even though we can
> +		 * filter reports based on the included context ID we can't block
> +		 * clients from seeing the raw / global counter values via
> +		 * MI_REPORT_PERF_COUNT commands and so consider it a privileged op to
> +		 * enable the OA unit by default.
> +		 */
> +		if (IS_HASWELL(dev_priv) && specific_ctx)
> +			privileged_op = false;
>   	}
>   
> -	/*
> -	 * On Haswell the OA unit supports clock gating off for a specific
> -	 * context and in this mode there's no visibility of metrics for the
> -	 * rest of the system, which we consider acceptable for a
> -	 * non-privileged client.
> -	 *
> -	 * For Gen8+ the OA unit no longer supports clock gating off for a
> -	 * specific context and the kernel can't securely stop the counters
> -	 * from updating as system-wide / global values. Even though we can
> -	 * filter reports based on the included context ID we can't block
> -	 * clients from seeing the raw / global counter values via
> -	 * MI_REPORT_PERF_COUNT commands and so consider it a privileged op to
> -	 * enable the OA unit by default.
> -	 */
> -	if (IS_HASWELL(dev_priv) && specific_ctx)
> -		privileged_op = false;
> +	if (props->single_context_hw) {
> +		unsigned int hw_id = props->ctx_hw_id;
> +
> +		specific_ctx = lookup_context_hw_id(dev_priv, hw_id);
> +		if (IS_ERR_OR_NULL(specific_ctx)) {
> +			DRM_DEBUG("Failed to look up HW context id.\n");
> +			ret = -ENOENT;
> +			goto err;
> +		}

If you change lookup_context_hw_id() as indicated above, this can be 
turned into :

if (IS_ERR(specific_ctx)) {
    DRM_DEBUG(...);
    ret = PTR_ERR(specific_ctx);
    goto err;
}

> +	}
>   
>   	/* Similar to perf's kernel.perf_paranoid_cpu sysctl option
>   	 * we check a dev.i915.perf_stream_paranoid sysctl option
> @@ -2686,6 +2724,14 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv,
>   			props->single_context = 1;
>   			props->ctx_handle = value;
>   			break;
> +		case DRM_I915_PERF_PROP_CTX_HW_ID:
> +			if (value >= MAX_CONTEXT_HW_ID) {
> +				DRM_DEBUG("Invalid OA HW context ID\n");
> +				return -EINVAL;
> +			}
> +			props->single_context_hw = 1;
> +			props->ctx_hw_id = value;
> +			break;
>   		case DRM_I915_PERF_PROP_SAMPLE_OA:
>   			props->sample_flags |= SAMPLE_OA_REPORT;
>   			break;
> @@ -2757,6 +2803,11 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv,
>   		uprop += 2;
>   	}
>   
> +	if (props->single_context && props->single_context_hw) {
> +		DRM_DEBUG("Assign context handler and HW id simultaneously\n");
> +		return -EINVAL;
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 7ccbd6a2bbe0..ddafa556e290 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1381,6 +1381,11 @@ enum drm_i915_perf_property_id {
>   	 */
>   	DRM_I915_PERF_PROP_OA_EXPONENT,
>   
> +	/**
> +	 * The value specifies ctx with hw_id
> +	 */
> +	DRM_I915_PERF_PROP_CTX_HW_ID,
> +
>   	DRM_I915_PERF_PROP_MAX /* non-ABI */
>   };
>   


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] drm/i915: Add perf property support for context HW id
  2017-07-19  5:39 [PATCH v2 1/2] drm/i915: Add perf property support for context HW id Zhenyu Wang
                   ` (2 preceding siblings ...)
  2017-07-21 10:50 ` [PATCH v2 1/2] " Lionel Landwerlin
@ 2017-07-21 11:04 ` Chris Wilson
  2017-07-21 13:01   ` Lionel Landwerlin
  3 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2017-07-21 11:04 UTC (permalink / raw)
  To: Zhenyu Wang, intel-gfx; +Cc: intel-gvt-dev, Jiao Pengyuan

Quoting Zhenyu Wang (2017-07-19 06:39:48)
> +static struct i915_gem_context *
> +lookup_context_hw_id(struct drm_i915_private *dev_priv, unsigned int hw_id)
> +{
> +       struct i915_gem_context *ctx;
> +       int ret;
> +
> +       ret = i915_mutex_lock_interruptible(&dev_priv->drm);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
> +               if (!i915_gem_context_is_default(ctx))
> +                       continue;

This is still a massive what? Why ban normal contexts? Why are you not
banning the kernel context?

> +
> +               if (ctx->hw_id == hw_id) {
> +                       ret = 1;
> +                       i915_gem_context_get(ctx);
> +                       break;

Bad news, your reference counting is still broken. You actually need an
i915_gem_context_get_rcu() variant for pulling from this list.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] drm/i915: Add perf property support for context HW id
  2017-07-21 11:04 ` Chris Wilson
@ 2017-07-21 13:01   ` Lionel Landwerlin
  2017-07-25  4:26     ` Zhenyu Wang
  0 siblings, 1 reply; 8+ messages in thread
From: Lionel Landwerlin @ 2017-07-21 13:01 UTC (permalink / raw)
  To: Chris Wilson, Zhenyu Wang, intel-gfx; +Cc: intel-gvt-dev, Jiao Pengyuan

I think Chris' comments show this isn't actually tested.
Can you please write at least one test case for igt ?

We have a pretty long list of patches that need to land (still need 
review on some patches).
I would recommend you base you patches on this :

https://github.com/djdeath/intel-gpu-tools/tree/wip/djdeath/oa-next

Once your tests are passing, I'll put them in that branch.

Thanks!

On 21/07/17 12:04, Chris Wilson wrote:
> Quoting Zhenyu Wang (2017-07-19 06:39:48)
>> +static struct i915_gem_context *
>> +lookup_context_hw_id(struct drm_i915_private *dev_priv, unsigned int hw_id)
>> +{
>> +       struct i915_gem_context *ctx;
>> +       int ret;
>> +
>> +       ret = i915_mutex_lock_interruptible(&dev_priv->drm);
>> +       if (ret)
>> +               return ERR_PTR(ret);
>> +
>> +       list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
>> +               if (!i915_gem_context_is_default(ctx))
>> +                       continue;
> This is still a massive what? Why ban normal contexts? Why are you not
> banning the kernel context?
>
>> +
>> +               if (ctx->hw_id == hw_id) {
>> +                       ret = 1;
>> +                       i915_gem_context_get(ctx);
>> +                       break;
> Bad news, your reference counting is still broken. You actually need an
> i915_gem_context_get_rcu() variant for pulling from this list.
> -Chris
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] drm/i915: Add perf property support for context HW id
  2017-07-21 13:01   ` Lionel Landwerlin
@ 2017-07-25  4:26     ` Zhenyu Wang
  2017-07-25 11:04       ` Lionel Landwerlin
  0 siblings, 1 reply; 8+ messages in thread
From: Zhenyu Wang @ 2017-07-25  4:26 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: intel-gfx, intel-gvt-dev, Jiao Pengyuan


[-- Attachment #1.1: Type: text/plain, Size: 425 bytes --]

On 2017.07.21 14:01:01 +0100, Lionel Landwerlin wrote:
> I think Chris' comments show this isn't actually tested.

It turned out that's true...so currently Pengyuan just tried to
filter by exposed vGPU ctx_hw_id with global mode in gputop. Would
that be ok with you? If yes, then we don't need i915 perf change.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] drm/i915: Add perf property support for context HW id
  2017-07-25  4:26     ` Zhenyu Wang
@ 2017-07-25 11:04       ` Lionel Landwerlin
  0 siblings, 0 replies; 8+ messages in thread
From: Lionel Landwerlin @ 2017-07-25 11:04 UTC (permalink / raw)
  To: Zhenyu Wang; +Cc: intel-gfx, intel-gvt-dev, Jiao Pengyuan

On 25/07/17 05:26, Zhenyu Wang wrote:
> On 2017.07.21 14:01:01 +0100, Lionel Landwerlin wrote:
>> I think Chris' comments show this isn't actually tested.
> It turned out that's true...so currently Pengyuan just tried to
> filter by exposed vGPU ctx_hw_id with global mode in gputop. Would
> that be ok with you? If yes, then we don't need i915 perf change.
>
If that works for you, it's great.
I kind of like to have the hw_id in userspace too, it's pretty handy to 
debug IGT tests :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-07-25 11:04 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-19  5:39 [PATCH v2 1/2] drm/i915: Add perf property support for context HW id Zhenyu Wang
2017-07-19  5:39 ` [PATCH v2 2/2] drm/i915/gvt: expose vGPU context hw id Zhenyu Wang
2017-07-19  6:10 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/2] drm/i915: Add perf property support for context HW id Patchwork
2017-07-21 10:50 ` [PATCH v2 1/2] " Lionel Landwerlin
2017-07-21 11:04 ` Chris Wilson
2017-07-21 13:01   ` Lionel Landwerlin
2017-07-25  4:26     ` Zhenyu Wang
2017-07-25 11:04       ` Lionel Landwerlin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox