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

[Gustavo Sousa]

Quoting Jani Nikula (2024-07-29 10:19:25-03:00)
>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.

I talked with Lucas and now I understand how this could cause problems
if userspace starts to rely on DRM_XE_QUERY_CONFIG_NUM_CONFIGS in a way
that could break when DRM_XE_QUERY_CONFIG_NUM_CONFIGS increases.

That said, IMHO, I believe DRM_XE_QUERY_CONFIG_NUM_CONFIGS shouldn't be
treated as a stable value across "releases" of the driver and assume
that it could increase.

But, yeah, we might want to avoid eventual problems by not exporting it
to userspace in the first place.

--
Gustavo Sousa

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