On 23/01/2020 13:16, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2020-01-23 13:08:26)
On 23/01/2020 12:54, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2020-01-23 12:43:06)
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
In test cases which create new contexts and submit work against them using
the passed in engine index we are sometimes unsure whether this engine
index was potentially created based on a default context with engine map
configured (such as when under the __for_each_physical_engine iterator.
To simplify test code we add gem_context/queue_clone_with_engines which
is to be used in such scenario instead of the current pattern of
gem_context_create followed by gem_context_set_all_engines (which is also
removed by the patch).
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Suggested-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
lib/i915/gem_context.c | 59 ++++++++++++++++++++++++++++++++++
lib/i915/gem_context.h | 4 +++
lib/i915/gem_engine_topology.c | 11 -------
lib/i915/gem_engine_topology.h | 2 --
tests/i915/gem_ctx_clone.c | 15 +--------
tests/i915/gem_ctx_switch.c | 19 ++++-------
tests/i915/gem_exec_parallel.c | 3 +-
tests/i915/gem_spin_batch.c | 11 +++----
tests/perf_pmu.c | 3 +-
9 files changed, 76 insertions(+), 51 deletions(-)
diff --git a/lib/i915/gem_context.c b/lib/i915/gem_context.c
index 1fae5191f83f..f92d5ff3dfc5 100644
--- a/lib/i915/gem_context.c
+++ b/lib/i915/gem_context.c
@@ -372,6 +372,50 @@ uint32_t gem_context_clone(int i915,
return ctx;
}
+bool gem_has_context_clone(int i915)
+{
+ struct drm_i915_gem_context_create_ext_clone ext = {
+ { .name = I915_CONTEXT_CREATE_EXT_CLONE },
+ .clone_id = -1,
+ };
+ struct drm_i915_gem_context_create_ext create = {
+ .flags = I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS,
+ .extensions = to_user_pointer(&ext),
+ };
+ int err;
+
+ err = 0;
+ if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT, &create)) {
+ err = -errno;
+ igt_assume(err);
+ }
+ errno = 0;
+
+ return err == -ENOENT;
+}
+
+/**
+ * gem_context_clone_with_engines:
+ * @i915: open i915 drm file descriptor
+ * @src: i915 context id
+ *
+ * Special purpose wrapper to create a new context by cloning engines from @src.
+ *
+ * In can be called regardless of whether the kernel supports context cloning.
+ *
+ * Intended purpose is to use for creating contexts against which work will be
+ * submitted and the engine index came from external source, derived from a
+ * default context potentially configured with an engine map.
+ */
+uint32_t gem_context_clone_with_engines(int i915, uint32_t src)
+{
+ if (!gem_has_context_clone(i915))
+ return gem_context_create(i915);
Yes, that should cover us for older kernels and keep the for_each loops
happy.
+ else
+ return gem_context_clone(i915, src, 0,
+ I915_CONTEXT_CLONE_ENGINES);
0 and CLONE are the wrong way around.
Oopsie.
I would have done
int __gem_context_clone_with_engines(int i915, uint32_t src, uint32_t *out)
{
int err;
err = __gem_context_clone(i915, src, CLONE_ENGINES, 0, out);
if (err && !gem_has_context_clone(i915))
err = __gem_context_create(i915, out);
return err;
}
uint32_t gem_context_clone_with_engines(int i915, uint32_t src)
{
uint32_t ctx;
igt_assert_eq(__gem_context_clone_with_engine(i915, src, &ctx), 0);
return ctx;
}
I think I prefer my version as it is a bit more explicit.
I was hoping to do something more like
err = __clone()
if (err == ENOSYS)
err = __create()
Either way, I would suggest doing
int __gem_context_clone_with_engines(int i915, uint32_t src, uint32_t *out);
uint32_t gem_context_clone_with_engines(int i915, uint32_t src);
as I prefer that style of error message.
Error message? What do you mean?
Nothing else to complain about,
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
So this is conditional on rewriting it as above or not?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
