new engine discovery interface

Kernel commits:

[1] ae8f4544dd8f ("drm/i915: Engine discovery query")
[2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")

from [*] repository, implement a new uapi for engine discovery that consist in first querying the driver about the engines in the gpu [1] and then binding a context to the set of engines that it can access [2].

In igt the classic way for discovering engines is done through the for_each_physical_engine() macro, that would be replaced by the new for_each_engine_ctx().

A new function is added gem_init_engine_list() that is called during device open which creates the list of engines. That list is stored in the intel_execution_engines2 that replaces the current array which has more a reference meaning. Now the intel_execution_engines2 stores the engines currently present in the GPU.

[*] git://people.freedesktop.org/~tursulin/drm-intel

Signed-off-by Andi Shyti <[email protected]>

---
 lib/drmtest.c | 12 +++++--
 lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
 lib/igt_gt.h  | 10 +++++-
 3 files changed, 110 insertions(+), 11 deletions(-)

lib/drmtest.c +1 -1

index 7c124ac666ec..2d155eea8a13 100644
@@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
 
        fd = __search_and_open(base, offset, chipset);
        if (fd != -1)
-               return fd;
+               goto set_engines_and_return;
 
        pthread_mutex_lock(&mutex);
        for (const struct module *m = modules; m->module; m++) {

@@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
        }

lib/drmtest.c +2 -0

+
+       return fd;
 }
 
 /**

lib/igt_gt.c +11 -8

index a20619246296..ab73d4b45087 100644
@@ -577,14 +577,7 @@ bool gem_can_store_dword(int fd, unsigned int engine)
        return true;
 }
 
-const struct intel_execution_engine2 intel_execution_engines2[] = {
-       { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
-       { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
-       { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
-       { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
-       { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
-       { }
-};
+struct intel_execution_engine2 intel_execution_engines2[32] = { };
 
 unsigned int
 gem_class_instance_to_eb_flags(int gem_fd,

@@ -650,3 +643,93 @@ bool gem_ring_has_physical_engine(int fd, unsigned ring)
 
        return gem_has_ring(fd, ring);
 }
+
+static struct drm_i915_query_engine_info *query_engines(int fd)
+{
+       struct drm_i915_query query = { };
+       struct drm_i915_query_item item = { };
+       struct drm_i915_query_engine_info *query_engines;
+
+       item.query_id = DRM_I915_QUERY_ENGINE_INFO;
+       query.items_ptr = to_user_pointer(&item);
+       query.num_items = 1;

lib/igt_gt.c +8 -0

+       igt_assert((query_engines = calloc(item.length, 1)));
+       item.data_ptr = to_user_pointer(query_engines);
+
+       /* The second ioctl stores the engines in query_engines */
+       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
+
+       return query_engines;
+}

lib/igt_gt.c +6 -0

