[PATCH] tests/gem_reloc_overflow: Add gen8+ specifc tests

[PATCH] tests/gem_reloc_overflow: Add gen8+ specifc tests

[rafael.barbalho]

From: Rafael Barbalho <rafael.barbalho@intel.com>

Broadwell introduces 64-bit relocation addresses which add extra
corner cases. The test was refactored slightly with some tests that
were in the source offset tests were moved to the more generic reloc
test area. The source offset tests are now gen aware and called twice to
test both cpu & gtt relocation paths. In addition 2 new gen8+ test
were added to the test:

* Relocation straddling page a page
* Insufficient space for a relocation at the end of the buffer.

Signed-off-by: Rafael Barbalho <rafael.barbalho@intel.com>

Conflicts:
	tests/gem_reloc_overflow.c
---
 tests/gem_reloc_overflow.c | 148 +++++++++++++++++++++++++++++++--------------
 1 file changed, 103 insertions(+), 45 deletions(-)

diff --git a/tests/gem_reloc_overflow.c b/tests/gem_reloc_overflow.c
index f7ba1d7..38ad2a5 100644
--- a/tests/gem_reloc_overflow.c
+++ b/tests/gem_reloc_overflow.c
@@ -24,6 +24,7 @@
  * Authors:
  *    Kees Cook <keescook@chromium.org>
  *    Daniel Vetter <daniel.vetter@ffwll.ch>
+ *    Rafael Barbalho <rafael.barbalho@intel.com>
  *
  */
 
@@ -60,13 +61,14 @@ struct drm_i915_gem_relocation_entry *reloc;
 uint32_t handle;
 uint32_t batch_handle;
 
-
-static void source_offset_tests(void)
+static void source_offset_tests(int devid, bool reloc_gtt)
 {
 	struct drm_i915_gem_relocation_entry single_reloc;
+	char *dst_gtt;
+	char *relocation_type;
 
 	igt_fixture {
-		handle = gem_create(fd, 4096);
+		handle = gem_create(fd, 8192);
 
 		execobjs[1].handle = batch_handle;
 		execobjs[1].relocation_count = 0;
@@ -76,21 +78,70 @@ static void source_offset_tests(void)
 		execobjs[0].relocation_count = 1;
 		execobjs[0].relocs_ptr = (uintptr_t) &single_reloc;
 		execbuf.buffer_count = 2;
+
+		if (reloc_gtt) {
+			dst_gtt = gem_mmap(fd, handle, 8192, PROT_READ | PROT_WRITE);
+			igt_assert(dst_gtt != MAP_FAILED);
+			gem_set_domain(fd, handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
+			memset(dst_gtt, 0, 8192);
+			munmap(dst_gtt, 8192);
+			relocation_type = "reloc-gtt";
+		} else {
+			relocation_type = "reloc-cpu";
+		}
 	}
 
-	igt_subtest("source-offset-end") {
-		single_reloc.offset = 4096 - 4;
-		single_reloc.delta = 0;
-		single_reloc.target_handle = handle;
-		single_reloc.read_domains = I915_GEM_DOMAIN_RENDER;
-		single_reloc.write_domain = I915_GEM_DOMAIN_RENDER;
-		single_reloc.presumed_offset = 0;
+	if (intel_gen(devid) >= 8) {
+		igt_subtest_f("source-offset-page-stradle-gen8+-%s", relocation_type) {
+			single_reloc.offset = 4096 - 4;
+			single_reloc.delta = 0;
+			single_reloc.target_handle = handle;
+			single_reloc.read_domains = I915_GEM_DOMAIN_RENDER;
+			single_reloc.write_domain = I915_GEM_DOMAIN_RENDER;
+			single_reloc.presumed_offset = 0;
+
+			igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) == 0);
+			single_reloc.delta = 1024;
+			igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) == 0);
+		}
 
-		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) == 0);
+		igt_subtest_f("source-offset-end-gen8+-%s", relocation_type) {
+			single_reloc.offset = 8192 - 8;
+			single_reloc.delta = 0;
+			single_reloc.target_handle = handle;
+			single_reloc.read_domains = I915_GEM_DOMAIN_RENDER;
+			single_reloc.write_domain = I915_GEM_DOMAIN_RENDER;
+			single_reloc.presumed_offset = 0;
+
+			igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) == 0);
+		}
+
+		igt_subtest_f("source-offset-overflow-gen8+-%s", relocation_type) {
+			single_reloc.offset = 8192 - 4;
+			single_reloc.delta = 0;
+			single_reloc.target_handle = handle;
+			single_reloc.read_domains = I915_GEM_DOMAIN_RENDER;
+			single_reloc.write_domain = I915_GEM_DOMAIN_RENDER;
+			single_reloc.presumed_offset = 0;
+
+			igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) != 0);
+			igt_assert(errno == EINVAL);
+		}
+	} else {
+		igt_subtest_f("source-offset-end-%s", relocation_type) {
+			single_reloc.offset = 8192 - 4;
+			single_reloc.delta = 0;
+			single_reloc.target_handle = handle;
+			single_reloc.read_domains = I915_GEM_DOMAIN_RENDER;
+			single_reloc.write_domain = I915_GEM_DOMAIN_RENDER;
+			single_reloc.presumed_offset = 0;
+
+			igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) == 0);
+		}
 	}
 
-	igt_subtest("source-offset-big") {
-		single_reloc.offset = 4096;
+	igt_subtest_f("source-offset-big-%s", relocation_type) {
+		single_reloc.offset = 8192;
 		single_reloc.delta = 0;
 		single_reloc.target_handle = handle;
 		single_reloc.read_domains = I915_GEM_DOMAIN_RENDER;
@@ -101,7 +152,7 @@ static void source_offset_tests(void)
 		igt_assert(errno == EINVAL);
 	}
 
-	igt_subtest("source-offset-negative") {
+	igt_subtest_f("source-offset-negative-%s", relocation_type) {
 		single_reloc.offset = (int64_t) -4;
 		single_reloc.delta = 0;
 		single_reloc.target_handle = handle;
@@ -113,7 +164,7 @@ static void source_offset_tests(void)
 		igt_assert(errno == EINVAL);
 	}
 
-	igt_subtest("source-offset-unaligned") {
+	igt_subtest_f("source-offset-unaligned-%s", relocation_type) {
 		single_reloc.offset = 1;
 		single_reloc.delta = 0;
 		single_reloc.target_handle = handle;
@@ -126,34 +177,6 @@ static void source_offset_tests(void)
 	}
 
 	igt_fixture {
-		execobjs[0].handle = batch_handle;
-		execobjs[0].relocation_count = 0;
-		execobjs[0].relocs_ptr = 0;
-
-		execbuf.buffer_count = 1;
-	}
-
-	igt_subtest("batch-start-unaligend") {
-		execbuf.batch_start_offset = 1;
-		execbuf.batch_len = 8;
-
-		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) != 0);
-		igt_assert(errno == EINVAL);
-	}
-
-	igt_subtest("batch-end-unaligend") {
-		execbuf.batch_start_offset = 0;
-		execbuf.batch_len = 7;
-
-		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) != 0);
-		igt_assert(errno == EINVAL);
-	}
-
-	igt_fixture {
-		/* Undo damage for next tests. */
-		execbuf.batch_start_offset = 0;
-		execbuf.batch_len = 8;
-
 		gem_close(fd, handle);
 	}
 }
@@ -185,6 +208,36 @@ static void reloc_tests(void)
 		igt_assert(errno == EINVAL);
 	}
 
