Re: [PATCH v8 6/7] drm/xe/uapi: Add a device query to get EU stall sampling information

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

On Wed, 15 Jan 2025 12:02:12 -0800, Harish Chegondi wrote:
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index d9b20afc57c1..7d518f97ba34 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -700,6 +700,7 @@ struct drm_xe_device_query {
>  #define DRM_XE_DEVICE_QUERY_ENGINE_CYCLES	6
>  #define DRM_XE_DEVICE_QUERY_UC_FW_VERSION	7
>  #define DRM_XE_DEVICE_QUERY_OA_UNITS		8
> +#define DRM_XE_DEVICE_QUERY_EU_STALL		9
>	/** @query: The type of data to query */
>	__u32 query;
>
> @@ -1754,8 +1755,8 @@ enum drm_xe_eu_stall_property_id {
>	DRM_XE_EU_STALL_PROP_GT_ID = 1,
>
>	/**
> -	 * @DRM_XE_EU_STALL_PROP_SAMPLE_RATE: Sampling rate
> -	 * in GPU cycles.
> +	 * @DRM_XE_EU_STALL_PROP_SAMPLE_RATE: Sampling rate in
> +	 * GPU cycles from @sampling_rates in struct @drm_xe_query_eu_stall
>	 */
>	DRM_XE_EU_STALL_PROP_SAMPLE_RATE,
>
> @@ -1767,6 +1768,41 @@ enum drm_xe_eu_stall_property_id {
>	DRM_XE_EU_STALL_PROP_WAIT_NUM_REPORTS,
>  };
>
> +/**
> + * struct drm_xe_query_eu_stall - Information about EU stall sampling.
> + *
> + * If a query is made with a struct @drm_xe_device_query where .query
> + * is equal to @DRM_XE_DEVICE_QUERY_EU_STALL, then the reply uses
> + * struct @drm_xe_query_eu_stall in .data.
> + */
> +struct drm_xe_query_eu_stall {
> +	/** @extensions: Pointer to the first extension struct, if any */
> +	__u64 extensions;
> +
> +	/** @capabilities: EU stall capabilities bit-mask */
> +	__u64 capabilities;
> +#define DRM_XE_EU_STALL_CAPS_BASE		(1 << 0)
> +
> +	/** @record_size: size of each EU stall data record */
> +	__u64 record_size;
> +
> +	/** @per_xecore_buf_size: Per XeCore buffer size */
> +	__u64 per_xecore_buf_size;

Someone else should still probably check if the term "xecore" in
"per_xecore_buf_size" is appropriate. I don't know if it is, or if it is
future proof, as I had remarked earlier.

> +
> +	/** @num_sampling_rates: Number of sampling rates supported */
> +	__u64 num_sampling_rates;
> +
> +	/** @reserved: Reserved */
> +	__u64 reserved[5];

I think we should move this reserved array before num_sampling_rates. If
later we take up a reserved u64 (replace it by a different struct member)
we'd want num_sampling_rates and sampling_rates[] together.

> +
> +	/**
> +	 * @sampling_rates: Flexible array of sampling rates
> +	 * sorted in the fastest to slowest order.
> +	 * Sampling rates are specified in GPU clock cycles.
> +	 */
> +	__u64 sampling_rates[];
> +};

The Mesa PR still seems to have some data structs from an older version,
but after addressing the above comments, the uapi introduced in this series
is LGTM, so for the uapi:

Acked-by: Ashutosh Dixit <ashutosh.dixit@intel.com>