Re: [PATCH v4 4/5] drm/xe/query: Add a device query to get EU stall data information

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

On Sun, 13 Oct 2024 23:00:35 -0700, Harish Chegondi wrote:

Hi Harish,

Only reviewing the uapi for now (xe_drm.h changes). Mostly nits for this
patch.

>
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 50ad6b2e1450..f1a0f06b61f4 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_SAMPLING	9

Just call this DRM_XE_DEVICE_QUERY_EU_STALL?


>	/** @query: The type of data to query */
>	__u32 query;
>
> @@ -1738,6 +1739,25 @@ enum drm_xe_eu_stall_property_id {
>	DRM_XE_EU_STALL_PROP_GT_ID,
>  };
>
> +/**
> + * struct drm_xe_query_eu_stall_data - Information about EU stall data
> + *
> + * If a query is made with a struct drm_xe_device_query where .query
> + * is equal to DRM_XE_DEVICE_QUERY_EU_STALL_SAMPLING, then the reply uses
> + * struct drm_xe_query_eu_stall_data in .data.

Maybe add a @ before all references in this file. So @drm_xe_device_query,
@DRM_XE_DEVICE_QUERY_EU_STALL_SAMPLING and @drm_xe_query_eu_stall_data.

I think better to do this even if this hasn't been done for queries
elsewhere in this file, looks like a miss.

> + */
> +struct drm_xe_query_eu_stall_data {

Just call this 'struct drm_xe_query_eu_stall'?

> +	/** @extensions: Pointer to the first extension struct, if any */
> +	__u64 extensions;
> +
> +	/** @record_size: size of each EU stall data record */
> +	__u64 record_size;
> +
> +	/** @capabilities: EU stall capabilities bit-mask */
> +	__u64 capabilities;
> +#define DRM_XE_EU_STALL_CAPS_BASE		(1 << 0)
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> --
> 2.45.1
>

Thanks.
--
Ashutosh