[igt-dev] [PATCH v20 0/6] new engine discovery interface

Andi Shyti andi at etezian.org
Fri Apr 12 00:32:04 UTC 2019 

From: Andi Shyti <andi.shyti at intel.com>

Hi,

In this patchset I propose an alternative way of engine discovery
thanks to the new interfaces developed by Tvrtko and Chris[4].

The changes to perf_pmu are a proposal, most probably they don't
work (it's anyway an RFC) because the dependency to legacy code
is still too strong.

Thanks Tvrtko, Chris, Antonio and Petri for your comments in the
previous RFCs.

Andi

v19 --> v20
===========
 - added some debugs from Tvrtko to get more info about gem_wait
   failure.
 - few fixes in gem_engine_topology.c from Tvrtko's comments,
   including a bigger fix about an uncontrolled variable
   increment in the _next function

v18 --> v19
===========
 - integrated Tvrtko's fixup patch [17]. From this patch some
   changes have been moved to gem_engine_topology as a new helper
   for getting the engine's properties.

v17 --> v18
===========
 - three patches have been applied (the ones that add
   gem_context_has_engine() function)
 - few cosmetic fixes
 - and some changes coming from Tvrtko's review on v17

v16 --> v17
===========
amongst many little things, three main changes:
 - improved perf_pmu adaptation to gem_engine_topology
 - removed the exec-ctx test, perf_pmu will be the flag test
 - when creating the engine list, now the
   for_each_engine_physical can be executed safely during subtest
   listing

v15 --> v16
===========
 - few changes to the gem_engine_topology stuff
 - added une more dummy test which loops through the physical
   engines, as well.
 - changes to test/perf_pmu required some more changes than
   expected (the 3 last patches)

v14 --> v15
===========
PATCH v14: [16]

 - virtual engines will be called "virtual" like unrecognised
   engines will be called "unknown"

 - renamed the for_each loops to more meaningful names
   (__for_each_static_engine and for_each_context_engine) and
   moved into gem_engine_topology.h

 - minor changes about data types.

v13 --> v14
===========
PATCH v13: [15]
minor changes this time:
 - squashed patch 2 and 3 (from v13) with a little rename and
   added Chris r-b

 - fixed some index issues and string assignement leaks

 - squashed patches 5, 6, 7 and 8 from v13

v12 --> v13
===========
PATCH v12: [14]
This patch is also very different from the previous other than
some reorganization of the code these are the main changes:

 - the previous version lacked the case when the context had its
   engines mapped. checks in the following order

 if the driver doesn't have the new API
  -> get the engines from the static list
 if the driver has the API but the context has nothing mapped
  -> get the engines from "query" and map them
 if the driver has the API and the context has engines mapped
  -> get the engines from the context

 - the helper functions have been removed as they were of no use.

v11 --> v12
===========
PATCH v11: [13]
This 12th version starts from a completely different thought.
Here's the main differences:

 - The list of engines is provided in an engine_data structure
   which contains an index (useful for looping through and for
   engine/context index mapping) instead of an array of engines.

 - The list of engines is generated every time the init function
   is called and nothing is allocated in heap memory.

 - The ioctl check is done already during the initialization part
   and if the new ioctls are not implemented, then the init
   function still stores only those present in the GPU.

 - The for_each loop is implemented by re-using the previous
   'for_each_engine_class_instance()' implemented by Tvrtko.

 - The gem_topology library offers few helper functions for
   checking the engine presence, checking the implementation of
   the ioctls and executing the buffer, in order to be completely
   unaware of the driver implementation.

Thanks Tvrtko for all your inputs.

v10 --> v11
===========
RFC v10: [12]
few cosmetic changes in v11 and minor architectural details.
Thanks Tvrtko.

- the 'query_engines()' functions are static as no one is using
  them yet.

- removed the 'gem_has_engine_topology()' function because it's
  very little used, 'get_active_engines()' can be used instead.

- a minor ring -> engine renaming coming from Chris. 

v9 --> v10
==========
RFC v9: [11]
also this time quite many changes, thanks Chris for the reviews,
here the most relevant of them.

- gem_query.[ch] have been renamed to gem_engine_topology.[ch]
  and all the functions ended up there as they are referring to
  the topology of the engines.

- the functions 'get_active_engines()',
  'gem_set_context_get_engines()' and
  'igt_require_gem_engine_list()' will be the main interface to
  the gem_engine_topology library, refer to patch 2 for details.

- the define 'for_each_engine2()' doesn't expose anymore the
  iterator.

- 'gem_context_has_engine()' has been moved from ioctl_wrappers.c
  to gem_context.c.

- the gem_exec_basic exec-ctx subtest does not abort if the new
  getparam/setparam and query apis are not implemented as it can
  work with both (as it was done at the beginning).

v8 --> v9
=========
RFC v8: [10]
quite many changes, please refer to the review in [10]. Thanks
Chris for the review. These are the most relevant:

- all the allocation in gem_query have been made in stack, not
  anymore allocated dynamically.

- removed get/set_context as it was already implemented and I
  didn't know.

- renamed some functions and variables to hopefully more
  meaningful names.

V7 --> v8
=========
RFC v7: [9]

- all functions have been moved from lib/igt_gt.{c,h} and
  lib/ioctl_wrappers.{c,h} to lib/i916/gem_query.{c,h}. (thanks
  Chris)

- 'for_each_engine_ctx' has been renamed to 'for_each_engine2' to
  be consistent with the '2' that indicates the new 'struct
  intel_execution_engine2' data structure.

V6 --> V7
=========
RFC v6: [8]

- a new patch has been added (patch 3) which adds a new
  requirement check through the igt_require_gem_engine_list()
  function. (thanks Chris) This function will initialize the
  engine list instead of the instead of igt_require_gem() as it
  was in v6

- all the ioctls have been wrapped (thanks Chris and Antonio) and
  new library functions have been added and assert the ioctls

- gem_init_engine_list() function returns the errno from the
  GETPARAM ioctl in order to be used as a requirement. (thanks
  Chris)

- fixed few requires/asserts

- The engine list "intel_active_engines2" is allocated of the
  number of engines instead of a political 64 (thanks Antonio).

- some parameter renaming in gem_has_ring_by_idx(). (thanks
  Chris).

- the original "intel_execution_engines2" has not been renamed,
  because it is used to create subtests before even executing any
  test/ioctl. By renaming it, some subtest generations failed.
  (thanks Petri)

V5 --> V6
=========
RFC v5: [7]
- Chris implemented the getparam ioctl which allows to the test
  to figure otu whether the new interface has been implemented.
  This way the for_each_engine_ctx() is able to work with new and
  old kernel uapi (thanks Chris)

V4 --> V5
=========
RFC v4: [6]

- the engine list is now built in 'igt_require_gem()' instead of
  '__open_driver()' so that we keep this discovery method
  specific to the i915 driver (thanks Chris).

- All the query/setparam structures dynamic allocation based on
  the number of engines, now are politically allocated 64 times,
  to avoid extra ioctl calls that retrieve the engine number
  (thanks Chris)

- use igt_ioctl instead of ioctl (thanks Chris)

- allocate intel_execution_engines2 dynamically instead of
  statically (thanks Tvrtko)

- simplify the test in 'gem_exec_basic()' so that simply checks
  the presence of the engine instead of executing a buffer
  (thank Chris)

- a new patch has been added (patch 3) that extends the
  'gem_has_ring()' boolean function. The new version sets the
  index as it's mapped in the kernel.The previous function is now
  a wrapper to the new function.

V3 --> V4
=========
PATCH v3: [3]

- re-architectured the discovery mechanism based on Tvrtko's
  sugestion and reviews.. In this version the discovery is done
  during the device opening and stored in a NULL terminated
  array, which replaces the existing intel_execution_engines2
  that is mainly used as a reference.

V2 --> V3
=========
RFC v2: [2]

- removed a standalone gem_query_engines_demo test and added the
  exec-ctx subtest inside gem_exec_basic (thanks Tvrtko).

- fixed most of Tvrtko's comments in [5], which consist in
  putting the mallocs igt_assert and ictls in igt_require and few
  refactoring (thanks Tvrtko).

V1 --> V2
=========
RFC v1: [1]

- added a demo test that simply queries the driver about the
  engines and executes a buffer (thanks Tvrtko)

- refactored the for_each_engine_ctx() macro so that what in the
  previous version was done by the "bind" function, now it's done
  in the first iteration. (Thanks Crhis)

- removed the "gem_has_ring_ctx()" because it was out of the
  scope.

- rename functions to more meaningful names

[1] RFC v1: https://lists.freedesktop.org/archives/igt-dev/2018-November/007025.html
[2] RFC v2: https://lists.freedesktop.org/archives/igt-dev/2018-November/007079.html
[3] PATCH v3: https://lists.freedesktop.org/archives/igt-dev/2018-November/007148.html
[4] https://cgit.freedesktop.org/~tursulin/drm-intel/log/?h=media
[5] https://lists.freedesktop.org/archives/igt-dev/2018-November/007100.html
[6] https://lists.freedesktop.org/archives/igt-dev/2019-January/008029.html
[7] https://lists.freedesktop.org/archives/igt-dev/2019-January/008165.html
[8] https://lists.freedesktop.org/archives/igt-dev/2019-February/008902.html
[9] https://lists.freedesktop.org/archives/igt-dev/2019-February/009185.html
[10] https://lists.freedesktop.org/archives/igt-dev/2019-February/009205.html
[11] https://lists.freedesktop.org/archives/igt-dev/2019-February/009277.html
[12] https://lists.freedesktop.org/archives/igt-dev/2019-March/010197.html
[13] https://lists.freedesktop.org/archives/igt-dev/2019-March/010467.html
[14] https://lists.freedesktop.org/archives/igt-dev/2019-March/010776.html
[15] https://lists.freedesktop.org/archives/igt-dev/2019-March/010827.html
[16] https://lists.freedesktop.org/archives/igt-dev/2019-March/010916.html
[17] https://lists.freedesktop.org/archives/igt-dev/2019-April/011821.html

Andi Shyti (6):
  include/drm-uapi: import i915_drm.h header file
  lib/i915: add gem_engine_topology library and for_each loop definition
  lib: igt_gt: add eb flags to class helper
  lib: igt_gt: make gem_engine_can_store_dword() check engine class
  lib: igt_dummyload: use for_each_context_engine()
  test: perf_pmu: use the gem_engine_topology library

 include/drm-uapi/i915_drm.h    | 361 +++++++++++++++++++++++++++------
 lib/Makefile.sources           |   2 +
 lib/i915/gem_engine_topology.c | 298 +++++++++++++++++++++++++++
 lib/i915/gem_engine_topology.h |  80 ++++++++
 lib/igt.h                      |   1 +
 lib/igt_dummyload.c            |  33 ++-
 lib/igt_gt.c                   |  29 ++-
 lib/igt_gt.h                   |  12 +-
 lib/meson.build                |   1 +
 tests/perf_pmu.c               | 146 +++++++------
 10 files changed, 830 insertions(+), 133 deletions(-)
 create mode 100644 lib/i915/gem_engine_topology.c
 create mode 100644 lib/i915/gem_engine_topology.h

-- 
2.20.1

More information about the igt-dev mailing list