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

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-15 13:32:39)
> 
> 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?

Yes, I would like at least one version of for_each_engine() that
iterated over a preconfigured context. Then the default version would
setup the context to iterate over all engines, before calling the low
level version.

Then I also imagine that the pretty printer might pull out the engine
details from the context at the point of need, just to reduce the amount
of state we need to pass around the for loop (and into functions).

Pros and cons.

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

Yup, if GETPARAM engines[] fails, the index would be reduced to
I915_EXEC_RING and no fancy VCS.

However, I don't think that's a problem. There is nothing wrong with
testing both ABI (and I think I've said that we should always have
coverage of both ;), with having more tests for newer hardware on the new
ABI and just leaving the old ABI in the attic (i.e. move BAT/full over
to the new interface). For that I was thinking that being able to query
engines[] from the test routines would be a quick and easy way to
identify either the static legacy name or the new dynamic name.
-Chris