* Re: [RFC PATCH 1/2] drm/i915: Add perf property support for context HW id
2017-07-18 7:51 ` [RFC PATCH 1/2] drm/i915: Add perf property support for context HW id Zhenyu Wang
@ 2017-07-18 11:30 ` Lionel Landwerlin
2017-07-18 11:34 ` Chris Wilson
2017-07-18 11:33 ` Lionel Landwerlin
2017-07-18 11:43 ` Lionel Landwerlin
2 siblings, 1 reply; 9+ messages in thread
From: Lionel Landwerlin @ 2017-07-18 11:30 UTC (permalink / raw)
To: Zhenyu Wang, intel-gfx; +Cc: intel-gvt-dev, Pengyuan
Looks fine to me, down there a couple of suggestions.
Cheers,
-
Lionel
On 18/07/17 08:51, 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.
>
> Cc: Jiao, Pengyuan <pengyuan.jiao@intel.com>
> Cc: Niu, Bing <bing.niu@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_perf.c | 37 +++++++++++++++++++++++++++++++++++++
> include/uapi/drm/i915_drm.h | 5 +++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index d9f77a4d85db..350fd259b2d0 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -348,7 +348,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;
> @@ -2555,6 +2557,28 @@ i915_perf_open_ioctl_locked(struct drm_i915_private *dev_priv,
> }
> }
>
> + if (props->single_context_hw) {
> + struct i915_gem_context *ctx;
> +
> + mutex_lock(&dev_priv->drm.struct_mutex);
Maybe use i915_mutex_lock_interruptible() ?
> + list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
> + if (!i915_gem_context_is_default(ctx))
> + continue;
> +
> + if (ctx->hw_id == props->ctx_hw_id) {
> + specific_ctx = ctx;
> + i915_gem_context_get(specific_ctx);
> + break;
> + }
> + }
> + mutex_unlock(&dev_priv->drm.struct_mutex);
Maybe you could put the logic above into a function?
> +
> + if (!specific_ctx) {
> + DRM_DEBUG("Failed to look up HW context id.\n");
> + 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
> @@ -2708,6 +2732,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;
> @@ -2779,6 +2811,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] 9+ messages in thread
* Re: [RFC PATCH 1/2] drm/i915: Add perf property support for context HW id
2017-07-18 11:30 ` Lionel Landwerlin
@ 2017-07-18 11:34 ` Chris Wilson
2017-07-18 11:43 ` Lionel Landwerlin
0 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2017-07-18 11:34 UTC (permalink / raw)
To: Lionel Landwerlin, Zhenyu Wang, intel-gfx; +Cc: intel-gvt-dev, Pengyuan
Quoting Lionel Landwerlin (2017-07-18 12:30:10)
> Looks fine to me, down there a couple of suggestions.
>
> Cheers,
>
> -
> Lionel
>
> On 18/07/17 08:51, 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.
> >
> > Cc: Jiao, Pengyuan <pengyuan.jiao@intel.com>
> > Cc: Niu, Bing <bing.niu@intel.com>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Cc: intel-gvt-dev@lists.freedesktop.org
> > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_perf.c | 37 +++++++++++++++++++++++++++++++++++++
> > include/uapi/drm/i915_drm.h | 5 +++++
> > 2 files changed, 42 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index d9f77a4d85db..350fd259b2d0 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -348,7 +348,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;
> > @@ -2555,6 +2557,28 @@ i915_perf_open_ioctl_locked(struct drm_i915_private *dev_priv,
> > }
> > }
> >
> > + if (props->single_context_hw) {
> > + struct i915_gem_context *ctx;
> > +
> > + mutex_lock(&dev_priv->drm.struct_mutex);
>
> Maybe use i915_mutex_lock_interruptible() ?
>
> > + list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
> > + if (!i915_gem_context_is_default(ctx))
> > + continue;
> > +
> > + if (ctx->hw_id == props->ctx_hw_id) {
> > + specific_ctx = ctx;
> > + i915_gem_context_get(specific_ctx);
> > + break;
> > + }
> > + }
> > + mutex_unlock(&dev_priv->drm.struct_mutex);
>
> Maybe you could put the logic above into a function?
Please, please make sure this guarded by extreme paranoia. This indeed
has the opposite effect and allows any user to snoop on another.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 1/2] drm/i915: Add perf property support for context HW id
2017-07-18 11:34 ` Chris Wilson
@ 2017-07-18 11:43 ` Lionel Landwerlin
0 siblings, 0 replies; 9+ messages in thread
From: Lionel Landwerlin @ 2017-07-18 11:43 UTC (permalink / raw)
To: Chris Wilson, Zhenyu Wang, intel-gfx; +Cc: intel-gvt-dev, Pengyuan
On 18/07/17 12:34, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2017-07-18 12:30:10)
>> Looks fine to me, down there a couple of suggestions.
>>
>> Cheers,
>>
>> -
>> Lionel
>>
>> On 18/07/17 08:51, 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.
>>>
>>> Cc: Jiao, Pengyuan <pengyuan.jiao@intel.com>
>>> Cc: Niu, Bing <bing.niu@intel.com>
>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>> Cc: intel-gvt-dev@lists.freedesktop.org
>>> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_perf.c | 37 +++++++++++++++++++++++++++++++++++++
>>> include/uapi/drm/i915_drm.h | 5 +++++
>>> 2 files changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>>> index d9f77a4d85db..350fd259b2d0 100644
>>> --- a/drivers/gpu/drm/i915/i915_perf.c
>>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>>> @@ -348,7 +348,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;
>>> @@ -2555,6 +2557,28 @@ i915_perf_open_ioctl_locked(struct drm_i915_private *dev_priv,
>>> }
>>> }
>>>
>>> + if (props->single_context_hw) {
>>> + struct i915_gem_context *ctx;
>>> +
>>> + mutex_lock(&dev_priv->drm.struct_mutex);
>> Maybe use i915_mutex_lock_interruptible() ?
>>
>>> + list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
>>> + if (!i915_gem_context_is_default(ctx))
>>> + continue;
>>> +
>>> + if (ctx->hw_id == props->ctx_hw_id) {
>>> + specific_ctx = ctx;
>>> + i915_gem_context_get(specific_ctx);
>>> + break;
>>> + }
>>> + }
>>> + mutex_unlock(&dev_priv->drm.struct_mutex);
>> Maybe you could put the logic above into a function?
> Please, please make sure this guarded by extreme paranoia. This indeed
> has the opposite effect and allows any user to snoop on another.
> -Chris
>
Thanks Chris!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 1/2] drm/i915: Add perf property support for context HW id
2017-07-18 7:51 ` [RFC PATCH 1/2] drm/i915: Add perf property support for context HW id Zhenyu Wang
2017-07-18 11:30 ` Lionel Landwerlin
@ 2017-07-18 11:33 ` Lionel Landwerlin
2017-07-18 11:43 ` Lionel Landwerlin
2 siblings, 0 replies; 9+ messages in thread
From: Lionel Landwerlin @ 2017-07-18 11:33 UTC (permalink / raw)
To: Zhenyu Wang, intel-gfx; +Cc: intel-gvt-dev, Pengyuan
On 18/07/17 08:51, 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.
>
> Cc: Jiao, Pengyuan <pengyuan.jiao@intel.com>
> Cc: Niu, Bing <bing.niu@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_perf.c | 37 +++++++++++++++++++++++++++++++++++++
> include/uapi/drm/i915_drm.h | 5 +++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index d9f77a4d85db..350fd259b2d0 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -348,7 +348,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;
> @@ -2555,6 +2557,28 @@ i915_perf_open_ioctl_locked(struct drm_i915_private *dev_priv,
> }
> }
>
> + if (props->single_context_hw) {
> + struct i915_gem_context *ctx;
> +
> + mutex_lock(&dev_priv->drm.struct_mutex);
> + list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
> + if (!i915_gem_context_is_default(ctx))
> + continue;
> +
> + if (ctx->hw_id == props->ctx_hw_id) {
> + specific_ctx = ctx;
> + i915_gem_context_get(specific_ctx);
> + break;
> + }
> + }
> + mutex_unlock(&dev_priv->drm.struct_mutex);
> +
> + if (!specific_ctx) {
> + DRM_DEBUG("Failed to look up HW context id.\n");
And you're missing setting ret = -ENOENT here :)
> + 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
> @@ -2708,6 +2732,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;
> @@ -2779,6 +2811,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] 9+ messages in thread
* Re: [RFC PATCH 1/2] drm/i915: Add perf property support for context HW id
2017-07-18 7:51 ` [RFC PATCH 1/2] drm/i915: Add perf property support for context HW id Zhenyu Wang
2017-07-18 11:30 ` Lionel Landwerlin
2017-07-18 11:33 ` Lionel Landwerlin
@ 2017-07-18 11:43 ` Lionel Landwerlin
2 siblings, 0 replies; 9+ messages in thread
From: Lionel Landwerlin @ 2017-07-18 11:43 UTC (permalink / raw)
To: Zhenyu Wang, intel-gfx; +Cc: intel-gvt-dev, Pengyuan
On 18/07/17 08:51, 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.
>
> Cc: Jiao, Pengyuan <pengyuan.jiao@intel.com>
> Cc: Niu, Bing <bing.niu@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_perf.c | 37 +++++++++++++++++++++++++++++++++++++
> include/uapi/drm/i915_drm.h | 5 +++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index d9f77a4d85db..350fd259b2d0 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -348,7 +348,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;
> @@ -2555,6 +2557,28 @@ i915_perf_open_ioctl_locked(struct drm_i915_private *dev_priv,
> }
> }
>
> + if (props->single_context_hw) {
> + struct i915_gem_context *ctx;
> +
> + mutex_lock(&dev_priv->drm.struct_mutex);
> + list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
> + if (!i915_gem_context_is_default(ctx))
> + continue;
> +
> + if (ctx->hw_id == props->ctx_hw_id) {
> + specific_ctx = ctx;
> + i915_gem_context_get(specific_ctx);
> + break;
> + }
> + }
> + mutex_unlock(&dev_priv->drm.struct_mutex);
> +
> + if (!specific_ctx) {
> + DRM_DEBUG("Failed to look up HW context id.\n");
> + goto err;
> + }
> + }
Chris pointed something really important.
The condition below :
if (IS_HASWELL(dev_priv) && specific_ctx)
needs to be moved into the if (props->single_context) so we ensure that
only an operation using a context handle can be considered non
privileged on Haswell.
On Gen8+, everything requires privileged access though.
> +
> /*
> * 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
> @@ -2708,6 +2732,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;
> @@ -2779,6 +2811,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] 9+ messages in thread