[Intel-gfx] [igt-dev] [PATCH i-g-t 2/3] tests/gem_eio: Speed up test execution
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Mar 23 09:46:49 UTC 2018
On 22/03/2018 17:44, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-03-22 17:24:16)
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> If we stop relying on regular GPU hangs to be detected, but trigger them
>> manually as soon as we know our batch of interest is actually executing
>> on the GPU, we can dramatically speed up various subtests.
>>
>> This is enabled by the pollable spin batch added in the previous patch.
>>
>> v2:
>> * Test gem_wait after reset/wedge and with reset/wedge after a few
>> predefined intervals since gem_wait invocation. (Chris Wilson)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Antonio Argenziano <antonio.argenziano at intel.com>
>> ---
>> lib.tar | Bin 0 -> 102400 bytes
>> tests/gem_eio.c | 214 ++++++++++++++++++++++++++++++++++++++++++--------------
>> 2 files changed, 160 insertions(+), 54 deletions(-)
>> create mode 100644 lib.tar
>>
>> diff --git a/tests/gem_eio.c b/tests/gem_eio.c
>> index 4bcc5937db39..2f9e954085ec 100644
>> --- a/tests/gem_eio.c
>> +++ b/tests/gem_eio.c
>> @@ -35,6 +35,7 @@
>> #include <inttypes.h>
>> #include <errno.h>
>> #include <sys/ioctl.h>
>> +#include <signal.h>
>>
>> #include <drm.h>
>>
>> @@ -71,26 +72,23 @@ static void trigger_reset(int fd)
>> gem_quiescent_gpu(fd);
>> }
>>
>> -static void wedge_gpu(int fd)
>> +static void manual_hang(int drm_fd)
>> {
>> - /* First idle the GPU then disable GPU resets before injecting a hang */
>> - gem_quiescent_gpu(fd);
>> -
>> - igt_require(i915_reset_control(false));
>> + int dir = igt_debugfs_dir(drm_fd);
>>
>> - igt_debug("Wedging GPU by injecting hang\n");
>> - igt_post_hang_ring(fd, igt_hang_ring(fd, I915_EXEC_DEFAULT));
>> + igt_sysfs_set(dir, "i915_wedged", "-1");
>>
>> - igt_assert(i915_reset_control(true));
>> + close(dir);
>> }
>>
>> -static void wedgeme(int drm_fd)
>> +static void wedge_gpu(int fd)
>> {
>> - int dir = igt_debugfs_dir(drm_fd);
>> -
>> - igt_sysfs_set(dir, "i915_wedged", "-1");
>> + /* First idle the GPU then disable GPU resets before injecting a hang */
>> + gem_quiescent_gpu(fd);
>>
>> - close(dir);
>> + igt_require(i915_reset_control(false));
>> + manual_hang(fd);
>> + igt_assert(i915_reset_control(true));
>> }
>>
>> static int __gem_throttle(int fd)
>> @@ -149,26 +147,111 @@ static int __gem_wait(int fd, uint32_t handle, int64_t timeout)
>> return err;
>> }
>>
>> -static void test_wait(int fd)
>> +static igt_spin_t * __spin_poll(int fd, uint32_t ctx, unsigned long flags)
>> +{
>> + if (gem_can_store_dword(fd, flags))
>> + return __igt_spin_batch_new_poll(fd, ctx, flags);
>> + else
>> + return __igt_spin_batch_new(fd, ctx, flags, 0);
>> +}
>> +
>> +static void __spin_wait(int fd, igt_spin_t *spin)
>> +{
>> + if (spin->running) {
>> + igt_spin_busywait_until_running(spin);
>> + } else {
>> + igt_debug("__spin_wait - usleep mode\n");
>> + usleep(500e3); /* Better than nothing! */
>> + }
>> +}
>> +
>> +static igt_spin_t * spin_sync(int fd, uint32_t ctx, unsigned long flags)
>> +{
>> + igt_spin_t *spin = __spin_poll(fd, ctx, flags);
>> +
>> + __spin_wait(fd, spin);
>> +
>> + return spin;
>> +}
>> +
>> +static int debugfs_dir = -1;
>> +
>> +static void hang_handler(int sig)
>> +{
>> + igt_sysfs_set(debugfs_dir, "i915_wedged", "-1");
>> +}
>> +
>> +static void hang_after(int fd, unsigned int us)
>> +{
>> + struct sigaction sa = { .sa_handler = hang_handler };
>> + struct itimerval itv = { };
>> +
>> + debugfs_dir = igt_debugfs_dir(fd);
>> + igt_assert_fd(debugfs_dir);
>> +
>> + igt_assert_eq(sigaction(SIGALRM, &sa, NULL), 0);
>> +
>> + itv.it_value.tv_sec = us / 1000000;
>
> USEC_PER_SEC.
>
>> + itv.it_value.tv_usec = us % 1000000;
>> + setitimer(ITIMER_REAL, &itv, NULL);
>
> Ok, that gives a single shot signal.
>
> I would have used
> struct sigevent sev = {
> .sigev_notify = SIGEV_THREAD,
> .sigev_value.sigval_int = debugfs_dir
> .sigev_notify_function = hang_handler
> };
> timer_create(CLOCK_MONOTONIC, &sec, &timer);
> timer_settime(timer, 0, &its, NULL);
>
> Then
>
> static void hang_handler(union sigval arg)
> {
> igt_sysfs_set(arg.sival_int, "i915_wedged", 1);
> }
>
> No signals, nor globals required :)
I wasn't familiar with this facility.
It creates a new thread, so any hopes for small microsecond delays might
be ruined. I can try it if you think it is still worth it?
> The problem with using a signal is that it interrupts the gem_wait()
> and so we don't actually check that it is being woken by the hang
> because it is already awake. Gah.
Hm... if I am following correctly, we end up with -ERESTARTSYS and the
the ioctl can get restarted for us, if I would set SA_RESTART.
At the moment it happens to work because drmIoctl restart the signal.
>> +static void cleanup_hang(void)
>> +{
>> + struct itimerval itv = { };
>> +
>> + igt_assert_eq(setitimer(ITIMER_REAL, &itv, NULL), 0);
>
> You also need a sleep here as it does not flush inflight signals.
> (Also timer_destroy :)
I always pass a longer timeout to gem_wait than the signal so I think it
should be guaranteed that the signal had fired before gem_wait will be
exiting.
Regards,
Tvrtko
>
>> + igt_assert_fd(debugfs_dir);
>> + close(debugfs_dir);
>> + debugfs_dir = -1;
>> +}
>> +
>> +static int __check_wait(int fd, uint32_t bo, unsigned int wait)
>> +{
>> + unsigned long wait_timeout = 250e6; /* Some margin for actual reset. */
>> + int ret;
>> +
>> + if (wait) {
>> + wait_timeout += wait * 2000; /* x2 for safety. */
>> + wait_timeout += 250e6; /* Margin for signal delay. */;
>> + hang_after(fd, wait);
>> + } else {
>> + manual_hang(fd);
>> + }
>> +
>> + ret = __gem_wait(fd, bo, wait_timeout);
>
> Ok, I understand where the concerned about how long it took to recover
> from reset came from :)
>
>> +
>> + if (wait)
>> + cleanup_hang();
>> +
>> + return ret;
>> +}
>> +
>> +#define TEST_WEDGE (1)
>> +
>> +static void test_wait(int fd, unsigned int flags, unsigned int wait)
>> {
>> - igt_hang_t hang;
>> + igt_spin_t *hang;
>>
>> igt_require_gem(fd);
>>
>> - /* If the request we wait on completes due to a hang (even for
>> + /*
>> + * If the request we wait on completes due to a hang (even for
>> * that request), the user expects the return value to 0 (success).
>> */
>> - hang = igt_hang_ring(fd, I915_EXEC_DEFAULT);
>> - igt_assert_eq(__gem_wait(fd, hang.handle, -1), 0);
>> - igt_post_hang_ring(fd, hang);
>>
>> - /* If the GPU is wedged during the wait, again we expect the return
>> - * value to be 0 (success).
>> - */
>> - igt_require(i915_reset_control(false));
>> - hang = igt_hang_ring(fd, I915_EXEC_DEFAULT);
>> - igt_assert_eq(__gem_wait(fd, hang.handle, -1), 0);
>> - igt_post_hang_ring(fd, hang);
>> + if (flags & TEST_WEDGE)
>> + igt_require(i915_reset_control(false));
>> + else
>> + igt_require(i915_reset_control(true));
>> +
>> + hang = spin_sync(fd, 0, I915_EXEC_DEFAULT);
>> +
>> + igt_assert_eq(__check_wait(fd, hang->handle, wait), 0);
>> +
>> + igt_spin_batch_free(fd, hang);
>> +
>> igt_require(i915_reset_control(true));
>>
>> trigger_reset(fd);
>> @@ -181,7 +264,7 @@ static void test_suspend(int fd, int state)
>>
>> /* Check we can suspend when the driver is already wedged */
>> igt_require(i915_reset_control(false));
>> - wedgeme(fd);
>> + manual_hang(fd);
>>
>> igt_system_suspend_autoresume(state, SUSPEND_TEST_DEVICES);
>>
>> @@ -189,7 +272,7 @@ static void test_suspend(int fd, int state)
>> trigger_reset(fd);
>> }
>>
>> -static void test_inflight(int fd)
>> +static void test_inflight(int fd, unsigned int wait)
>> {
>> const uint32_t bbe = MI_BATCH_BUFFER_END;
>> struct drm_i915_gem_exec_object2 obj[2];
>> @@ -209,11 +292,10 @@ static void test_inflight(int fd)
>> int fence[64]; /* conservative estimate of ring size */
>>
>> gem_quiescent_gpu(fd);
>> -
>> igt_debug("Starting %s on engine '%s'\n", __func__, e__->name);
>> igt_require(i915_reset_control(false));
>>
>> - hang = __igt_spin_batch_new(fd, 0, engine, 0);
>> + hang = spin_sync(fd, 0, engine);
>> obj[0].handle = hang->handle;
>>
>> memset(&execbuf, 0, sizeof(execbuf));
>> @@ -227,7 +309,8 @@ static void test_inflight(int fd)
>> igt_assert(fence[n] != -1);
>> }
>>
>> - igt_assert_eq(__gem_wait(fd, obj[1].handle, -1), 0);
>> + igt_assert_eq(__check_wait(fd, obj[1].handle, wait), 0);
>> +
>> for (unsigned int n = 0; n < ARRAY_SIZE(fence); n++) {
>> igt_assert_eq(sync_fence_status(fence[n]), -EIO);
>> close(fence[n]);
>> @@ -256,7 +339,7 @@ static void test_inflight_suspend(int fd)
>> obj[1].handle = gem_create(fd, 4096);
>> gem_write(fd, obj[1].handle, 0, &bbe, sizeof(bbe));
>>
>> - hang = __igt_spin_batch_new(fd, 0, 0, 0);
>> + hang = spin_sync(fd, 0, 0);
>> obj[0].handle = hang->handle;
>>
>> memset(&execbuf, 0, sizeof(execbuf));
>> @@ -273,7 +356,8 @@ static void test_inflight_suspend(int fd)
>> igt_set_autoresume_delay(30);
>> igt_system_suspend_autoresume(SUSPEND_STATE_MEM, SUSPEND_TEST_NONE);
>>
>> - igt_assert_eq(__gem_wait(fd, obj[1].handle, -1), 0);
>> + igt_assert_eq(__check_wait(fd, obj[1].handle, 10), 0);
>> +
>> for (unsigned int n = 0; n < ARRAY_SIZE(fence); n++) {
>> igt_assert_eq(sync_fence_status(fence[n]), -EIO);
>> close(fence[n]);
>> @@ -301,7 +385,7 @@ static uint32_t context_create_safe(int i915)
>> return param.ctx_id;
>> }
>>
>> -static void test_inflight_contexts(int fd)
>> +static void test_inflight_contexts(int fd, unsigned int wait)
>> {
>> struct drm_i915_gem_exec_object2 obj[2];
>> const uint32_t bbe = MI_BATCH_BUFFER_END;
>> @@ -330,7 +414,7 @@ static void test_inflight_contexts(int fd)
>> igt_debug("Starting %s on engine '%s'\n", __func__, e__->name);
>> igt_require(i915_reset_control(false));
>>
>> - hang = __igt_spin_batch_new(fd, 0, engine, 0);
>> + hang = spin_sync(fd, 0, engine);
>> obj[0].handle = hang->handle;
>>
>> memset(&execbuf, 0, sizeof(execbuf));
>> @@ -345,7 +429,8 @@ static void test_inflight_contexts(int fd)
>> igt_assert(fence[n] != -1);
>> }
>>
>> - igt_assert_eq(__gem_wait(fd, obj[1].handle, -1), 0);
>> + igt_assert_eq(__check_wait(fd, obj[1].handle, wait), 0);
>> +
>> for (unsigned int n = 0; n < ARRAY_SIZE(fence); n++) {
>> igt_assert_eq(sync_fence_status(fence[n]), -EIO);
>> close(fence[n]);
>> @@ -375,7 +460,7 @@ static void test_inflight_external(int fd)
>> fence = igt_cork_plug(&cork, fd);
>>
>> igt_require(i915_reset_control(false));
>> - hang = __igt_spin_batch_new(fd, 0, 0, 0);
>> + hang = __spin_poll(fd, 0, 0);
>>
>> memset(&obj, 0, sizeof(obj));
>> obj.handle = gem_create(fd, 4096);
>> @@ -393,6 +478,9 @@ static void test_inflight_external(int fd)
>> fence = execbuf.rsvd2 >> 32;
>> igt_assert(fence != -1);
>>
>> + __spin_wait(fd, hang);
>> + manual_hang(fd);
>> +
>> gem_sync(fd, hang->handle); /* wedged, with an unready batch */
>> igt_assert(!gem_bo_busy(fd, hang->handle));
>> igt_assert(gem_bo_busy(fd, obj.handle));
>> @@ -407,7 +495,7 @@ static void test_inflight_external(int fd)
>> trigger_reset(fd);
>> }
>>
>> -static void test_inflight_internal(int fd)
>> +static void test_inflight_internal(int fd, unsigned int wait)
>> {
>> struct drm_i915_gem_execbuffer2 execbuf;
>> struct drm_i915_gem_exec_object2 obj[2];
>> @@ -420,7 +508,7 @@ static void test_inflight_internal(int fd)
>> igt_require(gem_has_exec_fence(fd));
>>
>> igt_require(i915_reset_control(false));
>> - hang = __igt_spin_batch_new(fd, 0, 0, 0);
>> + hang = spin_sync(fd, 0, 0);
>>
>> memset(obj, 0, sizeof(obj));
>> obj[0].handle = hang->handle;
>> @@ -441,7 +529,8 @@ static void test_inflight_internal(int fd)
>> nfence++;
>> }
>>
>> - igt_assert_eq(__gem_wait(fd, obj[1].handle, -1), 0);
>> + igt_assert_eq(__check_wait(fd, obj[1].handle, wait), 0);
>> +
>> while (nfence--) {
>> igt_assert_eq(sync_fence_status(fences[nfence]), -EIO);
>> close(fences[nfence]);
>> @@ -484,29 +573,46 @@ igt_main
>> igt_subtest("execbuf")
>> test_execbuf(fd);
>>
>> - igt_subtest("wait")
>> - test_wait(fd);
>> -
>> igt_subtest("suspend")
>> test_suspend(fd, SUSPEND_STATE_MEM);
>>
>> igt_subtest("hibernate")
>> test_suspend(fd, SUSPEND_STATE_DISK);
>>
>> - igt_subtest("in-flight")
>> - test_inflight(fd);
>> -
>> - igt_subtest("in-flight-contexts")
>> - test_inflight_contexts(fd);
>> -
>> igt_subtest("in-flight-external")
>> test_inflight_external(fd);
>>
>> - igt_subtest("in-flight-internal") {
>> - igt_skip_on(gem_has_semaphores(fd));
>> - test_inflight_internal(fd);
>> - }
>> -
>> igt_subtest("in-flight-suspend")
>> test_inflight_suspend(fd);
>> +
>> + igt_subtest_group {
>> + const struct {
>> + unsigned int wait;
>> + const char *name;
>> + } waits[] = {
>> + { .wait = 0, .name = "immediate" },
>> + { .wait = 10, .name = "10us" },
>
> i915_request_spin is set to 2us currently :| I guess that's a really hard
> window to hit reliably. Maybe we should spin for 200ms just to make
> testing easier!
>
>> + { .wait = 10000, .name = "10ms" },
>> + };
>> + unsigned int i;
>> +
>> + for (i = 0; i < sizeof(waits) / sizeof(waits[0]); i++) {
>> + igt_subtest_f("wait-%s", waits[i].name)
>> + test_wait(fd, 0, waits[i].wait);
>> +
>> + igt_subtest_f("wait-wedge-%s", waits[i].name)
>> + test_wait(fd, TEST_WEDGE, waits[i].wait);
>
> Ok.
>
>> +
>> + igt_subtest_f("in-flight-%s", waits[i].name)
>> + test_inflight(fd, waits[i].wait);
>> +
>> + igt_subtest_f("in-flight-contexts-%s", waits[i].name)
>> + test_inflight_contexts(fd, waits[i].wait);
>> +
>> + igt_subtest_f("in-flight-internal-%s", waits[i].name) {
>> + igt_skip_on(gem_has_semaphores(fd));
>> + test_inflight_internal(fd, waits[i].wait);
>
> And ok.
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list