Re: [Intel-gfx] [PATCH v3 8/9] drm/i915/perf: Add engine class instance parameters to perf

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 27 Feb 2023 18:23:28 -0800, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 8df261c5ab9b..8ce20004a9dd 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -2758,6 +2758,28 @@ enum drm_i915_perf_property_id {
>	 */
>	DRM_I915_PERF_PROP_POLL_OA_PERIOD,
>
> +	/**
> +	 * In platforms with multiple OA buffers, the engine class instance must

I'd say "one engine class instance".

> +	 * be passed to open a stream to a OA unit corresponding to the engine.
> +	 * Multiple engines may be mapped to the same OA unit.

Maybe (at the risk of over-stating) something like "Multiple engines may be
mapped to the same OA unit. The OA unit is identified by class:instance of
any engine mapped to it".

But it's ok to not change anything here.

> +	 *
> +	 * In addition to the class:instance, if a gem context is also passed, then
> +	 * 1) the report headers of OA reports from any contexts that do not
> +	 *    match this specific engine context are squashed.

This one I don't understand: we seem to be mixing gem contexts and engine
context here. Also, afais there are no changes related to this in this
series. This context squashing has always been happening, so why add this
comment to DRM_I915_PERF_PROP_OA_ENGINE_CLASS (or to this patch)? If we
want to clarify something maybe this comment should be added to
DRM_I915_PERF_PROP_CTX_HANDLE? But otherwise I think we should just drop
this comment, at least from this patch?

> +	 * 2) if the engine supports MI_REPORT_PERF_COUNT, this specific engine
> +	 *    context is configured for this command.
> +	 *
> +	 * This property is available in perf revision 6.
> +	 */
> +	DRM_I915_PERF_PROP_OA_ENGINE_CLASS,
> +
> +	/**
> +	 * This parameter specifies the engine instance.
> +	 *
> +	 * This property is available in perf revision 6.
> +	 */
> +	DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE,
> +
>	DRM_I915_PERF_PROP_MAX /* non-ABI */
>  };

Thanks.
--
Ashutosh