[igt-dev] [PATCH v23 00/14] new engine discovery interface
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon May 13 09:04:20 UTC 2019
On 13/05/2019 01:44, Andi Shyti wrote:
> 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].
>
> 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
>
More information about the igt-dev
mailing list