+       igt_assert((ctx_engine = malloc(size)));
+
+       ctx_engine->extensions = 0;
+       for (i = 0; i <= n; i++) {
+               ctx_engine->class_instance[i].class =
+                                       intel_execution_engines2[i].class;

Quoting Andi Shyti (2019-01-15 12:35:10)

Quoted 45 lines

Kernel commits:

[1] ae8f4544dd8f ("drm/i915: Engine discovery query")
[2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")

from [*] repository, implement a new uapi for engine discovery that consist in first querying the driver about the engines in the gpu [1] and then binding a context to the set of engines that it can access [2].

In igt the classic way for discovering engines is done through the for_each_physical_engine() macro, that would be replaced by the new for_each_engine_ctx().

A new function is added gem_init_engine_list() that is called during device open which creates the list of engines. That list is stored in the intel_execution_engines2 that replaces the current array which has more a reference meaning. Now the intel_execution_engines2 stores the engines currently present in the GPU.

[*] git://people.freedesktop.org/~tursulin/drm-intel

Signed-off-by Andi Shyti <[email protected]>

---
  lib/drmtest.c | 12 +++++--
  lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
  lib/igt_gt.h  | 10 +++++-
  3 files changed, 110 insertions(+), 11 deletions(-)

lib/drmtest.c +1 -1

index 7c124ac666ec..2d155eea8a13 100644
@@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
 
         fd = __search_and_open(base, offset, chipset);
         if (fd != -1)
-               return fd;
+               goto set_engines_and_return;
 
         pthread_mutex_lock(&mutex);
         for (const struct module *m = modules; m->module; m++) {

@@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
         }

lib/drmtest.c static int __open_driver(const char *base, int offset, unsigned int chipset) · line 323

Quoted 10 lines

lib/drmtest.c +7 -1

         pthread_mutex_unlock(&mutex);
 
-       return __search_and_open(base, offset, chipset);
+       fd = __search_and_open(base, offset, chipset);
+       if (fd < 0)
+               return fd;
+
+set_engines_and_return:
+       if (is_i915_device(fd))
+               gem_init_engine_list(fd);

Do we really want more implicit actions on opening an fd?

We already have igt_require_gem() which would make an interesting starting point, for that we may want to use fd not from drm_open_driver(). However, there seems to be no issue with creating the names on the fly (and ask for the complementary getter for engines[] so that an index could be translated back to class:instance).

Certainly having drmtest presume GEM (and be subject to all of the extra rules) given i915 seems a bit rude.

Quoted 14 lines

lib/drmtest.c +2 -0

+
+       return fd;
  }
 
  /**

lib/igt_gt.c +0 -0

index a20619246296..ab73d4b45087 100644
@@ -577,14 +577,7 @@ bool gem_can_store_dword(int fd, unsigned int engine)
         return true;
  }
 

lib/igt_gt.c bool gem_can_store_dword(int fd, unsigned int engine) · line 580

Quoted 9 lines

lib/igt_gt.c +1 -8

-const struct intel_execution_engine2 intel_execution_engines2[] = {
-       { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
-       { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
-       { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
-       { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
-       { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
-       { }
-};
+struct intel_execution_engine2 intel_execution_engines2[32] = { };

Quoted 17 lines

lib/igt_gt.c +10 -0

  unsigned int
  gem_class_instance_to_eb_flags(int gem_fd,

@@ -650,3 +643,93 @@ bool gem_ring_has_physical_engine(int fd, unsigned ring)
 
         return gem_has_ring(fd, ring);
  }
+
+static struct drm_i915_query_engine_info *query_engines(int fd)
+{
+       struct drm_i915_query query = { };
+       struct drm_i915_query_item item = { };
+       struct drm_i915_query_engine_info *query_engines;
+
+       item.query_id = DRM_I915_QUERY_ENGINE_INFO;
+       query.items_ptr = to_user_pointer(&item);
+       query.num_items = 1;

lib/igt_gt.c bool gem_ring_has_physical_engine(int fd, unsigned ring) · line 665

Quoted 10 lines

lib/igt_gt.c +10 -0

+
+       /*
+        * The first ioctl is sent with item.length = 0
+        * which asks to the driver to store in length the
+        * memory needed for the engines. In the driver, length
+        * is equal to
+        *
+        *   len = sizeof(struct drm_i915_query_engine_info) +
+        *                   INTEL_INFO(i915)->num_rings *
+        *                   sizeof(struct drm_i915_engine_info);

Nah, do not imply you are tied to implementation details - that is the whole point of querying the length first. Do note that you can over allocate (say use a small bit of stack) and do the query in one shot, only allocating from heap if we need more room.

lib/igt_gt.c bool gem_ring_has_physical_engine(int fd, unsigned ring) · line 667

Quoted 2 lines

lib/igt_gt.c +2 -0

+        */
+       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));

Bad news for old kernels.

Quoted 9 lines

lib/igt_gt.c +8 -0

+       igt_assert((query_engines = calloc(item.length, 1)));
+       item.data_ptr = to_user_pointer(query_engines);
+
+       /* The second ioctl stores the engines in query_engines */
+       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
+
+       return query_engines;
+}

lib/igt_gt.c bool gem_ring_has_physical_engine(int fd, unsigned ring) · line 686

Quoted 10 lines

lib/igt_gt.c +10 -0

