[Intel-gfx] [PATCH i-g-t v2] tests/i915/gem_ctx_switch: Update with engine discovery
Andi Shyti
andi.shyti at intel.com
Thu Jun 27 20:15:30 UTC 2019
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?
> +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.
(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 :)
> +}
> 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.c
> index 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.
> 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).
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 at intel.com>
Thanks, this patch clarifies a few more things as well,
Andi
More information about the Intel-gfx
mailing list