[PATCH i-g-t v5 0/7] kms_flip_event_leak and kms_vblank fixes for

[PATCH i-g-t v5 0/7] kms_flip_event_leak and kms_vblank fixes for

[robert.foss]

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

In addition to the changes made in v4, danvet suggested that the argument for
the (newly renamed) helper function kmstest_get_vbl_flag should be changed.

I investigated it and found that all of the places where the helper function
currently is used, would require more of a rework with changed arguments than
with unchanged arguments, so I decided to not change the arguments.

Changes since v1:
- kms_vblank: Removed un-used members of data_t struct.
- Rename plane_counter to n_planes.
- Removed un-needed handling CURSOR plane location.
- Added names for additional planes in update kmstest_plane_name.

Changes since v2:
- Rebased onto trunk which already contains kms_flip_event_leak changes.

Changes since v3:
- kms_vblank.c: Removed drm_fd from data_t, to decrease the number of
unrelated changes in the patch.
- kms_vblank.c: Changed "int valid_tests" to "unsigned valid_tests".
- kms_vblank.c: Changed crtc_id_flag/crtc_id_to_flag to
pipe_id_flag/pipe_id_to_flag.
- kms_vblank.c: Changed to "vbl.request.type |= crtc_id_flag;".
- kms_vblank.c: Moved crtc_id_to_flag to igt_kms.c.
- kms_vblank.c: Renamed crtc_id_to_flag to pipe_id_to_vbl_flag.
- kms_flip.c: Changed test busy_mode boolean into mode_busy bitfield in struct
data.
- igt_kms.h: Moved IGT_PLANE_CURSOR position comment.

Changes since v4:
- kms_vblank.c: Removed whitespace at end of file.
- igt_kms: Changed name of helper function to kmstest_get_vbl_flag
- igt_kms: Changed kmstest_get_vbl_flag to not set DRM_VBLANK_RELATIVE flag

Robert Foss (7):
  lib/igt_kms: Add support for up to 10 planes.
  lib/igt_kms: Fix plane counting in igt_display_init.
  lib/igt_kms: Switch to verbose assert.
  lib/igt_kms: Added pipe_id_to_vbl_flag() to igt_kms.
  kms_vblank: Switch from using crtc0 statically to explicitly setting
    mode.
  igt_kms: Change igt_wait_for_vblank to use helper function.
  kms_flip: Change __wait_for_vblank to use helper function.

 lib/igt_kms.c      |  36 ++++++++++--
 lib/igt_kms.h      |   9 ++-
 tests/kms_flip.c   |   8 +--
 tests/kms_vblank.c | 161 ++++++++++++++++++++++++++++++++++++++++++++---------
 4 files changed, 179 insertions(+), 35 deletions(-)

-- 
2.7.4

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

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

[robert.foss]

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

Increase the number of planes supported to 10.

kmstest_plane_name only previously supported 4 planes,
this patch adds support for up to 10 planes.

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

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index f85be1e..c2283fd 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -320,6 +320,12 @@ const char *kmstest_plane_name(enum igt_plane plane)
 		[IGT_PLANE_1] = "plane1",
 		[IGT_PLANE_2] = "plane2",
 		[IGT_PLANE_3] = "plane3",
+		[IGT_PLANE_4] = "plane4",
+		[IGT_PLANE_5] = "plane5",
+		[IGT_PLANE_6] = "plane6",
+		[IGT_PLANE_7] = "plane7",
+		[IGT_PLANE_8] = "plane8",
+		[IGT_PLANE_9] = "plane9",
 		[IGT_PLANE_CURSOR] = "cursor",
 	};
 
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 5c83401..d9a4e11 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -54,7 +54,13 @@ enum igt_plane {
 	IGT_PLANE_PRIMARY = IGT_PLANE_1,
 	IGT_PLANE_2,
 	IGT_PLANE_3,
-	IGT_PLANE_CURSOR,
+	IGT_PLANE_4,
+	IGT_PLANE_5,
+	IGT_PLANE_6,
+	IGT_PLANE_7,
+	IGT_PLANE_8,
+	IGT_PLANE_9,
+	IGT_PLANE_CURSOR, /* IGT_PLANE_CURSOR is always the last plane. */
 	IGT_MAX_PLANES,
 };
 