+
+void __set_ctx_engine_map(int fd, uint32_t ctx_id)
+{
+       int i;
+       unsigned char n;
+       struct drm_i915_gem_context_param ctx_param;
+       struct i915_context_param_engines *ctx_engine;
+       size_t size;
+
+       for (n = 0; intel_execution_engines2[n].name; n++);

Close your eyes and tell me where the ';' is.

lib/igt_gt.c bool gem_ring_has_physical_engine(int fd, unsigned ring) · line 689

Quoted 2 lines

lib/igt_gt.c +2 -0

+       size = sizeof(struct i915_context_param_engines) +
+                     (n + 1) * sizeof(*ctx_engine->class_instance);

Should be small enough for a stack allocations of say 64 engines (the limit of the current execbuf uabi).

Quoted 6 lines

lib/igt_gt.c +6 -0

+       igt_assert((ctx_engine = malloc(size)));
+
+       ctx_engine->extensions = 0;
+       for (i = 0; i <= n; i++) {
+               ctx_engine->class_instance[i].class =
+                                       intel_execution_engines2[i].class;

lib/igt_gt.c bool gem_ring_has_physical_engine(int fd, unsigned ring) · line 706

Quoted 10 lines

lib/igt_gt.c +10 -0

+               ctx_engine->class_instance[i].instance =
+                                       intel_execution_engines2[i].instance;
+       }
+
+       ctx_param.ctx_id = ctx_id;
+       ctx_param.size = size;
+       ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
+       ctx_param.value = to_user_pointer(ctx_engine);
+
+       igt_assert(!ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));

Do you really want to risk that this won't be interrupted by a signal at any point in the future?

lib/igt_gt.c bool gem_ring_has_physical_engine(int fd, unsigned ring) · line 709

Quoted 3 lines

lib/igt_gt.c +3 -0

+
+       free(ctx_engine);
+}

Quoted 65 lines

Quoting Andi Shyti (2019-01-15 12:35:10)

Quoted 45 lines

Kernel commits:

[1] ae8f4544dd8f ("drm/i915: Engine discovery query")
[2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")

from [*] repository, implement a new uapi for engine discovery that consist in first querying the driver about the engines in the gpu [1] and then binding a context to the set of engines that it can access [2].

In igt the classic way for discovering engines is done through the for_each_physical_engine() macro, that would be replaced by the new for_each_engine_ctx().

A new function is added gem_init_engine_list() that is called during device open which creates the list of engines. That list is stored in the intel_execution_engines2 that replaces the current array which has more a reference meaning. Now the intel_execution_engines2 stores the engines currently present in the GPU.

[*] git://people.freedesktop.org/~tursulin/drm-intel

Signed-off-by Andi Shyti <[email protected]>

---
  lib/drmtest.c | 12 +++++--
  lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
  lib/igt_gt.h  | 10 +++++-
  3 files changed, 110 insertions(+), 11 deletions(-)

lib/drmtest.c +1 -1

index 7c124ac666ec..2d155eea8a13 100644
@@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
 
         fd = __search_and_open(base, offset, chipset);
         if (fd != -1)
-               return fd;
+               goto set_engines_and_return;
 
         pthread_mutex_lock(&mutex);
         for (const struct module *m = modules; m->module; m++) {

@@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
         }

lib/drmtest.c static int __open_driver(const char *base, int offset, unsigned int chipset) · line 323

Quoted 10 lines

lib/drmtest.c +7 -1

         pthread_mutex_unlock(&mutex);
 
-       return __search_and_open(base, offset, chipset);
+       fd = __search_and_open(base, offset, chipset);
+       if (fd < 0)
+               return fd;
+
+set_engines_and_return:
+       if (is_i915_device(fd))
+               gem_init_engine_list(fd);

Do we really want more implicit actions on opening an fd?

We already have igt_require_gem() which would make an interesting starting point, for that we may want to use fd not from drm_open_driver(). However, there seems to be no issue with creating the names on the fly (and ask for the complementary getter for engines[] so that an index could be translated back to class:instance).

Quoted 2 lines

Certainly having drmtest presume GEM (and be subject to all of the extra rules) given i915 seems a bit rude.

My suggestion was to co-site with existing "is i915" code in drm_open_driver and drm_open_driver_render. But igt_require_gem also sounds okay.

I didn't understand the bit about the complementary getter.

Quoted 15 lines

lib/drmtest.c static int __open_driver(const char *base, int offset, unsigned int chipset)

Quoted 9 lines

lib/drmtest.c +9 -0

+       /*
+        * The first ioctl is sent with item.length = 0
+        * which asks to the driver to store in length the
+        * memory needed for the engines. In the driver, length
+        * is equal to
+        *
+        *   len = sizeof(struct drm_i915_query_engine_info) +
+        *                   INTEL_INFO(i915)->num_rings *
+        *                   sizeof(struct drm_i915_engine_info);

Nah, do not imply you are tied to implementation details - that is the whole point of querying the length first. Do note that you can over allocate (say use a small bit of stack) and do the query in one shot, only allocating from heap if we need more room.

Quoted 4 lines

lib/drmtest.c

Quoted 2 lines

lib/drmtest.c +2 -0

+        */
+       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));

