[igt-dev] [PATCH i-g-t] lib: Fix intel_get_current_physical_engine() iterator

[igt-dev] [PATCH i-g-t] lib: Fix intel_get_current_physical_engine() iterator

[Chris Wilson]

If we run out of engines, intel_get_current_physical_engine() degrades
into an infinite loop as although it advanced the iterator, it did not
update its local engine pointer.

Reported-by: Petri Latvala <petri.latvala@intel.com>
Fixes: 17c77e7b0c3c ("lib/i915: add gem_engine_topology library and for_each loop definition")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Andi Shyti <andi.shyti@intel.com>
Cc: Petri Latvala <petri.latvala@intel.com>
---
 lib/i915/gem_engine_topology.c | 49 +++++-----------------------------
 lib/i915/gem_engine_topology.h | 36 ++++++++++++++++---------
 2 files changed, 29 insertions(+), 56 deletions(-)

diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
index fdd1b9516..17f67786f 100644
--- a/lib/i915/gem_engine_topology.c
+++ b/lib/i915/gem_engine_topology.c
@@ -81,11 +81,10 @@ static void ctx_map_engines(int fd, struct intel_engine_data *ed,
 			    struct drm_i915_gem_context_param *param)
 {
 	struct i915_context_param_engines *engines =
-			from_user_pointer(param->value);
+		from_user_pointer(param->value);
 	int i = 0;
 
-	for (typeof(engines->engines[0]) *p =
-	     &engines->engines[0];
+	for (struct i915_engine_class_instance *p = &engines->engines[0];
 	     i < ed->nengines; i++, p++) {
 		p->engine_class = ed->engines[i].class;
 		p->engine_instance = ed->engines[i].instance;
@@ -136,7 +135,7 @@ static void query_engine_list(int fd, struct intel_engine_data *ed)
 {
 	uint8_t buff[SIZEOF_QUERY] = { };
 	struct drm_i915_query_engine_info *query_engine =
-			(struct drm_i915_query_engine_info *) buff;
+		(struct drm_i915_query_engine_info *)buff;
 	int i;
 
 	query_engines(fd, query_engine, SIZEOF_QUERY);
@@ -149,41 +148,6 @@ static void query_engine_list(int fd, struct intel_engine_data *ed)
 	ed->nengines = query_engine->num_engines;
 }
 
-struct intel_execution_engine2 *
-intel_get_current_engine(struct intel_engine_data *ed)
-{
-	if (!ed->n)
-		ed->current_engine = &ed->engines[0];
-	else if (ed->n >= ed->nengines)
-		ed->current_engine = NULL;
-
-	return ed->current_engine;
-}
-
-void intel_next_engine(struct intel_engine_data *ed)
-{
-	if (ed->n + 1 < ed->nengines) {
-		ed->n++;
-		ed->current_engine = &ed->engines[ed->n];
-	} else {
-		ed->n = ed->nengines;
-		ed->current_engine = NULL;
-	}
-}
-
-struct intel_execution_engine2 *
-intel_get_current_physical_engine(struct intel_engine_data *ed)
-{
-	struct intel_execution_engine2 *e;
-
-	for (e = intel_get_current_engine(ed);
-	     e && e->is_virtual;
-	     intel_next_engine(ed))
-		;
-
-	return e;
-}
-
 static int gem_topology_get_param(int fd,
 				  struct drm_i915_gem_context_param *p)
 {
@@ -197,10 +161,9 @@ static int gem_topology_get_param(int fd,
 		return 0;
 
 	/* size will store the engine count */
-	p->size = (p->size - sizeof(struct i915_context_param_engines)) /
-		  (offsetof(struct i915_context_param_engines,
-			    engines[1]) -
-		  sizeof(struct i915_context_param_engines));
+	igt_assert(p->size >= sizeof(struct i915_context_param_engines));
+	p->size -= sizeof(struct i915_context_param_engines);
+	p->size /= sizeof(struct i915_engine_class_instance);
 
 	igt_assert_f(p->size <= GEM_MAX_ENGINES, "unsupported engine count\n");
 
diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
index 2415fd1e3..a1018afca 100644
--- a/lib/i915/gem_engine_topology.h
+++ b/lib/i915/gem_engine_topology.h
@@ -30,9 +30,7 @@
 #define GEM_MAX_ENGINES		I915_EXEC_RING_MASK + 1
 
 struct intel_engine_data {
-	uint32_t nengines;
-	uint32_t n;
-	struct intel_execution_engine2 *current_engine;
+	uint32_t nengines, cur;
 	struct intel_execution_engine2 engines[GEM_MAX_ENGINES];
 };
 
@@ -40,31 +38,43 @@ bool gem_has_engine_topology(int fd);
 struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id);
 
 /* iteration functions */
-struct intel_execution_engine2 *
-intel_get_current_engine(struct intel_engine_data *ed);
-
-struct intel_execution_engine2 *
-intel_get_current_physical_engine(struct intel_engine_data *ed);
-
-void intel_next_engine(struct intel_engine_data *ed);
-
 int gem_context_lookup_engine(int fd, uint64_t engine, uint32_t ctx_id,
 			      struct intel_execution_engine2 *e);
 
 void gem_context_set_all_engines(int fd, uint32_t ctx);
 
+static inline struct intel_execution_engine2 *
+intel_get_current_engine(struct intel_engine_data *ed)
+{
+	if (ed->cur >= ed->nengines)
+		return NULL;
+
+	return &ed->engines[ed->cur];
+}
+
+static inline struct intel_execution_engine2 *
+intel_get_current_physical_engine(struct intel_engine_data *ed)
+{
+	struct intel_execution_engine2 *e;
+
+	for (; (e = intel_get_current_engine(ed)) && e->is_virtual; ed->cur++)
+		;
+
+	return e;
+}
+
 #define __for_each_static_engine(e__) \
 	for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
 
 #define for_each_context_engine(fd__, ctx__, e__) \
 	for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
 	     ((e__) = intel_get_current_engine(&i__)); \
-	     intel_next_engine(&i__))
+	     i__.cur++)
 
 /* needs to replace "for_each_physical_engine" when conflicts are fixed */
 #define __for_each_physical_engine(fd__, e__) \
 	for (struct intel_engine_data i__ = intel_init_engine_list(fd__, 0); \
 	     ((e__) = intel_get_current_physical_engine(&i__)); \
-	     intel_next_engine(&i__))
+	     i__.cur++)
 
 #endif /* GEM_ENGINE_TOPOLOGY_H */
-- 
2.20.1

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

Re: [igt-dev] [PATCH i-g-t] lib: Fix intel_get_current_physical_engine() iterator

[Andi Shyti]

Hi Chris,

On Wed, May 29, 2019 at 02:24:21PM +0100, Chris Wilson wrote:
> If we run out of engines, intel_get_current_physical_engine() degrades
> into an infinite loop as although it advanced the iterator, it did not
> update its local engine pointer.

The patch looks like it does everything "but" what you say in the
commit log :)

> 
> Reported-by: Petri Latvala <petri.latvala@intel.com>
> Fixes: 17c77e7b0c3c ("lib/i915: add gem_engine_topology library and for_each loop definition")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> ---
>  lib/i915/gem_engine_topology.c | 49 +++++-----------------------------
>  lib/i915/gem_engine_topology.h | 36 ++++++++++++++++---------
>  2 files changed, 29 insertions(+), 56 deletions(-)
> 
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index fdd1b9516..17f67786f 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -81,11 +81,10 @@ static void ctx_map_engines(int fd, struct intel_engine_data *ed,
>  			    struct drm_i915_gem_context_param *param)
>  {
>  	struct i915_context_param_engines *engines =
> -			from_user_pointer(param->value);
> +		from_user_pointer(param->value);
>  	int i = 0;
>  
> -	for (typeof(engines->engines[0]) *p =
> -	     &engines->engines[0];
> +	for (struct i915_engine_class_instance *p = &engines->engines[0];
>  	     i < ed->nengines; i++, p++) {
>  		p->engine_class = ed->engines[i].class;
>  		p->engine_instance = ed->engines[i].instance;
> @@ -136,7 +135,7 @@ static void query_engine_list(int fd, struct intel_engine_data *ed)
>  {
>  	uint8_t buff[SIZEOF_QUERY] = { };
>  	struct drm_i915_query_engine_info *query_engine =
> -			(struct drm_i915_query_engine_info *) buff;
> +		(struct drm_i915_query_engine_info *)buff;

Until here, nothing is related to the description in the commit
log. Can we put the above in a different patch?

> -struct intel_execution_engine2 *
> -intel_get_current_engine(struct intel_engine_data *ed)
> -{
> -	if (!ed->n)
> -		ed->current_engine = &ed->engines[0];
> -	else if (ed->n >= ed->nengines)
> -		ed->current_engine = NULL;
> -
> -	return ed->current_engine;
> -}
> -
> -void intel_next_engine(struct intel_engine_data *ed)
> -{
> -	if (ed->n + 1 < ed->nengines) {
> -		ed->n++;
> -		ed->current_engine = &ed->engines[ed->n];
> -	} else {
> -		ed->n = ed->nengines;
> -		ed->current_engine = NULL;
> -	}
> -}
> -
> -struct intel_execution_engine2 *
> -intel_get_current_physical_engine(struct intel_engine_data *ed)
> -{
> -	struct intel_execution_engine2 *e;
> -
> -	for (e = intel_get_current_engine(ed);
> -	     e && e->is_virtual;
> -	     intel_next_engine(ed))
> -		;
> -
> -	return e;
> -}
> -

Moving these functions to inline in the header file is unrelated
to the patch topic, right?

>  static int gem_topology_get_param(int fd,
>  				  struct drm_i915_gem_context_param *p)
>  {
> @@ -197,10 +161,9 @@ static int gem_topology_get_param(int fd,
>  		return 0;
>  
>  	/* size will store the engine count */
> -	p->size = (p->size - sizeof(struct i915_context_param_engines)) /
> -		  (offsetof(struct i915_context_param_engines,
> -			    engines[1]) -
> -		  sizeof(struct i915_context_param_engines));
> +	igt_assert(p->size >= sizeof(struct i915_context_param_engines));
> +	p->size -= sizeof(struct i915_context_param_engines);
> +	p->size /= sizeof(struct i915_engine_class_instance);

This is also unrelated.

>  struct intel_engine_data {
> -	uint32_t nengines;
> -	uint32_t n;
> -	struct intel_execution_engine2 *current_engine;

so we don't have anymore current_engine... I had the feeling the
Tvrtko really wanted it :)

> +	uint32_t nengines, cur;

[ some copy paste ]

> -	uint32_t nengines;
> -	uint32_t n;
> +	uint32_t nengines, cur;

mmhhh... why?

> +static inline struct intel_execution_engine2 *
> +intel_get_current_engine(struct intel_engine_data *ed)
> +{
> +	if (ed->cur >= ed->nengines)
> +		return NULL;
> +
> +	return &ed->engines[ed->cur];
> +}
> +
> +static inline struct intel_execution_engine2 *
> +intel_get_current_physical_engine(struct intel_engine_data *ed)
> +{
> +	struct intel_execution_engine2 *e;
> +
> +	for (; (e = intel_get_current_engine(ed)) && e->is_virtual; ed->cur++)
> +		;

The above two lines are the only ones related to the commit
message.  Can we keep this patch smaller? and put cosmetics in a
different patchset?

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

[igt-dev] ✓ Fi.CI.BAT: success for lib: Fix intel_get_current_physical_engine() iterator

[Patchwork]

== Series Details ==

Series: lib: Fix intel_get_current_physical_engine() iterator
URL   : https://patchwork.freedesktop.org/series/61327/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6162 -> IGTPW_3074
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Possible fixes ####

  * igt@kms_chamelium@dp-edid-read:
    - fi-kbl-7500u:       [FAIL][1] ([fdo#109483] / [fdo#109635 ]) -> [PASS][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6162/fi-kbl-7500u/igt@kms_chamelium@dp-edid-read.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3074/fi-kbl-7500u/igt@kms_chamelium@dp-edid-read.html

  
  [fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483
  [fdo#109635 ]: https://bugs.freedesktop.org/show_bug.cgi?id=109635 


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

  Missing    (6): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-pnv-d510 fi-byt-clapper fi-bdw-samus 


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

  * IGT: IGT_5024 -> IGTPW_3074

  CI_DRM_6162: d35e150f090fed2b37052ace606850ed7780c34d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3074: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3074/
  IGT_5024: f414756be2ac57e194919973da7b86644ba61241 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

[igt-dev] ✓ Fi.CI.IGT: success for lib: Fix intel_get_current_physical_engine() iterator

[Patchwork]

== Series Details ==

Series: lib: Fix intel_get_current_physical_engine() iterator
URL   : https://patchwork.freedesktop.org/series/61327/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6162_full -> IGTPW_3074_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@vcs0-s3:
    - shard-glk:          [PASS][1] -> [INCOMPLETE][2] ([fdo#103359] / [k.org#198133])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6162/shard-glk6/igt@gem_ctx_isolation@vcs0-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3074/shard-glk1/igt@gem_ctx_isolation@vcs0-s3.html

  * igt@gem_persistent_relocs@forked-interruptible-faulting-reloc:
    - shard-hsw:          [PASS][3] -> [INCOMPLETE][4] ([fdo#103540]) +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6162/shard-hsw5/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3074/shard-hsw5/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc.html

  * igt@i915_pm_rc6_residency@rc6-accuracy:
    - shard-snb:          [PASS][5] -> [SKIP][6] ([fdo#109271])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6162/shard-snb2/igt@i915_pm_rc6_residency@rc6-accuracy.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3074/shard-snb5/igt@i915_pm_rc6_residency@rc6-accuracy.html

  * igt@i915_suspend@fence-restore-untiled:
    - shard-apl:          [PASS][7] -> [DMESG-WARN][8] ([fdo#108566])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6162/shard-apl3/igt@i915_suspend@fence-restore-untiled.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3074/shard-apl1/igt@i915_suspend@fence-restore-untiled.html

  * igt@i915_suspend@forcewake:
    - shard-kbl:          [PASS][9] -> [INCOMPLETE][10] ([fdo#103665])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6162/shard-kbl2/igt@i915_suspend@forcewake.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3074/shard-kbl6/igt@i915_suspend@forcewake.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [PASS][11] -> [FAIL][12] ([fdo#99912])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6162/shard-kbl3/igt@kms_setmode@basic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3074/shard-kbl2/igt@kms_setmode@basic.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-apl:          [DMESG-WARN][13] ([fdo#108566]) -> [PASS][14] +6 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6162/shard-apl3/igt@gem_ctx_isolation@rcs0-s3.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3074/shard-apl2/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-kbl:          [FAIL][15] ([fdo#108686]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6162/shard-kbl3/igt@gem_tiled_swapping@non-threaded.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3074/shard-kbl2/igt@gem_tiled_swapping@non-threaded.html

  * igt@kms_flip@2x-plain-flip-interruptible:
    - shard-hsw:          [SKIP][17] ([fdo#109271]) -> [PASS][18] +27 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6162/shard-hsw1/igt@kms_flip@2x-plain-flip-interruptible.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3074/shard-hsw1/igt@kms_flip@2x-plain-flip-interruptible.html

  
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (9 -> 5)
------------------------------

  Missing    (4): pig-skl-6260u shard-skl pig-hsw-4770r pig-glk-j5005 


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

  * IGT: IGT_5024 -> IGTPW_3074
  * Piglit: piglit_4509 -> None

  CI_DRM_6162: d35e150f090fed2b37052ace606850ed7780c34d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3074: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3074/
  IGT_5024: f414756be2ac57e194919973da7b86644ba61241 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [igt-dev] [PATCH i-g-t] lib: Fix intel_get_current_physical_engine() iterator

[Tvrtko Ursulin]

On 29/05/2019 14:24, Chris Wilson wrote:
> If we run out of engines, intel_get_current_physical_engine() degrades
> into an infinite loop as although it advanced the iterator, it did not
> update its local engine pointer.

We had one infinite loop in there already.. AFAIR it was on one engine 
platforms. Does the new incarnation happen actually via the 
__for_each_physical_engine iterator or perhaps only when calling 
intel_get_current_physical_engine after loop end? Why it wasn't seen in 
testing?

Regards,

Tvrtko

> Reported-by: Petri Latvala <petri.latvala@intel.com>
> Fixes: 17c77e7b0c3c ("lib/i915: add gem_engine_topology library and for_each loop definition")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> ---
>   lib/i915/gem_engine_topology.c | 49 +++++-----------------------------
>   lib/i915/gem_engine_topology.h | 36 ++++++++++++++++---------
>   2 files changed, 29 insertions(+), 56 deletions(-)
> 
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index fdd1b9516..17f67786f 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -81,11 +81,10 @@ static void ctx_map_engines(int fd, struct intel_engine_data *ed,
>   			    struct drm_i915_gem_context_param *param)
>   {
>   	struct i915_context_param_engines *engines =
> -			from_user_pointer(param->value);
> +		from_user_pointer(param->value);
>   	int i = 0;
>   
> -	for (typeof(engines->engines[0]) *p =
> -	     &engines->engines[0];
> +	for (struct i915_engine_class_instance *p = &engines->engines[0];
>   	     i < ed->nengines; i++, p++) {
>   		p->engine_class = ed->engines[i].class;
>   		p->engine_instance = ed->engines[i].instance;
> @@ -136,7 +135,7 @@ static void query_engine_list(int fd, struct intel_engine_data *ed)
>   {
>   	uint8_t buff[SIZEOF_QUERY] = { };
>   	struct drm_i915_query_engine_info *query_engine =
> -			(struct drm_i915_query_engine_info *) buff;
> +		(struct drm_i915_query_engine_info *)buff;
>   	int i;
>   
>   	query_engines(fd, query_engine, SIZEOF_QUERY);
> @@ -149,41 +148,6 @@ static void query_engine_list(int fd, struct intel_engine_data *ed)
>   	ed->nengines = query_engine->num_engines;
>   }
>   
> -struct intel_execution_engine2 *
> -intel_get_current_engine(struct intel_engine_data *ed)
> -{
> -	if (!ed->n)
> -		ed->current_engine = &ed->engines[0];
> -	else if (ed->n >= ed->nengines)
> -		ed->current_engine = NULL;
> -
> -	return ed->current_engine;
> -}
> -
> -void intel_next_engine(struct intel_engine_data *ed)
> -{
> -	if (ed->n + 1 < ed->nengines) {
> -		ed->n++;
> -		ed->current_engine = &ed->engines[ed->n];
> -	} else {
> -		ed->n = ed->nengines;
> -		ed->current_engine = NULL;
> -	}
> -}
> -
> -struct intel_execution_engine2 *
> -intel_get_current_physical_engine(struct intel_engine_data *ed)
> -{
> -	struct intel_execution_engine2 *e;
> -
> -	for (e = intel_get_current_engine(ed);
> -	     e && e->is_virtual;
> -	     intel_next_engine(ed))
> -		;
> -
> -	return e;
> -}
> -
>   static int gem_topology_get_param(int fd,
>   				  struct drm_i915_gem_context_param *p)
>   {
> @@ -197,10 +161,9 @@ static int gem_topology_get_param(int fd,
>   		return 0;
>   
>   	/* size will store the engine count */
> -	p->size = (p->size - sizeof(struct i915_context_param_engines)) /
> -		  (offsetof(struct i915_context_param_engines,
> -			    engines[1]) -
> -		  sizeof(struct i915_context_param_engines));
> +	igt_assert(p->size >= sizeof(struct i915_context_param_engines));
> +	p->size -= sizeof(struct i915_context_param_engines);
> +	p->size /= sizeof(struct i915_engine_class_instance);
>   
>   	igt_assert_f(p->size <= GEM_MAX_ENGINES, "unsupported engine count\n");
>   
> diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
> index 2415fd1e3..a1018afca 100644
> --- a/lib/i915/gem_engine_topology.h
> +++ b/lib/i915/gem_engine_topology.h
> @@ -30,9 +30,7 @@
>   #define GEM_MAX_ENGINES		I915_EXEC_RING_MASK + 1
>   
>   struct intel_engine_data {
> -	uint32_t nengines;
> -	uint32_t n;
> -	struct intel_execution_engine2 *current_engine;
> +	uint32_t nengines, cur;
>   	struct intel_execution_engine2 engines[GEM_MAX_ENGINES];
>   };
>   
> @@ -40,31 +38,43 @@ bool gem_has_engine_topology(int fd);
>   struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id);
>   
>   /* iteration functions */
> -struct intel_execution_engine2 *
> -intel_get_current_engine(struct intel_engine_data *ed);
> -
> -struct intel_execution_engine2 *
> -intel_get_current_physical_engine(struct intel_engine_data *ed);
> -
> -void intel_next_engine(struct intel_engine_data *ed);
> -
>   int gem_context_lookup_engine(int fd, uint64_t engine, uint32_t ctx_id,
>   			      struct intel_execution_engine2 *e);
>   
>   void gem_context_set_all_engines(int fd, uint32_t ctx);
>   
> +static inline struct intel_execution_engine2 *
> +intel_get_current_engine(struct intel_engine_data *ed)
> +{
> +	if (ed->cur >= ed->nengines)
> +		return NULL;
> +
> +	return &ed->engines[ed->cur];
> +}
> +
> +static inline struct intel_execution_engine2 *
> +intel_get_current_physical_engine(struct intel_engine_data *ed)
> +{
> +	struct intel_execution_engine2 *e;
> +
> +	for (; (e = intel_get_current_engine(ed)) && e->is_virtual; ed->cur++)
> +		;
> +
> +	return e;
> +}
> +
>   #define __for_each_static_engine(e__) \
>   	for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
>   
>   #define for_each_context_engine(fd__, ctx__, e__) \
>   	for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
>   	     ((e__) = intel_get_current_engine(&i__)); \
> -	     intel_next_engine(&i__))
> +	     i__.cur++)
>   
>   /* needs to replace "for_each_physical_engine" when conflicts are fixed */
>   #define __for_each_physical_engine(fd__, e__) \
>   	for (struct intel_engine_data i__ = intel_init_engine_list(fd__, 0); \
>   	     ((e__) = intel_get_current_physical_engine(&i__)); \
> -	     intel_next_engine(&i__))
> +	     i__.cur++)
>   
>   #endif /* GEM_ENGINE_TOPOLOGY_H */
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] lib: Fix intel_get_current_physical_engine() iterator

[Petri Latvala]

On Mon, Jun 03, 2019 at 11:19:48AM +0100, Tvrtko Ursulin wrote:
> 
> On 29/05/2019 14:24, Chris Wilson wrote:
> > If we run out of engines, intel_get_current_physical_engine() degrades
> > into an infinite loop as although it advanced the iterator, it did not
> > update its local engine pointer.
> 
> We had one infinite loop in there already.. AFAIR it was on one engine
> platforms. Does the new incarnation happen actually via the
> __for_each_physical_engine iterator or perhaps only when calling
> intel_get_current_physical_engine after loop end? Why it wasn't seen in
> testing?


The new incarnation happens with a wedged GPU. That's a case that's
hard to come by in testing.


-- 
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] lib: Fix intel_get_current_physical_engine() iterator

[Tvrtko Ursulin]

On 03/06/2019 11:32, Petri Latvala wrote:
> On Mon, Jun 03, 2019 at 11:19:48AM +0100, Tvrtko Ursulin wrote:
>>
>> On 29/05/2019 14:24, Chris Wilson wrote:
>>> If we run out of engines, intel_get_current_physical_engine() degrades
>>> into an infinite loop as although it advanced the iterator, it did not
>>> update its local engine pointer.
>>
>> We had one infinite loop in there already.. AFAIR it was on one engine
>> platforms. Does the new incarnation happen actually via the
>> __for_each_physical_engine iterator or perhaps only when calling
>> intel_get_current_physical_engine after loop end? Why it wasn't seen in
>> testing?
> 
> 
> The new incarnation happens with a wedged GPU. That's a case that's
> hard to come by in testing.

1.
Colour me confused. :) How does a wedged GPU affect this loop?

2.
Are we missing a test? Wedge-do-stuff-unwedge should be easy to write.

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] lib: Fix intel_get_current_physical_engine() iterator

[Petri Latvala]

On Mon, Jun 03, 2019 at 11:39:25AM +0100, Tvrtko Ursulin wrote:
> 
> On 03/06/2019 11:32, Petri Latvala wrote:
> > On Mon, Jun 03, 2019 at 11:19:48AM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 29/05/2019 14:24, Chris Wilson wrote:
> > > > If we run out of engines, intel_get_current_physical_engine() degrades
> > > > into an infinite loop as although it advanced the iterator, it did not
> > > > update its local engine pointer.
> > > 
> > > We had one infinite loop in there already.. AFAIR it was on one engine
> > > platforms. Does the new incarnation happen actually via the
> > > __for_each_physical_engine iterator or perhaps only when calling
> > > intel_get_current_physical_engine after loop end? Why it wasn't seen in
> > > testing?
> > 
> > 
> > The new incarnation happens with a wedged GPU. That's a case that's
> > hard to come by in testing.
> 
> 1.
> Colour me confused. :) How does a wedged GPU affect this loop?

Wedging could be a red herring, but regardless the GPU was in a funky
state.

An easy reproduction method is just

#  ./perf_pmu

(as normal user, not root!)


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] lib: Fix intel_get_current_physical_engine() iterator

[Tvrtko Ursulin]

On 03/06/2019 12:19, Petri Latvala wrote:
> On Mon, Jun 03, 2019 at 11:39:25AM +0100, Tvrtko Ursulin wrote:
>>
>> On 03/06/2019 11:32, Petri Latvala wrote:
>>> On Mon, Jun 03, 2019 at 11:19:48AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 29/05/2019 14:24, Chris Wilson wrote:
>>>>> If we run out of engines, intel_get_current_physical_engine() degrades
>>>>> into an infinite loop as although it advanced the iterator, it did not
>>>>> update its local engine pointer.
>>>>
>>>> We had one infinite loop in there already.. AFAIR it was on one engine
>>>> platforms. Does the new incarnation happen actually via the
>>>> __for_each_physical_engine iterator or perhaps only when calling
>>>> intel_get_current_physical_engine after loop end? Why it wasn't seen in
>>>> testing?
>>>
>>>
>>> The new incarnation happens with a wedged GPU. That's a case that's
>>> hard to come by in testing.
>>
>> 1.
>> Colour me confused. :) How does a wedged GPU affect this loop?
> 
> Wedging could be a red herring, but regardless the GPU was in a funky
> state.
> 
> An easy reproduction method is just
> 
> #  ./perf_pmu
> 
> (as normal user, not root!)

See it now, so the problem is code is not capable of handling zero 
engines (which it happens if no access to drm master like in your example).

As such, Chris' patch looks okay to me.

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

Regards,

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