-- 
2.7.4

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

[PATCH i-g-t v5 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 c2283fd..876f65c 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1289,6 +1289,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 n_planes = 0;
 
 		pipe->crtc_id = resources->crtcs[i];
 		pipe->display = display;
@@ -1336,8 +1337,10 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 				break;
 			}
 
+			n_planes++;
 			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);
@@ -1386,8 +1389,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 = n_planes;
 
 		/* make sure we don't overflow the plane array */
 		igt_assert(pipe->n_planes <= IGT_MAX_PLANES);
-- 
2.7.4

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

[PATCH i-g-t v5 3/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 876f65c..9af9603 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1392,7 +1392,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 		pipe->n_planes = n_planes;
 
 		/* 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.7.4

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

[PATCH i-g-t v5 4/7] lib/igt_kms: Added pipe_id_to_vbl_flag() to igt_kms.

[robert.foss]

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

Added function pipe_id_to_vbl_flag() to igt_kms.
This function formats a pipe_id into the format
expected by DRM while processing DRM_IOCTL_WAIT_VBLANK.

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

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 9af9603..5ef73c8 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -2557,3 +2557,19 @@ void igt_reset_connectors(void)
 		close(fd);
 	}
 }
+
+/**
+ * kmstest_get_vbl_flag:
+ *
+ * Convert a pipe id into the flag representation
+ * expected in DRM while processing DRM_IOCTL_WAIT_VBLANK.
+ */
+uint32_t kmstest_get_vbl_flag(uint32_t pipe_id)
+{
+	if (pipe_id == 0)
+		return 0;
+	else if (pipe_id == 1)
+		return _DRM_VBLANK_SECONDARY;
+	else
+		return pipe_id << 1;
+}
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index d9a4e11..9fecf2b 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -400,6 +400,7 @@ void igt_wait_for_vblank(int drm_fd, enum pipe pipe);
 
 void igt_enable_connectors(void);
 void igt_reset_connectors(void);
+uint32_t kmstest_get_vbl_flag(uint32_t pipe_id);
 
 #define EDID_LENGTH 128
 const unsigned char* igt_kms_get_base_edid(void);
-- 
2.7.4

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

[PATCH i-g-t v5 5/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 | 161 ++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 136 insertions(+), 25 deletions(-)

diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
index 40ab6fd..9bc4929 100644
--- a/tests/kms_vblank.c
+++ b/tests/kms_vblank.c
@@ -44,6 +44,14 @@
 
 IGT_TEST_DESCRIPTION("Test speed of WaitVblank.");
 
+typedef struct {
+	igt_display_t display;
+	struct igt_fb primary_fb;
+	igt_output_t *output;
+	enum pipe pipe;
+	uint8_t mode_busy:1;
+} data_t;
+
 static double elapsed(const struct timespec *start,
 		      const struct timespec *end,
 		      int loop)
@@ -51,38 +59,119 @@ 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 bool prepare_crtc(data_t *data, int fd, igt_output_t *output)
 {
-	union drm_wait_vblank vbl;
+	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;
+	}
 
-	memset(&vbl, 0, sizeof(vbl));
-	vbl.request.type = DRM_VBLANK_RELATIVE;
-	return drmIoctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl) == 0;
+	/* create and set the primary plane fb */
+	mode = igt_output_get_mode(output);
+	igt_create_color_fb(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(fd, data->pipe);
+
+	return true;
+}
+
+static void cleanup_crtc(data_t *data, int fd, igt_output_t *output)
+{
+	igt_display_t *display = &data->display;
+	igt_plane_t *primary;
+
+	igt_remove_fb(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 run_test(data_t *data, int fd, void (*testfunc)(data_t *, int))
+{
+	igt_display_t *display = &data->display;
+	igt_output_t *output;
+	enum pipe p;
+	unsigned int valid_tests = 0;
+
+	for_each_connected_output(display, output) {
+		for_each_pipe(display, p) {
+			data->pipe = p;
+
+			if (!prepare_crtc(data, fd, 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, fd);
+
+			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, fd, output);
+		}
+	}
+
+	igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
 }
 
-static void accuracy(int fd)
+static void accuracy(data_t *data, int fd)
 {
 	union drm_wait_vblank vbl;
 	unsigned long target;
+	uint32_t pipe_id_flag;
 	int n;
 
 	memset(&vbl, 0, sizeof(vbl));
+	pipe_id_flag = kmstest_get_vbl_flag(data->pipe);
 
 	vbl.request.type = DRM_VBLANK_RELATIVE;
+	vbl.request.type |= pipe_id_flag;
 	vbl.request.sequence = 1;
 	do_ioctl(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 |= pipe_id_flag;
 		vbl.request.sequence = 1;
 		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 
 		vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
+		vbl.request.type |= pipe_id_flag;
 		vbl.request.sequence = target;
 		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 	}
 	vbl.request.type = DRM_VBLANK_RELATIVE;
+	vbl.request.type |= pipe_id_flag;
 	vbl.request.sequence = 0;
 	do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 	igt_assert_eq(vbl.reply.sequence, target);
@@ -94,22 +183,26 @@ static void accuracy(int fd)
 	}
 }
 
-static void vblank_query(int fd, bool busy)
+static void vblank_query(data_t *data, int fd)
 {
 	union drm_wait_vblank vbl;
 	struct timespec start, end;
 	unsigned long sq, count = 0;
 	struct drm_event_vblank buf;
+	uint32_t pipe_id_flag;
 
 	memset(&vbl, 0, sizeof(vbl));
+	pipe_id_flag = kmstest_get_vbl_flag(data->pipe);
 
-	if (busy) {
+	if (data->mode_busy) {
 		vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
+		vbl.request.type |= pipe_id_flag;
 		vbl.request.sequence = 72;
 		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 	}
 
 	vbl.request.type = DRM_VBLANK_RELATIVE;
+	vbl.request.type |= pipe_id_flag;
 	vbl.request.sequence = 0;
 	do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 
@@ -118,6 +211,7 @@ static void vblank_query(int fd, bool busy)
 	clock_gettime(CLOCK_MONOTONIC, &start);
 	do {
 		vbl.request.type = DRM_VBLANK_RELATIVE;
+		vbl.request.type |= pipe_id_flag;
 		vbl.request.sequence = 0;
 		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 		count++;
@@ -125,28 +219,32 @@ static void vblank_query(int fd, bool busy)
 	clock_gettime(CLOCK_MONOTONIC, &end);
 
 	igt_info("Time to query current counter (%s):		%7.3fµs\n",
-		 busy ? "busy" : "idle", elapsed(&start, &end, count));
+		 data->mode_busy ? "busy" : "idle", elapsed(&start, &end, count));
 
-	if (busy)
+	if (data->mode_busy)
 		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
 }
 
-static void vblank_wait(int fd, bool busy)
+static void vblank_wait(data_t *data, int fd)
 {
 	union drm_wait_vblank vbl;
 	struct timespec start, end;
 	unsigned long sq, count = 0;
 	struct drm_event_vblank buf;
+	uint32_t pipe_id_flag;
 
 	memset(&vbl, 0, sizeof(vbl));
+	pipe_id_flag = kmstest_get_vbl_flag(data->pipe);
 
-	if (busy) {
+	if (data->mode_busy) {
 		vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
+		vbl.request.type |= pipe_id_flag;
 		vbl.request.sequence = 72;
 		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 	}
 
 	vbl.request.type = DRM_VBLANK_RELATIVE;
+	vbl.request.type |= pipe_id_flag;
 	vbl.request.sequence = 0;
 	do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 
@@ -155,6 +253,7 @@ static void vblank_wait(int fd, bool busy)
 	clock_gettime(CLOCK_MONOTONIC, &start);
 	do {
 		vbl.request.type = DRM_VBLANK_RELATIVE;
+		vbl.request.type |= pipe_id_flag;
 		vbl.request.sequence = 1;
 		do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
 		count++;
@@ -163,36 +262,48 @@ static void vblank_wait(int fd, bool busy)
 
 	igt_info("Time to wait for %ld/%d vblanks (%s):		%7.3fµs\n",
 		 count, (int)(vbl.reply.sequence - sq),
-		 busy ? "busy" : "idle",
+		 data->mode_busy ? "busy" : "idle",
 		 elapsed(&start, &end, count));
 
-	if (busy)
+	if (data->mode_busy)
 		igt_assert_eq(read(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));
+		kmstest_set_vt_graphics_mode();
+		igt_display_init(&data.display, fd);
 	}
 
-	igt_subtest("accuracy")
-		accuracy(fd);
+	igt_subtest("accuracy") {
+		data.mode_busy = 0;
+		run_test(&data, fd, accuracy);
+	}
 
-	igt_subtest("query-idle")
-		vblank_query(fd, false);
+	igt_subtest("query-idle") {
+		data.mode_busy = 0;
+		run_test(&data, fd, vblank_query);
+	}
 
-	igt_subtest("query-busy")
-		vblank_query(fd, true);
+	igt_subtest("query-busy") {
+		data.mode_busy = 1;
+		run_test(&data, fd, vblank_query);
+	}
 
-	igt_subtest("wait-idle")
-		vblank_wait(fd, false);
+	igt_subtest("wait-idle") {
+		data.mode_busy = 0;
+		run_test(&data, fd, vblank_wait);
+	}
 
-	igt_subtest("wait-busy")
-		vblank_wait(fd, true);
+	igt_subtest("wait-busy") {
+		data.mode_busy = 1;
+		run_test(&data, fd, vblank_wait);
+	}
 }
-- 
2.7.4

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

[PATCH i-g-t v5 6/7] igt_kms: Change igt_wait_for_vblank to use helper function.

[robert.foss]

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

Change igt_wait_for_vblank() to use kmstest_get_vbl_flag() helper function.

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 5ef73c8..bc022c8 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -2488,11 +2488,13 @@ void igt_crtc_set_background(igt_pipe_t *pipe, uint64_t background)
 void igt_wait_for_vblank(int drm_fd, enum pipe pipe)
 {
 	drmVBlank wait_vbl;
+	uint32_t pipe_id_flag;
 
 	memset(&wait_vbl, 0, sizeof(wait_vbl));
+	pipe_id_flag = kmstest_get_vbl_flag(pipe);
 
-	wait_vbl.request.type = pipe << DRM_VBLANK_HIGH_CRTC_SHIFT |
-				DRM_VBLANK_RELATIVE;
+	wait_vbl.request.type = DRM_VBLANK_RELATIVE;
+	wait_vbl.request.type |= pipe_id_flag;
 	wait_vbl.request.sequence = 1;
 
 	igt_assert(drmWaitVBlank(drm_fd, &wait_vbl) == 0);
-- 
2.7.4

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

[PATCH i-g-t v5 7/7] kms_flip: Change __wait_for_vblank to use helper function.

[robert.foss]

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

Change __wait_for_vblank() to use kmstest_get_vbl_flag() helper function.

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

diff --git a/tests/kms_flip.c b/tests/kms_flip.c
index eda2fcc..0ecca07 100644
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -481,15 +481,15 @@ static int __wait_for_vblank(unsigned int flags, int crtc_idx,
 {
 	drmVBlank wait_vbl;
 	int ret;
-	unsigned crtc_idx_mask;
+	uint32_t pipe_id_flag;
 	bool event = !(flags & TEST_VBLANK_BLOCK);
 
 	memset(&wait_vbl, 0, sizeof(wait_vbl));
 
-	crtc_idx_mask = crtc_idx << DRM_VBLANK_HIGH_CRTC_SHIFT;
-	igt_assert(!(crtc_idx_mask & ~DRM_VBLANK_HIGH_CRTC_MASK));
+	pipe_id_flag = kmstest_get_vbl_flag(crtc_idx);
+	igt_assert(!(pipe_id_flag & ~DRM_VBLANK_HIGH_CRTC_MASK));
 
-	wait_vbl.request.type = crtc_idx_mask;
+	wait_vbl.request.type = pipe_id_flag;
 	if (flags & TEST_VBLANK_ABSOLUTE)
 		wait_vbl.request.type |= DRM_VBLANK_ABSOLUTE;
 	else
-- 
2.7.4

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

Re: [PATCH i-g-t v5 7/7] kms_flip: Change __wait_for_vblank to use helper function.

[Marius Vlad]

[-- Attachment #1.1: Type: text/plain, Size: 1552 bytes --]

On Mon, May 16, 2016 at 09:38:32AM -0400, robert.foss@collabora.com wrote:
> From: Robert Foss <robert.foss@collabora.com>
> 
> Change __wait_for_vblank() to use kmstest_get_vbl_flag() helper function.
> 
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  tests/kms_flip.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> index eda2fcc..0ecca07 100644
> --- a/tests/kms_flip.c
> +++ b/tests/kms_flip.c
> @@ -481,15 +481,15 @@ static int __wait_for_vblank(unsigned int flags, int crtc_idx,
>  {
>  	drmVBlank wait_vbl;
>  	int ret;
> -	unsigned crtc_idx_mask;
> +	uint32_t pipe_id_flag;
>  	bool event = !(flags & TEST_VBLANK_BLOCK);
>  
>  	memset(&wait_vbl, 0, sizeof(wait_vbl));
>  
> -	crtc_idx_mask = crtc_idx << DRM_VBLANK_HIGH_CRTC_SHIFT;
> -	igt_assert(!(crtc_idx_mask & ~DRM_VBLANK_HIGH_CRTC_MASK));
If crtc_idx is 1 (pipe B), crtc_idx_mask = (1 << DRM_VBLANK_HIGH_CRTC_SHIFT) = 2.
> +	pipe_id_flag = kmstest_get_vbl_flag(crtc_idx);
> +	igt_assert(!(pipe_id_flag & ~DRM_VBLANK_HIGH_CRTC_MASK));
If crtc_idx is 1 (pipe B), kmstest_get_vbl_flag(crtc_idx) = DRM_VBLANK_SECONDARY = 0x20000000

And the assertion fails as !(0x20000000 & ~0x0000003e) = 0.

Should kmstest_get_vbl_flag() always return pipe_id << 1?

>  
> -	wait_vbl.request.type = crtc_idx_mask;
> +	wait_vbl.request.type = pipe_id_flag;
>  	if (flags & TEST_VBLANK_ABSOLUTE)
>  		wait_vbl.request.type |= DRM_VBLANK_ABSOLUTE;
>  	else
> -- 
> 2.7.4
> 

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH i-g-t v5 7/7] kms_flip: Change __wait_for_vblank to use helper function.

[Robert Foss]

On 2016-05-17 06:32 AM, Marius Vlad wrote:
> On Mon, May 16, 2016 at 09:38:32AM -0400, robert.foss@collabora.com wrote:
>> From: Robert Foss <robert.foss@collabora.com>
>>
>> Change __wait_for_vblank() to use kmstest_get_vbl_flag() helper function.
>>
>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>> ---
>>   tests/kms_flip.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/kms_flip.c b/tests/kms_flip.c
>> index eda2fcc..0ecca07 100644
>> --- a/tests/kms_flip.c
>> +++ b/tests/kms_flip.c
>> @@ -481,15 +481,15 @@ static int __wait_for_vblank(unsigned int flags, int crtc_idx,
>>   {
>>   	drmVBlank wait_vbl;
>>   	int ret;
>> -	unsigned crtc_idx_mask;
>> +	uint32_t pipe_id_flag;
>>   	bool event = !(flags & TEST_VBLANK_BLOCK);
>>
>>   	memset(&wait_vbl, 0, sizeof(wait_vbl));
>>
>> -	crtc_idx_mask = crtc_idx << DRM_VBLANK_HIGH_CRTC_SHIFT;
>> -	igt_assert(!(crtc_idx_mask & ~DRM_VBLANK_HIGH_CRTC_MASK));
> If crtc_idx is 1 (pipe B), crtc_idx_mask = (1 << DRM_VBLANK_HIGH_CRTC_SHIFT) = 2.
>> +	pipe_id_flag = kmstest_get_vbl_flag(crtc_idx);
>> +	igt_assert(!(pipe_id_flag & ~DRM_VBLANK_HIGH_CRTC_MASK));
> If crtc_idx is 1 (pipe B), kmstest_get_vbl_flag(crtc_idx) = DRM_VBLANK_SECONDARY = 0x20000000
>
> And the assertion fails as !(0x20000000 & ~0x0000003e) = 0.
>
> Should kmstest_get_vbl_flag() always return pipe_id << 1?

 From re-reading the pipe id parsing code in drm_irq.c drm_wait_vblank()
it would seem to me that the assertion is incorrect, specifically for 
the case of _DRM_VBLANK_SECONDARY.

Supplying the flag _DRM_VBLANK_SECONDARY and setting the high crtc id 
field to 1 fboth seem to be valid ways to communicate the same thing.

	flags = vblwait->request.type & _DRM_VBLANK_FLAGS_MASK;
	high_pipe = (vblwait->request.type &_DRM_VBLANK_HIGH_CRTC_MASK);
	if (high_pipe)
		pipe = high_pipe >> _DRM_VBLANK_HIGH_CRTC_SHIFT;
	else
		pipe = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;


Maybe adding correct and more thorough assertion to 
kmstest_get_vbl_flag() and removing the failing assertion is the way 
forward.

>
>>
>> -	wait_vbl.request.type = crtc_idx_mask;
>> +	wait_vbl.request.type = pipe_id_flag;
>>   	if (flags & TEST_VBLANK_ABSOLUTE)
>>   		wait_vbl.request.type |= DRM_VBLANK_ABSOLUTE;
>>   	else
>> --
>> 2.7.4
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v5 7/7] kms_flip: Change __wait_for_vblank to use helper function.

[Marius Vlad]

[-- Attachment #1.1: Type: text/plain, Size: 2833 bytes --]

On Tue, May 17, 2016 at 10:48:02AM -0400, Robert Foss wrote:
> 
> 
> On 2016-05-17 06:32 AM, Marius Vlad wrote:
> >On Mon, May 16, 2016 at 09:38:32AM -0400, robert.foss@collabora.com wrote:
> >>From: Robert Foss <robert.foss@collabora.com>
> >>
> >>Change __wait_for_vblank() to use kmstest_get_vbl_flag() helper function.
> >>
> >>Signed-off-by: Robert Foss <robert.foss@collabora.com>
> >>---
> >>  tests/kms_flip.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> >>index eda2fcc..0ecca07 100644
> >>--- a/tests/kms_flip.c
> >>+++ b/tests/kms_flip.c
> >>@@ -481,15 +481,15 @@ static int __wait_for_vblank(unsigned int flags, int crtc_idx,
> >>  {
> >>  	drmVBlank wait_vbl;
> >>  	int ret;
> >>-	unsigned crtc_idx_mask;
> >>+	uint32_t pipe_id_flag;
> >>  	bool event = !(flags & TEST_VBLANK_BLOCK);
> >>
> >>  	memset(&wait_vbl, 0, sizeof(wait_vbl));
> >>
> >>-	crtc_idx_mask = crtc_idx << DRM_VBLANK_HIGH_CRTC_SHIFT;
> >>-	igt_assert(!(crtc_idx_mask & ~DRM_VBLANK_HIGH_CRTC_MASK));
> >If crtc_idx is 1 (pipe B), crtc_idx_mask = (1 << DRM_VBLANK_HIGH_CRTC_SHIFT) = 2.
> >>+	pipe_id_flag = kmstest_get_vbl_flag(crtc_idx);
> >>+	igt_assert(!(pipe_id_flag & ~DRM_VBLANK_HIGH_CRTC_MASK));
> >If crtc_idx is 1 (pipe B), kmstest_get_vbl_flag(crtc_idx) = DRM_VBLANK_SECONDARY = 0x20000000
> >
> >And the assertion fails as !(0x20000000 & ~0x0000003e) = 0.
> >
> >Should kmstest_get_vbl_flag() always return pipe_id << 1?
> 
> From re-reading the pipe id parsing code in drm_irq.c drm_wait_vblank()
> it would seem to me that the assertion is incorrect, specifically for the
> case of _DRM_VBLANK_SECONDARY.
> 
> Supplying the flag _DRM_VBLANK_SECONDARY and setting the high crtc id field
> to 1 fboth seem to be valid ways to communicate the same thing.
Indeed.
> 
> 	flags = vblwait->request.type & _DRM_VBLANK_FLAGS_MASK;
> 	high_pipe = (vblwait->request.type &_DRM_VBLANK_HIGH_CRTC_MASK);
> 	if (high_pipe)
> 		pipe = high_pipe >> _DRM_VBLANK_HIGH_CRTC_SHIFT;
> 	else
> 		pipe = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;
> 
> 
> Maybe adding correct and more thorough assertion to kmstest_get_vbl_flag()
> and removing the failing assertion is the way forward.
Sounds reasonable to me, though I assume that the assertion had/has
some point in determining that you have a valid crtc_idx.
> 
> >
> >>
> >>-	wait_vbl.request.type = crtc_idx_mask;
> >>+	wait_vbl.request.type = pipe_id_flag;
> >>  	if (flags & TEST_VBLANK_ABSOLUTE)
> >>  		wait_vbl.request.type |= DRM_VBLANK_ABSOLUTE;
> >>  	else
> >>--
> >>2.7.4
> >>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH i-g-t v5 0/7] kms_flip_event_leak and kms_vblank fixes for

[Marius Vlad]

[-- Attachment #1.1: Type: text/plain, Size: 2935 bytes --]

Hi,

I'm getting mixed results w/ this series applied. The kernel seems to
trip in different ways either from suspend or on reload (running the BAT
suite on a HSW machine), and though it seems totally
unrelated I can't reproduce the same behaviour without the series
applied. Haven't managed to get some valid output as the machine
dead-locks/freezes, so please be patient until I try to isolate the
problem. It could be I stumbled something totally unrelated.


On Mon, May 16, 2016 at 09:38:25AM -0400, robert.foss@collabora.com wrote:
> From: Robert Foss <robert.foss@collabora.com>
> 
> In addition to the changes made in v4, danvet suggested that the argument for
> the (newly renamed) helper function kmstest_get_vbl_flag should be changed.
> 
> I investigated it and found that all of the places where the helper function
> currently is used, would require more of a rework with changed arguments than
> with unchanged arguments, so I decided to not change the arguments.
> 
> Changes since v1:
> - kms_vblank: Removed un-used members of data_t struct.
> - Rename plane_counter to n_planes.
> - Removed un-needed handling CURSOR plane location.
> - Added names for additional planes in update kmstest_plane_name.
> 
> Changes since v2:
> - Rebased onto trunk which already contains kms_flip_event_leak changes.
> 
> Changes since v3:
> - kms_vblank.c: Removed drm_fd from data_t, to decrease the number of
> unrelated changes in the patch.
> - kms_vblank.c: Changed "int valid_tests" to "unsigned valid_tests".
> - kms_vblank.c: Changed crtc_id_flag/crtc_id_to_flag to
> pipe_id_flag/pipe_id_to_flag.
> - kms_vblank.c: Changed to "vbl.request.type |= crtc_id_flag;".
> - kms_vblank.c: Moved crtc_id_to_flag to igt_kms.c.
> - kms_vblank.c: Renamed crtc_id_to_flag to pipe_id_to_vbl_flag.
> - kms_flip.c: Changed test busy_mode boolean into mode_busy bitfield in struct
> data.
> - igt_kms.h: Moved IGT_PLANE_CURSOR position comment.
> 
> Changes since v4:
> - kms_vblank.c: Removed whitespace at end of file.
> - igt_kms: Changed name of helper function to kmstest_get_vbl_flag
> - igt_kms: Changed kmstest_get_vbl_flag to not set DRM_VBLANK_RELATIVE flag
> 
> Robert Foss (7):
>   lib/igt_kms: Add support for up to 10 planes.
>   lib/igt_kms: Fix plane counting in igt_display_init.
>   lib/igt_kms: Switch to verbose assert.
>   lib/igt_kms: Added pipe_id_to_vbl_flag() to igt_kms.
>   kms_vblank: Switch from using crtc0 statically to explicitly setting
>     mode.
>   igt_kms: Change igt_wait_for_vblank to use helper function.
>   kms_flip: Change __wait_for_vblank to use helper function.
> 
>  lib/igt_kms.c      |  36 ++++++++++--
>  lib/igt_kms.h      |   9 ++-
>  tests/kms_flip.c   |   8 +--
>  tests/kms_vblank.c | 161 ++++++++++++++++++++++++++++++++++++++++++++---------
>  4 files changed, 179 insertions(+), 35 deletions(-)
> 
> -- 
> 2.7.4
> 

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH i-g-t v5 0/7] kms_flip_event_leak and kms_vblank fixes for

[Robert Foss]

Hey,

Thanks for being thorough, give me a shout if there is anything specific 
I can help you look into.


Rob.

On 2016-05-24 07:19 AM, Marius Vlad wrote:
> Hi,
>
> I'm getting mixed results w/ this series applied. The kernel seems to
> trip in different ways either from suspend or on reload (running the BAT
> suite on a HSW machine), and though it seems totally
> unrelated I can't reproduce the same behaviour without the series
> applied. Haven't managed to get some valid output as the machine
> dead-locks/freezes, so please be patient until I try to isolate the
> problem. It could be I stumbled something totally unrelated.
>
>
> On Mon, May 16, 2016 at 09:38:25AM -0400, robert.foss@collabora.com wrote:
>> From: Robert Foss <robert.foss@collabora.com>
>>
>> In addition to the changes made in v4, danvet suggested that the argument for
>> the (newly renamed) helper function kmstest_get_vbl_flag should be changed.
>>
>> I investigated it and found that all of the places where the helper function
>> currently is used, would require more of a rework with changed arguments than
>> with unchanged arguments, so I decided to not change the arguments.
>>
>> Changes since v1:
>> - kms_vblank: Removed un-used members of data_t struct.
>> - Rename plane_counter to n_planes.
>> - Removed un-needed handling CURSOR plane location.
>> - Added names for additional planes in update kmstest_plane_name.
>>
>> Changes since v2:
>> - Rebased onto trunk which already contains kms_flip_event_leak changes.
>>
>> Changes since v3:
>> - kms_vblank.c: Removed drm_fd from data_t, to decrease the number of
>> unrelated changes in the patch.
>> - kms_vblank.c: Changed "int valid_tests" to "unsigned valid_tests".
>> - kms_vblank.c: Changed crtc_id_flag/crtc_id_to_flag to
>> pipe_id_flag/pipe_id_to_flag.
>> - kms_vblank.c: Changed to "vbl.request.type |= crtc_id_flag;".
>> - kms_vblank.c: Moved crtc_id_to_flag to igt_kms.c.
>> - kms_vblank.c: Renamed crtc_id_to_flag to pipe_id_to_vbl_flag.
>> - kms_flip.c: Changed test busy_mode boolean into mode_busy bitfield in struct
>> data.
>> - igt_kms.h: Moved IGT_PLANE_CURSOR position comment.
>>
>> Changes since v4:
>> - kms_vblank.c: Removed whitespace at end of file.
>> - igt_kms: Changed name of helper function to kmstest_get_vbl_flag
>> - igt_kms: Changed kmstest_get_vbl_flag to not set DRM_VBLANK_RELATIVE flag
>>
>> Robert Foss (7):
>>    lib/igt_kms: Add support for up to 10 planes.
>>    lib/igt_kms: Fix plane counting in igt_display_init.
>>    lib/igt_kms: Switch to verbose assert.
>>    lib/igt_kms: Added pipe_id_to_vbl_flag() to igt_kms.
>>    kms_vblank: Switch from using crtc0 statically to explicitly setting
>>      mode.
>>    igt_kms: Change igt_wait_for_vblank to use helper function.
>>    kms_flip: Change __wait_for_vblank to use helper function.
>>
>>   lib/igt_kms.c      |  36 ++++++++++--
>>   lib/igt_kms.h      |   9 ++-
>>   tests/kms_flip.c   |   8 +--
>>   tests/kms_vblank.c | 161 ++++++++++++++++++++++++++++++++++++++++++++---------
>>   4 files changed, 179 insertions(+), 35 deletions(-)
>>
>> --
>> 2.7.4
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx