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

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

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.

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[];
    };

   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