| Message ID | 20180117131143.27896-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/01/2018 13:11, Chris Wilson wrote: > When testing that the timeout fired, we need to be sure we have waited > just long enough for the timeout to have occurred and for the softirq > (on another cpu) to have completed. Sleeping for an arbitrary amount is > prone to error, so wait for the timeout instead and complain if it was > too late. > > v2: Use wait_event_timeout to provide an upper bound > > References: https://bugs.freedesktop.org/show_bug.cgi?id=104670 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 27 +++++--------------------- > 1 file changed, 5 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c > index 4fb51deb81a1..301f0ce4cd3c 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c > +++ b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c > @@ -661,7 +661,6 @@ static int test_dma_fence(void *arg) > { > struct i915_sw_fence *timeout = NULL, *not = NULL; > unsigned long delay = i915_selftest.timeout_jiffies; > - unsigned long end, sleep; > struct dma_fence *dma; > int err; > > @@ -688,36 +687,20 @@ static int test_dma_fence(void *arg) > } > > /* We round the timeout for the fence up to the next second */ > - end = round_jiffies_up(jiffies + delay); > + delay = round_jiffies_up_relative(delay); > > - sleep = jiffies_to_usecs(delay) / 3; > - usleep_range(sleep, 2 * sleep); > - if (time_after(jiffies, end)) { > - pr_debug("Slept too long, delay=%lu, skipping!\n", delay); > - goto skip; > - } > - > - if (i915_sw_fence_done(timeout) || i915_sw_fence_done(not)) { > - pr_err("Fences signaled too early\n"); This check still holds, no? Or there is another test looking at that? > + if (wait_event_timeout(timeout->wait, > + i915_sw_fence_done(timeout), > + 2 * delay)) { > + pr_err("Timeout fence unsignaled!\n"); > goto err; Doh! Regards, Tvrtko > } > > - do { > - sleep = jiffies_to_usecs(end - jiffies + 1); > - usleep_range(sleep, 2 * sleep); > - } while (!time_after(jiffies, end)); > - > if (i915_sw_fence_done(not)) { > pr_err("No timeout fence signaled!\n"); > goto err; > } > > - if (!i915_sw_fence_done(timeout)) { > - pr_err("Timeout fence unsignaled!\n"); > - goto err; > - } > - > -skip: > dma_fence_signal(dma); > > if (!i915_sw_fence_done(timeout) || !i915_sw_fence_done(not)) { >
Quoting Tvrtko Ursulin (2018-01-17 13:36:06) > > On 17/01/2018 13:11, Chris Wilson wrote: > > When testing that the timeout fired, we need to be sure we have waited > > just long enough for the timeout to have occurred and for the softirq > > (on another cpu) to have completed. Sleeping for an arbitrary amount is > > prone to error, so wait for the timeout instead and complain if it was > > too late. > > > > v2: Use wait_event_timeout to provide an upper bound > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=104670 > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 27 +++++--------------------- > > 1 file changed, 5 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c > > index 4fb51deb81a1..301f0ce4cd3c 100644 > > --- a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c > > +++ b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c > > @@ -661,7 +661,6 @@ static int test_dma_fence(void *arg) > > { > > struct i915_sw_fence *timeout = NULL, *not = NULL; > > unsigned long delay = i915_selftest.timeout_jiffies; > > - unsigned long end, sleep; > > struct dma_fence *dma; > > int err; > > > > @@ -688,36 +687,20 @@ static int test_dma_fence(void *arg) > > } > > > > /* We round the timeout for the fence up to the next second */ > > - end = round_jiffies_up(jiffies + delay); > > + delay = round_jiffies_up_relative(delay); > > > > - sleep = jiffies_to_usecs(delay) / 3; > > - usleep_range(sleep, 2 * sleep); > > - if (time_after(jiffies, end)) { > > - pr_debug("Slept too long, delay=%lu, skipping!\n", delay); > > - goto skip; > > - } > > - > > - if (i915_sw_fence_done(timeout) || i915_sw_fence_done(not)) { > > - pr_err("Fences signaled too early\n"); > > This check still holds, no? Or there is another test looking at that? We can still use it, I was just a little dubious if it's worth it. I'll put it back, since it is better than nothing. -Chris
diff --git a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c index 4fb51deb81a1..301f0ce4cd3c 100644 --- a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c @@ -661,7 +661,6 @@ static int test_dma_fence(void *arg) { struct i915_sw_fence *timeout = NULL, *not = NULL; unsigned long delay = i915_selftest.timeout_jiffies; - unsigned long end, sleep; struct dma_fence *dma; int err; @@ -688,36 +687,20 @@ static int test_dma_fence(void *arg) } /* We round the timeout for the fence up to the next second */ - end = round_jiffies_up(jiffies + delay); + delay = round_jiffies_up_relative(delay); - sleep = jiffies_to_usecs(delay) / 3; - usleep_range(sleep, 2 * sleep); - if (time_after(jiffies, end)) { - pr_debug("Slept too long, delay=%lu, skipping!\n", delay); - goto skip; - } - - if (i915_sw_fence_done(timeout) || i915_sw_fence_done(not)) { - pr_err("Fences signaled too early\n"); + if (wait_event_timeout(timeout->wait, + i915_sw_fence_done(timeout), + 2 * delay)) { + pr_err("Timeout fence unsignaled!\n"); goto err; } - do { - sleep = jiffies_to_usecs(end - jiffies + 1); - usleep_range(sleep, 2 * sleep); - } while (!time_after(jiffies, end)); - if (i915_sw_fence_done(not)) { pr_err("No timeout fence signaled!\n"); goto err; } - if (!i915_sw_fence_done(timeout)) { - pr_err("Timeout fence unsignaled!\n"); - goto err; - } - -skip: dma_fence_signal(dma); if (!i915_sw_fence_done(timeout) || !i915_sw_fence_done(not)) {
When testing that the timeout fired, we need to be sure we have waited just long enough for the timeout to have occurred and for the softirq (on another cpu) to have completed. Sleeping for an arbitrary amount is prone to error, so wait for the timeout instead and complain if it was too late. v2: Use wait_event_timeout to provide an upper bound References: https://bugs.freedesktop.org/show_bug.cgi?id=104670 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 27 +++++--------------------- 1 file changed, 5 insertions(+), 22 deletions(-)