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

[Lucas De Marchi]

On Wed, Apr 24, 2024 at 04:26:44PM GMT, Ashutosh Dixit wrote:
>On Thu, 14 Mar 2024 18:35:13 -0700, Ashutosh Dixit wrote:
>>
>> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>> index 4bfa06ebf6da..54d0912f2ba8 100644
>>
>> +/**
>> + * struct drm_xe_oa_unit - describe OA unit
>> + */
>> +struct drm_xe_oa_unit {
>> +	/** @extensions: Pointer to the first extension struct, if any */
>> +	__u64 extensions;
>> +
>> +	/** @oa_unit_id: OA unit ID */
>> +	__u32 oa_unit_id;
>> +
>> +	/** @oa_unit_type: OA unit type of @drm_xe_oa_unit_type */
>> +	__u32 oa_unit_type;
>> +
>> +	/** @capabilities: OA capabilities bit-mask */
>> +	__u64 capabilities;
>> +#define DRM_XE_OA_CAPS_BASE		(1 << 0)
>> +
>> +	/** @oa_timestamp_freq: OA timestamp freq */
>> +	__u64 oa_timestamp_freq;
>> +
>> +	/** @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[];
>> +};
>> +
>> +/**
>> + * 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.
>> + *
>> + * OA unit properties for all OA units can be accessed using a code block
>> + * such as the one below:
>> + *
>> + * .. code-block:: C
>> + *
>> + *	struct drm_xe_query_oa_units *qoa;
>> + *	struct drm_xe_oa_unit *oau;
>> + *	u8 *poau;
>> + *
>> + *	// malloc qoa and issue DRM_XE_DEVICE_QUERY_OA_UNITS. Then:
>> + *	poau = (u8 *)&qoa->oa_units[0];
>> + *	for (int i = 0; i < qoa->num_oa_units; i++) {
>> + *		oau = (struct drm_xe_oa_unit *)poau;
>> + *		// Access 'struct drm_xe_oa_unit' fields here
>> + *		poau += sizeof(*oau) + oau->num_engines * sizeof(oau->eci[0]);
>> + *	}
>> + */
>> +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;
>> +	/** @oa_units: OA units returned for this device */
>> +	struct drm_xe_oa_unit oa_units[];
>> +};
>
>It has been pointed out that the doubly nested flexible arrays used here
>break compilation on Windows due to this MSVC issue:
>
>https://learn.microsoft.com/en-us/cpp/error-messages/compiler-errors-1/compiler-error-c2233?view=msvc-170
>
>Umesh had previously already pointed this out here:
>
>https://patchwork.freedesktop.org/patch/571290/?series=121084&rev=7#comment_1048210
>
>In any case, this is considered unacceptabe and needs to change.
>
>Current options:
>
>a. Include a "next" pointer in 'struct drm_xe_oa_unit' (suggested by
>   Lucas).
>
>   This works except people not familiar with the MSVC issue might wonder
>   why it is not simply an array, or why we are using a linked list
>   construct for something which is actually an array.

it's actually quite a common approach when you are parsing binary files
or protocol buffers:  you have a header that points to the first item
and each item points to the next one. Extending it to this query just
seemed natural to me, but I don't have a strong opinion right now on
which of them is better.

>
>b. I am thinking another option may be to just do this:
>
>   diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>   index 03c559af2027..6c864dc1b65f 100644
>   --- a/include/uapi/drm/xe_drm.h
>   +++ b/include/uapi/drm/xe_drm.h
>   @@ -1506,8 +1506,8 @@ struct drm_xe_query_oa_units {
>           __u32 num_oa_units;
>           /** @pad: MBZ */
>           __u32 pad;
>   -       /** @oa_units: OA units returned for this device */
>   -       struct drm_xe_oa_unit oa_units[];
>   +       /** @oa_units: struct drm_xe_oa_unit array returned for this device */
>   +       __u8 oa_units[];

given the expected use is to use just as an address that is then cast to
the actual type, I think that should work fine even if it looks ugly.

One caveat is about alignment: we'd in theory need to make this u64 so
it matches the alignment. Not a real issue here since all the UAPI
structs have pads to cover holes.

Lucas De Marchi

>    };
>
>   Though the next pointer has the advantage of it being simpler to reach
>   the nth element of the array.
>
>So I am still trying to figure out the least hacky way to change this. If
>you have any suggestions/opinion about this please respond.
>
>Thanks.
>--
>Ashutosh