Bad news for old kernels.

Do we care about old kernels? I thought for IGT we did not.

Quoting Tvrtko Ursulin (2019-01-15 13:14:10)

Quoted 74 lines · Chris Wilson

Quoted 59 lines

Quoting Andi Shyti (2019-01-15 12:35:10)

Quoted 45 lines

Kernel commits:

[1] ae8f4544dd8f ("drm/i915: Engine discovery query")
[2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")

from [*] repository, implement a new uapi for engine discovery that consist in first querying the driver about the engines in the gpu [1] and then binding a context to the set of engines that it can access [2].

In igt the classic way for discovering engines is done through the for_each_physical_engine() macro, that would be replaced by the new for_each_engine_ctx().

A new function is added gem_init_engine_list() that is called during device open which creates the list of engines. That list is stored in the intel_execution_engines2 that replaces the current array which has more a reference meaning. Now the intel_execution_engines2 stores the engines currently present in the GPU.

[*] git://people.freedesktop.org/~tursulin/drm-intel

Signed-off-by Andi Shyti <[email protected]>

---
   lib/drmtest.c | 12 +++++--
   lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
   lib/igt_gt.h  | 10 +++++-
   3 files changed, 110 insertions(+), 11 deletions(-)

lib/drmtest.c +1 -1

index 7c124ac666ec..2d155eea8a13 100644
@@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
 
          fd = __search_and_open(base, offset, chipset);
          if (fd != -1)
-               return fd;
+               goto set_engines_and_return;
 
          pthread_mutex_lock(&mutex);
          for (const struct module *m = modules; m->module; m++) {

@@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
          }

lib/drmtest.c static int __open_driver(const char *base, int offset, unsigned int chipset) · line 323

Quoted 10 lines

lib/drmtest.c +7 -1

          pthread_mutex_unlock(&mutex);
 
-       return __search_and_open(base, offset, chipset);
+       fd = __search_and_open(base, offset, chipset);
+       if (fd < 0)
+               return fd;
+
+set_engines_and_return:
+       if (is_i915_device(fd))
+               gem_init_engine_list(fd);

Do we really want more implicit actions on opening an fd?

Quoted 8 lines

We already have igt_require_gem() which would make an interesting starting point, for that we may want to use fd not from drm_open_driver(). However, there seems to be no issue with creating the names on the fly (and ask for the complementary getter for engines[] so that an index could be translated back to class:instance).

Certainly having drmtest presume GEM (and be subject to all of the extra rules) given i915 seems a bit rude.

My suggestion was to co-site with existing "is i915" code in drm_open_driver and drm_open_driver_render. But igt_require_gem also sounds okay.

Quoted 1 line

I didn't understand the bit about the complementary getter.

Just we're missing the GETPARAM engines interface, which I think could be used to retrieve the information required for pretty printing at runtime, and so avoid global tables that may not match my context.

Quoted 20 lines

Quoted 19 lines

lib/drmtest.c static int __open_driver(const char *base, int offset, unsigned int chipset)

Quoted 9 lines

lib/drmtest.c +9 -0

+       /*
+        * The first ioctl is sent with item.length = 0
+        * which asks to the driver to store in length the
+        * memory needed for the engines. In the driver, length
+        * is equal to
+        *
+        *   len = sizeof(struct drm_i915_query_engine_info) +
+        *                   INTEL_INFO(i915)->num_rings *
+        *                   sizeof(struct drm_i915_engine_info);

Nah, do not imply you are tied to implementation details - that is the whole point of querying the length first. Do note that you can over allocate (say use a small bit of stack) and do the query in one shot, only allocating from heap if we need more room.

lib/drmtest.c

Quoted 2 lines

lib/drmtest.c +2 -0

+        */
+       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));

Bad news for old kernels.

Quoted 1 line

Do we care about old kernels? I thought for IGT we did not.

We supposedly do! :)

We definitely do care about being able to bisect kernels from an arm's length ago. Beyond an arm's length, no one will notice. Certainly we don't intend to make things harder than we need to. :)

Quoted 75 lines

Quoting Tvrtko Ursulin (2019-01-15 13:14:10)

Quoted 74 lines · Chris Wilson

Quoted 59 lines

Quoting Andi Shyti (2019-01-15 12:35:10)

Quoted 45 lines

Kernel commits:

[1] ae8f4544dd8f ("drm/i915: Engine discovery query")
[2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")

from [*] repository, implement a new uapi for engine discovery that consist in first querying the driver about the engines in the gpu [1] and then binding a context to the set of engines that it can access [2].

In igt the classic way for discovering engines is done through the for_each_physical_engine() macro, that would be replaced by the new for_each_engine_ctx().

A new function is added gem_init_engine_list() that is called during device open which creates the list of engines. That list is stored in the intel_execution_engines2 that replaces the current array which has more a reference meaning. Now the intel_execution_engines2 stores the engines currently present in the GPU.

[*] git://people.freedesktop.org/~tursulin/drm-intel

Signed-off-by Andi Shyti <[email protected]>

---
   lib/drmtest.c | 12 +++++--
   lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
   lib/igt_gt.h  | 10 +++++-
   3 files changed, 110 insertions(+), 11 deletions(-)

lib/drmtest.c +1 -1

index 7c124ac666ec..2d155eea8a13 100644
@@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
 
          fd = __search_and_open(base, offset, chipset);
          if (fd != -1)
-               return fd;
+               goto set_engines_and_return;
 
          pthread_mutex_lock(&mutex);
          for (const struct module *m = modules; m->module; m++) {

@@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
          }

lib/drmtest.c static int __open_driver(const char *base, int offset, unsigned int chipset) · line 323

Quoted 10 lines

lib/drmtest.c +7 -1

          pthread_mutex_unlock(&mutex);
 
-       return __search_and_open(base, offset, chipset);
+       fd = __search_and_open(base, offset, chipset);
+       if (fd < 0)
+               return fd;
+
+set_engines_and_return:
+       if (is_i915_device(fd))
+               gem_init_engine_list(fd);

Do we really want more implicit actions on opening an fd?

Quoted 8 lines

We already have igt_require_gem() which would make an interesting starting point, for that we may want to use fd not from drm_open_driver(). However, there seems to be no issue with creating the names on the fly (and ask for the complementary getter for engines[] so that an index could be translated back to class:instance).

Certainly having drmtest presume GEM (and be subject to all of the extra rules) given i915 seems a bit rude.

My suggestion was to co-site with existing "is i915" code in drm_open_driver and drm_open_driver_render. But igt_require_gem also sounds okay.

Quoted 5 lines

Quoted 1 line

I didn't understand the bit about the complementary getter.

Just we're missing the GETPARAM engines interface, which I think could be used to retrieve the information required for pretty printing at runtime, and so avoid global tables that may not match my context.

Are you imagining for_each_engine_ctx to query if the context already has engine map set up and in that case iterate over those engines? And if there is no engine map to set up one containing all engines?

Or actually, maybe the former as the only behaviour of for_each_engine_ctx, and only for_each_physical_engine auto-configuring the map?

Quoted 20 lines

Quoted 20 lines

Quoted 19 lines

lib/drmtest.c static int __open_driver(const char *base, int offset, unsigned int chipset)

Quoted 9 lines

lib/drmtest.c +9 -0

+       /*
+        * The first ioctl is sent with item.length = 0
+        * which asks to the driver to store in length the
+        * memory needed for the engines. In the driver, length
+        * is equal to
+        *
+        *   len = sizeof(struct drm_i915_query_engine_info) +
+        *                   INTEL_INFO(i915)->num_rings *
+        *                   sizeof(struct drm_i915_engine_info);

Nah, do not imply you are tied to implementation details - that is the whole point of querying the length first. Do note that you can over allocate (say use a small bit of stack) and do the query in one shot, only allocating from heap if we need more room.

lib/drmtest.c

Quoted 2 lines

lib/drmtest.c +2 -0

+        */
+       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));

Bad news for old kernels.

Quoted 7 lines

Quoted 1 line

Do we care about old kernels? I thought for IGT we did not.

We supposedly do! :)

We definitely do care about being able to bisect kernels from an arm's length ago. Beyond an arm's length, no one will notice. Certainly we don't intend to make things harder than we need to. :)

Question is how does the complete new IGT world look. If everything depends on engine map interface (everything as all for_each_physical_engine tests), then we can have it fall back to a static table as today but lose some coverage. (Since we don't have a way of probing or accessing all engines without the new uapi, we can only have a subset of engines in the static fall back table.)

Quoted 4 lines

Quoted 4 lines

lib/igt_gt.c +0 -4

-const struct intel_execution_engine2 intel_execution_engines2[] = {
-       { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
-       { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
-       { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },

Quoted 5 lines

lib/igt_gt.c bool gem_can_store_dword(int fd, unsigned int engine) · line 580

Quoted 5 lines

lib/igt_gt.c +1 -4

-       { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
-       { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
-       { }
-};
+struct intel_execution_engine2 intel_execution_engines2[32] = { };

I'd just make it a pointer, both size and explicit static initialization to zero are not needed.

Hi Tvrtko,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

lib/igt_gt.c +0 -4

-const struct intel_execution_engine2 intel_execution_engines2[] = {
-       { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
-       { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
-       { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },

Quoted 8 lines

Quoted 5 lines

lib/igt_gt.c bool gem_can_store_dword(int fd, unsigned int engine) · line 580

Quoted 5 lines

lib/igt_gt.c +1 -4

-       { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
-       { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
-       { }
-};
+struct intel_execution_engine2 intel_execution_engines2[32] = { };

I'd just make it a pointer, both size and explicit static initialization to zero are not needed.

So that you want to allocate *intel_execution_engines2 somewhere in gem_init_engine_list(), is this what you are recommending?

Quoted 13 lines

Quoted 12 lines

Quoted 9 lines

lib/igt_gt.c bool gem_can_store_dword(int fd, unsigned int engine) · line 580

Quoted 9 lines

lib/igt_gt.c +1 -8

-const struct intel_execution_engine2 intel_execution_engines2[] = {
-       { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
-       { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
-       { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
-       { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
-       { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
-       { }
-};
+struct intel_execution_engine2 intel_execution_engines2[32] = { };

I'd just make it a pointer, both size and explicit static initialization to zero are not needed.

Quoted 2 lines

So that you want to allocate *intel_execution_engines2 somewhere in gem_init_engine_list(), is this what you are recommending?

Yep. Do you see an advantage of putting it in the data segment? I don't think failure to allocate this on heap at startup is a concern.

Quoted 11 lines

lib/drmtest.c +1 -1

@@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
 
        fd = __search_and_open(base, offset, chipset);
        if (fd != -1)
-               return fd;
+               goto set_engines_and_return;
 
        pthread_mutex_lock(&mutex);
        for (const struct module *m = modules; m->module; m++) {

@@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
        }

lib/drmtest.c static int __open_driver(const char *base, int offset, unsigned int chipset) · line 323

Quoted 10 lines

lib/drmtest.c +7 -1

        pthread_mutex_unlock(&mutex);
 
-       return __search_and_open(base, offset, chipset);
+       fd = __search_and_open(base, offset, chipset);
+       if (fd < 0)
+               return fd;
+
+set_engines_and_return:
+       if (is_i915_device(fd))
+               gem_init_engine_list(fd);

Do we really want more implicit actions on opening an fd?

We already have igt_require_gem() which would make an interesting starting point, for that we may want to use fd not from drm_open_driver(). However, there seems to be no issue with creating the names on the fly (and ask for the complementary getter for engines[] so that an index could be translated back to class:instance).

Certainly having drmtest presume GEM (and be subject to all of the extra rules) given i915 seems a bit rude.

lib/igt_gt.c bool gem_ring_has_physical_engine(int fd, unsigned ring) · line 665

Quoted 9 lines

lib/igt_gt.c +9 -0

+       /*
+        * The first ioctl is sent with item.length = 0
+        * which asks to the driver to store in length the
+        * memory needed for the engines. In the driver, length
+        * is equal to
+        *
+        *   len = sizeof(struct drm_i915_query_engine_info) +
+        *                   INTEL_INFO(i915)->num_rings *
+        *                   sizeof(struct drm_i915_engine_info);

Nah, do not imply you are tied to implementation details - that is the whole point of querying the length first. Do note that you can over allocate (say use a small bit of stack) and do the query in one shot, only allocating from heap if we need more room.

Review

Quoted 2 lines

(quoted diff) +2 -0

+        */
+       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));

Bad news for old kernels.

lib/igt_gt.c bool gem_ring_has_physical_engine(int fd, unsigned ring) · line 686

Quoted 9 lines

lib/igt_gt.c +9 -0

+void __set_ctx_engine_map(int fd, uint32_t ctx_id)
+{
+       int i;
+       unsigned char n;
+       struct drm_i915_gem_context_param ctx_param;
+       struct i915_context_param_engines *ctx_engine;
+       size_t size;
+
+       for (n = 0; intel_execution_engines2[n].name; n++);

Close your eyes and tell me where the ';' is.

lib/igt_gt.c bool gem_ring_has_physical_engine(int fd, unsigned ring) · line 689

Quoted 2 lines

lib/igt_gt.c +2 -0

+       size = sizeof(struct i915_context_param_engines) +
+                     (n + 1) * sizeof(*ctx_engine->class_instance);

Should be small enough for a stack allocations of say 64 engines (the limit of the current execbuf uabi).

Quoted 11 lines

lib/igt_gt.c +11 -0

+       igt_assert((ctx_engine = malloc(size)));
+
+       ctx_engine->extensions = 0;
+       for (i = 0; i <= n; i++) {
+               ctx_engine->class_instance[i].class =
+                                       intel_execution_engines2[i].class;
+               ctx_engine->class_instance[i].instance =
+                                       intel_execution_engines2[i].instance;
+       }
+
+       ctx_param.ctx_id = ctx_id;

lib/igt_gt.c bool gem_ring_has_physical_engine(int fd, unsigned ring) · line 706

Quoted 5 lines

lib/igt_gt.c +5 -0

+       ctx_param.size = size;
+       ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
+       ctx_param.value = to_user_pointer(ctx_engine);
+
+       igt_assert(!ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));

Do you really want to risk that this won't be interrupted by a signal at any point in the future?

The "exec-ctx" is a demo subtest inserted in the gem_exec_basic test. The main scope is to demonstrate an alternative way for querying engines as implemented in the new API in commit 87079e04 ("lib: implement new engine discovery interface").

The "exec-ctx" subtest simply gets the list of engines, binds them to a context and executes a buffer. This is done through a new "for_each_engine_ctx" loop which iterates through the engines.

Signed-off-by Andi Shyti <[email protected]>

---
 tests/i915/gem_exec_basic.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

tests/i915/gem_exec_basic.c +0 -0

index dcb83864b1c1..2b09f8cd79b7 100644
@@ -114,6 +114,22 @@ static void gtt(int fd, unsigned ring)
        munmap(execbuf, 4096);
 }
 

tests/i915/gem_exec_basic.c static void gtt(int fd, unsigned ring) · line 126

Quoted 4 lines

tests/i915/gem_exec_basic.c +4 -0

+       struct drm_i915_gem_execbuffer2 execbuf = {
+               .buffers_ptr = to_user_pointer(&exec),
+               .buffer_count = 1,
+               .flags = engine->map_index,

No. Do not store engine->map_index in the global array, as that is per-context and I would like to iterate over different arrays of engines.

tests/i915/gem_exec_basic.c static void gtt(int fd, unsigned ring) · line 130

Quoted 4 lines

tests/i915/gem_exec_basic.c +4 -0

+               .rsvd1 = ctx,
+       };
+
+       gem_execbuf(fd, &execbuf);

This doesn't actually prove anything other than no -EINVAL. How many different ways might we have accidentally caused success. Could we feed in a known failure execbuf and then look for specific failures? (Hint, what happens if handle = 0.)