tests/i915/gem_exec_create: Added __for_each_physical_engine to utilize all available engines.

From: saigowth <[email protected]>

Replaced the legacy for_each_engine* defines with the ones implemented in the gem_engine_topology library.

Cc Dec Katarzyna <[email protected]>

Cc Ursulin Tvrtko <[email protected]>

Signed-off-by sai gowtham <[email protected]>

---
 tests/i915/gem_exec_create.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

tests/i915/gem_exec_create.c +3 -2

index 102a161d..c70572af 100644
@@ -58,14 +58,15 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
 
 static void all(int fd, unsigned flags, int timeout, int ncpus)
 {
+	const struct intel_execution_engine2 *e;
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	struct drm_i915_gem_execbuffer2 execbuf;
 	struct drm_i915_gem_exec_object2 obj;
 	unsigned engines[16], nengine;
 
 	nengine = 0;
-	for_each_physical_engine(e, fd)
-		engines[nengine++] = eb_ring(e);
+	__for_each_physical_engine(fd, e)
+		engines[nengine++] = e->flags;
 	igt_require(nengine);
 
 	memset(&obj, 0, sizeof(obj));

tests/i915/gem_exec_create.c +10 -10

 		count = 0;
 		clock_gettime(CLOCK_MONOTONIC, &start);
 		do {
 			for (int loop = 0; loop < 1024; loop++) {
 				for (int n = 0; n < nengine; n++) {
-					obj.handle =  gem_create(fd, 4096);
-					gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
+					obj.handle =  gem_create(i915, 4096);
+					gem_write(i915, obj.handle, 0, &bbe, sizeof(bbe));
 					execbuf.flags &= ~ENGINE_FLAGS;
 					execbuf.flags |= engines[n];
-					gem_execbuf(fd, &execbuf);
+					gem_execbuf(i915, &execbuf);
 					if (flags & LEAK)
-						gem_madvise(fd, obj.handle, I915_MADV_DONTNEED);
+						gem_madvise(i915, obj.handle, I915_MADV_DONTNEED);
 					else
-						gem_close(fd, obj.handle);
+						gem_close(i915, obj.handle);
 				}
 			}
 			count += nengine * 1024;
 			clock_gettime(CLOCK_MONOTONIC, &now);
 		} while (elapsed(&start, &now) < timeout); /* Hang detection ~120s */
-		obj.handle =  gem_create(fd, 4096);
-		gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
+		obj.handle =  gem_create(i915, 4096);
+		gem_write(i915, obj.handle, 0, &bbe, sizeof(bbe));
 		for (int n = 0; n < nengine; n++) {
 			execbuf.flags &= ~ENGINE_FLAGS;
 			execbuf.flags |= engines[n];
-			gem_execbuf(fd, &execbuf);
+			gem_execbuf(i915, &execbuf);
 		}
-		gem_sync(fd, obj.handle);
-		gem_close(fd, obj.handle);
+		gem_sync(i915, obj.handle);
+		gem_close(i915, obj.handle);
 		clock_gettime(CLOCK_MONOTONIC, &now);
 
 		time = elapsed(&start, &now) / count;

Signature · 6 lines

2.25.0

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

From: saigowth <[email protected]>

Replaced the legacy for_each_engine* defines with the ones implemented in the gem_engine_topology library.

Cc Dec Katarzyna <[email protected]>

Cc Ursulin Tvrtko <[email protected]>

Signed-off-by sai gowtham <[email protected]>

---
 tests/i915/gem_exec_create.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

(quoted diff) +0 -0

b/tests/i915/gem_exec_create.c index 102a161d..c70572af 100644

tests/i915/gem_exec_create.c +3 -2

@@ -58,14 +58,15 @@ static double elapsed(const struct timespec 
 *start, const struct timespec *end)
 
 static void all(int fd, unsigned flags, int timeout, int ncpus)  {
+	const struct intel_execution_engine2 *e;
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	struct drm_i915_gem_execbuffer2 execbuf;
 	struct drm_i915_gem_exec_object2 obj;
 	unsigned engines[16], nengine;
 
 	nengine = 0;
-	for_each_physical_engine(e, fd)
-		engines[nengine++] = eb_ring(e);
+	__for_each_physical_engine(fd, e)
+		engines[nengine++] = e->flags;
 	igt_require(nengine);
 
 	memset(&obj, 0, sizeof(obj));

@@ -86,38 +87,40 @@ static void all(int fd, unsigned flags, int 
 timeout, int ncpus)

tests/i915/gem_exec_create.c +10 -10

 		count = 0;
 		clock_gettime(CLOCK_MONOTONIC, &start);
 		do {
 			for (int loop = 0; loop < 1024; loop++) {
 				for (int n = 0; n < nengine; n++) {
-					obj.handle =  gem_create(fd, 4096);
-					gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
+					obj.handle =  gem_create(i915, 4096);
+					gem_write(i915, obj.handle, 0, &bbe, sizeof(bbe));
 					execbuf.flags &= ~ENGINE_FLAGS;
 					execbuf.flags |= engines[n];
-					gem_execbuf(fd, &execbuf);
+					gem_execbuf(i915, &execbuf);
 					if (flags & LEAK)
-						gem_madvise(fd, obj.handle, I915_MADV_DONTNEED);
+						gem_madvise(i915, obj.handle, I915_MADV_DONTNEED);
 					else
-						gem_close(fd, obj.handle);
+						gem_close(i915, obj.handle);
 				}
 			}
 			count += nengine * 1024;
 			clock_gettime(CLOCK_MONOTONIC, &now);
 		} while (elapsed(&start, &now) < timeout); /* Hang detection ~120s */
-		obj.handle =  gem_create(fd, 4096);
-		gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
+		obj.handle =  gem_create(i915, 4096);
+		gem_write(i915, obj.handle, 0, &bbe, sizeof(bbe));
 		for (int n = 0; n < nengine; n++) {
 			execbuf.flags &= ~ENGINE_FLAGS;
 			execbuf.flags |= engines[n];
-			gem_execbuf(fd, &execbuf);
+			gem_execbuf(i915, &execbuf);
 		}
-		gem_sync(fd, obj.handle);
-		gem_close(fd, obj.handle);
+		gem_sync(i915, obj.handle);
+		gem_close(i915, obj.handle);
 		clock_gettime(CLOCK_MONOTONIC, &now);
 

time = elapsed(&start, &now) / count;
--
2.25.0

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

After fork we need to transfer the engine map from parent fd default context. (gem_context_copy_engines)

Also, since we have __for_each_physical_engine in the top level igt_fixture, I think _all_ subtests run with default ctx engine map. So all which submit to one from either all_engines or ppgtt_engines need to make sure engine maps are aligned.

it was suggested by tvrtko in one of the patch: https://patchwork.freedesktop.org/patch/351622/?series=72889&rev=1

-----Original Message-----
From: Dec, Katarzyna <[email protected]>
Sent: Thursday, March 5, 2020 6:13 PM
To: Ch, Sai Gowtham <[email protected]>; [email protected]
Subject: Re: [igt-dev] [PATCH] [PATCH i-g-t]tests/i915/gem_exec_create: Added __for_each_physical_engine to utilize all available engines.

Quoted 36 lines · sai gowtham

From: saigowth <[email protected]>

Replaced the legacy for_each_engine* defines with the ones implemented in the gem_engine_topology library.

Cc Dec Katarzyna <[email protected]>

Cc Ursulin Tvrtko <[email protected]>

Signed-off-by sai gowtham <[email protected]>

---
  tests/i915/gem_exec_create.c | 29 ++++++++++++++++-------------
  1 file changed, 16 insertions(+), 13 deletions(-)

(quoted diff) +0 -0

b/tests/i915/gem_exec_create.c index 102a161d..c70572af 100644

tests/i915/gem_exec_create.c +3 -2

@@ -58,14 +58,15 @@ static double elapsed(const struct timespec
 *start, const struct timespec *end)
 
  static void all(int fd, unsigned flags, int timeout, int ncpus)  {
+	const struct intel_execution_engine2 *e;
  	const uint32_t bbe = MI_BATCH_BUFFER_END;
  	struct drm_i915_gem_execbuffer2 execbuf;
  	struct drm_i915_gem_exec_object2 obj;
  	unsigned engines[16], nengine;
 
  	nengine = 0;
-	for_each_physical_engine(e, fd)
-		engines[nengine++] = eb_ring(e);
+	__for_each_physical_engine(fd, e)
+		engines[nengine++] = e->flags;
  	igt_require(nengine);
 
  	memset(&obj, 0, sizeof(obj));

@@ -86,38 +87,40 @@ static void all(int fd, unsigned flags, int
 timeout, int ncpus)

tests/i915/gem_exec_create.c static void all(int fd, unsigned flags, int · line 96

Quoted 10 lines

tests/i915/gem_exec_create.c +3 -1

 
  	intel_detect_and_clear_missed_interrupts(fd);
  	igt_fork(child, ncpus) {
+		int i915;
  		struct timespec start, now;
  		unsigned long count;
  		double time;
-
+		i915 = gem_reopen_driver(fd);
+		gem_context_copy_engines(fd, 0, i915, 0);

Why do we need to reopen device and copy it's context? What's the purpose? Kasia

Quoted 44 lines

tests/i915/gem_exec_create.c +10 -10

  		count = 0;
  		clock_gettime(CLOCK_MONOTONIC, &start);
  		do {
  			for (int loop = 0; loop < 1024; loop++) {
  				for (int n = 0; n < nengine; n++) {
-					obj.handle =  gem_create(fd, 4096);
-					gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
+					obj.handle =  gem_create(i915, 4096);
+					gem_write(i915, obj.handle, 0, &bbe, sizeof(bbe));
  					execbuf.flags &= ~ENGINE_FLAGS;
  					execbuf.flags |= engines[n];
-					gem_execbuf(fd, &execbuf);
+					gem_execbuf(i915, &execbuf);
  					if (flags & LEAK)
-						gem_madvise(fd, obj.handle, I915_MADV_DONTNEED);
+						gem_madvise(i915, obj.handle, I915_MADV_DONTNEED);
  					else
-						gem_close(fd, obj.handle);
+						gem_close(i915, obj.handle);
  				}
  			}
  			count += nengine * 1024;
  			clock_gettime(CLOCK_MONOTONIC, &now);
  		} while (elapsed(&start, &now) < timeout); /* Hang detection ~120s */
-		obj.handle =  gem_create(fd, 4096);
-		gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
+		obj.handle =  gem_create(i915, 4096);
+		gem_write(i915, obj.handle, 0, &bbe, sizeof(bbe));
  		for (int n = 0; n < nengine; n++) {
  			execbuf.flags &= ~ENGINE_FLAGS;
  			execbuf.flags |= engines[n];
-			gem_execbuf(fd, &execbuf);
+			gem_execbuf(i915, &execbuf);
  		}
-		gem_sync(fd, obj.handle);
-		gem_close(fd, obj.handle);
+		gem_sync(i915, obj.handle);
+		gem_close(i915, obj.handle);
  		clock_gettime(CLOCK_MONOTONIC, &now);
 

time = elapsed(&start, &now) / count;
--
2.25.0

Quoted 4 lines

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Quoted 9 lines

After fork we need to transfer the engine map from parent fd default context. (gem_context_copy_engines)

Also, since we have __for_each_physical_engine in the top level igt_fixture, I think _all_ subtests run with default ctx engine map. So all which submit to one from either all_engines or ppgtt_engines need to make sure engine maps are aligned.

it was suggested by tvrtko in one of the patch: https://patchwork.freedesktop.org/patch/351622/?series=72889&rev=1

I was sadly wrong there, it is not required to reopen and copy engines after fork. So __for_each_physical_engine should do it in this case.

Regards,

Tvrtko

P.S. You could also dynamically size (allocate) the engines array while at it to be more in line with the other recent cleanups.

Hm.. actually, do we even need the array? Andi, __for_each_physical_engine will always create engine map with consecutive e->flags integers, right? There can be no holes?

Hi Tvrtko,

Quoted 20 lines

Quoted 9 lines

After fork we need to transfer the engine map from parent fd default context. (gem_context_copy_engines)

Also, since we have __for_each_physical_engine in the top level igt_fixture, I think _all_ subtests run with default ctx engine map. So all which submit to one from either all_engines or ppgtt_engines need to make sure engine maps are aligned.

it was suggested by tvrtko in one of the patch: https://patchwork.freedesktop.org/patch/351622/?series=72889&rev=1

I was sadly wrong there, it is not required to reopen and copy engines after fork. So __for_each_physical_engine should do it in this case.

Regards,

Tvrtko

P.S. You could also dynamically size (allocate) the engines array while at it to be more in line with the other recent cleanups.

Quoted 3 lines

Hm.. actually, do we even need the array? Andi, __for_each_physical_engine will always create engine map with consecutive e->flags integers, right? There can be no holes?

yes, you are right, we do not need in this case the array and there shouldn't be holes, indeed.

Hi Tvrtko,

Quoted 21 lines

Quoted 10 lines

After fork we need to transfer the engine map from parent fd default context. (gem_context_copy_engines)

Also, since we have __for_each_physical_engine in the top level igt_fixture, I think _all_ subtests run with default ctx engine map. So all which submit to one from either all_engines or ppgtt_engines need to make sure engine maps are aligned.

it was suggested by tvrtko in one of the patch:
https://patchwork.freedesktop.org/patch/351622/?series=72889&rev=1

I was sadly wrong there, it is not required to reopen and copy engines after fork. So __for_each_physical_engine should do it in this case.

Regards,

Tvrtko

P.S. You could also dynamically size (allocate) the engines array while at it to be more in line with the other recent cleanups.

Quoted 3 lines

Hm.. actually, do we even need the array? Andi,
__for_each_physical_engine will always create engine map with consecutive e->flags integers, right? There can be no holes?

yes, you are right, we do not need in this case the array and there shouldn't be holes, indeed.

Hi Andi,Tvrtko ,

Could you please suggest me which would be the best way ?

Thanks,
Gowtham

-----Original Message-----
From: Tvrtko Ursulin <[email protected]>
Sent: Friday, March 6, 2020 7:37 PM
To: Shyti, Andi <[email protected]>
Cc: Ch, Sai Gowtham <[email protected]>; Dec, Katarzyna <[email protected]>; [email protected]
Subject: Re: [igt-dev] [PATCH] [PATCH i-g-t]tests/i915/gem_exec_create: Added __for_each_physical_engine to utilize all available engines.

Quoted 27 lines · Andi Shyti

Hi Tvrtko,

Quoted 24 lines

Quoted 10 lines

After fork we need to transfer the engine map from parent fd default context. (gem_context_copy_engines)

Also, since we have __for_each_physical_engine in the top level igt_fixture, I think _all_ subtests run with default ctx engine map. So all which submit to one from either all_engines or ppgtt_engines need to make sure engine maps are aligned.

it was suggested by tvrtko in one of the patch:
https://patchwork.freedesktop.org/patch/351622/?series=72889&rev=1

I was sadly wrong there, it is not required to reopen and copy engines after fork. So __for_each_physical_engine should do it in this case.

Regards,

Tvrtko

P.S. You could also dynamically size (allocate) the engines array while at it to be more in line with the other recent cleanups.

Hm.. actually, do we even need the array? Andi,
__for_each_physical_engine will always create engine map with consecutive e->flags integers, right? There can be no holes?

Quoted 2 lines

yes, you are right, we do not need in this case the array and there shouldn't be holes, indeed.

Then the question is whether it is a good idea to rely on this from tests or not? I can't imagine a scenario where IGT couldn't create a map with no holes but maybe my imagination is bad.

Regards,

Tvrtko

From: saigowth <[email protected]>