+	igt_fixture {
+		execobjs[0].handle = batch_handle;
+		execobjs[0].relocation_count = 0;
+		execobjs[0].relocs_ptr = 0;
+
+		execbuf.buffer_count = 1;
+	}
+
+	igt_subtest("batch-start-unaligned") {
+		execbuf.batch_start_offset = 1;
+		execbuf.batch_len = 8;
+
+		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) != 0);
+		igt_assert(errno == EINVAL);
+	}
+
+	igt_subtest("batch-end-unaligned") {
+		execbuf.batch_start_offset = 0;
+		execbuf.batch_len = 7;
+
+		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) != 0);
+		igt_assert(errno == EINVAL);
+	}
+
+	igt_fixture {
+		/* Undo damage for next tests. */
+		execbuf.batch_start_offset = 0;
+		execbuf.batch_len = 8;
+	}
+
 	igt_subtest("wrapped-overflow") {
 		/* Attempt wrapped overflow entries. */
 		for (i = 0; i < num; i++) {
@@ -252,12 +305,16 @@ static void buffer_count_tests(void)
 
 igt_main
 {
+	int devid = 0;
+
 	igt_fixture {
 		int ring;
 		uint32_t batch_data [2] = { MI_NOOP, MI_BATCH_BUFFER_END };
 
 		fd = drm_open_any();
 
+		devid = intel_get_drm_devid(fd);
+
 		/* Create giant reloc buffer area. */
 		num = 257;
 		entries = ((1ULL << 32) / (num - 1));
@@ -271,7 +328,7 @@ igt_main
 		for (int i = 0; i < num; i++)
 			handles[i] = gem_create(fd, 4096);
 
-		if (intel_gen(intel_get_drm_devid(fd)) >= 6)
+		if (intel_gen(devid) >= 6)
 			ring = I915_EXEC_BLT;
 		else
 			ring = 0;
@@ -296,7 +353,8 @@ igt_main
 
 	reloc_tests();
 
-	source_offset_tests();
+	source_offset_tests(devid, false);
+	source_offset_tests(devid, true);
 
 	buffer_count_tests();
 
-- 
1.8.4

Re: [PATCH] tests/gem_reloc_overflow: Add gen8+ specifc tests

[Daniel Vetter]

On Wed, Nov 06, 2013 at 06:12:12PM +0000, rafael.barbalho@intel.com wrote:
> From: Rafael Barbalho <rafael.barbalho@intel.com>
> 
> Broadwell introduces 64-bit relocation addresses which add extra
> corner cases. The test was refactored slightly with some tests that
> were in the source offset tests were moved to the more generic reloc
> test area. The source offset tests are now gen aware and called twice to
> test both cpu & gtt relocation paths. In addition 2 new gen8+ test
> were added to the test:
> 
> * Relocation straddling page a page
> * Insufficient space for a relocation at the end of the buffer.
> 
> Signed-off-by: Rafael Barbalho <rafael.barbalho@intel.com>
> 
> Conflicts:
> 	tests/gem_reloc_overflow.c
> ---
>  tests/gem_reloc_overflow.c | 148 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 103 insertions(+), 45 deletions(-)
> 
> diff --git a/tests/gem_reloc_overflow.c b/tests/gem_reloc_overflow.c
> index f7ba1d7..38ad2a5 100644
> --- a/tests/gem_reloc_overflow.c
> +++ b/tests/gem_reloc_overflow.c
> @@ -24,6 +24,7 @@
>   * Authors:
>   *    Kees Cook <keescook@chromium.org>
>   *    Daniel Vetter <daniel.vetter@ffwll.ch>
> + *    Rafael Barbalho <rafael.barbalho@intel.com>
>   *
>   */
>  
> @@ -60,13 +61,14 @@ struct drm_i915_gem_relocation_entry *reloc;
>  uint32_t handle;
>  uint32_t batch_handle;
>  
> -
> -static void source_offset_tests(void)
> +static void source_offset_tests(int devid, bool reloc_gtt)
>  {
>  	struct drm_i915_gem_relocation_entry single_reloc;
> +	char *dst_gtt;
> +	char *relocation_type;
>  
>  	igt_fixture {
> -		handle = gem_create(fd, 4096);
> +		handle = gem_create(fd, 8192);
>  
>  		execobjs[1].handle = batch_handle;
>  		execobjs[1].relocation_count = 0;
> @@ -76,21 +78,70 @@ static void source_offset_tests(void)
>  		execobjs[0].relocation_count = 1;
>  		execobjs[0].relocs_ptr = (uintptr_t) &single_reloc;
>  		execbuf.buffer_count = 2;
> +
> +		if (reloc_gtt) {
> +			dst_gtt = gem_mmap(fd, handle, 8192, PROT_READ | PROT_WRITE);
> +			igt_assert(dst_gtt != MAP_FAILED);
> +			gem_set_domain(fd, handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +			memset(dst_gtt, 0, 8192);
> +			munmap(dst_gtt, 8192);
> +			relocation_type = "reloc-gtt";
> +		} else {
> +			relocation_type = "reloc-cpu";
> +		}
>  	}
>  
> -	igt_subtest("source-offset-end") {
> -		single_reloc.offset = 4096 - 4;
> -		single_reloc.delta = 0;
> -		single_reloc.target_handle = handle;
> -		single_reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> -		single_reloc.write_domain = I915_GEM_DOMAIN_RENDER;
> -		single_reloc.presumed_offset = 0;
> +	if (intel_gen(devid) >= 8) {
> +		igt_subtest_f("source-offset-page-stradle-gen8+-%s", relocation_type) {
> +			single_reloc.offset = 4096 - 4;
> +			single_reloc.delta = 0;
> +			single_reloc.target_handle = handle;
> +			single_reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> +			single_reloc.write_domain = I915_GEM_DOMAIN_RENDER;
> +			single_reloc.presumed_offset = 0;
> +
> +			igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) == 0);
> +			single_reloc.delta = 1024;
> +			igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) == 0);
> +		}
>  
> -		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) == 0);
> +		igt_subtest_f("source-offset-end-gen8+-%s", relocation_type) {
> +			single_reloc.offset = 8192 - 8;
> +			single_reloc.delta = 0;
> +			single_reloc.target_handle = handle;
> +			single_reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> +			single_reloc.write_domain = I915_GEM_DOMAIN_RENDER;
> +			single_reloc.presumed_offset = 0;
> +
> +			igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) == 0);
> +		}
> +
> +		igt_subtest_f("source-offset-overflow-gen8+-%s", relocation_type) {
> +			single_reloc.offset = 8192 - 4;
> +			single_reloc.delta = 0;
> +			single_reloc.target_handle = handle;
> +			single_reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> +			single_reloc.write_domain = I915_GEM_DOMAIN_RENDER;
> +			single_reloc.presumed_offset = 0;
> +
> +			igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) != 0);
> +			igt_assert(errno == EINVAL);
> +		}
> +	} else {
> +		igt_subtest_f("source-offset-end-%s", relocation_type) {
> +			single_reloc.offset = 8192 - 4;
> +			single_reloc.delta = 0;
> +			single_reloc.target_handle = handle;
> +			single_reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> +			single_reloc.write_domain = I915_GEM_DOMAIN_RENDER;
> +			single_reloc.presumed_offset = 0;
> +
> +			igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) == 0);
> +		}
>  	}
>  
> -	igt_subtest("source-offset-big") {
> -		single_reloc.offset = 4096;
> +	igt_subtest_f("source-offset-big-%s", relocation_type) {
> +		single_reloc.offset = 8192;
>  		single_reloc.delta = 0;
>  		single_reloc.target_handle = handle;
>  		single_reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> @@ -101,7 +152,7 @@ static void source_offset_tests(void)
>  		igt_assert(errno == EINVAL);
>  	}
>  
> -	igt_subtest("source-offset-negative") {
> +	igt_subtest_f("source-offset-negative-%s", relocation_type) {
>  		single_reloc.offset = (int64_t) -4;
>  		single_reloc.delta = 0;
>  		single_reloc.target_handle = handle;
> @@ -113,7 +164,7 @@ static void source_offset_tests(void)
>  		igt_assert(errno == EINVAL);
>  	}
>  
> -	igt_subtest("source-offset-unaligned") {
> +	igt_subtest_f("source-offset-unaligned-%s", relocation_type) {
>  		single_reloc.offset = 1;
>  		single_reloc.delta = 0;
>  		single_reloc.target_handle = handle;
> @@ -126,34 +177,6 @@ static void source_offset_tests(void)
>  	}
>  
>  	igt_fixture {
> -		execobjs[0].handle = batch_handle;
> -		execobjs[0].relocation_count = 0;
> -		execobjs[0].relocs_ptr = 0;
> -
> -		execbuf.buffer_count = 1;
> -	}
> -
> -	igt_subtest("batch-start-unaligend") {
> -		execbuf.batch_start_offset = 1;
> -		execbuf.batch_len = 8;
> -
> -		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) != 0);
> -		igt_assert(errno == EINVAL);
> -	}
> -
> -	igt_subtest("batch-end-unaligend") {
> -		execbuf.batch_start_offset = 0;
> -		execbuf.batch_len = 7;
> -
> -		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) != 0);
> -		igt_assert(errno == EINVAL);
> -	}
> -
> -	igt_fixture {
> -		/* Undo damage for next tests. */
> -		execbuf.batch_start_offset = 0;
> -		execbuf.batch_len = 8;
> -
>  		gem_close(fd, handle);
>  	}
>  }
> @@ -185,6 +208,36 @@ static void reloc_tests(void)
>  		igt_assert(errno == EINVAL);
>  	}
>  
> +	igt_fixture {
> +		execobjs[0].handle = batch_handle;
> +		execobjs[0].relocation_count = 0;
> +		execobjs[0].relocs_ptr = 0;
> +
> +		execbuf.buffer_count = 1;
> +	}
> +
> +	igt_subtest("batch-start-unaligned") {
> +		execbuf.batch_start_offset = 1;
> +		execbuf.batch_len = 8;
> +
> +		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) != 0);
> +		igt_assert(errno == EINVAL);
> +	}
> +
> +	igt_subtest("batch-end-unaligned") {
> +		execbuf.batch_start_offset = 0;
> +		execbuf.batch_len = 7;
> +
> +		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) != 0);
> +		igt_assert(errno == EINVAL);
> +	}
> +
> +	igt_fixture {
> +		/* Undo damage for next tests. */
> +		execbuf.batch_start_offset = 0;
> +		execbuf.batch_len = 8;
> +	}

