[PATCH i-g-t] tests/gem_exec_basic: don't use gem_require_ring to check ring availability

[PATCH i-g-t] tests/gem_exec_basic: don't use gem_require_ring to check ring availability

[daniele.ceraolospurio]

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

gem_require_ring will submit an execbuf using the provided flags and
skip the test if the ioctl fails. This test is however designed to catch
issues with the ioctl, so it should fail if the ioctl fails on a ring
that the HW possesses.

Instead of using gem_require_ring we can use the getparam ioctl. The new
checker has been added to the test file and not to the commmon library
because this test is the only special case where we want to not use
gem_has_ring

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 tests/gem_exec_basic.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/tests/gem_exec_basic.c b/tests/gem_exec_basic.c
index 3f91b78..9a5de64 100644
--- a/tests/gem_exec_basic.c
+++ b/tests/gem_exec_basic.c
@@ -25,6 +25,30 @@
 
 IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl rings.");
 
+static bool has_ring(int fd, unsigned ring)
+{
+	switch (ring & I915_EXEC_RING_MASK) {
+	case 0:
+	case I915_EXEC_RENDER:
+		return true;
+
+	case I915_EXEC_BSD:
+		if (ring & 3<<13)
+			return gem_has_bsd2(fd);
+		else
+			return gem_has_bsd(fd);
+
+	case I915_EXEC_BLT:
+		return gem_has_blt(fd);
+
+	case I915_EXEC_VEBOX:
+		return gem_has_vebox(fd);
+	}
+
+	igt_assert_f(0, "invalid exec flag 0x%x\n", ring);
+	return false;
+}
+
 static void noop(int fd, unsigned ring)
 {
 	uint32_t bbe = MI_BATCH_BUFFER_END;
@@ -32,7 +56,11 @@ static void noop(int fd, unsigned ring)
 	struct drm_i915_gem_exec_object2 exec;
 	int ret;
 
-	gem_require_ring(fd, ring);
+	/* we can't use gem_require_ring here because otherwise the test will
+	 * skip if there is a bug with the flags, but we want to fail in that
+	 * situation
+	 */
+	igt_require(has_ring(fd, ring));
 
 	memset(&exec, 0, sizeof(exec));
 	exec.handle = gem_create(fd, 4096);
-- 
1.9.1

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

Re: [PATCH i-g-t] tests/gem_exec_basic: don't use gem_require_ring to check ring availability

[Chris Wilson]

On Fri, Jan 29, 2016 at 09:21:33AM +0000, daniele.ceraolospurio@intel.com wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> gem_require_ring will submit an execbuf using the provided flags and
> skip the test if the ioctl fails. This test is however designed to catch
> issues with the ioctl, so it should fail if the ioctl fails on a ring
> that the HW possesses.
> 
> Instead of using gem_require_ring we can use the getparam ioctl. The new
> checker has been added to the test file and not to the commmon library
> because this test is the only special case where we want to not use
> gem_has_ring

That would be gem_exec_param.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/gem_exec_basic: don't use gem_require_ring to check ring availability

[Daniele Ceraolo Spurio]

On 29/01/16 10:58, Chris Wilson wrote:
> On Fri, Jan 29, 2016 at 09:21:33AM +0000, daniele.ceraolospurio@intel.com wrote:
>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>> gem_require_ring will submit an execbuf using the provided flags and
>> skip the test if the ioctl fails. This test is however designed to catch
>> issues with the ioctl, so it should fail if the ioctl fails on a ring
>> that the HW possesses.
>>
>> Instead of using gem_require_ring we can use the getparam ioctl. The new
>> checker has been added to the test file and not to the commmon library
>> because this test is the only special case where we want to not use
>> gem_has_ring
> That would be gem_exec_param.
> -Chris

I don't understand what you mean, can you elaborate a bit?

What I wanted to fix here is the fact that the logic to skip the test 
and the test itself are identical, which means that this test can't 
fail. As far as I can tell gem_exec_param is trying to catch errors in 
the handling of invalid flags, while in this test we check for errors in 
the handling of valid flags instead.

Thanks,
Daniele

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

Re: [PATCH i-g-t] tests/gem_exec_basic: don't use gem_require_ring to check ring availability

[Chris Wilson]

On Fri, Jan 29, 2016 at 11:16:37AM +0000, Daniele Ceraolo Spurio wrote:
> 
> 
> On 29/01/16 10:58, Chris Wilson wrote:
> >On Fri, Jan 29, 2016 at 09:21:33AM +0000, daniele.ceraolospurio@intel.com wrote:
> >>From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >>
> >>gem_require_ring will submit an execbuf using the provided flags and
> >>skip the test if the ioctl fails. This test is however designed to catch
> >>issues with the ioctl, so it should fail if the ioctl fails on a ring
> >>that the HW possesses.
> >>
> >>Instead of using gem_require_ring we can use the getparam ioctl. The new
> >>checker has been added to the test file and not to the commmon library
> >>because this test is the only special case where we want to not use
> >>gem_has_ring
> >That would be gem_exec_param.
> >-Chris
> 
> I don't understand what you mean, can you elaborate a bit?

For the purposes of checking that the kernel honours the ABI, the tests
belong in gem_exec_params.

For the purposes of CI, a testing going from PASS -> SKIP is just as
indicative of a problem as test going from PASS -> FAIL or any other
state.
 
> What I wanted to fix here is the fact that the logic to skip the
> test and the test itself are identical, which means that this test
> can't fail. As far as I can tell gem_exec_param is trying to catch
> errors in the handling of invalid flags, while in this test we check
> for errors in the handling of valid flags instead.

Basically the logic is repeated, that is not an issue for its purpose.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/gem_exec_basic: don't use gem_require_ring to check ring availability

[Daniele Ceraolo Spurio]

On 29/01/16 11:35, Chris Wilson wrote:
> On Fri, Jan 29, 2016 at 11:16:37AM +0000, Daniele Ceraolo Spurio wrote:
>>
>> On 29/01/16 10:58, Chris Wilson wrote:
>>> On Fri, Jan 29, 2016 at 09:21:33AM +0000, daniele.ceraolospurio@intel.com wrote:
>>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>>
>>>> gem_require_ring will submit an execbuf using the provided flags and
>>>> skip the test if the ioctl fails. This test is however designed to catch
>>>> issues with the ioctl, so it should fail if the ioctl fails on a ring
>>>> that the HW possesses.
>>>>
>>>> Instead of using gem_require_ring we can use the getparam ioctl. The new
>>>> checker has been added to the test file and not to the commmon library
>>>> because this test is the only special case where we want to not use
>>>> gem_has_ring
>>> That would be gem_exec_param.
>>> -Chris
>> I don't understand what you mean, can you elaborate a bit?
> For the purposes of checking that the kernel honours the ABI, the tests
> belong in gem_exec_params.
>
> For the purposes of CI, a testing going from PASS -> SKIP is just as
> indicative of a problem as test going from PASS -> FAIL or any other
> state.

The difference would be that the CI system still reports that BAT 
succeeded if one or more tests go from PASS to SKIP (e.g. 
http://lists.freedesktop.org/archives/intel-gfx/2016-January/086586.html).

>   
>> What I wanted to fix here is the fact that the logic to skip the
>> test and the test itself are identical, which means that this test
>> can't fail. As far as I can tell gem_exec_param is trying to catch
>> errors in the handling of invalid flags, while in this test we check
>> for errors in the handling of valid flags instead.
> Basically the logic is repeated, that is not an issue for its purpose.
> -Chris

This patch can be dropped then.

Thanks,
Daniele

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

Re: [PATCH i-g-t] tests/gem_exec_basic: don't use gem_require_ring to check ring availability

[Chris Wilson]

On Fri, Jan 29, 2016 at 11:58:14AM +0000, Daniele Ceraolo Spurio wrote:
> 
> 
> On 29/01/16 11:35, Chris Wilson wrote:
> >On Fri, Jan 29, 2016 at 11:16:37AM +0000, Daniele Ceraolo Spurio wrote:
> >>
> >>On 29/01/16 10:58, Chris Wilson wrote:
> >>>On Fri, Jan 29, 2016 at 09:21:33AM +0000, daniele.ceraolospurio@intel.com wrote:
> >>>>From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >>>>
> >>>>gem_require_ring will submit an execbuf using the provided flags and
> >>>>skip the test if the ioctl fails. This test is however designed to catch
> >>>>issues with the ioctl, so it should fail if the ioctl fails on a ring
> >>>>that the HW possesses.
> >>>>
> >>>>Instead of using gem_require_ring we can use the getparam ioctl. The new
> >>>>checker has been added to the test file and not to the commmon library
> >>>>because this test is the only special case where we want to not use
> >>>>gem_has_ring
> >>>That would be gem_exec_param.
> >>>-Chris
> >>I don't understand what you mean, can you elaborate a bit?
> >For the purposes of checking that the kernel honours the ABI, the tests
> >belong in gem_exec_params.
> >
> >For the purposes of CI, a testing going from PASS -> SKIP is just as
> >indicative of a problem as test going from PASS -> FAIL or any other
> >state.
> 
> The difference would be that the CI system still reports that BAT
> succeeded if one or more tests go from PASS to SKIP (e.g. http://lists.freedesktop.org/archives/intel-gfx/2016-January/086586.html).

Fortunately, the results are sometimes even read.
 
> >>What I wanted to fix here is the fact that the logic to skip the
> >>test and the test itself are identical, which means that this test
> >>can't fail. As far as I can tell gem_exec_param is trying to catch
> >>errors in the handling of invalid flags, while in this test we check
> >>for errors in the handling of valid flags instead.
> >Basically the logic is repeated, that is not an issue for its purpose.
> >-Chris
> 
> This patch can be dropped then.

But can be refactored for gem_exec_param!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t] tests/gem_exec_params: test all valid execution flags

[daniele.ceraolospurio]

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

The control subtest has been extended to check the execution flags for
all the rings that are present in the HW.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 tests/gem_exec_params.c | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/tests/gem_exec_params.c b/tests/gem_exec_params.c
index 06dfd63..e192150 100644
--- a/tests/gem_exec_params.c
+++ b/tests/gem_exec_params.c
@@ -46,6 +46,30 @@
 #define LOCAL_I915_EXEC_BSD_RING2 (2<<13)
 #define LOCAL_I915_EXEC_RESOURCE_STREAMER (1<<15)
 
+static bool has_ring(int fd, unsigned ring_exec_flags)
+{
+	switch (ring_exec_flags & I915_EXEC_RING_MASK) {
+	case 0:
+	case I915_EXEC_RENDER:
+		return true;
+
+	case I915_EXEC_BSD:
+		if (ring_exec_flags & LOCAL_I915_EXEC_BSD_MASK)
+			return gem_has_bsd2(fd);
+		else
+			return gem_has_bsd(fd);
+
+	case I915_EXEC_BLT:
+		return gem_has_blt(fd);
+
+	case I915_EXEC_VEBOX:
+		return gem_has_vebox(fd);
+	}
+
+	igt_assert_f(0, "invalid exec flag 0x%x\n", ring_exec_flags);
+	return false;
+}
+
 struct drm_i915_gem_execbuffer2 execbuf;
 struct drm_i915_gem_exec_object2 gem_exec[1];
 uint32_t batch[2] = {MI_BATCH_BUFFER_END};
@@ -54,6 +78,8 @@ int fd;
 
 igt_main
 {
+	const struct intel_execution_engine *e;
+
 	igt_fixture {
 		fd = drm_open_driver(DRIVER_INTEL);
 
@@ -85,13 +111,12 @@ igt_main
 	}
 
 	igt_subtest("control") {
-		igt_assert(drmIoctl(fd,
-				    DRM_IOCTL_I915_GEM_EXECBUFFER2,
-				    &execbuf) == 0);
-		execbuf.flags = I915_EXEC_RENDER;
-		igt_assert(drmIoctl(fd,
-				    DRM_IOCTL_I915_GEM_EXECBUFFER2,
-				    &execbuf) == 0);
+		for (e = intel_execution_engines; e->name; e++) {
+			if (has_ring(fd, e->exec_id | e->flags)) {
+				execbuf.flags = e->exec_id | e->flags;
+				gem_execbuf(fd, &execbuf);
+			}
+		}
 	}
 
 #define RUN_FAIL(expected_errno) do { \
-- 
1.9.1

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

Re: [PATCH i-g-t] tests/gem_exec_params: test all valid execution flags

[Daniel Vetter]

On Mon, Feb 01, 2016 at 02:24:37PM +0000, daniele.ceraolospurio@intel.com wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> The control subtest has been extended to check the execution flags for
> all the rings that are present in the HW.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Applied, thanks.
-Daniel

> ---
>  tests/gem_exec_params.c | 39 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/gem_exec_params.c b/tests/gem_exec_params.c
> index 06dfd63..e192150 100644
> --- a/tests/gem_exec_params.c
> +++ b/tests/gem_exec_params.c
> @@ -46,6 +46,30 @@
>  #define LOCAL_I915_EXEC_BSD_RING2 (2<<13)
>  #define LOCAL_I915_EXEC_RESOURCE_STREAMER (1<<15)
>  
> +static bool has_ring(int fd, unsigned ring_exec_flags)
> +{
> +	switch (ring_exec_flags & I915_EXEC_RING_MASK) {
> +	case 0:
> +	case I915_EXEC_RENDER:
> +		return true;
> +
> +	case I915_EXEC_BSD:
> +		if (ring_exec_flags & LOCAL_I915_EXEC_BSD_MASK)
> +			return gem_has_bsd2(fd);
> +		else
> +			return gem_has_bsd(fd);
> +
> +	case I915_EXEC_BLT:
> +		return gem_has_blt(fd);
> +
> +	case I915_EXEC_VEBOX:
> +		return gem_has_vebox(fd);
> +	}
> +
> +	igt_assert_f(0, "invalid exec flag 0x%x\n", ring_exec_flags);
> +	return false;
> +}
> +
>  struct drm_i915_gem_execbuffer2 execbuf;
>  struct drm_i915_gem_exec_object2 gem_exec[1];
>  uint32_t batch[2] = {MI_BATCH_BUFFER_END};
> @@ -54,6 +78,8 @@ int fd;
>  
>  igt_main
>  {
> +	const struct intel_execution_engine *e;
> +
>  	igt_fixture {
>  		fd = drm_open_driver(DRIVER_INTEL);
>  
> @@ -85,13 +111,12 @@ igt_main
>  	}
>  
>  	igt_subtest("control") {
> -		igt_assert(drmIoctl(fd,
> -				    DRM_IOCTL_I915_GEM_EXECBUFFER2,
> -				    &execbuf) == 0);
> -		execbuf.flags = I915_EXEC_RENDER;
> -		igt_assert(drmIoctl(fd,
> -				    DRM_IOCTL_I915_GEM_EXECBUFFER2,
> -				    &execbuf) == 0);
> +		for (e = intel_execution_engines; e->name; e++) {
> +			if (has_ring(fd, e->exec_id | e->flags)) {
> +				execbuf.flags = e->exec_id | e->flags;
> +				gem_execbuf(fd, &execbuf);
> +			}
> +		}
>  	}
>  
>  #define RUN_FAIL(expected_errno) do { \
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx