All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery
@ 2019-07-30  3:58 Ramalingam C
  2019-07-30 11:27 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Ramalingam C @ 2019-07-30  3:58 UTC (permalink / raw)
  To: intel-gfx

Legacy execbuf abi tests are prefixed with legacy. New test are added to
run on physical engines accessed through engine discovery.

So legacy tests run on the unconfigured (with engine map) context and
use a new helper gem_eb_flags_to_engine to look up the engine from the
intel_execution_engines2 static list. This is only to enable the core
test code to be shared.

Places where new contexts are created had to be updated to either
equally configure the contexts or not.

v2:
  retained the test as it is for legacy uapi testing and duplciated for
	new engine discovery [Tvrtko]
v3:
  Few nits addressed [Tvrtko]
v4:
  In legacy uAPI test path, iterate through for_each_engine [Tvrtko]
v5:
  Function for exec_flag comparison [Tvrtko]

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 lib/igt_gt.c                | 25 ++++++++++++++++++
 lib/igt_gt.h                |  1 +
 tests/i915/gem_exec_async.c | 52 +++++++++++++++++++++++++++----------
 3 files changed, 64 insertions(+), 14 deletions(-)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index 78e3cd08925b..f594a46282bc 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -633,3 +633,28 @@ bool gem_ring_has_physical_engine(int fd, unsigned ring)
 
 	return gem_has_ring(fd, ring);
 }
