[PATCH i-g-t 1/3] lib: Assert that we do manage to submit at least one batch when measuring

[PATCH i-g-t 1/3] lib: Assert that we do manage to submit at least one batch when measuring

[Chris Wilson]

As we measure the ring size, we never expect to find we can not submit
no batches at all. Assert against the unexpected.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Antonio Argenziano <antonio.argenziano@intel.com>
---
 lib/i915/gem_ring.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/i915/gem_ring.c b/lib/i915/gem_ring.c
index 10d2f2cd4..7d64165eb 100644
--- a/lib/i915/gem_ring.c
+++ b/lib/i915/gem_ring.c
@@ -100,6 +100,7 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, enum measure_ring_flags
 	} while (1);
 
 	igt_assert_eq(__execbuf(fd, &execbuf), -EINTR);
+	igt_assert(count);
 
 	memset(&itv, 0, sizeof(itv));
 	setitimer(ITIMER_REAL, &itv, NULL);
-- 
2.17.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 2/3] lib: Align ring measurement to timer

[Chris Wilson]

After hitting the SIGINT from execbuf, wait until the next timer signal
before trying again. This aligns the start of the ioctl to the timer,
hopefully maximising the amount of time we have for processing before
the next signal -- trying to prevent the case where we are scheduled out
in the middle of processing and so hit the timer signal too early.

References: https://bugs.freedesktop.org/show_bug.cgi?id=106695
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 lib/i915/gem_ring.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/i915/gem_ring.c b/lib/i915/gem_ring.c
index 7d64165eb..0c061000c 100644
--- a/lib/i915/gem_ring.c
+++ b/lib/i915/gem_ring.c
@@ -96,6 +96,8 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, enum measure_ring_flags
 		if (last == count)
 			break;
 
+		/* sleep until the next timer interrupt (woken on signal) */
+		pause();
 		last = count;
 	} while (1);
 
-- 
2.17.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/3] lib: Align ring measurement to timer

[Antonio Argenziano]

On 30/05/18 03:33, Chris Wilson wrote:
> After hitting the SIGINT from execbuf, wait until the next timer signal
> before trying again. This aligns the start of the ioctl to the timer,
> hopefully maximising the amount of time we have for processing before
> the next signal -- trying to prevent the case where we are scheduled out
> in the middle of processing and so hit the timer signal too early.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=106695
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Not sure I understand what is the sequence of events, is the problem we 
get a signal in the middle of a 'good' execbuf and exit the while loop 
prematurely? If so maybe we can also think of making the timer 'VIRTUAL' 
so that it would decrement only when the process is executing.

Thanks,
Antonio

> ---
>   lib/i915/gem_ring.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/lib/i915/gem_ring.c b/lib/i915/gem_ring.c
> index 7d64165eb..0c061000c 100644
> --- a/lib/i915/gem_ring.c
> +++ b/lib/i915/gem_ring.c
> @@ -96,6 +96,8 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, enum measure_ring_flags
>   		if (last == count)
>   			break;
>   
> +		/* sleep until the next timer interrupt (woken on signal) */
> +		pause();

Does it cause any (sensible) slowdown?

Thanks,
Antonio

>   		last = count;
>   	} while (1);
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/3] lib: Align ring measurement to timer

[Chris Wilson]

Quoting Antonio Argenziano (2018-05-30 18:30:36)
> 
> 
> On 30/05/18 03:33, Chris Wilson wrote:
> > After hitting the SIGINT from execbuf, wait until the next timer signal
> > before trying again. This aligns the start of the ioctl to the timer,
> > hopefully maximising the amount of time we have for processing before
> > the next signal -- trying to prevent the case where we are scheduled out
> > in the middle of processing and so hit the timer signal too early.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=106695
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Not sure I understand what is the sequence of events, is the problem we 
> get a signal in the middle of a 'good' execbuf and exit the while loop 
> prematurely? If so maybe we can also think of making the timer 'VIRTUAL' 
> so that it would decrement only when the process is executing.

If it's VIRTUAL it'll never fire when we wait for space (as being asleep
no user/sys time is consumed).

