[PATCH i-g-t 0/4] Move imported bo pread/pwrite tests

[PATCH i-g-t 0/4] Move imported bo pread/pwrite tests

[Daniele Ceraolo Spurio]

We already had coverage in prime_vgem.c, but since we're testing
pread/pwrite ioctl it makes more sense to have it in the corresponding
test binaries

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>

Daniele Ceraolo Spurio (4):
  lib/igt_vgem: extract vgem_create_and_import
  tests/gem_pread: test reads from a bo not backed by struct page
  test/gem_pwrite: test writes to a bo not backed by struct page
  tests/prime_vgem: drop basic-read/write subtests

 lib/igt_dummyload.c |  10 +---
 lib/igt_vgem.c      |  35 +++++++++++
 lib/igt_vgem.h      |   2 +
 tests/gem_pread.c   | 160 ++++++++++++++++++++++++++++++++++---------------
 tests/gem_pwrite.c  | 167 ++++++++++++++++++++++++++++++++++++++++------------
 tests/prime_vgem.c  | 104 +++-----------------------------
 6 files changed, 287 insertions(+), 191 deletions(-)

-- 
1.9.1

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

[PATCH i-g-t 1/4] lib/igt_vgem: extract vgem_create_and_import

[Daniele Ceraolo Spurio]

The same code to create and import a vgem object is used in a couple of
places and a couple more are coming up in the next patches so extract
the code into a common function

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 lib/igt_dummyload.c | 10 ++--------
 lib/igt_vgem.c      | 35 +++++++++++++++++++++++++++++++++
 lib/igt_vgem.h      |  2 ++
 tests/prime_vgem.c  | 56 +++++++++++++----------------------------------------
 4 files changed, 52 insertions(+), 51 deletions(-)

diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
index 913cc93..03541f7 100644
--- a/lib/igt_dummyload.c
+++ b/lib/igt_dummyload.c
@@ -317,23 +317,17 @@ igt_cork_t *igt_cork_new(int fd)
 {
 	igt_cork_t *cork;
 	struct vgem_bo bo;
-	int dmabuf;
 
 	cork = calloc(1, sizeof(igt_cork_t));
 	igt_assert(cork);
 
 	cork->device = drm_open_driver(DRIVER_VGEM);
 
-	igt_require(vgem_has_fences(cork->device));
-
 	bo.width = bo.height = 1;
 	bo.bpp = 4;
-	vgem_create(cork->device, &bo);
-	cork->fence = vgem_fence_attach(cork->device, &bo, VGEM_FENCE_WRITE);
 
-	dmabuf = prime_handle_to_fd(cork->device, bo.handle);
-	cork->handle = prime_fd_to_handle(fd, dmabuf);
-	close(dmabuf);
+	cork->handle = vgem_create_and_import(cork->device, &bo, fd,
+					      &cork->fence);
 
 	return cork;
 }
diff --git a/lib/igt_vgem.c b/lib/igt_vgem.c
index 7f933b2..7fc62f2 100644
--- a/lib/igt_vgem.c
+++ b/lib/igt_vgem.c
@@ -66,6 +66,41 @@ void vgem_create(int fd, struct vgem_bo *bo)
 	igt_assert_eq(__vgem_create(fd, bo), 0);
 }
 
+/**
+ * vgem_create_and_import:
+ * @vgem_fd: open vgem file descriptor
+ * @bo: vgem_bo struct containing width, height and bpp of the object to open
+ * @import_fd: open drm file descriptor to be used to import the vgem bo
+ * @fence: optional return variable to store a fence attached to the vgem bo
+ *
+ * This function creates a vgem bo and imports it to the provided device. If
+ * the fence parameter if provided a fence is attached to the bo and returned.
+ * The provided vgem_bo struct is updated as in vgem_create.
+ *
+ * Returns:
+ * Handle of the imported bo.
+ */
+uint32_t vgem_create_and_import(int vgem_fd, struct vgem_bo *bo, int import_fd,
+				uint32_t *fence)
+{
+	int dmabuf;
+	uint32_t handle;
+
+	vgem_create(vgem_fd, bo);
+
+	if (fence) {
+		igt_require(vgem_has_fences(vgem_fd));
+		*fence = vgem_fence_attach(vgem_fd, bo, VGEM_FENCE_WRITE);
+	}
+
+	dmabuf = prime_handle_to_fd(vgem_fd, bo->handle);
+	handle = prime_fd_to_handle(import_fd, dmabuf);
+	igt_assert(handle);
+	close(dmabuf);
+
+	return handle;
+}
+
 void *__vgem_mmap(int fd, struct vgem_bo *bo, unsigned prot)
 {
 	struct drm_mode_map_dumb arg;
diff --git a/lib/igt_vgem.h b/lib/igt_vgem.h
index 92045f0..94b1186 100644
--- a/lib/igt_vgem.h
+++ b/lib/igt_vgem.h
@@ -36,6 +36,8 @@ struct vgem_bo {
 
 int __vgem_create(int fd, struct vgem_bo *bo);
 void vgem_create(int fd, struct vgem_bo *bo);
+uint32_t vgem_create_and_import(int vgem_fd, struct vgem_bo *bo, int import_fd,
+				uint32_t *fence);
 
 void *__vgem_mmap(int fd, struct vgem_bo *bo, unsigned prot);
 void *vgem_mmap(int fd, struct vgem_bo *bo, unsigned prot);
diff --git a/tests/prime_vgem.c b/tests/prime_vgem.c
index 0ffaee9..489e9b6 100644
--- a/tests/prime_vgem.c
+++ b/tests/prime_vgem.c
@@ -35,16 +35,12 @@ static void test_read(int vgem, int i915)
 	struct vgem_bo scratch;
 	uint32_t handle;
 	uint32_t *ptr;
-	int dmabuf, i;
+	int i;
 
 	scratch.width = 1024;
 	scratch.height = 1024;
 	scratch.bpp = 32;
-	vgem_create(vgem, &scratch);
-
-	dmabuf = prime_handle_to_fd(vgem, scratch.handle);
-	handle = prime_fd_to_handle(i915, dmabuf);
-	close(dmabuf);
+	handle = vgem_create_and_import(vgem, &scratch, i915, NULL);
 
 	ptr = vgem_mmap(vgem, &scratch, PROT_WRITE);
 	for (i = 0; i < 1024; i++)
@@ -66,7 +62,7 @@ static void test_fence_read(int i915, int vgem)
 	uint32_t handle;
 	uint32_t *ptr;
 	uint32_t fence;
-	int dmabuf, i;
+	int i;
 	int master[2], slave[2];
 
 	igt_assert(pipe(master) == 0);
@@ -75,11 +71,7 @@ static void test_fence_read(int i915, int vgem)
 	scratch.width = 1024;
 	scratch.height = 1024;
 	scratch.bpp = 32;
-	vgem_create(vgem, &scratch);
-
-	dmabuf = prime_handle_to_fd(vgem, scratch.handle);
-	handle = prime_fd_to_handle(i915, dmabuf);
-	close(dmabuf);
+	handle = vgem_create_and_import(vgem, &scratch, i915, NULL);
 
 	igt_fork(child, 1) {
 		for (i = 0; i < 1024; i++) {
@@ -121,7 +113,7 @@ static void test_fence_mmap(int i915, int vgem)
 	uint32_t handle;
 	uint32_t *ptr;
 	uint32_t fence;
-	int dmabuf, i;
+	int i;
 	int master[2], slave[2];
 
 	igt_assert(pipe(master) == 0);
@@ -130,11 +122,7 @@ static void test_fence_mmap(int i915, int vgem)
 	scratch.width = 1024;
 	scratch.height = 1024;
 	scratch.bpp = 32;
-	vgem_create(vgem, &scratch);
-
-	dmabuf = prime_handle_to_fd(vgem, scratch.handle);
-	handle = prime_fd_to_handle(i915, dmabuf);
-	close(dmabuf);
+	handle = vgem_create_and_import(vgem, &scratch, i915, NULL);
 
 	igt_fork(child, 1) {
 		ptr = gem_mmap__gtt(i915, handle, 4096*1024, PROT_READ);
@@ -176,16 +164,12 @@ static void test_write(int vgem, int i915)
 	struct vgem_bo scratch;
 	uint32_t handle;
 	uint32_t *ptr;
-	int dmabuf, i;
+	int i;
 
 	scratch.width = 1024;
 	scratch.height = 1024;
 	scratch.bpp = 32;
-	vgem_create(vgem, &scratch);
-
-	dmabuf = prime_handle_to_fd(vgem, scratch.handle);
-	handle = prime_fd_to_handle(i915, dmabuf);
-	close(dmabuf);
+	handle = vgem_create_and_import(vgem, &scratch, i915, NULL);
 
 	ptr = vgem_mmap(vgem, &scratch, PROT_READ);
 	gem_close(vgem, scratch.handle);
@@ -204,16 +188,12 @@ static void test_gtt(int vgem, int i915)
 	struct vgem_bo scratch;
 	uint32_t handle;
 	uint32_t *ptr;
-	int dmabuf, i;
+	int i;
 
 	scratch.width = 1024;
 	scratch.height = 1024;
 	scratch.bpp = 32;
-	vgem_create(vgem, &scratch);
-
-	dmabuf = prime_handle_to_fd(vgem, scratch.handle);
-	handle = prime_fd_to_handle(i915, dmabuf);
-	close(dmabuf);
+	handle = vgem_create_and_import(vgem, &scratch, i915, NULL);
 
 	ptr = gem_mmap__gtt(i915, handle, scratch.size, PROT_WRITE);
 	for (i = 0; i < 1024; i++)
@@ -241,16 +221,12 @@ static void test_gtt_interleaved(int vgem, int i915)
 	struct vgem_bo scratch;
 	uint32_t handle;
 	uint32_t *ptr, *gtt;
-	int dmabuf, i;
+	int i;
 
 	scratch.width = 1024;
 	scratch.height = 1024;
 	scratch.bpp = 32;
-	vgem_create(vgem, &scratch);
-
-	dmabuf = prime_handle_to_fd(vgem, scratch.handle);
-	handle = prime_fd_to_handle(i915, dmabuf);
-	close(dmabuf);
+	handle = vgem_create_and_import(vgem, &scratch, i915, NULL);
 
 	/* This assumes that GTT is perfectedly coherent. On certain machines,
 	 * it is possible for a direct acces to bypass the GTT indirection.
@@ -709,17 +685,11 @@ static void test_flip(int i915, int vgem, unsigned hang)
 	signal(SIGHUP, sighandler);
 
 	for (int i = 0; i < 2; i++) {
-		int fd;
-
 		bo[i].width = 1024;
 		bo[i].height = 768;
 		bo[i].bpp = 32;
-		vgem_create(vgem, &bo[i]);
 
-		fd = prime_handle_to_fd(vgem, bo[i].handle);
-		handle[i] = prime_fd_to_handle(i915, fd);
-		igt_assert(handle[i]);
-		close(fd);
+		handle[i] = vgem_create_and_import(vgem, &bo[i], i915, NULL);
 
 		do_or_die(__kms_addfb(i915, handle[i],
 				      bo[i].width, bo[i].height, bo[i].pitch,
-- 
1.9.1

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

[PATCH i-g-t 2/4] tests/gem_pread: test reads from a bo not backed by struct page

[Daniele Ceraolo Spurio]

Using an imported vgem bo we can test reads from an object not backed
by struct page. These reads use different paths in the kernel.

While at it, extract some common code in an helper function and fix the
src vs dst naming (they are the other way around).

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

diff --git a/tests/gem_pread.c b/tests/gem_pread.c
index 39a46ed..89a9a5d 100644
--- a/tests/gem_pread.c
+++ b/tests/gem_pread.c
@@ -26,6 +26,7 @@
  */
 
 #include "igt.h"
+#include "igt_vgem.h"
 #include <unistd.h>
 #include <stdlib.h>
 #include <stdint.h>
@@ -73,25 +74,91 @@ static const char *bytes_per_sec(char *buf, double v)
 	return buf;
 }
 
+static void do_read_loop(int fd, const char *name, uint32_t handle,
+			 uint32_t *dst, int size)
+{
+	int count;
+	double usecs;
+	char buf[100];
+	const char* bps;
+	unsigned i;
+
+	for (count = 1; count <= 1<<17; count <<= 1) {
+		struct timeval start, end;
+
+		memset(dst, 0, size);
+		gettimeofday(&start, NULL);
+		do_gem_read(fd, handle, dst, size, count);
+		gettimeofday(&end, NULL);
+		usecs = elapsed(&start, &end, count);
+		bps = bytes_per_sec(buf, size/usecs*1e6);
+		igt_info("Time to %s pread %d bytes x %6d:	%7.3fµs, %s\n",
+			 name, size, count, usecs, bps);
+		fflush(stdout);
+
+		for (i = 0; i < size / 4096; i++)
+			igt_assert_eq(dst[i * 1024], i);
+	}
+}
+
+static uint32_t create_i915_bo(int fd, uint64_t size)
+{
+	uint32_t handle = gem_create(fd, size);
+	uint32_t *ptr;
+	unsigned i;
+
+	ptr = gem_mmap__cpu(fd, handle, 0, size, PROT_WRITE);
+	gem_set_domain(fd, handle, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
+	for (i = 0; i < size / 4096; i++)
+		ptr[i * 1024] = i;
+	munmap(ptr, size);
+	return handle;
+}
+
+/*
+ * imported BOs are not backed by struct page and therefore the preads go
+ * through different driver paths compared to a normal bo
+ */
+static uint32_t create_foreign_bo(int fd, uint64_t size)
+{
+	struct vgem_bo scratch;
+	int vgem;
+	uint32_t handle;
+	uint32_t *ptr;
+	unsigned i;
+
+	vgem = drm_open_driver(DRIVER_VGEM);
 
-uint32_t *src, dst;
-int fd, count;
+	scratch.width = 1024;
+	scratch.height = size / 4096;
+	scratch.bpp = 32;
+	handle = vgem_create_and_import(vgem, &scratch, fd, NULL);
+	igt_assert_eq(size, scratch.size);
+
+	ptr = vgem_mmap(vgem, &scratch, PROT_WRITE);
+	for (i = 0; i < size / 4096; i++)
+		ptr[i * 1024] = i;
+	munmap(ptr, scratch.size);
+
+	close(vgem);
+
+	return handle;
+}
 
 int main(int argc, char **argv)
 {
+	int fd;
+	uint32_t *dst;
+	uint32_t handle;
 	int object_size = 0;
-	double usecs;
-	char buf[100];
-	const char* bps;
 	const struct {
-		int level;
-		const char *name;
-	} cache[] = {
-		{ 0, "uncached" },
-		{ 1, "snoop" },
-		{ 2, "display" },
-		{ -1 },
-	}, *c;
+		const char *prefix;
+		uint32_t (*bo_create)(int fd, uint64_t size);
+	} modes[] = {
+		{ "", create_i915_bo },
+		{ "foreign-bo-", create_foreign_bo },
+		{ NULL, NULL }
+	}, *m;
 
 	igt_subtest_init(argc, argv);
 	igt_skip_on_simulation();
@@ -104,49 +171,44 @@ int main(int argc, char **argv)
 
 	igt_fixture {
 		fd = drm_open_driver(DRIVER_INTEL);
-
-		dst = gem_create(fd, object_size);
-		src = malloc(object_size);
-	}
-
-	igt_subtest("basic") {
-		for (count = 1; count <= 1<<17; count <<= 1) {
-			struct timeval start, end;
-
-			gettimeofday(&start, NULL);
-			do_gem_read(fd, dst, src, object_size, count);
-			gettimeofday(&end, NULL);
-			usecs = elapsed(&start, &end, count);
-			bps = bytes_per_sec(buf, object_size/usecs*1e6);
-			igt_info("Time to pread %d bytes x %6d:	%7.3fµs, %s\n",
-				 object_size, count, usecs, bps);
-			fflush(stdout);
-		}
+		dst = malloc(object_size);
+		igt_assert(dst);
 	}
 
-	for (c = cache; c->level != -1; c++) {
-		igt_subtest(c->name) {
-			gem_set_caching(fd, dst, c->level);
-
-			for (count = 1; count <= 1<<17; count <<= 1) {
-				struct timeval start, end;
-
-				gettimeofday(&start, NULL);
-				do_gem_read(fd, dst, src, object_size, count);
-				gettimeofday(&end, NULL);
-				usecs = elapsed(&start, &end, count);
-				bps = bytes_per_sec(buf, object_size/usecs*1e6);
-				igt_info("Time to %s pread %d bytes x %6d:	%7.3fµs, %s\n",
-					 c->name, object_size, count, usecs, bps);
-				fflush(stdout);
+	for (m = modes; m->bo_create != NULL; m++) {
+		igt_subtest_group {
+			const struct {
+				int level;
+				const char *name;
+			} cache[] = {
+				{ 0, "uncached" },
+				{ 1, "snoop" },
+				{ 2, "display" },
+				{ -1 },
+			}, *c;
+
+			igt_fixture
+				handle = m->bo_create(fd, object_size);
+
+			igt_subtest_f("%sbasic", m->prefix)
+				do_read_loop(fd, "basic", handle, dst,
+					     object_size);
+
+			for (c = cache; c->level != -1; c++) {
+				igt_subtest_f("%s%s", m->prefix, c->name) {
+					gem_set_caching(fd, handle, c->level);
+					do_read_loop(fd, c->name, handle, dst,
+						     object_size);
+				}
 			}
+
+			igt_fixture
+				gem_close(fd, handle);
 		}
 	}
 
 	igt_fixture {
-		free(src);
-		gem_close(fd, dst);
-
+		free(dst);
 		close(fd);
 	}
 
-- 
1.9.1

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

[PATCH i-g-t 3/4] test/gem_pwrite: test writes to a bo not backed by struct page

[Daniele Ceraolo Spurio]

Using an imported vgem bo we can test writes to an object not backed
by struct page. These reads use different paths in the kernel.

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

diff --git a/tests/gem_pwrite.c b/tests/gem_pwrite.c
index 1c3d23d..8e24c4b 100644
--- a/tests/gem_pwrite.c
+++ b/tests/gem_pwrite.c
@@ -26,6 +26,7 @@
  */
 
 #include "igt.h"
+#include "igt_vgem.h"
 #include <unistd.h>
 #include <stdlib.h>
 #include <stdint.h>
@@ -79,6 +80,93 @@ static const char *bytes_per_sec(char *buf, double v)
 	return buf;
 }
 
+static void do_write_loop(int fd, const char *name, uint32_t handle,
+			  uint32_t *src, int size, int count)
+{
+	double usecs;
+	char buf[100];
+	const char* bps;
+	struct timeval start, end;
+
+	gettimeofday(&start, NULL);
+	do_gem_write(fd, handle, src, size, count);
+	gettimeofday(&end, NULL);
+	usecs = elapsed(&start, &end, count);
+	bps = bytes_per_sec(buf, size/usecs*1e6);
+	igt_info("Time to %s pwrite %d bytes x %6d:\t%7.3fµs, %s\n",
+		 name, size, count, usecs, bps);
+	fflush(stdout);
+}
+
+static void test_i915_bo(int fd, const char *name, uint32_t handle,
+			 uint32_t *src, int size)
+{
+	unsigned i, count;
+
+	for (count = 1; count <= 1<<17; count <<= 1) {
+		uint32_t *ptr = gem_mmap__cpu(fd, handle, 0, size, PROT_WRITE);
+		gem_set_domain(fd, handle, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
+		memset(ptr, 0, size);
+		munmap(ptr, size);
+
+		do_write_loop(fd, name, handle, src, size, count);
+
+		ptr = gem_mmap__cpu(fd, handle, 0, size, PROT_READ);
+		gem_set_domain(fd, handle, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
+
+		for (i = 0; i < size / 4096; i++)
+			igt_assert_eq(ptr[i * 1024], i);
+	}
+}
+
+struct foreign_bo {
+	int vgem_fd;
+	struct vgem_bo bo;
+	uint32_t handle;
+};
+
+static struct foreign_bo create_foreign_bo(int fd, uint32_t size)
+{
+	struct foreign_bo f_bo;
+
+	f_bo.vgem_fd = drm_open_driver(DRIVER_VGEM);
+
+	f_bo.bo.width = 1024;
+	f_bo.bo.height = size / 4096;
+	f_bo.bo.bpp = 32;
+	f_bo.handle = vgem_create_and_import(f_bo.vgem_fd, &f_bo.bo, fd, NULL);
+	igt_assert_eq(size, f_bo.bo.size);
+
+	return f_bo;
+}
+
+static void destroy_foreign_bo(int fd, struct foreign_bo *f_bo)
+{
+	gem_close(fd, f_bo->handle);
+	gem_close(f_bo->vgem_fd, f_bo->bo.handle);
+	close(f_bo->vgem_fd);
+}
+
+static void test_foreign_bo(int fd, const char *name, struct foreign_bo *f_bo,
+			    uint32_t *src, int size)
+{
+	unsigned i, count;
+
+	for (count = 1; count <= 1<<17; count <<= 1) {
+		uint32_t *ptr = vgem_mmap(f_bo->vgem_fd, &f_bo->bo, PROT_WRITE);
+		memset(ptr, 0, size);
+		munmap(ptr, size);
+
+		do_write_loop(fd, name, f_bo->handle, src, size, count);
+
+		ptr = vgem_mmap(f_bo->vgem_fd, &f_bo->bo, PROT_READ);
+		gem_set_domain(fd, f_bo->handle, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
+
+		for (i = 0; i < size / 4096; i++)
+			igt_assert_eq(ptr[i * 1024], i);
+	}
+}
+
 #define FORWARD 0x1
 #define BACKWARD 0x2
 #define RANDOM 0x4
@@ -205,16 +293,11 @@ static void test_big_gtt(int fd, int scale, unsigned flags)
 	gem_close(fd, handle);
 }
 
-uint32_t *src, dst;
-int fd;
-
 int main(int argc, char **argv)
 {
+	int fd;
+	uint32_t *src;
 	int object_size = 0;
-	double usecs;
-	const char* bps;
-	char buf[100];
-	int count;
 	const struct {
 		int level;
 		const char *name;
@@ -236,50 +319,56 @@ int main(int argc, char **argv)
 	object_size = (object_size + 3) & -4;
 
 	igt_fixture {
+		unsigned i = 0;
 		fd = drm_open_driver(DRIVER_INTEL);
-
-		dst = gem_create(fd, object_size);
 		src = malloc(object_size);
+		igt_assert(src);
+
+		for (i = 0; i < object_size / 4096; i++)
+			((uint32_t *)src)[i * 1024] = i;
 	}
 
-	igt_subtest("basic") {
-		for (count = 1; count <= 1<<17; count <<= 1) {
-			struct timeval start, end;
-
-			gettimeofday(&start, NULL);
-			do_gem_write(fd, dst, src, object_size, count);
-			gettimeofday(&end, NULL);
-			usecs = elapsed(&start, &end, count);
-			bps = bytes_per_sec(buf, object_size/usecs*1e6);
-			igt_info("Time to pwrite %d bytes x %6d:	%7.3fµs, %s\n",
-				 object_size, count, usecs, bps);
-			fflush(stdout);
+	igt_subtest_group {
+		uint32_t handle;
+		igt_fixture
+			handle = gem_create(fd, object_size);
+
+		igt_subtest("basic")
+			test_i915_bo(fd, "basic", handle, src, object_size);
+
+		for (c = cache; c->level != -1; c++) {
+			igt_subtest(c->name) {
+				gem_set_caching(fd, handle, c->level);
+				test_i915_bo(fd, c->name, handle, src, object_size);
+			}
 		}
+
+		igt_fixture
+			gem_close(fd, handle);
 	}
 
-	for (c = cache; c->level != -1; c++) {
-		igt_subtest(c->name) {
-			gem_set_caching(fd, dst, c->level);
-
-			for (count = 1; count <= 1<<17; count <<= 1) {
-				struct timeval start, end;
-
-				gettimeofday(&start, NULL);
-				do_gem_write(fd, dst, src, object_size, count);
-				gettimeofday(&end, NULL);
-				usecs = elapsed(&start, &end, count);
-				bps = bytes_per_sec(buf, object_size/usecs*1e6);
-				igt_info("Time to %s pwrite %d bytes x %6d:	%7.3fµs, %s\n",
-					 c->name, object_size, count, usecs, bps);
-				fflush(stdout);
+	igt_subtest_group {
+		struct foreign_bo f_bo;
+
+		igt_fixture
+			f_bo = create_foreign_bo(fd, object_size);
+
+		igt_subtest("foreign-bo-basic")
+			test_foreign_bo(fd, "basic", &f_bo, src, object_size);
+
+		for (c = cache; c->level != -1; c++) {
+			igt_subtest_f("foreign-bo-%s", c->name) {
+				gem_set_caching(fd, f_bo.handle, c->level);
+				test_foreign_bo(fd, c->name, &f_bo, src, object_size);
 			}
 		}
+
+		igt_fixture
+			destroy_foreign_bo(fd, &f_bo);
 	}
 
-	igt_fixture {
+	igt_fixture
 		free(src);
-		gem_close(fd, dst);
-	}
 
 	{
 		const struct mode {
-- 
1.9.1

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

[PATCH i-g-t 4/4] tests/prime_vgem: drop basic-read/write subtests

[Daniele Ceraolo Spurio]

the same coverage is now in gem_pread and gem_pwrite

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

diff --git a/tests/prime_vgem.c b/tests/prime_vgem.c
index 489e9b6..5fb68fa 100644
--- a/tests/prime_vgem.c
+++ b/tests/prime_vgem.c
@@ -30,32 +30,6 @@
 
 IGT_TEST_DESCRIPTION("Basic check of polling for prime/vgem fences.");
 
-static void test_read(int vgem, int i915)
-{
-	struct vgem_bo scratch;
-	uint32_t handle;
-	uint32_t *ptr;
-	int i;
-
-	scratch.width = 1024;
-	scratch.height = 1024;
-	scratch.bpp = 32;
-	handle = vgem_create_and_import(vgem, &scratch, i915, NULL);
-
-	ptr = vgem_mmap(vgem, &scratch, PROT_WRITE);
-	for (i = 0; i < 1024; i++)
-		ptr[1024*i] = i;
-	munmap(ptr, scratch.size);
-	gem_close(vgem, scratch.handle);
-
-	for (i = 0; i < 1024; i++) {
-		uint32_t tmp;
-		gem_read(i915, handle, 4096*i, &tmp, sizeof(tmp));
-		igt_assert_eq(tmp, i);
-	}
-	gem_close(i915, handle);
-}
-
 static void test_fence_read(int i915, int vgem)
 {
 	struct vgem_bo scratch;
@@ -159,30 +133,6 @@ static void test_fence_mmap(int i915, int vgem)
 	close(slave[1]);
 }
 
-static void test_write(int vgem, int i915)
-{
-	struct vgem_bo scratch;
-	uint32_t handle;
-	uint32_t *ptr;
-	int i;
-
-	scratch.width = 1024;
-	scratch.height = 1024;
-	scratch.bpp = 32;
-	handle = vgem_create_and_import(vgem, &scratch, i915, NULL);
-
-	ptr = vgem_mmap(vgem, &scratch, PROT_READ);
-	gem_close(vgem, scratch.handle);
-
-	for (i = 0; i < 1024; i++)
-		gem_write(i915, handle, 4096*i, &i, sizeof(i));
-	gem_close(i915, handle);
-
-	for (i = 0; i < 1024; i++)
-		igt_assert_eq(ptr[1024*i], i);
-	munmap(ptr, scratch.size);
-}
-
 static void test_gtt(int vgem, int i915)
 {
 	struct vgem_bo scratch;
@@ -742,12 +692,6 @@ igt_main
 		gem_require_mmap_wc(i915);
 	}
 
-	igt_subtest("basic-read")
-		test_read(vgem, i915);
-
-	igt_subtest("basic-write")
-		test_write(vgem, i915);
-
 	igt_subtest("basic-gtt")
 		test_gtt(vgem, i915);
 
-- 
1.9.1

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

Re: [PATCH i-g-t 0/4] Move imported bo pread/pwrite tests

[Daniele Ceraolo Spurio]

On 12/10/17 15:30, Daniele Ceraolo Spurio wrote:
> We already had coverage in prime_vgem.c, but since we're testing
> pread/pwrite ioctl it makes more sense to have it in the corresponding
> test binaries
> 

Forgot to mention that this series depends on both the other series I've 
sent

Daniele

> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Daniele Ceraolo Spurio (4):
>    lib/igt_vgem: extract vgem_create_and_import
>    tests/gem_pread: test reads from a bo not backed by struct page
>    test/gem_pwrite: test writes to a bo not backed by struct page
>    tests/prime_vgem: drop basic-read/write subtests
> 
>   lib/igt_dummyload.c |  10 +---
>   lib/igt_vgem.c      |  35 +++++++++++
>   lib/igt_vgem.h      |   2 +
>   tests/gem_pread.c   | 160 ++++++++++++++++++++++++++++++++++---------------
>   tests/gem_pwrite.c  | 167 ++++++++++++++++++++++++++++++++++++++++------------
>   tests/prime_vgem.c  | 104 +++-----------------------------
>   6 files changed, 287 insertions(+), 191 deletions(-)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 0/4] Move imported bo pread/pwrite tests

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2017-10-12 23:30:40)
> We already had coverage in prime_vgem.c, but since we're testing
> pread/pwrite ioctl it makes more sense to have it in the corresponding
> test binaries

Redundancy is not an issue (sometimes it is a virtue, the slight
variations in test setup lead to wider coverage and serendipity - when we
don't have a fuzzer, we have to rely on random variations between
authors). The purpose of each test is different, one is testing prime and
interactions with i915 on/from a foreign bo; the other looking at
pwrite/pread in depth. Each subtest is meaningful as part of the
encompassing test.

The counter proposal is to be able to select a cross-selection of
subtests for a given set of tags/functions/code. And we still need a
fuzzer, model based or not.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Move imported bo pread/pwrite tests

[Patchwork]

== Series Details ==

Series: Move imported bo pread/pwrite tests
URL   : https://patchwork.freedesktop.org/series/31860/
State : failure

== Summary ==

Series 31860 revision 1 was fully merged or fully failed: no git log

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

Re: [PATCH i-g-t 0/4] Move imported bo pread/pwrite tests

[Daniele Ceraolo Spurio]

On 12/10/17 15:39, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2017-10-12 23:30:40)
>> We already had coverage in prime_vgem.c, but since we're testing
>> pread/pwrite ioctl it makes more sense to have it in the corresponding
>> test binaries
> 
> Redundancy is not an issue (sometimes it is a virtue, the slight
> variations in test setup lead to wider coverage and serendipity - when we
> don't have a fuzzer, we have to rely on random variations between
> authors). The purpose of each test is different, one is testing prime and
> interactions with i915 on/from a foreign bo; the other looking at
> pwrite/pread in depth. Each subtest is meaningful as part of the
> encompassing test.
> 
> The counter proposal is to be able to select a cross-selection of
> subtests for a given set of tags/functions/code. And we still need a
> fuzzer, model based or not.
> -Chris
> 

Would it be ok to just drop patch 4/4 and rephrase the commit messages? 
There is some extra redundancy compared to before as I've added 
read/write checks to pread/pwrite but that shouldn't hurt IMHO and I'd 
prefer to keep it.

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

Re: [PATCH i-g-t 1/4] lib/igt_vgem: extract vgem_create_and_import

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2017-10-12 23:30:41)
> The same code to create and import a vgem object is used in a couple of
> places and a couple more are coming up in the next patches so extract
> the code into a common function
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  lib/igt_dummyload.c | 10 ++--------
>  lib/igt_vgem.c      | 35 +++++++++++++++++++++++++++++++++
>  lib/igt_vgem.h      |  2 ++
>  tests/prime_vgem.c  | 56 +++++++++++++----------------------------------------
>  4 files changed, 52 insertions(+), 51 deletions(-)
> 
> diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
> index 913cc93..03541f7 100644
> --- a/lib/igt_dummyload.c
> +++ b/lib/igt_dummyload.c
> @@ -317,23 +317,17 @@ igt_cork_t *igt_cork_new(int fd)
>  {
>         igt_cork_t *cork;
>         struct vgem_bo bo;
> -       int dmabuf;
>  
>         cork = calloc(1, sizeof(igt_cork_t));
>         igt_assert(cork);
>  
>         cork->device = drm_open_driver(DRIVER_VGEM);
>  
> -       igt_require(vgem_has_fences(cork->device));
> -
>         bo.width = bo.height = 1;
>         bo.bpp = 4;
> -       vgem_create(cork->device, &bo);
> -       cork->fence = vgem_fence_attach(cork->device, &bo, VGEM_FENCE_WRITE);
>  
> -       dmabuf = prime_handle_to_fd(cork->device, bo.handle);
> -       cork->handle = prime_fd_to_handle(fd, dmabuf);
> -       close(dmabuf);
> +       cork->handle = vgem_create_and_import(cork->device, &bo, fd,
> +                                             &cork->fence);
>  
>         return cork;
>  }
> diff --git a/lib/igt_vgem.c b/lib/igt_vgem.c
> index 7f933b2..7fc62f2 100644
> --- a/lib/igt_vgem.c
> +++ b/lib/igt_vgem.c
> @@ -66,6 +66,41 @@ void vgem_create(int fd, struct vgem_bo *bo)
>         igt_assert_eq(__vgem_create(fd, bo), 0);
>  }
>  
> +/**
> + * vgem_create_and_import:
> + * @vgem_fd: open vgem file descriptor
> + * @bo: vgem_bo struct containing width, height and bpp of the object to open
> + * @import_fd: open drm file descriptor to be used to import the vgem bo
> + * @fence: optional return variable to store a fence attached to the vgem bo
> + *
> + * This function creates a vgem bo and imports it to the provided device. If
> + * the fence parameter if provided a fence is attached to the bo and returned.
> + * The provided vgem_bo struct is updated as in vgem_create.
> + *
> + * Returns:
> + * Handle of the imported bo.
> + */
> +uint32_t vgem_create_and_import(int vgem_fd, struct vgem_bo *bo, int import_fd,
> +                               uint32_t *fence)
> +{
> +       int dmabuf;
> +       uint32_t handle;
> +
> +       vgem_create(vgem_fd, bo);
> +
> +       if (fence) {
> +               igt_require(vgem_has_fences(vgem_fd));
> +               *fence = vgem_fence_attach(vgem_fd, bo, VGEM_FENCE_WRITE);
> +       }

Leave this to the caller. Don't have overly specific functions, try to
provide tools for writing future tests.

> +       dmabuf = prime_handle_to_fd(vgem_fd, bo->handle);
> +       handle = prime_fd_to_handle(import_fd, dmabuf);
> +       igt_assert(handle);

Not your concern. Add it to prime_fd_to_handle() if you want.

> +       close(dmabuf);
> +

This is common enough to be refactored,
	import_handle = prime_link(export_fd, export_handle, import_fd);
?

> +       return handle;
> +}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/4] tests/gem_pread: test reads from a bo not backed by struct page

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2017-10-12 23:30:42)
> Using an imported vgem bo we can test reads from an object not backed
> by struct page. These reads use different paths in the kernel.
> 
> While at it, extract some common code in an helper function and fix the
> src vs dst naming (they are the other way around).
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/gem_pread.c | 160 +++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 111 insertions(+), 49 deletions(-)
> 
> diff --git a/tests/gem_pread.c b/tests/gem_pread.c
> index 39a46ed..89a9a5d 100644
> --- a/tests/gem_pread.c
> +++ b/tests/gem_pread.c
> @@ -26,6 +26,7 @@
>   */
>  
>  #include "igt.h"
> +#include "igt_vgem.h"
>  #include <unistd.h>
>  #include <stdlib.h>
>  #include <stdint.h>
> @@ -73,25 +74,91 @@ static const char *bytes_per_sec(char *buf, double v)
>         return buf;
>  }
>  
> +static void do_read_loop(int fd, const char *name, uint32_t handle,
> +                        uint32_t *dst, int size)
> +{
> +       int count;
> +       double usecs;
> +       char buf[100];
> +       const char* bps;
> +       unsigned i;
> +
> +       for (count = 1; count <= 1<<17; count <<= 1) {
> +               struct timeval start, end;
> +
> +               memset(dst, 0, size);
> +               gettimeofday(&start, NULL);
> +               do_gem_read(fd, handle, dst, size, count);
> +               gettimeofday(&end, NULL);
> +               usecs = elapsed(&start, &end, count);
> +               bps = bytes_per_sec(buf, size/usecs*1e6);
> +               igt_info("Time to %s pread %d bytes x %6d:      %7.3fµs, %s\n",
> +                        name, size, count, usecs, bps);
> +               fflush(stdout);
> +
> +               for (i = 0; i < size / 4096; i++)
> +                       igt_assert_eq(dst[i * 1024], i);
> +       }
> +}

Which of course shouldn't exist in a plain test. We have benchmarks/ to
cover this. (Linking each benchmark to its conformance test would be
useful.)

> +
> +static uint32_t create_i915_bo(int fd, uint64_t size)
> +{
> +       uint32_t handle = gem_create(fd, size);
> +       uint32_t *ptr;
> +       unsigned i;

igt_skip_on(overflows_type(size, size_t));

We have an interesting challenge here with u64 and 32b intptr_t.

> +
> +       ptr = gem_mmap__cpu(fd, handle, 0, size, PROT_WRITE);
> +       gem_set_domain(fd, handle, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> +       for (i = 0; i < size / 4096; i++)

for (size_t i = 0; i < size/4096; i++)

> +               ptr[i * 1024] = i;
> +       munmap(ptr, size);
> +       return handle;
> +}
> +
> +/*
> + * imported BOs are not backed by struct page and therefore the preads go
> + * through different driver paths compared to a normal bo
> + */
> +static uint32_t create_foreign_bo(int fd, uint64_t size)
> +{
> +       struct vgem_bo scratch;
> +       int vgem;
> +       uint32_t handle;
> +       uint32_t *ptr;
> +       unsigned i;
> +
> +       vgem = drm_open_driver(DRIVER_VGEM);
>  
> -uint32_t *src, dst;
> -int fd, count;
> +       scratch.width = 1024;
> +       scratch.height = size / 4096;
> +       scratch.bpp = 32;
> +       handle = vgem_create_and_import(vgem, &scratch, fd, NULL);
> +       igt_assert_eq(size, scratch.size);
> +
> +       ptr = vgem_mmap(vgem, &scratch, PROT_WRITE);
> +       for (i = 0; i < size / 4096; i++)
> +               ptr[i * 1024] = i;
> +       munmap(ptr, scratch.size);
> +
> +       close(vgem);
> +
> +       return handle;
> +}
>  
>  int main(int argc, char **argv)
>  {
> +       int fd;
> +       uint32_t *dst;
> +       uint32_t handle;
>         int object_size = 0;
> -       double usecs;
> -       char buf[100];
> -       const char* bps;
>         const struct {
> -               int level;
> -               const char *name;
> -       } cache[] = {
> -               { 0, "uncached" },
> -               { 1, "snoop" },
> -               { 2, "display" },
> -               { -1 },
> -       }, *c;
> +               const char *prefix;
> +               uint32_t (*bo_create)(int fd, uint64_t size);
> +       } modes[] = {
> +               { "", create_i915_bo },
> +               { "foreign-bo-", create_foreign_bo },
> +               { NULL, NULL }
> +       }, *m;
>  
>         igt_subtest_init(argc, argv);
>         igt_skip_on_simulation();
> @@ -104,49 +171,44 @@ int main(int argc, char **argv)
>  
>         igt_fixture {
>                 fd = drm_open_driver(DRIVER_INTEL);
> -
> -               dst = gem_create(fd, object_size);
> -               src = malloc(object_size);
> -       }
> -
> -       igt_subtest("basic") {
> -               for (count = 1; count <= 1<<17; count <<= 1) {
> -                       struct timeval start, end;
> -
> -                       gettimeofday(&start, NULL);
> -                       do_gem_read(fd, dst, src, object_size, count);
> -                       gettimeofday(&end, NULL);
> -                       usecs = elapsed(&start, &end, count);
> -                       bps = bytes_per_sec(buf, object_size/usecs*1e6);
> -                       igt_info("Time to pread %d bytes x %6d: %7.3fµs, %s\n",
> -                                object_size, count, usecs, bps);
> -                       fflush(stdout);
> -               }
> +               dst = malloc(object_size);
> +               igt_assert(dst);
>         }
>  
> -       for (c = cache; c->level != -1; c++) {
> -               igt_subtest(c->name) {
> -                       gem_set_caching(fd, dst, c->level);
> -
> -                       for (count = 1; count <= 1<<17; count <<= 1) {
> -                               struct timeval start, end;
> -
> -                               gettimeofday(&start, NULL);
> -                               do_gem_read(fd, dst, src, object_size, count);
> -                               gettimeofday(&end, NULL);
> -                               usecs = elapsed(&start, &end, count);
> -                               bps = bytes_per_sec(buf, object_size/usecs*1e6);
> -                               igt_info("Time to %s pread %d bytes x %6d:      %7.3fµs, %s\n",
> -                                        c->name, object_size, count, usecs, bps);
> -                               fflush(stdout);
> +       for (m = modes; m->bo_create != NULL; m++) {
> +               igt_subtest_group {
> +                       const struct {
> +                               int level;
> +                               const char *name;
> +                       } cache[] = {
> +                               { 0, "uncached" },
> +                               { 1, "snoop" },
> +                               { 2, "display" },
> +                               { -1 },
> +                       }, *c;
> +
> +                       igt_fixture
> +                               handle = m->bo_create(fd, object_size);
> +
> +                       igt_subtest_f("%sbasic", m->prefix)
> +                               do_read_loop(fd, "basic", handle, dst,
> +                                            object_size);
> +
> +                       for (c = cache; c->level != -1; c++) {
> +                               igt_subtest_f("%s%s", m->prefix, c->name) {
> +                                       gem_set_caching(fd, handle, c->level);
> +                                       do_read_loop(fd, c->name, handle, dst,
> +                                                    object_size);
> +                               }
>                         }

Oh dear. We need stateless tests as well, i.e. since the handle is
common the driver execution paths will vary depending upon the sequence
of subtests; and in particular running this by hand will give a
different result to CI running each subtest individually.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx