[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

[Intel-gfx] [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

[igt-dev] [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

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[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

[igt-dev] [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

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/3] lib: Assert that we do manage to submit at least one batch when measuring

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/3] lib: Assert that we do manage to submit at least one batch when measuring
URL   : https://patchwork.freedesktop.org/series/43941/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4256 -> IGTPW_1408 =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1408 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1408, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/43941/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_1408:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        SKIP -> PASS

    
== Known issues ==

  Here are the changes found in IGTPW_1408 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-cnl-psr:         PASS -> FAIL (fdo#103481)

    igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
      fi-cfl-8700k:       PASS -> FAIL (fdo#103481)

    
    ==== Possible fixes ====

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       FAIL (fdo#100368) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-bwr-2160:        FAIL (fdo#103182) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103182 https://bugs.freedesktop.org/show_bug.cgi?id=103182
  fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481


== Participating hosts (45 -> 39) ==

  Missing    (6): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-cfl-u2 fi-skl-6700hq 


== Build changes ==

    * IGT: IGT_4500 -> IGTPW_1408

  CI_DRM_4256: 02a9ddab7c0319d16af25f22f9b15b271e6bbc8f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1408: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1408/
  IGT_4500: 43f7a746ac092e41d4745ed190d4268ef1f82c55 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1408/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.IGT: failure for series starting with [i-g-t,1/3] lib: Assert that we do manage to submit at least one batch when measuring

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/3] lib: Assert that we do manage to submit at least one batch when measuring
URL   : https://patchwork.freedesktop.org/series/43941/
State : failure

== Summary ==

= CI Bug Log - changes from IGT_4500_full -> IGTPW_1408_full =

== Summary - FAILURE ==

  Serious unknown changes coming with IGTPW_1408_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1408_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/43941/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_1408_full:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_hangman@error-state-capture-blt:
      shard-snb:          PASS -> FAIL

    
    ==== Warnings ====

    igt@gem_exec_schedule@deep-bsd1:
      shard-kbl:          SKIP -> PASS +1

    igt@gem_exec_schedule@deep-bsd2:
      shard-kbl:          PASS -> SKIP +1

    
== Known issues ==

  Here are the changes found in IGTPW_1408_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_parallel@bsd1:
      shard-snb:          SKIP -> INCOMPLETE (fdo#105411)

    igt@gem_mmap_gtt@coherency:
      shard-glk:          NOTRUN -> FAIL (fdo#100587)

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          PASS -> FAIL (fdo#105454, fdo#106509)

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#102887)

    igt@kms_flip@2x-plain-flip-fb-recreate:
      shard-glk:          PASS -> FAIL (fdo#100368)

    igt@kms_flip_tiling@flip-y-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724, fdo#103822)

    igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw:
      shard-snb:          NOTRUN -> FAIL (fdo#104724, fdo#103167)

    igt@kms_rotation_crc@sprite-rotation-180:
      shard-snb:          PASS -> FAIL (fdo#104724, fdo#103925)

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    igt@perf@polling:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    
    ==== Possible fixes ====

    igt@kms_ccs@pipe-b-missing-ccs-buffer:
      shard-kbl:          DMESG-WARN (fdo#103558, fdo#105602) -> PASS +18

    igt@kms_flip@2x-dpms-vs-vblank-race-interruptible:
      shard-hsw:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@2x-plain-flip-fb-recreate-interruptible:
      shard-glk:          FAIL (fdo#100368) -> PASS +1

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@kms_frontbuffer_tracking@fbc-suspend:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS +1

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS

    igt@perf_pmu@multi-client-bcs0:
      shard-snb:          INCOMPLETE (fdo#105411) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#100587 https://bugs.freedesktop.org/show_bug.cgi?id=100587
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * IGT: IGT_4500 -> IGTPW_1408
    * Linux: CI_DRM_4255 -> CI_DRM_4256

  CI_DRM_4255: 5550ce1b359dcbd8d9387e0501bc372f0c158eaa @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4256: 02a9ddab7c0319d16af25f22f9b15b271e6bbc8f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1408: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1408/
  IGT_4500: 43f7a746ac092e41d4745ed190d4268ef1f82c55 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1408/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

Re: [igt-dev] [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);
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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: [Intel-gfx] [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 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: [igt-dev] [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>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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: [Intel-gfx] [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: [igt-dev] [Intel-gfx] [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
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

Re: [igt-dev] [Intel-gfx] [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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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: [igt-dev] [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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev