[PATCH i-g-t 0/7] Enable kms_flip_event_leak and kms_vblank on vc4.

[PATCH i-g-t 0/7] Enable kms_flip_event_leak and kms_vblank on vc4.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

This series enables tests/kms_flip_event_leak and tests/kms_vblank on non-intel
hardware.
It's been tested with vc4 on an rpi2 running kernel 4.6.0-rc1.

Robert Foss (7):
  lib/igt_kms: Add support for up to 10 planes per pipe.
  lib/igt_kms: Fix plane counting in igt_display_init.
  lib/igt_kms: Make sure that default planes aren't overwritten.
  lib/igt_kms: Only move the in cursor plane for Intel hw.
  lib/igt_kms: Switch to verbose assert.
  kms_vblank: Switch from using crtc0 statically to explicitly setting
    mode.
  kms_flip_event_leak: Enable test on DRIVER_ANY.

 lib/igt_kms.c               |  12 ++-
 lib/igt_kms.h               |   2 +-
 tests/kms_flip_event_leak.c |   8 +-
 tests/kms_vblank.c          | 186 ++++++++++++++++++++++++++++++++++----------
 4 files changed, 160 insertions(+), 48 deletions(-)

-- 
2.5.0

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

[PATCH i-g-t 1/7] lib/igt_kms: Add support for up to 10 planes per pipe.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

The VC4 DRM currently uses 10 planes which is more than any
other DRM, let's allocate space for the worst case scenario.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 lib/igt_kms.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 2c189ed..3edfc3d 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -267,7 +267,7 @@ struct igt_pipe {
 	igt_display_t *display;
 	enum pipe pipe;
 	bool enabled;
-#define IGT_MAX_PLANES	4
+#define IGT_MAX_PLANES	10
 	int n_planes;
 	igt_plane_t planes[IGT_MAX_PLANES];
 	uint64_t background; /* Background color MSB BGR 16bpc LSB */
-- 
2.5.0

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

[PATCH i-g-t 2/7] lib/igt_kms: Fix plane counting in igt_display_init.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

Fix issue where the plane counting fails due to the number and
configuration of planes being unlike the intel configuration.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 lib/igt_kms.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index ef24a49..07fb73b 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1283,6 +1283,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 		igt_plane_t *plane;
 		int p = IGT_PLANE_2;
 		int j, type;
+		uint8_t plane_counter = 0;
 
 		pipe->crtc_id = resources->crtcs[i];
 		pipe->display = display;
@@ -1330,8 +1331,10 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 				break;
 			}
 
+			plane_counter++;
 			plane->pipe = pipe;
 			plane->drm_plane = drm_plane;
+
 			if (is_atomic == 0) {
 				display->is_atomic = 1;
 				igt_atomic_fill_plane_props(display, plane, IGT_NUM_PLANE_PROPS, igt_plane_prop_names);
@@ -1380,8 +1383,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 			plane->is_cursor = true;
 		}
 
-		/* planes = 1 primary, (p-1) sprites, 1 cursor */
-		pipe->n_planes = p + 1;
+		pipe->n_planes = plane_counter;
 
 		/* make sure we don't overflow the plane array */
 		igt_assert(pipe->n_planes <= IGT_MAX_PLANES);
-- 
2.5.0

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

[PATCH i-g-t 3/7] lib/igt_kms: Make sure that default planes aren't overwritten.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

Avoid overwriting planes with statically asigned indices
with planes that have dynamically assigned indices.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 lib/igt_kms.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 07fb73b..3f953ec 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1326,6 +1326,8 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 				display->has_universal_planes = 1;
 				break;
 			default:
+				while (p == IGT_PLANE_PRIMARY || p == IGT_PLANE_CURSOR)
+					p++;
 				plane = &pipe->planes[p];
 				plane->index = p++;
 				break;
-- 
2.5.0

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

[PATCH i-g-t 4/7] lib/igt_kms: Only move the in cursor plane for Intel hw.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

Avoid moving the cursor plane when on non-intel hardware.
Running the move block on hardware with more than IGT_PLANE_CURSOR
number of planes causes planes do be zeroed out.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 lib/igt_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 3f953ec..522ede5 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1363,7 +1363,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 			 * only 1 sprite, that's the wrong slot and we need to
 			 * move it down.
 			 */
-			if (p != IGT_PLANE_CURSOR) {
+			if (IS_INTEL(drm_fd) && p != IGT_PLANE_CURSOR) {
 				pipe->planes[p] = pipe->planes[IGT_PLANE_CURSOR];
 				pipe->planes[p].index = p;
 				memset(&pipe->planes[IGT_PLANE_CURSOR], 0,
-- 
2.5.0

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

[PATCH i-g-t 5/7] lib/igt_kms: Switch to verbose assert.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

Switch igt_assert to igt_assert_lte to provide more diagnostic
information.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 lib/igt_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 522ede5..f6d0760 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1388,7 +1388,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 		pipe->n_planes = plane_counter;
 
 		/* make sure we don't overflow the plane array */
-		igt_assert(pipe->n_planes <= IGT_MAX_PLANES);
+		igt_assert_lte(pipe->n_planes, IGT_MAX_PLANES);
 	}
 
 	/*
-- 
2.5.0

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

[PATCH i-g-t 6/7] kms_vblank: Switch from using crtc0 statically to explicitly setting mode.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

Previously crtc0 was statically used for VBLANK tests, but
that assumption is not valid for the VC4 platform.
Instead we're now explicitly setting the mode.

Also add support for testing all connected connectors during
the same test.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 tests/kms_vblank.c | 186 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 147 insertions(+), 39 deletions(-)

diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
index 40ab6fd..1a026d2 100644
--- a/tests/kms_vblank.c
+++ b/tests/kms_vblank.c
@@ -44,6 +44,20 @@
 
 IGT_TEST_DESCRIPTION("Test speed of WaitVblank.");
 
+typedef struct {
+	int drm_fd;
+	igt_display_t display;
+	struct igt_fb primary_fb;
+	struct igt_fb fb;
+	igt_output_t *output;
+	enum pipe pipe;
+	int left, right, top, bottom;
+	int screenw, screenh;
+	int refresh;
+	uint32_t devid;
+	unsigned flags;
+} data_t;
+
 static double elapsed(const struct timespec *start,
 		      const struct timespec *end,
 		      int loop)
@@ -51,75 +65,165 @@ static double elapsed(const struct timespec *start,
 	return (1e6*(end->tv_sec - start->tv_sec) + (end->tv_nsec - start->tv_nsec)/1000)/loop;
 }
 
-static bool crtc0_active(int fd)
+static uint32_t crtc_id_to_flag(uint32_t crtc_id)
 {
-	union drm_wait_vblank vbl;
+	if (crtc_id == 0)
+		return 0;
+	else if (crtc_id == 1)
+		return 1 | _DRM_VBLANK_SECONDARY;
+	else
+		return crtc_id << 1;
+}
 
-	memset(&vbl, 0, sizeof(vbl));
-	vbl.request.type = DRM_VBLANK_RELATIVE;
-	return drmIoctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl) == 0;
+static bool prepare_crtc(data_t *data, igt_output_t *output)
+{
+	drmModeModeInfo *mode;
+	igt_display_t *display = &data->display;
+	igt_plane_t *primary;
+
+	/* select the pipe we want to use */
+	igt_output_set_pipe(output, data->pipe);
+	igt_display_commit(display);
+
+	if (!output->valid) {
+		igt_output_set_pipe(output, PIPE_ANY);
+		igt_display_commit(display);
+		return false;
+	}
+
+	/* create and set the primary plane fb */
+	mode = igt_output_get_mode(output);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    LOCAL_DRM_FORMAT_MOD_NONE,
+			    0.0, 0.0, 0.0,
+			    &data->primary_fb);
+
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+	igt_plane_set_fb(primary, &data->primary_fb);
+
+	igt_display_commit(display);
+
+	igt_wait_for_vblank(data->drm_fd, data->pipe);
+
+	return true;
+}
+
+static void cleanup_crtc(data_t *data, igt_output_t *output)
+{
+	igt_display_t *display = &data->display;
+	igt_plane_t *primary;
+
+	igt_remove_fb(data->drm_fd, &data->primary_fb);
+
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+
+	igt_output_set_pipe(output, PIPE_ANY);
+	igt_display_commit(display);
 }
 
-static void accuracy(int fd)
+static void run_test(data_t *data, void (*testfunc)(data_t *, bool), bool boolean)
+{
+	igt_display_t *display = &data->display;
+	igt_output_t *output;
+	enum pipe p;
+	int valid_tests = 0;
+
+	for_each_connected_output(display, output) {
+		data->output = output;
+		for_each_pipe(display, p) {
+			data->pipe = p;
+
+			if (!prepare_crtc(data, output))
+				continue;
+
+			valid_tests++;
+
+			igt_info("Beginning %s on pipe %s, connector %s\n",
+				 igt_subtest_name(),
+				 kmstest_pipe_name(data->pipe),
+				 igt_output_name(output));
+
+			testfunc(data, boolean);
+
+			igt_info("\n%s on pipe %s, connector %s: PASSED\n\n",
+				 igt_subtest_name(),
+				 kmstest_pipe_name(data->pipe),
+				 igt_output_name(output));
+
+			/* cleanup what prepare_crtc() has done */
+			cleanup_crtc(data, output);
+		}
+	}
+
+	igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
+}
+
+static void accuracy(data_t *data, bool busy)
 {
 	union drm_wait_vblank vbl;
 	unsigned long target;
+	uint32_t crtc_id_flag;
 	int n;
 
 	memset(&vbl, 0, sizeof(vbl));
+	crtc_id_flag = crtc_id_to_flag(data->pipe);
 
-	vbl.request.type = DRM_VBLANK_RELATIVE;
+	vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
 	vbl.request.sequence = 1;
-	do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
+	do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 
 	target = vbl.reply.sequence + 60;
 	for (n = 0; n < 60; n++) {
-		vbl.request.type = DRM_VBLANK_RELATIVE;
+		vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
 		vbl.request.sequence = 1;
-		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
+		do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 
-		vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
+		vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT | crtc_id_flag;
 		vbl.request.sequence = target;
-		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
+		do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 	}
-	vbl.request.type = DRM_VBLANK_RELATIVE;
+	vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
 	vbl.request.sequence = 0;
-	do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
+	do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 	igt_assert_eq(vbl.reply.sequence, target);
 
 	for (n = 0; n < 60; n++) {
 		struct drm_event_vblank ev;
-		igt_assert_eq(read(fd, &ev, sizeof(ev)), sizeof(ev));
+		igt_assert_eq(read(data->drm_fd, &ev, sizeof(ev)), sizeof(ev));
 		igt_assert_eq(ev.sequence, target);
 	}
 }
 
-static void vblank_query(int fd, bool busy)
+static void vblank_query(data_t *data, bool busy)
 {
 	union drm_wait_vblank vbl;
 	struct timespec start, end;
 	unsigned long sq, count = 0;
 	struct drm_event_vblank buf;
+	uint32_t crtc_id_flag;
 
 	memset(&vbl, 0, sizeof(vbl));
+	crtc_id_flag = crtc_id_to_flag(data->pipe);
 
 	if (busy) {
-		vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
+		vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT | crtc_id_flag;
 		vbl.request.sequence = 72;
-		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
+		do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 	}
 
-	vbl.request.type = DRM_VBLANK_RELATIVE;
+	vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
 	vbl.request.sequence = 0;
-	do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
+	do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 
 	sq = vbl.reply.sequence;
 
 	clock_gettime(CLOCK_MONOTONIC, &start);
 	do {
-		vbl.request.type = DRM_VBLANK_RELATIVE;
+		vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
 		vbl.request.sequence = 0;
-		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
+		do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 		count++;
 	} while ((vbl.reply.sequence - sq) <= 60);
 	clock_gettime(CLOCK_MONOTONIC, &end);
@@ -128,35 +232,37 @@ static void vblank_query(int fd, bool busy)
 		 busy ? "busy" : "idle", elapsed(&start, &end, count));
 
 	if (busy)
-		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
+		igt_assert_eq(read(data->drm_fd, &buf, sizeof(buf)), sizeof(buf));
 }
 
-static void vblank_wait(int fd, bool busy)
+static void vblank_wait(data_t *data, bool busy)
 {
 	union drm_wait_vblank vbl;
 	struct timespec start, end;
 	unsigned long sq, count = 0;
 	struct drm_event_vblank buf;
+	uint32_t crtc_id_flag;
 
 	memset(&vbl, 0, sizeof(vbl));
+	crtc_id_flag = crtc_id_to_flag(data->pipe);
 
 	if (busy) {
-		vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
+		vbl.request.type |= DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT | crtc_id_flag;
 		vbl.request.sequence = 72;
-		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
+		do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 	}
 
-	vbl.request.type = DRM_VBLANK_RELATIVE;
+	vbl.request.type |= DRM_VBLANK_RELATIVE | crtc_id_flag;
 	vbl.request.sequence = 0;
-	do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
+	do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 
 	sq = vbl.reply.sequence;
 
 	clock_gettime(CLOCK_MONOTONIC, &start);
 	do {
-		vbl.request.type = DRM_VBLANK_RELATIVE;
+		vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
 		vbl.request.sequence = 1;
-		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
+		do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 		count++;
 	} while ((vbl.reply.sequence - sq) <= 60);
 	clock_gettime(CLOCK_MONOTONIC, &end);
@@ -167,32 +273,34 @@ static void vblank_wait(int fd, bool busy)
 		 elapsed(&start, &end, count));
 
 	if (busy)
-		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
+		igt_assert_eq(read(data->drm_fd, &buf, sizeof(buf)), sizeof(buf));
 }
 
 igt_main
 {
-	int fd;
+	data_t data;
 
 	igt_skip_on_simulation();
 
 	igt_fixture {
-		fd = drm_open_driver(DRIVER_ANY);
-		igt_require(crtc0_active(fd));
+		data.drm_fd = drm_open_driver(DRIVER_ANY);
+		kmstest_set_vt_graphics_mode();
+		igt_display_init(&data.display, data.drm_fd);
 	}
 
 	igt_subtest("accuracy")
-		accuracy(fd);
+		run_test(&data, accuracy, false);
 
 	igt_subtest("query-idle")
-		vblank_query(fd, false);
+		run_test(&data, vblank_query, false);
 
 	igt_subtest("query-busy")
-		vblank_query(fd, true);
+		run_test(&data, vblank_query, true);
 
 	igt_subtest("wait-idle")
-		vblank_wait(fd, false);
+		run_test(&data, vblank_wait, false);
 
 	igt_subtest("wait-busy")
-		vblank_wait(fd, true);
+		run_test(&data, vblank_wait, true);
 }
+
-- 
2.5.0

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

[PATCH i-g-t 7/7] kms_flip_event_leak: Enable test on DRIVER_ANY.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

Change DRIVER_INTEL to DRIVER_ANY to enable tests/kms_flip_event_leak
on any driver.
Switch the tiling format to something intel indenpendant.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 tests/kms_flip_event_leak.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/kms_flip_event_leak.c b/tests/kms_flip_event_leak.c
index c19ed98..a1389b4 100644
--- a/tests/kms_flip_event_leak.c
+++ b/tests/kms_flip_event_leak.c
@@ -62,13 +62,13 @@ static bool test(data_t *data, enum pipe pipe, igt_output_t *output)
 
 	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
 			    DRM_FORMAT_XRGB8888,
-			    LOCAL_I915_FORMAT_MOD_X_TILED,
+			    LOCAL_DRM_FORMAT_MOD_NONE,
 			    0.0, 0.0, 0.0, &fb[0]);
 
 	igt_plane_set_fb(primary, &fb[0]);
 	igt_display_commit2(&data->display, COMMIT_LEGACY);
 
-	fd = drm_open_driver(DRIVER_INTEL);
+	fd = drm_open_driver(DRIVER_ANY);
 
 	ret = drmDropMaster(data->drm_fd);
 	igt_assert_eq(ret, 0);
