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

Message ID 1454059293-16452-1-git-send-email-daniele.ceraolospurio@intel.com
State New
Headers show

Commit Message

Daniele Ceraolo Spurio Jan. 29, 2016, 9:21 a.m. UTC
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(-)

Comments

Chris Wilson Jan. 29, 2016, 10:58 a.m. UTC | #1
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
Daniele Ceraolo Spurio Jan. 29, 2016, 11:16 a.m. UTC | #2
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
Chris Wilson Jan. 29, 2016, 11:35 a.m. UTC | #3
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
Daniele Ceraolo Spurio Jan. 29, 2016, 11:58 a.m. UTC | #4
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
Chris Wilson Jan. 29, 2016, 12:11 p.m. UTC | #5
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

Patch
diff mbox

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);