Re: [PATCH igt 03/10] igt/gem_ctx_switch: Exercise all engines at once

Tvrtko Ursulin
2017-07-31T09:57:25Z

On 28/07/2017 13:08, Chris Wilson wrote:
> Just a small variant to apply a continuous context-switch load to all
> engines.
> ---
> tests/gem_ctx_switch.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 80 insertions(+)
>
> diff --git a/tests/gem_ctx_switch.c b/tests/gem_ctx_switch.c
> index b6ea71cf..df22efec 100644
> --- a/tests/gem_ctx_switch.c
> +++ b/tests/gem_ctx_switch.c
> @@ -140,6 +140,81 @@ static void single(int fd, uint32_t handle,
> gem_context_destroy(fd, contexts[n]);
> }
>
> +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;

Need to add MAX_ENGINES somewhere in lib/ for maintainability.

> + const char *name[16];
> + uint32_t contexts[64];
> + unsigned int nengine;
> + int n;
> +
> + nengine = 0;
> + for_each_engine(fd, e) {
> + if (e == 0 || e == I915_EXEC_BSD)
> + continue;

Engine discrimination! :) Why? BSD1 won't work except on large GTs.

Also, if we cannot agree to drop I915_EXEC_DEFAULT from the
for_each_engine list, I think we at least need to stop the "e == 0"
magic which is sprinkled around the tests now. Even though this very
fact demonstrates having it in a list is most often not what is wanted.

Not sure exactly what to suggest as an alternative. for_each_hw_engine?
real engine? gt engine?

> +
> + engine[nengine] = e;
> + name[nengine] = e__->name;
> + nengine++;
> + }
> + igt_require(nengine);
> +
> + igt_require(__gem_context_create(fd, &contexts[0]) == 0);

Feels to opaque - should we pencil in adding of
igt_require_context_support or something?

> + for (n = 1; n < 64; n++)
> + contexts[n] = gem_context_create(fd);
> +
> + memset(obj, 0, sizeof(obj));
> + obj[1].handle = handle;
> +
> + memset(&execbuf, 0, sizeof(execbuf));
> + execbuf.buffers_ptr = to_user_pointer(obj + 1);
> + execbuf.buffer_count = 1;
> + execbuf.rsvd1 = contexts[0];
> + 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)) {

Why the same execbuf two times in a row?

> + execbuf.flags = 0;
> + gem_execbuf(fd, &execbuf);
> + }
> + gem_sync(fd, handle);

What is the purpose of exercising context[0] before the main loop below?

> + execbuf.buffers_ptr = to_user_pointer(obj);
> + execbuf.buffer_count = 2;
> +
> + igt_fork(child, nengine) {
> + struct timespec start, now;
> + unsigned int count = 0;
> +
> + obj[0].handle = gem_create(fd, 4096);
> + execbuf.flags |= engine[child];
> + gem_execbuf(fd, &execbuf);
> + gem_sync(fd, obj[0].handle);
> +
> + clock_gettime(CLOCK_MONOTONIC, &start);
> + do {
> + for (int loop = 0; loop < 64; loop++) {
> + execbuf.rsvd1 = contexts[loop % 64];
> + gem_execbuf(fd, &execbuf);
> + }
> + count += 64;
> + clock_gettime(CLOCK_MONOTONIC, &now);
> + } while (elapsed(&start, &now) < timeout);
> + gem_sync(fd, obj[0].handle);
> + clock_gettime(CLOCK_MONOTONIC, &now);
> + gem_close(fd, obj[0].handle);
> +
> + igt_info("[%d] %s: %'u cycles: %.3fus%s\n",
> + child, name[child], count, elapsed(&start, &now)*1e6 / count,
> + flags & INTERRUPTIBLE ? " (interruptible)" : "");
> + }
> + igt_waitchildren();
> +
> + for (n = 0; n < 64; n++)
> + gem_context_destroy(fd, contexts[n]);
> +}
> +
> igt_main
> {
> const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> @@ -177,6 +252,11 @@ igt_main
> single(fd, light, e, INTERRUPTIBLE, ncpus, 150);
> }
>
> + igt_subtest("basic-all")
> + all(fd, light, 0, 20);
> + igt_subtest("basic-all-heavy")
> + all(fd, heavy, 0, 20);
> +
> igt_fixture {
> igt_stop_hang_detector();
> gem_close(fd, heavy);
>

Regards,

Tvrtko