Re: [igt-dev] [RFC PATCH v4 2/3] lib: implement new engine discovery interface

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 15/01/2019 13:00, Chris Wilson wrote:
> Quoting Andi Shyti (2019-01-15 12:35:10)
>> 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 <andi.shyti@intel.com>
>> ---
>>   lib/drmtest.c | 12 +++++--
>>   lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
>>   lib/igt_gt.h  | 10 +++++-
>>   3 files changed, 110 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/drmtest.c b/lib/drmtest.c
>> index 7c124ac666ec..2d155eea8a13 100644
>> --- a/lib/drmtest.c
>> +++ b/lib/drmtest.c
>> @@ -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)
>>          }
>>          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.

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.

> 
>> +
>> +       return fd;
>>   }
>>   
>>   /**
>> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
>> index a20619246296..ab73d4b45087 100644
>> --- a/lib/igt_gt.c
>> +++ b/lib/igt_gt.c
>> @@ -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] = { };

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

>>   
>>   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;
>> +
>> +       /*
>> +        * 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.
> 
>> +        */
>> +       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.

Regards,

Tvrtko

> 
>> +       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;
>> +}
>> +
>> +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.
> 
>> +       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).
> 
>> +       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;
>> +       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?
> 
>> +
>> +       free(ctx_engine);
>> +}
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev