[i-g-t PATCH v1 00/14] Get a few more tests to run on !i915

[i-g-t PATCH v1 00/14] Get a few more tests to run on !i915

[Tomeu Vizoso]

Hi,

have restarted work on getting tests in IGT to run on drivers other than
i915.

These changes make the modified tests pass in a Radxa Rock2 board by
using dumb buffers as much as possible and having subtests skip if they
require tiled BOs. The plan is for igt_create_bo_with_dimensions to be
able to hide differences in the buffer creation API as much as possible.

Thanks,

Tomeu


Tomeu Vizoso (14):
  lib: add igt_require_intel
  lib: Have gem_set_tiling require intel
  lib: Expose is_i915_device
  lib: Have intel_get_drm_devid call igt_require_intel
  lib: Call intel_get_drm_devid only from intel code
  lib: Add wrapper for DRM_IOCTL_MODE_CREATE_DUMB
  lib: Map dumb buffers
  lib: Add igt_create_bo_with_dimensions
  tests: Open any driver
  kms_addfb_basic: call igt_create_bo_with_dimensions
  kms_addfb_basic: move tiling functionality into each subtest
  kms_addfb_basic: Split tiling_tests off
  kms_addfb_basic: Move calls to gem_set_tiling to the subtests
  kms_addfb_basic: Get intel gen from within subtest

 lib/drmtest.c           |   7 ++-
 lib/drmtest.h           |   4 ++
 lib/igt_fb.c            | 116 +++++++++++++++++++++++++++++++++-------------
 lib/igt_fb.h            |   6 +++
 lib/igt_kms.c           |   2 +-
 lib/intel_chipset.c     |   2 +
 lib/ioctl_wrappers.c    |  38 +++++++++++++++
 lib/ioctl_wrappers.h    |   1 +
 tests/drm_read.c        |  18 +------
 tests/gem_exec_blt.c    |  18 +------
 tests/kms_addfb_basic.c | 121 +++++++++++++++++++++++++++++++++++-------------
 tests/kms_atomic.c      |   2 +-
 tests/kms_setmode.c     |   2 +-
 tests/kms_vblank.c      |   2 +-
 14 files changed, 235 insertions(+), 104 deletions(-)

-- 
2.5.0

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

[i-g-t PATCH v1 01/14] lib: add igt_require_intel

[Tomeu Vizoso]

Add function that requires that the driver we are talking to is i915.

This allows us to skip subtests that are specific to that driver.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 lib/drmtest.c | 5 +++++
 lib/drmtest.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index 7b2227fe0f6a..bb9ca507a922 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -399,3 +399,8 @@ int drm_open_driver_render(int chipset)
 
 	return fd;
 }
+
+void igt_require_intel(int fd)
+{
+	igt_require(is_i915_device(fd) && is_intel(fd));
+}
diff --git a/lib/drmtest.h b/lib/drmtest.h
index 9fcab9316bc4..af7da37d5ff8 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -82,6 +82,8 @@ int __drm_open_driver(int chipset);
 
 void gem_quiescent_gpu(int fd);
 
+void igt_require_intel(int fd);
+
 /**
  * do_or_die:
  * @x: command
-- 
2.5.0

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

[i-g-t PATCH v1 02/14] lib: Have gem_set_tiling require intel

[Tomeu Vizoso]

Before calling a i915-specific IOCTL, require i915.

This allows us to skip subtests that are specific to that driver, though
what should eventually happen is that tests don't generally call
gem_set_tiling directly but go through an abstraction layer that
constructs the buffer object in a driver-specific way.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 lib/ioctl_wrappers.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 5d4972931506..0221b7fef3a1 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -138,6 +138,8 @@ int __gem_set_tiling(int fd, uint32_t handle, uint32_t tiling, uint32_t stride)
 	struct drm_i915_gem_set_tiling st;
 	int ret;
 
+	igt_require_intel(fd);
+
 	memset(&st, 0, sizeof(st));
 	do {
 		st.handle = handle;
-- 
2.5.0

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

[i-g-t PATCH v1 03/14] lib: Expose is_i915_device

[Tomeu Vizoso]

Lib and test code can use this function to avoid i915-specific behavior
when running on other drivers.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 lib/drmtest.c | 2 +-
 lib/drmtest.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index bb9ca507a922..2ed84a01083a 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -90,7 +90,7 @@ static int __get_drm_device_name(int fd, char *name)
 	return -1;
 }
 
-static bool is_i915_device(int fd)
+bool is_i915_device(int fd)
 {
 	int ret;
 	char name[5] = "";
diff --git a/lib/drmtest.h b/lib/drmtest.h
index af7da37d5ff8..1d73115fb6bd 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -84,6 +84,8 @@ void gem_quiescent_gpu(int fd);
 
 void igt_require_intel(int fd);
 
+bool is_i915_device(int fd);
+
 /**
  * do_or_die:
  * @x: command
-- 
2.5.0

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

[i-g-t PATCH v1 04/14] lib: Have intel_get_drm_devid call igt_require_intel

[Tomeu Vizoso]

I915_PARAM_CHIPSET_ID is a i915-only thing, so if a subtest ends up
calling it when testing another driver, let's skip it.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 lib/intel_chipset.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/intel_chipset.c b/lib/intel_chipset.c
index 0e5b016cdb3d..9056cbf57811 100644
--- a/lib/intel_chipset.c
+++ b/lib/intel_chipset.c
@@ -38,6 +38,7 @@
 #include <sys/mman.h>
 #include "i915_drm.h"
 
+#include "drmtest.h"
 #include "intel_chipset.h"
 #include "igt_core.h"
 
@@ -129,6 +130,7 @@ intel_get_drm_devid(int fd)
 {
 	const char *override;
 
+	igt_require_intel(fd);
 	igt_assert(__drm_device_id);
 
 	override = getenv("INTEL_DEVID_OVERRIDE");
-- 
2.5.0

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

[i-g-t PATCH v1 05/14] lib: Call intel_get_drm_devid only from intel code

[Tomeu Vizoso]

It only makes sense when testing the i915 driver, so don't call it
otherwise.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 lib/igt_fb.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 5f23136e01ac..3e76a419b3ee 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -76,15 +76,14 @@ static struct format_desc_struct {
 static void igt_get_fb_tile_size(int fd, uint64_t tiling, int fb_bpp,
 				 unsigned *width_ret, unsigned *height_ret)
 {
-	uint32_t devid = intel_get_drm_devid(fd);
-
 	switch (tiling) {
 	case LOCAL_DRM_FORMAT_MOD_NONE:
 		*width_ret = 64;
 		*height_ret = 1;
 		break;
 	case LOCAL_I915_FORMAT_MOD_X_TILED:
-		if (intel_gen(devid) == 2) {
+		if (is_i915_device(fd) &&
+		    intel_gen(intel_get_drm_devid(fd)) == 2) {
 			*width_ret = 128;
 			*height_ret = 16;
 		} else {
@@ -93,7 +92,8 @@ static void igt_get_fb_tile_size(int fd, uint64_t tiling, int fb_bpp,
 		}
 		break;
 	case LOCAL_I915_FORMAT_MOD_Y_TILED:
-		if (IS_915(devid))
+		if (is_i915_device(fd) &&
+		    IS_915(intel_get_drm_devid(fd)))
 			*width_ret = 512;
 		else
 			*width_ret = 128;
@@ -145,8 +145,9 @@ void igt_calc_fb_size(int fd, int width, int height, int bpp, uint64_t tiling,
 
 	igt_get_fb_tile_size(fd, tiling, bpp, &tile_width, &tile_height);
 
-	if (intel_gen(intel_get_drm_devid(fd)) <= 3 &&
-	    tiling != LOCAL_DRM_FORMAT_MOD_NONE) {
+	if (is_i915_device(fd) &&
+	    tiling != LOCAL_DRM_FORMAT_MOD_NONE &&
+	    intel_gen(intel_get_drm_devid(fd)) <= 3) {
 		int v;
 
 		/* Round the tiling up to the next power-of-two and the region
-- 
2.5.0

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

[i-g-t PATCH v1 06/14] lib: Add wrapper for DRM_IOCTL_MODE_CREATE_DUMB

[Tomeu Vizoso]

In order to test drivers that don't have support for proper buffer
objects, add a wrapper for creating dumb buffer objects that will be
called from the lib code for those subtests that don't need to care.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 lib/ioctl_wrappers.c | 36 ++++++++++++++++++++++++++++++++++++
 lib/ioctl_wrappers.h |  1 +
 tests/drm_read.c     | 16 +---------------
 tests/gem_exec_blt.c | 18 +-----------------
 4 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 0221b7fef3a1..d842d860ba65 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -531,6 +531,42 @@ uint32_t gem_create(int fd, uint64_t size)
 }
 
 /**
+ * dumb_create:
+ * @fd: open drm file descriptor
+ * @width: width of the buffer in pixels
+ * @height: height of the buffer in pixels
+ * @bpp: bytes per pixel of the buffer
+ *
+ * This wraps the CREATE_DUMB ioctl, which allocates a new dumb buffer object
+ * for the specified dimensions.
+ *
+ * Returns: The file-private handle of the created buffer object
+ */
+uint32_t dumb_create(int fd, int width, int height, int bpp, unsigned *stride,
+		     unsigned *size)
+{
+	struct drm_mode_create_dumb create;
+
+	memset(&create, 0, sizeof(create));
+	create.width = width;
+	create.height = height;
+	create.bpp = bpp;
+
+	create.handle = 0;
+	do_ioctl(fd, DRM_IOCTL_MODE_CREATE_DUMB, &create);
+	igt_assert(create.handle);
+	igt_assert(create.size >= width * height * bpp / 8);
+
+	if (stride)
+		*stride = create.pitch;
+
+	if (size)
+		*size = create.size;
+
+	return create.handle;
+}
+
+/**
  * __gem_execbuf:
  * @fd: open i915 drm file descriptor
  * @execbuf: execbuffer data structure
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index f59eafba4bdd..9282ffdd8520 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -62,6 +62,7 @@ uint32_t __gem_create_stolen(int fd, uint64_t size);
 uint32_t gem_create_stolen(int fd, uint64_t size);
 uint32_t __gem_create(int fd, int size);
 uint32_t gem_create(int fd, uint64_t size);
+uint32_t dumb_create(int fd, int width, int height, int bpp, unsigned *stride, unsigned *size);
 void gem_execbuf(int fd, struct drm_i915_gem_execbuffer2 *execbuf);
 int __gem_execbuf(int fd, struct drm_i915_gem_execbuffer2 *execbuf);
 
diff --git a/tests/drm_read.c b/tests/drm_read.c
index faa3df862ea6..a27e5522daa0 100644
--- a/tests/drm_read.c
+++ b/tests/drm_read.c
@@ -120,20 +120,6 @@ static void test_invalid_buffer(int in)
 	teardown(fd);
 }
 
-static uint32_t dumb_create(int fd)
-{
-	struct drm_mode_create_dumb arg;
-
-	arg.bpp = 32;
-	arg.width = 32;
-	arg.height = 32;
-
-	do_ioctl(fd, DRM_IOCTL_MODE_CREATE_DUMB, &arg);
-	igt_assert(arg.size >= 4096);
-
-	return arg.handle;
-}
-
 static void test_fault_buffer(int in)
 {
 	int fd = setup(in, 0);
@@ -141,7 +127,7 @@ static void test_fault_buffer(int in)
 	char *buf;
 
 	memset(&arg, 0, sizeof(arg));
-	arg.handle = dumb_create(fd);
+	arg.handle = dumb_create(fd, 32, 32, 32, NULL, NULL);
 
 	do_ioctl(fd, DRM_IOCTL_MODE_MAP_DUMB, &arg);
 
diff --git a/tests/gem_exec_blt.c b/tests/gem_exec_blt.c
index 74f5c2ba87ad..42a030fb4f57 100644
--- a/tests/gem_exec_blt.c
+++ b/tests/gem_exec_blt.c
@@ -170,22 +170,6 @@ static const char *bytes_per_sec(char *buf, double v)
 	return buf;
 }
 
-static uint32_t dumb_create(int fd)
-{
-	struct drm_mode_create_dumb arg;
-	int ret;
-
-	arg.bpp = 32;
-	arg.width = 32;
-	arg.height = 32;
-
-	ret = drmIoctl(fd, DRM_IOCTL_MODE_CREATE_DUMB, &arg);
-	igt_assert_eq(ret, 0);
-	igt_assert(arg.size >= 4096);
-
-	return arg.handle;
-}
-
 static int dcmp(const void *A, const void *B)
 {
 	const double *a = A, *b = B;
@@ -209,7 +193,7 @@ static void run(int object_size, bool dumb)
 
 	fd = drm_open_driver(DRIVER_INTEL);
 	if (dumb)
-		handle = dumb_create(fd);
+		handle = dumb_create(fd, 32, 32, 32, NULL, NULL);
 	else
 		handle = gem_create(fd, 4096);
 
-- 
2.5.0

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

[i-g-t PATCH v1 07/14] lib: Map dumb buffers

[Tomeu Vizoso]

If a buffer object is dumb, call DRM_IOCTL_MODE_MAP_DUMB when mapping
it. Also, don't call DRM_IOCTL_I915_GEM_SET_DOMAIN on dumb buffers.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 lib/igt_fb.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 3e76a419b3ee..cd1605308308 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -984,7 +984,20 @@ static void destroy_cairo_surface__gtt(void *arg)
 
 static void create_cairo_surface__gtt(int fd, struct igt_fb *fb)
 {
-	void *ptr = gem_mmap__gtt(fd, fb->gem_handle, fb->size, PROT_READ | PROT_WRITE);
+	void *ptr;
+
+	if (fb->is_dumb) {
+		struct drm_mode_map_dumb arg = {};
+
+		arg.handle = fb->gem_handle;
+
+		do_ioctl(fd, DRM_IOCTL_MODE_MAP_DUMB, &arg);
+
+		ptr = mmap(0, fb->size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, arg.offset);
+		igt_assert(ptr != MAP_FAILED);
+	} else
+		ptr = gem_mmap__gtt(fd, fb->gem_handle, fb->size,
+				    PROT_READ | PROT_WRITE);
 
 	fb->cairo_surface =
 		cairo_image_surface_create_for_data(ptr,
@@ -1006,8 +1019,9 @@ static cairo_surface_t *get_cairo_surface(int fd, struct igt_fb *fb)
 			create_cairo_surface__gtt(fd, fb);
 	}
 
-	gem_set_domain(fd, fb->gem_handle,
-		       I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
+	if (!fb->is_dumb)
+		gem_set_domain(fd, fb->gem_handle, I915_GEM_DOMAIN_CPU,
+			       I915_GEM_DOMAIN_CPU);
 
 	igt_assert(cairo_surface_status(fb->cairo_surface) == CAIRO_STATUS_SUCCESS);
 	return fb->cairo_surface;
-- 
2.5.0

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

[i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions

[Tomeu Vizoso]

igt_create_bo_with_dimensions() is intended to abstract differences
between drivers in buffer object creation.

The driver-specific ioctls will be called if the driver that is being
tested can satisfy the needs of the calling subtest, or it will be
skipped otherwise.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 lib/igt_fb.c | 83 ++++++++++++++++++++++++++++++++++++++++++------------------
 lib/igt_fb.h |  6 +++++
 2 files changed, 65 insertions(+), 24 deletions(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index cd1605308308..0a3526f4e4ea 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height, int bpp, uint64_t tiling,
 	*size_ret = size;
 }
 
+int igt_create_bo_with_dimensions(int fd, int width, int height,
+				  uint32_t format, uint64_t modifier,
+				  unsigned stride, unsigned *size_ret,
+				  unsigned *stride_ret, bool *is_dumb)
+{
+	int bpp = igt_drm_format_to_bpp(format);
+	int bo;
+
+	igt_assert((modifier && stride) || (!modifier && !stride));
+
+	if (modifier) {
+		unsigned size, calculated_stride;
+
+		igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
+				 &calculated_stride);
+		if (stride == 0)
+			stride = calculated_stride;
+
+		if (is_dumb)
+			*is_dumb = false;
+
+		if (is_i915_device(fd)) {
+
+			bo = gem_create(fd, size);
+			gem_set_tiling(fd, bo, modifier, stride);
+
+			if (size_ret)
+				*size_ret = size;
+
+			if (stride_ret)
+				*stride_ret = stride;
+
+			return bo;
+		} else {
+			bool driver_has_tiling_support = false;
+
+			igt_require(driver_has_tiling_support);
+			return -EINVAL;
+		}
+	} else {
+		if (is_dumb)
+			*is_dumb = true;
+		return dumb_create(fd, width, height, bpp, stride_ret, size_ret);
+	}
+}
+
 /* helpers to create nice-looking framebuffers */
-static int create_bo_for_fb(int fd, int width, int height, int bpp,
+static int create_bo_for_fb(int fd, int width, int height, uint32_t format,
 			    uint64_t tiling, unsigned bo_size,
 			    unsigned bo_stride, uint32_t *gem_handle_ret,
-			    unsigned *size_ret, unsigned *stride_ret)
+			    unsigned *size_ret, unsigned *stride_ret,
+			    bool *is_dumb)
 {
 	uint32_t gem_handle;
 	int ret = 0;
-	unsigned size, stride;
-
-	igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride);
-	if (bo_size == 0)
-		bo_size = size;
-	if (bo_stride == 0)
-		bo_stride = stride;
-
-	gem_handle = gem_create(fd, bo_size);
 
-	if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
-		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
-				       bo_stride);
+	gem_handle = igt_create_bo_with_dimensions(fd, width, height, format,
+						   tiling, bo_stride, size_ret,
+						   stride_ret, is_dumb);
 
-	*stride_ret = bo_stride;
-	*size_ret = bo_size;
 	*gem_handle_ret = gem_handle;
 
 	return ret;
@@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
 			   unsigned bo_stride)
 {
 	uint32_t fb_id;
-	int bpp;
 
 	memset(fb, 0, sizeof(*fb));
 
-	bpp = igt_drm_format_to_bpp(format);
-
-	igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], tiling=0x%"PRIx64", size=%d)\n",
-		  __func__, width, height, format, bpp, tiling, bo_size);
-	do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size,
+	igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64", size=%d)\n",
+		  __func__, width, height, format, tiling, bo_size);
+	do_or_die(create_bo_for_fb(fd, width, height, format, tiling, bo_size,
 				   bo_stride, &fb->gem_handle, &fb->size,
-				   &fb->stride));
+				   &fb->stride, &fb->is_dumb));
 
 	igt_debug("%s(handle=%d, pitch=%d)\n",
 		  __func__, fb->gem_handle, fb->stride);
@@ -860,6 +893,7 @@ struct fb_blit_upload {
 		uint32_t handle;
 		unsigned size, stride;
 		uint8_t *map;
+		bool is_dumb;
 	} linear;
 };
 
@@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct igt_fb *fb)
 				LOCAL_DRM_FORMAT_MOD_NONE, 0, 0,
 				&blit->linear.handle,
 				&blit->linear.size,
-				&blit->linear.stride);
+				&blit->linear.stride,
+				&blit->linear.is_dumb);
 
 	igt_assert(ret == 0);
 
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index 4e6a76947c18..10e59deebe88 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -47,6 +47,7 @@ typedef struct _cairo cairo_t;
 struct igt_fb {
 	uint32_t fb_id;
 	uint32_t gem_handle;
+	bool is_dumb;
 	uint32_t drm_format;
 	int width;
 	int height;
@@ -97,6 +98,11 @@ unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode,
 				  uint32_t format, uint64_t tiling);
 void igt_remove_fb(int fd, struct igt_fb *fb);
 
+int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t format,
+				  uint64_t modifier, unsigned stride,
+				  unsigned *stride_out, unsigned *size_out,
+				  bool *is_dumb);
+
 /* cairo-based painting */
 cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb);
 void igt_paint_color(cairo_t *cr, int x, int y, int w, int h,
-- 
2.5.0

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

[i-g-t PATCH v1 09/14] tests: Open any driver

[Tomeu Vizoso]

For those tests that now pass on drivers other than i915, call
drm_open_driver_master with DRIVER_ANY.

Also do so from igt_enable_connectors.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 lib/igt_kms.c           | 2 +-
 tests/drm_read.c        | 2 +-
 tests/kms_addfb_basic.c | 2 +-
 tests/kms_atomic.c      | 2 +-
 tests/kms_setmode.c     | 2 +-
 tests/kms_vblank.c      | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 90c8da7a3772..cd46ee496ba3 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -2018,7 +2018,7 @@ void igt_enable_connectors(void)
 	drmModeConnector *c;
 	int drm_fd;
 
-	drm_fd = drm_open_driver(DRIVER_INTEL);
+	drm_fd = drm_open_driver(DRIVER_ANY);
 
 	res = drmModeGetResources(drm_fd);
 
diff --git a/tests/drm_read.c b/tests/drm_read.c
index a27e5522daa0..c758ae8322b0 100644
--- a/tests/drm_read.c
+++ b/tests/drm_read.c
@@ -225,7 +225,7 @@ igt_main
 	siginterrupt(SIGALRM, 1);
 
 	igt_fixture {
-		fd = drm_open_driver_master(DRIVER_INTEL);
+		fd = drm_open_driver_master(DRIVER_ANY);
 		igt_require(pipe0_enabled(fd));
 	}
 
diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
index 73000d6e6fd7..d36cc6aab231 100644
--- a/tests/kms_addfb_basic.c
+++ b/tests/kms_addfb_basic.c
@@ -426,7 +426,7 @@ int gen;
 igt_main
 {
 	igt_fixture {
-		fd = drm_open_driver_master(DRIVER_INTEL);
+		fd = drm_open_driver_master(DRIVER_ANY);
 		gen = intel_gen(intel_get_drm_devid(fd));
 	}
 
diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
index 2f3080af8633..afed1735a7fc 100644
--- a/tests/kms_atomic.c
+++ b/tests/kms_atomic.c
@@ -720,7 +720,7 @@ static void atomic_setup(struct kms_atomic_state *state)
 	drmModePlaneResPtr res_plane;
 	int i;
 
-	desc->fd = drm_open_driver_master(DRIVER_INTEL);
+	desc->fd = drm_open_driver_master(DRIVER_ANY);
 	igt_assert_fd(desc->fd);
 
 	do_or_die(drmSetClientCap(desc->fd, DRM_CLIENT_CAP_ATOMIC, 1));
diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c
index 531ce8391fa7..bf3c260f8ac5 100644
--- a/tests/kms_setmode.c
+++ b/tests/kms_setmode.c
@@ -718,7 +718,7 @@ int main(int argc, char **argv)
 		     "only one of -d and -t is accepted\n");
 
 	igt_fixture {
-		drm_fd = drm_open_driver_master(DRIVER_INTEL);
+		drm_fd = drm_open_driver_master(DRIVER_ANY);
 		if (!dry_run)
 			kmstest_set_vt_graphics_mode();
 
diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
index e27a5dbd60cb..40ab6fdb1a11 100644
--- a/tests/kms_vblank.c
+++ b/tests/kms_vblank.c
@@ -177,7 +177,7 @@ igt_main
 	igt_skip_on_simulation();
 
 	igt_fixture {
-		fd = drm_open_driver(DRIVER_INTEL);
+		fd = drm_open_driver(DRIVER_ANY);
 		igt_require(crtc0_active(fd));
 	}
 
-- 
2.5.0

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

[i-g-t PATCH v1 10/14] kms_addfb_basic: call igt_create_bo_with_dimensions

[Tomeu Vizoso]

Many tests can do their work on drivers other than i915 and even with
just dumb buffers, so call igt_create_bo_with_dimensions instead of
gem_create which will paper out the differences and call the proper
ioctls or cause the subtest to be skipped if that's not possible.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 tests/kms_addfb_basic.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
index d36cc6aab231..473b5e9d7236 100644
--- a/tests/kms_addfb_basic.c
+++ b/tests/kms_addfb_basic.c
@@ -51,9 +51,11 @@ static void invalid_tests(int fd)
 	f.pitches[0] = 512*4;
 
 	igt_fixture {
-		gem_bo = gem_create(fd, 1024*1024*4);
+		gem_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
 		igt_assert(gem_bo);
-		gem_bo_small = gem_create(fd, 1024*1024*4 - 4096);
+		gem_bo_small = igt_create_bo_with_dimensions(fd, 1024, 1023,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
 		igt_assert(gem_bo_small);
 
 		f.handles[0] = gem_bo;
@@ -129,7 +131,8 @@ static void pitch_tests(int fd)
 	f.pitches[0] = 1024*4;
 
 	igt_fixture {
-		gem_bo = gem_create(fd, 1024*1024*4);
+		gem_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
 		igt_assert(gem_bo);
 	}
 
@@ -211,9 +214,11 @@ static void size_tests(int fd)
 	f_8.pitches[0] = 1024*2;
 
 	igt_fixture {
-		gem_bo = gem_create(fd, 1024*1024*4);
+		gem_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
 		igt_assert(gem_bo);
-		gem_bo_small = gem_create(fd, 1024*1024*4 - 4096);
+		gem_bo_small = igt_create_bo_with_dimensions(fd, 1024, 1023,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
 		igt_assert(gem_bo_small);
 	}
 
@@ -293,7 +298,8 @@ static void addfb25_tests(int fd)
 	struct local_drm_mode_fb_cmd2 f = {};
 
 	igt_fixture {
-		gem_bo = gem_create(fd, 1024*1024*4);
+		gem_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
 		igt_assert(gem_bo);
 
 		memset(&f, 0, sizeof(f));
@@ -364,9 +370,11 @@ static void addfb25_ytile(int fd, int gen)
 	int shouldret;
 
 	igt_fixture {
-		gem_bo = gem_create(fd, 1024*1024*4);
+		gem_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
 		igt_assert(gem_bo);
-		gem_bo_small = gem_create(fd, 1024*1023*4);
+		gem_bo_small = igt_create_bo_with_dimensions(fd, 1024, 1023,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
 		igt_assert(gem_bo_small);
 
 		shouldret = gen >= 9 ? 0 : -1;
-- 
2.5.0

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

[i-g-t PATCH v1 11/14] kms_addfb_basic: move tiling functionality into each subtest

[Tomeu Vizoso]

Because calls to gem_set_tiling will cause the subtest to be skipped on
drivers other than i915, move them to each subtest that needs them so
the other subtests aren't skipped as well.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 tests/kms_addfb_basic.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
index 473b5e9d7236..e61d2502d78b 100644
--- a/tests/kms_addfb_basic.c
+++ b/tests/kms_addfb_basic.c
@@ -156,17 +156,17 @@ static void pitch_tests(int fd)
 		}
 	}
 
-	igt_fixture
-		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
 	f.pitches[0] = 1024*4;
 
 	igt_subtest("basic-X-tiled") {
+		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
 		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == 0);
 		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0);
 		f.fb_id = 0;
 	}
 
 	igt_subtest("framebuffer-vs-set-tiling") {
+		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
 		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == 0);
 		igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 512*4) == -EBUSY);
 		igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4) == -EBUSY);
@@ -176,14 +176,14 @@ static void pitch_tests(int fd)
 
 	f.pitches[0] = 512*4;
 	igt_subtest("tile-pitch-mismatch") {
+		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
 		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
 			   errno == EINVAL);
 	}
 
-	igt_fixture
-		gem_set_tiling(fd, gem_bo, I915_TILING_Y, 1024*4);
 	f.pitches[0] = 1024*4;
 	igt_subtest("basic-Y-tiled") {
+		gem_set_tiling(fd, gem_bo, I915_TILING_Y, 1024*4);
 		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
 			   errno == EINVAL);
 	}
-- 
2.5.0

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

[i-g-t PATCH v1 12/14] kms_addfb_basic: Split tiling_tests off

[Tomeu Vizoso]

Move tests requiring tiled BOs to the end so they don't cause unrelated
subtests to be skipped when testing drivers with only dumb buffer
support.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 tests/kms_addfb_basic.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
index e61d2502d78b..6dfaecfc38a7 100644
--- a/tests/kms_addfb_basic.c
+++ b/tests/kms_addfb_basic.c
@@ -156,16 +156,50 @@ static void pitch_tests(int fd)
 		}
 	}
 
+	igt_fixture
+		gem_close(fd, gem_bo);
+}
+
+
+static void tiling_tests(int fd)
+{
+	struct drm_mode_fb_cmd2 f = {};
+	uint32_t tiled_x_bo;
+	uint32_t tiled_y_bo;
+
+	f.width = 512;
+	f.height = 512;
+	f.pixel_format = DRM_FORMAT_XRGB8888;
 	f.pitches[0] = 1024*4;
 
+	igt_fixture {
+		tiled_x_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
+			DRM_FORMAT_XRGB8888, LOCAL_I915_FORMAT_MOD_X_TILED,
+			1024*4, NULL, NULL, NULL);
+		igt_assert(tiled_x_bo);
+
+		tiled_y_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
+			DRM_FORMAT_XRGB8888, LOCAL_I915_FORMAT_MOD_Y_TILED,
+			1024*4, NULL, NULL, NULL);
+		igt_assert(tiled_y_bo);
+
+		gem_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
+		igt_assert(gem_bo);
+	}
+
+	f.pitches[0] = 1024*4;
 	igt_subtest("basic-X-tiled") {
-		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
+		f.handles[0] = tiled_x_bo;
+
 		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == 0);
 		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0);
 		f.fb_id = 0;
 	}
 
 	igt_subtest("framebuffer-vs-set-tiling") {
+		f.handles[0] = gem_bo;
+
 		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
 		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == 0);
 		igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 512*4) == -EBUSY);
@@ -176,20 +210,24 @@ static void pitch_tests(int fd)
 
 	f.pitches[0] = 512*4;
 	igt_subtest("tile-pitch-mismatch") {
-		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
+		f.handles[0] = tiled_x_bo;
+
 		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
 			   errno == EINVAL);
 	}
 
 	f.pitches[0] = 1024*4;
 	igt_subtest("basic-Y-tiled") {
-		gem_set_tiling(fd, gem_bo, I915_TILING_Y, 1024*4);
+		f.handles[0] = tiled_y_bo;
+
 		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
 			   errno == EINVAL);
 	}
 
-	igt_fixture
-		gem_close(fd, gem_bo);
+	igt_fixture {
+		gem_close(fd, tiled_x_bo);
+		gem_close(fd, tiled_y_bo);
+	}
 }
 
 static void size_tests(int fd)
@@ -448,6 +486,8 @@ igt_main
 
 	addfb25_ytile(fd, gen);
 
+	tiling_tests(fd);
+
 	igt_fixture
 		close(fd);
 }
-- 
2.5.0

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

[i-g-t PATCH v1 13/14] kms_addfb_basic: Move calls to gem_set_tiling to the subtests

[Tomeu Vizoso]

So they don't cause unrelated subtests to be skipped when testing
drivers other than i915.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 tests/kms_addfb_basic.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
index 6dfaecfc38a7..daba6b9aaeed 100644
--- a/tests/kms_addfb_basic.c
+++ b/tests/kms_addfb_basic.c
@@ -316,10 +316,8 @@ static void size_tests(int fd)
 		f.fb_id = 0;
 	}
 
-	igt_fixture
-		gem_set_tiling(fd, gem_bo_small, I915_TILING_X, 1024*4);
-
 	igt_subtest("bo-too-small-due-to-tiling") {
+		gem_set_tiling(fd, gem_bo_small, I915_TILING_X, 1024*4);
 		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
 			   errno == EINVAL);
 	}
@@ -368,10 +366,8 @@ static void addfb25_tests(int fd)
 		igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) < 0 && errno == EINVAL);
 	}
 
-	igt_fixture
-		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
-
 	igt_subtest("addfb25-X-tiled-mismatch") {
+		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
 		igt_require_fb_modifiers(fd);
 
 		f.modifier[0] = LOCAL_DRM_FORMAT_MOD_NONE;
@@ -379,6 +375,7 @@ static void addfb25_tests(int fd)
 	}
 
 	igt_subtest("addfb25-X-tiled") {
+		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
 		igt_require_fb_modifiers(fd);
 
 		f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED;
-- 
2.5.0

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

[i-g-t PATCH v1 14/14] kms_addfb_basic: Get intel gen from within subtest

[Tomeu Vizoso]

Because determining the Intel GFX generation requires a call to
DRM_IOCTL_I915_GETPARAM, move the code that requires it to a subtest
that can be skipped on drivers other than i915.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 tests/kms_addfb_basic.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
index daba6b9aaeed..30f312291fe7 100644
--- a/tests/kms_addfb_basic.c
+++ b/tests/kms_addfb_basic.c
@@ -399,10 +399,21 @@ static void addfb25_tests(int fd)
 		gem_close(fd, gem_bo);
 }
 
-static void addfb25_ytile(int fd, int gen)
+static int addfb_expected_ret(int fd)
+{
+	int gen;
+
+	if (!is_i915_device(fd))
+		return 0;
+
+	gen = intel_gen(intel_get_drm_devid(fd));
+	return gen >= 9 ? 0 : -1;
+}
+
+static void addfb25_ytile(int fd)
 {
 	struct local_drm_mode_fb_cmd2 f = {};
-	int shouldret;
+	int gen;
 
 	igt_fixture {
 		gem_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
@@ -412,8 +423,6 @@ static void addfb25_ytile(int fd, int gen)
 			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
 		igt_assert(gem_bo_small);
 
-		shouldret = gen >= 9 ? 0 : -1;
-
 		memset(&f, 0, sizeof(f));
 
 		f.width = 1024;
@@ -430,8 +439,9 @@ static void addfb25_ytile(int fd, int gen)
 		igt_require_fb_modifiers(fd);
 
 		f.modifier[0] = LOCAL_I915_FORMAT_MOD_Y_TILED;
-		igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == shouldret);
-		if (!shouldret)
+		igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) ==
+			   addfb_expected_ret(fd));
+		if (!addfb_expected_ret(fd))
 			igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0);
 		f.fb_id = 0;
 	}
@@ -440,14 +450,17 @@ static void addfb25_ytile(int fd, int gen)
 		igt_require_fb_modifiers(fd);
 
 		f.modifier[0] = LOCAL_I915_FORMAT_MOD_Yf_TILED;
-		igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) == shouldret);
-		if (!shouldret)
+		igt_assert(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) ==
+			   addfb_expected_ret(fd));
+		if (!addfb_expected_ret(fd))
 			igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0);
 		f.fb_id = 0;
 	}
 
 	igt_subtest("addfb25-Y-tiled-small") {
 		igt_require_fb_modifiers(fd);
+
+		gen = intel_gen(intel_get_drm_devid(fd));
 		igt_require(gen >= 9);
 
 		f.modifier[0] = LOCAL_I915_FORMAT_MOD_Y_TILED;
@@ -464,14 +477,11 @@ static void addfb25_ytile(int fd, int gen)
 }
 
 int fd;
-int gen;
 
 igt_main
 {
-	igt_fixture {
+	igt_fixture
 		fd = drm_open_driver_master(DRIVER_ANY);
-		gen = intel_gen(intel_get_drm_devid(fd));
-	}
 
 	invalid_tests(fd);
 
@@ -481,7 +491,7 @@ igt_main
 
 	addfb25_tests(fd);
 
-	addfb25_ytile(fd, gen);
+	addfb25_ytile(fd);
 
 	tiling_tests(fd);
 
-- 
2.5.0

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

Re: [i-g-t PATCH v1 01/14] lib: add igt_require_intel

[Chris Wilson]

On Wed, Mar 02, 2016 at 03:00:08PM +0100, Tomeu Vizoso wrote:
> Add function that requires that the driver we are talking to is i915.
> 
> This allows us to skip subtests that are specific to that driver.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> 
>  lib/drmtest.c | 5 +++++
>  lib/drmtest.h | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index 7b2227fe0f6a..bb9ca507a922 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -399,3 +399,8 @@ int drm_open_driver_render(int chipset)
>  
>  	return fd;
>  }
> +
> +void igt_require_intel(int fd)
> +{
> +	igt_require(is_i915_device(fd) && is_intel(fd));
> +}

If you quickly change is_intel() to has_known_intel_chipset() this then
reads much better.
-Chris

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

Re: [i-g-t PATCH v1 07/14] lib: Map dumb buffers

[Chris Wilson]

On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote:
> @@ -1006,8 +1019,9 @@ static cairo_surface_t *get_cairo_surface(int fd, struct igt_fb *fb)
>  			create_cairo_surface__gtt(fd, fb);
>  	}
>  
> -	gem_set_domain(fd, fb->gem_handle,
> -		       I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> +	if (!fb->is_dumb)
> +		gem_set_domain(fd, fb->gem_handle, I915_GEM_DOMAIN_CPU,
> +			       I915_GEM_DOMAIN_CPU);

At the risk of opening a can-of-worms, what is the synchronisation
protocol for dumb buffers? Even CPU access to a dumb needs set-domain on
Intel.
-Chris

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

Re: [i-g-t PATCH v1 07/14] lib: Map dumb buffers

[Daniel Stone]

On Wed, 2016-03-02 at 14:21 +0000, Chris Wilson wrote:
> On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote:
> > @@ -1006,8 +1019,9 @@ static cairo_surface_t *get_cairo_surface(int
> > fd, struct igt_fb *fb)
> >  			create_cairo_surface__gtt(fd, fb);
> >  	}
> >  
> > -	gem_set_domain(fd, fb->gem_handle,
> > -		       I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> > +	if (!fb->is_dumb)
> > +		gem_set_domain(fd, fb->gem_handle,
> > I915_GEM_DOMAIN_CPU,
> > +			       I915_GEM_DOMAIN_CPU);
> At the risk of opening a can-of-worms, what is the synchronisation
> protocol for dumb buffers? Even CPU access to a dumb needs set-domain 
> on
> Intel.

Then Intel is broken, because the literal entire point of dumb buffers
is that you do not require driver-specific calls to operate them.

Map, populate, unmap, display.

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

Re: [i-g-t PATCH v1 07/14] lib: Map dumb buffers

[Chris Wilson]

On Wed, Mar 02, 2016 at 02:22:58PM +0000, Daniel Stone wrote:
> On Wed, 2016-03-02 at 14:21 +0000, Chris Wilson wrote:
> > On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote:
> > > @@ -1006,8 +1019,9 @@ static cairo_surface_t *get_cairo_surface(int
> > > fd, struct igt_fb *fb)
> > >  			create_cairo_surface__gtt(fd, fb);
> > >  	}
> > >  
> > > -	gem_set_domain(fd, fb->gem_handle,
> > > -		       I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> > > +	if (!fb->is_dumb)
> > > +		gem_set_domain(fd, fb->gem_handle,
> > > I915_GEM_DOMAIN_CPU,
> > > +			       I915_GEM_DOMAIN_CPU);
> > At the risk of opening a can-of-worms, what is the synchronisation
> > protocol for dumb buffers? Even CPU access to a dumb needs set-domain 
> > on
> > Intel.
> 
> Then Intel is broken, because the literal entire point of dumb buffers
> is that you do not require driver-specific calls to operate them.
> 
> Map, populate, unmap, display.

Don't forget to call dirtyfb then.
-Chris

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

Re: [i-g-t PATCH v1 07/14] lib: Map dumb buffers

[Daniel Stone]

On Wed, 2016-03-02 at 14:39 +0000, Chris Wilson wrote:
> On Wed, Mar 02, 2016 at 02:22:58PM +0000, Daniel Stone wrote:
> > On Wed, 2016-03-02 at 14:21 +0000, Chris Wilson wrote:
> > > On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote:
> > > > -	gem_set_domain(fd, fb->gem_handle,
> > > > -		       I915_GEM_DOMAIN_CPU,
> > > > I915_GEM_DOMAIN_CPU);
> > > > +	if (!fb->is_dumb)
> > > > +		gem_set_domain(fd, fb->gem_handle,
> > > > I915_GEM_DOMAIN_CPU,
> > > > +			       I915_GEM_DOMAIN_CPU);
> > > At the risk of opening a can-of-worms, what is the
> > > synchronisation
> > > protocol for dumb buffers? Even CPU access to a dumb needs set-
> > > domain 
> > > on
> > > Intel.
> > Then Intel is broken, because the literal entire point of dumb
> > buffers
> > is that you do not require driver-specific calls to operate them.
> > 
> > Map, populate, unmap, display.
> Don't forget to call dirtyfb then.

Are you talking about frontbuffer rendering, or pageflipping between
two dumb buffers?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [i-g-t PATCH v1 07/14] lib: Map dumb buffers

[Chris Wilson]

On Wed, Mar 02, 2016 at 02:40:44PM +0000, Daniel Stone wrote:
> 
> On Wed, 2016-03-02 at 14:39 +0000, Chris Wilson wrote:
> > On Wed, Mar 02, 2016 at 02:22:58PM +0000, Daniel Stone wrote:
> > > On Wed, 2016-03-02 at 14:21 +0000, Chris Wilson wrote:
> > > > On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote:
> > > > > -	gem_set_domain(fd, fb->gem_handle,
> > > > > -		       I915_GEM_DOMAIN_CPU,
> > > > > I915_GEM_DOMAIN_CPU);
> > > > > +	if (!fb->is_dumb)
> > > > > +		gem_set_domain(fd, fb->gem_handle,
> > > > > I915_GEM_DOMAIN_CPU,
> > > > > +			       I915_GEM_DOMAIN_CPU);
> > > > At the risk of opening a can-of-worms, what is the
> > > > synchronisation
> > > > protocol for dumb buffers? Even CPU access to a dumb needs set-
> > > > domain 
> > > > on
> > > > Intel.
> > > Then Intel is broken, because the literal entire point of dumb
> > > buffers
> > > is that you do not require driver-specific calls to operate them.
> > > 
> > > Map, populate, unmap, display.
> > Don't forget to call dirtyfb then.
> 
> Are you talking about frontbuffer rendering, or pageflipping between
> two dumb buffers?

Afaik, no one yet tracks damage on a backbuffer before a flip. But we
don't constrain the tests to backbuffer as we do need to exercise
frontbuffer rendering and iirc those tests all use set-domain. I don't
see any PSR/FBC testing using the dumb framebuffers... Or is the dumb
framebuffer purely a backbuffer + flip model?
-Chris

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

Re: [i-g-t PATCH v1 07/14] lib: Map dumb buffers

[Daniel Stone]

On Wed, 2016-03-02 at 14:54 +0000, Chris Wilson wrote:
> On Wed, Mar 02, 2016 at 02:40:44PM +0000, Daniel Stone wrote:
> > On Wed, 2016-03-02 at 14:39 +0000, Chris Wilson wrote:
> > > Don't forget to call dirtyfb then.
> > Are you talking about frontbuffer rendering, or pageflipping
> > between
> > two dumb buffers?
> Afaik, no one yet tracks damage on a backbuffer before a flip. But we
> don't constrain the tests to backbuffer as we do need to exercise
> frontbuffer rendering and iirc those tests all use set-domain. I
> don't
> see any PSR/FBC testing using the dumb framebuffers... Or is the dumb
> framebuffer purely a backbuffer + flip model?

Right, Weston strictly uses pageflipping, because frontbuffer rendering
isn't pretty. xf86-video-modesetting uses frontbuffer rendering, and
does DirtyFB. So I'd say that's our ABI.

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

Re: [i-g-t PATCH v1 06/14] lib: Add wrapper for DRM_IOCTL_MODE_CREATE_DUMB

[Daniel Vetter]

On Wed, Mar 02, 2016 at 03:00:13PM +0100, Tomeu Vizoso wrote:
> In order to test drivers that don't have support for proper buffer
> objects, add a wrapper for creating dumb buffer objects that will be
> called from the lib code for those subtests that don't need to care.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Hm, in a way ioctl_wrappers.c is just i915 gem ioctl wrappers. I think
dumb makes much more sense as part of the kmstest_* set of functions in
igt_kms.c. Also maybe kmstest_dumb_create for consistency.
-Daniel

> ---
> 
>  lib/ioctl_wrappers.c | 36 ++++++++++++++++++++++++++++++++++++
>  lib/ioctl_wrappers.h |  1 +
>  tests/drm_read.c     | 16 +---------------
>  tests/gem_exec_blt.c | 18 +-----------------
>  4 files changed, 39 insertions(+), 32 deletions(-)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 0221b7fef3a1..d842d860ba65 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -531,6 +531,42 @@ uint32_t gem_create(int fd, uint64_t size)
>  }
>  
>  /**
> + * dumb_create:
> + * @fd: open drm file descriptor
> + * @width: width of the buffer in pixels
> + * @height: height of the buffer in pixels
> + * @bpp: bytes per pixel of the buffer
> + *
> + * This wraps the CREATE_DUMB ioctl, which allocates a new dumb buffer object
> + * for the specified dimensions.
> + *
> + * Returns: The file-private handle of the created buffer object
> + */
> +uint32_t dumb_create(int fd, int width, int height, int bpp, unsigned *stride,
> +		     unsigned *size)
> +{
> +	struct drm_mode_create_dumb create;
> +
> +	memset(&create, 0, sizeof(create));
> +	create.width = width;
> +	create.height = height;
> +	create.bpp = bpp;
> +
> +	create.handle = 0;
> +	do_ioctl(fd, DRM_IOCTL_MODE_CREATE_DUMB, &create);
> +	igt_assert(create.handle);
> +	igt_assert(create.size >= width * height * bpp / 8);
> +
> +	if (stride)
> +		*stride = create.pitch;
> +
> +	if (size)
> +		*size = create.size;
> +
> +	return create.handle;
> +}
> +
> +/**
>   * __gem_execbuf:
>   * @fd: open i915 drm file descriptor
>   * @execbuf: execbuffer data structure
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index f59eafba4bdd..9282ffdd8520 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -62,6 +62,7 @@ uint32_t __gem_create_stolen(int fd, uint64_t size);
>  uint32_t gem_create_stolen(int fd, uint64_t size);
>  uint32_t __gem_create(int fd, int size);
>  uint32_t gem_create(int fd, uint64_t size);
> +uint32_t dumb_create(int fd, int width, int height, int bpp, unsigned *stride, unsigned *size);
>  void gem_execbuf(int fd, struct drm_i915_gem_execbuffer2 *execbuf);
>  int __gem_execbuf(int fd, struct drm_i915_gem_execbuffer2 *execbuf);
>  
> diff --git a/tests/drm_read.c b/tests/drm_read.c
> index faa3df862ea6..a27e5522daa0 100644
> --- a/tests/drm_read.c
> +++ b/tests/drm_read.c
> @@ -120,20 +120,6 @@ static void test_invalid_buffer(int in)
>  	teardown(fd);
>  }
>  
> -static uint32_t dumb_create(int fd)
> -{
> -	struct drm_mode_create_dumb arg;
> -
> -	arg.bpp = 32;
> -	arg.width = 32;
> -	arg.height = 32;
> -
> -	do_ioctl(fd, DRM_IOCTL_MODE_CREATE_DUMB, &arg);
> -	igt_assert(arg.size >= 4096);
> -
> -	return arg.handle;
> -}
> -
>  static void test_fault_buffer(int in)
>  {
>  	int fd = setup(in, 0);
> @@ -141,7 +127,7 @@ static void test_fault_buffer(int in)
>  	char *buf;
>  
>  	memset(&arg, 0, sizeof(arg));
> -	arg.handle = dumb_create(fd);
> +	arg.handle = dumb_create(fd, 32, 32, 32, NULL, NULL);
>  
>  	do_ioctl(fd, DRM_IOCTL_MODE_MAP_DUMB, &arg);
>  
> diff --git a/tests/gem_exec_blt.c b/tests/gem_exec_blt.c
> index 74f5c2ba87ad..42a030fb4f57 100644
> --- a/tests/gem_exec_blt.c
> +++ b/tests/gem_exec_blt.c
> @@ -170,22 +170,6 @@ static const char *bytes_per_sec(char *buf, double v)
>  	return buf;
>  }
>  
> -static uint32_t dumb_create(int fd)
> -{
> -	struct drm_mode_create_dumb arg;
> -	int ret;
> -
> -	arg.bpp = 32;
> -	arg.width = 32;
> -	arg.height = 32;
> -
> -	ret = drmIoctl(fd, DRM_IOCTL_MODE_CREATE_DUMB, &arg);
> -	igt_assert_eq(ret, 0);
> -	igt_assert(arg.size >= 4096);
> -
> -	return arg.handle;
> -}
> -
>  static int dcmp(const void *A, const void *B)
>  {
>  	const double *a = A, *b = B;
> @@ -209,7 +193,7 @@ static void run(int object_size, bool dumb)
>  
>  	fd = drm_open_driver(DRIVER_INTEL);
>  	if (dumb)
> -		handle = dumb_create(fd);
> +		handle = dumb_create(fd, 32, 32, 32, NULL, NULL);
>  	else
>  		handle = gem_create(fd, 4096);
>  
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://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

Re: [i-g-t PATCH v1 07/14] lib: Map dumb buffers

[Daniel Vetter]

On Wed, Mar 02, 2016 at 02:54:20PM +0000, Chris Wilson wrote:
> On Wed, Mar 02, 2016 at 02:40:44PM +0000, Daniel Stone wrote:
> > 
> > On Wed, 2016-03-02 at 14:39 +0000, Chris Wilson wrote:
> > > On Wed, Mar 02, 2016 at 02:22:58PM +0000, Daniel Stone wrote:
> > > > On Wed, 2016-03-02 at 14:21 +0000, Chris Wilson wrote:
> > > > > On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote:
> > > > > > -	gem_set_domain(fd, fb->gem_handle,
> > > > > > -		       I915_GEM_DOMAIN_CPU,
> > > > > > I915_GEM_DOMAIN_CPU);
> > > > > > +	if (!fb->is_dumb)
> > > > > > +		gem_set_domain(fd, fb->gem_handle,
> > > > > > I915_GEM_DOMAIN_CPU,
> > > > > > +			       I915_GEM_DOMAIN_CPU);
> > > > > At the risk of opening a can-of-worms, what is the
> > > > > synchronisation
> > > > > protocol for dumb buffers? Even CPU access to a dumb needs set-
> > > > > domain 
> > > > > on
> > > > > Intel.
> > > > Then Intel is broken, because the literal entire point of dumb
> > > > buffers
> > > > is that you do not require driver-specific calls to operate them.
> > > > 
> > > > Map, populate, unmap, display.
> > > Don't forget to call dirtyfb then.
> > 
> > Are you talking about frontbuffer rendering, or pageflipping between
> > two dumb buffers?
> 
> Afaik, no one yet tracks damage on a backbuffer before a flip. But we
> don't constrain the tests to backbuffer as we do need to exercise
> frontbuffer rendering and iirc those tests all use set-domain. I don't
> see any PSR/FBC testing using the dumb framebuffers... Or is the dumb
> framebuffer purely a backbuffer + flip model?

Yeah, but for those tests we do have explict set_domain calls. Anyway the
dumb model is mmap + either flip (setcrtc, setplane, page_flip, atomic) or
dirtyfb. I think for low level functions like these exposing this
explicitly to tests is ok.

If you mix dumb with rendering you simply need to know what you're doing.
But for rendering that's a requirement anyway.
-Daniel
-- 
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

Re: [i-g-t PATCH v1 07/14] lib: Map dumb buffers

[Daniel Vetter]

On Sat, Mar 05, 2016 at 01:24:19PM +0100, Daniel Vetter wrote:
> On Wed, Mar 02, 2016 at 02:54:20PM +0000, Chris Wilson wrote:
> > On Wed, Mar 02, 2016 at 02:40:44PM +0000, Daniel Stone wrote:
> > > 
> > > On Wed, 2016-03-02 at 14:39 +0000, Chris Wilson wrote:
> > > > On Wed, Mar 02, 2016 at 02:22:58PM +0000, Daniel Stone wrote:
> > > > > On Wed, 2016-03-02 at 14:21 +0000, Chris Wilson wrote:
> > > > > > On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote:
> > > > > > > -	gem_set_domain(fd, fb->gem_handle,
> > > > > > > -		       I915_GEM_DOMAIN_CPU,
> > > > > > > I915_GEM_DOMAIN_CPU);
> > > > > > > +	if (!fb->is_dumb)
> > > > > > > +		gem_set_domain(fd, fb->gem_handle,
> > > > > > > I915_GEM_DOMAIN_CPU,
> > > > > > > +			       I915_GEM_DOMAIN_CPU);
> > > > > > At the risk of opening a can-of-worms, what is the
> > > > > > synchronisation
> > > > > > protocol for dumb buffers? Even CPU access to a dumb needs set-
> > > > > > domain 
> > > > > > on
> > > > > > Intel.
> > > > > Then Intel is broken, because the literal entire point of dumb
> > > > > buffers
> > > > > is that you do not require driver-specific calls to operate them.
> > > > > 
> > > > > Map, populate, unmap, display.
> > > > Don't forget to call dirtyfb then.
> > > 
> > > Are you talking about frontbuffer rendering, or pageflipping between
> > > two dumb buffers?
> > 
> > Afaik, no one yet tracks damage on a backbuffer before a flip. But we
> > don't constrain the tests to backbuffer as we do need to exercise
> > frontbuffer rendering and iirc those tests all use set-domain. I don't
> > see any PSR/FBC testing using the dumb framebuffers... Or is the dumb
> > framebuffer purely a backbuffer + flip model?
> 
> Yeah, but for those tests we do have explict set_domain calls. Anyway the
> dumb model is mmap + either flip (setcrtc, setplane, page_flip, atomic) or
> dirtyfb. I think for low level functions like these exposing this
> explicitly to tests is ok.
> 
> If you mix dumb with rendering you simply need to know what you're doing.
> But for rendering that's a requirement anyway.

Ok, maybe I should read the patch, it's a high-level function ;-) In that
case, yes please add a dirtyfb ioctl call _after_ the rendering to make it
all just work. Since we don't know how callers use it.

But for library completeness I think it'd be good to have kmstest_
wrappers for both dumb mmap and dirtyfb (maybe just defer to the libdrm
one) like with the dumb create one.
-Daniel
-- 
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

Re: [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions

[Daniel Vetter]

On Wed, Mar 02, 2016 at 03:00:15PM +0100, Tomeu Vizoso wrote:
> igt_create_bo_with_dimensions() is intended to abstract differences
> between drivers in buffer object creation.
> 
> The driver-specific ioctls will be called if the driver that is being
> tested can satisfy the needs of the calling subtest, or it will be
> skipped otherwise.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> 
>  lib/igt_fb.c | 83 ++++++++++++++++++++++++++++++++++++++++++------------------
>  lib/igt_fb.h |  6 +++++
>  2 files changed, 65 insertions(+), 24 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index cd1605308308..0a3526f4e4ea 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height, int bpp, uint64_t tiling,
>  	*size_ret = size;
>  }
>  
> +int igt_create_bo_with_dimensions(int fd, int width, int height,

Needs gtkdoc. Also this seems to overlap in functionality with the very
recently added igt_calc_fb_size. Could we perhaps implement your new
function here using igt_calc_fb_size plus igt_create_fb_with_size? Then
this would just be a convenience wrapper.

I just want to make sure that we have a consistent interface to igt_fb.c
functionality and avoid the need that driver-specific tiling formats need
to overwrite bazillion of different places.
-Daniel

> +				  uint32_t format, uint64_t modifier,
> +				  unsigned stride, unsigned *size_ret,
> +				  unsigned *stride_ret, bool *is_dumb)
> +{
> +	int bpp = igt_drm_format_to_bpp(format);
> +	int bo;
> +
> +	igt_assert((modifier && stride) || (!modifier && !stride));
> +
> +	if (modifier) {
> +		unsigned size, calculated_stride;
> +
> +		igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
> +				 &calculated_stride);
> +		if (stride == 0)
> +			stride = calculated_stride;
> +
> +		if (is_dumb)
> +			*is_dumb = false;
> +
> +		if (is_i915_device(fd)) {
> +
> +			bo = gem_create(fd, size);
> +			gem_set_tiling(fd, bo, modifier, stride);
> +
> +			if (size_ret)
> +				*size_ret = size;
> +
> +			if (stride_ret)
> +				*stride_ret = stride;
> +
> +			return bo;
> +		} else {
> +			bool driver_has_tiling_support = false;
> +
> +			igt_require(driver_has_tiling_support);
> +			return -EINVAL;
> +		}
> +	} else {
> +		if (is_dumb)
> +			*is_dumb = true;
> +		return dumb_create(fd, width, height, bpp, stride_ret, size_ret);
> +	}
> +}
> +
>  /* helpers to create nice-looking framebuffers */
> -static int create_bo_for_fb(int fd, int width, int height, int bpp,
> +static int create_bo_for_fb(int fd, int width, int height, uint32_t format,
>  			    uint64_t tiling, unsigned bo_size,
>  			    unsigned bo_stride, uint32_t *gem_handle_ret,
> -			    unsigned *size_ret, unsigned *stride_ret)
> +			    unsigned *size_ret, unsigned *stride_ret,
> +			    bool *is_dumb)
>  {
>  	uint32_t gem_handle;
>  	int ret = 0;
> -	unsigned size, stride;
> -
> -	igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride);
> -	if (bo_size == 0)
> -		bo_size = size;
> -	if (bo_stride == 0)
> -		bo_stride = stride;
> -
> -	gem_handle = gem_create(fd, bo_size);
>  
> -	if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
> -		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
> -				       bo_stride);
> +	gem_handle = igt_create_bo_with_dimensions(fd, width, height, format,
> +						   tiling, bo_stride, size_ret,
> +						   stride_ret, is_dumb);
>  
> -	*stride_ret = bo_stride;
> -	*size_ret = bo_size;
>  	*gem_handle_ret = gem_handle;
>  
>  	return ret;
> @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
>  			   unsigned bo_stride)
>  {
>  	uint32_t fb_id;
> -	int bpp;
>  
>  	memset(fb, 0, sizeof(*fb));
>  
> -	bpp = igt_drm_format_to_bpp(format);
> -
> -	igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], tiling=0x%"PRIx64", size=%d)\n",
> -		  __func__, width, height, format, bpp, tiling, bo_size);
> -	do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size,
> +	igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64", size=%d)\n",
> +		  __func__, width, height, format, tiling, bo_size);
> +	do_or_die(create_bo_for_fb(fd, width, height, format, tiling, bo_size,
>  				   bo_stride, &fb->gem_handle, &fb->size,
> -				   &fb->stride));
> +				   &fb->stride, &fb->is_dumb));
>  
>  	igt_debug("%s(handle=%d, pitch=%d)\n",
>  		  __func__, fb->gem_handle, fb->stride);
> @@ -860,6 +893,7 @@ struct fb_blit_upload {
>  		uint32_t handle;
>  		unsigned size, stride;
>  		uint8_t *map;
> +		bool is_dumb;
>  	} linear;
>  };
>  
> @@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct igt_fb *fb)
>  				LOCAL_DRM_FORMAT_MOD_NONE, 0, 0,
>  				&blit->linear.handle,
>  				&blit->linear.size,
> -				&blit->linear.stride);
> +				&blit->linear.stride,
> +				&blit->linear.is_dumb);
>  
>  	igt_assert(ret == 0);
>  
> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
> index 4e6a76947c18..10e59deebe88 100644
> --- a/lib/igt_fb.h
> +++ b/lib/igt_fb.h
> @@ -47,6 +47,7 @@ typedef struct _cairo cairo_t;
>  struct igt_fb {
>  	uint32_t fb_id;
>  	uint32_t gem_handle;
> +	bool is_dumb;
>  	uint32_t drm_format;
>  	int width;
>  	int height;
> @@ -97,6 +98,11 @@ unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode,
>  				  uint32_t format, uint64_t tiling);
>  void igt_remove_fb(int fd, struct igt_fb *fb);
>  
> +int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t format,
> +				  uint64_t modifier, unsigned stride,
> +				  unsigned *stride_out, unsigned *size_out,
> +				  bool *is_dumb);
> +
>  /* cairo-based painting */
>  cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb);
>  void igt_paint_color(cairo_t *cr, int x, int y, int w, int h,
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://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

Re: [i-g-t PATCH v1 12/14] kms_addfb_basic: Split tiling_tests off

[Daniel Vetter]

On Wed, Mar 02, 2016 at 03:00:19PM +0100, Tomeu Vizoso wrote:
> Move tests requiring tiled BOs to the end so they don't cause unrelated
> subtests to be skipped when testing drivers with only dumb buffer
> support.

This uncovers a deficiency in igt skip infrastructure when you have
disjoint sets of subtests with common requirements, e.g.

{
	igt_require_intel();

	... intel tiling subtests;
}

{
	igt_require_amd();

	... amd tiling subtests;
}

The current rule is that a skip outside of subtest skips _all_ remaining
subtests. We might want to introduce some kind of igt_subtest_group block
so that only that group is skipped.

Anyway, just a thought, nothing you'll have to tackle here ;-)
-Daniel

> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> 
>  tests/kms_addfb_basic.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
> index e61d2502d78b..6dfaecfc38a7 100644
> --- a/tests/kms_addfb_basic.c
> +++ b/tests/kms_addfb_basic.c
> @@ -156,16 +156,50 @@ static void pitch_tests(int fd)
>  		}
>  	}
>  
> +	igt_fixture
> +		gem_close(fd, gem_bo);
> +}
> +
> +
> +static void tiling_tests(int fd)
> +{
> +	struct drm_mode_fb_cmd2 f = {};
> +	uint32_t tiled_x_bo;
> +	uint32_t tiled_y_bo;
> +
> +	f.width = 512;
> +	f.height = 512;
> +	f.pixel_format = DRM_FORMAT_XRGB8888;
>  	f.pitches[0] = 1024*4;
>  
> +	igt_fixture {
> +		tiled_x_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
> +			DRM_FORMAT_XRGB8888, LOCAL_I915_FORMAT_MOD_X_TILED,
> +			1024*4, NULL, NULL, NULL);
> +		igt_assert(tiled_x_bo);
> +
> +		tiled_y_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
> +			DRM_FORMAT_XRGB8888, LOCAL_I915_FORMAT_MOD_Y_TILED,
> +			1024*4, NULL, NULL, NULL);
> +		igt_assert(tiled_y_bo);
> +
> +		gem_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
> +			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
> +		igt_assert(gem_bo);
> +	}
> +
> +	f.pitches[0] = 1024*4;
>  	igt_subtest("basic-X-tiled") {
> -		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
> +		f.handles[0] = tiled_x_bo;
> +
>  		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == 0);
>  		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0);
>  		f.fb_id = 0;
>  	}
>  
>  	igt_subtest("framebuffer-vs-set-tiling") {
> +		f.handles[0] = gem_bo;
> +
>  		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
>  		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == 0);
>  		igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 512*4) == -EBUSY);
> @@ -176,20 +210,24 @@ static void pitch_tests(int fd)
>  
>  	f.pitches[0] = 512*4;
>  	igt_subtest("tile-pitch-mismatch") {
> -		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
> +		f.handles[0] = tiled_x_bo;
> +
>  		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
>  			   errno == EINVAL);
>  	}
>  
>  	f.pitches[0] = 1024*4;
>  	igt_subtest("basic-Y-tiled") {
> -		gem_set_tiling(fd, gem_bo, I915_TILING_Y, 1024*4);
> +		f.handles[0] = tiled_y_bo;
> +
>  		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
>  			   errno == EINVAL);
>  	}
>  
> -	igt_fixture
> -		gem_close(fd, gem_bo);
> +	igt_fixture {
> +		gem_close(fd, tiled_x_bo);
> +		gem_close(fd, tiled_y_bo);
> +	}
>  }
>  
>  static void size_tests(int fd)
> @@ -448,6 +486,8 @@ igt_main
>  
>  	addfb25_ytile(fd, gen);
>  
> +	tiling_tests(fd);
> +
>  	igt_fixture
>  		close(fd);
>  }
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://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

Re: [i-g-t PATCH v1 00/14] Get a few more tests to run on !i915

[Daniel Vetter]

On Wed, Mar 02, 2016 at 03:00:07PM +0100, Tomeu Vizoso wrote:
> Hi,
> 
> have restarted work on getting tests in IGT to run on drivers other than
> i915.
> 
> These changes make the modified tests pass in a Radxa Rock2 board by
> using dumb buffers as much as possible and having subtests skip if they
> require tiled BOs. The plan is for igt_create_bo_with_dimensions to be
> able to hide differences in the buffer creation API as much as possible.

lgtm to me overall. Some small comments on the library, but with those
addressed imo ready to go. Would be great if someone else (Daniel Stone?)
could check the final patches in detail before pushing, but upfront ack
from my side.

Cheers, Daniel

> 
> Thanks,
> 
> Tomeu
> 
> 
> Tomeu Vizoso (14):
>   lib: add igt_require_intel
>   lib: Have gem_set_tiling require intel
>   lib: Expose is_i915_device
>   lib: Have intel_get_drm_devid call igt_require_intel
>   lib: Call intel_get_drm_devid only from intel code
>   lib: Add wrapper for DRM_IOCTL_MODE_CREATE_DUMB
>   lib: Map dumb buffers
>   lib: Add igt_create_bo_with_dimensions
>   tests: Open any driver
>   kms_addfb_basic: call igt_create_bo_with_dimensions
>   kms_addfb_basic: move tiling functionality into each subtest
>   kms_addfb_basic: Split tiling_tests off
>   kms_addfb_basic: Move calls to gem_set_tiling to the subtests
>   kms_addfb_basic: Get intel gen from within subtest
> 
>  lib/drmtest.c           |   7 ++-
>  lib/drmtest.h           |   4 ++
>  lib/igt_fb.c            | 116 +++++++++++++++++++++++++++++++++-------------
>  lib/igt_fb.h            |   6 +++
>  lib/igt_kms.c           |   2 +-
>  lib/intel_chipset.c     |   2 +
>  lib/ioctl_wrappers.c    |  38 +++++++++++++++
>  lib/ioctl_wrappers.h    |   1 +
>  tests/drm_read.c        |  18 +------
>  tests/gem_exec_blt.c    |  18 +------
>  tests/kms_addfb_basic.c | 121 +++++++++++++++++++++++++++++++++++-------------
>  tests/kms_atomic.c      |   2 +-
>  tests/kms_setmode.c     |   2 +-
>  tests/kms_vblank.c      |   2 +-
>  14 files changed, 235 insertions(+), 104 deletions(-)
> 
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://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

Re: [i-g-t PATCH v1 12/14] kms_addfb_basic: Split tiling_tests off

[Tomeu Vizoso]

On 03/05/2016 01:33 PM, Daniel Vetter wrote:
> On Wed, Mar 02, 2016 at 03:00:19PM +0100, Tomeu Vizoso wrote:
>> Move tests requiring tiled BOs to the end so they don't cause unrelated
>> subtests to be skipped when testing drivers with only dumb buffer
>> support.
> 
> This uncovers a deficiency in igt skip infrastructure when you have
> disjoint sets of subtests with common requirements, e.g.
> 
> {
> 	igt_require_intel();
> 
> 	... intel tiling subtests;
> }
> 
> {
> 	igt_require_amd();
> 
> 	... amd tiling subtests;
> }
> 
> The current rule is that a skip outside of subtest skips _all_ remaining
> subtests. We might want to introduce some kind of igt_subtest_group block
> so that only that group is skipped.
> 
> Anyway, just a thought, nothing you'll have to tackle here ;-)

I think it's likely I will find more problematic cases as I get more
tests to run on !i915, so will think of it.

Thanks,

Tomeu

> -Daniel
> 
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>>  tests/kms_addfb_basic.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 45 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
>> index e61d2502d78b..6dfaecfc38a7 100644
>> --- a/tests/kms_addfb_basic.c
>> +++ b/tests/kms_addfb_basic.c
>> @@ -156,16 +156,50 @@ static void pitch_tests(int fd)
>>  		}
>>  	}
>>  
>> +	igt_fixture
>> +		gem_close(fd, gem_bo);
>> +}
>> +
>> +
>> +static void tiling_tests(int fd)
>> +{
>> +	struct drm_mode_fb_cmd2 f = {};
>> +	uint32_t tiled_x_bo;
>> +	uint32_t tiled_y_bo;
>> +
>> +	f.width = 512;
>> +	f.height = 512;
>> +	f.pixel_format = DRM_FORMAT_XRGB8888;
>>  	f.pitches[0] = 1024*4;
>>  
>> +	igt_fixture {
>> +		tiled_x_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
>> +			DRM_FORMAT_XRGB8888, LOCAL_I915_FORMAT_MOD_X_TILED,
>> +			1024*4, NULL, NULL, NULL);
>> +		igt_assert(tiled_x_bo);
>> +
>> +		tiled_y_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
>> +			DRM_FORMAT_XRGB8888, LOCAL_I915_FORMAT_MOD_Y_TILED,
>> +			1024*4, NULL, NULL, NULL);
>> +		igt_assert(tiled_y_bo);
>> +
>> +		gem_bo = igt_create_bo_with_dimensions(fd, 1024, 1024,
>> +			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
>> +		igt_assert(gem_bo);
>> +	}
>> +
>> +	f.pitches[0] = 1024*4;
>>  	igt_subtest("basic-X-tiled") {
>> -		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
>> +		f.handles[0] = tiled_x_bo;
>> +
>>  		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == 0);
>>  		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id) == 0);
>>  		f.fb_id = 0;
>>  	}
>>  
>>  	igt_subtest("framebuffer-vs-set-tiling") {
>> +		f.handles[0] = gem_bo;
>> +
>>  		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
>>  		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == 0);
>>  		igt_assert(__gem_set_tiling(fd, gem_bo, I915_TILING_X, 512*4) == -EBUSY);
>> @@ -176,20 +210,24 @@ static void pitch_tests(int fd)
>>  
>>  	f.pitches[0] = 512*4;
>>  	igt_subtest("tile-pitch-mismatch") {
>> -		gem_set_tiling(fd, gem_bo, I915_TILING_X, 1024*4);
>> +		f.handles[0] = tiled_x_bo;
>> +
>>  		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
>>  			   errno == EINVAL);
>>  	}
>>  
>>  	f.pitches[0] = 1024*4;
>>  	igt_subtest("basic-Y-tiled") {
>> -		gem_set_tiling(fd, gem_bo, I915_TILING_Y, 1024*4);
>> +		f.handles[0] = tiled_y_bo;
>> +
>>  		igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
>>  			   errno == EINVAL);
>>  	}
>>  
>> -	igt_fixture
>> -		gem_close(fd, gem_bo);
>> +	igt_fixture {
>> +		gem_close(fd, tiled_x_bo);
>> +		gem_close(fd, tiled_y_bo);
>> +	}
>>  }
>>  
>>  static void size_tests(int fd)
>> @@ -448,6 +486,8 @@ igt_main
>>  
>>  	addfb25_ytile(fd, gen);
>>  
>> +	tiling_tests(fd);
>> +
>>  	igt_fixture
>>  		close(fd);
>>  }
>> -- 
>> 2.5.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

Re: [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions

[Tomeu Vizoso]

On 03/05/2016 01:30 PM, Daniel Vetter wrote:
> On Wed, Mar 02, 2016 at 03:00:15PM +0100, Tomeu Vizoso wrote:
>> igt_create_bo_with_dimensions() is intended to abstract differences
>> between drivers in buffer object creation.
>>
>> The driver-specific ioctls will be called if the driver that is being
>> tested can satisfy the needs of the calling subtest, or it will be
>> skipped otherwise.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>>  lib/igt_fb.c | 83 ++++++++++++++++++++++++++++++++++++++++++------------------
>>  lib/igt_fb.h |  6 +++++
>>  2 files changed, 65 insertions(+), 24 deletions(-)
>>
>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>> index cd1605308308..0a3526f4e4ea 100644
>> --- a/lib/igt_fb.c
>> +++ b/lib/igt_fb.c
>> @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height, int bpp, uint64_t tiling,
>>  	*size_ret = size;
>>  }
>>  
>> +int igt_create_bo_with_dimensions(int fd, int width, int height,
> 
> Needs gtkdoc. Also this seems to overlap in functionality with the very
> recently added igt_calc_fb_size. Could we perhaps implement your new
> function here using igt_calc_fb_size plus igt_create_fb_with_size? Then
> this would just be a convenience wrapper.

Feel a bit lost here, sorry. igt_create_bo_with_dimensions is
implemented in terms of igt_calc_fb_size already, and
igt_create_fb_with_size doesn't exist.

I see that there's igt_create_fb_with_bo_size, but why a function
creating BOs would call another that creates FBs?

Thanks,

Tomeu

> I just want to make sure that we have a consistent interface to igt_fb.c
> functionality and avoid the need that driver-specific tiling formats need
> to overwrite bazillion of different places.
> -Daniel
> 
>> +				  uint32_t format, uint64_t modifier,
>> +				  unsigned stride, unsigned *size_ret,
>> +				  unsigned *stride_ret, bool *is_dumb)
>> +{
>> +	int bpp = igt_drm_format_to_bpp(format);
>> +	int bo;
>> +
>> +	igt_assert((modifier && stride) || (!modifier && !stride));
>> +
>> +	if (modifier) {
>> +		unsigned size, calculated_stride;
>> +
>> +		igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
>> +				 &calculated_stride);
>> +		if (stride == 0)
>> +			stride = calculated_stride;
>> +
>> +		if (is_dumb)
>> +			*is_dumb = false;
>> +
>> +		if (is_i915_device(fd)) {
>> +
>> +			bo = gem_create(fd, size);
>> +			gem_set_tiling(fd, bo, modifier, stride);
>> +
>> +			if (size_ret)
>> +				*size_ret = size;
>> +
>> +			if (stride_ret)
>> +				*stride_ret = stride;
>> +
>> +			return bo;
>> +		} else {
>> +			bool driver_has_tiling_support = false;
>> +
>> +			igt_require(driver_has_tiling_support);
>> +			return -EINVAL;
>> +		}
>> +	} else {
>> +		if (is_dumb)
>> +			*is_dumb = true;
>> +		return dumb_create(fd, width, height, bpp, stride_ret, size_ret);
>> +	}
>> +}
>> +
>>  /* helpers to create nice-looking framebuffers */
>> -static int create_bo_for_fb(int fd, int width, int height, int bpp,
>> +static int create_bo_for_fb(int fd, int width, int height, uint32_t format,
>>  			    uint64_t tiling, unsigned bo_size,
>>  			    unsigned bo_stride, uint32_t *gem_handle_ret,
>> -			    unsigned *size_ret, unsigned *stride_ret)
>> +			    unsigned *size_ret, unsigned *stride_ret,
>> +			    bool *is_dumb)
>>  {
>>  	uint32_t gem_handle;
>>  	int ret = 0;
>> -	unsigned size, stride;
>> -
>> -	igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride);
>> -	if (bo_size == 0)
>> -		bo_size = size;
>> -	if (bo_stride == 0)
>> -		bo_stride = stride;
>> -
>> -	gem_handle = gem_create(fd, bo_size);
>>  
>> -	if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
>> -		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
>> -				       bo_stride);
>> +	gem_handle = igt_create_bo_with_dimensions(fd, width, height, format,
>> +						   tiling, bo_stride, size_ret,
>> +						   stride_ret, is_dumb);
>>  
>> -	*stride_ret = bo_stride;
>> -	*size_ret = bo_size;
>>  	*gem_handle_ret = gem_handle;
>>  
>>  	return ret;
>> @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
>>  			   unsigned bo_stride)
>>  {
>>  	uint32_t fb_id;
>> -	int bpp;
>>  
>>  	memset(fb, 0, sizeof(*fb));
>>  
>> -	bpp = igt_drm_format_to_bpp(format);
>> -
>> -	igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], tiling=0x%"PRIx64", size=%d)\n",
>> -		  __func__, width, height, format, bpp, tiling, bo_size);
>> -	do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size,
>> +	igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64", size=%d)\n",
>> +		  __func__, width, height, format, tiling, bo_size);
>> +	do_or_die(create_bo_for_fb(fd, width, height, format, tiling, bo_size,
>>  				   bo_stride, &fb->gem_handle, &fb->size,
>> -				   &fb->stride));
>> +				   &fb->stride, &fb->is_dumb));
>>  
>>  	igt_debug("%s(handle=%d, pitch=%d)\n",
>>  		  __func__, fb->gem_handle, fb->stride);
>> @@ -860,6 +893,7 @@ struct fb_blit_upload {
>>  		uint32_t handle;
>>  		unsigned size, stride;
>>  		uint8_t *map;
>> +		bool is_dumb;
>>  	} linear;
>>  };
>>  
>> @@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct igt_fb *fb)
>>  				LOCAL_DRM_FORMAT_MOD_NONE, 0, 0,
>>  				&blit->linear.handle,
>>  				&blit->linear.size,
>> -				&blit->linear.stride);
>> +				&blit->linear.stride,
>> +				&blit->linear.is_dumb);
>>  
>>  	igt_assert(ret == 0);
>>  
>> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
>> index 4e6a76947c18..10e59deebe88 100644
>> --- a/lib/igt_fb.h
>> +++ b/lib/igt_fb.h
>> @@ -47,6 +47,7 @@ typedef struct _cairo cairo_t;
>>  struct igt_fb {
>>  	uint32_t fb_id;
>>  	uint32_t gem_handle;
>> +	bool is_dumb;
>>  	uint32_t drm_format;
>>  	int width;
>>  	int height;
>> @@ -97,6 +98,11 @@ unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode,
>>  				  uint32_t format, uint64_t tiling);
>>  void igt_remove_fb(int fd, struct igt_fb *fb);
>>  
>> +int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t format,
>> +				  uint64_t modifier, unsigned stride,
>> +				  unsigned *stride_out, unsigned *size_out,
>> +				  bool *is_dumb);
>> +
>>  /* cairo-based painting */
>>  cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb);
>>  void igt_paint_color(cairo_t *cr, int x, int y, int w, int h,
>> -- 
>> 2.5.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

Re: [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions

[Ville Syrjälä]

On Sat, Mar 05, 2016 at 01:30:34PM +0100, Daniel Vetter wrote:
> On Wed, Mar 02, 2016 at 03:00:15PM +0100, Tomeu Vizoso wrote:
> > igt_create_bo_with_dimensions() is intended to abstract differences
> > between drivers in buffer object creation.
> > 
> > The driver-specific ioctls will be called if the driver that is being
> > tested can satisfy the needs of the calling subtest, or it will be
> > skipped otherwise.
> > 
> > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > ---
> > 
> >  lib/igt_fb.c | 83 ++++++++++++++++++++++++++++++++++++++++++------------------
> >  lib/igt_fb.h |  6 +++++
> >  2 files changed, 65 insertions(+), 24 deletions(-)
> > 
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > index cd1605308308..0a3526f4e4ea 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height, int bpp, uint64_t tiling,
> >  	*size_ret = size;
> >  }
> >  
> > +int igt_create_bo_with_dimensions(int fd, int width, int height,
> 
> Needs gtkdoc. Also this seems to overlap in functionality with the very
> recently added igt_calc_fb_size. Could we perhaps implement your new
> function here using igt_calc_fb_size plus igt_create_fb_with_size? Then
> this would just be a convenience wrapper.

BTW I also have some improvements to igt_calc_fb_size() lined up to deal
with offsets[] and whatnot. It also fixes it to account for the passed
in stride when computing the required fb size.

git://github.com/vsyrjala/linux.git fb_offsets

> 
> I just want to make sure that we have a consistent interface to igt_fb.c
> functionality and avoid the need that driver-specific tiling formats need
> to overwrite bazillion of different places.
> -Daniel
> 
> > +				  uint32_t format, uint64_t modifier,
> > +				  unsigned stride, unsigned *size_ret,
> > +				  unsigned *stride_ret, bool *is_dumb)
> > +{
> > +	int bpp = igt_drm_format_to_bpp(format);
> > +	int bo;
> > +
> > +	igt_assert((modifier && stride) || (!modifier && !stride));
> > +
> > +	if (modifier) {
> > +		unsigned size, calculated_stride;
> > +
> > +		igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
> > +				 &calculated_stride);
> > +		if (stride == 0)
> > +			stride = calculated_stride;
> > +
> > +		if (is_dumb)
> > +			*is_dumb = false;
> > +
> > +		if (is_i915_device(fd)) {
> > +
> > +			bo = gem_create(fd, size);
> > +			gem_set_tiling(fd, bo, modifier, stride);
> > +
> > +			if (size_ret)
> > +				*size_ret = size;
> > +
> > +			if (stride_ret)
> > +				*stride_ret = stride;
> > +
> > +			return bo;
> > +		} else {
> > +			bool driver_has_tiling_support = false;
> > +
> > +			igt_require(driver_has_tiling_support);
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		if (is_dumb)
> > +			*is_dumb = true;
> > +		return dumb_create(fd, width, height, bpp, stride_ret, size_ret);
> > +	}
> > +}
> > +
> >  /* helpers to create nice-looking framebuffers */
> > -static int create_bo_for_fb(int fd, int width, int height, int bpp,
> > +static int create_bo_for_fb(int fd, int width, int height, uint32_t format,
> >  			    uint64_t tiling, unsigned bo_size,
> >  			    unsigned bo_stride, uint32_t *gem_handle_ret,
> > -			    unsigned *size_ret, unsigned *stride_ret)
> > +			    unsigned *size_ret, unsigned *stride_ret,
> > +			    bool *is_dumb)
> >  {
> >  	uint32_t gem_handle;
> >  	int ret = 0;
> > -	unsigned size, stride;
> > -
> > -	igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride);
> > -	if (bo_size == 0)
> > -		bo_size = size;
> > -	if (bo_stride == 0)
> > -		bo_stride = stride;
> > -
> > -	gem_handle = gem_create(fd, bo_size);
> >  
> > -	if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
> > -		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
> > -				       bo_stride);
> > +	gem_handle = igt_create_bo_with_dimensions(fd, width, height, format,
> > +						   tiling, bo_stride, size_ret,
> > +						   stride_ret, is_dumb);
> >  
> > -	*stride_ret = bo_stride;
> > -	*size_ret = bo_size;
> >  	*gem_handle_ret = gem_handle;
> >  
> >  	return ret;
> > @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
> >  			   unsigned bo_stride)
> >  {
> >  	uint32_t fb_id;
> > -	int bpp;
> >  
> >  	memset(fb, 0, sizeof(*fb));
> >  
> > -	bpp = igt_drm_format_to_bpp(format);
> > -
> > -	igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], tiling=0x%"PRIx64", size=%d)\n",
> > -		  __func__, width, height, format, bpp, tiling, bo_size);
> > -	do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size,
> > +	igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64", size=%d)\n",
> > +		  __func__, width, height, format, tiling, bo_size);
> > +	do_or_die(create_bo_for_fb(fd, width, height, format, tiling, bo_size,
> >  				   bo_stride, &fb->gem_handle, &fb->size,
> > -				   &fb->stride));
> > +				   &fb->stride, &fb->is_dumb));
> >  
> >  	igt_debug("%s(handle=%d, pitch=%d)\n",
> >  		  __func__, fb->gem_handle, fb->stride);
> > @@ -860,6 +893,7 @@ struct fb_blit_upload {
> >  		uint32_t handle;
> >  		unsigned size, stride;
> >  		uint8_t *map;
> > +		bool is_dumb;
> >  	} linear;
> >  };
> >  
> > @@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct igt_fb *fb)
> >  				LOCAL_DRM_FORMAT_MOD_NONE, 0, 0,
> >  				&blit->linear.handle,
> >  				&blit->linear.size,
> > -				&blit->linear.stride);
> > +				&blit->linear.stride,
> > +				&blit->linear.is_dumb);
> >  
> >  	igt_assert(ret == 0);
> >  
> > diff --git a/lib/igt_fb.h b/lib/igt_fb.h
> > index 4e6a76947c18..10e59deebe88 100644
> > --- a/lib/igt_fb.h
> > +++ b/lib/igt_fb.h
> > @@ -47,6 +47,7 @@ typedef struct _cairo cairo_t;
> >  struct igt_fb {
> >  	uint32_t fb_id;
> >  	uint32_t gem_handle;
> > +	bool is_dumb;
> >  	uint32_t drm_format;
> >  	int width;
> >  	int height;
> > @@ -97,6 +98,11 @@ unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode,
> >  				  uint32_t format, uint64_t tiling);
> >  void igt_remove_fb(int fd, struct igt_fb *fb);
> >  
> > +int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t format,
> > +				  uint64_t modifier, unsigned stride,
> > +				  unsigned *stride_out, unsigned *size_out,
> > +				  bool *is_dumb);
> > +
> >  /* cairo-based painting */
> >  cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb);
> >  void igt_paint_color(cairo_t *cr, int x, int y, int w, int h,
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions

[Daniel Stone]

Hey,

On 5 March 2016 at 12:30, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Mar 02, 2016 at 03:00:15PM +0100, Tomeu Vizoso wrote:
>> +int igt_create_bo_with_dimensions(int fd, int width, int height,
>
> Needs gtkdoc. Also this seems to overlap in functionality with the very
> recently added igt_calc_fb_size. Could we perhaps implement your new
> function here using igt_calc_fb_size plus igt_create_fb_with_size? Then
> this would just be a convenience wrapper.

It, er, already is implemented in terms of igt_calc_fb_size? When it
can be, at least.

Dumb buffers calculate stride and size for you, so they _cannot_ be
implemented in those terms. So the idea behind this is that you have
two API entrypoints: one where you only care about having a buffer
with particular dimensions and format (most tests, can use dumb), and
one where you want to very specifically control allocation parameters
(e.g. invalid-stride/size-too-small tests, cannot use dumb).

> I just want to make sure that we have a consistent interface to igt_fb.c
> functionality and avoid the need that driver-specific tiling formats need
> to overwrite bazillion of different places.

That makes sense, and is indeed the intention of this series. It's not
complete or the entire way there yet, but Tomeu wanted to get this out
there as a pretty good starting point to build on.

>> +     igt_assert((modifier && stride) || (!modifier && !stride));

As an aside, I think this is wrong. !modifier && stride can be valid,
though it shouldn't be necessary most of the time. :)

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

Re: [i-g-t PATCH v1 00/14] Get a few more tests to run on !i915

[Daniel Stone]

Hi,

On 5 March 2016 at 12:34, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Mar 02, 2016 at 03:00:07PM +0100, Tomeu Vizoso wrote:
>> have restarted work on getting tests in IGT to run on drivers other than
>> i915.
>>
>> These changes make the modified tests pass in a Radxa Rock2 board by
>> using dumb buffers as much as possible and having subtests skip if they
>> require tiled BOs. The plan is for igt_create_bo_with_dimensions to be
>> able to hide differences in the buffer creation API as much as possible.
>
> lgtm to me overall. Some small comments on the library, but with those
> addressed imo ready to go. Would be great if someone else (Daniel Stone?)
> could check the final patches in detail before pushing, but upfront ack
> from my side.

I went through and reviewed the changes here, and have been happy with
them after some very minor bikeshedding. The updated versions also
addressed all the other comments which came up on the list. So I've
pushed them with my review, passing on Intel, Rockchip and VC4. Latter
updates to get the flip/vblank/etc tests running have also exposed
some bugs in Rockchip and VC4 both. :)

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

Re: [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions

[Tvrtko Ursulin]

Hi,


On 02/03/16 14:00, Tomeu Vizoso wrote:
> igt_create_bo_with_dimensions() is intended to abstract differences
> between drivers in buffer object creation.
>
> The driver-specific ioctls will be called if the driver that is being
> tested can satisfy the needs of the calling subtest, or it will be
> skipped otherwise.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
>  lib/igt_fb.c | 83 ++++++++++++++++++++++++++++++++++++++++++------------------
>  lib/igt_fb.h |  6 +++++
>  2 files changed, 65 insertions(+), 24 deletions(-)
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index cd1605308308..0a3526f4e4ea 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height, int bpp, uint64_t tiling,
>  	*size_ret = size;
>  }
>
> +int igt_create_bo_with_dimensions(int fd, int width, int height,
> +				  uint32_t format, uint64_t modifier,
> +				  unsigned stride, unsigned *size_ret,
> +				  unsigned *stride_ret, bool *is_dumb)
> +{
> +	int bpp = igt_drm_format_to_bpp(format);
> +	int bo;
> +
> +	igt_assert((modifier && stride) || (!modifier && !stride));
> +
> +	if (modifier) {
> +		unsigned size, calculated_stride;
> +
> +		igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
> +				 &calculated_stride);
> +		if (stride == 0)
> +			stride = calculated_stride;
> +
> +		if (is_dumb)
> +			*is_dumb = false;
> +
> +		if (is_i915_device(fd)) {
> +
> +			bo = gem_create(fd, size);
> +			gem_set_tiling(fd, bo, modifier, stride);

This is broken, gem_set_tiling does not take a fb modifier but an object 
tiling mode.

You can demonstrate the failure if you got a Skylake system with:

tests/kms_flip_tiling --r flip-changes-tiling-Yf

I would like to be able to tell you that all subtests there should pass, 
since they have been at some point, but I have a feeling that there are 
other breakages affecting it these days. :(

Regards,

Tvrtko


> +
> +			if (size_ret)
> +				*size_ret = size;
> +
> +			if (stride_ret)
> +				*stride_ret = stride;
> +
> +			return bo;
> +		} else {
> +			bool driver_has_tiling_support = false;
> +
> +			igt_require(driver_has_tiling_support);
> +			return -EINVAL;
> +		}
> +	} else {
> +		if (is_dumb)
> +			*is_dumb = true;
> +		return dumb_create(fd, width, height, bpp, stride_ret, size_ret);
> +	}
> +}
> +
>  /* helpers to create nice-looking framebuffers */
> -static int create_bo_for_fb(int fd, int width, int height, int bpp,
> +static int create_bo_for_fb(int fd, int width, int height, uint32_t format,
>  			    uint64_t tiling, unsigned bo_size,
>  			    unsigned bo_stride, uint32_t *gem_handle_ret,
> -			    unsigned *size_ret, unsigned *stride_ret)
> +			    unsigned *size_ret, unsigned *stride_ret,
> +			    bool *is_dumb)
>  {
>  	uint32_t gem_handle;
>  	int ret = 0;
> -	unsigned size, stride;
> -
> -	igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride);
> -	if (bo_size == 0)
> -		bo_size = size;
> -	if (bo_stride == 0)
> -		bo_stride = stride;
> -
> -	gem_handle = gem_create(fd, bo_size);
>
> -	if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
> -		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
> -				       bo_stride);
> +	gem_handle = igt_create_bo_with_dimensions(fd, width, height, format,
> +						   tiling, bo_stride, size_ret,
> +						   stride_ret, is_dumb);
>
> -	*stride_ret = bo_stride;
> -	*size_ret = bo_size;
>  	*gem_handle_ret = gem_handle;
>
>  	return ret;
> @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
>  			   unsigned bo_stride)
>  {
>  	uint32_t fb_id;
> -	int bpp;
>
>  	memset(fb, 0, sizeof(*fb));
>
> -	bpp = igt_drm_format_to_bpp(format);
> -
> -	igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], tiling=0x%"PRIx64", size=%d)\n",
> -		  __func__, width, height, format, bpp, tiling, bo_size);
> -	do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size,
> +	igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64", size=%d)\n",
> +		  __func__, width, height, format, tiling, bo_size);
> +	do_or_die(create_bo_for_fb(fd, width, height, format, tiling, bo_size,
>  				   bo_stride, &fb->gem_handle, &fb->size,
> -				   &fb->stride));
> +				   &fb->stride, &fb->is_dumb));
>
>  	igt_debug("%s(handle=%d, pitch=%d)\n",
>  		  __func__, fb->gem_handle, fb->stride);
> @@ -860,6 +893,7 @@ struct fb_blit_upload {
>  		uint32_t handle;
>  		unsigned size, stride;
>  		uint8_t *map;
> +		bool is_dumb;
>  	} linear;
>  };
>
> @@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct igt_fb *fb)
>  				LOCAL_DRM_FORMAT_MOD_NONE, 0, 0,
>  				&blit->linear.handle,
>  				&blit->linear.size,
> -				&blit->linear.stride);
> +				&blit->linear.stride,
> +				&blit->linear.is_dumb);
>
>  	igt_assert(ret == 0);
>
> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
> index 4e6a76947c18..10e59deebe88 100644
> --- a/lib/igt_fb.h
> +++ b/lib/igt_fb.h
> @@ -47,6 +47,7 @@ typedef struct _cairo cairo_t;
>  struct igt_fb {
>  	uint32_t fb_id;
>  	uint32_t gem_handle;
> +	bool is_dumb;
>  	uint32_t drm_format;
>  	int width;
>  	int height;
> @@ -97,6 +98,11 @@ unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode,
>  				  uint32_t format, uint64_t tiling);
>  void igt_remove_fb(int fd, struct igt_fb *fb);
>
> +int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t format,
> +				  uint64_t modifier, unsigned stride,
> +				  unsigned *stride_out, unsigned *size_out,
> +				  bool *is_dumb);
> +
>  /* cairo-based painting */
>  cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb);
>  void igt_paint_color(cairo_t *cr, int x, int y, int w, int h,
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions

[Tomeu Vizoso]

On 1 November 2016 at 16:44, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>
> Hi,
>
>
>
> On 02/03/16 14:00, Tomeu Vizoso wrote:
>>
>> igt_create_bo_with_dimensions() is intended to abstract differences
>> between drivers in buffer object creation.
>>
>> The driver-specific ioctls will be called if the driver that is being
>> tested can satisfy the needs of the calling subtest, or it will be
>> skipped otherwise.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>>  lib/igt_fb.c | 83
>> ++++++++++++++++++++++++++++++++++++++++++------------------
>>  lib/igt_fb.h |  6 +++++
>>  2 files changed, 65 insertions(+), 24 deletions(-)
>>
>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>> index cd1605308308..0a3526f4e4ea 100644
>> --- a/lib/igt_fb.c
>> +++ b/lib/igt_fb.c
>> @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height,
>> int bpp, uint64_t tiling,
>>         *size_ret = size;
>>  }
>>
>> +int igt_create_bo_with_dimensions(int fd, int width, int height,
>> +                                 uint32_t format, uint64_t modifier,
>> +                                 unsigned stride, unsigned *size_ret,
>> +                                 unsigned *stride_ret, bool *is_dumb)
>> +{
>> +       int bpp = igt_drm_format_to_bpp(format);
>> +       int bo;
>> +
>> +       igt_assert((modifier && stride) || (!modifier && !stride));
>> +
>> +       if (modifier) {
>> +               unsigned size, calculated_stride;
>> +
>> +               igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
>> +                                &calculated_stride);
>> +               if (stride == 0)
>> +                       stride = calculated_stride;
>> +
>> +               if (is_dumb)
>> +                       *is_dumb = false;
>> +
>> +               if (is_i915_device(fd)) {
>> +
>> +                       bo = gem_create(fd, size);
>> +                       gem_set_tiling(fd, bo, modifier, stride);
>
>
> This is broken, gem_set_tiling does not take a fb modifier but an object
> tiling mode.
>
> You can demonstrate the failure if you got a Skylake system with:
>
> tests/kms_flip_tiling --r flip-changes-tiling-Yf

Hi,

that subtest fails occasionally here due to CRC issues, but the one
that fails due to the tiling constant passed to gem_set_tiling is
flip-Yf-tiled.

Have fixed it by converting the modifier to a tiling mode, but I also
needed to make sure that we don't pass to the kernel the Yf or Ys
constants as the kernel doesn't know about those.

Both fixes have been pushed already.

> I would like to be able to tell you that all subtests there should pass,
> since they have been at some point, but I have a feeling that there are
> other breakages affecting it these days. :(

Yeah, at least I can say that they are passing now in this particular
machine (i3-6100U).

To make such issues less likely in the future, I have sent a patch
that makes clear when a variable contains a fb modifier constant, and
when a tiling mode. Haven't pushed it because I'm not 100% sure it's
worth it, so I would appreciate any opinions.

Thanks,

Tomeu

> Regards,
>
> Tvrtko
>
>
>
>> +
>> +                       if (size_ret)
>> +                               *size_ret = size;
>> +
>> +                       if (stride_ret)
>> +                               *stride_ret = stride;
>> +
>> +                       return bo;
>> +               } else {
>> +                       bool driver_has_tiling_support = false;
>> +
>> +                       igt_require(driver_has_tiling_support);
>> +                       return -EINVAL;
>> +               }
>> +       } else {
>> +               if (is_dumb)
>> +                       *is_dumb = true;
>> +               return dumb_create(fd, width, height, bpp, stride_ret,
>> size_ret);
>> +       }
>> +}
>> +
>>  /* helpers to create nice-looking framebuffers */
>> -static int create_bo_for_fb(int fd, int width, int height, int bpp,
>> +static int create_bo_for_fb(int fd, int width, int height, uint32_t
>> format,
>>                             uint64_t tiling, unsigned bo_size,
>>                             unsigned bo_stride, uint32_t *gem_handle_ret,
>> -                           unsigned *size_ret, unsigned *stride_ret)
>> +                           unsigned *size_ret, unsigned *stride_ret,
>> +                           bool *is_dumb)
>>  {
>>         uint32_t gem_handle;
>>         int ret = 0;
>> -       unsigned size, stride;
>> -
>> -       igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride);
>> -       if (bo_size == 0)
>> -               bo_size = size;
>> -       if (bo_stride == 0)
>> -               bo_stride = stride;
>> -
>> -       gem_handle = gem_create(fd, bo_size);
>>
>> -       if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
>> -               ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
>> -                                      bo_stride);
>> +       gem_handle = igt_create_bo_with_dimensions(fd, width, height,
>> format,
>> +                                                  tiling, bo_stride,
>> size_ret,
>> +                                                  stride_ret, is_dumb);
>>
>> -       *stride_ret = bo_stride;
>> -       *size_ret = bo_size;
>>         *gem_handle_ret = gem_handle;
>>
>>         return ret;
>> @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int
>> height,
>>                            unsigned bo_stride)
>>  {
>>         uint32_t fb_id;
>> -       int bpp;
>>
>>         memset(fb, 0, sizeof(*fb));
>>
>> -       bpp = igt_drm_format_to_bpp(format);
>> -
>> -       igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d],
>> tiling=0x%"PRIx64", size=%d)\n",
>> -                 __func__, width, height, format, bpp, tiling, bo_size);
>> -       do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling,
>> bo_size,
>> +       igt_debug("%s(width=%d, height=%d, format=0x%x,
>> tiling=0x%"PRIx64", size=%d)\n",
>> +                 __func__, width, height, format, tiling, bo_size);
>> +       do_or_die(create_bo_for_fb(fd, width, height, format, tiling,
>> bo_size,
>>                                    bo_stride, &fb->gem_handle, &fb->size,
>> -                                  &fb->stride));
>> +                                  &fb->stride, &fb->is_dumb));
>>
>>         igt_debug("%s(handle=%d, pitch=%d)\n",
>>                   __func__, fb->gem_handle, fb->stride);
>> @@ -860,6 +893,7 @@ struct fb_blit_upload {
>>                 uint32_t handle;
>>                 unsigned size, stride;
>>                 uint8_t *map;
>> +               bool is_dumb;
>>         } linear;
>>  };
>>
>> @@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct
>> igt_fb *fb)
>>                                 LOCAL_DRM_FORMAT_MOD_NONE, 0, 0,
>>                                 &blit->linear.handle,
>>                                 &blit->linear.size,
>> -                               &blit->linear.stride);
>> +                               &blit->linear.stride,
>> +                               &blit->linear.is_dumb);
>>
>>         igt_assert(ret == 0);
>>
>> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
>> index 4e6a76947c18..10e59deebe88 100644
>> --- a/lib/igt_fb.h
>> +++ b/lib/igt_fb.h
>> @@ -47,6 +47,7 @@ typedef struct _cairo cairo_t;
>>  struct igt_fb {
>>         uint32_t fb_id;
>>         uint32_t gem_handle;
>> +       bool is_dumb;
>>         uint32_t drm_format;
>>         int width;
>>         int height;
>> @@ -97,6 +98,11 @@ unsigned int igt_create_stereo_fb(int drm_fd,
>> drmModeModeInfo *mode,
>>                                   uint32_t format, uint64_t tiling);
>>  void igt_remove_fb(int fd, struct igt_fb *fb);
>>
>> +int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t
>> format,
>> +                                 uint64_t modifier, unsigned stride,
>> +                                 unsigned *stride_out, unsigned
>> *size_out,
>> +                                 bool *is_dumb);
>> +
>>  /* cairo-based painting */
>>  cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb);
>>  void igt_paint_color(cairo_t *cr, int x, int y, int w, int h,
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions

[Tvrtko Ursulin]

On 10/11/2016 13:17, Tomeu Vizoso wrote:
> On 1 November 2016 at 16:44, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>
>> Hi,
>>
>>
>>
>> On 02/03/16 14:00, Tomeu Vizoso wrote:
>>>
>>> igt_create_bo_with_dimensions() is intended to abstract differences
>>> between drivers in buffer object creation.
>>>
>>> The driver-specific ioctls will be called if the driver that is being
>>> tested can satisfy the needs of the calling subtest, or it will be
>>> skipped otherwise.
>>>
>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>> ---
>>>
>>>  lib/igt_fb.c | 83
>>> ++++++++++++++++++++++++++++++++++++++++++------------------
>>>  lib/igt_fb.h |  6 +++++
>>>  2 files changed, 65 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>>> index cd1605308308..0a3526f4e4ea 100644
>>> --- a/lib/igt_fb.c
>>> +++ b/lib/igt_fb.c
>>> @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height,
>>> int bpp, uint64_t tiling,
>>>         *size_ret = size;
>>>  }
>>>
>>> +int igt_create_bo_with_dimensions(int fd, int width, int height,
>>> +                                 uint32_t format, uint64_t modifier,
>>> +                                 unsigned stride, unsigned *size_ret,
>>> +                                 unsigned *stride_ret, bool *is_dumb)
>>> +{
>>> +       int bpp = igt_drm_format_to_bpp(format);
>>> +       int bo;
>>> +
>>> +       igt_assert((modifier && stride) || (!modifier && !stride));
>>> +
>>> +       if (modifier) {
>>> +               unsigned size, calculated_stride;
>>> +
>>> +               igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
>>> +                                &calculated_stride);
>>> +               if (stride == 0)
>>> +                       stride = calculated_stride;
>>> +
>>> +               if (is_dumb)
>>> +                       *is_dumb = false;
>>> +
>>> +               if (is_i915_device(fd)) {
>>> +
>>> +                       bo = gem_create(fd, size);
>>> +                       gem_set_tiling(fd, bo, modifier, stride);
>>
>>
>> This is broken, gem_set_tiling does not take a fb modifier but an object
>> tiling mode.
>>
>> You can demonstrate the failure if you got a Skylake system with:
>>
>> tests/kms_flip_tiling --r flip-changes-tiling-Yf
>
> Hi,
>
> that subtest fails occasionally here due to CRC issues, but the one
> that fails due to the tiling constant passed to gem_set_tiling is
> flip-Yf-tiled.
>
> Have fixed it by converting the modifier to a tiling mode, but I also
> needed to make sure that we don't pass to the kernel the Yf or Ys
> constants as the kernel doesn't know about those.
>
> Both fixes have been pushed already.

With the two patches you pushed flip-changes-tiling-Yf is still broken 
due the tiling mode mismatch, not the CRC.

Perhaps you tested with all three patches you posted today?

>> I would like to be able to tell you that all subtests there should pass,
>> since they have been at some point, but I have a feeling that there are
>> other breakages affecting it these days. :(
>
> Yeah, at least I can say that they are passing now in this particular
> machine (i3-6100U).
>
> To make such issues less likely in the future, I have sent a patch
> that makes clear when a variable contains a fb modifier constant, and
> when a tiling mode. Haven't pushed it because I'm not 100% sure it's
> worth it, so I would appreciate any opinions.

I will look at that one later.

Regards,

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

Re: [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions

[Tomeu Vizoso]

On 11/10/2016 05:23 PM, Tvrtko Ursulin wrote:
> 
> On 10/11/2016 13:17, Tomeu Vizoso wrote:
>> On 1 November 2016 at 16:44, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>>
>>> Hi,
>>>
>>>
>>>
>>> On 02/03/16 14:00, Tomeu Vizoso wrote:
>>>>
>>>> igt_create_bo_with_dimensions() is intended to abstract differences
>>>> between drivers in buffer object creation.
>>>>
>>>> The driver-specific ioctls will be called if the driver that is being
>>>> tested can satisfy the needs of the calling subtest, or it will be
>>>> skipped otherwise.
>>>>
>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>> ---
>>>>
>>>>  lib/igt_fb.c | 83
>>>> ++++++++++++++++++++++++++++++++++++++++++------------------
>>>>  lib/igt_fb.h |  6 +++++
>>>>  2 files changed, 65 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>>>> index cd1605308308..0a3526f4e4ea 100644
>>>> --- a/lib/igt_fb.c
>>>> +++ b/lib/igt_fb.c
>>>> @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height,
>>>> int bpp, uint64_t tiling,
>>>>         *size_ret = size;
>>>>  }
>>>>
>>>> +int igt_create_bo_with_dimensions(int fd, int width, int height,
>>>> +                                 uint32_t format, uint64_t modifier,
>>>> +                                 unsigned stride, unsigned *size_ret,
>>>> +                                 unsigned *stride_ret, bool *is_dumb)
>>>> +{
>>>> +       int bpp = igt_drm_format_to_bpp(format);
>>>> +       int bo;
>>>> +
>>>> +       igt_assert((modifier && stride) || (!modifier && !stride));
>>>> +
>>>> +       if (modifier) {
>>>> +               unsigned size, calculated_stride;
>>>> +
>>>> +               igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
>>>> +                                &calculated_stride);
>>>> +               if (stride == 0)
>>>> +                       stride = calculated_stride;
>>>> +
>>>> +               if (is_dumb)
>>>> +                       *is_dumb = false;
>>>> +
>>>> +               if (is_i915_device(fd)) {
>>>> +
>>>> +                       bo = gem_create(fd, size);
>>>> +                       gem_set_tiling(fd, bo, modifier, stride);
>>>
>>>
>>> This is broken, gem_set_tiling does not take a fb modifier but an object
>>> tiling mode.
>>>
>>> You can demonstrate the failure if you got a Skylake system with:
>>>
>>> tests/kms_flip_tiling --r flip-changes-tiling-Yf
>>
>> Hi,
>>
>> that subtest fails occasionally here due to CRC issues, but the one
>> that fails due to the tiling constant passed to gem_set_tiling is
>> flip-Yf-tiled.
>>
>> Have fixed it by converting the modifier to a tiling mode, but I also
>> needed to make sure that we don't pass to the kernel the Yf or Ys
>> constants as the kernel doesn't know about those.
>>
>> Both fixes have been pushed already.
> 
> With the two patches you pushed flip-changes-tiling-Yf is still broken 
> due the tiling mode mismatch, not the CRC.
> 
> Perhaps you tested with all three patches you posted today?

Just before pushing I tested with this:

IGT-Version: 1.16-g050c00d (x86_64) (Linux: 4.5.5-300.fc24.x86_64 x86_64)

So I can only think of a difference in the hw causing a different
codepath to execute (it's a i3-6100U), or a difference in the kernel.

This is a remote box and I don't have yet all the infrastructure in
place so I could monitor the boot, nor power cycle it, so I cannot test
with a newer kernel yet.

If you can confirm that we should be passing I915_TILING_NONE to
set_tiling when the FB has I915_FORMAT_MOD_Yf_TILED, I can fix that, but
I would also like to understand why the test is failing for you.

Regards,

Tomeu

>>> I would like to be able to tell you that all subtests there should pass,
>>> since they have been at some point, but I have a feeling that there are
>>> other breakages affecting it these days. :(
>>
>> Yeah, at least I can say that they are passing now in this particular
>> machine (i3-6100U).
>>
>> To make such issues less likely in the future, I have sent a patch
>> that makes clear when a variable contains a fb modifier constant, and
>> when a tiling mode. Haven't pushed it because I'm not 100% sure it's
>> worth it, so I would appreciate any opinions.
> 
> I will look at that one later.
> 
> Regards,
> 
> Tvrtko
> 

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

Re: [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions

[Tvrtko Ursulin]

On 11/11/2016 11:23, Tomeu Vizoso wrote:
> On 11/10/2016 05:23 PM, Tvrtko Ursulin wrote:
>>
>> On 10/11/2016 13:17, Tomeu Vizoso wrote:
>>> On 1 November 2016 at 16:44, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>>>
>>>> Hi,
>>>>
>>>>
>>>>
>>>> On 02/03/16 14:00, Tomeu Vizoso wrote:
>>>>>
>>>>> igt_create_bo_with_dimensions() is intended to abstract differences
>>>>> between drivers in buffer object creation.
>>>>>
>>>>> The driver-specific ioctls will be called if the driver that is being
>>>>> tested can satisfy the needs of the calling subtest, or it will be
>>>>> skipped otherwise.
>>>>>
>>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>>> ---
>>>>>
>>>>>  lib/igt_fb.c | 83
>>>>> ++++++++++++++++++++++++++++++++++++++++++------------------
>>>>>  lib/igt_fb.h |  6 +++++
>>>>>  2 files changed, 65 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>>>>> index cd1605308308..0a3526f4e4ea 100644
>>>>> --- a/lib/igt_fb.c
>>>>> +++ b/lib/igt_fb.c
>>>>> @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height,
>>>>> int bpp, uint64_t tiling,
>>>>>         *size_ret = size;
>>>>>  }
>>>>>
>>>>> +int igt_create_bo_with_dimensions(int fd, int width, int height,
>>>>> +                                 uint32_t format, uint64_t modifier,
>>>>> +                                 unsigned stride, unsigned *size_ret,
>>>>> +                                 unsigned *stride_ret, bool *is_dumb)
>>>>> +{
>>>>> +       int bpp = igt_drm_format_to_bpp(format);
>>>>> +       int bo;
>>>>> +
>>>>> +       igt_assert((modifier && stride) || (!modifier && !stride));
>>>>> +
>>>>> +       if (modifier) {
>>>>> +               unsigned size, calculated_stride;
>>>>> +
>>>>> +               igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
>>>>> +                                &calculated_stride);
>>>>> +               if (stride == 0)
>>>>> +                       stride = calculated_stride;
>>>>> +
>>>>> +               if (is_dumb)
>>>>> +                       *is_dumb = false;
>>>>> +
>>>>> +               if (is_i915_device(fd)) {
>>>>> +
>>>>> +                       bo = gem_create(fd, size);
>>>>> +                       gem_set_tiling(fd, bo, modifier, stride);
>>>>
>>>>
>>>> This is broken, gem_set_tiling does not take a fb modifier but an object
>>>> tiling mode.
>>>>
>>>> You can demonstrate the failure if you got a Skylake system with:
>>>>
>>>> tests/kms_flip_tiling --r flip-changes-tiling-Yf
>>>
>>> Hi,
>>>
>>> that subtest fails occasionally here due to CRC issues, but the one
>>> that fails due to the tiling constant passed to gem_set_tiling is
>>> flip-Yf-tiled.
>>>
>>> Have fixed it by converting the modifier to a tiling mode, but I also
>>> needed to make sure that we don't pass to the kernel the Yf or Ys
>>> constants as the kernel doesn't know about those.
>>>
>>> Both fixes have been pushed already.
>>
>> With the two patches you pushed flip-changes-tiling-Yf is still broken
>> due the tiling mode mismatch, not the CRC.
>>
>> Perhaps you tested with all three patches you posted today?
>
> Just before pushing I tested with this:
>
> IGT-Version: 1.16-g050c00d (x86_64) (Linux: 4.5.5-300.fc24.x86_64 x86_64)
>
> So I can only think of a difference in the hw causing a different
> codepath to execute (it's a i3-6100U), or a difference in the kernel.
>
> This is a remote box and I don't have yet all the infrastructure in
> place so I could monitor the boot, nor power cycle it, so I cannot test
> with a newer kernel yet.
>
> If you can confirm that we should be passing I915_TILING_NONE to
> set_tiling when the FB has I915_FORMAT_MOD_Yf_TILED, I can fix that, but
> I would also like to understand why the test is failing for you.

It should definitely be I915_TILING_NONE. If you look at the i915 code, 
intel_display.c/intel_framebuffer_init will reject attempts to add a 
framebuffer where object tiling does not match the framebuffer modifier. 
And the intel_fb_modifier_to_tiling helper translates the Yf/Ys 
modifiers to NONE tiling.

So I have no idea how it could have possibly worked for you. :)

Regards,

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

Re: [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions

[Tomeu Vizoso]

On 11 November 2016 at 12:33, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
> On 11/11/2016 11:23, Tomeu Vizoso wrote:
>>
>> On 11/10/2016 05:23 PM, Tvrtko Ursulin wrote:
>>>
>>>
>>> On 10/11/2016 13:17, Tomeu Vizoso wrote:
>>>>
>>>> On 1 November 2016 at 16:44, Tvrtko Ursulin <tursulin@ursulin.net>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>>
>>>>> On 02/03/16 14:00, Tomeu Vizoso wrote:
>>>>>>
>>>>>>
>>>>>> igt_create_bo_with_dimensions() is intended to abstract differences
>>>>>> between drivers in buffer object creation.
>>>>>>
>>>>>> The driver-specific ioctls will be called if the driver that is being
>>>>>> tested can satisfy the needs of the calling subtest, or it will be
>>>>>> skipped otherwise.
>>>>>>
>>>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>>>> ---
>>>>>>
>>>>>>  lib/igt_fb.c | 83
>>>>>> ++++++++++++++++++++++++++++++++++++++++++------------------
>>>>>>  lib/igt_fb.h |  6 +++++
>>>>>>  2 files changed, 65 insertions(+), 24 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>>>>>> index cd1605308308..0a3526f4e4ea 100644
>>>>>> --- a/lib/igt_fb.c
>>>>>> +++ b/lib/igt_fb.c
>>>>>> @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int
>>>>>> height,
>>>>>> int bpp, uint64_t tiling,
>>>>>>         *size_ret = size;
>>>>>>  }
>>>>>>
>>>>>> +int igt_create_bo_with_dimensions(int fd, int width, int height,
>>>>>> +                                 uint32_t format, uint64_t modifier,
>>>>>> +                                 unsigned stride, unsigned *size_ret,
>>>>>> +                                 unsigned *stride_ret, bool *is_dumb)
>>>>>> +{
>>>>>> +       int bpp = igt_drm_format_to_bpp(format);
>>>>>> +       int bo;
>>>>>> +
>>>>>> +       igt_assert((modifier && stride) || (!modifier && !stride));
>>>>>> +
>>>>>> +       if (modifier) {
>>>>>> +               unsigned size, calculated_stride;
>>>>>> +
>>>>>> +               igt_calc_fb_size(fd, width, height, bpp, modifier,
>>>>>> &size,
>>>>>> +                                &calculated_stride);
>>>>>> +               if (stride == 0)
>>>>>> +                       stride = calculated_stride;
>>>>>> +
>>>>>> +               if (is_dumb)
>>>>>> +                       *is_dumb = false;
>>>>>> +
>>>>>> +               if (is_i915_device(fd)) {
>>>>>> +
>>>>>> +                       bo = gem_create(fd, size);
>>>>>> +                       gem_set_tiling(fd, bo, modifier, stride);
>>>>>
>>>>>
>>>>>
>>>>> This is broken, gem_set_tiling does not take a fb modifier but an
>>>>> object
>>>>> tiling mode.
>>>>>
>>>>> You can demonstrate the failure if you got a Skylake system with:
>>>>>
>>>>> tests/kms_flip_tiling --r flip-changes-tiling-Yf
>>>>
>>>>
>>>> Hi,
>>>>
>>>> that subtest fails occasionally here due to CRC issues, but the one
>>>> that fails due to the tiling constant passed to gem_set_tiling is
>>>> flip-Yf-tiled.
>>>>
>>>> Have fixed it by converting the modifier to a tiling mode, but I also
>>>> needed to make sure that we don't pass to the kernel the Yf or Ys
>>>> constants as the kernel doesn't know about those.
>>>>
>>>> Both fixes have been pushed already.
>>>
>>>
>>> With the two patches you pushed flip-changes-tiling-Yf is still broken
>>> due the tiling mode mismatch, not the CRC.
>>>
>>> Perhaps you tested with all three patches you posted today?
>>
>>
>> Just before pushing I tested with this:
>>
>> IGT-Version: 1.16-g050c00d (x86_64) (Linux: 4.5.5-300.fc24.x86_64 x86_64)
>>
>> So I can only think of a difference in the hw causing a different
>> codepath to execute (it's a i3-6100U), or a difference in the kernel.
>>
>> This is a remote box and I don't have yet all the infrastructure in
>> place so I could monitor the boot, nor power cycle it, so I cannot test
>> with a newer kernel yet.
>>
>> If you can confirm that we should be passing I915_TILING_NONE to
>> set_tiling when the FB has I915_FORMAT_MOD_Yf_TILED, I can fix that, but
>> I would also like to understand why the test is failing for you.
>
>
> It should definitely be I915_TILING_NONE. If you look at the i915 code,
> intel_display.c/intel_framebuffer_init will reject attempts to add a
> framebuffer where object tiling does not match the framebuffer modifier. And
> the intel_fb_modifier_to_tiling helper translates the Yf/Ys modifiers to
> NONE tiling.

Cool, will do that change now.

> So I have no idea how it could have possibly worked for you. :)

Yeah, I'm curious as well, will check in a bit.

Thanks,

Tomeu


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