[igt-dev] [PATCH v5 08/21] drm-uapi/xe: Make DRM_XE_DEVICE_QUERY_ENGINES future proof
Souza, Jose
jose.souza at intel.com
Fri Dec 1 14:09:09 UTC 2023
On Thu, 2023-11-30 at 18:45 +0000, Francois Dugast wrote:
> From: Rodrigo Vivi <rodrigo.vivi at intel.com>
>
> Align with kernel commit ("drm/xe: Make DRM_XE_DEVICE_QUERY_ENGINES future proof")
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Signed-off-by: Francois Dugast <francois.dugast at intel.com>
> ---
> benchmarks/gem_wsim.c | 2 +-
> include/drm-uapi/xe_drm.h | 24 +++++++++++++++++++++++-
> lib/xe/xe_query.c | 16 ++++++++--------
> lib/xe/xe_query.h | 8 ++++----
> tests/intel/xe_create.c | 7 ++++---
> tests/intel/xe_drm_fdinfo.c | 5 +++--
> tests/intel/xe_exec_store.c | 8 ++++----
> tests/intel/xe_intel_bb.c | 3 ++-
> tests/intel/xe_noexec_ping_pong.c | 5 +++--
> tests/intel/xe_waitfence.c | 6 +++---
> 10 files changed, 55 insertions(+), 29 deletions(-)
>
> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> index abbe49a06..514fa4ba7 100644
> --- a/benchmarks/gem_wsim.c
> +++ b/benchmarks/gem_wsim.c
> @@ -689,7 +689,7 @@ xe_get_default_engine(void)
> struct drm_xe_engine_class_instance default_hwe, *hwe;
>
> /* select RCS0 | CCS0 or first available engine */
> - default_hwe = *xe_engine(fd, 0);
> + default_hwe = xe_engine(fd, 0)->instance;
> xe_for_each_engine(fd, hwe) {
> if ((hwe->engine_class == DRM_XE_ENGINE_CLASS_RENDER ||
> hwe->engine_class == DRM_XE_ENGINE_CLASS_COMPUTE) &&
> diff --git a/include/drm-uapi/xe_drm.h b/include/drm-uapi/xe_drm.h
> index 01e0587dc..6e97270dc 100644
> --- a/include/drm-uapi/xe_drm.h
> +++ b/include/drm-uapi/xe_drm.h
> @@ -124,7 +124,14 @@ struct xe_user_extension {
> #define DRM_IOCTL_XE_EXEC_QUEUE_GET_PROPERTY DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_EXEC_QUEUE_GET_PROPERTY, struct drm_xe_exec_queue_get_property)
> #define DRM_IOCTL_XE_WAIT_USER_FENCE DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
>
> -/** struct drm_xe_engine_class_instance - instance of an engine class */
> +/**
> + * struct drm_xe_engine_class_instance - instance of an engine class
> + *
> + * It is returned as part of the @drm_xe_query_engine_info, but it also is
> + * used as the input of engine selection for both @drm_xe_exec_queue_create
> + * and @drm_xe_query_engine_cycles
> + *
> + */
> struct drm_xe_engine_class_instance {
> #define DRM_XE_ENGINE_CLASS_RENDER 0
> #define DRM_XE_ENGINE_CLASS_COPY 1
> @@ -145,6 +152,21 @@ struct drm_xe_engine_class_instance {
> __u16 pad;
> };
>
> +/**
> + * struct drm_xe_query_engine_info - describe hardware engine
> + *
> + * If a query is made with a struct @drm_xe_device_query where .query
> + * is equal to %DRM_XE_DEVICE_QUERY_ENGINES, then the reply uses an array of
> + * struct @drm_xe_query_engine_info in .data.
> + */
> +struct drm_xe_query_engine_info {
> + /** @instance: The @drm_xe_engine_class_instance */
> + struct drm_xe_engine_class_instance instance;
> +
> + /** @reserved: Reserved */
> + __u64 reserved[3];
> +};
> +
> /**
> * enum drm_xe_memory_class - Supported memory classes.
> */
> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> index ef7aaa6a1..f9dec1f7a 100644
> --- a/lib/xe/xe_query.c
> +++ b/lib/xe/xe_query.c
> @@ -72,10 +72,10 @@ static uint64_t __memory_regions(const struct drm_xe_query_gt_list *gt_list)
> return regions;
> }
>
> -static struct drm_xe_engine_class_instance *
> -xe_query_engines_new(int fd, unsigned int *num_engines)
> +static struct drm_xe_query_engine_info *
> +xe_query_engines(int fd, unsigned int *num_engines)
> {
> - struct drm_xe_engine_class_instance *engines;
> + struct drm_xe_query_engine_info *engines;
> struct drm_xe_device_query query = {
> .extensions = 0,
> .query = DRM_XE_DEVICE_QUERY_ENGINES,
> @@ -253,7 +253,7 @@ struct xe_device *xe_device_get(int fd)
> xe_dev->dev_id = xe_dev->config->info[DRM_XE_QUERY_CONFIG_REV_AND_DEVICE_ID] & 0xffff;
> xe_dev->gt_list = xe_query_gt_list_new(fd);
> xe_dev->memory_regions = __memory_regions(xe_dev->gt_list);
> - xe_dev->engines = xe_query_engines_new(fd, &xe_dev->number_engines);
> + xe_dev->engines = xe_query_engines(fd, &xe_dev->number_engines);
> xe_dev->mem_regions = xe_query_mem_regions_new(fd);
> xe_dev->vram_size = calloc(xe_dev->gt_list->num_gt, sizeof(*xe_dev->vram_size));
> xe_dev->visible_vram_size = calloc(xe_dev->gt_list->num_gt, sizeof(*xe_dev->visible_vram_size));
> @@ -427,16 +427,16 @@ uint64_t vram_if_possible(int fd, int gt)
> *
> * Returns engines array of xe device @fd.
> */
> -xe_dev_FN(xe_engines, engines, struct drm_xe_engine_class_instance *);
> +xe_dev_FN(xe_engines, engines, struct drm_xe_query_engine_info *);
>
> /**
> * xe_engine:
> * @fd: xe device fd
> * @idx: engine index
> *
> - * Returns engine instance of xe device @fd and @idx.
> + * Returns engine info of xe device @fd and @idx.
> */
> -struct drm_xe_engine_class_instance *xe_engine(int fd, int idx)
> +struct drm_xe_query_engine_info *xe_engine(int fd, int idx)
> {
> struct xe_device *xe_dev;
>
> @@ -658,7 +658,7 @@ bool xe_has_engine_class(int fd, uint16_t engine_class)
> igt_assert(xe_dev);
>
> for (int i = 0; i < xe_dev->number_engines; i++)
> - if (xe_dev->engines[i].engine_class == engine_class)
> + if (xe_dev->engines[i].instance.engine_class == engine_class)
> return true;
>
> return false;
> diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
> index bf9f2b955..fede00036 100644
> --- a/lib/xe/xe_query.h
> +++ b/lib/xe/xe_query.h
> @@ -33,7 +33,7 @@ struct xe_device {
> uint64_t memory_regions;
>
> /** @engines: array of hardware engines */
> - struct drm_xe_engine_class_instance *engines;
> + struct drm_xe_query_engine_info *engines;
>
> /** @number_engines: length of hardware engines array */
> unsigned int number_engines;
> @@ -62,7 +62,7 @@ struct xe_device {
>
> #define xe_for_each_engine(__fd, __hwe) \
> for (int __i = 0; __i < xe_number_engines(__fd) && \
> - (__hwe = xe_engine(__fd, __i)); ++__i)
> + (__hwe = &xe_engine(__fd, __i)->instance); ++__i)
> #define xe_for_each_engine_class(__class) \
> for (__class = 0; __class < DRM_XE_ENGINE_CLASS_COMPUTE + 1; \
> ++__class)
Can be done as a follow up but would be good to rename xe_for_each_hw_engine() to something like xe_for_each_engine_class_instance(), hw_engine is not
a name used in Xe anymore.
Other follow up is have a for_each macro for drm_xe_query_engine_info but that can be added when there is actual usage for it.
Up to you to decide to do the rename now or later, anyways:
Reviewed-by: José Roberto de Souza <jose.souza at intel.com>
> @@ -81,8 +81,8 @@ uint64_t all_memory_regions(int fd);
> uint64_t system_memory(int fd);
> uint64_t vram_memory(int fd, int gt);
> uint64_t vram_if_possible(int fd, int gt);
> -struct drm_xe_engine_class_instance *xe_engines(int fd);
> -struct drm_xe_engine_class_instance *xe_engine(int fd, int idx);
> +struct drm_xe_query_engine_info *xe_engines(int fd);
> +struct drm_xe_query_engine_info *xe_engine(int fd, int idx);
> struct drm_xe_query_mem_region *xe_mem_region(int fd, uint64_t region);
> const char *xe_region_name(uint64_t region);
> uint16_t xe_region_class(int fd, uint64_t region);
> diff --git a/tests/intel/xe_create.c b/tests/intel/xe_create.c
> index a3327d9f2..a1ef8a725 100644
> --- a/tests/intel/xe_create.c
> +++ b/tests/intel/xe_create.c
> @@ -149,7 +149,7 @@ static void create_execqueues(int fd, enum exec_queue_destroy ed)
> igt_nsec_elapsed(&tv);
>
> igt_fork(n, nproc) {
> - struct drm_xe_engine_class_instance *hwe;
> + struct drm_xe_query_engine_info *engine;
> uint32_t exec_queue, exec_queues[exec_queues_per_process];
> int idx, err, i;
>
> @@ -157,8 +157,9 @@ static void create_execqueues(int fd, enum exec_queue_destroy ed)
>
> for (i = 0; i < exec_queues_per_process; i++) {
> idx = rand() % num_engines;
> - hwe = xe_engine(fd, idx);
> - err = __xe_exec_queue_create(fd, vm, hwe, 0, &exec_queue);
> + engine = xe_engine(fd, idx);
> + err = __xe_exec_queue_create(fd, vm, &engine->instance,
> + 0, &exec_queue);
> igt_debug("[%2d] Create exec_queue: err=%d, exec_queue=%u [idx = %d]\n",
> n, err, exec_queue, i);
> if (err)
> diff --git a/tests/intel/xe_drm_fdinfo.c b/tests/intel/xe_drm_fdinfo.c
> index d50cc6df1..cec3e0825 100644
> --- a/tests/intel/xe_drm_fdinfo.c
> +++ b/tests/intel/xe_drm_fdinfo.c
> @@ -40,7 +40,7 @@ IGT_TEST_DESCRIPTION("Read and verify drm client memory consumption using fdinfo
> #define BO_SIZE (65536)
>
> /* Subtests */
> -static void test_active(int fd, struct drm_xe_engine_class_instance *eci)
> +static void test_active(int fd, struct drm_xe_query_engine_info *engine)
> {
> struct drm_xe_query_mem_region *memregion;
> uint64_t memreg = all_memory_regions(fd), region;
> @@ -89,7 +89,8 @@ static void test_active(int fd, struct drm_xe_engine_class_instance *eci)
> data = xe_bo_map(fd, bo, bo_size);
>
> for (i = 0; i < N_EXEC_QUEUES; i++) {
> - exec_queues[i] = xe_exec_queue_create(fd, vm, eci, 0);
> + exec_queues[i] = xe_exec_queue_create(fd, vm,
> + &engine->instance, 0);
> bind_exec_queues[i] = xe_bind_exec_queue_create(fd, vm, 0, true);
> syncobjs[i] = syncobj_create(fd, 0);
> }
> diff --git a/tests/intel/xe_exec_store.c b/tests/intel/xe_exec_store.c
> index 0b7b3d3e9..48e843af5 100644
> --- a/tests/intel/xe_exec_store.c
> +++ b/tests/intel/xe_exec_store.c
> @@ -63,7 +63,7 @@ static void store(int fd)
> .syncs = to_user_pointer(&sync),
> };
> struct data *data;
> - struct drm_xe_engine_class_instance *engine;
> + struct drm_xe_query_engine_info *engine;
> uint32_t vm;
> uint32_t exec_queue;
> uint32_t syncobj;
> @@ -82,14 +82,14 @@ static void store(int fd)
>
> engine = xe_engine(fd, 1);
> bo = xe_bo_create(fd, vm, bo_size,
> - vram_if_possible(fd, engine->gt_id),
> + vram_if_possible(fd, engine->instance.gt_id),
> DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM);
>
> - xe_vm_bind_async(fd, vm, engine->gt_id, bo, 0, addr, bo_size, &sync, 1);
> + xe_vm_bind_async(fd, vm, engine->instance.gt_id, bo, 0, addr, bo_size, &sync, 1);
> data = xe_bo_map(fd, bo, bo_size);
> store_dword_batch(data, addr, value);
>
> - exec_queue = xe_exec_queue_create(fd, vm, engine, 0);
> + exec_queue = xe_exec_queue_create(fd, vm, &engine->instance, 0);
> exec.exec_queue_id = exec_queue;
> exec.address = data->addr;
> sync.flags &= DRM_XE_SYNC_FLAG_SIGNAL;
> diff --git a/tests/intel/xe_intel_bb.c b/tests/intel/xe_intel_bb.c
> index c686604d2..804e176ee 100644
> --- a/tests/intel/xe_intel_bb.c
> +++ b/tests/intel/xe_intel_bb.c
> @@ -193,7 +193,8 @@ static void simple_bb(struct buf_ops *bops, bool new_context)
>
> if (new_context) {
> vm = xe_vm_create(xe, DRM_XE_VM_CREATE_FLAG_ASYNC_DEFAULT, 0);
> - ctx = xe_exec_queue_create(xe, vm, xe_engine(xe, 0), 0);
> + ctx = xe_exec_queue_create(xe, vm, &xe_engine(xe, 0)->instance,
> + 0);
> intel_bb_destroy(ibb);
> ibb = intel_bb_create_with_context(xe, ctx, vm, NULL, PAGE_SIZE);
> intel_bb_out(ibb, MI_BATCH_BUFFER_END);
> diff --git a/tests/intel/xe_noexec_ping_pong.c b/tests/intel/xe_noexec_ping_pong.c
> index e27cc4582..585af413d 100644
> --- a/tests/intel/xe_noexec_ping_pong.c
> +++ b/tests/intel/xe_noexec_ping_pong.c
> @@ -43,7 +43,7 @@
> * there is worked queued on one of the VM's compute exec_queues.
> */
>
> -static void test_ping_pong(int fd, struct drm_xe_engine_class_instance *eci)
> +static void test_ping_pong(int fd, struct drm_xe_query_engine_info *engine)
> {
> size_t vram_size = xe_vram_size(fd, 0);
> size_t align = xe_get_default_alignment(fd);
> @@ -75,7 +75,8 @@ static void test_ping_pong(int fd, struct drm_xe_engine_class_instance *eci)
> xe_vm_bind(fd, vm[i], bo[i][j], 0, 0x40000 + j*bo_size,
> bo_size, NULL, 0);
> }
> - exec_queues[i] = xe_exec_queue_create(fd, vm[i], eci, 0);
> + exec_queues[i] = xe_exec_queue_create(fd, vm[i],
> + &engine->instance, 0);
> }
>
> igt_info("Now sleeping for %ds.\n", SECONDS_TO_WAIT);
> diff --git a/tests/intel/xe_waitfence.c b/tests/intel/xe_waitfence.c
> index ad8adc2b0..bab2bed42 100644
> --- a/tests/intel/xe_waitfence.c
> +++ b/tests/intel/xe_waitfence.c
> @@ -81,7 +81,7 @@ enum waittype {
> static void
> waitfence(int fd, enum waittype wt)
> {
> - struct drm_xe_engine_class_instance *eci = NULL;
> + struct drm_xe_query_engine_info *engine = NULL;
> struct timespec ts;
> int64_t current, signalled;
> uint32_t bo_1;
> @@ -114,11 +114,11 @@ waitfence(int fd, enum waittype wt)
> igt_debug("wait type: RELTIME - timeout: %ld, timeout left: %ld\n",
> MS_TO_NS(10), timeout);
> } else if (wt == ENGINE) {
> - eci = xe_engine(fd, 1);
> + engine = xe_engine(fd, 1);
> clock_gettime(CLOCK_MONOTONIC, &ts);
> current = ts.tv_sec * 1e9 + ts.tv_nsec;
> timeout = current + MS_TO_NS(10);
> - signalled = wait_with_eci_abstime(fd, &wait_fence, 7, eci, timeout);
> + signalled = wait_with_eci_abstime(fd, &wait_fence, 7, &engine->instance, timeout);
> igt_debug("wait type: ENGINE ABSTIME - timeout: %" PRId64
> ", signalled: %" PRId64
> ", elapsed: %" PRId64 "\n",
More information about the igt-dev
mailing list