tests/gem_exec_fence: Fix test_fence_await for hanging workloads

*Patch Details*
*Series:* tests/gem_exec_fence: Fix test_fence_await for hanging workloads (rev2)
*URL:* https://patchwork.freedesktop.org/series/106531/ <https://patchwork.freedesktop.org/series/106531/>
*State:* failure
*Details:* https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/index.html <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/index.html>

CI Bug Log - changes from CI_DRM_11943 -> IGTPW_7563

Summary

*FAILURE*

Serious unknown changes coming with IGTPW_7563 absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in IGTPW_7563, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/index.html

    Participating hosts (33 -> 38)

Additional (8): fi-kbl-soraka bat-dg2-8 bat-adlm-1 bat-adlp-6 fi-snb-2520m bat-rplp-1 bat-rpls-1 bat-rpls-2 Missing (3): fi-hsw-4770 fi-bdw-samus bat-dg1-5

Possible new issues

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

IGT changes

Possible regressions

* igt@i915_selftest@live@slpc:
o fi-kbl-soraka: NOTRUN -> INCOMPLETE
<https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-kbl-soraka/igt@i915_selftest@[email protected]>

Suppressed

The following results come from untrusted machines, tests, or statuses. They do not affect the overall result.

* igt@i915_selftest@live@migrate:
o {bat-rpls-1}: NOTRUN -> INCOMPLETE
<https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/bat-rpls-1/igt@i915_selftest@[email protected]>

Known issues

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

IGT changes

Issues hit

*

    igt@gem_exec_gttfill@basic:

      o fi-kbl-soraka: NOTRUN -> SKIP
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-kbl-soraka/igt@[email protected]>
        (fdo#109271
        <https://bugs.freedesktop.org/show_bug.cgi?id=109271>) +8
        similar issues

*

    igt@gem_exec_suspend@basic-s3@smem:

      o fi-rkl-11600: NOTRUN -> INCOMPLETE
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-rkl-11600/igt@gem_exec_suspend@[email protected]>
        (i915#6179 <https://gitlab.freedesktop.org/drm/intel/issues/6179>)

*

    igt@gem_huc_copy@huc-copy:

      o fi-kbl-soraka: NOTRUN -> SKIP
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-kbl-soraka/igt@[email protected]>
        (fdo#109271
        <https://bugs.freedesktop.org/show_bug.cgi?id=109271> /
        i915#2190 <https://gitlab.freedesktop.org/drm/intel/issues/2190>)

*

    igt@gem_lmem_swapping@basic:

      o fi-kbl-soraka: NOTRUN -> SKIP
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-kbl-soraka/igt@[email protected]>
        (fdo#109271
        <https://bugs.freedesktop.org/show_bug.cgi?id=109271> /
        i915#4613
        <https://gitlab.freedesktop.org/drm/intel/issues/4613>) +3
        similar issues

*

    igt@i915_selftest@live@execlists:

      o fi-bdw-gvtdvm: PASS
        <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11943/fi-bdw-gvtdvm/igt@i915_selftest@[email protected]>
        -> INCOMPLETE
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-bdw-gvtdvm/igt@i915_selftest@[email protected]>
        (i915#2940 <https://gitlab.freedesktop.org/drm/intel/issues/2940>)

*

    igt@i915_selftest@live@gt_pm:

      o fi-kbl-soraka: NOTRUN -> DMESG-FAIL
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html>
        (i915#1886 <https://gitlab.freedesktop.org/drm/intel/issues/1886>)

*

    igt@kms_chamelium@common-hpd-after-suspend:

      o fi-bdw-5557u: NOTRUN -> SKIP
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-bdw-5557u/igt@[email protected]>
        (fdo#109271
        <https://bugs.freedesktop.org/show_bug.cgi?id=109271> /
        fdo#111827 <https://bugs.freedesktop.org/show_bug.cgi?id=111827>)

*

    igt@kms_chamelium@hdmi-crc-fast:

      o fi-snb-2520m: NOTRUN -> SKIP
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-snb-2520m/igt@[email protected]>
        (fdo#109271
        <https://bugs.freedesktop.org/show_bug.cgi?id=109271> /
        fdo#111827
        <https://bugs.freedesktop.org/show_bug.cgi?id=111827>) +8
        similar issues

*

    igt@kms_chamelium@hdmi-hpd-fast:

      o fi-kbl-soraka: NOTRUN -> SKIP
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-kbl-soraka/igt@[email protected]>
        (fdo#109271
        <https://bugs.freedesktop.org/show_bug.cgi?id=109271> /
        fdo#111827
        <https://bugs.freedesktop.org/show_bug.cgi?id=111827>) +7
        similar issues

*

    igt@prime_vgem@basic-fence-flip:

      o fi-snb-2520m: NOTRUN -> SKIP
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-snb-2520m/igt@[email protected]>
        (fdo#109271
        <https://bugs.freedesktop.org/show_bug.cgi?id=109271>) +21
        similar issues

*

    igt@runner@aborted:

      o fi-bdw-gvtdvm: NOTRUN -> FAIL
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-bdw-gvtdvm/igt@[email protected]>
        (i915#4312 <https://gitlab.freedesktop.org/drm/intel/issues/4312>)


        Possible fixes

*

    igt@i915_selftest@live@coherency:

      o {bat-dg2-9}: DMESG-WARN
        <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11943/bat-dg2-9/igt@i915_selftest@[email protected]>
        (i915#5763
        <https://gitlab.freedesktop.org/drm/intel/issues/5763>) -> PASS
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/bat-dg2-9/igt@i915_selftest@[email protected]>
        +7 similar issues

*

    igt@i915_selftest@live@hangcheck:

      o {fi-ehl-2}: INCOMPLETE
        <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11943/fi-ehl-2/igt@i915_selftest@[email protected]>
        (i915#5153
        <https://gitlab.freedesktop.org/drm/intel/issues/5153> /
        i915#6106
        <https://gitlab.freedesktop.org/drm/intel/issues/6106>) -> PASS
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-ehl-2/igt@i915_selftest@[email protected]>


        Warnings

* igt@i915_suspend@basic-s3-without-i915:

      o fi-rkl-11600: INCOMPLETE
        <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11943/fi-rkl-11600/igt@[email protected]>
        (i915#5982
        <https://gitlab.freedesktop.org/drm/intel/issues/5982>) -> FAIL
        <https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/fi-rkl-11600/igt@[email protected]>
        (fdo#103375 <https://bugs.freedesktop.org/show_bug.cgi?id=103375>)

{name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE).

Build changes

* CI: CI-20190529 -> None
* IGT: IGT_6598 -> IGTPW_7563

CI-20190529: 20190529
CI_DRM_11943: fedf33eeec77c1a0dfb243eacdbce82ca0ffa704 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_7563: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7563/index.html
IGT_6598: 97e103419021d0863db527e3f2cf39ccdd132db5 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

From: Chris Wilson <[email protected]>

test_fence_await verifies if a fence used to pipeline work signals correctly. await-hang and nb-await-hang test cases inject GPU hang, which causes an erroneous state, meaning the fence will be signaled without execution. The error causes an instant reset of the command streamer for the hanging workload. This revealed a problem with how we verify the fence state and results. The test assumes that the error notification happens after a hang is declared, which takes longer than submitting the next set of fences, making the test pass every time. With the immediate reset, this might not happen, so the

assertion fails, as there are no active fences in the GPU hang case.

Move the check for active fence to the path for non-hanging workload, and verify results only in this scenario.

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Karolina Drobnik <[email protected]>

---
 tests/i915/gem_exec_fence.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

tests/i915/gem_exec_fence.c +0 -0

index d46914c2..260aa82c 100644

tests/i915/gem_exec_fence.c +8 -4

-	for (int n = 0; n < i; n++)
-		igt_assert_eq_u32(out[n], 0);
+	if ((flags & HANG) == 0) {
+		/* Check for invalidly completing the task early */
+		igt_assert(fence_busy(spin->out_fence));
+		for (int n = 0; n < i; n++)
+			igt_assert_eq_u32(out[n], 0);
 
-	if ((flags & HANG) == 0)
 		igt_spin_end(spin);
+	}
 
 	igt_waitchildren();
 
 	gem_set_domain(fd, scratch, I915_GEM_DOMAIN_GTT, 0);
-	while (i--)
+	igt_assert(!fence_busy(spin->out_fence));
+	while ((flags & HANG) == 0 && i--)
 		igt_assert_eq_u32(out[i], i);
 	munmap(out, 4096);
 

Signature · 1 line

2.25.1

From: Chris Wilson <[email protected]>

Wait until we know the request is running on the GPU before sleeping and giving a chance for other fences to run ahead.

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Karolina Drobnik <[email protected]>

---
 tests/i915/gem_exec_fence.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

tests/i915/gem_exec_fence.c +4 -1

index 260aa82c..7ff7614d 100644
@@ -324,7 +324,9 @@ static void test_fence_await(int fd, const intel_ctx_t *ctx,
 			    .ahnd = ahnd,
 			    .ctx = ctx,
 			    .engine = e->flags,
-			    .flags = IGT_SPIN_FENCE_OUT | spin_hang(flags));
+			    .flags = IGT_SPIN_FENCE_OUT |
+				     IGT_SPIN_POLL_RUN |
+				     spin_hang(flags));
 	igt_assert(spin->out_fence != -1);
 
 	i = 0;

@@ -347,6 +349,7 @@ static void test_fence_await(int fd, const intel_ctx_t *ctx,
 		i++;
 	}
 
+	igt_spin_busywait_until_started(spin);
 	/* Long, but not too long to anger preemption disable checks */
 	usleep(50 * 1000); /* 50 ms, typical preempt reset is 150+ms */
 

Signature · 1 line

2.25.1

tests/i915/gem_exec_fence.c +0 -0

 		igt_assert_eq_u32(out[i], i);
 	munmap(out, 4096);
 
 

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const intel_ctx_t

Quoted 27 lines · On Tuesday, 26 July 2022 12:13:11 CEST Karolina Drobnik wrote:

From: Chris Wilson <[email protected]>

test_fence_await verifies if a fence used to pipeline work signals correctly. await-hang and nb-await-hang test cases inject GPU hang, which causes an erroneous state, meaning the fence will be signaled without execution. The error causes an instant reset of the command streamer for the hanging workload. This revealed a problem with how we verify the fence state and results. The test assumes that the error notification happens after a hang is declared, which takes longer than submitting the next set of fences, making the test pass every time. With the immediate reset, this might not happen, so the assertion fails, as there are no active fences in the GPU hang case.

Move the check for active fence to the path for non-hanging workload, and verify results only in this scenario.

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Karolina Drobnik <[email protected]>

---
  tests/i915/gem_exec_fence.c | 14 ++++++++------
  1 file changed, 8 insertions(+), 6 deletions(-)

tests/i915/gem_exec_fence.c +0 -0

index d46914c2..260aa82c 100644
@@ -350,18 +350,20 @@ static void test_fence_await(int fd, const intel_ctx_t

*ctx,

Quoted 3 lines

tests/i915/gem_exec_fence.c +0 -0

  	/* Long, but not too long to anger preemption disable checks */
  	usleep(50 * 1000); /* 50 ms, typical preempt reset is 150+ms */
 

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const intel_ctx_t *ctx, · line 357

Quoted 9 lines

tests/i915/gem_exec_fence.c +5 -4

-	/* Check for invalidly completing the task early */
-	igt_assert(fence_busy(spin->out_fence));
-	for (int n = 0; n < i; n++)
-		igt_assert_eq_u32(out[n], 0);
+	if ((flags & HANG) == 0) {
+		/* Check for invalidly completing the task early */
+		igt_assert(fence_busy(spin->out_fence));
+		for (int n = 0; n < i; n++)
+			igt_assert_eq_u32(out[n], 0);

AFAICU, in the 'hang' variant of the scenario we skip the above asserts because the spin batch could have already hanged, then its out fence already signalled and store batches waiting for that signal already executed. If that's the case, how do this variant of gem_exec_fence test asserts that the fence actually worked as expected?

Quoted 5 lines

tests/i915/gem_exec_fence.c +1 -1

-	if ((flags & HANG) == 0)
  		igt_spin_end(spin);
+	}
 

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const intel_ctx_t *ctx, · line 365

Quoted 5 lines

tests/i915/gem_exec_fence.c +1 -1

  	igt_waitchildren();
 
  	gem_set_domain(fd, scratch, I915_GEM_DOMAIN_GTT, 0);
-	while (i--)
+	igt_assert(!fence_busy(spin->out_fence));

We only check that the out fence of the presumably hanged spin batch no longer blocks execution of store batches.

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const intel_ctx_t *ctx, · line 366

Quoted 1 line

tests/i915/gem_exec_fence.c +1 -0

+	while ((flags & HANG) == 0 && i--)

Besides, why don't we at least assert successful results of store batches? Do we expect them not having their job done correctly when completed after the hang of the spin batch have occurred?

Am I missing something?

Thanks,
Janusz

Quoted 4 lines

tests/i915/gem_exec_fence.c +0 -0

  		igt_assert_eq_u32(out[i], i);
  	munmap(out, 4096);
 
 

Hi Janusz,

Thanks a lot for taking a look at the patch.

Quoted 23 lines · Janusz Krzysztofik

Quoted 22 lines · On Tuesday, 26 July 2022 12:13:11 CEST Karolina Drobnik wrote:

From: Chris Wilson <[email protected]>

test_fence_await verifies if a fence used to pipeline work signals correctly. await-hang and nb-await-hang test cases inject GPU hang, which causes an erroneous state, meaning the fence will be signaled without execution. The error causes an instant reset of the command streamer for the hanging workload. This revealed a problem with how we verify the fence state and results. The test assumes that the error notification happens after a hang is declared, which takes longer than submitting the next set of fences, making the test pass every time. With the immediate reset, this might not happen, so the assertion fails, as there are no active fences in the GPU hang case.

Move the check for active fence to the path for non-hanging workload, and verify results only in this scenario.

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Karolina Drobnik <[email protected]>

---
  tests/i915/gem_exec_fence.c | 14 ++++++++------
  1 file changed, 8 insertions(+), 6 deletions(-)

Quoted 5 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const

Quoted 5 lines

tests/i915/gem_exec_fence.c +0 -0

index d46914c2..260aa82c 100644
@@ -350,18 +350,20 @@ static void test_fence_await(int fd, const 

Quoted 4 lines

*ctx,

Quoted 3 lines

tests/i915/gem_exec_fence.c +0 -0

  	/* Long, but not too long to anger preemption disable checks */
  	usleep(50 * 1000); /* 50 ms, typical preempt reset is 150+ms */
 

Quoted 4 lines

Quoted 4 lines

tests/i915/gem_exec_fence.c +0 -4

-	/* Check for invalidly completing the task early */
-	igt_assert(fence_busy(spin->out_fence));
-	for (int n = 0; n < i; n++)
-		igt_assert_eq_u32(out[n], 0);

Quoted 8 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const intel_ctx_t *ctx, · line 357

Quoted 5 lines

tests/i915/gem_exec_fence.c +5 -0

+	if ((flags & HANG) == 0) {
+		/* Check for invalidly completing the task early */
+		igt_assert(fence_busy(spin->out_fence));
+		for (int n = 0; n < i; n++)
+			igt_assert_eq_u32(out[n], 0);

AFAICU, in the 'hang' variant of the scenario we skip the above asserts because the spin batch could have already hanged, then its out fence

Quoted 2 lines

signalled and store batches waiting for that signal already executed. If that's the case, how do this variant of gem_exec_fence test asserts that

Quoted 1 line

fence actually worked as expected?

With this change, yes, we would skip them. Still, the store batches wouldn't be executed, as we reset the CS on hang as a part of the error handling. For valid jobs, we expect to (1) see an active fence at the beginning of the request, (2) get a signaled fence after the wait, (3) store out[i] == i. With a hang, (1) and (3) would be false.

In this particular loop, we could have garbage here with hang, not 0s (although, from my tests it looks like they are zeroed, but maybe I got lucky)

Quoted 6 lines

Quoted 6 lines

tests/i915/gem_exec_fence.c +1 -1

-	if ((flags & HANG) == 0)
  		igt_spin_end(spin);
+	}
 

Quoted 7 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const intel_ctx_t *ctx, · line 365

Quoted 5 lines

tests/i915/gem_exec_fence.c +1 -1

  	igt_waitchildren();
 
  	gem_set_domain(fd, scratch, I915_GEM_DOMAIN_GTT, 0);
-	while (i--)
+	igt_assert(!fence_busy(spin->out_fence));

We only check that the out fence of the presumably hanged spin batch no

Quoted 1 line

blocks execution of store batches.

This check applies to all workloads, all of them should be done with work at this point

Quoted 4 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const intel_ctx_t *ctx, · line 366

Quoted 2 lines

tests/i915/gem_exec_fence.c +1 -0

+	while ((flags & HANG) == 0 && i--)

Besides, why don't we at least assert successful results of store batches?

Quoted 1 line

we expect them not having their job done correctly when completed after

Quoted 1 line

hang of the spin batch have occurred?

We don't expect them to store anything meaningful, because we get a reset. So, this check only applies to well-behaved jobs.

All the best,
Karolina

Quoted 14 lines

Am I missing something?

Thanks,
Janusz

Quoted 4 lines

tests/i915/gem_exec_fence.c +0 -0

  		igt_assert_eq_u32(out[i], i);
  	munmap(out, 4096);
 
 

Hi Karolina,

Quoted 28 lines · On Friday, 29 July 2022 09:38:43 CEST Karolina Drobnik wrote:

Hi Janusz,

Thanks a lot for taking a look at the patch.

Quoted 23 lines · Janusz Krzysztofik

Quoted 22 lines · On Tuesday, 26 July 2022 12:13:11 CEST Karolina Drobnik wrote:

From: Chris Wilson <[email protected]>

test_fence_await verifies if a fence used to pipeline work signals correctly. await-hang and nb-await-hang test cases inject GPU hang, which causes an erroneous state, meaning the fence will be signaled without execution. The error causes an instant reset of the command streamer for the hanging workload. This revealed a problem with how we verify the fence state and results. The test assumes that the error notification happens after a hang is declared, which takes longer than submitting the next set of fences, making the test pass every time. With the immediate reset, this might not happen, so the assertion fails, as there are no active fences in the GPU hang case.

Move the check for active fence to the path for non-hanging workload, and verify results only in this scenario.

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Karolina Drobnik <[email protected]>

---
   tests/i915/gem_exec_fence.c | 14 ++++++++------
   1 file changed, 8 insertions(+), 6 deletions(-)

Quoted 5 lines

Quoted 5 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const

Quoted 5 lines

tests/i915/gem_exec_fence.c +0 -0

index d46914c2..260aa82c 100644
@@ -350,18 +350,20 @@ static void test_fence_await(int fd, const

intel_ctx_t

Quoted 4 lines

Quoted 4 lines

*ctx,

Quoted 3 lines

tests/i915/gem_exec_fence.c +0 -0

   	/* Long, but not too long to anger preemption disable checks */
   	usleep(50 * 1000); /* 50 ms, typical preempt reset is 150+ms */
 

Quoted 12 lines

Quoted 4 lines

Quoted 4 lines

tests/i915/gem_exec_fence.c +0 -4

-	/* Check for invalidly completing the task early */
-	igt_assert(fence_busy(spin->out_fence));
-	for (int n = 0; n < i; n++)
-		igt_assert_eq_u32(out[n], 0);

Quoted 8 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const intel_ctx_t *ctx, · line 357

Quoted 5 lines

tests/i915/gem_exec_fence.c +5 -0

+	if ((flags & HANG) == 0) {
+		/* Check for invalidly completing the task early */
+		igt_assert(fence_busy(spin->out_fence));
+		for (int n = 0; n < i; n++)
+			igt_assert_eq_u32(out[n], 0);

AFAICU, in the 'hang' variant of the scenario we skip the above asserts because the spin batch could have already hanged, then its out fence

already

Quoted 2 lines

Quoted 2 lines

signalled and store batches waiting for that signal already executed. If that's the case, how do this variant of gem_exec_fence test asserts that

the

Quoted 11 lines

Quoted 1 line

fence actually worked as expected?

With this change, yes, we would skip them. Still, the store batches wouldn't be executed, as we reset the CS on hang as a part of the error handling. For valid jobs, we expect to (1) see an active fence at the beginning of the request, (2) get a signaled fence after the wait, (3) store out[i] == i. With a hang, (1) and (3) would be false.

In this particular loop, we could have garbage here with hang, not 0s (although, from my tests it looks like they are zeroed, but maybe I got lucky)

OK, so I missed the fact that the store batches won't be executed at all due to reset of the whole command stream that also kills those batches. But my question is still valid: as soon as we omit those checks as not valid from *await-hang variants, how do those variants still exercise fencing? IOW, how are those variants supposed to ever fail should something be wrong with i915 implementation of fencing specifically?

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

tests/i915/gem_exec_fence.c +1 -1

-	if ((flags & HANG) == 0)
   		igt_spin_end(spin);
+	}
 

Quoted 7 lines

Quoted 7 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const intel_ctx_t *ctx, · line 365

Quoted 5 lines

tests/i915/gem_exec_fence.c +1 -1

   	igt_waitchildren();
 
   	gem_set_domain(fd, scratch, I915_GEM_DOMAIN_GTT, 0);
-	while (i--)
+	igt_assert(!fence_busy(spin->out_fence));

We only check that the out fence of the presumably hanged spin batch no

longer

Quoted 4 lines

Quoted 1 line

blocks execution of store batches.

This check applies to all workloads, all of them should be done with work at this point

OK, but since that's the only explicit assert in the *-hang processing path, does it tell us anything about fencing working or not?

I think it doesn't,
and as long as I'm not wrong, I think such scenarios hardly belong to gem_exec_fence.

Otherwise, I think we should at least add descriptions of those subtests, providing some information on what is actually exercised.

Thanks,
Janusz

Hi Janusz,

Quoted 69 lines · Janusz Krzysztofik

Hi Karolina,

Quoted 28 lines · On Friday, 29 July 2022 09:38:43 CEST Karolina Drobnik wrote:

Hi Janusz,

Thanks a lot for taking a look at the patch.

Quoted 23 lines · Janusz Krzysztofik

Quoted 22 lines · On Tuesday, 26 July 2022 12:13:11 CEST Karolina Drobnik wrote:

From: Chris Wilson <[email protected]>

test_fence_await verifies if a fence used to pipeline work signals correctly. await-hang and nb-await-hang test cases inject GPU hang, which causes an erroneous state, meaning the fence will be signaled without execution. The error causes an instant reset of the command streamer for the hanging workload. This revealed a problem with how we verify the fence state and results. The test assumes that the error notification happens after a hang is declared, which takes longer than submitting the next set of fences, making the test pass every time. With the immediate reset, this might not happen, so the assertion fails, as there are no active fences in the GPU hang case.

Move the check for active fence to the path for non-hanging workload, and verify results only in this scenario.

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Karolina Drobnik <[email protected]>

---
   tests/i915/gem_exec_fence.c | 14 ++++++++------
   1 file changed, 8 insertions(+), 6 deletions(-)

Quoted 5 lines

Quoted 5 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const

Quoted 5 lines

tests/i915/gem_exec_fence.c +0 -0

index d46914c2..260aa82c 100644
@@ -350,18 +350,20 @@ static void test_fence_await(int fd, const

intel_ctx_t

Quoted 4 lines

Quoted 4 lines

*ctx,

Quoted 3 lines

tests/i915/gem_exec_fence.c +0 -0

   	/* Long, but not too long to anger preemption disable checks */
   	usleep(50 * 1000); /* 50 ms, typical preempt reset is 150+ms */
 

Quoted 12 lines

Quoted 4 lines

Quoted 4 lines

tests/i915/gem_exec_fence.c +0 -4

-	/* Check for invalidly completing the task early */
-	igt_assert(fence_busy(spin->out_fence));
-	for (int n = 0; n < i; n++)
-		igt_assert_eq_u32(out[n], 0);

Quoted 8 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const intel_ctx_t *ctx, · line 357

Quoted 5 lines

tests/i915/gem_exec_fence.c +5 -0

+	if ((flags & HANG) == 0) {
+		/* Check for invalidly completing the task early */
+		igt_assert(fence_busy(spin->out_fence));
+		for (int n = 0; n < i; n++)
+			igt_assert_eq_u32(out[n], 0);

AFAICU, in the 'hang' variant of the scenario we skip the above asserts because the spin batch could have already hanged, then its out fence

already

Quoted 2 lines

Quoted 2 lines

signalled and store batches waiting for that signal already executed. If that's the case, how do this variant of gem_exec_fence test asserts that

the

Quoted 11 lines

Quoted 1 line

fence actually worked as expected?

With this change, yes, we would skip them. Still, the store batches wouldn't be executed, as we reset the CS on hang as a part of the error handling. For valid jobs, we expect to (1) see an active fence at the beginning of the request, (2) get a signaled fence after the wait, (3) store out[i] == i. With a hang, (1) and (3) would be false.

In this particular loop, we could have garbage here with hang, not 0s (although, from my tests it looks like they are zeroed, but maybe I got lucky)

Quoted 6 lines

OK, so I missed the fact that the store batches won't be executed at all due to reset of the whole command stream that also kills those batches. But my question is still valid: as soon as we omit those checks as not valid from *await-hang variants, how do those variants still exercise fencing? IOW, how are those variants supposed to ever fail should something be wrong with i915 implementation of fencing specifically?

They would fail in the case where a hang happened but the fence is still active, so it's this last assert you're referring to.

Quoted 13 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

tests/i915/gem_exec_fence.c +1 -1

-	if ((flags & HANG) == 0)
   		igt_spin_end(spin);
+	}
 

Quoted 7 lines

Quoted 7 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const intel_ctx_t *ctx, · line 365

Quoted 5 lines

tests/i915/gem_exec_fence.c +1 -1

   	igt_waitchildren();
 
   	gem_set_domain(fd, scratch, I915_GEM_DOMAIN_GTT, 0);
-	while (i--)
+	igt_assert(!fence_busy(spin->out_fence));

We only check that the out fence of the presumably hanged spin batch no

Quoted 8 lines

longer

Quoted 4 lines

Quoted 1 line

blocks execution of store batches.

This check applies to all workloads, all of them should be done with work at this point

OK, but since that's the only explicit assert in the *-hang processing path, does it tell us anything about fencing working or not?

It says that we were given an active fence, we wait at it and hope it signals when an error is reported. Like I said, we can't check the results itself, as they are meaningless with the reset. If we have an active fence at this point, that's bad, and the test should fail.

Quoted 3 lines

I think it doesn't,
and as long as I'm not wrong, I think such scenarios hardly belong to gem_exec_fence.

Hm, I'm not sure if I follow, but this exact transition (from active -> (record error) -> signal) is one of the possible scenarios we wish to test.

Or, do you mean that this test case doesn't test drm_i915_gem_exec_fence? This test suite exercises different scenarios of using fences implemented with sync_files. Maybe this could be split up, but these seem to be connected, so they ended up in one file.

Quoted 2 lines

Otherwise, I think we should at least add descriptions of those subtests, providing some information on what is actually exercised.

The hang cases reuse test_fence_await which has _some_ description in the basic cases. But I agree, it would be nice to have more documentation for other subtests, but it's out of scope of this fix.

Many thanks,
Karolina

Quoted 3 lines

Thanks,
Janusz

Hi,

Quoted 109 lines · Janusz Krzysztofik

Quoted 32 lines · On Monday, 1 August 2022 07:53:54 CEST Karolina Drobnik wrote:

Hi,

Quoted 29 lines · Janusz Krzysztofik

Quoted 28 lines · On Friday, 29 July 2022 13:32:37 CEST Karolina Drobnik wrote:

Hi Janusz,

Quoted 25 lines · Janusz Krzysztofik

Hi Karolina,

Quoted 22 lines · On Friday, 29 July 2022 09:38:43 CEST Karolina Drobnik wrote:

Hi Janusz,

Thanks a lot for taking a look at the patch.

Quoted 17 lines · Janusz Krzysztofik

Quoted 16 lines · On Tuesday, 26 July 2022 12:13:11 CEST Karolina Drobnik wrote:

From: Chris Wilson <[email protected]>

test_fence_await verifies if a fence used to pipeline work signals correctly. await-hang and nb-await-hang test cases inject GPU hang, which causes an erroneous state, meaning the fence will be signaled without execution. The error causes an instant reset of the command streamer for the hanging workload. This revealed a problem with how we verify the fence state and results. The test assumes that the error notification happens after a hang is declared, which takes longer than submitting the next set of fences, making the test pass every time. With the immediate reset, this might not happen, so the assertion fails, as there are no active fences in the GPU hang case.

Move the check for active fence to the path for non-hanging workload, and verify results only in this scenario.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

tests/i915/

Quoted 7 lines

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Karolina Drobnik <[email protected]>

---
     tests/i915/gem_exec_fence.c | 14 ++++++++------
     1 file changed, 8 insertions(+), 6 deletions(-)

tests/i915/ renamed from tests/i915/gem_exec_fence.c +0 -0

gem_exec_fence.c

Quoted 12 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const

Quoted 4 lines

index d46914c2..260aa82c 100644

tests/i915/gem_exec_fence.c +0 -0

@@ -350,18 +350,20 @@ static void test_fence_await(int fd, const

intel_ctx_t

Quoted 1 line

Quoted 1 line

*ctx,

(...)

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

tests/i915/gem_exec_fence.c +1 -1

-	if ((flags & HANG) == 0)
     		igt_spin_end(spin);
+	}

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

tests/i915/gem_exec_fence.c

Quoted 5 lines

     	igt_waitchildren();
     
     	gem_set_domain(fd, scratch, I915_GEM_DOMAIN_GTT, 0);

tests/i915/gem_exec_fence.c +1 -1

-	while (i--)
+	igt_assert(!fence_busy(spin->out_fence));

We only check that the out fence of the presumably hanged spin batch

no

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

longer

Quoted 4 lines

Quoted 1 line

blocks execution of store batches.

This check applies to all workloads, all of them should be done with work at this point

OK, but since that's the only explicit assert in the *-hang processing

path,

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 1 line

does it tell us anything about fencing working or not?

It says that we were given an active fence, we wait at it and hope it signals when an error is reported. Like I said, we can't check the results itself, as they are meaningless with the reset. If we have an active fence at this point, that's bad, and the test should fail.

Quoted 3 lines

I think it doesn't,
and as long as I'm not wrong, I think such scenarios hardly belong to gem_exec_fence.

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Hm, I'm not sure if I follow, but this exact transition (from active -> (record error) -> signal) is one of the possible scenarios we wish to test.

OK, so we check if an active fence is signalled on error. But then, what

does

Quoted 8 lines

Quoted 1 line

'active' mean here?

Active means that the fence was sent and we wait to hear back from the GPU. If you'd like to see what are the state transitions in fences, you can check out Mainline Explicit Fencing series of blog posts/presentation.

Quoted 2 lines

Do we consider a fence active as soon as it has been exported to userspace? Or only after it has been imported back from

userspace

Quoted 7 lines

Quoted 1 line

by at least one consumer?

We assume them to be active as soon as the fence's fd is returned from the driver to userspace (we deal with out-fences here).

Quoted 2 lines

Assuming the former (as I guess), what do we need
the store batches for in these now modified *await-hang scenarios? What

extra

Quoted 5 lines

Quoted 1 line

value do those scenarios provide compared to (nb-)?wait-hang ?

Hm, I don't quite understand the question here -- are you asking why do we check the store results? test_fence_await is reused by other subtests, not only the hanging cases, and we expect to see the stores.

Quoted 4 lines

The patch introduces two processing paths to test_fence_await(), one of them for *-hang variants. In that *-hang processing path, why do we still submit batches that depend on the fence if we don't examine their execution and results, only their completion?

Ah, OK, now I get it. Do you suggest that we should also add a check for HANG flag for igt_store_word block? But we still need to send something to test this case.

Quoted 3 lines

They get completed regardless of fencing working or not, I believe, > then that part of the *-hang processing path related to store batches seems useless for me in a test focused on fencing.

Do they get completed? We'll get a CS reset and this batch won't complete.

Yes, in the case of hangs we just check the fence,

but in
general we do care about these stores. This patch aims to fix the problem uncovered during manual testing (*-hang variants are not run in CI), it does not try to rewrite the test (because that's what would be required to completely separate these two test cases).

Many thanks,
Karolina

Quoted 14 lines

Thanks,
Janusz

Quoted 7 lines

Many thanks,
Karolina

Quoted 2 lines

Thanks,
Janusz

Quoted 117 lines · On Monday, 1 August 2022 11:51:27 CEST Karolina Drobnik wrote:

Hi,

Quoted 113 lines · Janusz Krzysztofik

Quoted 32 lines · On Monday, 1 August 2022 07:53:54 CEST Karolina Drobnik wrote:

Hi,

Quoted 29 lines · Janusz Krzysztofik

Quoted 28 lines · On Friday, 29 July 2022 13:32:37 CEST Karolina Drobnik wrote:

Hi Janusz,

Quoted 25 lines · Janusz Krzysztofik

Hi Karolina,

Quoted 22 lines · On Friday, 29 July 2022 09:38:43 CEST Karolina Drobnik wrote:

Hi Janusz,

Thanks a lot for taking a look at the patch.

Quoted 17 lines · Janusz Krzysztofik

Quoted 16 lines · On Tuesday, 26 July 2022 12:13:11 CEST Karolina Drobnik wrote:

From: Chris Wilson <[email protected]>

test_fence_await verifies if a fence used to pipeline work signals correctly. await-hang and nb-await-hang test cases inject GPU hang, which causes an erroneous state, meaning the fence will be signaled without execution. The error causes an instant reset of the command streamer for the hanging workload. This revealed a problem with how we verify the fence state and results. The test assumes that the error notification happens after a hang is declared, which takes longer than submitting the next set of fences, making the test pass every time. With the immediate reset, this might not happen, so the assertion fails, as there are no active fences in the GPU hang case.

Move the check for active fence to the path for non-hanging workload, and verify results only in this scenario.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

tests/i915/

Quoted 7 lines

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Karolina Drobnik <[email protected]>

---
      tests/i915/gem_exec_fence.c | 14 ++++++++------
      1 file changed, 8 insertions(+), 6 deletions(-)

tests/i915/ renamed from tests/i915/gem_exec_fence.c +0 -0

gem_exec_fence.c

Quoted 12 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const

Quoted 4 lines

index d46914c2..260aa82c 100644

tests/i915/gem_exec_fence.c +0 -0

@@ -350,18 +350,20 @@ static void test_fence_await(int fd, const

intel_ctx_t

Quoted 1 line

Quoted 1 line

*ctx,

(...)

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

tests/i915/gem_exec_fence.c +1 -1

-	if ((flags & HANG) == 0)
      		igt_spin_end(spin);
+	}

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

tests/i915/gem_exec_fence.c

Quoted 5 lines

      	igt_waitchildren();
      
      	gem_set_domain(fd, scratch, I915_GEM_DOMAIN_GTT, 0);

tests/i915/gem_exec_fence.c +1 -1

-	while (i--)
+	igt_assert(!fence_busy(spin->out_fence));

We only check that the out fence of the presumably hanged spin batch

no

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

longer

Quoted 4 lines

Quoted 1 line

blocks execution of store batches.

This check applies to all workloads, all of them should be done with work at this point

OK, but since that's the only explicit assert in the *-hang processing

path,

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 1 line

does it tell us anything about fencing working or not?

It says that we were given an active fence, we wait at it and hope it signals when an error is reported. Like I said, we can't check the results itself, as they are meaningless with the reset. If we have an active fence at this point, that's bad, and the test should fail.

Quoted 3 lines

I think it doesn't,
and as long as I'm not wrong, I think such scenarios hardly belong to gem_exec_fence.

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Hm, I'm not sure if I follow, but this exact transition (from active -> (record error) -> signal) is one of the possible scenarios we wish to test.

OK, so we check if an active fence is signalled on error. But then, what

does

Quoted 8 lines

Quoted 1 line

'active' mean here?

Active means that the fence was sent and we wait to hear back from the GPU. If you'd like to see what are the state transitions in fences, you can check out Mainline Explicit Fencing series of blog posts/presentation.

Quoted 2 lines

Do we consider a fence active as soon as it has been exported to userspace? Or only after it has been imported back from

userspace

Quoted 7 lines

Quoted 1 line

by at least one consumer?

We assume them to be active as soon as the fence's fd is returned from the driver to userspace (we deal with out-fences here).

Quoted 2 lines

Assuming the former (as I guess), what do we need
the store batches for in these now modified *await-hang scenarios? What

extra

Quoted 5 lines

Quoted 1 line

value do those scenarios provide compared to (nb-)?wait-hang ?

Hm, I don't quite understand the question here -- are you asking why do we check the store results? test_fence_await is reused by other subtests, not only the hanging cases, and we expect to see the stores.

The patch introduces two processing paths to test_fence_await(), one of them for *-hang variants. In that *-hang processing path, why do we still submit batches that depend on the fence if we don't examine their execution and results, only their completion?

Quoted 3 lines

Ah, OK, now I get it. Do you suggest that we should also add a check for HANG flag for igt_store_word block? But we still need to send something to test this case.

Why do we need to send something? What that something should look like? IOW, what are we trying to exercise with this case? How is it supposed to be different from wait-hang scenario?

Quoted 6 lines

Quoted 3 lines

They get completed regardless of fencing working or not, I believe, > then that part of the *-hang processing path related to store batches seems useless for me in a test focused on fencing.

Do they get completed? We'll get a CS reset and this batch won't complete.

OK, by completed I meant no longer queued nor running.

Quoted 1 line

Yes, in the case of hangs we just check the fence,

Again, how this is supposed to be different from wait-hang scenario?

Thanks,
Janusz

Quoted 24 lines

but in
general we do care about these stores. This patch aims to fix the problem uncovered during manual testing (*-hang variants are not run in CI), it does not try to rewrite the test (because that's what would be required to completely separate these two test cases).

Many thanks,
Karolina

Quoted 14 lines

Thanks,
Janusz

Quoted 7 lines

Many thanks,
Karolina

Quoted 2 lines

Thanks,
Janusz

Hi,

Quoted 122 lines · Janusz Krzysztofik

Quoted 120 lines · On Monday, 1 August 2022 11:51:27 CEST Karolina Drobnik wrote:

Hi,

Quoted 105 lines · Janusz Krzysztofik

Quoted 32 lines · On Monday, 1 August 2022 07:53:54 CEST Karolina Drobnik wrote:

Hi,

Quoted 29 lines · Janusz Krzysztofik

Quoted 28 lines · On Friday, 29 July 2022 13:32:37 CEST Karolina Drobnik wrote:

Hi Janusz,

Quoted 25 lines · Janusz Krzysztofik

Hi Karolina,

Quoted 22 lines · On Friday, 29 July 2022 09:38:43 CEST Karolina Drobnik wrote:

Hi Janusz,

Thanks a lot for taking a look at the patch.

Quoted 17 lines · Janusz Krzysztofik

Quoted 16 lines · On Tuesday, 26 July 2022 12:13:11 CEST Karolina Drobnik wrote:

From: Chris Wilson <[email protected]>

test_fence_await verifies if a fence used to pipeline work signals correctly. await-hang and nb-await-hang test cases inject GPU hang, which causes an erroneous state, meaning the fence will be signaled without execution. The error causes an instant reset of the command streamer for the hanging workload. This revealed a problem with how we verify the fence state and results. The test assumes that the error notification happens after a hang is declared, which takes longer than submitting the next set of fences, making the test pass every time. With the immediate reset, this might not happen, so the assertion fails, as there are no active fences in the GPU hang case.

Move the check for active fence to the path for non-hanging workload, and verify results only in this scenario.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

tests/i915/

Quoted 7 lines

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Karolina Drobnik <[email protected]>

---
      tests/i915/gem_exec_fence.c | 14 ++++++++------
      1 file changed, 8 insertions(+), 6 deletions(-)

tests/i915/ renamed from tests/i915/gem_exec_fence.c +0 -0

gem_exec_fence.c

Quoted 12 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const

Quoted 4 lines

index d46914c2..260aa82c 100644

tests/i915/gem_exec_fence.c +0 -0

@@ -350,18 +350,20 @@ static void test_fence_await(int fd, const

intel_ctx_t

Quoted 1 line

Quoted 1 line

*ctx,

(...)

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

tests/i915/gem_exec_fence.c +1 -1

-	if ((flags & HANG) == 0)
      		igt_spin_end(spin);
+	}

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

tests/i915/gem_exec_fence.c

Quoted 5 lines

      	igt_waitchildren();
      
      	gem_set_domain(fd, scratch, I915_GEM_DOMAIN_GTT, 0);

tests/i915/gem_exec_fence.c +1 -1

-	while (i--)
+	igt_assert(!fence_busy(spin->out_fence));

We only check that the out fence of the presumably hanged spin batch

no

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

longer

Quoted 4 lines

Quoted 1 line

blocks execution of store batches.

This check applies to all workloads, all of them should be done with work at this point

OK, but since that's the only explicit assert in the *-hang processing

path,

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 1 line

does it tell us anything about fencing working or not?

It says that we were given an active fence, we wait at it and hope it signals when an error is reported. Like I said, we can't check the results itself, as they are meaningless with the reset. If we have an active fence at this point, that's bad, and the test should fail.

Quoted 3 lines

I think it doesn't,
and as long as I'm not wrong, I think such scenarios hardly belong to gem_exec_fence.

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Hm, I'm not sure if I follow, but this exact transition (from active -> (record error) -> signal) is one of the possible scenarios we wish to test.

OK, so we check if an active fence is signalled on error. But then, what

does

Quoted 8 lines

Quoted 1 line

'active' mean here?

Active means that the fence was sent and we wait to hear back from the GPU. If you'd like to see what are the state transitions in fences, you can check out Mainline Explicit Fencing series of blog posts/presentation.

Quoted 2 lines

Do we consider a fence active as soon as it has been exported to userspace? Or only after it has been imported back from

userspace

Quoted 7 lines

Quoted 1 line

by at least one consumer?

We assume them to be active as soon as the fence's fd is returned from the driver to userspace (we deal with out-fences here).

Quoted 2 lines

Assuming the former (as I guess), what do we need
the store batches for in these now modified *await-hang scenarios? What

extra

Quoted 2 lines

Quoted 1 line

value do those scenarios provide compared to (nb-)?wait-hang ?

Quoted 8 lines

Quoted 3 lines

Hm, I don't quite understand the question here -- are you asking why do we check the store results? test_fence_await is reused by other subtests, not only the hanging cases, and we expect to see the stores.

The patch introduces two processing paths to test_fence_await(), one of them for *-hang variants. In that *-hang processing path, why do we still submit batches that depend on the fence if we don't examine their execution and results, only their completion?

Ah, OK, now I get it. Do you suggest that we should also add a check for HANG flag for igt_store_word block? But we still need to send something to test this case.

Quoted 3 lines

Why do we need to send something? What that something should look like? IOW, what are we trying to exercise with this case? How is it supposed to be different from wait-hang scenario?

test_fence_await tests pipelining work (here, writing something to memory in parallel) and cross-checks its status with what's reported by the fence,

whereas test_fence_busy checks if the fence signals to userspace after hang (a more general case). I'm sorry, I didn't make this distinction clear in the beginning.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

They get completed regardless of fencing working or not, I believe, > then that part of the *-hang processing path related to store batches seems useless for me in a test focused on fencing.

Do they get completed? We'll get a CS reset and this batch won't complete.

OK, by completed I meant no longer queued nor running.

OK, I see -- yes, that's correct

Thanks,
Karolina

Quoted 35 lines

Quoted 1 line

Yes, in the case of hangs we just check the fence,

Again, how this is supposed to be different from wait-hang scenario?

Thanks,
Janusz

Quoted 24 lines

but in
general we do care about these stores. This patch aims to fix the problem uncovered during manual testing (*-hang variants are not run in CI), it does not try to rewrite the test (because that's what would be required to completely separate these two test cases).

Many thanks,
Karolina

Quoted 14 lines

Thanks,
Janusz

Quoted 7 lines

Many thanks,
Karolina

Quoted 2 lines

Thanks,
Janusz

Quoted 129 lines · On Monday, 1 August 2022 15:39:36 CEST Karolina Drobnik wrote:

Hi,

Quoted 125 lines · Janusz Krzysztofik

Quoted 112 lines · On Monday, 1 August 2022 11:51:27 CEST Karolina Drobnik wrote:

Hi,

Quoted 109 lines · Janusz Krzysztofik

Quoted 32 lines · On Monday, 1 August 2022 07:53:54 CEST Karolina Drobnik wrote:

Hi,

Quoted 29 lines · Janusz Krzysztofik

Quoted 28 lines · On Friday, 29 July 2022 13:32:37 CEST Karolina Drobnik wrote:

Hi Janusz,

Quoted 25 lines · Janusz Krzysztofik

Hi Karolina,

Quoted 22 lines · On Friday, 29 July 2022 09:38:43 CEST Karolina Drobnik wrote:

Hi Janusz,

Thanks a lot for taking a look at the patch.

Quoted 17 lines · Janusz Krzysztofik

Quoted 16 lines · On Tuesday, 26 July 2022 12:13:11 CEST Karolina Drobnik wrote:

From: Chris Wilson <[email protected]>

test_fence_await verifies if a fence used to pipeline work signals correctly. await-hang and nb-await-hang test cases inject GPU hang, which causes an erroneous state, meaning the fence will be signaled without execution. The error causes an instant reset of the command streamer for the hanging workload. This revealed a problem with how we verify the fence state and results. The test assumes that the error notification happens after a hang is declared, which takes longer than submitting the next set of fences, making the test pass every time. With the immediate reset, this might not happen, so the assertion fails, as there are no active fences in the GPU hang case.

Move the check for active fence to the path for non-hanging workload, and verify results only in this scenario.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

tests/i915/

Quoted 7 lines

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Karolina Drobnik <[email protected]>

---
       tests/i915/gem_exec_fence.c | 14 ++++++++------
       1 file changed, 8 insertions(+), 6 deletions(-)

tests/i915/ renamed from tests/i915/gem_exec_fence.c +0 -0

gem_exec_fence.c

Quoted 12 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

tests/i915/gem_exec_fence.c static void test_fence_await(int fd, const

Quoted 4 lines

index d46914c2..260aa82c 100644

tests/i915/gem_exec_fence.c +0 -0

@@ -350,18 +350,20 @@ static void test_fence_await(int fd, const

intel_ctx_t

Quoted 1 line

Quoted 1 line

*ctx,

(...)

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

tests/i915/gem_exec_fence.c +1 -1

-	if ((flags & HANG) == 0)
       		igt_spin_end(spin);
+	}

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

tests/i915/gem_exec_fence.c

Quoted 5 lines

       	igt_waitchildren();
       
       	gem_set_domain(fd, scratch, I915_GEM_DOMAIN_GTT, 0);

tests/i915/gem_exec_fence.c +1 -1

-	while (i--)
+	igt_assert(!fence_busy(spin->out_fence));

We only check that the out fence of the presumably hanged spin batch

no

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

longer

Quoted 4 lines

Quoted 1 line

blocks execution of store batches.

This check applies to all workloads, all of them should be done with work at this point

OK, but since that's the only explicit assert in the *-hang processing

path,

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 1 line

does it tell us anything about fencing working or not?

It says that we were given an active fence, we wait at it and hope it signals when an error is reported. Like I said, we can't check the results itself, as they are meaningless with the reset. If we have an active fence at this point, that's bad, and the test should fail.

Quoted 3 lines

I think it doesn't,
and as long as I'm not wrong, I think such scenarios hardly belong to gem_exec_fence.

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Hm, I'm not sure if I follow, but this exact transition (from active -> (record error) -> signal) is one of the possible scenarios we wish to test.

OK, so we check if an active fence is signalled on error. But then, what

does

Quoted 8 lines

Quoted 1 line

'active' mean here?

Active means that the fence was sent and we wait to hear back from the GPU. If you'd like to see what are the state transitions in fences, you can check out Mainline Explicit Fencing series of blog posts/presentation.

Quoted 2 lines

Do we consider a fence active as soon as it has been exported to userspace? Or only after it has been imported back from

userspace

Quoted 7 lines

Quoted 1 line

by at least one consumer?

We assume them to be active as soon as the fence's fd is returned from the driver to userspace (we deal with out-fences here).

Quoted 2 lines

Assuming the former (as I guess), what do we need
the store batches for in these now modified *await-hang scenarios? What

extra

Quoted 5 lines

Quoted 1 line

value do those scenarios provide compared to (nb-)?wait-hang ?

Hm, I don't quite understand the question here -- are you asking why do we check the store results? test_fence_await is reused by other subtests, not only the hanging cases, and we expect to see the stores.

Quoted 8 lines

Quoted 4 lines

The patch introduces two processing paths to test_fence_await(), one of them for *-hang variants. In that *-hang processing path, why do we still submit batches that depend on the fence if we don't examine their execution and results, only their completion?

Ah, OK, now I get it. Do you suggest that we should also add a check for HANG flag for igt_store_word block? But we still need to send something to test this case.

Why do we need to send something? What that something should look like? IOW, what are we trying to exercise with this case? How is it supposed to be different from wait-hang scenario?

Quoted 3 lines

test_fence_await tests pipelining work (here, writing something to memory in parallel) and cross-checks its status with what's reported by the fence,

True for *await (no HANG) subtests, but in *await-hang variants I still can't recognize checks in place as cross-checks and what value they add compared to wait-hang.

Does a check for invalidated work no longer queued nor running tell us anything about the fence behavior, i.e., if and how it affected / interacted with that work? I think it doesn't, then, I'm not sure if we still need such limited scenarios.

Please convince me that we do and I'll be happy
to push the series with my S-o-b: added.

Thanks,
Janusz

Hi,

Quoted 28 lines · Janusz Krzysztofik

Quoted 27 lines · On Monday, 1 August 2022 15:39:36 CEST Karolina Drobnik wrote:

Hi,

Quoted 24 lines · Janusz Krzysztofik

Quoted 23 lines · On Monday, 1 August 2022 11:51:27 CEST Karolina Drobnik wrote:

Hi,

Quoted 20 lines · Janusz Krzysztofik

Quoted 19 lines · On Monday, 1 August 2022 07:53:54 CEST Karolina Drobnik wrote:

Hi,

Quoted 16 lines · Janusz Krzysztofik

Quoted 15 lines · On Friday, 29 July 2022 13:32:37 CEST Karolina Drobnik wrote:

Hi Janusz,

Quoted 12 lines · Janusz Krzysztofik

Hi Karolina,

Quoted 9 lines · On Friday, 29 July 2022 09:38:43 CEST Karolina Drobnik wrote:

Hi Janusz,

Thanks a lot for taking a look at the patch.

Quoted 4 lines · Janusz Krzysztofik

Quoted 3 lines · On Tuesday, 26 July 2022 12:13:11 CEST Karolina Drobnik wrote:

From: Chris Wilson <[email protected]>

test_fence_await verifies if a fence used to pipeline work

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

correctly. await-hang and nb-await-hang test cases inject GPU

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

which causes an erroneous state, meaning the fence will be

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

without execution. The error causes an instant reset of the

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

streamer for the hanging workload. This revealed a problem with

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

we verify the fence state and results. The test assumes that

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

error notification happens after a hang is declared, which

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

longer than submitting the next set of fences, making the test

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

every time. With the immediate reset, this might not happen, so

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

assertion fails, as there are no active fences in the GPU hang

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Move the check for active fence to the path for non-hanging

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

and verify results only in this scenario.

Signed-off-by: Chris Wilson <[email protected]>
Signed-off-by: Karolina Drobnik <[email protected]>
---
tests/i915/gem_exec_fence.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

diff --git a/tests/i915/gem_exec_fence.c b/tests/i915/

gem_exec_fence.c

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

index d46914c2..260aa82c 100644
--- a/tests/i915/gem_exec_fence.c
+++ b/tests/i915/gem_exec_fence.c
@@ -350,18 +350,20 @@ static void test_fence_await(int fd,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

intel_ctx_t

Quoted 1 line

Quoted 1 line

*ctx,

(...)

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

- if ((flags & HANG) == 0)
igt_spin_end(spin);
+ }

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

igt_waitchildren();

gem_set_domain(fd, scratch, I915_GEM_DOMAIN_GTT, 0);
- while (i--)
+ igt_assert(!fence_busy(spin->out_fence));

We only check that the out fence of the presumably hanged spin

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

no

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

longer

Quoted 3 lines

Quoted 1 line

blocks execution of store batches.

This check applies to all workloads, all of them should be done

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

work at this point

OK, but since that's the only explicit assert in the *-hang

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

path,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

does it tell us anything about fencing working or not?

It says that we were given an active fence, we wait at it and hope

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

signals when an error is reported. Like I said, we can't check the results itself, as they are meaningless with the reset. If we have

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

active fence at this point, that's bad, and the test should fail.

Quoted 2 lines

I think it doesn't,
and as long as I'm not wrong, I think such scenarios hardly belong

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

gem_exec_fence.

Hm, I'm not sure if I follow, but this exact transition (from

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

(record error) -> signal) is one of the possible scenarios we wish

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

test.

OK, so we check if an active fence is signalled on error. But then,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

does

Quoted 3 lines

Quoted 1 line

'active' mean here?

Active means that the fence was sent and we wait to hear back from

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

GPU. If you'd like to see what are the state transitions in fences,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

can check out Mainline Explicit Fencing series of blog posts/

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Do we consider a fence active as soon as it has been exported to userspace? Or only after it has been imported back from

userspace

Quoted 3 lines

Quoted 1 line

by at least one consumer?

We assume them to be active as soon as the fence's fd is returned

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

the driver to userspace (we deal with out-fences here).

Quoted 2 lines

Assuming the former (as I guess), what do we need
the store batches for in these now modified *await-hang scenarios?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

extra

Quoted 3 lines

Quoted 1 line

value do those scenarios provide compared to (nb-)?wait-hang ?

Hm, I don't quite understand the question here -- are you asking why

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

we check the store results? test_fence_await is reused by other subtests, not only the hanging cases, and we expect to see the

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

The patch introduces two processing paths to test_fence_await(), one

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

for *-hang variants. In that *-hang processing path, why do we still

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

batches that depend on the fence if we don't examine their execution

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

results, only their completion?

Ah, OK, now I get it. Do you suggest that we should also add a check

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

HANG flag for igt_store_word block? But we still need to send something to test this case.

Why do we need to send something? What that something should look like?

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

what are we trying to exercise with this case? How is it supposed to be different from wait-hang scenario?

test_fence_await tests pipelining work (here, writing something to memory in parallel) and cross-checks its status with what's reported by the fence,

True for *await (no HANG) subtests, but in *await-hang variants I still

Quoted 1 line

recognize checks in place as cross-checks and what value they add compared

Quoted 1 line

wait-hang.

wait_hang and await_hang are different in the sense of work they do. test_fence_busy is a test with a simple batch buffer (with no actual writes) that is _iteratively_ executed on different physical engines, whereas test_fence_await spawns work on different engines in _parallel_. It's a different scenario from the fence's perspective, as we have one fence on the spinner and pass it to other engines which (asynchronously) wait on it. The value added here is the check for 1 fence - many engines (wait_hang has 1-1 mapping).

Quoted 3 lines

Does a check for invalidated work no longer queued nor running tell us anything about the fence behavior, i.e., if and how it affected / interacted with that work? I think it doesn't, then, I'm not sure if we

Quoted 1 line

need such limited scenarios.

I don't think I quite understand -- we check if we've sent a hanging workload, which is expected to result in an error, there's no additional check for the work itself (how one could check it?). As the error is raised from any of the engines (I'm taking about the await case), the fence is expected to signal, and this is what we check. The fence itself can't interact with that work

Quoted 2 lines

Please convince me that we do and I'll be happy
to push the series with my S-o-b: added.

I have commit rights now, so I can push it myself,

but I want to address
all the comments before moving on.

If you have further questions or my answers are not satisfactory (which is fair), you can reach out to the author. He knows all the bits and pieces of this test suite. I'm more of a messenger here.

All the best,
Karolina

Quoted 2 lines

Thanks,
Janusz

Quoted 4 lines · On Tuesday, 2 August 2022 12:20:29 CEST Karolina Drobnik wrote:

Hi,

Quoted 1 line · Janusz Krzysztofik

On Monday, 1 August 2022 15:39:36 CEST Karolina Drobnik wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

The patch introduces two processing paths to test_fence_await(), one

of them

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

for *-hang variants. In that *-hang processing path, why do we still

submit

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

batches that depend on the fence if we don't examine their execution

and

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

results, only their completion?

Ah, OK, now I get it. Do you suggest that we should also add a check

for

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

HANG flag for igt_store_word block? But we still need to send something to test this case.

Why do we need to send something? What that something should look like?

IOW,

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

what are we trying to exercise with this case? How is it supposed to be different from wait-hang scenario?

test_fence_await tests pipelining work (here, writing something to memory in parallel) and cross-checks its status with what's reported by the fence,

True for *await (no HANG) subtests, but in *await-hang variants I still

can't

Quoted 1 line

Quoted 1 line

recognize checks in place as cross-checks and what value they add compared

to

Quoted 14 lines

Quoted 1 line

wait-hang.

wait_hang and await_hang are different in the sense of work they do. test_fence_busy is a test with a simple batch buffer (with no actual writes) that is _iteratively_ executed on different physical engines, whereas test_fence_await spawns work on different engines in _parallel_. It's a different scenario from the fence's perspective, as we have one fence on the spinner and pass it to other engines which (asynchronously) wait on it. The value added here is the check for 1 fence - many engines (wait_hang has 1-1 mapping).

Quoted 3 lines

Does a check for invalidated work no longer queued nor running tell us anything about the fence behavior, i.e., if and how it affected / interacted with that work? I think it doesn't, then, I'm not sure if we

still

Quoted 9 lines

Quoted 1 line

need such limited scenarios.

I don't think I quite understand -- we check if we've sent a hanging workload, which is expected to result in an error, there's no additional check for the work itself (how one could check it?). As the error is raised from any of the engines (I'm taking about the await case), the fence is expected to signal, and this is what we check. The fence itself can't interact with that work

Quoted 4 lines

Quoted 2 lines

Please convince me that we do and I'll be happy
to push the series with my S-o-b: added.

I have commit rights now, so I can push it myself,

OK, let's take a different approach then. Please feel free to push the series with R-b:s collected so far yourself, and I'll try to update and restore disabled checks in a follow-up patch.

Thanks,
Janusz