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

[Harish Chegondi <harish.chegondi@intel.com>]

On Thu, Jan 16, 2025 at 02:34:59PM -0800, Dixit, Ashutosh wrote:
> 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.
Had a chat with Matt Roper offline regarding this. He said XeCore is a
formal name in the hardware for a GPU core. So I think this is
appropriate.
> 
> > +
> > +	/** @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.
I noticed that in structures with reserved fields, the reserved
fields are at the bottom of the structure. Although flexible array 
sampling_rates is at the bottom, no storage is allocated for
sampling_rates. I can move the reserved array, but is it okay for
reserved array to not be at the end of the structure? Even if I move,
the num_sampling_rates and sampling_rates[] will be next to each other,
but no storage will be allocated to sampling_rates.
> 
> > +
> > +	/**
> > +	 * @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>