[Intel-gfx] [PATCH i-g-t 1/2] i915/query: Cross-check engine list against execbuf interface

[Intel-gfx] [PATCH i-g-t 1/2] i915/query: Cross-check engine list against execbuf interface

[Chris Wilson]

Check that every engine listed can be used in execbuf.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Andi Shyti <andi.shyti@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/i915/i915_query.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/tests/i915/i915_query.c b/tests/i915/i915_query.c
index e7c6fc91e..cdf2d3403 100644
--- a/tests/i915/i915_query.c
+++ b/tests/i915/i915_query.c
@@ -633,6 +633,16 @@ has_engine(struct drm_i915_query_engine_info *engines,
 	return false;
 }
 
+static void gem_context_reset_engines(int i915, uint32_t ctx)
+{
+	struct drm_i915_gem_context_param param = {
+		.ctx_id = ctx,
+		.param = I915_CONTEXT_PARAM_ENGINES,
+	};
+
+	gem_context_set_param(i915, &param);
+}
+
 static void engines(int fd)
 {
 	struct drm_i915_query_engine_info *engines;
@@ -678,10 +688,24 @@ static void engines(int fd)
 	igt_assert_eq(engines->rsvd[1], 0);
 	igt_assert_eq(engines->rsvd[2], 0);
 
-	/* Check results match the legacy GET_PARAM (where we can). */
+	/* Confirm the individual engines exist with EXECBUFFER2 */
 	for (i = 0; i < engines->num_engines; i++) {
 		struct drm_i915_engine_info *engine =
 			(struct drm_i915_engine_info *)&engines->engines[i];
+		I915_DEFINE_CONTEXT_PARAM_ENGINES(p_engines, 1) = {
+			.engines = { engine->engine }
+		};
+		struct drm_i915_gem_context_param param = {
+			.param = I915_CONTEXT_PARAM_ENGINES,
+			.value = to_user_pointer(&p_engines),
+			.size = sizeof(p_engines),
+		};
+
+		struct drm_i915_gem_exec_object2 obj = {};
+		struct drm_i915_gem_execbuffer2 execbuf = {
+			.buffers_ptr = to_user_pointer(&obj),
+			.buffer_count = 1,
+		};
 
 		igt_debug("%u: class=%u instance=%u flags=%llx capabilities=%llx\n",
 			  i,
@@ -689,6 +713,15 @@ static void engines(int fd)
 			  engine->engine.engine_instance,
 			  engine->flags,
 			  engine->capabilities);
+		gem_context_set_param(fd, &param);
+		igt_assert_eq(__gem_execbuf(fd, &execbuf), -ENOENT);
+	}
+	gem_context_reset_engines(fd, 0);
+
+	/* Check results match the legacy GET_PARAM (where we can). */
+	for (i = 0; i < engines->num_engines; i++) {
+		struct drm_i915_engine_info *engine =
+			(struct drm_i915_engine_info *)&engines->engines[i];
 
 		switch (engine->engine.engine_class) {
 		case I915_ENGINE_CLASS_RENDER:
-- 
2.29.2

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

[Intel-gfx] [PATCH i-g-t 2/2] i915/query: Directly check query results against GETPARAM

[Chris Wilson]

Simplify the cross-check by asserting that the existence of an engine in
the list matches the existence of the engine as reported by GETPARAM.
By using the comparison, we check both directions at once.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Petri Latvala <petri.latvala@intel.com>
---
 tests/i915/i915_query.c | 49 ++++++++---------------------------------
 1 file changed, 9 insertions(+), 40 deletions(-)

diff --git a/tests/i915/i915_query.c b/tests/i915/i915_query.c
index cdf2d3403..b415650ae 100644
--- a/tests/i915/i915_query.c
+++ b/tests/i915/i915_query.c
@@ -719,46 +719,15 @@ static void engines(int fd)
 	gem_context_reset_engines(fd, 0);
 
 	/* Check results match the legacy GET_PARAM (where we can). */
-	for (i = 0; i < engines->num_engines; i++) {
-		struct drm_i915_engine_info *engine =
-			(struct drm_i915_engine_info *)&engines->engines[i];
-
-		switch (engine->engine.engine_class) {
-		case I915_ENGINE_CLASS_RENDER:
-			/* Will be tested later. */
-			break;
-		case I915_ENGINE_CLASS_COPY:
-			igt_assert(gem_has_blt(fd));
-			break;
-		case I915_ENGINE_CLASS_VIDEO:
-			switch (engine->engine.engine_instance) {
-			case 0:
-				igt_assert(gem_has_bsd(fd));
-				break;
-			case 1:
-				igt_assert(gem_has_bsd2(fd));
-				break;
-			}
-			break;
-		case I915_ENGINE_CLASS_VIDEO_ENHANCE:
-			igt_assert(gem_has_vebox(fd));
-			break;
-		default:
-			igt_assert(0);
-		}
-	}
-
-	/* Reverse check to the above - all GET_PARAM engines are present. */
-	igt_assert(has_engine(engines, I915_ENGINE_CLASS_RENDER, 0));
-	if (gem_has_blt(fd))
-		igt_assert(has_engine(engines, I915_ENGINE_CLASS_COPY, 0));
-	if (gem_has_bsd(fd))
-		igt_assert(has_engine(engines, I915_ENGINE_CLASS_VIDEO, 0));
-	if (gem_has_bsd2(fd))
-		igt_assert(has_engine(engines, I915_ENGINE_CLASS_VIDEO, 1));
-	if (gem_has_vebox(fd))
-		igt_assert(has_engine(engines, I915_ENGINE_CLASS_VIDEO_ENHANCE,
-				       0));
+	igt_assert_eq(has_engine(engines, I915_ENGINE_CLASS_RENDER, 0), 1);
+	igt_assert_eq(has_engine(engines, I915_ENGINE_CLASS_COPY, 0),
+		      gem_has_blt(fd));
+	igt_assert_eq(has_engine(engines, I915_ENGINE_CLASS_VIDEO, 0),
+		      gem_has_bsd(fd));
+	igt_assert_eq(has_engine(engines, I915_ENGINE_CLASS_VIDEO, 1),
+		      gem_has_bsd2(fd));
+	igt_assert_eq(has_engine(engines, I915_ENGINE_CLASS_VIDEO_ENHANCE, 0),
+		      gem_has_vebox(fd));
 
 	free(engines);
 }
-- 
2.29.2

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

Re: [Intel-gfx] [PATCH i-g-t 2/2] i915/query: Directly check query results against GETPARAM

[Petri Latvala]

On Mon, Dec 07, 2020 at 04:11:50PM +0000, Chris Wilson wrote:
> Simplify the cross-check by asserting that the existence of an engine in
> the list matches the existence of the engine as reported by GETPARAM.
> By using the comparison, we check both directions at once.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Petri Latvala <petri.latvala@intel.com>


For the series,
Reviewed-by: Petri Latvala <petri.latvala@intel.com>

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

Re: [Intel-gfx] [PATCH i-g-t 2/2] i915/query: Directly check query results against GETPARAM

[Tvrtko Ursulin]

On 08/12/2020 11:04, Petri Latvala wrote:
> On Mon, Dec 07, 2020 at 04:11:50PM +0000, Chris Wilson wrote:
>> Simplify the cross-check by asserting that the existence of an engine in
>> the list matches the existence of the engine as reported by GETPARAM.
>> By using the comparison, we check both directions at once.
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Petri Latvala <petri.latvala@intel.com>
> 
> 
> For the series,
> Reviewed-by: Petri Latvala <petri.latvala@intel.com>

Yeah it's a yes from me as well. Either test was merged with or before 
the engine map feature so it had to be a bit more backward compatible.

I wonder at which point we re-implement gem_has_xcs family to use the 
query and move the get_param based tests to a single legacy test.

Regards,

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

Re: [Intel-gfx] [PATCH i-g-t 2/2] i915/query: Directly check query results against GETPARAM

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-12-08 11:18:56)
> 
> On 08/12/2020 11:04, Petri Latvala wrote:
> > On Mon, Dec 07, 2020 at 04:11:50PM +0000, Chris Wilson wrote:
> >> Simplify the cross-check by asserting that the existence of an engine in
> >> the list matches the existence of the engine as reported by GETPARAM.
> >> By using the comparison, we check both directions at once.
> >>
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Petri Latvala <petri.latvala@intel.com>
> > 
> > 
> > For the series,
> > Reviewed-by: Petri Latvala <petri.latvala@intel.com>
> 
> Yeah it's a yes from me as well. Either test was merged with or before 
> the engine map feature so it had to be a bit more backward compatible.

As a sanity check,

drm/i915: Allow a context to define its set of engines
CommitDate: Wed May 22 08:40:31 2019 +0100

drm/i915: Engine discovery query
CommitDate: Wed May 22 14:17:55 2019 +0100

So they are paired. If the kernel supports the engine query, it will
support the engine map.

> I wonder at which point we re-implement gem_has_xcs family to use the 
> query and move the get_param based tests to a single legacy test.

gem_has_xcs() is a quirk of igt, and we are very very close to
completely removing it. The only place where it remains relevant is
verifying that we do not break the existing GETPARAM (so this test and
gem_exec_param).

That seems like an afternoon task to move the GETPARAM into a dungeon
and throw away the key.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 1/2] i915/query: Cross-check engine list against execbuf interface

