Re: [RFC PATCH 1/2] drm/i915: Add perf property support for context HW id

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

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