diff mbox

[igt] igt/gem_exec_params: Delete the circularly defined tests

Message ID 20170815225740.383-1-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson Aug. 15, 2017, 10:57 p.m. UTC
For the set of execbuf flags who by definition are based on whether or
not the kernel supports that feature, the ultimate arbiter of whether or
not the kernel accepts the flag is the kernel. The negative tests were
second guessing the kernel and not checking behaviour. Indeed, the tests
failed quite spectacularly to spot a 5 year old bug in engine selection.

The dubious invalid-flag negative test remains. It will always be
a catch-22, either the test is neutered before the kernel, or it will
lag behind the kernel and fail. It doesn't actually tell us if we fluked
out in failing the execbuf, but by testing all unknown flags it
functions better at the role it was meant for (that all as of yet unused
flags are not accepted).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_exec_params.c | 277 ++++++++----------------------------------------
 1 file changed, 42 insertions(+), 235 deletions(-)

Comments

Arkadiusz Hiler Aug. 18, 2017, 9:33 a.m. UTC | #1
On Tue, Aug 15, 2017 at 11:57:40PM +0100, Chris Wilson wrote:
> For the set of execbuf flags who by definition are based on whether or
> not the kernel supports that feature, the ultimate arbiter of whether or
> not the kernel accepts the flag is the kernel. The negative tests were
> second guessing the kernel and not checking behaviour. Indeed, the tests
> failed quite spectacularly to spot a 5 year old bug in engine selection.
> 
> The dubious invalid-flag negative test remains. It will always be
> a catch-22, either the test is neutered before the kernel, or it will
> lag behind the kernel and fail. It doesn't actually tell us if we fluked
> out in failing the execbuf, but by testing all unknown flags it
> functions better at the role it was meant for (that all as of yet unused
> flags are not accepted).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
diff mbox

Patch

diff --git a/tests/gem_exec_params.c b/tests/gem_exec_params.c
index ba6d67cb..e3432cba 100644
--- a/tests/gem_exec_params.c
+++ b/tests/gem_exec_params.c
@@ -25,54 +25,15 @@ 
  *
  */
 
-#include "igt.h"
-#include <unistd.h>
-#include <stdlib.h>
-#include <stdint.h>
-#include <stdio.h>
-#include <string.h>
-#include <fcntl.h>
-#include <inttypes.h>
-#include <errno.h>
-#include <sys/stat.h>
 #include <sys/ioctl.h>
-#include <sys/time.h>
-#include "drm.h"
+#include <errno.h>
 
+#include "igt.h"
 
-#define LOCAL_I915_EXEC_VEBOX (4<<0)
-#define LOCAL_I915_EXEC_BSD_MASK (3<<13)
-#define LOCAL_I915_EXEC_BSD_RING1 (1<<13)
-#define LOCAL_I915_EXEC_BSD_RING2 (2<<13)
-#define LOCAL_I915_EXEC_RESOURCE_STREAMER (1<<15)
 #define LOCAL_I915_EXEC_FENCE_IN (1 << 16)
 #define LOCAL_I915_EXEC_FENCE_OUT (1 << 17)
 #define LOCAL_I915_EXEC_BATCH_FIRST (1 << 18)
 
-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;
-}
-
 static bool has_exec_batch_first(int fd)
 {
 	int val = -1;
@@ -182,158 +143,30 @@  static void test_batch_first(int fd)
 struct drm_i915_gem_execbuffer2 execbuf;
 struct drm_i915_gem_exec_object2 gem_exec[1];
 uint32_t batch[2] = {MI_BATCH_BUFFER_END};
-uint32_t handle, devid;
+uint32_t devid;
 int fd;
 
 igt_main
 {
-	const struct intel_execution_engine *e;
-
 	igt_fixture {
 		fd = drm_open_driver(DRIVER_INTEL);
 		igt_require_gem(fd);
 
 		devid = intel_get_drm_devid(fd);
 
-		handle = gem_create(fd, 4096);
-		gem_write(fd, handle, 0, batch, sizeof(batch));
-
-		gem_exec[0].handle = handle;
-		gem_exec[0].relocation_count = 0;
-		gem_exec[0].relocs_ptr = 0;
-		gem_exec[0].alignment = 0;
-		gem_exec[0].offset = 0;
-		gem_exec[0].flags = 0;
-		gem_exec[0].rsvd1 = 0;
-		gem_exec[0].rsvd2 = 0;
+		gem_exec[0].handle =  gem_create(fd, 4096);
+		gem_write(fd, gem_exec[0].handle, 0, batch, sizeof(batch));
 
 		execbuf.buffers_ptr = to_user_pointer(gem_exec);
 		execbuf.buffer_count = 1;
-		execbuf.batch_start_offset = 0;
-		execbuf.batch_len = 8;
-		execbuf.cliprects_ptr = 0;
-		execbuf.num_cliprects = 0;
-		execbuf.DR1 = 0;
-		execbuf.DR4 = 0;
-		execbuf.flags = 0;
-		i915_execbuffer2_set_context_id(execbuf, 0);
-		execbuf.rsvd2 = 0;
-	}
-
-	igt_subtest("control") {
-		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 { \
-		igt_assert(drmIoctl(fd, \
-				    DRM_IOCTL_I915_GEM_EXECBUFFER2, \
-				    &execbuf) == -1); \
-		igt_assert_eq(errno, expected_errno); \
-	} while(0)
-
-	igt_subtest("no-bsd") {
-		igt_require(!gem_has_bsd(fd));
-		execbuf.flags = I915_EXEC_BSD;
-		RUN_FAIL(EINVAL);
-	}
-	igt_subtest("no-blt") {
-		igt_require(!gem_has_blt(fd));
-		execbuf.flags = I915_EXEC_BLT;
-		RUN_FAIL(EINVAL);
-	}
-	igt_subtest("no-vebox") {
-		igt_require(!gem_has_vebox(fd));
-		execbuf.flags = LOCAL_I915_EXEC_VEBOX;
-		RUN_FAIL(EINVAL);
-	}
-	igt_subtest("invalid-ring") {
-		execbuf.flags = I915_EXEC_RING_MASK;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("invalid-ring2") {
-		execbuf.flags = LOCAL_I915_EXEC_VEBOX+1;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("invalid-bsd-ring") {
-		igt_require(gem_has_bsd2(fd));
-		execbuf.flags = I915_EXEC_BSD | LOCAL_I915_EXEC_BSD_MASK;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("invalid-bsd1-flag-on-render") {
-		execbuf.flags = I915_EXEC_RENDER | LOCAL_I915_EXEC_BSD_RING1;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("invalid-bsd2-flag-on-render") {
-		execbuf.flags = I915_EXEC_RENDER | LOCAL_I915_EXEC_BSD_RING2;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("invalid-bsd1-flag-on-blt") {
-		execbuf.flags = I915_EXEC_BLT | LOCAL_I915_EXEC_BSD_RING1;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("invalid-bsd2-flag-on-blt") {
-		execbuf.flags = I915_EXEC_BLT | LOCAL_I915_EXEC_BSD_RING2;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("invalid-bsd1-flag-on-vebox") {
-		igt_require(gem_has_vebox(fd));
-		execbuf.flags = LOCAL_I915_EXEC_VEBOX | LOCAL_I915_EXEC_BSD_RING1;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("invalid-bsd2-flag-on-vebox") {
-		igt_require(gem_has_vebox(fd));
-		execbuf.flags = LOCAL_I915_EXEC_VEBOX | LOCAL_I915_EXEC_BSD_RING2;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("rel-constants-invalid-ring") {
-		igt_require(gem_has_bsd(fd));
-		execbuf.flags = I915_EXEC_BSD | I915_EXEC_CONSTANTS_ABSOLUTE;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("rel-constants-invalid-rel-gen5") {
-		igt_require(intel_gen(devid) > 5);
-		execbuf.flags = I915_EXEC_RENDER | I915_EXEC_CONSTANTS_REL_SURFACE;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("rel-constants-invalid") {
-		execbuf.flags = I915_EXEC_RENDER | (I915_EXEC_CONSTANTS_REL_SURFACE+(1<<6));
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("sol-reset-invalid") {
-		igt_require(gem_has_bsd(fd));
-		execbuf.flags = I915_EXEC_BSD | I915_EXEC_GEN7_SOL_RESET;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("sol-reset-not-gen7") {
-		igt_require(intel_gen(devid) != 7);
-		execbuf.flags = I915_EXEC_RENDER | I915_EXEC_GEN7_SOL_RESET;
-		RUN_FAIL(EINVAL);
 	}
 
 	igt_subtest("secure-non-root") {
 		igt_fork(child, 1) {
 			igt_drop_root();
 
-			execbuf.flags = I915_EXEC_RENDER | I915_EXEC_SECURE;
-			RUN_FAIL(EPERM);
+			execbuf.flags = I915_EXEC_SECURE;
+			igt_assert_eq(__gem_execbuf(fd, &execbuf), -EPERM);
 		}
 
 		igt_waitchildren();
@@ -341,12 +174,10 @@  igt_main
 
 	igt_subtest("secure-non-master") {
 		do_or_die(drmDropMaster(fd));
-		execbuf.flags = I915_EXEC_RENDER | I915_EXEC_SECURE;
-		RUN_FAIL(EPERM);
+		execbuf.flags = I915_EXEC_SECURE;
+		igt_assert_eq(__gem_execbuf(fd, &execbuf), -EPERM);
 		do_or_die(drmSetMaster(fd));
-		igt_assert(drmIoctl(fd,
-				    DRM_IOCTL_I915_GEM_EXECBUFFER2,
-				    &execbuf) == 0);
+		gem_execbuf(fd, &execbuf);
 	}
 
 	/* HANDLE_LUT and NO_RELOC are already exercised by gem_exec_lut_handle,
@@ -354,84 +185,60 @@  igt_main
 	 * gem_exec_fence, invalid usage of EXEC_FENCE_IN is tested below. */
 
 	igt_subtest("invalid-flag") {
-		/* NOTE: This test intentionally exercise the next available
-		 * flag. Don't "fix" this testcase without adding the required
-		 * tests for the new flag first. */
-		execbuf.flags = I915_EXEC_RENDER | (LOCAL_I915_EXEC_BATCH_FIRST << 1);
-		RUN_FAIL(EINVAL);
+		/* Any individual unknown flag should result in an -EINVAL.
+		 * Since there is a lag between the kernel advertising
+		 * a new feature and us knowing about that feature, this
+		 * test *will* fail during the interim.
+		 */
+		for (int bit = ffs(__I915_EXEC_UNKNOWN_FLAGS) - 1;
+		     bit < 64;
+		     bit++) {
+			int err;
+
+			execbuf.flags = 1ull << bit;
+			err = __gem_execbuf(fd, &execbuf);
+			if (err == 0) {
+				igt_warn("Unknown flag (1ull << %d) accepted by execbuf\n",
+					 bit);
+			} else {
+				igt_assert_eq(err, -EINVAL);
+			}
+		}
+
+		/* Any combination of invalid flags should also be an error.
+		 * But that will take too long to test them all, so we assume
+		 * how the kernel will behave.
+		 */
+		execbuf.flags = __I915_EXEC_UNKNOWN_FLAGS;
+		igt_assert_eq(__gem_execbuf(fd, &execbuf), -EINVAL);
 	}
 
 	/* rsvd1 aka context id is already exercised  by gem_ctx_bad_exec */
 
 	igt_subtest("cliprects-invalid") {
-		igt_require(intel_gen(devid) >= 5);
 		execbuf.flags = 0;
 		execbuf.num_cliprects = 1;
-		RUN_FAIL(EINVAL);
+		igt_assert_eq(__gem_execbuf(fd, &execbuf), -EINVAL);
 		execbuf.num_cliprects = 0;
-	}
-
-	igt_subtest("rs-invalid-on-bsd-ring") {
-		igt_require(IS_HASWELL(devid) || intel_gen(devid) >= 8);
-		execbuf.flags = I915_EXEC_BSD | LOCAL_I915_EXEC_RESOURCE_STREAMER;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("rs-invalid-on-blt-ring") {
-		igt_require(IS_HASWELL(devid) || intel_gen(devid) >= 8);
-		execbuf.flags = I915_EXEC_BLT | LOCAL_I915_EXEC_RESOURCE_STREAMER;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("rs-invalid-on-vebox-ring") {
-		igt_require(IS_HASWELL(devid) || intel_gen(devid) >= 8);
-		execbuf.flags = I915_EXEC_VEBOX | LOCAL_I915_EXEC_RESOURCE_STREAMER;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("rs-invalid-gen") {
-		igt_require(!IS_HASWELL(devid) && intel_gen(devid) < 8);
-		execbuf.flags = I915_EXEC_RENDER | LOCAL_I915_EXEC_RESOURCE_STREAMER;
-		RUN_FAIL(EINVAL);
+		gem_execbuf(fd, &execbuf);
 	}
 
 	igt_subtest("invalid-fence-in") {
 		igt_require(gem_has_exec_fence(fd));
 		execbuf.flags = LOCAL_I915_EXEC_FENCE_IN;
 		execbuf.rsvd2 = -1;
-		RUN_FAIL(EINVAL);
+		igt_assert_eq(__gem_execbuf(fd, &execbuf), -EINVAL);
 		execbuf.rsvd2 = fd;
-		RUN_FAIL(EINVAL);
-	}
-
-	igt_subtest("rsvd2-dirt") {
-		igt_require(!gem_has_exec_fence(fd));
+		igt_assert_eq(__gem_execbuf(fd, &execbuf), -EINVAL);
 		execbuf.flags = 0;
-		execbuf.rsvd2 = 1;
-		RUN_FAIL(EINVAL);
-		execbuf.rsvd2 = 0;
+		gem_execbuf(fd, &execbuf);
 	}
 
 	igt_subtest("batch-first")
 		test_batch_first(fd);
 
-#define DIRT(name) \
-	igt_subtest(#name "-dirt") { \
-		execbuf.flags = 0; \
-		execbuf.name = 1; \
-		RUN_FAIL(EINVAL); \
-		execbuf.name = 0; \
-	}
-
-	DIRT(cliprects_ptr);
-	DIRT(DR1);
-	DIRT(DR4);
-#undef DIRT
-
-#undef RUN_FAIL
-
 	igt_fixture {
-		gem_close(fd, handle);
+		gem_close(fd, gem_exec[0].handle);
 
 		close(fd);
 	}