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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 29 Jul 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> 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@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.

Agreed.


BR,
Jani.


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

-- 
Jani Nikula, Intel