[Andi Shyti]

Hi Chris,

> -	/* Check results match the legacy GET_PARAM (where we can). */
> +	/* Confirm the individual engines exist with EXECBUFFER2 */
>  	for (i = 0; i < engines->num_engines; i++) {
>  		struct drm_i915_engine_info *engine =
>  			(struct drm_i915_engine_info *)&engines->engines[i];
> +		I915_DEFINE_CONTEXT_PARAM_ENGINES(p_engines, 1) = {
> +			.engines = { engine->engine }
> +		};
> +		struct drm_i915_gem_context_param param = {
> +			.param = I915_CONTEXT_PARAM_ENGINES,
> +			.value = to_user_pointer(&p_engines),
> +			.size = sizeof(p_engines),
> +		};
> +
> +		struct drm_i915_gem_exec_object2 obj = {};
> +		struct drm_i915_gem_execbuffer2 execbuf = {
> +			.buffers_ptr = to_user_pointer(&obj),
> +			.buffer_count = 1,
> +		};
>  
>  		igt_debug("%u: class=%u instance=%u flags=%llx capabilities=%llx\n",
>  			  i,
> @@ -689,6 +713,15 @@ static void engines(int fd)
>  			  engine->engine.engine_instance,
>  			  engine->flags,
>  			  engine->capabilities);
> +		gem_context_set_param(fd, &param);
> +		igt_assert_eq(__gem_execbuf(fd, &execbuf), -ENOENT);
> +	}
> +	gem_context_reset_engines(fd, 0);
> +
> +	/* Check results match the legacy GET_PARAM (where we can). */
> +	for (i = 0; i < engines->num_engines; i++) {
> +		struct drm_i915_engine_info *engine =
> +			(struct drm_i915_engine_info *)&engines->engines[i];

I would have liked it with one single for loop, perhaps resetting
engines individually.

But this works, as well and I'm not strong with this:

Reviewed-by: Andi Shyti <andi.shyti@intel.com>

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