[igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[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 <stuart.summers@intel.com>
---
 tests/i915/gem_ctx_isolation.c   | 4 ++--
 tests/i915/gem_ctx_persistence.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
index 6aa27133..9435209e 100644
--- a/tests/i915/gem_ctx_isolation.c
+++ b/tests/i915/gem_ctx_isolation.c
@@ -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));
diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c
index d68431ae..30772159 100644
--- a/tests/i915/gem_ctx_persistence.c
+++ b/tests/i915/gem_ctx_persistence.c
@@ -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);
-- 
2.21.0.5.gaeb582a983

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

[igt-dev] ✗ Fi.CI.BAT: failure for tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Patchwork]

== 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@i915_module_load@reload-no-display.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-kbl-7560u/igt@i915_module_load@reload-no-display.html

  
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@i915_pm_rpm@module-reload.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-skl-lmem/igt@i915_pm_rpm@module-reload.html

  * 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@kms_chamelium@dp-crc-fast.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-icl-u2/igt@kms_chamelium@dp-crc-fast.html

  
#### 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@i915_pm_rpm@module-reload.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html

  * 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@kms_frontbuffer_tracking@basic.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-icl-guc/igt@kms_frontbuffer_tracking@basic.html

  
#### 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@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-kbl-x1275/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * 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@kms_flip@basic-flip-vs-modeset.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-modeset.html

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

Re: [igt-dev] ✗ Fi.CI.BAT: failure for tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Summers, Stuart]

[-- Attachment #1.1: Type: text/plain, Size: 7507 bytes --]

On Tue, 2019-12-03 at 06:03 +0000, Patchwork wrote:
> == 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

Not sure how this is related to the changes I made.

-Stuart

> 
>   
> #### 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@i915_module_load@reload-no-display.html
>    [4]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-kbl-7560u/igt@i915_module_load@reload-no-display.html
> 
>   
> 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@i915_pm_rpm@module-reload.html
>    [6]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-skl-lmem/igt@i915_pm_rpm@module-reload.html
> 
>   * 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@kms_chamelium@dp-crc-fast.html
>    [10]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-icl-u2/igt@kms_chamelium@dp-crc-fast.html
> 
>   
> #### 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@i915_pm_rpm@module-reload.html
>    [12]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
> 
>   * 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@kms_frontbuffer_tracking@basic.html
>    [20]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-icl-guc/igt@kms_frontbuffer_tracking@basic.html
> 
>   
> #### 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@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
>    [22]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-kbl-x1275/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
> 
>   * 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@kms_flip@basic-flip-vs-modeset.html
>    [24]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3799/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-modeset.html
> 
>   
>   {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

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Tvrtko Ursulin]

On 03/12/2019 05:11, Stuart Summers wrote:
> 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 <stuart.summers@intel.com>
> ---
>   tests/i915/gem_ctx_isolation.c   | 4 ++--
>   tests/i915/gem_ctx_persistence.c | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> index 6aa27133..9435209e 100644
> --- a/tests/i915/gem_ctx_isolation.c
> +++ b/tests/i915/gem_ctx_isolation.c
> @@ -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));
> diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c
> index d68431ae..30772159 100644
> --- a/tests/i915/gem_ctx_persistence.c
> +++ b/tests/i915/gem_ctx_persistence.c
> @@ -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.

Regards,

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

Re: [igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Petri Latvala]

On Wed, Dec 04, 2019 at 09:36:11AM +0000, Tvrtko Ursulin wrote:
> 
> On 03/12/2019 05:11, Stuart Summers wrote:
> > 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 <stuart.summers@intel.com>
> > ---
> >   tests/i915/gem_ctx_isolation.c   | 4 ++--
> >   tests/i915/gem_ctx_persistence.c | 2 +-
> >   2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> > index 6aa27133..9435209e 100644
> > --- a/tests/i915/gem_ctx_isolation.c
> > +++ b/tests/i915/gem_ctx_isolation.c
> > @@ -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));
> > diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c
> > index d68431ae..30772159 100644
> > --- a/tests/i915/gem_ctx_persistence.c
> > +++ b/tests/i915/gem_ctx_persistence.c
> > @@ -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.

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.


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

Re: [igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Summers, Stuart]

[-- Attachment #1.1: Type: text/plain, Size: 2932 bytes --]

On Wed, 2019-12-04 at 16:53 +0200, Petri Latvala wrote:
> On Wed, Dec 04, 2019 at 09:36:11AM +0000, Tvrtko Ursulin wrote:
> > 
> > On 03/12/2019 05:11, Stuart Summers wrote:
> > > 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 <stuart.summers@intel.com>
> > > ---
> > >   tests/i915/gem_ctx_isolation.c   | 4 ++--
> > >   tests/i915/gem_ctx_persistence.c | 2 +-
> > >   2 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tests/i915/gem_ctx_isolation.c
> > > b/tests/i915/gem_ctx_isolation.c
> > > index 6aa27133..9435209e 100644
> > > --- a/tests/i915/gem_ctx_isolation.c
> > > +++ b/tests/i915/gem_ctx_isolation.c
> > > @@ -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));
> > > diff --git a/tests/i915/gem_ctx_persistence.c
> > > b/tests/i915/gem_ctx_persistence.c
> > > index d68431ae..30772159 100644
> > > --- a/tests/i915/gem_ctx_persistence.c
> > > +++ b/tests/i915/gem_ctx_persistence.c
> > > @@ -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.
> 
> 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?

Thanks,
Stuart

> 
> 

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Petri Latvala]

On Wed, Dec 04, 2019 at 07:05:12PM +0000, Summers, Stuart wrote:
> On Wed, 2019-12-04 at 16:53 +0200, Petri Latvala wrote:
> > On Wed, Dec 04, 2019 at 09:36:11AM +0000, Tvrtko Ursulin wrote:
> > > 
> > > On 03/12/2019 05:11, Stuart Summers wrote:
> > > > 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 <stuart.summers@intel.com>
> > > > ---
> > > >   tests/i915/gem_ctx_isolation.c   | 4 ++--
> > > >   tests/i915/gem_ctx_persistence.c | 2 +-
> > > >   2 files changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/tests/i915/gem_ctx_isolation.c
> > > > b/tests/i915/gem_ctx_isolation.c
> > > > index 6aa27133..9435209e 100644
> > > > --- a/tests/i915/gem_ctx_isolation.c
> > > > +++ b/tests/i915/gem_ctx_isolation.c
> > > > @@ -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));
> > > > diff --git a/tests/i915/gem_ctx_persistence.c
> > > > b/tests/i915/gem_ctx_persistence.c
> > > > index d68431ae..30772159 100644
> > > > --- a/tests/i915/gem_ctx_persistence.c
> > > > +++ b/tests/i915/gem_ctx_persistence.c
> > > > @@ -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.
> > 
> > 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?

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.


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

Re: [igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Tvrtko Ursulin]

On 05/12/2019 09:01, Petri Latvala wrote:
> On Wed, Dec 04, 2019 at 07:05:12PM +0000, Summers, Stuart wrote:
>> On Wed, 2019-12-04 at 16:53 +0200, Petri Latvala wrote:
>>> On Wed, Dec 04, 2019 at 09:36:11AM +0000, Tvrtko Ursulin wrote:
>>>>
>>>> On 03/12/2019 05:11, Stuart Summers wrote:
>>>>> 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 <stuart.summers@intel.com>
>>>>> ---
>>>>>    tests/i915/gem_ctx_isolation.c   | 4 ++--
>>>>>    tests/i915/gem_ctx_persistence.c | 2 +-
>>>>>    2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/tests/i915/gem_ctx_isolation.c
>>>>> b/tests/i915/gem_ctx_isolation.c
>>>>> index 6aa27133..9435209e 100644
>>>>> --- a/tests/i915/gem_ctx_isolation.c
>>>>> +++ b/tests/i915/gem_ctx_isolation.c
>>>>> @@ -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));
>>>>> diff --git a/tests/i915/gem_ctx_persistence.c
>>>>> b/tests/i915/gem_ctx_persistence.c
>>>>> index d68431ae..30772159 100644
>>>>> --- a/tests/i915/gem_ctx_persistence.c
>>>>> +++ b/tests/i915/gem_ctx_persistence.c
>>>>> @@ -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.
>>>
>>> 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?
> 
> 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?

Regards,

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

Re: [igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Andi Shyti]

Hi,

On Thu, Dec 05, 2019 at 09:22:56AM +0000, Tvrtko Ursulin wrote:
> On 05/12/2019 09:01, Petri Latvala wrote:
> > On Wed, Dec 04, 2019 at 07:05:12PM +0000, Summers, Stuart wrote:
> > > On Wed, 2019-12-04 at 16:53 +0200, Petri Latvala wrote:
> > > > On Wed, Dec 04, 2019 at 09:36:11AM +0000, Tvrtko Ursulin wrote:
> > > > > 
> > > > > On 03/12/2019 05:11, Stuart Summers wrote:
> > > > > > 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 <stuart.summers@intel.com>
> > > > > > ---
> > > > > >    tests/i915/gem_ctx_isolation.c   | 4 ++--
> > > > > >    tests/i915/gem_ctx_persistence.c | 2 +-
> > > > > >    2 files changed, 3 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/tests/i915/gem_ctx_isolation.c
> > > > > > b/tests/i915/gem_ctx_isolation.c
> > > > > > index 6aa27133..9435209e 100644
> > > > > > --- a/tests/i915/gem_ctx_isolation.c
> > > > > > +++ b/tests/i915/gem_ctx_isolation.c
> > > > > > @@ -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));
> > > > > > diff --git a/tests/i915/gem_ctx_persistence.c
> > > > > > b/tests/i915/gem_ctx_persistence.c
> > > > > > index d68431ae..30772159 100644
> > > > > > --- a/tests/i915/gem_ctx_persistence.c
> > > > > > +++ b/tests/i915/gem_ctx_persistence.c
> > > > > > @@ -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.
> > > > 
> > > > 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?
> > 
> > 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?

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

Was this what you asked?

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

Re: [igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Tvrtko Ursulin]

On 05/12/2019 09:47, Andi Shyti wrote:
> Hi,
> 
> On Thu, Dec 05, 2019 at 09:22:56AM +0000, Tvrtko Ursulin wrote:
>> On 05/12/2019 09:01, Petri Latvala wrote:
>>> On Wed, Dec 04, 2019 at 07:05:12PM +0000, Summers, Stuart wrote:
>>>> On Wed, 2019-12-04 at 16:53 +0200, Petri Latvala wrote:
>>>>> On Wed, Dec 04, 2019 at 09:36:11AM +0000, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> On 03/12/2019 05:11, Stuart Summers wrote:
>>>>>>> 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 <stuart.summers@intel.com>
>>>>>>> ---
>>>>>>>     tests/i915/gem_ctx_isolation.c   | 4 ++--
>>>>>>>     tests/i915/gem_ctx_persistence.c | 2 +-
>>>>>>>     2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/tests/i915/gem_ctx_isolation.c
>>>>>>> b/tests/i915/gem_ctx_isolation.c
>>>>>>> index 6aa27133..9435209e 100644
>>>>>>> --- a/tests/i915/gem_ctx_isolation.c
>>>>>>> +++ b/tests/i915/gem_ctx_isolation.c
>>>>>>> @@ -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));
>>>>>>> diff --git a/tests/i915/gem_ctx_persistence.c
>>>>>>> b/tests/i915/gem_ctx_persistence.c
>>>>>>> index d68431ae..30772159 100644
>>>>>>> --- a/tests/i915/gem_ctx_persistence.c
>>>>>>> +++ b/tests/i915/gem_ctx_persistence.c
>>>>>>> @@ -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.
>>>>>
>>>>> 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?
>>>
>>> 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?
> 
> __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..

Regards,

Tvrtko


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

Re: [igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Petri Latvala]

On Thu, Dec 05, 2019 at 09:54:25AM +0000, Tvrtko Ursulin wrote:
> 
> On 05/12/2019 09:47, Andi Shyti wrote:
> > Hi,
> > 
> > On Thu, Dec 05, 2019 at 09:22:56AM +0000, Tvrtko Ursulin wrote:
> > > On 05/12/2019 09:01, Petri Latvala wrote:
> > > > On Wed, Dec 04, 2019 at 07:05:12PM +0000, Summers, Stuart wrote:
> > > > > On Wed, 2019-12-04 at 16:53 +0200, Petri Latvala wrote:
> > > > > > On Wed, Dec 04, 2019 at 09:36:11AM +0000, Tvrtko Ursulin wrote:
> > > > > > > 
> > > > > > > On 03/12/2019 05:11, Stuart Summers wrote:
> > > > > > > > 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 <stuart.summers@intel.com>
> > > > > > > > ---
> > > > > > > >     tests/i915/gem_ctx_isolation.c   | 4 ++--
> > > > > > > >     tests/i915/gem_ctx_persistence.c | 2 +-
> > > > > > > >     2 files changed, 3 insertions(+), 3 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/tests/i915/gem_ctx_isolation.c
> > > > > > > > b/tests/i915/gem_ctx_isolation.c
> > > > > > > > index 6aa27133..9435209e 100644
> > > > > > > > --- a/tests/i915/gem_ctx_isolation.c
> > > > > > > > +++ b/tests/i915/gem_ctx_isolation.c
> > > > > > > > @@ -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));
> > > > > > > > diff --git a/tests/i915/gem_ctx_persistence.c
> > > > > > > > b/tests/i915/gem_ctx_persistence.c
> > > > > > > > index d68431ae..30772159 100644
> > > > > > > > --- a/tests/i915/gem_ctx_persistence.c
> > > > > > > > +++ b/tests/i915/gem_ctx_persistence.c
> > > > > > > > @@ -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.
> > > > > > 
> > > > > > 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?
> > > > 
> > > > 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?
> > 
> > __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.


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

Re: [igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Andi Shyti]

Hi Tvrtko,

> > On Thu, Dec 05, 2019 at 09:22:56AM +0000, Tvrtko Ursulin wrote:
> > > On 05/12/2019 09:01, Petri Latvala wrote:
> > > > On Wed, Dec 04, 2019 at 07:05:12PM +0000, Summers, Stuart wrote:
> > > > > On Wed, 2019-12-04 at 16:53 +0200, Petri Latvala wrote:
> > > > > > On Wed, Dec 04, 2019 at 09:36:11AM +0000, Tvrtko Ursulin wrote:
> > > > > > > 
> > > > > > > On 03/12/2019 05:11, Stuart Summers wrote:
> > > > > > > > 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 <stuart.summers@intel.com>
> > > > > > > > ---
> > > > > > > >     tests/i915/gem_ctx_isolation.c   | 4 ++--
> > > > > > > >     tests/i915/gem_ctx_persistence.c | 2 +-
> > > > > > > >     2 files changed, 3 insertions(+), 3 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/tests/i915/gem_ctx_isolation.c
> > > > > > > > b/tests/i915/gem_ctx_isolation.c
> > > > > > > > index 6aa27133..9435209e 100644
> > > > > > > > --- a/tests/i915/gem_ctx_isolation.c
> > > > > > > > +++ b/tests/i915/gem_ctx_isolation.c
> > > > > > > > @@ -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));
> > > > > > > > diff --git a/tests/i915/gem_ctx_persistence.c
> > > > > > > > b/tests/i915/gem_ctx_persistence.c
> > > > > > > > index d68431ae..30772159 100644
> > > > > > > > --- a/tests/i915/gem_ctx_persistence.c
> > > > > > > > +++ b/tests/i915/gem_ctx_persistence.c
> > > > > > > > @@ -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.
> > > > > > 
> > > > > > 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?
> > > > 
> > > > 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?
> > 
> > __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..

we added __for_each_static_engine() for simplicity and used when
the user does want to loop through static engines.

But if a user wants static engines only if the API is not
supported, then for_each_physical_engine() works, as well.

Indeed, the code says:

if (get_param()) { /* we check here if the API fails */

	Loop through the static list if igt_only_list_subtests
	otherwise loop through exising engines (as it was done
	before by executing a batch buffer).

} else if (!param.size) {

	query and map engines

} else {

	get the engines from the previously contructed list

}

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

Re: [igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Tvrtko Ursulin]

On 05/12/2019 10:09, Petri Latvala wrote:
> On Thu, Dec 05, 2019 at 09:54:25AM +0000, Tvrtko Ursulin wrote:
>>
>> On 05/12/2019 09:47, Andi Shyti wrote:
>>> Hi,
>>>
>>> On Thu, Dec 05, 2019 at 09:22:56AM +0000, Tvrtko Ursulin wrote:
>>>> On 05/12/2019 09:01, Petri Latvala wrote:
>>>>> On Wed, Dec 04, 2019 at 07:05:12PM +0000, Summers, Stuart wrote:
>>>>>> On Wed, 2019-12-04 at 16:53 +0200, Petri Latvala wrote:
>>>>>>> On Wed, Dec 04, 2019 at 09:36:11AM +0000, Tvrtko Ursulin wrote:
>>>>>>>>
>>>>>>>> On 03/12/2019 05:11, Stuart Summers wrote:
>>>>>>>>> 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 <stuart.summers@intel.com>
>>>>>>>>> ---
>>>>>>>>>      tests/i915/gem_ctx_isolation.c   | 4 ++--
>>>>>>>>>      tests/i915/gem_ctx_persistence.c | 2 +-
>>>>>>>>>      2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/tests/i915/gem_ctx_isolation.c
>>>>>>>>> b/tests/i915/gem_ctx_isolation.c
>>>>>>>>> index 6aa27133..9435209e 100644
>>>>>>>>> --- a/tests/i915/gem_ctx_isolation.c
>>>>>>>>> +++ b/tests/i915/gem_ctx_isolation.c
>>>>>>>>> @@ -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));
>>>>>>>>> diff --git a/tests/i915/gem_ctx_persistence.c
>>>>>>>>> b/tests/i915/gem_ctx_persistence.c
>>>>>>>>> index d68431ae..30772159 100644
>>>>>>>>> --- a/tests/i915/gem_ctx_persistence.c
>>>>>>>>> +++ b/tests/i915/gem_ctx_persistence.c
>>>>>>>>> @@ -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.
>>>>>>>
>>>>>>> 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?
>>>>>
>>>>> 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?
>>>
>>> __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.

Okay, sorry for the confusion:

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

Regards,

Tvrtko


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

Re: [igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Summers, Stuart]

[-- Attachment #1.1: Type: text/plain, Size: 6376 bytes --]

On Thu, 2019-12-05 at 10:23 +0000, Tvrtko Ursulin wrote:
> On 05/12/2019 10:09, Petri Latvala wrote:
> > On Thu, Dec 05, 2019 at 09:54:25AM +0000, Tvrtko Ursulin wrote:
> > > 
> > > On 05/12/2019 09:47, Andi Shyti wrote:
> > > > Hi,
> > > > 
> > > > On Thu, Dec 05, 2019 at 09:22:56AM +0000, Tvrtko Ursulin wrote:
> > > > > On 05/12/2019 09:01, Petri Latvala wrote:
> > > > > > On Wed, Dec 04, 2019 at 07:05:12PM +0000, Summers, Stuart
> > > > > > wrote:
> > > > > > > On Wed, 2019-12-04 at 16:53 +0200, Petri Latvala wrote:
> > > > > > > > On Wed, Dec 04, 2019 at 09:36:11AM +0000, Tvrtko
> > > > > > > > Ursulin wrote:
> > > > > > > > > 
> > > > > > > > > On 03/12/2019 05:11, Stuart Summers wrote:
> > > > > > > > > > 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 <
> > > > > > > > > > stuart.summers@intel.com>
> > > > > > > > > > ---
> > > > > > > > > >      tests/i915/gem_ctx_isolation.c   | 4 ++--
> > > > > > > > > >      tests/i915/gem_ctx_persistence.c | 2 +-
> > > > > > > > > >      2 files changed, 3 insertions(+), 3
> > > > > > > > > > deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/tests/i915/gem_ctx_isolation.c
> > > > > > > > > > b/tests/i915/gem_ctx_isolation.c
> > > > > > > > > > index 6aa27133..9435209e 100644
> > > > > > > > > > --- a/tests/i915/gem_ctx_isolation.c
> > > > > > > > > > +++ b/tests/i915/gem_ctx_isolation.c
> > > > > > > > > > @@ -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));
> > > > > > > > > > diff --git a/tests/i915/gem_ctx_persistence.c
> > > > > > > > > > b/tests/i915/gem_ctx_persistence.c
> > > > > > > > > > index d68431ae..30772159 100644
> > > > > > > > > > --- a/tests/i915/gem_ctx_persistence.c
> > > > > > > > > > +++ b/tests/i915/gem_ctx_persistence.c
> > > > > > > > > > @@ -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
> > > > > > > > > > g(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.
> > > > > > > > 
> > > > > > > > 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?
> > > > > > 
> > > > > > 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?
> > > > 
> > > > __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.
> 
> Okay, sorry for the confusion:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks for the review here and the discussion. Not sure I have push
rights here yet. Any chance for some help getting this merged?

Thanks,
Stuart

> 
> Regards,
> 
> Tvrtko
> 
> 

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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