Moving these tests around is tricky - they should be carefully constructed
so that the wrong batch start/end is the only thing which is wrong with
the metadata, and that with correct start/len (0, 8) it would execute
perfectly.

This is to make sure that we really exercise this cornercase and don't get
caught in some other check (that originally was done later but then might
have moved around).

tldr; Have you check that this is still true?
-Daniel
> +
>  	igt_subtest("wrapped-overflow") {
>  		/* Attempt wrapped overflow entries. */
>  		for (i = 0; i < num; i++) {
> @@ -252,12 +305,16 @@ static void buffer_count_tests(void)
>  
>  igt_main
>  {
> +	int devid = 0;
> +
>  	igt_fixture {
>  		int ring;
>  		uint32_t batch_data [2] = { MI_NOOP, MI_BATCH_BUFFER_END };
>  
>  		fd = drm_open_any();
>  
> +		devid = intel_get_drm_devid(fd);
> +
>  		/* Create giant reloc buffer area. */
>  		num = 257;
>  		entries = ((1ULL << 32) / (num - 1));
> @@ -271,7 +328,7 @@ igt_main
>  		for (int i = 0; i < num; i++)
>  			handles[i] = gem_create(fd, 4096);
>  
> -		if (intel_gen(intel_get_drm_devid(fd)) >= 6)
> +		if (intel_gen(devid) >= 6)
>  			ring = I915_EXEC_BLT;
>  		else
>  			ring = 0;
> @@ -296,7 +353,8 @@ igt_main
>  
>  	reloc_tests();
>  
> -	source_offset_tests();
> +	source_offset_tests(devid, false);
> +	source_offset_tests(devid, true);
>  
>  	buffer_count_tests();
>  
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] tests/gem_reloc_overflow: Add gen8+ specifc tests

[Barbalho, Rafael]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, November 06, 2013 6:52 PM
> To: Barbalho, Rafael
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] tests/gem_reloc_overflow: Add gen8+
> specifc tests
> 

* snip *

> > @@ -185,6 +208,36 @@ static void reloc_tests(void)
> >  		igt_assert(errno == EINVAL);
> >  	}
> >
> > +	igt_fixture {
> > +		execobjs[0].handle = batch_handle;
> > +		execobjs[0].relocation_count = 0;
> > +		execobjs[0].relocs_ptr = 0;
> > +
> > +		execbuf.buffer_count = 1;
> > +	}
> > +
> > +	igt_subtest("batch-start-unaligned") {
> > +		execbuf.batch_start_offset = 1;
> > +		execbuf.batch_len = 8;
> > +
> > +		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2,
> &execbuf) != 0);
> > +		igt_assert(errno == EINVAL);
> > +	}
> > +
> > +	igt_subtest("batch-end-unaligned") {
> > +		execbuf.batch_start_offset = 0;
> > +		execbuf.batch_len = 7;
> > +
> > +		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2,
> &execbuf) != 0);
> > +		igt_assert(errno == EINVAL);
> > +	}
> > +
> > +	igt_fixture {
> > +		/* Undo damage for next tests. */
> > +		execbuf.batch_start_offset = 0;
> > +		execbuf.batch_len = 8;
> > +	}
> 
> Moving these tests around is tricky - they should be carefully constructed so
> that the wrong batch start/end is the only thing which is wrong with the
> metadata, and that with correct start/len (0, 8) it would execute perfectly.
> 
> This is to make sure that we really exercise this cornercase and don't get
> caught in some other check (that originally was done later but then might
> have moved around).
> 
> tldr; Have you check that this is still true?

