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

Mon Jul 29 13:41:19 UTC 2024

On Mon, Jul 29, 2024 at 04:19:25PM GMT, Jani Nikula wrote:
>On Mon, 29 Jul 2024, Lucas De Marchi <lucas.demarchi at 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 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.
>
>Agreed.

just another note that making this for the driver-only would be good. So
we only use the _MAX/_COUNT when iterating instead of hardcoding the
last element.  That'd mean defining a __DRM_XE_QUERY_CONFIG_NUM_CONFIGS
or the like in an internal header or .c if it's used in only one place.

Lucas De Marchi

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