+
+bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2)
+{
+	if (eb_flag1 == eb_flag2)
+		return false;
+
+	/* DEFAULT stands for RENDER in legacy uAPI*/
+	if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
+	     (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
+		return false;
+
+	/*
+	 * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
+	 * same engine.
+	 */
+	if ((eb_flag1 == I915_EXEC_BSD) &&
+	    (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
+			return false;
+
+	if ((eb_flag2 == I915_EXEC_BSD) &&
+	    (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
+			return false;
+
+	return true;
+}
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index 73b5002a04bd..760b8baefc48 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -101,5 +101,6 @@ extern const struct intel_execution_engine2 {
 } intel_execution_engines2[];
 
 int gem_execbuf_flags_to_engine_class(unsigned int flags);
+bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2);
 
 #endif /* IGT_GT_H */
diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
index 9a06af7e29cb..5fdce5ac5b77 100644
--- a/tests/i915/gem_exec_async.c
+++ b/tests/i915/gem_exec_async.c
@@ -80,9 +80,10 @@ static void store_dword(int fd, unsigned ring,
 	gem_close(fd, obj[1].handle);
 }
 
-static void one(int fd, unsigned ring, uint32_t flags)
+static void one(int fd, const struct intel_execution_engine2 *e2, bool legacy)
 {
 	const int gen = intel_gen(intel_get_drm_devid(fd));
+	const struct intel_execution_engine2 *other2;
 	struct drm_i915_gem_exec_object2 obj[2];
 #define SCRATCH 0
 #define BATCH 1
@@ -138,20 +139,33 @@ static void one(int fd, unsigned ring, uint32_t flags)
 	memset(&execbuf, 0, sizeof(execbuf));
 	execbuf.buffers_ptr = to_user_pointer(obj);
 	execbuf.buffer_count = 2;
-	execbuf.flags = ring | flags;
+	execbuf.flags = e2->flags;
 	igt_require(__gem_execbuf(fd, &execbuf) == 0);
 	gem_close(fd, obj[BATCH].handle);
 
 	i = 0;
-	for_each_physical_engine(fd, other) {
-		if (other == ring)
-			continue;
+	if (legacy) {
+		for_each_engine(fd, other) {
+			if (gem_eb_flags_are_different_engines(e2->flags, other))
+				continue;
 
-		if (!gem_can_store_dword(fd, other))
-			continue;
+			if (!gem_can_store_dword(fd, other))
+				continue;
+
+			store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
+			i++;
+		}
+	} else {
+		__for_each_physical_engine(fd, other2) {
+			if (gem_engine_is_equal(e2, other2))
+				continue;
 
-		store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
-		i++;
+			if (!gem_class_can_store_dword(fd, other2->class))
+				continue;
+
+			store_dword(fd, other2->flags, obj[SCRATCH].handle, 4*i, i);
+			i++;
+		}
 	}
 
 	*batch = MI_BATCH_BUFFER_END;
@@ -185,7 +199,9 @@ static bool has_async_execbuf(int fd)
 
 igt_main
 {
+	const struct intel_execution_engine2 *e2;
 	const struct intel_execution_engine *e;
+	struct intel_execution_engine2 e2__;
 	int fd = -1;
 
 	igt_skip_on_simulation();
@@ -200,14 +216,22 @@ igt_main
 	}
 
 	for (e = intel_execution_engines; e->name; e++) {
-		/* default exec-id is purely symbolic */
-		if (e->exec_id == 0)
+		e2__ = gem_eb_flags_to_engine(e->exec_id | e->flags);
+		if (e2__.flags == -1)
 			continue;
+		e2 = &e2__;
 
-		igt_subtest_f("concurrent-writes-%s", e->name) {
+		igt_subtest_f("legacy-concurrent-writes-%s", e2->name) {
 			igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
-			igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
-			one(fd, e->exec_id, e->flags);
+			igt_require(gem_class_can_store_dword(fd, e2->class));
+			one(fd, e2, true);
+		}
+	}
+
+	__for_each_physical_engine(fd, e2) {
+		igt_subtest_f("concurrent-writes-%s", e2->name) {
+			igt_require(gem_class_can_store_dword(fd, e2->class));
+			one(fd, e2, false);
 		}
 	}
 
-- 
2.19.1

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery
  2019-07-30 12:05 ` [PATCH i-g-t v5] " Tvrtko Ursulin
@ 2019-07-30  8:04   ` Ramalingam C
  2019-07-30 15:20     ` Tvrtko Ursulin
  0 siblings, 1 reply; 14+ messages in thread
From: Ramalingam C @ 2019-07-30  8:04 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

On 2019-07-30 at 13:05:27 +0100, Tvrtko Ursulin wrote:
> 
> On 30/07/2019 04:58, Ramalingam C wrote:
> > Legacy execbuf abi tests are prefixed with legacy. New test are added to
> > run on physical engines accessed through engine discovery.
> > 
> > So legacy tests run on the unconfigured (with engine map) context and
> > use a new helper gem_eb_flags_to_engine to look up the engine from the
> > intel_execution_engines2 static list. This is only to enable the core
> > test code to be shared.
> > 
> > Places where new contexts are created had to be updated to either
> > equally configure the contexts or not.
> > 
> > v2:
> >    retained the test as it is for legacy uapi testing and duplciated for
> > 	new engine discovery [Tvrtko]
> > v3:
> >    Few nits addressed [Tvrtko]
> > v4:
> >    In legacy uAPI test path, iterate through for_each_engine [Tvrtko]
> > v5:
> >    Function for exec_flag comparison [Tvrtko]
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > ---
> >   lib/igt_gt.c                | 25 ++++++++++++++++++
> >   lib/igt_gt.h                |  1 +
> >   tests/i915/gem_exec_async.c | 52 +++++++++++++++++++++++++++----------
> >   3 files changed, 64 insertions(+), 14 deletions(-)
> > 
> > diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> > index 78e3cd08925b..f594a46282bc 100644
> > --- a/lib/igt_gt.c
> > +++ b/lib/igt_gt.c
> > @@ -633,3 +633,28 @@ bool gem_ring_has_physical_engine(int fd, unsigned ring)
> >   
> >   	return gem_has_ring(fd, ring);
> >   }
> > +
> > +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2)
> 
> I think we try to avoid implied int but not sure in this case whether to suggest unsigned int, long or uint64_t. If we are suggesting in the function name that any flags can be passed in perhaps it should be uint64_t and then we filter on the engine bits (flags.. &= I915_EXEC_RING_MASK | (3 << 13)) before checking. Yeah, I think that would be more robust for a generic helper.
> 
> And add a doc blurb for this helper since it is non-obvious why we went for different and not same. My thinking was the name different would be clearer to express kind of tri-state nature of this check. (Flags may be different, but engines are not guaranteed to be different.) Have I over-complicated it? Do we need to make it clearer by naming it gem_eb_flags_are_guaranteed_different_engines? :)
For me current shape looks good enough :) I will use the uint64_t for
parameter types.
> 
> > +{
> > +	if (eb_flag1 == eb_flag2)
> > +		return false;
> > +
> > +	/* DEFAULT stands for RENDER in legacy uAPI*/
> > +	if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
> > +	     (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
> > +		return false;
> > +
> > +	/*
> > +	 * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
> > +	 * same engine.
> > +	 */
> > +	if ((eb_flag1 == I915_EXEC_BSD) &&
> > +	    (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> > +			return false;
> > +
> > +	if ((eb_flag2 == I915_EXEC_BSD) &&
> > +	    (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> > +			return false;
> 
> I think this works.
> 
> I've also come up with something to merge the two checks, not 100% it's correct or more readable:
> 
> if (((flag1 | flag2) & I915_EXEC_RING_MASK)) == I915_EXEC_BSD && // at least one is BSD
>     !((flag1 ^ flag2) & I915_EXEC_RING_MASK) && // both are BSD
>     (((flag1 | flag2) & (3 << 13))) != 3) // not guaranteed different
> 	return false;
> 
> Would need feeding in some values and checking it works as expected. Probably not worth it since I doubt it is more readable.
readability perspective, we could stick to the original version. If we
want to go ahead we need to do below ops:

if (((flag1 | flag2) & I915_EXEC_BSD) &&  //Atleast one is BSD
    !((flag1 ^ flag2) & I915_EXEC_BSD) && //Both are BSD
    ((flag1 | flag2) & I915_EXEC_RING_MASK) != I915_EXEC_RING_MASK) //Not BSD0 and BSD1
	return false;
> 
> > +
> > +	return true;
> > +}
> > diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> > index 73b5002a04bd..760b8baefc48 100644
> > --- a/lib/igt_gt.h
> > +++ b/lib/igt_gt.h
> > @@ -101,5 +101,6 @@ extern const struct intel_execution_engine2 {
> >   } intel_execution_engines2[];
> >   
> >   int gem_execbuf_flags_to_engine_class(unsigned int flags);
> > +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2);
> >   
> >   #endif /* IGT_GT_H */
> > diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
> > index 9a06af7e29cb..5fdce5ac5b77 100644
> > --- a/tests/i915/gem_exec_async.c
> > +++ b/tests/i915/gem_exec_async.c
> > @@ -80,9 +80,10 @@ static void store_dword(int fd, unsigned ring,
> >   	gem_close(fd, obj[1].handle);
> >   }
> >   
> > -static void one(int fd, unsigned ring, uint32_t flags)
> > +static void one(int fd, const struct intel_execution_engine2 *e2, bool legacy)
> >   {
> >   	const int gen = intel_gen(intel_get_drm_devid(fd));
> > +	const struct intel_execution_engine2 *other2;
> >   	struct drm_i915_gem_exec_object2 obj[2];
> >   #define SCRATCH 0
> >   #define BATCH 1
> > @@ -138,20 +139,33 @@ static void one(int fd, unsigned ring, uint32_t flags)
> >   	memset(&execbuf, 0, sizeof(execbuf));
> >   	execbuf.buffers_ptr = to_user_pointer(obj);
> >   	execbuf.buffer_count = 2;
> > -	execbuf.flags = ring | flags;
> > +	execbuf.flags = e2->flags;
> >   	igt_require(__gem_execbuf(fd, &execbuf) == 0);
> >   	gem_close(fd, obj[BATCH].handle);
> >   
> >   	i = 0;
> > -	for_each_physical_engine(fd, other) {
> > -		if (other == ring)
> > -			continue;
> > +	if (legacy) {
> > +		for_each_engine(fd, other) {
> > +			if (gem_eb_flags_are_different_engines(e2->flags, other))
> 
> if (!different)
Will fix it. Thanks.

-Ram
> 
> Regards,
> 
> Tvrtko
> 
> > +				continue;
> >   
> > -		if (!gem_can_store_dword(fd, other))
> > -			continue;
> > +			if (!gem_can_store_dword(fd, other))
> > +				continue;
> > +
> > +			store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
> > +			i++;
> > +		}
> > +	} else {
> > +		__for_each_physical_engine(fd, other2) {
> > +			if (gem_engine_is_equal(e2, other2))
> > +				continue;
> >   
> > -		store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
> > -		i++;
> > +			if (!gem_class_can_store_dword(fd, other2->class))
> > +				continue;
> > +
> > +			store_dword(fd, other2->flags, obj[SCRATCH].handle, 4*i, i);
> > +			i++;
> > +		}
> >   	}
> >   
> >   	*batch = MI_BATCH_BUFFER_END;
> > @@ -185,7 +199,9 @@ static bool has_async_execbuf(int fd)
> >   
> >   igt_main
> >   {
> > +	const struct intel_execution_engine2 *e2;
> >   	const struct intel_execution_engine *e;
> > +	struct intel_execution_engine2 e2__;
> >   	int fd = -1;
> >   
> >   	igt_skip_on_simulation();
> > @@ -200,14 +216,22 @@ igt_main
> >   	}
> >   
> >   	for (e = intel_execution_engines; e->name; e++) {
> > -		/* default exec-id is purely symbolic */
> > -		if (e->exec_id == 0)
> > +		e2__ = gem_eb_flags_to_engine(e->exec_id | e->flags);
> > +		if (e2__.flags == -1)
> >   			continue;
> > +		e2 = &e2__;
> >   
> > -		igt_subtest_f("concurrent-writes-%s", e->name) {
> > +		igt_subtest_f("legacy-concurrent-writes-%s", e2->name) {
> >   			igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
> > -			igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
> > -			one(fd, e->exec_id, e->flags);
> > +			igt_require(gem_class_can_store_dword(fd, e2->class));
> > +			one(fd, e2, true);
> > +		}
> > +	}
> > +
> > +	__for_each_physical_engine(fd, e2) {
> > +		igt_subtest_f("concurrent-writes-%s", e2->name) {
> > +			igt_require(gem_class_can_store_dword(fd, e2->class));
> > +			one(fd, e2, false);
> >   		}
> >   	}
> >   
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* ✓ Fi.CI.BAT: success for tests/i915/gem_exec_async: Update with engine discovery
  2019-07-30  3:58 [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery Ramalingam C
@ 2019-07-30 11:27 ` Patchwork
  2019-07-30 12:05 ` [PATCH i-g-t v5] " Tvrtko Ursulin
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2019-07-30 11:27 UTC (permalink / raw)
  To: Ramalingam C; +Cc: intel-gfx

== Series Details ==

Series: tests/i915/gem_exec_async: Update with engine discovery
URL   : https://patchwork.freedesktop.org/series/64425/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6580 -> IGTPW_3308
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_hangcheck:
    - fi-icl-u2:          [PASS][1] -> [INCOMPLETE][2] ([fdo#107713] / [fdo#108569])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/fi-icl-u2/igt@i915_selftest@live_hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/fi-icl-u2/igt@i915_selftest@live_hangcheck.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-kbl-7500u:       [PASS][3] -> [SKIP][4] ([fdo#109271]) +23 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/fi-kbl-7500u/igt@prime_vgem@basic-fence-flip.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/fi-kbl-7500u/igt@prime_vgem@basic-fence-flip.html

  
#### Possible fixes ####

  * {igt@gem_ctx_switch@legacy-render}:
    - {fi-icl-guc}:       [INCOMPLETE][5] ([fdo#107713]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/fi-icl-guc/igt@gem_ctx_switch@legacy-render.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/fi-icl-guc/igt@gem_ctx_switch@legacy-render.html

  * igt@gem_render_tiled_blits@basic:
    - fi-icl-dsi:         [INCOMPLETE][7] ([fdo#107713]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/fi-icl-dsi/igt@gem_render_tiled_blits@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/fi-icl-dsi/igt@gem_render_tiled_blits@basic.html

  * igt@i915_selftest@live_execlists:
    - fi-skl-gvtdvm:      [DMESG-FAIL][9] ([fdo#111108]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/fi-skl-gvtdvm/igt@i915_selftest@live_execlists.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/fi-skl-gvtdvm/igt@i915_selftest@live_execlists.html

  * igt@kms_busy@basic-flip-c:
    - fi-kbl-7500u:       [SKIP][11] ([fdo#109271] / [fdo#109278]) -> [PASS][12] +2 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/fi-kbl-7500u/igt@kms_busy@basic-flip-c.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/fi-kbl-7500u/igt@kms_busy@basic-flip-c.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       [WARN][13] ([fdo#109380]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/fi-kbl-7567u/igt@kms_chamelium@common-hpd-after-suspend.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/fi-kbl-7567u/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][15] ([fdo#109485]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
    - fi-kbl-7567u:       [FAIL][17] ([fdo#109485]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/fi-kbl-7567u/igt@kms_chamelium@hdmi-hpd-fast.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/fi-kbl-7567u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-c:
    - fi-kbl-7567u:       [SKIP][19] ([fdo#109271]) -> [PASS][20] +23 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/fi-kbl-7567u/igt@kms_pipe_crc_basic@read-crc-pipe-c.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/fi-kbl-7567u/igt@kms_pipe_crc_basic@read-crc-pipe-c.html

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

  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109380]: https://bugs.freedesktop.org/show_bug.cgi?id=109380
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
  [fdo#111108]: https://bugs.freedesktop.org/show_bug.cgi?id=111108


Participating hosts (54 -> 45)
------------------------------

  Additional (1): fi-icl-u3 
  Missing    (10): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-skl-6770hq fi-byt-squawks fi-bsw-cyan fi-icl-y fi-skl-lmem fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * IGT: IGT_5115 -> IGTPW_3308

  CI-20190529: 20190529
  CI_DRM_6580: 95a5a5fb8d2de1f66a8c57941ae823b82b75708d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3308: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/
  IGT_5115: 21be7a02ac8a8ff46b561c36a69e4dd5a0c2938b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+igt@gem_exec_async@concurrent-writes-bcs0
+igt@gem_exec_async@concurrent-writes-rcs0
+igt@gem_exec_async@concurrent-writes-vcs0
+igt@gem_exec_async@concurrent-writes-vcs1
+igt@gem_exec_async@concurrent-writes-vcs2
+igt@gem_exec_async@concurrent-writes-vecs0
+igt@gem_exec_async@legacy-concurrent-writes-bcs0
+igt@gem_exec_async@legacy-concurrent-writes-default
+igt@gem_exec_async@legacy-concurrent-writes-rcs0
+igt@gem_exec_async@legacy-concurrent-writes-vcs0
+igt@gem_exec_async@legacy-concurrent-writes-vcs1
+igt@gem_exec_async@legacy-concurrent-writes-vecs0
-igt@gem_exec_async@concurrent-writes-blt
-igt@gem_exec_async@concurrent-writes-bsd
-igt@gem_exec_async@concurrent-writes-bsd1
-igt@gem_exec_async@concurrent-writes-bsd2
-igt@gem_exec_async@concurrent-writes-render
-igt@gem_exec_async@concurrent-writes-vebox

== Logs ==

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery
  2019-07-30  3:58 [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery Ramalingam C
  2019-07-30 11:27 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2019-07-30 12:05 ` Tvrtko Ursulin
  2019-07-30  8:04   ` Ramalingam C
  2019-07-30 19:03 ` ✓ Fi.CI.IGT: success for " Patchwork
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Tvrtko Ursulin @ 2019-07-30 12:05 UTC (permalink / raw)
  To: Ramalingam C, intel-gfx


On 30/07/2019 04:58, Ramalingam C wrote:
> Legacy execbuf abi tests are prefixed with legacy. New test are added to
> run on physical engines accessed through engine discovery.
> 
> So legacy tests run on the unconfigured (with engine map) context and
> use a new helper gem_eb_flags_to_engine to look up the engine from the
> intel_execution_engines2 static list. This is only to enable the core
> test code to be shared.
> 
> Places where new contexts are created had to be updated to either
> equally configure the contexts or not.
> 
> v2:
>    retained the test as it is for legacy uapi testing and duplciated for
> 	new engine discovery [Tvrtko]
> v3:
>    Few nits addressed [Tvrtko]
> v4:
>    In legacy uAPI test path, iterate through for_each_engine [Tvrtko]
> v5:
>    Function for exec_flag comparison [Tvrtko]
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>   lib/igt_gt.c                | 25 ++++++++++++++++++
>   lib/igt_gt.h                |  1 +
>   tests/i915/gem_exec_async.c | 52 +++++++++++++++++++++++++++----------
>   3 files changed, 64 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 78e3cd08925b..f594a46282bc 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -633,3 +633,28 @@ bool gem_ring_has_physical_engine(int fd, unsigned ring)
>   
>   	return gem_has_ring(fd, ring);
>   }
> +
> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2)

I think we try to avoid implied int but not sure in this case whether to suggest unsigned int, long or uint64_t. If we are suggesting in the function name that any flags can be passed in perhaps it should be uint64_t and then we filter on the engine bits (flags.. &= I915_EXEC_RING_MASK | (3 << 13)) before checking. Yeah, I think that would be more robust for a generic helper.

And add a doc blurb for this helper since it is non-obvious why we went for different and not same. My thinking was the name different would be clearer to express kind of tri-state nature of this check. (Flags may be different, but engines are not guaranteed to be different.) Have I over-complicated it? Do we need to make it clearer by naming it gem_eb_flags_are_guaranteed_different_engines? :)

> +{
> +	if (eb_flag1 == eb_flag2)
> +		return false;
> +
> +	/* DEFAULT stands for RENDER in legacy uAPI*/
> +	if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
> +	     (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
> +		return false;
> +
> +	/*
> +	 * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
> +	 * same engine.
> +	 */
> +	if ((eb_flag1 == I915_EXEC_BSD) &&
> +	    (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> +			return false;
> +
> +	if ((eb_flag2 == I915_EXEC_BSD) &&
> +	    (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> +			return false;

I think this works.

I've also come up with something to merge the two checks, not 100% it's correct or more readable:

if (((flag1 | flag2) & I915_EXEC_RING_MASK)) == I915_EXEC_BSD && // at least one is BSD
    !((flag1 ^ flag2) & I915_EXEC_RING_MASK) && // both are BSD
    (((flag1 | flag2) & (3 << 13))) != 3) // not guaranteed different
	return false;

Would need feeding in some values and checking it works as expected. Probably not worth it since I doubt it is more readable.

> +
> +	return true;
> +}
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 73b5002a04bd..760b8baefc48 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -101,5 +101,6 @@ extern const struct intel_execution_engine2 {
>   } intel_execution_engines2[];
>   
>   int gem_execbuf_flags_to_engine_class(unsigned int flags);
> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2);
>   
>   #endif /* IGT_GT_H */
> diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
> index 9a06af7e29cb..5fdce5ac5b77 100644
> --- a/tests/i915/gem_exec_async.c
> +++ b/tests/i915/gem_exec_async.c
> @@ -80,9 +80,10 @@ static void store_dword(int fd, unsigned ring,
>   	gem_close(fd, obj[1].handle);
>   }
>   
> -static void one(int fd, unsigned ring, uint32_t flags)
> +static void one(int fd, const struct intel_execution_engine2 *e2, bool legacy)
>   {
>   	const int gen = intel_gen(intel_get_drm_devid(fd));
> +	const struct intel_execution_engine2 *other2;
>   	struct drm_i915_gem_exec_object2 obj[2];
>   #define SCRATCH 0
>   #define BATCH 1
> @@ -138,20 +139,33 @@ static void one(int fd, unsigned ring, uint32_t flags)
>   	memset(&execbuf, 0, sizeof(execbuf));
>   	execbuf.buffers_ptr = to_user_pointer(obj);
>   	execbuf.buffer_count = 2;
> -	execbuf.flags = ring | flags;
> +	execbuf.flags = e2->flags;
>   	igt_require(__gem_execbuf(fd, &execbuf) == 0);
>   	gem_close(fd, obj[BATCH].handle);
>   
>   	i = 0;
> -	for_each_physical_engine(fd, other) {
> -		if (other == ring)
> -			continue;
> +	if (legacy) {
> +		for_each_engine(fd, other) {
> +			if (gem_eb_flags_are_different_engines(e2->flags, other))

if (!different)

Regards,

Tvrtko

> +				continue;
>   
> -		if (!gem_can_store_dword(fd, other))
> -			continue;
> +			if (!gem_can_store_dword(fd, other))
> +				continue;
> +
> +			store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
> +			i++;
> +		}
> +	} else {
> +		__for_each_physical_engine(fd, other2) {
> +			if (gem_engine_is_equal(e2, other2))
> +				continue;
>   
> -		store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
> -		i++;
> +			if (!gem_class_can_store_dword(fd, other2->class))
> +				continue;
> +
> +			store_dword(fd, other2->flags, obj[SCRATCH].handle, 4*i, i);
> +			i++;
> +		}
>   	}
>   
>   	*batch = MI_BATCH_BUFFER_END;
> @@ -185,7 +199,9 @@ static bool has_async_execbuf(int fd)
>   
>   igt_main
>   {
> +	const struct intel_execution_engine2 *e2;
>   	const struct intel_execution_engine *e;
> +	struct intel_execution_engine2 e2__;
>   	int fd = -1;
>   
>   	igt_skip_on_simulation();
> @@ -200,14 +216,22 @@ igt_main
>   	}
>   
>   	for (e = intel_execution_engines; e->name; e++) {
> -		/* default exec-id is purely symbolic */
> -		if (e->exec_id == 0)
> +		e2__ = gem_eb_flags_to_engine(e->exec_id | e->flags);
> +		if (e2__.flags == -1)
>   			continue;
> +		e2 = &e2__;
>   
> -		igt_subtest_f("concurrent-writes-%s", e->name) {
> +		igt_subtest_f("legacy-concurrent-writes-%s", e2->name) {
>   			igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
> -			igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
> -			one(fd, e->exec_id, e->flags);
> +			igt_require(gem_class_can_store_dword(fd, e2->class));
> +			one(fd, e2, true);
> +		}
> +	}
> +
> +	__for_each_physical_engine(fd, e2) {
> +		igt_subtest_f("concurrent-writes-%s", e2->name) {
> +			igt_require(gem_class_can_store_dword(fd, e2->class));
> +			one(fd, e2, false);
>   		}
>   	}
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery
  2019-07-30  8:04   ` Ramalingam C
@ 2019-07-30 15:20     ` Tvrtko Ursulin
  2019-07-30 17:07       ` Chris Wilson
  0 siblings, 1 reply; 14+ messages in thread
From: Tvrtko Ursulin @ 2019-07-30 15:20 UTC (permalink / raw)
  To: Ramalingam C; +Cc: intel-gfx


On 30/07/2019 09:04, Ramalingam C wrote:
> On 2019-07-30 at 13:05:27 +0100, Tvrtko Ursulin wrote:
>>
>> On 30/07/2019 04:58, Ramalingam C wrote:
>>> Legacy execbuf abi tests are prefixed with legacy. New test are added to
>>> run on physical engines accessed through engine discovery.
>>>
>>> So legacy tests run on the unconfigured (with engine map) context and
>>> use a new helper gem_eb_flags_to_engine to look up the engine from the
>>> intel_execution_engines2 static list. This is only to enable the core
>>> test code to be shared.
>>>
>>> Places where new contexts are created had to be updated to either
>>> equally configure the contexts or not.
>>>
>>> v2:
>>>     retained the test as it is for legacy uapi testing and duplciated for
>>> 	new engine discovery [Tvrtko]
>>> v3:
>>>     Few nits addressed [Tvrtko]
>>> v4:
>>>     In legacy uAPI test path, iterate through for_each_engine [Tvrtko]
>>> v5:
>>>     Function for exec_flag comparison [Tvrtko]
>>>
>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>> ---
>>>    lib/igt_gt.c                | 25 ++++++++++++++++++
>>>    lib/igt_gt.h                |  1 +
>>>    tests/i915/gem_exec_async.c | 52 +++++++++++++++++++++++++++----------
>>>    3 files changed, 64 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
>>> index 78e3cd08925b..f594a46282bc 100644
>>> --- a/lib/igt_gt.c
>>> +++ b/lib/igt_gt.c
>>> @@ -633,3 +633,28 @@ bool gem_ring_has_physical_engine(int fd, unsigned ring)
>>>    
>>>    	return gem_has_ring(fd, ring);
>>>    }
>>> +
>>> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2)
>>
>> I think we try to avoid implied int but not sure in this case whether to suggest unsigned int, long or uint64_t. If we are suggesting in the function name that any flags can be passed in perhaps it should be uint64_t and then we filter on the engine bits (flags.. &= I915_EXEC_RING_MASK | (3 << 13)) before checking. Yeah, I think that would be more robust for a generic helper.
>>
>> And add a doc blurb for this helper since it is non-obvious why we went for different and not same. My thinking was the name different would be clearer to express kind of tri-state nature of this check. (Flags may be different, but engines are not guaranteed to be different.) Have I over-complicated it? Do we need to make it clearer by naming it gem_eb_flags_are_guaranteed_different_engines? :)
> For me current shape looks good enough :) I will use the uint64_t for
> parameter types.

Okay but please add some documentation for the helper (we've been very 
bad in this work in this respect so far) and also filter out non-engine 
selection bits from the flags before doing the checks.

>>
>>> +{
>>> +	if (eb_flag1 == eb_flag2)
>>> +		return false;
>>> +
>>> +	/* DEFAULT stands for RENDER in legacy uAPI*/
>>> +	if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
>>> +	     (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
>>> +		return false;
>>> +
>>> +	/*
>>> +	 * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
>>> +	 * same engine.
>>> +	 */
>>> +	if ((eb_flag1 == I915_EXEC_BSD) &&
>>> +	    (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
>>> +			return false;
>>> +
>>> +	if ((eb_flag2 == I915_EXEC_BSD) &&
>>> +	    (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
>>> +			return false;
>>
>> I think this works.
>>
>> I've also come up with something to merge the two checks, not 100% it's correct or more readable:
>>
>> if (((flag1 | flag2) & I915_EXEC_RING_MASK)) == I915_EXEC_BSD && // at least one is BSD
>>      !((flag1 ^ flag2) & I915_EXEC_RING_MASK) && // both are BSD
>>      (((flag1 | flag2) & (3 << 13))) != 3) // not guaranteed different
>> 	return false;
>>
>> Would need feeding in some values and checking it works as expected. Probably not worth it since I doubt it is more readable.
> readability perspective, we could stick to the original version. If we
> want to go ahead we need to do below ops:

Stick with your version I think.

Chris is being quiet BTW. Either we are below his radar and he'll scream 
later, or we managed to approach something he finds passable. ;)

Regards,

Tvrtko

> 
> if (((flag1 | flag2) & I915_EXEC_BSD) &&  //Atleast one is BSD
>      !((flag1 ^ flag2) & I915_EXEC_BSD) && //Both are BSD
>      ((flag1 | flag2) & I915_EXEC_RING_MASK) != I915_EXEC_RING_MASK) //Not BSD0 and BSD1
> 	return false;
>>
>>> +
>>> +	return true;
>>> +}
>>> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
>>> index 73b5002a04bd..760b8baefc48 100644
>>> --- a/lib/igt_gt.h
>>> +++ b/lib/igt_gt.h
>>> @@ -101,5 +101,6 @@ extern const struct intel_execution_engine2 {
>>>    } intel_execution_engines2[];
>>>    
>>>    int gem_execbuf_flags_to_engine_class(unsigned int flags);
>>> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2);
>>>    
>>>    #endif /* IGT_GT_H */
>>> diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
>>> index 9a06af7e29cb..5fdce5ac5b77 100644
>>> --- a/tests/i915/gem_exec_async.c
>>> +++ b/tests/i915/gem_exec_async.c
>>> @@ -80,9 +80,10 @@ static void store_dword(int fd, unsigned ring,
>>>    	gem_close(fd, obj[1].handle);
>>>    }
>>>    
>>> -static void one(int fd, unsigned ring, uint32_t flags)
>>> +static void one(int fd, const struct intel_execution_engine2 *e2, bool legacy)
>>>    {
>>>    	const int gen = intel_gen(intel_get_drm_devid(fd));
>>> +	const struct intel_execution_engine2 *other2;
>>>    	struct drm_i915_gem_exec_object2 obj[2];
>>>    #define SCRATCH 0
>>>    #define BATCH 1
>>> @@ -138,20 +139,33 @@ static void one(int fd, unsigned ring, uint32_t flags)
>>>    	memset(&execbuf, 0, sizeof(execbuf));
>>>    	execbuf.buffers_ptr = to_user_pointer(obj);
>>>    	execbuf.buffer_count = 2;
>>> -	execbuf.flags = ring | flags;
>>> +	execbuf.flags = e2->flags;
>>>    	igt_require(__gem_execbuf(fd, &execbuf) == 0);
>>>    	gem_close(fd, obj[BATCH].handle);
>>>    
>>>    	i = 0;
>>> -	for_each_physical_engine(fd, other) {
>>> -		if (other == ring)
>>> -			continue;
>>> +	if (legacy) {
>>> +		for_each_engine(fd, other) {
>>> +			if (gem_eb_flags_are_different_engines(e2->flags, other))
>>
>> if (!different)
> Will fix it. Thanks.
> 
> -Ram
>>
>> Regards,
>>
>> Tvrtko
>>
>>> +				continue;
>>>    
>>> -		if (!gem_can_store_dword(fd, other))
>>> -			continue;
>>> +			if (!gem_can_store_dword(fd, other))
>>> +				continue;
>>> +
>>> +			store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
>>> +			i++;
>>> +		}
>>> +	} else {
>>> +		__for_each_physical_engine(fd, other2) {
>>> +			if (gem_engine_is_equal(e2, other2))
>>> +				continue;
>>>    
>>> -		store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
>>> -		i++;
>>> +			if (!gem_class_can_store_dword(fd, other2->class))
>>> +				continue;
>>> +
>>> +			store_dword(fd, other2->flags, obj[SCRATCH].handle, 4*i, i);
>>> +			i++;
>>> +		}
>>>    	}
>>>    
>>>    	*batch = MI_BATCH_BUFFER_END;
>>> @@ -185,7 +199,9 @@ static bool has_async_execbuf(int fd)
>>>    
>>>    igt_main
>>>    {
>>> +	const struct intel_execution_engine2 *e2;
>>>    	const struct intel_execution_engine *e;
>>> +	struct intel_execution_engine2 e2__;
>>>    	int fd = -1;
>>>    
>>>    	igt_skip_on_simulation();
>>> @@ -200,14 +216,22 @@ igt_main
>>>    	}
>>>    
>>>    	for (e = intel_execution_engines; e->name; e++) {
>>> -		/* default exec-id is purely symbolic */
>>> -		if (e->exec_id == 0)
>>> +		e2__ = gem_eb_flags_to_engine(e->exec_id | e->flags);
>>> +		if (e2__.flags == -1)
>>>    			continue;
>>> +		e2 = &e2__;
>>>    
>>> -		igt_subtest_f("concurrent-writes-%s", e->name) {
>>> +		igt_subtest_f("legacy-concurrent-writes-%s", e2->name) {
>>>    			igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
>>> -			igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
>>> -			one(fd, e->exec_id, e->flags);
>>> +			igt_require(gem_class_can_store_dword(fd, e2->class));
>>> +			one(fd, e2, true);
>>> +		}
>>> +	}
>>> +
>>> +	__for_each_physical_engine(fd, e2) {
>>> +		igt_subtest_f("concurrent-writes-%s", e2->name) {
>>> +			igt_require(gem_class_can_store_dword(fd, e2->class));
>>> +			one(fd, e2, false);
>>>    		}
>>>    	}
>>>    
>>>
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery
  2019-07-30 15:20     ` Tvrtko Ursulin
@ 2019-07-30 17:07       ` Chris Wilson
  2019-07-31  6:12         ` Tvrtko Ursulin
  0 siblings, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2019-07-30 17:07 UTC (permalink / raw)
  To: Ramalingam C, Tvrtko Ursulin; +Cc: intel-gfx

Quoting Tvrtko Ursulin (2019-07-30 16:20:08)
> 
> On 30/07/2019 09:04, Ramalingam C wrote:
> > On 2019-07-30 at 13:05:27 +0100, Tvrtko Ursulin wrote:
> >>
> >> On 30/07/2019 04:58, Ramalingam C wrote:
> >>> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2)
> >>
> >> I think we try to avoid implied int but not sure in this case whether to suggest unsigned int, long or uint64_t. If we are suggesting in the function name that any flags can be passed in perhaps it should be uint64_t and then we filter on the engine bits (flags.. &= I915_EXEC_RING_MASK | (3 << 13)) before checking. Yeah, I think that would be more robust for a generic helper.
> >>
> >> And add a doc blurb for this helper since it is non-obvious why we went for different and not same. My thinking was the name different would be clearer to express kind of tri-state nature of this check. (Flags may be different, but engines are not guaranteed to be different.) Have I over-complicated it? Do we need to make it clearer by naming it gem_eb_flags_are_guaranteed_different_engines? :)
> > For me current shape looks good enough :) I will use the uint64_t for
> > parameter types.
> 
> Okay but please add some documentation for the helper (we've been very 
> bad in this work in this respect so far) and also filter out non-engine 
> selection bits from the flags before doing the checks.
> 
> >>
> >>> +{
> >>> +   if (eb_flag1 == eb_flag2)
> >>> +           return false;
> >>> +
> >>> +   /* DEFAULT stands for RENDER in legacy uAPI*/
> >>> +   if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
> >>> +        (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
> >>> +           return false;
> >>> +
> >>> +   /*
> >>> +    * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
> >>> +    * same engine.
> >>> +    */
> >>> +   if ((eb_flag1 == I915_EXEC_BSD) &&
> >>> +       (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> >>> +                   return false;
> >>> +
> >>> +   if ((eb_flag2 == I915_EXEC_BSD) &&
> >>> +       (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> >>> +                   return false;
> >>
> >> I think this works.
> >>
> >> I've also come up with something to merge the two checks, not 100% it's correct or more readable:
> >>
> >> if (((flag1 | flag2) & I915_EXEC_RING_MASK)) == I915_EXEC_BSD && // at least one is BSD
> >>      !((flag1 ^ flag2) & I915_EXEC_RING_MASK) && // both are BSD
> >>      (((flag1 | flag2) & (3 << 13))) != 3) // not guaranteed different
> >>      return false;
> >>
> >> Would need feeding in some values and checking it works as expected. Probably not worth it since I doubt it is more readable.
> > readability perspective, we could stick to the original version. If we
> > want to go ahead we need to do below ops:
> 
> Stick with your version I think.
> 
> Chris is being quiet BTW. Either we are below his radar and he'll scream 
> later, or we managed to approach something he finds passable. ;)

Just waiting until I have to use it in anger :)

Gut feeling is that I won't have to use it, in that if I have two
different timelines pointing to the same physical engine, do I really
care? The situations where I would have distinct engine mappings strike
me as being cases where I'm testing timelines; otherwise I would create
contexts with the same ctx->engines[] map.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* ✓ Fi.CI.IGT: success for tests/i915/gem_exec_async: Update with engine discovery
  2019-07-30  3:58 [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery Ramalingam C
  2019-07-30 11:27 ` ✓ Fi.CI.BAT: success for " Patchwork
  2019-07-30 12:05 ` [PATCH i-g-t v5] " Tvrtko Ursulin
@ 2019-07-30 19:03 ` Patchwork
  2019-07-31  8:34 ` ✗ Fi.CI.BAT: failure for tests/i915/gem_exec_async: Update with engine discovery (rev2) Patchwork
  2019-07-31  8:58 ` ✗ GitLab.Pipeline: warning " Patchwork
  4 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2019-07-30 19:03 UTC (permalink / raw)
  To: Ramalingam C; +Cc: intel-gfx

== Series Details ==

Series: tests/i915/gem_exec_async: Update with engine discovery
URL   : https://patchwork.freedesktop.org/series/64425/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6580_full -> IGTPW_3308_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Possible regressions ####

  * {igt@gem_exec_async@legacy-concurrent-writes-default} (NEW):
    - shard-iclb:         NOTRUN -> [SKIP][1] +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-iclb6/igt@gem_exec_async@legacy-concurrent-writes-default.html

  
New tests
---------

  New tests have been introduced between CI_DRM_6580_full and IGTPW_3308_full:

### New IGT tests (12) ###

  * igt@gem_exec_async@concurrent-writes-bcs0:
    - Statuses : 6 pass(s)
    - Exec time: [0.00] s

  * igt@gem_exec_async@concurrent-writes-rcs0:
    - Statuses : 6 pass(s)
    - Exec time: [0.00] s

  * igt@gem_exec_async@concurrent-writes-vcs0:
    - Statuses : 5 pass(s) 1 skip(s)
    - Exec time: [0.0, 0.00] s

  * igt@gem_exec_async@concurrent-writes-vcs1:
    - Statuses : 1 pass(s)
    - Exec time: [0.00] s

  * igt@gem_exec_async@concurrent-writes-vcs2:
    - Statuses :
    - Exec time: [None] s

  * igt@gem_exec_async@concurrent-writes-vecs0:
    - Statuses : 4 pass(s)
    - Exec time: [0.00] s

  * igt@gem_exec_async@legacy-concurrent-writes-bcs0:
    - Statuses : 6 pass(s)
    - Exec time: [0.00] s

  * igt@gem_exec_async@legacy-concurrent-writes-default:
    - Statuses : 6 skip(s)
    - Exec time: [0.0] s

  * igt@gem_exec_async@legacy-concurrent-writes-rcs0:
    - Statuses : 6 pass(s)
    - Exec time: [0.00, 0.01] s

  * igt@gem_exec_async@legacy-concurrent-writes-vcs0:
    - Statuses : 1 pass(s) 5 skip(s)
    - Exec time: [0.0, 0.00] s

  * igt@gem_exec_async@legacy-concurrent-writes-vcs1:
    - Statuses : 1 pass(s) 5 skip(s)
    - Exec time: [0.0, 0.00] s

  * igt@gem_exec_async@legacy-concurrent-writes-vecs0:
    - Statuses : 5 pass(s) 1 skip(s)
    - Exec time: [0.0, 0.01] s

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-kbl:          [PASS][2] -> [DMESG-WARN][3] ([fdo#108566]) +3 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-kbl2/igt@gem_ctx_isolation@rcs0-s3.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-kbl4/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_eio@reset-stress:
    - shard-snb:          [PASS][4] -> [FAIL][5] ([fdo#109661])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-snb7/igt@gem_eio@reset-stress.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-snb4/igt@gem_eio@reset-stress.html

  * igt@i915_pm_rpm@i2c:
    - shard-hsw:          [PASS][6] -> [FAIL][7] ([fdo#104097])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-hsw7/igt@i915_pm_rpm@i2c.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-hsw1/igt@i915_pm_rpm@i2c.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-apl:          [PASS][8] -> [DMESG-WARN][9] ([fdo#108566]) +1 similar issue
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-apl2/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-apl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-hsw:          [PASS][10] -> [INCOMPLETE][11] ([fdo#103540])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-hsw4/igt@kms_flip@flip-vs-suspend-interruptible.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-hsw4/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-shrfb-draw-pwrite:
    - shard-glk:          [PASS][12] -> [FAIL][13] ([fdo#103167])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-glk8/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-shrfb-draw-pwrite.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-glk6/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-shrfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-pwrite:
    - shard-iclb:         [PASS][14] -> [FAIL][15] ([fdo#103167]) +4 similar issues
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-pwrite.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-pwrite.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [PASS][16] -> [FAIL][17] ([fdo#103166])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-iclb7/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-iclb7/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_cursor_plane_onoff:
    - shard-iclb:         [PASS][18] -> [SKIP][19] ([fdo#109441]) +2 similar issues
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-iclb7/igt@kms_psr@psr2_cursor_plane_onoff.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [PASS][20] -> [FAIL][21] ([fdo#99912])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-kbl1/igt@kms_setmode@basic.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-kbl3/igt@kms_setmode@basic.html

  
#### Possible fixes ####

  * igt@gem_userptr_blits@create-destroy-sync:
    - shard-apl:          [INCOMPLETE][22] ([fdo#103927]) -> [PASS][23]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-apl4/igt@gem_userptr_blits@create-destroy-sync.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-apl1/igt@gem_userptr_blits@create-destroy-sync.html

  * igt@kms_cursor_crc@pipe-c-cursor-alpha-opaque:
    - shard-kbl:          [FAIL][24] ([fdo#103232]) -> [PASS][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-kbl7/igt@kms_cursor_crc@pipe-c-cursor-alpha-opaque.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-alpha-opaque.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          [FAIL][26] ([fdo#105363]) -> [PASS][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-glk7/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-glk1/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-apl:          [DMESG-WARN][28] ([fdo#108566]) -> [PASS][29]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-apl3/igt@kms_flip@flip-vs-suspend-interruptible.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-apl3/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-glk:          [FAIL][30] ([fdo#103167]) -> [PASS][31]
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-glk6/igt@kms_frontbuffer_tracking@fbc-stridechange.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-glk1/igt@kms_frontbuffer_tracking@fbc-stridechange.html
    - shard-apl:          [FAIL][32] ([fdo#103167]) -> [PASS][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-apl6/igt@kms_frontbuffer_tracking@fbc-stridechange.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-apl1/igt@kms_frontbuffer_tracking@fbc-stridechange.html
    - shard-kbl:          [FAIL][34] ([fdo#103167]) -> [PASS][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-kbl4/igt@kms_frontbuffer_tracking@fbc-stridechange.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-kbl1/igt@kms_frontbuffer_tracking@fbc-stridechange.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-render:
    - shard-iclb:         [FAIL][36] ([fdo#103167]) -> [PASS][37] +2 similar issues
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-iclb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-render.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-iclb7/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-render.html

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-render:
    - shard-iclb:         [INCOMPLETE][38] ([fdo#106978] / [fdo#107713]) -> [PASS][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-iclb1/igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-render.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-iclb1/igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-render.html

  * igt@kms_psr@psr2_cursor_render:
    - shard-iclb:         [SKIP][40] ([fdo#109441]) -> [PASS][41] +1 similar issue
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-iclb8/igt@kms_psr@psr2_cursor_render.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-iclb2/igt@kms_psr@psr2_cursor_render.html

  * igt@kms_setmode@basic:
    - shard-apl:          [FAIL][42] ([fdo#99912]) -> [PASS][43]
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-apl8/igt@kms_setmode@basic.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-apl4/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-kbl:          [DMESG-WARN][44] ([fdo#108566]) -> [PASS][45] +3 similar issues
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-kbl3/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-kbl1/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

  
#### Warnings ####

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [DMESG-WARN][46] ([fdo#107724]) -> [SKIP][47] ([fdo#109349])
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6580/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/shard-iclb6/igt@kms_dp_dsc@basic-dsc-enable-edp.html

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

  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104097]: https://bugs.freedesktop.org/show_bug.cgi?id=104097
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#106978]: https://bugs.freedesktop.org/show_bug.cgi?id=106978
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109661]: https://bugs.freedesktop.org/show_bug.cgi?id=109661
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 6)
------------------------------

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


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

  * CI: CI-20190529 -> None
  * IGT: IGT_5115 -> IGTPW_3308
  * Piglit: piglit_4509 -> None

  CI-20190529: 20190529
  CI_DRM_6580: 95a5a5fb8d2de1f66a8c57941ae823b82b75708d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3308: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3308/
  IGT_5115: 21be7a02ac8a8ff46b561c36a69e4dd5a0c2938b @ 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_3308/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery
  2019-07-31  6:12         ` Tvrtko Ursulin
@ 2019-07-30 23:21           ` Ramalingam C
  2019-07-31  6:33             ` Tvrtko Ursulin
  2019-07-31  8:05           ` Chris Wilson
  1 sibling, 1 reply; 14+ messages in thread
From: Ramalingam C @ 2019-07-30 23:21 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

On 2019-07-31 at 07:12:35 +0100, Tvrtko Ursulin wrote:
> 
> On 30/07/2019 18:07, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-30 16:20:08)
> > > 
> > > On 30/07/2019 09:04, Ramalingam C wrote:
> > > > On 2019-07-30 at 13:05:27 +0100, Tvrtko Ursulin wrote:
> > > > > 
> > > > > On 30/07/2019 04:58, Ramalingam C wrote:
> > > > > > +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2)
> > > > > 
> > > > > I think we try to avoid implied int but not sure in this case whether to suggest unsigned int, long or uint64_t. If we are suggesting in the function name that any flags can be passed in perhaps it should be uint64_t and then we filter on the engine bits (flags.. &= I915_EXEC_RING_MASK | (3 << 13)) before checking. Yeah, I think that would be more robust for a generic helper.
> > > > > 
> > > > > And add a doc blurb for this helper since it is non-obvious why we went for different and not same. My thinking was the name different would be clearer to express kind of tri-state nature of this check. (Flags may be different, but engines are not guaranteed to be different.) Have I over-complicated it? Do we need to make it clearer by naming it gem_eb_flags_are_guaranteed_different_engines? :)
> > > > For me current shape looks good enough :) I will use the uint64_t for
> > > > parameter types.
> > > 
> > > Okay but please add some documentation for the helper (we've been very
> > > bad in this work in this respect so far) and also filter out non-engine
> > > selection bits from the flags before doing the checks.
> > > 
> > > > > 
> > > > > > +{
> > > > > > +   if (eb_flag1 == eb_flag2)
> > > > > > +           return false;
> > > > > > +
> > > > > > +   /* DEFAULT stands for RENDER in legacy uAPI*/
> > > > > > +   if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
> > > > > > +        (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
> > > > > > +           return false;
> > > > > > +
> > > > > > +   /*
> > > > > > +    * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
> > > > > > +    * same engine.
> > > > > > +    */
> > > > > > +   if ((eb_flag1 == I915_EXEC_BSD) &&
> > > > > > +       (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> > > > > > +                   return false;
> > > > > > +
> > > > > > +   if ((eb_flag2 == I915_EXEC_BSD) &&
> > > > > > +       (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> > > > > > +                   return false;
> > > > > 
> > > > > I think this works.
> > > > > 
> > > > > I've also come up with something to merge the two checks, not 100% it's correct or more readable:
> > > > > 
> > > > > if (((flag1 | flag2) & I915_EXEC_RING_MASK)) == I915_EXEC_BSD && // at least one is BSD
> > > > >       !((flag1 ^ flag2) & I915_EXEC_RING_MASK) && // both are BSD
> > > > >       (((flag1 | flag2) & (3 << 13))) != 3) // not guaranteed different
> > > > >       return false;
> > > > > 
> > > > > Would need feeding in some values and checking it works as expected. Probably not worth it since I doubt it is more readable.
> > > > readability perspective, we could stick to the original version. If we
> > > > want to go ahead we need to do below ops:
> > > 
> > > Stick with your version I think.
> > > 
> > > Chris is being quiet BTW. Either we are below his radar and he'll scream
> > > later, or we managed to approach something he finds passable. ;)
> > 
> > Just waiting until I have to use it in anger :)
> > 
> > Gut feeling is that I won't have to use it, in that if I have two
> > different timelines pointing to the same physical engine, do I really
> > care? The situations where I would have distinct engine mappings strike
> > me as being cases where I'm testing timelines; otherwise I would create
> > contexts with the same ctx->engines[] map.
> 
> I do dislike this complication of testing uapi flags but figuring out the
> physical engines under the covers. Do you think it would be clearer do drop
> this helper and instead use two contexts in this test? It would make it
> dependent on contexts though..
You mean different context for internal iteration of the engines?
Is it possible for all usecases? What if a test want to test a operation
in the same context on all other engines than a engine passed in?
Need to confirm if there is any such need though.

-Ram
> 
> Maybe we should altogether re-visit my plan to eliminate the legacy
> for_each_physical_engine iterator. I was hoping we don't have two. So wanted
> to make it explicit where we are testing the old eb ABI, and where we are
> testing new. However reality seems more complicated.. Would it instead work
> better to rename the old iterator to something like
> for_each_legacy_physical_engine?
> 
> Regards,
> 
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery
  2019-07-30 17:07       ` Chris Wilson
@ 2019-07-31  6:12         ` Tvrtko Ursulin
  2019-07-30 23:21           ` Ramalingam C
  2019-07-31  8:05           ` Chris Wilson
  0 siblings, 2 replies; 14+ messages in thread
From: Tvrtko Ursulin @ 2019-07-31  6:12 UTC (permalink / raw)
  To: Chris Wilson, Ramalingam C; +Cc: intel-gfx


On 30/07/2019 18:07, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-30 16:20:08)
>>
>> On 30/07/2019 09:04, Ramalingam C wrote:
>>> On 2019-07-30 at 13:05:27 +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 30/07/2019 04:58, Ramalingam C wrote:
>>>>> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2)
>>>>
>>>> I think we try to avoid implied int but not sure in this case whether to suggest unsigned int, long or uint64_t. If we are suggesting in the function name that any flags can be passed in perhaps it should be uint64_t and then we filter on the engine bits (flags.. &= I915_EXEC_RING_MASK | (3 << 13)) before checking. Yeah, I think that would be more robust for a generic helper.
>>>>
>>>> And add a doc blurb for this helper since it is non-obvious why we went for different and not same. My thinking was the name different would be clearer to express kind of tri-state nature of this check. (Flags may be different, but engines are not guaranteed to be different.) Have I over-complicated it? Do we need to make it clearer by naming it gem_eb_flags_are_guaranteed_different_engines? :)
>>> For me current shape looks good enough :) I will use the uint64_t for
>>> parameter types.
>>
>> Okay but please add some documentation for the helper (we've been very
>> bad in this work in this respect so far) and also filter out non-engine
>> selection bits from the flags before doing the checks.
>>
>>>>
>>>>> +{
>>>>> +   if (eb_flag1 == eb_flag2)
>>>>> +           return false;
>>>>> +
>>>>> +   /* DEFAULT stands for RENDER in legacy uAPI*/
>>>>> +   if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
>>>>> +        (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
>>>>> +           return false;
>>>>> +
>>>>> +   /*
>>>>> +    * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
>>>>> +    * same engine.
>>>>> +    */
>>>>> +   if ((eb_flag1 == I915_EXEC_BSD) &&
>>>>> +       (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
>>>>> +                   return false;
>>>>> +
>>>>> +   if ((eb_flag2 == I915_EXEC_BSD) &&
>>>>> +       (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
>>>>> +                   return false;
>>>>
>>>> I think this works.
>>>>
>>>> I've also come up with something to merge the two checks, not 100% it's correct or more readable:
>>>>
>>>> if (((flag1 | flag2) & I915_EXEC_RING_MASK)) == I915_EXEC_BSD && // at least one is BSD
>>>>       !((flag1 ^ flag2) & I915_EXEC_RING_MASK) && // both are BSD
>>>>       (((flag1 | flag2) & (3 << 13))) != 3) // not guaranteed different
>>>>       return false;
>>>>
>>>> Would need feeding in some values and checking it works as expected. Probably not worth it since I doubt it is more readable.
>>> readability perspective, we could stick to the original version. If we
>>> want to go ahead we need to do below ops:
>>
>> Stick with your version I think.
>>
>> Chris is being quiet BTW. Either we are below his radar and he'll scream
>> later, or we managed to approach something he finds passable. ;)
> 
> Just waiting until I have to use it in anger :)
> 
> Gut feeling is that I won't have to use it, in that if I have two
> different timelines pointing to the same physical engine, do I really
> care? The situations where I would have distinct engine mappings strike
> me as being cases where I'm testing timelines; otherwise I would create
> contexts with the same ctx->engines[] map.

I do dislike this complication of testing uapi flags but figuring out 
the physical engines under the covers. Do you think it would be clearer 
do drop this helper and instead use two contexts in this test? It would 
make it dependent on contexts though..

Maybe we should altogether re-visit my plan to eliminate the legacy 
for_each_physical_engine iterator. I was hoping we don't have two. So 
wanted to make it explicit where we are testing the old eb ABI, and 
where we are testing new. However reality seems more complicated.. Would 
it instead work better to rename the old iterator to something like 
for_each_legacy_physical_engine?

Regards,

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery
  2019-07-30 23:21           ` Ramalingam C
@ 2019-07-31  6:33             ` Tvrtko Ursulin
  0 siblings, 0 replies; 14+ messages in thread
From: Tvrtko Ursulin @ 2019-07-31  6:33 UTC (permalink / raw)
  To: Ramalingam C; +Cc: intel-gfx


On 31/07/2019 00:21, Ramalingam C wrote:
> On 2019-07-31 at 07:12:35 +0100, Tvrtko Ursulin wrote:
>>
>> On 30/07/2019 18:07, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-07-30 16:20:08)
>>>>
>>>> On 30/07/2019 09:04, Ramalingam C wrote:
>>>>> On 2019-07-30 at 13:05:27 +0100, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> On 30/07/2019 04:58, Ramalingam C wrote:
>>>>>>> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2)
>>>>>>
>>>>>> I think we try to avoid implied int but not sure in this case whether to suggest unsigned int, long or uint64_t. If we are suggesting in the function name that any flags can be passed in perhaps it should be uint64_t and then we filter on the engine bits (flags.. &= I915_EXEC_RING_MASK | (3 << 13)) before checking. Yeah, I think that would be more robust for a generic helper.
>>>>>>
>>>>>> And add a doc blurb for this helper since it is non-obvious why we went for different and not same. My thinking was the name different would be clearer to express kind of tri-state nature of this check. (Flags may be different, but engines are not guaranteed to be different.) Have I over-complicated it? Do we need to make it clearer by naming it gem_eb_flags_are_guaranteed_different_engines? :)
>>>>> For me current shape looks good enough :) I will use the uint64_t for
>>>>> parameter types.
>>>>
>>>> Okay but please add some documentation for the helper (we've been very
>>>> bad in this work in this respect so far) and also filter out non-engine
>>>> selection bits from the flags before doing the checks.
>>>>
>>>>>>
>>>>>>> +{
>>>>>>> +   if (eb_flag1 == eb_flag2)
>>>>>>> +           return false;
>>>>>>> +
>>>>>>> +   /* DEFAULT stands for RENDER in legacy uAPI*/
>>>>>>> +   if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
>>>>>>> +        (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
>>>>>>> +           return false;
>>>>>>> +
>>>>>>> +   /*
>>>>>>> +    * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
>>>>>>> +    * same engine.
>>>>>>> +    */
>>>>>>> +   if ((eb_flag1 == I915_EXEC_BSD) &&
>>>>>>> +       (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
>>>>>>> +                   return false;
>>>>>>> +
>>>>>>> +   if ((eb_flag2 == I915_EXEC_BSD) &&
>>>>>>> +       (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
>>>>>>> +                   return false;
>>>>>>
>>>>>> I think this works.
>>>>>>
>>>>>> I've also come up with something to merge the two checks, not 100% it's correct or more readable:
>>>>>>
>>>>>> if (((flag1 | flag2) & I915_EXEC_RING_MASK)) == I915_EXEC_BSD && // at least one is BSD
>>>>>>        !((flag1 ^ flag2) & I915_EXEC_RING_MASK) && // both are BSD
>>>>>>        (((flag1 | flag2) & (3 << 13))) != 3) // not guaranteed different
>>>>>>        return false;
>>>>>>
>>>>>> Would need feeding in some values and checking it works as expected. Probably not worth it since I doubt it is more readable.
>>>>> readability perspective, we could stick to the original version. If we
>>>>> want to go ahead we need to do below ops:
>>>>
>>>> Stick with your version I think.
>>>>
>>>> Chris is being quiet BTW. Either we are below his radar and he'll scream
>>>> later, or we managed to approach something he finds passable. ;)
>>>
>>> Just waiting until I have to use it in anger :)
>>>
>>> Gut feeling is that I won't have to use it, in that if I have two
>>> different timelines pointing to the same physical engine, do I really
>>> care? The situations where I would have distinct engine mappings strike
>>> me as being cases where I'm testing timelines; otherwise I would create
>>> contexts with the same ctx->engines[] map.
>>
>> I do dislike this complication of testing uapi flags but figuring out the
>> physical engines under the covers. Do you think it would be clearer do drop
>> this helper and instead use two contexts in this test? It would make it
>> dependent on contexts though..
> You mean different context for internal iteration of the engines?

Yes, just because test appears wanting to ascertain the second 
submission will not hang due object activity tracking. So we either need 
a guaranteed different engine (this patch), or a separate context.

> Is it possible for all usecases? What if a test want to test a operation

Probably not, and then it is undesirable to add context requirement to 
tests.

> in the same context on all other engines than a engine passed in?
> Need to confirm if there is any such need though.

Yeah that's also a possibility.

Given all the above, I wrote the below paragraph in the message you 
replied to. vvv

> 
> -Ram
>>
>> Maybe we should altogether re-visit my plan to eliminate the legacy
>> for_each_physical_engine iterator. I was hoping we don't have two. So wanted
>> to make it explicit where we are testing the old eb ABI, and where we are
>> testing new. However reality seems more complicated.. Would it instead work
>> better to rename the old iterator to something like
>> for_each_legacy_physical_engine?
>>
>> Regards,
>>
>> Tvrtko
> 

Regards,

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery
  2019-07-31  6:12         ` Tvrtko Ursulin
  2019-07-30 23:21           ` Ramalingam C
@ 2019-07-31  8:05           ` Chris Wilson
  2019-07-31  8:37             ` Chris Wilson
  1 sibling, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2019-07-31  8:05 UTC (permalink / raw)
  To: Ramalingam C, Tvrtko Ursulin; +Cc: intel-gfx

Quoting Tvrtko Ursulin (2019-07-31 07:12:35)
> 
> On 30/07/2019 18:07, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-30 16:20:08)
> >>
> >> On 30/07/2019 09:04, Ramalingam C wrote:
> >>> On 2019-07-30 at 13:05:27 +0100, Tvrtko Ursulin wrote:
> >>>>
> >>>> On 30/07/2019 04:58, Ramalingam C wrote:
> >>>>> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2)
> >>>>
> >>>> I think we try to avoid implied int but not sure in this case whether to suggest unsigned int, long or uint64_t. If we are suggesting in the function name that any flags can be passed in perhaps it should be uint64_t and then we filter on the engine bits (flags.. &= I915_EXEC_RING_MASK | (3 << 13)) before checking. Yeah, I think that would be more robust for a generic helper.
> >>>>
> >>>> And add a doc blurb for this helper since it is non-obvious why we went for different and not same. My thinking was the name different would be clearer to express kind of tri-state nature of this check. (Flags may be different, but engines are not guaranteed to be different.) Have I over-complicated it? Do we need to make it clearer by naming it gem_eb_flags_are_guaranteed_different_engines? :)
> >>> For me current shape looks good enough :) I will use the uint64_t for
> >>> parameter types.
> >>
> >> Okay but please add some documentation for the helper (we've been very
> >> bad in this work in this respect so far) and also filter out non-engine
> >> selection bits from the flags before doing the checks.
> >>
> >>>>
> >>>>> +{
> >>>>> +   if (eb_flag1 == eb_flag2)
> >>>>> +           return false;
> >>>>> +
> >>>>> +   /* DEFAULT stands for RENDER in legacy uAPI*/
> >>>>> +   if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
> >>>>> +        (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
> >>>>> +           return false;
> >>>>> +
> >>>>> +   /*
> >>>>> +    * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
> >>>>> +    * same engine.
> >>>>> +    */
> >>>>> +   if ((eb_flag1 == I915_EXEC_BSD) &&
> >>>>> +       (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> >>>>> +                   return false;
> >>>>> +
> >>>>> +   if ((eb_flag2 == I915_EXEC_BSD) &&
> >>>>> +       (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> >>>>> +                   return false;
> >>>>
> >>>> I think this works.
> >>>>
> >>>> I've also come up with something to merge the two checks, not 100% it's correct or more readable:
> >>>>
> >>>> if (((flag1 | flag2) & I915_EXEC_RING_MASK)) == I915_EXEC_BSD && // at least one is BSD
> >>>>       !((flag1 ^ flag2) & I915_EXEC_RING_MASK) && // both are BSD
> >>>>       (((flag1 | flag2) & (3 << 13))) != 3) // not guaranteed different
> >>>>       return false;
> >>>>
> >>>> Would need feeding in some values and checking it works as expected. Probably not worth it since I doubt it is more readable.
> >>> readability perspective, we could stick to the original version. If we
> >>> want to go ahead we need to do below ops:
> >>
> >> Stick with your version I think.
> >>
> >> Chris is being quiet BTW. Either we are below his radar and he'll scream
> >> later, or we managed to approach something he finds passable. ;)
> > 
> > Just waiting until I have to use it in anger :)
> > 
> > Gut feeling is that I won't have to use it, in that if I have two
> > different timelines pointing to the same physical engine, do I really
> > care? The situations where I would have distinct engine mappings strike
> > me as being cases where I'm testing timelines; otherwise I would create
> > contexts with the same ctx->engines[] map.
> 
> I do dislike this complication of testing uapi flags but figuring out 
> the physical engines under the covers. Do you think it would be clearer 
> do drop this helper and instead use two contexts in this test? It would 
> make it dependent on contexts though..

Going back to look at the use case...

 tests/i915/gem_exec_async.c | 81 ++++++++++++++++++++++++++++---------
 1 file changed, 63 insertions(+), 18 deletions(-)

diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
index 9a06af7e2..fafca98ad 100644
--- a/tests/i915/gem_exec_async.c
+++ b/tests/i915/gem_exec_async.c
@@ -80,7 +80,10 @@ static void store_dword(int fd, unsigned ring,
 	gem_close(fd, obj[1].handle);
 }
 
-static void one(int fd, unsigned ring, uint32_t flags)
+static void one(int fd,
+		unsigned int idx,
+		const struct intel_execution_engine2 *engines,
+		unsigned int num_engines)
 {
 	const int gen = intel_gen(intel_get_drm_devid(fd));
 	struct drm_i915_gem_exec_object2 obj[2];
@@ -88,7 +91,6 @@ static void one(int fd, unsigned ring, uint32_t flags)
 #define BATCH 1
 	struct drm_i915_gem_relocation_entry reloc;
 	struct drm_i915_gem_execbuffer2 execbuf;
-	unsigned int other;
 	uint32_t *batch;
 	int i;
 
@@ -138,19 +140,17 @@ static void one(int fd, unsigned ring, uint32_t flags)
 	memset(&execbuf, 0, sizeof(execbuf));
 	execbuf.buffers_ptr = to_user_pointer(obj);
 	execbuf.buffer_count = 2;
-	execbuf.flags = ring | flags;
+	execbuf.flags = engines[idx].flags;
 	igt_require(__gem_execbuf(fd, &execbuf) == 0);
 	gem_close(fd, obj[BATCH].handle);
 
 	i = 0;
-	for_each_physical_engine(fd, other) {
-		if (other == ring)
+	for (unsigned int other = 0; other < num_engines; other++) {
+		if (other == idx)
 			continue;
 
-		if (!gem_can_store_dword(fd, other))
-			continue;
-
-		store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
+		store_dword(fd, engines[other].flags,
+			    obj[SCRATCH].handle, 4*i, i);
 		i++;
 	}
 
@@ -185,7 +185,6 @@ static bool has_async_execbuf(int fd)
 
 igt_main
 {
-	const struct intel_execution_engine *e;
 	int fd = -1;
 
 	igt_skip_on_simulation();
@@ -199,15 +198,61 @@ igt_main
 		igt_fork_hang_detector(fd);
 	}
 
-	for (e = intel_execution_engines; e->name; e++) {
-		/* default exec-id is purely symbolic */
-		if (e->exec_id == 0)
-			continue;
+	/* First up, check the legacy engine selector ABI for independence */
+	igt_subtest_group {
+		struct intel_execution_engine2 engines[64];
+		unsigned int num_engines = 0;
+
+		igt_fixture {
+			const struct intel_execution_engine *e;
+
+			for (e = intel_execution_engines; e->name; e++) {
+				if (!gem_ring_has_physical_engine(fd, e->exec_id | e->flags))
+					continue;
+
+				/* Must be unique, no unknowable BSD aliases! */
+				engines[num_engines] =
+					gem_eb_flags_to_engine(e->exec_id | e->flags);
+				if (engines[num_engines].flags != -1)
+					continue;
+
+				if (!gem_can_store_dword(fd, engines[num_engines].flags))
+					continue;
+
+				num_engines++;
+				if (num_engines == ARRAY_SIZE(engines))
+					break;
+			}
+		}
+
+		for (unsigned int n = 0; n < num_engines; n++) {
+			igt_subtest_f("legacy-concurrent-writes-%s",
+				      engines[n].name)
+				one(fd, n, engines, num_engines);
+		}
+	}
+
+	/* Same again, but using a ctx->engine[] map and indices */
+	igt_subtest_group {
+		struct intel_execution_engine2 engines[64];
+		unsigned int num_engines = 0;
+
+		igt_fixture {
+			const struct intel_execution_engine2 *e;
+
+			__for_each_physical_engine(fd, e) {
+				if (!gem_class_can_store_dword(fd, e->class))
+					continue;
+
+				engines[num_engines++] = *e;
+				if (num_engines == ARRAY_SIZE(engines))
+					break;
+			}
+		}
 
-		igt_subtest_f("concurrent-writes-%s", e->name) {
-			igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
-			igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
-			one(fd, e->exec_id, e->flags);
+		for (unsigned int n = 0; n < num_engines; n++) {
+			igt_subtest_f("concurrent-writes-%s", engines[n].name)
+				one(fd, n, engines, num_engines);
 		}
 	}

(Fill in the gaps for the new structs!)

Is more of what I was expecting. (Eventually no one will notice the BSD
aliasing bit rotting and we can remove it from the uABI.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* ✗ Fi.CI.BAT: failure for tests/i915/gem_exec_async: Update with engine discovery (rev2)
  2019-07-30  3:58 [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery Ramalingam C
                   ` (2 preceding siblings ...)
  2019-07-30 19:03 ` ✓ Fi.CI.IGT: success for " Patchwork
@ 2019-07-31  8:34 ` Patchwork
  2019-07-31  8:58 ` ✗ GitLab.Pipeline: warning " Patchwork
  4 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2019-07-31  8:34 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: tests/i915/gem_exec_async: Update with engine discovery (rev2)
URL   : https://patchwork.freedesktop.org/series/64425/
State : failure

== Summary ==

IGT patchset build failed on latest successful build
d2e6dd2f789596da5bd06efc2e9448e3160583b6 README: Add missing dependencies

258/282 testcase check: gem_eio                 OK       0.11 s 
259/282 testcase check: gem_exec_balancer       OK       0.14 s 
260/282 testcase check: gem_mocs_settings       OK       0.08 s 
261/282 testcase check: perf_pmu                OK       0.12 s 
262/282 testcase check: testdisplay             OK       0.10 s 
263/282 testcase check: sw_sync                 OK       0.10 s 
264/282 testcase check: amdgpu/amd_abm          OK       0.11 s 
265/282 testcase check: amdgpu/amd_basic        OK       0.09 s 
266/282 testcase check: amdgpu/amd_bypass       OK       0.10 s 
267/282 testcase check: amdgpu/amd_color        OK       0.14 s 
268/282 testcase check: amdgpu/amd_cs_nop       OK       0.12 s 
269/282 testcase check: amdgpu/amd_prime        OK       0.10 s 
270/282 runner                                  OK       1.47 s 
271/282 runner_json                             OK       0.09 s 
272/282 assembler: test/mov                     OK       0.06 s 
273/282 assembler: test/frc                     OK       0.08 s 
274/282 assembler: test/regtype                 OK       0.07 s 
275/282 assembler: test/rndd                    OK       0.07 s 
276/282 assembler: test/rndu                    OK       0.07 s 
277/282 assembler: test/rnde                    OK       0.05 s 
278/282 assembler: test/rnde-intsrc             OK       0.05 s 
279/282 assembler: test/rndz                    OK       0.05 s 
280/282 assembler: test/lzd                     OK       0.05 s 
281/282 assembler: test/not                     OK       0.04 s 
282/282 assembler: test/immediate               OK       0.06 s 

OK:       281
FAIL:       1
SKIP:       0
TIMEOUT:    0


The output from the failed tests:

148/282 testcase check: gem_exec_async          FAIL     0.25 s (exit status 1)

--- command ---
/home/cidrm/igt-gpu-tools/tests/igt_command_line.sh gem_exec_async
--- stdout ---
tests/gem_exec_async:
  Checking invalid option handling...
  Checking valid option handling...
  Checking subtest enumeration...
FAIL: tests/gem_exec_async
-------

Full log written to /home/cidrm/igt-gpu-tools/build/meson-logs/testlog.txt
FAILED: meson-test 
/usr/bin/python3 -u /usr/bin/meson test --no-rebuild --print-errorlogs
ninja: build stopped: subcommand failed.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery
  2019-07-31  8:05           ` Chris Wilson
@ 2019-07-31  8:37             ` Chris Wilson
  0 siblings, 0 replies; 14+ messages in thread
From: Chris Wilson @ 2019-07-31  8:37 UTC (permalink / raw)
  To: Ramalingam C, Tvrtko Ursulin; +Cc: intel-gfx

Quoting Chris Wilson (2019-07-31 09:05:25)
> +       /* First up, check the legacy engine selector ABI for independence */
> +       igt_subtest_group {
> +               struct intel_execution_engine2 engines[64];
> +               unsigned int num_engines = 0;
> +
> +               igt_fixture {
> +                       const struct intel_execution_engine *e;
> +
> +                       for (e = intel_execution_engines; e->name; e++) {
> +                               if (!gem_ring_has_physical_engine(fd, e->exec_id | e->flags))
> +                                       continue;
> +
> +                               /* Must be unique, no unknowable BSD aliases! */
> +                               engines[num_engines] =
> +                                       gem_eb_flags_to_engine(e->exec_id | e->flags);
> +                               if (engines[num_engines].flags != -1)
> +                                       continue;
> +
> +                               if (!gem_can_store_dword(fd, engines[num_engines].flags))
> +                                       continue;
> +
> +                               num_engines++;
> +                               if (num_engines == ARRAY_SIZE(engines))
> +                                       break;
> +                       }
> +               }

I also expect this to be repeated enough to merit something like
num_engines = gem_engines_get_legacy(fd,
				     engines, ARRAY_SIZE(engines),
				     LEGACY_PHYSICAL | LEGACY_CAN_STORE_DWORD);
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* ✗ GitLab.Pipeline: warning for tests/i915/gem_exec_async: Update with engine discovery (rev2)
  2019-07-30  3:58 [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery Ramalingam C
                   ` (3 preceding siblings ...)
  2019-07-31  8:34 ` ✗ Fi.CI.BAT: failure for tests/i915/gem_exec_async: Update with engine discovery (rev2) Patchwork
@ 2019-07-31  8:58 ` Patchwork
  4 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2019-07-31  8:58 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: tests/i915/gem_exec_async: Update with engine discovery (rev2)
URL   : https://patchwork.freedesktop.org/series/64425/
State : warning

== Summary ==

Pipeline status: FAILED.

See https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/pipelines/52430 for more details.

== Logs ==

For more details see: https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/pipelines/52430
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-07-31  8:58 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-30  3:58 [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery Ramalingam C
2019-07-30 11:27 ` ✓ Fi.CI.BAT: success for " Patchwork
2019-07-30 12:05 ` [PATCH i-g-t v5] " Tvrtko Ursulin
2019-07-30  8:04   ` Ramalingam C
2019-07-30 15:20     ` Tvrtko Ursulin
2019-07-30 17:07       ` Chris Wilson
2019-07-31  6:12         ` Tvrtko Ursulin
2019-07-30 23:21           ` Ramalingam C
2019-07-31  6:33             ` Tvrtko Ursulin
2019-07-31  8:05           ` Chris Wilson
2019-07-31  8:37             ` Chris Wilson
2019-07-30 19:03 ` ✓ Fi.CI.IGT: success for " Patchwork
2019-07-31  8:34 ` ✗ Fi.CI.BAT: failure for tests/i915/gem_exec_async: Update with engine discovery (rev2) Patchwork
2019-07-31  8:58 ` ✗ GitLab.Pipeline: warning " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.