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 Tue, 21 Jan 2025 18:48:55 -0800, Harish Chegondi wrote:
>

Hi Harish,

> 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.

OK, in that case I am fine with this. Let's put this to rest :)

> >
> > > +
> > > +	/** @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.

Yes let's move it before num_sampling_rates. Look at 'struct
drm_xe_oa_unit'.

> >
> > > +
> > > +	/**
> > > +	 * @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>