Re: [igt-dev] [PATCH v23 00/14] new engine discovery interface

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

On 13/05/2019 01:44, Andi Shyti wrote:
> From: Andi Shyti <andi.shyti@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].
> 
> Thanks Tvrtko, Chris, Antonio and Petri for your comments in the
> previous RFCs.
> 
> Andi
> 
> v22 --> v23
> ===========
> updated the following tests to the new APIs:
> 
> gem_busy
> gem_cs_tlb
> gem_ctx_exec
> gem_exec_basic
> gem_exec_parallel
> gem_exec_store
> gem_wait
> i915_hangman

So there are no changes in "old" patches, just new patches in series 
(patches 7 to 14)?

Regards,

Tvrtko


> v21 --> v22
> ===========
>   - just removed context creation and deletion from
>     engine_topology and fixed perf_pmu accordingly.
> 
> v20 --> v21
> ===========
>   - removed Tvrtko's debug messages
>   - few fixes from Chris last review
> 
> 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 (14):
>    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 execution buffer 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
>    test/i915: gem_busy: use the gem_engine_topology library
>    test/i915: gem_cs_tlb: use the gem_engine_topology library
>    test/i915: gem_ctx_exec: use the gem_engine_topology library
>    test/i915: gem_exec_basic: use the gem_engine_topology library
>    test/i915: gem_exec_parallel: use the gem_engine_topology library
>    test/i915: gem_exec_store: use the gem_engine_topology library
>    test/i915: gem_wait: use the gem_engine_topology library
>    test/i915: i915_hangman: use the gem_engine_topology library
> 
>   include/drm-uapi/i915_drm.h    | 209 +++++++++++++++++++++++-
>   lib/Makefile.sources           |   2 +
>   lib/i915/gem_engine_topology.c | 282 +++++++++++++++++++++++++++++++++
>   lib/i915/gem_engine_topology.h |  79 +++++++++
>   lib/igt.h                      |   1 +
>   lib/igt_dummyload.c            |  29 ++--
>   lib/igt_gt.c                   |  30 +++-
>   lib/igt_gt.h                   |  12 +-
>   lib/meson.build                |   1 +
>   tests/i915/gem_busy.c          | 133 ++++++----------
>   tests/i915/gem_cs_tlb.c        |   8 +-
>   tests/i915/gem_ctx_exec.c      |  15 +-
>   tests/i915/gem_exec_basic.c    |  28 ++--
>   tests/i915/gem_exec_parallel.c |   7 +-
>   tests/i915/gem_exec_store.c    |  25 ++-
>   tests/i915/gem_wait.c          |  24 +--
>   tests/i915/i915_hangman.c      |  15 +-
>   tests/perf_pmu.c               | 151 +++++++++++-------
>   18 files changed, 822 insertions(+), 229 deletions(-)
>   create mode 100644 lib/i915/gem_engine_topology.c
>   create mode 100644 lib/i915/gem_engine_topology.h
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev