tests/i915: Use engine query interface for gem_ctx_isolation/persistence

== Series Details ==

Series: tests/i915: Use engine query interface for gem_ctx_isolation/persistence
URL : https://patchwork.freedesktop.org/series/70329/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7467 -> IGTPW_3799
====================================================

Summary
-------

**FAILURE**

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

If you think the reported changes have nothing to do with the
changes
introduced in IGTPW_3799, 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_3799/index.html

Possible new issues
-------------------

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

### IGT changes ###

#### Possible regressions ####

* igt@i915_selftest@live_blt:
- fi-hsw-4770r: [PASS][1] -> [DMESG-FAIL][2]
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7467/fi-hsw-4770r/igt@i915_selftest@live_blt.html
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-hsw-4770r/igt@i915_selftest@live_blt.html

#### Suppressed ####

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

* igt@i915_module_load@reload-no-display:
- {fi-kbl-7560u}: [PASS][3] -> [DMESG-WARN][4]
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7467/fi-kbl-7560u/igt@[email protected]
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-kbl-7560u/igt@[email protected]

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

* igt@i915_pm_rpm@module-reload:
- fi-skl-lmem: [PASS][5] -> [DMESG-WARN][6] ([i915#592])
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7467/fi-skl-lmem/igt@[email protected]
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-skl-lmem/igt@[email protected]

* igt@i915_selftest@live_blt:
- fi-bwr-2160: [PASS][7] -> [INCOMPLETE][8] ([i915#695])
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7467/fi-bwr-2160/igt@i915_selftest@live_blt.html
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-bwr-2160/igt@i915_selftest@live_blt.html

* igt@kms_chamelium@dp-crc-fast:
- fi-icl-u2: [PASS][9] -> [FAIL][10] ([fdo#109635] /
[i915#262])
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7467/fi-icl-u2/igt@[email protected]
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-icl-u2/igt@[email protected]

#### Possible fixes ####

* igt@i915_pm_rpm@module-reload:
- fi-skl-6770hq: [DMESG-WARN][11] ([i915#592]) -> [PASS][12]
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7467/fi-skl-6770hq/igt@[email protected]
[12]:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-skl-6770hq/igt@[email protected]

* igt@i915_selftest@live_blt:
- fi-ivb-3770: [DMESG-FAIL][13] ([i915#563]) -> [PASS][14]
[13]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7467/fi-ivb-3770/igt@i915_selftest@live_blt.html
[14]:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-ivb-3770/igt@i915_selftest@live_blt.html
- fi-hsw-4770: [DMESG-FAIL][15] ([i915#563]) -> [PASS][16]
[15]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7467/fi-hsw-4770/igt@i915_selftest@live_blt.html
[16]:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-hsw-4770/igt@i915_selftest@live_blt.html

* igt@i915_selftest@live_gem_contexts:
- fi-hsw-peppy: [DMESG-FAIL][17] ([fdo#111692]) ->
[PASS][18]
[17]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7467/fi-hsw-peppy/igt@i915_selftest@live_gem_contexts.html
[18]:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-hsw-peppy/igt@i915_selftest@live_gem_contexts.html

* igt@kms_frontbuffer_tracking@basic:
- fi-icl-guc: [FAIL][19] ([i915#49]) -> [PASS][20]
[19]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7467/fi-icl-guc/igt@[email protected]
[20]:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-icl-guc/igt@[email protected]

#### Warnings ####

* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
- fi-kbl-x1275: [DMESG-WARN][21] ([i915#62] / [i915#92]) ->
[DMESG-WARN][22] ([i915#62] / [i915#92] / [i915#95]) +7 similar issues
[21]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7467/fi-kbl-x1275/igt@[email protected]
[22]:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-kbl-x1275/igt@[email protected]

* igt@kms_flip@basic-flip-vs-modeset:
- fi-kbl-x1275: [DMESG-WARN][23] ([i915#62] / [i915#92] /
[i915#95]) -> [DMESG-WARN][24] ([i915#62] / [i915#92]) +8 similar issues
[23]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7467/fi-kbl-x1275/igt@[email protected]
[24]:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-kbl-x1275/igt@[email protected]

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

[fdo#109635]: https://bugs.freedesktop.org/show_bug.cgi?id=109635
[fdo#111692]: https://bugs.freedesktop.org/show_bug.cgi?id=111692
[i915#262]: https://gitlab.freedesktop.org/drm/intel/issues/262
[i915#49]: https://gitlab.freedesktop.org/drm/intel/issues/49
[i915#563]: https://gitlab.freedesktop.org/drm/intel/issues/563
[i915#592]: https://gitlab.freedesktop.org/drm/intel/issues/592
[i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
[i915#695]: https://gitlab.freedesktop.org/drm/intel/issues/695
[i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
[i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95

Participating hosts (50 -> 45)
------------------------------

Missing (5): fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-ctg-p8600
fi-byt-clapper

Build changes
-------------

* CI: CI-20190529 -> None
* IGT: IGT_5321 -> IGTPW_3799

CI-20190529: 20190529
CI_DRM_7467: 14954f24e7251b067b2081aaa09a7da6840da0d5 @
git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_3799:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/index.html
IGT_5321: 9df50aef49e0da4413609d9866b41b82b725f2a0 @
git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/index.html

Align with gem_exec_basic and other tests using the newer engine query interface into i915 to enumerate active engines.

Signed-off-by Stuart Summers <[email protected]>

---
  tests/i915/gem_ctx_isolation.c   | 4 ++--
  tests/i915/gem_ctx_persistence.c | 2 +-
  2 files changed, 3 insertions(+), 3 deletions(-)

tests/i915/gem_ctx_isolation.c +2 -2

index 6aa27133..9435209e 100644
@@ -856,6 +856,7 @@ static unsigned int __has_context_isolation(int fd)
 
  igt_main
  {
+	struct intel_execution_engine2 *e;
  	unsigned int has_context_isolation = 0;
  	int fd = -1;
 

@@ -876,8 +877,7 @@ igt_main
  		igt_skip_on(gen > LAST_KNOWN_GEN);
  	}
 
-	for (const struct intel_execution_engine2 *e = intel_execution_engines2;
-	     e->name; e++) {
+	__for_each_physical_engine(fd, e) {
  		igt_subtest_group {
  			igt_fixture {
  				igt_require(has_context_isolation & (1 << e->class));

tests/i915/gem_ctx_persistence.c +0 -0

index d68431ae..30772159 100644

Quoted 33 lines · Stuart Summers

Align with gem_exec_basic and other tests using the newer engine query interface into i915 to enumerate active engines.

Signed-off-by Stuart Summers <[email protected]>

---
  tests/i915/gem_ctx_isolation.c   | 4 ++--
  tests/i915/gem_ctx_persistence.c | 2 +-
  2 files changed, 3 insertions(+), 3 deletions(-)

tests/i915/gem_ctx_isolation.c +2 -2

index 6aa27133..9435209e 100644
@@ -856,6 +856,7 @@ static unsigned int __has_context_isolation(int fd)
  igt_main
  {
+	struct intel_execution_engine2 *e;
  	unsigned int has_context_isolation = 0;
  	int fd = -1;

@@ -876,8 +877,7 @@ igt_main
  		igt_skip_on(gen > LAST_KNOWN_GEN);
  	}
-	for (const struct intel_execution_engine2 *e = intel_execution_engines2;
-	     e->name; e++) {
+	__for_each_physical_engine(fd, e) {
  		igt_subtest_group {
  			igt_fixture {
  				igt_require(has_context_isolation & (1 << e->class));

tests/i915/gem_ctx_persistence.c +0 -0

index d68431ae..30772159 100644

tests/i915/gem_ctx_persistence.c igt_main · line 733

Quoted 8 lines

tests/i915/gem_ctx_persistence.c +1 -1

@@ -727,7 +727,7 @@ igt_main
  	igt_subtest("hangcheck")
  		test_nohangcheck_hostile(i915);
-	__for_each_static_engine(e) {
+	__for_each_physical_engine(i915, e) {
  		igt_subtest_group {
  			igt_fixture {
  				gem_require_ring(i915, e->flags);
 

__for_each_static_engine is correct, at least if you don't want CI folks go look for their pitchforks. :) Same for the first hunk, everything that enumerates subtests needs to be static.

Option 2, the preferred one - convert the tests to igt_subtest_with_dynamic and then you can use __for_each_physical_engine.

Quoted 56 lines · Tvrtko Ursulin

Quoted 39 lines · Stuart Summers

Align with gem_exec_basic and other tests using the newer engine query interface into i915 to enumerate active engines.

Signed-off-by Stuart Summers <[email protected]>

---
  tests/i915/gem_ctx_isolation.c   | 4 ++--
  tests/i915/gem_ctx_persistence.c | 2 +-
  2 files changed, 3 insertions(+), 3 deletions(-)

tests/i915/gem_ctx_isolation.c +2 -2

index 6aa27133..9435209e 100644
@@ -856,6 +856,7 @@ static unsigned int
 __has_context_isolation(int fd)
  igt_main
  {
+	struct intel_execution_engine2 *e;
  	unsigned int has_context_isolation = 0;
  	int fd = -1;

@@ -876,8 +877,7 @@ igt_main
  		igt_skip_on(gen > LAST_KNOWN_GEN);
  	}
-	for (const struct intel_execution_engine2 *e =
 intel_execution_engines2;
-	     e->name; e++) {
+	__for_each_physical_engine(fd, e) {
  		igt_subtest_group {
  			igt_fixture {
  				igt_require(has_context_isolati

on & (1 << e->class));

tests/i915/gem_ctx_persistence.c +0 -0

index d68431ae..30772159 100644

tests/i915/gem_ctx_persistence.c igt_main · line 733

Quoted 8 lines

tests/i915/gem_ctx_persistence.c +1 -1

@@ -727,7 +727,7 @@ igt_main
  	igt_subtest("hangcheck")
  		test_nohangcheck_hostile(i915);
-	__for_each_static_engine(e) {
+	__for_each_physical_engine(i915, e) {
  		igt_subtest_group {
  			igt_fixture {
  				gem_require_ring(i915, e-
 >flags);

__for_each_static_engine is correct, at least if you don't want CI folks go
look for their pitchforks. :) Same for the first hunk, everything that
enumerates subtests needs to be static.

Quoted 3 lines

Option 2, the preferred one - convert the tests to igt_subtest_with_dynamic
and then you can use __for_each_physical_engine.

Doesn't __for_each_physical_engine anyway have a hack for being called in that context?

Btw, option 2 is
https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/44

Currently blocking that effort is getting
https://patchwork.freedesktop.org/series/70286/ into shape.

Quoted 67 lines · Petri Latvala

Quoted 56 lines · Tvrtko Ursulin

Quoted 39 lines · Stuart Summers

Align with gem_exec_basic and other tests using the newer engine query interface into i915 to enumerate active engines.

Signed-off-by Stuart Summers <[email protected]>

---
  tests/i915/gem_ctx_isolation.c   | 4 ++--
  tests/i915/gem_ctx_persistence.c | 2 +-
  2 files changed, 3 insertions(+), 3 deletions(-)

tests/i915/gem_ctx_isolation.c +2 -2

index 6aa27133..9435209e 100644
@@ -856,6 +856,7 @@ static unsigned int
 __has_context_isolation(int fd)
  igt_main
  {
+	struct intel_execution_engine2 *e;
  	unsigned int has_context_isolation = 0;
  	int fd = -1;

@@ -876,8 +877,7 @@ igt_main
  		igt_skip_on(gen > LAST_KNOWN_GEN);
  	}
-	for (const struct intel_execution_engine2 *e =
 intel_execution_engines2;
-	     e->name; e++) {
+	__for_each_physical_engine(fd, e) {
  		igt_subtest_group {
  			igt_fixture {
  				igt_require(has_context_isolati

on & (1 << e->class));

tests/i915/gem_ctx_persistence.c +0 -0

index d68431ae..30772159 100644

tests/i915/gem_ctx_persistence.c igt_main · line 733

Quoted 8 lines

tests/i915/gem_ctx_persistence.c +1 -1

@@ -727,7 +727,7 @@ igt_main
  	igt_subtest("hangcheck")
  		test_nohangcheck_hostile(i915);
-	__for_each_static_engine(e) {
+	__for_each_physical_engine(i915, e) {
  		igt_subtest_group {
  			igt_fixture {
  				gem_require_ring(i915, e-
 >flags);

__for_each_static_engine is correct, at least if you don't want CI folks go
look for their pitchforks. :) Same for the first hunk, everything that
enumerates subtests needs to be static.

Quoted 3 lines

Option 2, the preferred one - convert the tests to igt_subtest_with_dynamic
and then you can use __for_each_physical_engine.

Doesn't __for_each_physical_engine anyway have a hack for being called in that context?

Btw, option 2 is
https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/44

Quoted 2 lines

Currently blocking that effort is getting
https://patchwork.freedesktop.org/series/70286/ into shape.

Wait I'm a little confused, sorry for the naivete here. Does this mean these kinds of changes are blocked on the above series? Or are you saying I should go ahead and convert this to the dynamic subtests? Or that we can move forward with the current approach and convert at a later time?

Quoted 68 lines · Stuart

Quoted 67 lines · Petri Latvala

Quoted 56 lines · Tvrtko Ursulin

Quoted 39 lines · Stuart Summers

Align with gem_exec_basic and other tests using the newer engine query interface into i915 to enumerate active engines.

Signed-off-by Stuart Summers <[email protected]>

---
   tests/i915/gem_ctx_isolation.c   | 4 ++--
   tests/i915/gem_ctx_persistence.c | 2 +-
   2 files changed, 3 insertions(+), 3 deletions(-)

tests/i915/gem_ctx_isolation.c +2 -2

index 6aa27133..9435209e 100644
@@ -856,6 +856,7 @@ static unsigned int
 __has_context_isolation(int fd)
   igt_main
   {
+	struct intel_execution_engine2 *e;
   	unsigned int has_context_isolation = 0;
   	int fd = -1;

@@ -876,8 +877,7 @@ igt_main
   		igt_skip_on(gen > LAST_KNOWN_GEN);
   	}
-	for (const struct intel_execution_engine2 *e =
 intel_execution_engines2;
-	     e->name; e++) {
+	__for_each_physical_engine(fd, e) {
   		igt_subtest_group {
   			igt_fixture {
   				igt_require(has_context_isolati

on & (1 << e->class));

tests/i915/gem_ctx_persistence.c +0 -0

index d68431ae..30772159 100644

tests/i915/gem_ctx_persistence.c igt_main · line 733

Quoted 8 lines

tests/i915/gem_ctx_persistence.c +1 -1

@@ -727,7 +727,7 @@ igt_main
   	igt_subtest("hangcheck")
   		test_nohangcheck_hostile(i915);
-	__for_each_static_engine(e) {
+	__for_each_physical_engine(i915, e) {
   		igt_subtest_group {
   			igt_fixture {
   				gem_require_ring(i915, e-
 > flags);

__for_each_static_engine is correct, at least if you don't want CI folks go
look for their pitchforks. :) Same for the first hunk, everything that
enumerates subtests needs to be static.

Quoted 3 lines

Option 2, the preferred one - convert the tests to igt_subtest_with_dynamic
and then you can use __for_each_physical_engine.

Doesn't __for_each_physical_engine anyway have a hack for being called in that context?

Btw, option 2 is
https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/44

Quoted 8 lines

Quoted 2 lines

Currently blocking that effort is getting
https://patchwork.freedesktop.org/series/70286/ into shape.

Wait I'm a little confused, sorry for the naivete here. Does this mean these kinds of changes are blocked on the above series? Or are you saying I should go ahead and convert this to the dynamic subtests? Or that we can move forward with the current approach and convert at a later time?

Option 2 is blocked by series 70286, sorry for the confusion. I don't mind slapping your patch in while waiting for that, since it's $RAND days away still to get there, if it fixes a problem you're having now.

Quoted 78 lines · Petri Latvala

Quoted 76 lines · Stuart

Quoted 61 lines · Petri Latvala

Quoted 56 lines · Tvrtko Ursulin

Quoted 39 lines · Stuart Summers

Align with gem_exec_basic and other tests using the newer engine query interface into i915 to enumerate active engines.

Signed-off-by Stuart Summers <[email protected]>

---
   tests/i915/gem_ctx_isolation.c   | 4 ++--
   tests/i915/gem_ctx_persistence.c | 2 +-
   2 files changed, 3 insertions(+), 3 deletions(-)

tests/i915/gem_ctx_isolation.c +2 -2

index 6aa27133..9435209e 100644
@@ -856,6 +856,7 @@ static unsigned int
 __has_context_isolation(int fd)
   igt_main
   {
+	struct intel_execution_engine2 *e;
   	unsigned int has_context_isolation = 0;
   	int fd = -1;

@@ -876,8 +877,7 @@ igt_main
   		igt_skip_on(gen > LAST_KNOWN_GEN);
   	}
-	for (const struct intel_execution_engine2 *e =
 intel_execution_engines2;
-	     e->name; e++) {
+	__for_each_physical_engine(fd, e) {
   		igt_subtest_group {
   			igt_fixture {
   				igt_require(has_context_isolati

on & (1 << e->class));

tests/i915/gem_ctx_persistence.c +0 -0

index d68431ae..30772159 100644

tests/i915/gem_ctx_persistence.c igt_main · line 733

Quoted 8 lines

tests/i915/gem_ctx_persistence.c +1 -1

@@ -727,7 +727,7 @@ igt_main
   	igt_subtest("hangcheck")
   		test_nohangcheck_hostile(i915);
-	__for_each_static_engine(e) {
+	__for_each_physical_engine(i915, e) {
   		igt_subtest_group {
   			igt_fixture {
   				gem_require_ring(i915, e-
 > flags);

__for_each_static_engine is correct, at least if you don't want CI folks go
look for their pitchforks. :) Same for the first hunk, everything that
enumerates subtests needs to be static.

Quoted 3 lines

Option 2, the preferred one - convert the tests to igt_subtest_with_dynamic
and then you can use __for_each_physical_engine.

Quoted 8 lines

Doesn't __for_each_physical_engine anyway have a hack for being called in that context?

Btw, option 2 is
https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/44

Currently blocking that effort is getting
https://patchwork.freedesktop.org/series/70286/ into shape.

Wait I'm a little confused, sorry for the naivete here. Does this mean these kinds of changes are blocked on the above series? Or are you saying I should go ahead and convert this to the dynamic subtests? Or that we can move forward with the current approach and convert at a later time?

Quoted 3 lines

Option 2 is blocked by series 70286, sorry for the confusion. I don't mind slapping your patch in while waiting for that, since it's $RAND days away still to get there, if it fixes a problem you're having now.

I think hack for static enumeration if in --list-subtest mode did not work in practice. Andi, do you remember the details?

Hi,

Quoted 83 lines · Tvrtko Ursulin

Quoted 81 lines · Petri Latvala

Quoted 68 lines · Stuart

Quoted 67 lines · Petri Latvala

Quoted 56 lines · Tvrtko Ursulin

Quoted 39 lines · Stuart Summers

Align with gem_exec_basic and other tests using the newer engine query interface into i915 to enumerate active engines.

Signed-off-by Stuart Summers <[email protected]>

---
    tests/i915/gem_ctx_isolation.c   | 4 ++--
    tests/i915/gem_ctx_persistence.c | 2 +-
    2 files changed, 3 insertions(+), 3 deletions(-)

tests/i915/gem_ctx_isolation.c +2 -2

index 6aa27133..9435209e 100644
@@ -856,6 +856,7 @@ static unsigned int
 __has_context_isolation(int fd)
    igt_main
    {
+	struct intel_execution_engine2 *e;
    	unsigned int has_context_isolation = 0;
    	int fd = -1;

@@ -876,8 +877,7 @@ igt_main
    		igt_skip_on(gen > LAST_KNOWN_GEN);
    	}
-	for (const struct intel_execution_engine2 *e =
 intel_execution_engines2;
-	     e->name; e++) {
+	__for_each_physical_engine(fd, e) {
    		igt_subtest_group {
    			igt_fixture {
    				igt_require(has_context_isolati

on & (1 << e->class));

tests/i915/gem_ctx_persistence.c +0 -0

index d68431ae..30772159 100644

tests/i915/gem_ctx_persistence.c igt_main · line 733

Quoted 8 lines

tests/i915/gem_ctx_persistence.c +1 -1

@@ -727,7 +727,7 @@ igt_main
    	igt_subtest("hangcheck")
    		test_nohangcheck_hostile(i915);
-	__for_each_static_engine(e) {
+	__for_each_physical_engine(i915, e) {
    		igt_subtest_group {
    			igt_fixture {
    				gem_require_ring(i915, e-
 > flags);

__for_each_static_engine is correct, at least if you don't want CI folks go
look for their pitchforks. :) Same for the first hunk, everything that
enumerates subtests needs to be static.

Quoted 3 lines

Option 2, the preferred one - convert the tests to igt_subtest_with_dynamic
and then you can use __for_each_physical_engine.

Doesn't __for_each_physical_engine anyway have a hack for being called in that context?

Btw, option 2 is
https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/44

Quoted 8 lines

Quoted 2 lines

Currently blocking that effort is getting
https://patchwork.freedesktop.org/series/70286/ into shape.

Wait I'm a little confused, sorry for the naivete here. Does this mean these kinds of changes are blocked on the above series? Or are you saying I should go ahead and convert this to the dynamic subtests? Or that we can move forward with the current approach and convert at a later time?

Option 2 is blocked by series 70286, sorry for the confusion. I don't mind slapping your patch in while waiting for that, since it's $RAND days away still to get there, if it fixes a problem you're having now.

Quoted 2 lines

I think hack for static enumeration if in --list-subtest mode did not work in practice. Andi, do you remember the details?

__for_each_physical_engine checks for igt_only_list_subtests(), if so it behaves like __for_each_static_engine().

Was this what you asked?

Quoted 89 lines · Andi Shyti

Hi,

Quoted 85 lines · Tvrtko Ursulin

Quoted 78 lines · Petri Latvala

Quoted 68 lines · Stuart

Quoted 67 lines · Petri Latvala

Quoted 56 lines · Tvrtko Ursulin

Quoted 39 lines · Stuart Summers

Align with gem_exec_basic and other tests using the newer engine query interface into i915 to enumerate active engines.

Signed-off-by Stuart Summers <[email protected]>

---
    tests/i915/gem_ctx_isolation.c   | 4 ++--
    tests/i915/gem_ctx_persistence.c | 2 +-
    2 files changed, 3 insertions(+), 3 deletions(-)

tests/i915/gem_ctx_isolation.c +2 -2

index 6aa27133..9435209e 100644
@@ -856,6 +856,7 @@ static unsigned int
 __has_context_isolation(int fd)
    igt_main
    {
+	struct intel_execution_engine2 *e;
    	unsigned int has_context_isolation = 0;
    	int fd = -1;

@@ -876,8 +877,7 @@ igt_main
    		igt_skip_on(gen > LAST_KNOWN_GEN);
    	}
-	for (const struct intel_execution_engine2 *e =
 intel_execution_engines2;
-	     e->name; e++) {
+	__for_each_physical_engine(fd, e) {
    		igt_subtest_group {
    			igt_fixture {
    				igt_require(has_context_isolati

on & (1 << e->class));

tests/i915/gem_ctx_persistence.c +0 -0

index d68431ae..30772159 100644

tests/i915/gem_ctx_persistence.c igt_main · line 733

Quoted 8 lines

tests/i915/gem_ctx_persistence.c +1 -1

@@ -727,7 +727,7 @@ igt_main
    	igt_subtest("hangcheck")
    		test_nohangcheck_hostile(i915);
-	__for_each_static_engine(e) {
+	__for_each_physical_engine(i915, e) {
    		igt_subtest_group {
    			igt_fixture {
    				gem_require_ring(i915, e-
 > flags);

__for_each_static_engine is correct, at least if you don't want CI folks go
look for their pitchforks. :) Same for the first hunk, everything that
enumerates subtests needs to be static.

Quoted 3 lines

Option 2, the preferred one - convert the tests to igt_subtest_with_dynamic
and then you can use __for_each_physical_engine.

Doesn't __for_each_physical_engine anyway have a hack for being called in that context?

Btw, option 2 is
https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/44

Quoted 8 lines

Quoted 2 lines

Currently blocking that effort is getting
https://patchwork.freedesktop.org/series/70286/ into shape.

Wait I'm a little confused, sorry for the naivete here. Does this mean these kinds of changes are blocked on the above series? Or are you saying I should go ahead and convert this to the dynamic subtests? Or that we can move forward with the current approach and convert at a later time?

Quoted 3 lines

Option 2 is blocked by series 70286, sorry for the confusion. I don't mind slapping your patch in while waiting for that, since it's $RAND days away still to get there, if it fixes a problem you're having now.

I think hack for static enumeration if in --list-subtest mode did not work in practice. Andi, do you remember the details?

Quoted 4 lines

__for_each_physical_engine checks for igt_only_list_subtests(), if so it behaves like __for_each_static_engine().

Was this what you asked?

Yes. Am I mis-remembering that there was a problem with this mandating to keep using __for_each_static_engine directly for subtest enumeration? Maybe I am..

Quoted 91 lines · Tvrtko Ursulin

Quoted 89 lines · Andi Shyti

Hi,

Quoted 85 lines · Tvrtko Ursulin

Quoted 78 lines · Petri Latvala

Quoted 68 lines · Stuart

Quoted 67 lines · Petri Latvala

Quoted 56 lines · Tvrtko Ursulin

Quoted 39 lines · Stuart Summers

Align with gem_exec_basic and other tests using the newer engine query interface into i915 to enumerate active engines.

Signed-off-by Stuart Summers <[email protected]>

---
     tests/i915/gem_ctx_isolation.c   | 4 ++--
     tests/i915/gem_ctx_persistence.c | 2 +-
     2 files changed, 3 insertions(+), 3 deletions(-)

tests/i915/gem_ctx_isolation.c +2 -2

index 6aa27133..9435209e 100644
@@ -856,6 +856,7 @@ static unsigned int
 __has_context_isolation(int fd)
     igt_main
     {
+	struct intel_execution_engine2 *e;
     	unsigned int has_context_isolation = 0;
     	int fd = -1;

@@ -876,8 +877,7 @@ igt_main
     		igt_skip_on(gen > LAST_KNOWN_GEN);
     	}
-	for (const struct intel_execution_engine2 *e =
 intel_execution_engines2;
-	     e->name; e++) {
+	__for_each_physical_engine(fd, e) {
     		igt_subtest_group {
     			igt_fixture {
     				igt_require(has_context_isolati

on & (1 << e->class));

tests/i915/gem_ctx_persistence.c +0 -0

index d68431ae..30772159 100644

tests/i915/gem_ctx_persistence.c igt_main · line 733

Quoted 8 lines

tests/i915/gem_ctx_persistence.c +1 -1

@@ -727,7 +727,7 @@ igt_main
     	igt_subtest("hangcheck")
     		test_nohangcheck_hostile(i915);
-	__for_each_static_engine(e) {
+	__for_each_physical_engine(i915, e) {
     		igt_subtest_group {
     			igt_fixture {
     				gem_require_ring(i915, e-
 > flags);

__for_each_static_engine is correct, at least if you don't want CI folks go
look for their pitchforks. :) Same for the first hunk, everything that
enumerates subtests needs to be static.

Quoted 3 lines

Option 2, the preferred one - convert the tests to igt_subtest_with_dynamic
and then you can use __for_each_physical_engine.

Doesn't __for_each_physical_engine anyway have a hack for being called in that context?

Btw, option 2 is
https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/44

Quoted 8 lines

Quoted 2 lines

Currently blocking that effort is getting
https://patchwork.freedesktop.org/series/70286/ into shape.

Wait I'm a little confused, sorry for the naivete here. Does this mean these kinds of changes are blocked on the above series? Or are you saying I should go ahead and convert this to the dynamic subtests? Or that we can move forward with the current approach and convert at a later time?

Quoted 3 lines

Option 2 is blocked by series 70286, sorry for the confusion. I don't mind slapping your patch in while waiting for that, since it's $RAND days away still to get there, if it fixes a problem you're having now.

I think hack for static enumeration if in --list-subtest mode did not work in practice. Andi, do you remember the details?

Quoted 8 lines

Quoted 4 lines

__for_each_physical_engine checks for igt_only_list_subtests(), if so it behaves like __for_each_static_engine().

Was this what you asked?

Yes. Am I mis-remembering that there was a problem with this mandating to keep using __for_each_static_engine directly for subtest enumeration? Maybe I am..

__physical is used in a couple of tests to enumerate subtests already, and there are still issues caused by that. Not in the CI sense, the enumeration works, but can cause havoc when executing the tests. Don't have links available on hand now, sorry.

In a nutshell: It's ok for now. I don't like it, but it's used already and it's going away throughout soon. Feel free to use it until said otherwise.

Quoted 80 lines · Petri Latvala

Quoted 79 lines · Tvrtko Ursulin

Quoted 77 lines · Andi Shyti

Hi,

Quoted 74 lines · Tvrtko Ursulin

Quoted 73 lines · Petri Latvala

Quoted 71 lines · Stuart

Quoted 70 lines · Petri Latvala

Quoted 67 lines · Tvrtko Ursulin

Quoted 51 lines · Stuart Summers

Align with gem_exec_basic and other tests using the newer
engine query interface into i915 to enumerate active engines.

Signed-off-by: Stuart Summers <
[email protected]>

---
     tests/i915/gem_ctx_isolation.c   | 4 ++--
     tests/i915/gem_ctx_persistence.c | 2 +-
     2 files changed, 3 insertions(+), 3

deletions(-)

tests/i915/gem_ctx_isolation.c +2 -2

index 6aa27133..9435209e 100644
@@ -856,6 +856,7 @@ static unsigned int
 __has_context_isolation(int fd)
     igt_main
     {
+	struct intel_execution_engine2 *e;
     	unsigned int has_context_isolation = 0;
     	int fd = -1;

@@ -876,8 +877,7 @@ igt_main
     		igt_skip_on(gen >
 LAST_KNOWN_GEN);
     	}
-	for (const struct intel_execution_engine2 *e =
 intel_execution_engines2;
-	     e->name; e++) {
+	__for_each_physical_engine(fd, e) {
     		igt_subtest_group {
     			igt_fixture {

igt_require(has
_context_isolati
on & (1 << e->class));

tests/i915/gem_ctx_persistence.c +1 -1

index d68431ae..30772159 100644
@@ -727,7 +727,7 @@ igt_main
     	igt_subtest("hangcheck")
     		test_nohangcheck_hostile(i915);
-	__for_each_static_engine(e) {
+	__for_each_physical_engine(i915, e) {
     		igt_subtest_group {
     			igt_fixture {
     				gem_require_rin

Quoted 2 lines

g(i915, e-

Quoted 1 line

flags);

__for_each_static_engine is correct, at least if you don't want CI
folks go
look for their pitchforks. :) Same for the first hunk, everything
that
enumerates subtests needs to be static.

Option 2, the preferred one - convert the tests to igt_subtest_with_dynamic
and then you can use __for_each_physical_engine.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Doesn't __for_each_physical_engine anyway have a hack for being
called in that context?

Btw, option 2 is

Quoted 44 lines

Quoted 29 lines

Quoted 29 lines

Quoted 22 lines

Quoted 15 lines

Quoted 5 lines

Quoted 4 lines

Currently blocking that effort is getting
https://patchwork.freedesktop.org/series/70286/ into shape.

Quoted 9 lines

Wait I'm a little confused, sorry for the naivete here. Does this mean
these kinds of changes are blocked on the above series? Or are you
saying I should go ahead and convert this to the dynamic subtests? Or
that we can move forward with the current approach and convert at a
later time?

Quoted 6 lines

Option 2 is blocked by series 70286, sorry for the confusion. I don't
mind slapping your patch in while waiting for that, since it's $RAND
days away still to get there, if it fixes a problem you're having now.

Quoted 3 lines

I think hack for static enumeration if in --list-subtest mode did not work
in practice. Andi, do you remember the details?

__for_each_physical_engine checks for igt_only_list_subtests(), if so it behaves like __for_each_static_engine().

Quoted 7 lines

Quoted 1 line

Was this what you asked?

Yes. Am I mis-remembering that there was a problem with this mandating to
keep using __for_each_static_engine directly for subtest enumeration? Maybe
I am..

__physical is used in a couple of tests to enumerate subtests already,
and there are still issues caused by that. Not in the CI sense, the enumeration works, but can cause havoc when executing the tests. Don't
have links available on hand now, sorry.

Quoted 4 lines

In a nutshell: It's ok for now. I don't like it, but it's used already
and it's going away throughout soon. Feel free to use it until said otherwise.

Okay, sorry for the confusion:

Reviewed-by Tvrtko Ursulin <[email protected]>

Regards,

Tvrtko

Quoted 87 lines · Tvrtko Ursulin

Quoted 85 lines · Petri Latvala

Quoted 78 lines · Stuart

Quoted 68 lines · Petri Latvala

Quoted 67 lines · Tvrtko Ursulin

Quoted 56 lines · Stuart Summers

Quoted 39 lines

Align with gem_exec_basic and other tests using the newer engine query interface into i915 to enumerate active engines.

Signed-off-by Stuart Summers <[email protected]>

---
    tests/i915/gem_ctx_isolation.c   | 4 ++--
    tests/i915/gem_ctx_persistence.c | 2 +-
    2 files changed, 3 insertions(+), 3 deletions(-)

tests/i915/gem_ctx_isolation.c +2 -2

index 6aa27133..9435209e 100644
@@ -856,6 +856,7 @@ static unsigned int
 __has_context_isolation(int fd)
    igt_main
    {
+	struct intel_execution_engine2 *e;
    	unsigned int has_context_isolation = 0;
    	int fd = -1;

@@ -876,8 +877,7 @@ igt_main
    		igt_skip_on(gen > LAST_KNOWN_GEN);
    	}
-	for (const struct intel_execution_engine2 *e =
 intel_execution_engines2;
-	     e->name; e++) {
+	__for_each_physical_engine(fd, e) {
    		igt_subtest_group {
    			igt_fixture {
    				igt_require(has_context_isolati

on & (1 << e->class));

tests/i915/gem_ctx_persistence.c +0 -0

index d68431ae..30772159 100644

tests/i915/gem_ctx_persistence.c igt_main · line 733

Quoted 8 lines

tests/i915/gem_ctx_persistence.c +1 -1

@@ -727,7 +727,7 @@ igt_main
    	igt_subtest("hangcheck")
    		test_nohangcheck_hostile(i915);
-	__for_each_static_engine(e) {
+	__for_each_physical_engine(i915, e) {
    		igt_subtest_group {
    			igt_fixture {
    				gem_require_ring(i915, e-
 > flags);

__for_each_static_engine is correct, at least if you don't want CI folks go
look for their pitchforks. :) Same for the first hunk, everything that
enumerates subtests needs to be static.

Quoted 3 lines

Option 2, the preferred one - convert the tests to igt_subtest_with_dynamic
and then you can use __for_each_physical_engine.

Doesn't __for_each_physical_engine anyway have a hack for being called in that context?

Btw, option 2 is
https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/44

Quoted 8 lines

Quoted 2 lines

Currently blocking that effort is getting
https://patchwork.freedesktop.org/series/70286/ into shape.

Wait I'm a little confused, sorry for the naivete here. Does this mean these kinds of changes are blocked on the above series? Or are you saying I should go ahead and convert this to the dynamic subtests? Or that we can move forward with the current approach and convert at a later time?

Quoted 3 lines

Option 2 is blocked by series 70286, sorry for the confusion. I don't mind slapping your patch in while waiting for that, since it's $RAND days away still to get there, if it fixes a problem you're having now.

I think hack for static enumeration if in --list-subtest mode did not work in practice. Andi, do you remember the details?

Quoted 4 lines

__for_each_physical_engine checks for igt_only_list_subtests(), if so it behaves like __for_each_static_engine().

Was this what you asked?

Yes. Am I mis-remembering that there was a problem with this mandating to keep using __for_each_static_engine directly for subtest enumeration? Maybe I am..