On 27/06/2019 21:15, Andi Shyti wrote:
Hi Tvrtko,+const struct intel_execution_engine2 * +gem_eb_flags_to_engine(unsigned int flags) +{ + const struct intel_execution_engine2 *e2; + + __for_each_static_engine(e2) { + if (e2->flags == flags) + return e2; + } + + return NULL; +}the amount of "helpers" is getting almost unbearable for a simple mind like mine. This means that we can get rid of gem_execbuf_flags_to_engine_class gem_ring_is_physical_engine gem_ring_has_physical_engine in igt_gt.c, right?
If/when there are no callers left we can like everything. I dont' know if that is the case right now.
+bool gem_context_has_engine_map(int fd, uint32_t ctx) +{ + struct drm_i915_gem_context_param param = { + .param = I915_CONTEXT_PARAM_ENGINES, + .ctx_id = ctx + }; + int ret; + + ret = __gem_context_get_param(fd, ¶m); + igt_assert_eq(ret, 0); + + return param.size;a small nitpick: bool to me means '0' or '1'. So, if you do 'return param.size', I would call the function gem_context_get_param_size, otherwise I would return '!!param.size' and keep it bool.
Some people would then complain !! was not needed. ~o~And actually I think I want to remove the distinction of no map and map with no engines for the callers of this helpers. So returning size would not work for that.
(We are also somehow abusing on the size definition of bool in C99/C17 or previous.) I'm not asking you to change it, but it would make me happier :)
I don't understand the issue with size definition. Size is u32 - will not implicit conversion to bool just work?
+} diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h index 2415fd1e379b..b175483fac1c 100644 --- a/lib/i915/gem_engine_topology.h +++ b/lib/i915/gem_engine_topology.h @@ -53,6 +53,11 @@ int gem_context_lookup_engine(int fd, uint64_t engine, uint32_t ctx_id,void gem_context_set_all_engines(int fd, uint32_t ctx); +bool gem_context_has_engine_map(int fd, uint32_t ctx);+ +const struct intel_execution_engine2 * +gem_eb_flags_to_engine(unsigned int flags); + #define __for_each_static_engine(e__) \ for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)diff --git a/tests/i915/gem_ctx_switch.c b/tests/i915/gem_ctx_switch.cindex 647911d4c42e..407905de2d34 100644 --- a/tests/i915/gem_ctx_switch.c +++ b/tests/i915/gem_ctx_switch.c @@ -55,7 +55,7 @@ static double elapsed(const struct timespec *start, const struct timespec *end)static int measure_qlen(int fd,struct drm_i915_gem_execbuffer2 *execbuf, - unsigned int *engine, unsigned int nengine, + const struct intel_engine_data *engines, int timeout) { const struct drm_i915_gem_exec_object2 * const obj = @@ -63,15 +63,17 @@ static int measure_qlen(int fd, uint32_t ctx[64]; int min = INT_MAX, max = 0;- for (int i = 0; i < ARRAY_SIZE(ctx); i++)+ for (int i = 0; i < ARRAY_SIZE(ctx); i++) { ctx[i] = gem_context_create(fd); + gem_context_set_all_engines(fd, ctx[i]); + }- for (unsigned int n = 0; n < nengine; n++) {+ for (unsigned int n = 0; n < engines->nengines; n++) { uint64_t saved = execbuf->flags; struct timespec tv = {}; int q;- execbuf->flags |= engine[n];+ execbuf->flags |= engines->engines[n].flags;for (int i = 0; i < ARRAY_SIZE(ctx); i++) {execbuf->rsvd1 = ctx[i]; @@ -90,7 +92,8 @@ static int measure_qlen(int fd, * Be conservative and aim not to overshoot timeout, so scale * down by 8 for hopefully a max of 12.5% error. */ - q = ARRAY_SIZE(ctx) * timeout * 1e9 / igt_nsec_elapsed(&tv) / 8 + 1; + q = ARRAY_SIZE(ctx) * timeout * 1e9 / igt_nsec_elapsed(&tv) / + 8 + 1;I don't know whether it's me who is paranoic, but the change above doesn't match the commit log.
What do you mean: """ Also beware of drive-by formatting changes. """ ;) File to many lines falling of 80 char so I tidied it alongside.
if (q < min) min = q; if (q > max) @@ -107,7 +110,7 @@ static int measure_qlen(int fd, }static void single(int fd, uint32_t handle,- const struct intel_execution_engine *e, + const struct intel_execution_engine2 *e2, unsigned flags, const int ncpus, int timeout) @@ -125,13 +128,14 @@ static void single(int fd, uint32_t handle, shared = mmap(NULL, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0); igt_assert(shared != MAP_FAILED);- gem_require_ring(fd, e->exec_id | e->flags);- for (n = 0; n < 64; n++) { if (flags & QUEUE) contexts[n] = gem_queue_create(fd); else contexts[n] = gem_context_create(fd); + + if (gem_context_has_engine_map(fd, 0)) + gem_context_set_all_engines(fd, contexts[n]); }memset(&obj, 0, sizeof(obj));@@ -152,12 +156,12 @@ static void single(int fd, uint32_t handle, execbuf.buffers_ptr = to_user_pointer(&obj); execbuf.buffer_count = 1; execbuf.rsvd1 = contexts[0]; - execbuf.flags = e->exec_id | e->flags; + execbuf.flags = e2->flags; execbuf.flags |= LOCAL_I915_EXEC_HANDLE_LUT; execbuf.flags |= LOCAL_I915_EXEC_NO_RELOC; igt_require(__gem_execbuf(fd, &execbuf) == 0); if (__gem_execbuf(fd, &execbuf)) { - execbuf.flags = e->exec_id | e->flags; + execbuf.flags = e2->flags; reloc.target_handle = obj.handle; gem_execbuf(fd, &execbuf); } @@ -190,7 +194,8 @@ static void single(int fd, uint32_t handle, clock_gettime(CLOCK_MONOTONIC, &now);igt_info("[%d] %s: %'u cycles: %.3fus%s\n",- child, e->name, count, elapsed(&start, &now)*1e6 / count, + child, e2->name, count, + elapsed(&start, &now) * 1e6 / count, flags & INTERRUPTIBLE ? " (interruptible)" : "");shared[child].elapsed = elapsed(&start, &now);@@ -209,7 +214,7 @@ static void single(int fd, uint32_t handle, }igt_info("Total %s: %'lu cycles: %.3fus%s\n",- e->name, total, max*1e6 / total, + e2->name, total, max*1e6 / total, flags & INTERRUPTIBLE ? " (interruptible)" : ""); }@@ -223,25 +228,20 @@ static void all(int fd, uint32_t handle, unsigned flags, int timeout){ struct drm_i915_gem_execbuffer2 execbuf; struct drm_i915_gem_exec_object2 obj[2]; - unsigned int engine[16], e; - const char *name[16]; + struct intel_engine_data engines = { }; uint32_t contexts[65]; - unsigned int nengine; int n, qlen;- nengine = 0;- for_each_physical_engine(fd, e) { - engine[nengine] = e; - name[nengine] = e__->name; - nengine++; - } - igt_require(nengine); + engines = intel_init_engine_list(fd, 0); + igt_require(engines.nengines);Off-topic: This I guess can be the "flags" mapping that Chris was suggesting once, I guess we can achieve that by just doing the above without adding helpers (which would drive crazy people like me).
I don't remember what was this flags mapping. Something to return a list of eb->flags for all physical engines? Not sure I like that since flags carry is very limited information. Having a list of engine objects sounds much better.
The rest of the patch I trust you it works :) (however the devotion to whatever is legacy doesn't leave me with a good taste after all the work done) You can add my Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxx> Thanks, this patch clarifies a few more things as well,
Thanks! Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
