[igt-dev] [RFC PATCH v4 2/3] lib: implement new engine discovery interface
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Jan 15 13:14:10 UTC 2019
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 at 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);
>> +}
>
More information about the igt-dev
mailing list