@@ -78,7 +78,7 @@ static bool test(data_t *data, enum pipe pipe, igt_output_t *output)
 
 	igt_create_color_fb(fd, mode->hdisplay, mode->vdisplay,
 			    DRM_FORMAT_XRGB8888,
-			    LOCAL_I915_FORMAT_MOD_X_TILED,
+			    LOCAL_DRM_FORMAT_MOD_NONE,
 			    0.0, 0.0, 0.0, &fb[1]);
 	ret = drmModePageFlip(fd, output->config.crtc->crtc_id,
 			      fb[1].fb_id, DRM_MODE_PAGE_FLIP_EVENT,
@@ -109,7 +109,7 @@ igt_simple_main
 
 	igt_skip_on_simulation();
 
-	data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
+	data.drm_fd = drm_open_driver_master(DRIVER_ANY);
 	kmstest_set_vt_graphics_mode();
 
 	igt_display_init(&data.display, data.drm_fd);
-- 
2.5.0

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

Re: [PATCH i-g-t 6/7] kms_vblank: Switch from using crtc0 statically to explicitly setting mode.

[Daniel Vetter]

On Wed, Apr 20, 2016 at 10:59:48AM -0400, robert.foss@collabora.com wrote:
> From: Robert Foss <robert.foss@collabora.com>
> 
> Previously crtc0 was statically used for VBLANK tests, but
> that assumption is not valid for the VC4 platform.
> Instead we're now explicitly setting the mode.
> 
> Also add support for testing all connected connectors during
> the same test.
> 
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  tests/kms_vblank.c | 186 ++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 147 insertions(+), 39 deletions(-)
> 
> diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
> index 40ab6fd..1a026d2 100644
> --- a/tests/kms_vblank.c
> +++ b/tests/kms_vblank.c
> @@ -44,6 +44,20 @@
>  
>  IGT_TEST_DESCRIPTION("Test speed of WaitVblank.");
>  
> +typedef struct {
> +	int drm_fd;
> +	igt_display_t display;
> +	struct igt_fb primary_fb;
> +	struct igt_fb fb;
> +	igt_output_t *output;
> +	enum pipe pipe;
> +	int left, right, top, bottom;
> +	int screenw, screenh;
> +	int refresh;
> +	uint32_t devid;
> +	unsigned flags;
> +} data_t;
> +
>  static double elapsed(const struct timespec *start,
>  		      const struct timespec *end,
>  		      int loop)
> @@ -51,75 +65,165 @@ static double elapsed(const struct timespec *start,
>  	return (1e6*(end->tv_sec - start->tv_sec) + (end->tv_nsec - start->tv_nsec)/1000)/loop;
>  }
>  
> -static bool crtc0_active(int fd)
> +static uint32_t crtc_id_to_flag(uint32_t crtc_id)
>  {
> -	union drm_wait_vblank vbl;
> +	if (crtc_id == 0)
> +		return 0;
> +	else if (crtc_id == 1)
> +		return 1 | _DRM_VBLANK_SECONDARY;
> +	else
> +		return crtc_id << 1;
> +}
>  
> -	memset(&vbl, 0, sizeof(vbl));
> -	vbl.request.type = DRM_VBLANK_RELATIVE;
> -	return drmIoctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl) == 0;
> +static bool prepare_crtc(data_t *data, igt_output_t *output)
> +{
> +	drmModeModeInfo *mode;
> +	igt_display_t *display = &data->display;
> +	igt_plane_t *primary;
> +
> +	/* select the pipe we want to use */
> +	igt_output_set_pipe(output, data->pipe);
> +	igt_display_commit(display);
> +
> +	if (!output->valid) {
> +		igt_output_set_pipe(output, PIPE_ANY);
> +		igt_display_commit(display);
> +		return false;
> +	}
> +
> +	/* create and set the primary plane fb */
> +	mode = igt_output_get_mode(output);
> +	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> +			    DRM_FORMAT_XRGB8888,
> +			    LOCAL_DRM_FORMAT_MOD_NONE,
> +			    0.0, 0.0, 0.0,
> +			    &data->primary_fb);
> +
> +	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
> +	igt_plane_set_fb(primary, &data->primary_fb);
> +
> +	igt_display_commit(display);
> +
> +	igt_wait_for_vblank(data->drm_fd, data->pipe);
> +
> +	return true;
> +}
> +
> +static void cleanup_crtc(data_t *data, igt_output_t *output)
> +{
> +	igt_display_t *display = &data->display;
> +	igt_plane_t *primary;
> +
> +	igt_remove_fb(data->drm_fd, &data->primary_fb);
> +
> +	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
> +	igt_plane_set_fb(primary, NULL);
> +
> +	igt_output_set_pipe(output, PIPE_ANY);
> +	igt_display_commit(display);
>  }
>  
> -static void accuracy(int fd)
> +static void run_test(data_t *data, void (*testfunc)(data_t *, bool), bool boolean)
> +{
> +	igt_display_t *display = &data->display;
> +	igt_output_t *output;
> +	enum pipe p;
> +	int valid_tests = 0;
> +
> +	for_each_connected_output(display, output) {
> +		data->output = output;
> +		for_each_pipe(display, p) {
> +			data->pipe = p;
> +
> +			if (!prepare_crtc(data, output))
> +				continue;
> +
> +			valid_tests++;
> +
> +			igt_info("Beginning %s on pipe %s, connector %s\n",
> +				 igt_subtest_name(),
> +				 kmstest_pipe_name(data->pipe),
> +				 igt_output_name(output));
> +
> +			testfunc(data, boolean);
> +
> +			igt_info("\n%s on pipe %s, connector %s: PASSED\n\n",
> +				 igt_subtest_name(),
> +				 kmstest_pipe_name(data->pipe),
> +				 igt_output_name(output));
> +
> +			/* cleanup what prepare_crtc() has done */
> +			cleanup_crtc(data, output);
> +		}
> +	}
> +
> +	igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
> +}

Annoying that we have to have so much boilerplate, but no one yet figured
out how to make this prettier. Oh well.

Ack on patches 1-6, but I didn't do a detailed review. Would be good to
get that from Tomeu or Daniel Stone.
-Daniel

> +
> +static void accuracy(data_t *data, bool busy)
>  {
>  	union drm_wait_vblank vbl;
>  	unsigned long target;
> +	uint32_t crtc_id_flag;
>  	int n;
>  
>  	memset(&vbl, 0, sizeof(vbl));
> +	crtc_id_flag = crtc_id_to_flag(data->pipe);
>  
> -	vbl.request.type = DRM_VBLANK_RELATIVE;
> +	vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
>  	vbl.request.sequence = 1;
> -	do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> +	do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>  
>  	target = vbl.reply.sequence + 60;
>  	for (n = 0; n < 60; n++) {
> -		vbl.request.type = DRM_VBLANK_RELATIVE;
> +		vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
>  		vbl.request.sequence = 1;
> -		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> +		do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>  
> -		vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
> +		vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT | crtc_id_flag;
>  		vbl.request.sequence = target;
> -		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> +		do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>  	}
> -	vbl.request.type = DRM_VBLANK_RELATIVE;
> +	vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
>  	vbl.request.sequence = 0;
> -	do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> +	do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>  	igt_assert_eq(vbl.reply.sequence, target);
>  
>  	for (n = 0; n < 60; n++) {
>  		struct drm_event_vblank ev;
> -		igt_assert_eq(read(fd, &ev, sizeof(ev)), sizeof(ev));
> +		igt_assert_eq(read(data->drm_fd, &ev, sizeof(ev)), sizeof(ev));
>  		igt_assert_eq(ev.sequence, target);
>  	}
>  }
>  
> -static void vblank_query(int fd, bool busy)
> +static void vblank_query(data_t *data, bool busy)
>  {
>  	union drm_wait_vblank vbl;
>  	struct timespec start, end;
>  	unsigned long sq, count = 0;
>  	struct drm_event_vblank buf;
> +	uint32_t crtc_id_flag;
>  
>  	memset(&vbl, 0, sizeof(vbl));
> +	crtc_id_flag = crtc_id_to_flag(data->pipe);
>  
>  	if (busy) {
> -		vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
> +		vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT | crtc_id_flag;
>  		vbl.request.sequence = 72;
> -		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> +		do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>  	}
>  
> -	vbl.request.type = DRM_VBLANK_RELATIVE;
> +	vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
>  	vbl.request.sequence = 0;
> -	do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> +	do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>  
>  	sq = vbl.reply.sequence;
>  
>  	clock_gettime(CLOCK_MONOTONIC, &start);
>  	do {
> -		vbl.request.type = DRM_VBLANK_RELATIVE;
> +		vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
>  		vbl.request.sequence = 0;
> -		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> +		do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>  		count++;
>  	} while ((vbl.reply.sequence - sq) <= 60);
>  	clock_gettime(CLOCK_MONOTONIC, &end);
> @@ -128,35 +232,37 @@ static void vblank_query(int fd, bool busy)
>  		 busy ? "busy" : "idle", elapsed(&start, &end, count));
>  
>  	if (busy)
> -		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
> +		igt_assert_eq(read(data->drm_fd, &buf, sizeof(buf)), sizeof(buf));
>  }
>  
> -static void vblank_wait(int fd, bool busy)
> +static void vblank_wait(data_t *data, bool busy)
>  {
>  	union drm_wait_vblank vbl;
>  	struct timespec start, end;
>  	unsigned long sq, count = 0;
>  	struct drm_event_vblank buf;
> +	uint32_t crtc_id_flag;
>  
>  	memset(&vbl, 0, sizeof(vbl));
> +	crtc_id_flag = crtc_id_to_flag(data->pipe);
>  
>  	if (busy) {
> -		vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
> +		vbl.request.type |= DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT | crtc_id_flag;
>  		vbl.request.sequence = 72;
> -		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> +		do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>  	}
>  
> -	vbl.request.type = DRM_VBLANK_RELATIVE;
> +	vbl.request.type |= DRM_VBLANK_RELATIVE | crtc_id_flag;
>  	vbl.request.sequence = 0;
> -	do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> +	do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>  
>  	sq = vbl.reply.sequence;
>  
>  	clock_gettime(CLOCK_MONOTONIC, &start);
>  	do {
> -		vbl.request.type = DRM_VBLANK_RELATIVE;
> +		vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
>  		vbl.request.sequence = 1;
> -		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> +		do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>  		count++;
>  	} while ((vbl.reply.sequence - sq) <= 60);
>  	clock_gettime(CLOCK_MONOTONIC, &end);
> @@ -167,32 +273,34 @@ static void vblank_wait(int fd, bool busy)
>  		 elapsed(&start, &end, count));
>  
>  	if (busy)
> -		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
> +		igt_assert_eq(read(data->drm_fd, &buf, sizeof(buf)), sizeof(buf));
>  }
>  
>  igt_main
>  {
> -	int fd;
> +	data_t data;
>  
>  	igt_skip_on_simulation();
>  
>  	igt_fixture {
> -		fd = drm_open_driver(DRIVER_ANY);
> -		igt_require(crtc0_active(fd));
> +		data.drm_fd = drm_open_driver(DRIVER_ANY);
> +		kmstest_set_vt_graphics_mode();
> +		igt_display_init(&data.display, data.drm_fd);
>  	}
>  
>  	igt_subtest("accuracy")
> -		accuracy(fd);
> +		run_test(&data, accuracy, false);
>  
>  	igt_subtest("query-idle")
> -		vblank_query(fd, false);
> +		run_test(&data, vblank_query, false);
>  
>  	igt_subtest("query-busy")
> -		vblank_query(fd, true);
> +		run_test(&data, vblank_query, true);
>  
>  	igt_subtest("wait-idle")
> -		vblank_wait(fd, false);
> +		run_test(&data, vblank_wait, false);
>  
>  	igt_subtest("wait-busy")
> -		vblank_wait(fd, true);
> +		run_test(&data, vblank_wait, true);
>  }
> +
> -- 
> 2.5.0
> 

-- 
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: [PATCH i-g-t 7/7] kms_flip_event_leak: Enable test on DRIVER_ANY.

[Daniel Vetter]

On Wed, Apr 20, 2016 at 10:59:49AM -0400, robert.foss@collabora.com wrote:
> From: Robert Foss <robert.foss@collabora.com>
> 
> Change DRIVER_INTEL to DRIVER_ANY to enable tests/kms_flip_event_leak
> on any driver.
> Switch the tiling format to something intel indenpendant.
> 
> Signed-off-by: Robert Foss <robert.foss@collabora.com>

I thought we need the same treatment of explicitly setting a mode here as
with drm_event? Or was that another one? And where's the patch to make
drm_event generic?

With my confusion about all the pending patches resolved, ack on this one
too.
-Daniel

> ---
>  tests/kms_flip_event_leak.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/kms_flip_event_leak.c b/tests/kms_flip_event_leak.c
> index c19ed98..a1389b4 100644
> --- a/tests/kms_flip_event_leak.c
> +++ b/tests/kms_flip_event_leak.c
> @@ -62,13 +62,13 @@ static bool test(data_t *data, enum pipe pipe, igt_output_t *output)
>  
>  	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
>  			    DRM_FORMAT_XRGB8888,
> -			    LOCAL_I915_FORMAT_MOD_X_TILED,
> +			    LOCAL_DRM_FORMAT_MOD_NONE,
>  			    0.0, 0.0, 0.0, &fb[0]);
>  
>  	igt_plane_set_fb(primary, &fb[0]);
>  	igt_display_commit2(&data->display, COMMIT_LEGACY);
>  
> -	fd = drm_open_driver(DRIVER_INTEL);
> +	fd = drm_open_driver(DRIVER_ANY);
>  
>  	ret = drmDropMaster(data->drm_fd);
>  	igt_assert_eq(ret, 0);
> @@ -78,7 +78,7 @@ static bool test(data_t *data, enum pipe pipe, igt_output_t *output)
>  
>  	igt_create_color_fb(fd, mode->hdisplay, mode->vdisplay,
>  			    DRM_FORMAT_XRGB8888,
> -			    LOCAL_I915_FORMAT_MOD_X_TILED,
> +			    LOCAL_DRM_FORMAT_MOD_NONE,
>  			    0.0, 0.0, 0.0, &fb[1]);
>  	ret = drmModePageFlip(fd, output->config.crtc->crtc_id,
>  			      fb[1].fb_id, DRM_MODE_PAGE_FLIP_EVENT,
> @@ -109,7 +109,7 @@ igt_simple_main
>  
>  	igt_skip_on_simulation();
>  
> -	data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
> +	data.drm_fd = drm_open_driver_master(DRIVER_ANY);
>  	kmstest_set_vt_graphics_mode();
>  
>  	igt_display_init(&data.display, data.drm_fd);
> -- 
> 2.5.0
> 

-- 
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: [PATCH i-g-t 2/7] lib/igt_kms: Fix plane counting in igt_display_init.

[Tomeu Vizoso]

On 20 April 2016 at 16:59,  <robert.foss@collabora.com> wrote:
> From: Robert Foss <robert.foss@collabora.com>
>
> Fix issue where the plane counting fails due to the number and
> configuration of planes being unlike the intel configuration.
>
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  lib/igt_kms.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index ef24a49..07fb73b 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1283,6 +1283,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>                 igt_plane_t *plane;
>                 int p = IGT_PLANE_2;
>                 int j, type;
> +               uint8_t plane_counter = 0;

nitpick: I think it's more customary to call it n_planes or num_planes.

Regards,

Tomeu

>                 pipe->crtc_id = resources->crtcs[i];
>                 pipe->display = display;
> @@ -1330,8 +1331,10 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>                                 break;
>                         }
>
> +                       plane_counter++;
>                         plane->pipe = pipe;
>                         plane->drm_plane = drm_plane;
> +
>                         if (is_atomic == 0) {
>                                 display->is_atomic = 1;
>                                 igt_atomic_fill_plane_props(display, plane, IGT_NUM_PLANE_PROPS, igt_plane_prop_names);
> @@ -1380,8 +1383,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>                         plane->is_cursor = true;
>                 }
>
> -               /* planes = 1 primary, (p-1) sprites, 1 cursor */
> -               pipe->n_planes = p + 1;
> +               pipe->n_planes = plane_counter;
>
>                 /* make sure we don't overflow the plane array */
>                 igt_assert(pipe->n_planes <= IGT_MAX_PLANES);
> --
> 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: [PATCH i-g-t 3/7] lib/igt_kms: Make sure that default planes aren't overwritten.

[Tomeu Vizoso]

On 20 April 2016 at 16:59,  <robert.foss@collabora.com> wrote:
> From: Robert Foss <robert.foss@collabora.com>
>
> Avoid overwriting planes with statically asigned indices
> with planes that have dynamically assigned indices.
>
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  lib/igt_kms.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 07fb73b..3f953ec 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1326,6 +1326,8 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>                                 display->has_universal_planes = 1;
>                                 break;
>                         default:
> +                               while (p == IGT_PLANE_PRIMARY || p == IGT_PLANE_CURSOR)
> +                                       p++;

Had to read a fair bit of code to understand what's going on here, so
may make sense to add a comment.

Regards,

Tomeu

>                                 plane = &pipe->planes[p];
>                                 plane->index = p++;
>                                 break;
> --
> 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: [PATCH i-g-t 4/7] lib/igt_kms: Only move the in cursor plane for Intel hw.

[Tomeu Vizoso]

On 20 April 2016 at 16:59,  <robert.foss@collabora.com> wrote:
> From: Robert Foss <robert.foss@collabora.com>
>
> Avoid moving the cursor plane when on non-intel hardware.
> Running the move block on hardware with more than IGT_PLANE_CURSOR
> number of planes causes planes do be zeroed out.
>
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  lib/igt_kms.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 3f953ec..522ede5 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1363,7 +1363,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>                          * only 1 sprite, that's the wrong slot and we need to
>                          * move it down.
>                          */
> -                       if (p != IGT_PLANE_CURSOR) {
> +                       if (IS_INTEL(drm_fd) && p != IGT_PLANE_CURSOR) {

IS_INTEL takes a devid, not a fd, and its meaning is that the devid
corresponds to known hardware made by Intel.

In this case we probably want to know if the DRM driver is i915, and
for that you have is_i915_device(fd).

Regards,

Tomeu

>                                 pipe->planes[p] = pipe->planes[IGT_PLANE_CURSOR];
>                                 pipe->planes[p].index = p;
>                                 memset(&pipe->planes[IGT_PLANE_CURSOR], 0,
> --
> 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: [PATCH i-g-t 4/7] lib/igt_kms: Only move the in cursor plane for Intel hw.

[Daniel Vetter]

On Wed, Apr 20, 2016 at 10:59:46AM -0400, robert.foss@collabora.com wrote:
> From: Robert Foss <robert.foss@collabora.com>
> 
> Avoid moving the cursor plane when on non-intel hardware.
> Running the move block on hardware with more than IGT_PLANE_CURSOR
> number of planes causes planes do be zeroed out.
> 
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  lib/igt_kms.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 3f953ec..522ede5 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1363,7 +1363,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>  			 * only 1 sprite, that's the wrong slot and we need to
>  			 * move it down.
>  			 */
> -			if (p != IGT_PLANE_CURSOR) {
> +			if (IS_INTEL(drm_fd) && p != IGT_PLANE_CURSOR) {

Reading this again, isn't the problem that on some hw there's simply no
cursor plane? With universal planes the cursor plane should alias with
some of the real planes, and we simply need to make sure that we get that
aliasing right.

But if there's no cursor registered, well there's no cursor ...
-Daniel

>  				pipe->planes[p] = pipe->planes[IGT_PLANE_CURSOR];
>  				pipe->planes[p].index = p;
>  				memset(&pipe->planes[IGT_PLANE_CURSOR], 0,
> -- 
> 2.5.0
> 

-- 
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: [PATCH i-g-t 3/7] lib/igt_kms: Make sure that default planes aren't overwritten.

[Daniel Vetter]

On Wed, Apr 20, 2016 at 10:59:45AM -0400, robert.foss@collabora.com wrote:
> From: Robert Foss <robert.foss@collabora.com>
> 
> Avoid overwriting planes with statically asigned indices
> with planes that have dynamically assigned indices.
> 
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  lib/igt_kms.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 07fb73b..3f953ec 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1326,6 +1326,8 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>  				display->has_universal_planes = 1;
>  				break;
>  			default:
> +				while (p == IGT_PLANE_PRIMARY || p == IGT_PLANE_CURSOR)
> +					p++;

What we might need to do is to actually make primary and cursor
dynamically assigned. Since with most drivers that's the case that
happens. Otoh we have a variable number of planes on i915 too, so not
entirely sure now what's going on here ...
-Daniel

>  				plane = &pipe->planes[p];
>  				plane->index = p++;
>  				break;
> -- 
> 2.5.0
> 

-- 
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: [PATCH i-g-t 1/7] lib/igt_kms: Add support for up to 10 planes per pipe.

[Daniel Vetter]

On Wed, Apr 20, 2016 at 10:59:43AM -0400, robert.foss@collabora.com wrote:
> From: Robert Foss <robert.foss@collabora.com>
> 
> The VC4 DRM currently uses 10 planes which is more than any
> other DRM, let's allocate space for the worst case scenario.
> 
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  lib/igt_kms.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 2c189ed..3edfc3d 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -267,7 +267,7 @@ struct igt_pipe {
>  	igt_display_t *display;
>  	enum pipe pipe;
>  	bool enabled;
> -#define IGT_MAX_PLANES	4
> +#define IGT_MAX_PLANES	10

Reading more, don't we need to update stuff like kmstest_plane_name too?
-Daniel

>  	int n_planes;
>  	igt_plane_t planes[IGT_MAX_PLANES];
>  	uint64_t background; /* Background color MSB BGR 16bpc LSB */
> -- 
> 2.5.0
> 

-- 
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: [PATCH i-g-t 6/7] kms_vblank: Switch from using crtc0 statically to explicitly setting mode.

[Daniel Vetter]

On Thu, Apr 21, 2016 at 11:18:40AM +0200, Daniel Vetter wrote:
> Ack on patches 1-6, but I didn't do a detailed review. Would be good to
> get that from Tomeu or Daniel Stone.

Ok, I retract my ack. We need to extend all the places that use enum
igt_plane to correctly support more planes. And we might need to fix up
the implied assumption that a given plane can only be used on 1 crtc. But
maybe that's for a 2nd series.

Anyway I sent out a quick patch as a starting point. Please use that as
the baseline for v2.
-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: [PATCH i-g-t 4/7] lib/igt_kms: Only move the in cursor plane for Intel hw.

[Robert Foss]

On 04/21/2016 10:48 AM, Daniel Vetter wrote:
> On Wed, Apr 20, 2016 at 10:59:46AM -0400, robert.foss@collabora.com wrote:
>> From: Robert Foss <robert.foss@collabora.com>
>>
>> Avoid moving the cursor plane when on non-intel hardware.
>> Running the move block on hardware with more than IGT_PLANE_CURSOR
>> number of planes causes planes do be zeroed out.
>>
>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>> ---
>>   lib/igt_kms.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 3f953ec..522ede5 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -1363,7 +1363,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>>   			 * only 1 sprite, that's the wrong slot and we need to
>>   			 * move it down.
>>   			 */
>> -			if (p != IGT_PLANE_CURSOR) {
>> +			if (IS_INTEL(drm_fd) && p != IGT_PLANE_CURSOR) {
> Reading this again, isn't the problem that on some hw there's simply no
> cursor plane? With universal planes the cursor plane should alias with
> some of the real planes, and we simply need to make sure that we get that
> aliasing right.
>
> But if there's no cursor registered, well there's no cursor ...
> -Daniel
 From my point of view the problem is differentiating between the cases 
when
IGT_PLANE_CURSOR is aliasing with IGT_PLANE_4 (IGT_PLANE_3+1).

IGT_PLANE_CURSOR is assumed to be the last plane in the if statement,
which isn't the case for platform with more than 4 planes.

So identifying which platforms this assumption is true (IS_INTEL()) and
only proceeding for those platforms seemed like reasonable fix.
Maybe a more generic solution is preferable.
>>   				pipe->planes[p] = pipe->planes[IGT_PLANE_CURSOR];
>>   				pipe->planes[p].index = p;
>>   				memset(&pipe->planes[IGT_PLANE_CURSOR], 0,
>> -- 
>> 2.5.0
>>

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

Re: [PATCH i-g-t 3/7] lib/igt_kms: Make sure that default planes aren't overwritten.

[Robert Foss]

On 04/21/2016 10:50 AM, Daniel Vetter wrote:
> On Wed, Apr 20, 2016 at 10:59:45AM -0400, robert.foss@collabora.com wrote:
>> From: Robert Foss <robert.foss@collabora.com>
>>
>> Avoid overwriting planes with statically asigned indices
>> with planes that have dynamically assigned indices.
>>
>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>> ---
>>   lib/igt_kms.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 07fb73b..3f953ec 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -1326,6 +1326,8 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>>   				display->has_universal_planes = 1;
>>   				break;
>>   			default:
>> +				while (p == IGT_PLANE_PRIMARY || p == IGT_PLANE_CURSOR)
>> +					p++;
> What we might need to do is to actually make primary and cursor
> dynamically assigned. Since with most drivers that's the case that
> happens. Otoh we have a variable number of planes on i915 too, so not
> entirely sure now what's going on here ...
> -Daniel
Since PRIMARY and CURSOR planes are statically assigned and
that the other cases in this switch statement used the static
assignments, the loop simply makes sure the we don't overwrite
the PRIMARY or CURSOR plane.


>>   				plane = &pipe->planes[p];
>>   				plane->index = p++;
>>   				break;
>> -- 
>> 2.5.0
>>

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

Re: [PATCH i-g-t 3/7] lib/igt_kms: Make sure that default planes aren't overwritten.

[Daniel Vetter]

On Thu, Apr 21, 2016 at 01:31:48PM -0400, Robert Foss wrote:
> 
> 
> On 04/21/2016 10:50 AM, Daniel Vetter wrote:
> >On Wed, Apr 20, 2016 at 10:59:45AM -0400, robert.foss@collabora.com wrote:
> >>From: Robert Foss <robert.foss@collabora.com>
> >>
> >>Avoid overwriting planes with statically asigned indices
> >>with planes that have dynamically assigned indices.
> >>
> >>Signed-off-by: Robert Foss <robert.foss@collabora.com>
> >>---
> >>  lib/igt_kms.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >>index 07fb73b..3f953ec 100644
> >>--- a/lib/igt_kms.c
> >>+++ b/lib/igt_kms.c
> >>@@ -1326,6 +1326,8 @@ void igt_display_init(igt_display_t *display, int drm_fd)
> >>  				display->has_universal_planes = 1;
> >>  				break;
> >>  			default:
> >>+				while (p == IGT_PLANE_PRIMARY || p == IGT_PLANE_CURSOR)
> >>+					p++;
> >What we might need to do is to actually make primary and cursor
> >dynamically assigned. Since with most drivers that's the case that
> >happens. Otoh we have a variable number of planes on i915 too, so not
> >entirely sure now what's going on here ...
> >-Daniel
> Since PRIMARY and CURSOR planes are statically assigned and
> that the other cases in this switch statement used the static
> assignments, the loop simply makes sure the we don't overwrite
> the PRIMARY or CURSOR plane.

Well primary should be first, cursor last ... Are there drivers which
register planes in a funny order that we need this? If so a comment would
be good I think.
-Daniel

> 
> 
> >>  				plane = &pipe->planes[p];
> >>  				plane->index = p++;
> >>  				break;
> >>-- 
> >>2.5.0
> >>
> 

-- 
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: [PATCH i-g-t 4/7] lib/igt_kms: Only move the in cursor plane for Intel hw.

[Daniel Vetter]

On Thu, Apr 21, 2016 at 01:24:53PM -0400, Robert Foss wrote:
> 
> 
> On 04/21/2016 10:48 AM, Daniel Vetter wrote:
> >On Wed, Apr 20, 2016 at 10:59:46AM -0400, robert.foss@collabora.com wrote:
> >>From: Robert Foss <robert.foss@collabora.com>
> >>
> >>Avoid moving the cursor plane when on non-intel hardware.
> >>Running the move block on hardware with more than IGT_PLANE_CURSOR
> >>number of planes causes planes do be zeroed out.
> >>
> >>Signed-off-by: Robert Foss <robert.foss@collabora.com>
> >>---
> >>  lib/igt_kms.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >>index 3f953ec..522ede5 100644
> >>--- a/lib/igt_kms.c
> >>+++ b/lib/igt_kms.c
> >>@@ -1363,7 +1363,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
> >>  			 * only 1 sprite, that's the wrong slot and we need to
> >>  			 * move it down.
> >>  			 */
> >>-			if (p != IGT_PLANE_CURSOR) {
> >>+			if (IS_INTEL(drm_fd) && p != IGT_PLANE_CURSOR) {
> >Reading this again, isn't the problem that on some hw there's simply no
> >cursor plane? With universal planes the cursor plane should alias with
> >some of the real planes, and we simply need to make sure that we get that
> >aliasing right.
> >
> >But if there's no cursor registered, well there's no cursor ...
> >-Daniel
> From my point of view the problem is differentiating between the cases when
> IGT_PLANE_CURSOR is aliasing with IGT_PLANE_4 (IGT_PLANE_3+1).
> 
> IGT_PLANE_CURSOR is assumed to be the last plane in the if statement,
> which isn't the case for platform with more than 4 planes.

That's why you need to add more IGT_PLANE_x defines, to make sure _CURSOR
and the last one don't accidentally alias. Because if they do, we have a
problem.
-Daniel

> 
> So identifying which platforms this assumption is true (IS_INTEL()) and
> only proceeding for those platforms seemed like reasonable fix.
> Maybe a more generic solution is preferable.
> >>  				pipe->planes[p] = pipe->planes[IGT_PLANE_CURSOR];
> >>  				pipe->planes[p].index = p;
> >>  				memset(&pipe->planes[IGT_PLANE_CURSOR], 0,
> >>-- 
> >>2.5.0
> >>
> 

-- 
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: [PATCH i-g-t 3/7] lib/igt_kms: Make sure that default planes aren't overwritten.

[Robert Foss]

On 04/22/2016 08:53 AM, Daniel Vetter wrote:
> On Thu, Apr 21, 2016 at 01:31:48PM -0400, Robert Foss wrote:
>>
>> On 04/21/2016 10:50 AM, Daniel Vetter wrote:
>>> On Wed, Apr 20, 2016 at 10:59:45AM -0400, robert.foss@collabora.com wrote:
>>>> From: Robert Foss <robert.foss@collabora.com>
>>>>
>>>> Avoid overwriting planes with statically asigned indices
>>>> with planes that have dynamically assigned indices.
>>>>
>>>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>>>> ---
>>>>   lib/igt_kms.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>>> index 07fb73b..3f953ec 100644
>>>> --- a/lib/igt_kms.c
>>>> +++ b/lib/igt_kms.c
>>>> @@ -1326,6 +1326,8 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>>>>   				display->has_universal_planes = 1;
>>>>   				break;
>>>>   			default:
>>>> +				while (p == IGT_PLANE_PRIMARY || p == IGT_PLANE_CURSOR)
>>>> +					p++;
>>> What we might need to do is to actually make primary and cursor
>>> dynamically assigned. Since with most drivers that's the case that
>>> happens. Otoh we have a variable number of planes on i915 too, so not
>>> entirely sure now what's going on here ...
>>> -Daniel
>> Since PRIMARY and CURSOR planes are statically assigned and
>> that the other cases in this switch statement used the static
>> assignments, the loop simply makes sure the we don't overwrite
>> the PRIMARY or CURSOR plane.
> Well primary should be first, cursor last ... Are there drivers which
> register planes in a funny order that we need this? If so a comment would
> be good I think.
> -Daniel
I wasn't aware of the cursor plane always being last one.
I'll have the v2 patches reflect this.
>
>>
>>>>   				plane = &pipe->planes[p];
>>>>   				plane->index = p++;
>>>>   				break;
>>>> -- 
>>>> 2.5.0
>>>>

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