The only way I can explain 106695 would be with some very strange
scheduler behaviour, but even then it requires us to hit a path where we
actually check for a pending signal -- which should only happen when we
run out of ring space for this setup. Not even the device being wedged
(which it wasn't) would cause the ring to drain. Possibly going over 10s
and the cork being unplugged? Very stange.

> > ---
> >   lib/i915/gem_ring.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/lib/i915/gem_ring.c b/lib/i915/gem_ring.c
> > index 7d64165eb..0c061000c 100644
> > --- a/lib/i915/gem_ring.c
> > +++ b/lib/i915/gem_ring.c
> > @@ -96,6 +96,8 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, enum measure_ring_flags
> >               if (last == count)
> >                       break;
> >   
> > +             /* sleep until the next timer interrupt (woken on signal) */
> > +             pause();
> 
> Does it cause any (sensible) slowdown?

Adds at most one timer interval, 10us. Ok, at a push 2 timer intervals
if it takes longer than first to setup the sleep.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/3] lib: Align ring measurement to timer

[Antonio Argenziano]

On 30/05/18 12:52, Chris Wilson wrote:
> Quoting Antonio Argenziano (2018-05-30 18:30:36)
>>
>>
>> On 30/05/18 03:33, Chris Wilson wrote:
>>> After hitting the SIGINT from execbuf, wait until the next timer signal
>>> before trying again. This aligns the start of the ioctl to the timer,
>>> hopefully maximising the amount of time we have for processing before
>>> the next signal -- trying to prevent the case where we are scheduled out
>>> in the middle of processing and so hit the timer signal too early.
>>>
>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=106695
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> Not sure I understand what is the sequence of events, is the problem we
>> get a signal in the middle of a 'good' execbuf and exit the while loop
>> prematurely? If so maybe we can also think of making the timer 'VIRTUAL'
>> so that it would decrement only when the process is executing.
> 
> If it's VIRTUAL it'll never fire when we wait for space (as being asleep
> no user/sys time is consumed).
> 
> The only way I can explain 106695 would be with some very strange
> scheduler behaviour, but even then it requires us to hit a path where we
> actually check for a pending signal -- which should only happen when we
> run out of ring space for this setup. Not even the device being wedged
> (which it wasn't) would cause the ring to drain. Possibly going over 10s
> and the cork being unplugged? Very stange.

Just a bit concerned that we might be covering up some weird corner case 
where we are sleeping where we shouldn't.

But the patch does what advertised and seems sensible so:

Acked-by: Antonio Argenziano <antonio.argenziano@intel.com>

> 
>>> ---
>>>    lib/i915/gem_ring.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/lib/i915/gem_ring.c b/lib/i915/gem_ring.c
>>> index 7d64165eb..0c061000c 100644
>>> --- a/lib/i915/gem_ring.c
>>> +++ b/lib/i915/gem_ring.c
>>> @@ -96,6 +96,8 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, enum measure_ring_flags
>>>                if (last == count)
>>>                        break;
>>>    
>>> +             /* sleep until the next timer interrupt (woken on signal) */
>>> +             pause();
>>
>> Does it cause any (sensible) slowdown?
> 
> Adds at most one timer interval, 10us. Ok, at a push 2 timer intervals
> if it takes longer than first to setup the sleep.
> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/3] lib: Align ring measurement to timer

[Chris Wilson]

Quoting Antonio Argenziano (2018-05-31 15:42:03)
> 
> 
> On 30/05/18 12:52, Chris Wilson wrote:
> > Quoting Antonio Argenziano (2018-05-30 18:30:36)
> >>
> >>
> >> On 30/05/18 03:33, Chris Wilson wrote:
> >>> After hitting the SIGINT from execbuf, wait until the next timer signal
> >>> before trying again. This aligns the start of the ioctl to the timer,
> >>> hopefully maximising the amount of time we have for processing before
> >>> the next signal -- trying to prevent the case where we are scheduled out
> >>> in the middle of processing and so hit the timer signal too early.
> >>>
> >>> References: https://bugs.freedesktop.org/show_bug.cgi?id=106695
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>
> >> Not sure I understand what is the sequence of events, is the problem we
> >> get a signal in the middle of a 'good' execbuf and exit the while loop
> >> prematurely? If so maybe we can also think of making the timer 'VIRTUAL'
> >> so that it would decrement only when the process is executing.
> > 
> > If it's VIRTUAL it'll never fire when we wait for space (as being asleep
> > no user/sys time is consumed).
> > 
> > The only way I can explain 106695 would be with some very strange
> > scheduler behaviour, but even then it requires us to hit a path where we
> > actually check for a pending signal -- which should only happen when we
> > run out of ring space for this setup. Not even the device being wedged
> > (which it wasn't) would cause the ring to drain. Possibly going over 10s
> > and the cork being unplugged? Very stange.
> 
> Just a bit concerned that we might be covering up some weird corner case 
> where we are sleeping where we shouldn't.

The bugs are exactly the opposite, where there's a signal pending and we
ignore it ;)

And ignore them we do. If one day someone cares signal latency, we have
a lot of explaining to do...

It's just worrying because the only signal_pending() check we expect to
hit is in wait-for-space (i915_request_wait to be precise); and that
should be consistent between calls to execbuf. However, it's not meant
to be a defining test of user behaviour, just exploiting the limitation
of the implementation to report said limitation. All that we must do
is to be sure that we don't over-report or else the callers will hang
during their setup and fail. Under reporting is a nuisance, but not a
huge issue.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 3/3] lib: Double check ring measurement

[Chris Wilson]

Check twice for the signal interrupting the execbuf, because the real
world is messy.

References: https://bugs.freedesktop.org/show_bug.cgi?id=106695
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Antonio Argenziano <antonio.argenziano@intel.com>
---
 lib/i915/gem_ring.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/i915/gem_ring.c b/lib/i915/gem_ring.c
index 0c061000c..0708c8a2e 100644
--- a/lib/i915/gem_ring.c
+++ b/lib/i915/gem_ring.c
@@ -55,7 +55,7 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, enum measure_ring_flags
 	struct drm_i915_gem_exec_object2 obj[2];
 	struct drm_i915_gem_execbuffer2 execbuf;
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
-	unsigned int count, last;
+	unsigned int last[2]= { -1, -1 }, count;
 	struct itimerval itv;
 	IGT_CORK_HANDLE(cork);
 
@@ -85,7 +85,6 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, enum measure_ring_flags
 	itv.it_value.tv_usec = 10000;
 	setitimer(ITIMER_REAL, &itv, NULL);
 
-	last = -1;
 	count = 0;
 	do {
 		if (__execbuf(fd, &execbuf) == 0) {
@@ -93,12 +92,13 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, enum measure_ring_flags
 			continue;
 		}
 
-		if (last == count)
+		if (last[1] == count)
 			break;
 
 		/* sleep until the next timer interrupt (woken on signal) */
 		pause();
-		last = count;
+		last[1] = last[0];
+		last[0] = count;
 	} while (1);
 
 	igt_assert_eq(__execbuf(fd, &execbuf), -EINTR);
-- 
2.17.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 3/3] lib: Double check ring measurement

[Antonio Argenziano]

On 30/05/18 03:33, Chris Wilson wrote:
> Check twice for the signal interrupting the execbuf, because the real
> world is messy.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=106695
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Antonio Argenziano <antonio.argenziano@intel.com>

LGTM.

Reviewed-by: Antonio Argenziano <antonio.argenziano@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 3/3] lib: Double check ring measurement

[Chris Wilson]

Quoting Antonio Argenziano (2018-05-30 18:42:28)
> 
> 
> On 30/05/18 03:33, Chris Wilson wrote:
> > Check twice for the signal interrupting the execbuf, because the real
> > world is messy.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=106695
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> 
> LGTM.
> 
> Reviewed-by: Antonio Argenziano <antonio.argenziano@intel.com>

Picked up to sweep the bug under the carpet hopefully never to be heard
off again.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/3] lib: Assert that we do manage to submit at least one batch when measuring

[Antonio Argenziano]

On 30/05/18 03:33, Chris Wilson wrote:
> As we measure the ring size, we never expect to find we can not submit
> no batches at all. Assert against the unexpected.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> ---
>   lib/i915/gem_ring.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/lib/i915/gem_ring.c b/lib/i915/gem_ring.c
> index 10d2f2cd4..7d64165eb 100644
> --- a/lib/i915/gem_ring.c
> +++ b/lib/i915/gem_ring.c
> @@ -100,6 +100,7 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, enum measure_ring_flags
>   	} while (1);
>   
>   	igt_assert_eq(__execbuf(fd, &execbuf), -EINTR);
> +	igt_assert(count);

Maybe a courtesy print?

With or without,
Reviewed-by: Antonio Argenziano <antonio.argenziano@intel.com>

>   
>   	memset(&itv, 0, sizeof(itv));
>   	setitimer(ITIMER_REAL, &itv, NULL);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx