[PATCH v2] drm/xe/uapi: restructure query config types in an enum

[Lucas De Marchi]

On Mon, Jul 29, 2024 at 03:15:26PM GMT, Ohad Sharabi wrote:
>The config query has entries per config type.
>
>In query_config(), an array is allocated using the last element of
>the query config to dictate the array length.
>
>Instead, a more robust approach would be to use the standard "number of
>elements" last element of an enum such that one wouldn't need to modify
>the ioctl each time another entry is added.
>
>v2:
> - modify `enum drm_xe_query_config_type` doc
>
>Signed-off-by: Ohad Sharabi <osharabi at habana.ai>
>---
> drivers/gpu/drm/xe/xe_query.c |  2 +-
> include/uapi/drm/xe_drm.h     | 58 ++++++++++++++++++++++-------------
> 2 files changed, 38 insertions(+), 22 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
>index 73ef6e4c2dc9..aaa60ff764ee 100644
>--- a/drivers/gpu/drm/xe/xe_query.c
>+++ b/drivers/gpu/drm/xe/xe_query.c
>@@ -313,7 +313,7 @@ static int query_mem_regions(struct xe_device *xe,
>
> static int query_config(struct xe_device *xe, struct drm_xe_device_query *query)
> {
>-	const u32 num_params = DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY + 1;
>+	const u32 num_params = DRM_XE_QUERY_CONFIG_NUM_CONFIGS;
> 	size_t size =
> 		sizeof(struct drm_xe_query_config) + num_params * sizeof(u64);
> 	struct drm_xe_query_config __user *query_ptr =
>diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>index 29425d7fdc77..9222563cddc3 100644
>--- a/include/uapi/drm/xe_drm.h
>+++ b/include/uapi/drm/xe_drm.h
>@@ -378,6 +378,42 @@ struct drm_xe_query_mem_regions {
> 	struct drm_xe_mem_region mem_regions[];
> };
>
>+#define DRM_XE_QUERY_CONFIG_FLAG_HAS_VRAM	(1 << 0)
>+
>+/**
>+ * enum drm_xe_query_config_type - Indices for the supported configs returned
>+ * in &drm_xe_query_config.info array
>+ */
>+enum drm_xe_query_config_type {
>+	/**
>+	 * @DRM_XE_QUERY_CONFIG_REV_AND_DEVICE_ID: Device ID (lower 16 bits)
>+	 * and the device revision (next 8 bits).
>+	 */
>+	DRM_XE_QUERY_CONFIG_REV_AND_DEVICE_ID,
>+	/**
>+	 * @DRM_XE_QUERY_CONFIG_FLAGS: Flags describing the device
>+	 * configuration, see list below.
>+	 *
>+	 *    - %DRM_XE_QUERY_CONFIG_FLAG_HAS_VRAM - Flag is set if the device
>+	 *      has usable VRAM
>+	 */
>+	DRM_XE_QUERY_CONFIG_FLAGS,
>+	/**
>+	 * @DRM_XE_QUERY_CONFIG_MIN_ALIGNMENT: Minimal memory alignment
>+	 * required by this device, typically SZ_4K or SZ_64K.
>+	 */
>+	DRM_XE_QUERY_CONFIG_MIN_ALIGNMENT,
>+	/** @DRM_XE_QUERY_CONFIG_VA_BITS: Maximum bits of a virtual address. */
>+	DRM_XE_QUERY_CONFIG_VA_BITS,
>+	/**
>+	 * @DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY: Value of the highest
>+	 * available exec queue priority.
>+	 */
>+	DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY,
>+	/** @DRM_XE_QUERY_CONFIG_NUM_CONFIGS: must be last. Number of configs */
>+	DRM_XE_QUERY_CONFIG_NUM_CONFIGS,

But then when we change from 5 to 6, we are changing the uapi.  It's
very possible that userspace was using that number and then when they
update the header the build breaks.

I believe that's why none of our enums contain a max/count/num element
at the end.

Lucas De Marchi

>+};
>+
> /**
>  * struct drm_xe_query_config - describe the device configuration
>  *
>@@ -385,33 +421,13 @@ struct drm_xe_query_mem_regions {
>  * is equal to DRM_XE_DEVICE_QUERY_CONFIG, then the reply uses
>  * struct drm_xe_query_config in .data.
>  *
>- * The index in @info can be:
>- *  - %DRM_XE_QUERY_CONFIG_REV_AND_DEVICE_ID - Device ID (lower 16 bits)
>- *    and the device revision (next 8 bits)
>- *  - %DRM_XE_QUERY_CONFIG_FLAGS - Flags describing the device
>- *    configuration, see list below
>- *
>- *    - %DRM_XE_QUERY_CONFIG_FLAG_HAS_VRAM - Flag is set if the device
>- *      has usable VRAM
>- *  - %DRM_XE_QUERY_CONFIG_MIN_ALIGNMENT - Minimal memory alignment
>- *    required by this device, typically SZ_4K or SZ_64K
>- *  - %DRM_XE_QUERY_CONFIG_VA_BITS - Maximum bits of a virtual address
>- *  - %DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY - Value of the highest
>- *    available exec queue priority
>+ * The index in @info are the entries in `enum drm_xe_query_config`
>  */
> struct drm_xe_query_config {
> 	/** @num_params: number of parameters returned in info */
> 	__u32 num_params;
>-
> 	/** @pad: MBZ */
> 	__u32 pad;
>-
>-#define DRM_XE_QUERY_CONFIG_REV_AND_DEVICE_ID	0
>-#define DRM_XE_QUERY_CONFIG_FLAGS			1
>-	#define DRM_XE_QUERY_CONFIG_FLAG_HAS_VRAM	(1 << 0)
>-#define DRM_XE_QUERY_CONFIG_MIN_ALIGNMENT		2
>-#define DRM_XE_QUERY_CONFIG_VA_BITS			3
>-#define DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY	4
> 	/** @info: array of elements containing the config info */
> 	__u64 info[];
> };
>-- 
>2.34.1
>