Re: [PATCH 14/17] drm/xe/oa/uapi: Query OA unit properties

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

On Fri, 22 Dec 2023 16:40:47 -0800, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index 8156301df7315..5f41c5bfe5e0e 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -517,6 +517,7 @@ struct drm_xe_device_query {
> > #define DRM_XE_DEVICE_QUERY_HWCONFIG		4
> > #define DRM_XE_DEVICE_QUERY_GT_TOPOLOGY		5
> > #define DRM_XE_DEVICE_QUERY_ENGINE_CYCLES	6
> > +#define DRM_XE_DEVICE_QUERY_OA_UNITS		7
> >	/** @query: The type of data to query */
> >	__u32 query;
> >
> > @@ -1182,6 +1183,69 @@ enum drm_xe_oa_unit_type {
> >	DRM_XE_OA_UNIT_TYPE_OAM,
> > };
> >
> > +/**
> > + * struct drm_xe_query_oa_units - describe OA units
> > + *
> > + * If a query is made with a struct drm_xe_device_query where .query
> > + * is equal to DRM_XE_DEVICE_QUERY_OA_UNITS, then the reply uses struct
> > + * drm_xe_query_oa_units in .data.
> > + *
> > + * When there is an @open_stream, the query returns properties specific to
> > + * that @open_stream. Else default properties are returned.
> > + */
> > +struct drm_xe_query_oa_units {
> > +	/** @extensions: Pointer to the first extension struct, if any */
> > +	__u64 extensions;
> > +
> > +	/** @num_oa_units: number of OA units returned in oau[] */
> > +	__u32 num_oa_units;
> > +
> > +	/** @pad: MBZ */
> > +	__u32 pad;
> > +
> > +	/** @reserved: MBZ */
> > +	__u64 reserved[4];
>
> For some reason I have assumed reserved fields are added only at the end of
> the uApi struct, not sure though.

I have removed this in v8 and also brought 'struct drm_xe_query_oa_units'
in line with other query structs (see query_engines or query_mem_regions
e.g.).

>
> > +
> > +	/** @oa_units: OA units returned for this device */
> > +	struct drm_xe_oa_unit {
> > +		/** @oa_unit_id: OA unit ID */
> > +		__u16 oa_unit_id;
> > +
> > +		/** @oa_unit_type: OA unit type of @drm_xe_oa_unit_type */
> > +		__u16 oa_unit_type;
> > +
> > +		/** @gt_id: GT ID for this OA unit */
> > +		__u16 gt_id;
> > +
> > +		/** @open_stream: True if a stream is open on the OA unit */
> > +		__u16 open_stream;
> > +
> > +		/** @internal_events: True if internal events are available */
> > +		__u16 internal_events;
> > +
> > +		/** @pad: MBZ */
> > +		__u16 pad;
>
> __u16 pad[3] for 64bit alignment

internal_events and pad above are also removed.

> > +
> > +		/** @capabilities: OA capabilities bit-mask */
> > +		__u64 capabilities;
> > +
> > +		/** @oa_timestamp_freq: OA timestamp freq */
> > +		__u64 oa_timestamp_freq;
> > +
> > +		/** @oa_buf_size: OA buffer size */
> > +		__u64 oa_buf_size;
> > +
> > +		/** @reserved: MBZ */
> > +		__u64 reserved[4];
> > +
> > +		/** @num_engines: number of engines in @eci array */
> > +		__u64 num_engines;
> > +
> > +		/** @eci: engines attached to this OA unit */
> > +		struct drm_xe_engine_class_instance eci[];
> > +	} oa_units[];
>
> nesting of flexible arrays; not sure about that. i think some compilers may
> throw an error/warning. Sending an old message from Joonas offline.

From what I saw that old message is inconclusive. Windows guys have not
explained what they are doing and anyway why should Windows UMD talk to
Linux KMD. Windows can #ifdef the struct out if needed at their end.

It talks about this error:

https://learn.microsoft.com/en-us/cpp/error-messages/compiler-errors-1/compiler-error-c2233

	class B {
		char zeroarray[];
	};

	B array2[100];   // C2233

The above is obviously wrong but this is not we are doing in the above
struct (we have not sized the variable length struct, only indicated that a
variable length struct is inside another variable length struct, which is
legitimate as we index correctly into the arrays).

So I am ignoring this. Please let me know if you disagree. Or if you have
any suggestions about alternative ways of doing this, we could look into
it.

> In general, I feel the pad and reserved fields sprinkled into the
> structure. If we can avoid that in a way that they are all located at the
> end of the struct, I think that would look good. Not sure about the
> technical aspect though. I always assumed they were meant to be at the end
> (but then structs are nested anyways, so really not sure).

In v8 there is only a single reserved[4] array just before num_engines in
'struct drm_xe_oa_unit'. In case we need to add extra fields later on
(after that is num_engines and the variable length eci[] array which it's
better to keep together).


> > +};
> > +
> > /** enum drm_xe_oa_format_type - OA format types */
> > enum drm_xe_oa_format_type {
> >	DRM_XE_OA_FMT_TYPE_OAG,
> > --
> > 2.41.0
> >

Thanks.
--
Ashutosh