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

[Tvrtko Ursulin]

On 15/01/2019 13:19, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-15 13:14:10)
>>
>> 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.
> 
> 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?

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

Regards,

Tvrtko