[i-g-t,3/3] i915/gem_exec_fence: Avoid long preempt-off sleeps
diff mbox series

Message ID 20191108142250.22709-3-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [i-g-t,1/3] i915/gem_exec_fence: KMS master is not required
Related show

Commit Message

Chris Wilson Nov. 8, 2019, 2:22 p.m. UTC
The kernel is now enforcing that clients are not allowed to block higher
priority contexts from accessing the GPU; one is no longer allowed to
sleep for a second hogging the GPU. Reduce the sleep down to 50ms, short
enough not to anger the preempt-off checks while long enough for any
ordinary GPU work to complete.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_fence.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Tvrtko Ursulin Nov. 13, 2019, 10:54 a.m. UTC | #1
On 08/11/2019 14:22, Chris Wilson wrote:
> The kernel is now enforcing that clients are not allowed to block higher
> priority contexts from accessing the GPU; one is no longer allowed to
> sleep for a second hogging the GPU. Reduce the sleep down to 50ms, short
> enough not to anger the preempt-off checks while long enough for any
> ordinary GPU work to complete.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   tests/i915/gem_exec_fence.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/i915/gem_exec_fence.c b/tests/i915/gem_exec_fence.c
> index 4e8bdd7b3..002120bf3 100644
> --- a/tests/i915/gem_exec_fence.c
> +++ b/tests/i915/gem_exec_fence.c
> @@ -348,7 +348,8 @@ static void test_fence_await(int fd, unsigned ring, unsigned flags)
>   		i++;
>   	}
>   
> -	sleep(1);
> +	/* Long, but not too long to anger preemption disable checks */
> +	usleep(50 * 1000); /* 50 ms, typical preempt reset is 150+ms */

Was the point of this sleep two-fold: 1) allow spinner to start (use 
polling spinner?) and allow forked children to submit (use pipe sync?)?

But okay as a quick fix.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

>   
>   	/* Check for invalidly completing the task early */
>   	igt_assert(fence_busy(spin->out_fence));
>
Chris Wilson Nov. 13, 2019, 11:06 a.m. UTC | #2
Quoting Tvrtko Ursulin (2019-11-13 10:54:58)
> 
> On 08/11/2019 14:22, Chris Wilson wrote:
> > The kernel is now enforcing that clients are not allowed to block higher
> > priority contexts from accessing the GPU; one is no longer allowed to
> > sleep for a second hogging the GPU. Reduce the sleep down to 50ms, short
> > enough not to anger the preempt-off checks while long enough for any
> > ordinary GPU work to complete.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   tests/i915/gem_exec_fence.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/i915/gem_exec_fence.c b/tests/i915/gem_exec_fence.c
> > index 4e8bdd7b3..002120bf3 100644
> > --- a/tests/i915/gem_exec_fence.c
> > +++ b/tests/i915/gem_exec_fence.c
> > @@ -348,7 +348,8 @@ static void test_fence_await(int fd, unsigned ring, unsigned flags)
> >               i++;
> >       }
> >   
> > -     sleep(1);
> > +     /* Long, but not too long to anger preemption disable checks */
> > +     usleep(50 * 1000); /* 50 ms, typical preempt reset is 150+ms */
> 
> Was the point of this sleep two-fold: 1) allow spinner to start (use 
> polling spinner?) and allow forked children to submit (use pipe sync?)?

It was 3: wait long enough so we could be reasonably sure that the
children were indeed waiting and given the opportunity would not run
ahead of the parent.

This is a very loose, runs anywhere test. Revised variants are in
gem_exec_scheduler, which I like to think are a bit more precise.
Although I think the general test concept of "encouraging the children to
run first" could do with a little more exploration, as therein are where
the bugs lie.
-Chris

Patch
diff mbox series

diff --git a/tests/i915/gem_exec_fence.c b/tests/i915/gem_exec_fence.c
index 4e8bdd7b3..002120bf3 100644
--- a/tests/i915/gem_exec_fence.c
+++ b/tests/i915/gem_exec_fence.c
@@ -348,7 +348,8 @@  static void test_fence_await(int fd, unsigned ring, unsigned flags)
 		i++;
 	}
 
-	sleep(1);
+	/* Long, but not too long to anger preemption disable checks */
+	usleep(50 * 1000); /* 50 ms, typical preempt reset is 150+ms */
 
 	/* Check for invalidly completing the task early */
 	igt_assert(fence_busy(spin->out_fence));