When I first moved the test I did try to make sure to move the igt_fixture 
that set up the state for the tests did the correct thing. Just to make sure that I
didn't screw things up I've re-run tests with an annotated kernel and it is 
behaving as expected with the driver returning false from 
i915_gem_check_execbuffer due to buffer misalignment.

tldr; Yes.

Thanks,
Raf

Re: [PATCH] tests/gem_reloc_overflow: Add gen8+ specifc tests

[Daniel Vetter]

On Wed, Nov 06, 2013 at 07:52:23PM +0000, Barbalho, Rafael wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Wednesday, November 06, 2013 6:52 PM
> > To: Barbalho, Rafael
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] tests/gem_reloc_overflow: Add gen8+
> > specifc tests
> > 
> 
> * snip *
> 
> > > @@ -185,6 +208,36 @@ static void reloc_tests(void)
> > >  		igt_assert(errno == EINVAL);
> > >  	}
> > >
> > > +	igt_fixture {
> > > +		execobjs[0].handle = batch_handle;
> > > +		execobjs[0].relocation_count = 0;
> > > +		execobjs[0].relocs_ptr = 0;
> > > +
> > > +		execbuf.buffer_count = 1;
> > > +	}
> > > +
> > > +	igt_subtest("batch-start-unaligned") {
> > > +		execbuf.batch_start_offset = 1;
> > > +		execbuf.batch_len = 8;
> > > +
> > > +		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2,
> > &execbuf) != 0);
> > > +		igt_assert(errno == EINVAL);
> > > +	}
> > > +
> > > +	igt_subtest("batch-end-unaligned") {
> > > +		execbuf.batch_start_offset = 0;
> > > +		execbuf.batch_len = 7;
> > > +
> > > +		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2,
> > &execbuf) != 0);
> > > +		igt_assert(errno == EINVAL);
> > > +	}
> > > +
> > > +	igt_fixture {
> > > +		/* Undo damage for next tests. */
> > > +		execbuf.batch_start_offset = 0;
> > > +		execbuf.batch_len = 8;
> > > +	}
> > 
> > Moving these tests around is tricky - they should be carefully constructed so
> > that the wrong batch start/end is the only thing which is wrong with the
> > metadata, and that with correct start/len (0, 8) it would execute perfectly.
> > 
> > This is to make sure that we really exercise this cornercase and don't get
> > caught in some other check (that originally was done later but then might
> > have moved around).
> > 
> > tldr; Have you check that this is still true?
> 
> When I first moved the test I did try to make sure to move the igt_fixture 
> that set up the state for the tests did the correct thing. Just to make sure that I
> didn't screw things up I've re-run tests with an annotated kernel and it is 
> behaving as expected with the driver returning false from 
> i915_gem_check_execbuffer due to buffer misalignment.
> 
> tldr; Yes.

Cool, thanks for testing. Since I'm paranoid about this I've added an
assert into the relevant fixture block to make sure the batch would really
work safe for the condition we're testing. That way we're maximally robust
against kernel changes that move the tests around.

Also fixed some compile warnings in a follow-up.

Thanks for your patch, applied to i-g-t.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] tests/gem_reloc_overflow: Add gen8+ specifc tests

[Daniel Vetter]

On Thu, Nov 07, 2013 at 02:05:55PM +0100, Daniel Vetter wrote:
> Cool, thanks for testing. Since I'm paranoid about this I've added an
> assert into the relevant fixture block to make sure the batch would really
> work safe for the condition we're testing. That way we're maximally robust
> against kernel changes that move the tests around.
> 
> Also fixed some compile warnings in a follow-up.
> 
> Thanks for your patch, applied to i-g-t.

Meh, I've been a bit too quick, two important fixups where needed:
- Make the subtest enumeration work (it's supposed to work without an
  intel gpu present, e.g. for central build/test servers).
- Always enumerate all subtests (again to facilitate testing across
  platforms).

Maybe we need some docs about this stuff, currently most of it is encoded
as runtime asserts in the test suite library.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch