[PATCH i-g-t 0/5] igt/kms: Make fence waiting explicit.

[PATCH i-g-t 0/5] igt/kms: Make fence waiting explicit.

[Maarten Lankhorst]

I wanted to make kms_atomic_transition pass, but the nonblocking modeset
fencing tests were bogus.

This series changes the semantics for fencing slightly. It only keeps the out fences
in pipe_obj->out_fence_fd, it's up to the test to decide what to do with it, which
is probably just waiting on the fd or checking if it completed.

Maarten Lankhorst (5):
  tests/kms_atomic_transition: Add test for plane completion ordering.
  lib/kms: Handle fence interaction correctly WRT TEST_ONLY.
  tests/kms_atomic_transition: Only request fence on enabled pipes
  tests/kms_atomic_transition: Do not clear in-fences after atomic
    commit.
  igt/kms: Do not wait for fence completion during commit

 lib/igt_kms.c                 | 41 ++++++++----------
 tests/kms_atomic_transition.c | 99 +++++++++++++++++++++++++++++++++----------
 2 files changed, 95 insertions(+), 45 deletions(-)

-- 
2.11.0

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

[PATCH i-g-t 1/5] tests/kms_atomic_transition: Add test for plane completion ordering.

[Maarten Lankhorst]

When a plane gets disabled, that commit has to complete before an
atomic commit on that disabled plane only. Else for the old commit,
new_plane_state may have been freed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 tests/kms_atomic_transition.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index ba5cd4d6e289..2b56eef66b7b 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -30,6 +30,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <time.h>
+#include <poll.h>
 
 #ifndef DRM_CAP_CURSOR_WIDTH
 #define DRM_CAP_CURSOR_WIDTH 0x8
@@ -152,6 +153,7 @@ static drmEventContext drm_events = {
 
 enum transition_type {
 	TRANSITION_PLANES,
+	TRANSITION_AFTER_FREE,
 	TRANSITION_MODESET,
 	TRANSITION_MODESET_DISABLE,
 };
@@ -480,6 +482,31 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 			igt_skip("Cannot run tests without proper size sprite planes\n");
 	}
 
+	igt_display_commit2(display, COMMIT_ATOMIC);
+
+	if (type == TRANSITION_AFTER_FREE) {
+		struct pollfd pfd = { display->drm_fd, POLLIN };
+
+		wm_setup_plane(display, pipe, 0, parms);
+
+		atomic_commit(display, pipe, flags, (void *)(unsigned long)0, fencing);
+
+		for_each_plane_on_pipe(display, pipe, plane)
+			plane->fb_changed = true;
+
+		igt_display_commit2(display, COMMIT_ATOMIC);
+
+		/*
+		 * Previous atomic commit should have completed
+		 * before this plane-only atomic commit.
+		 */
+		igt_assert_eq(poll(&pfd, 1, 0), 1);
+
+		drmHandleEvent(display->drm_fd, &drm_events);
+
+		goto cleanup;
+	}
+
 	for (i = 0; i < iter_max; i++) {
 		igt_output_set_pipe(output, pipe);
 
@@ -854,6 +881,10 @@ igt_main
 		for_each_pipe_with_valid_output(&display, pipe, output)
 			run_transition_test(&display, pipe, output, TRANSITION_PLANES, true, true);
 
+	igt_subtest("plane-use-after-nonblocking-unbind")
+		for_each_pipe_with_valid_output(&display, pipe, output)
+			run_transition_test(&display, pipe, output, TRANSITION_AFTER_FREE, true, false);
+
 	igt_subtest("plane-all-modeset-transition")
 		for_each_pipe_with_valid_output(&display, pipe, output)
 			run_transition_test(&display, pipe, output, TRANSITION_MODESET, false, false);
-- 
2.11.0

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

[PATCH i-g-t 2/5] lib/kms: Handle fence interaction correctly WRT TEST_ONLY.

[Maarten Lankhorst]

All other atomic properties are reset in display_commit_changed,
which is the right place because TEST_ONLY commits don't need to
be reset.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 lib/igt_kms.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index faf9df2fcedf..8bf56faf41e9 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -2505,7 +2505,6 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
 	pipe_obj->out_fence_fd = -1;
 	if (pipe_obj->out_fence_requested)
 	{
-		pipe_obj->out_fence_requested = false;
 		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR,
 		    (uint64_t)(uintptr_t) &pipe_obj->out_fence_fd);
 	}
@@ -2586,27 +2585,6 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
 	}
 
 	ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data);
-	if (!ret) {
-
-		for_each_pipe(display, pipe) {
-			igt_pipe_t *pipe_obj = &display->pipes[pipe];
-			igt_plane_t *plane;
-
-			/* reset fence_fd to prevent it from being set for the next commit */
-			for_each_plane_on_pipe(display, pipe, plane) {
-				igt_plane_set_fence_fd(plane, -1);
-			}
-
-			if (pipe_obj->out_fence_fd == -1)
-				continue;
-
-			igt_assert(pipe_obj->out_fence_fd >= 0);
-			ret = sync_fence_wait(pipe_obj->out_fence_fd, 1000);
-			igt_assert(ret == 0);
-			close(pipe_obj->out_fence_fd);
-			pipe_obj->out_fence_fd = -1;
-		}
-	}
 
 	drmModeAtomicFree(req);
 	return ret;
@@ -2629,6 +2607,18 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
 		if (s != COMMIT_UNIVERSAL)
 			pipe_obj->mode_changed = false;
 
+		if (s == COMMIT_ATOMIC) {
+			pipe_obj->out_fence_requested = false;
+
+			if (pipe_obj->out_fence_fd == -1)
+				continue;
+
+			igt_assert(pipe_obj->out_fence_fd >= 0);
+			igt_assert_eq(sync_fence_wait(pipe_obj->out_fence_fd, 1000), 0);
+			close(pipe_obj->out_fence_fd);
+			pipe_obj->out_fence_fd = -1;
+		}
+
 		for_each_plane_on_pipe(display, pipe, plane) {
 			plane->fb_changed = false;
 			plane->position_changed = false;
@@ -2638,6 +2628,10 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
 			    !(plane->type == DRM_PLANE_TYPE_PRIMARY ||
 			      plane->type == DRM_PLANE_TYPE_CURSOR))
 				plane->rotation_changed = false;
+
+			if (s == COMMIT_ATOMIC)
+				/* reset fence_fd to prevent it from being set for the next commit */
+				igt_plane_set_fence_fd(plane, -1);
 		}
 	}
 
-- 
2.11.0

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

[PATCH i-g-t 3/5] tests/kms_atomic_transition: Only request fence on enabled pipes

[Maarten Lankhorst]

Trying to set a fence on disabled pipes will be rejected by the kernel:
[ 1275.865029] [drm:drm_atomic_check_only [drm]] [CRTC:39:pipe B] requesting event but off

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99911
---
 tests/kms_atomic_transition.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 2b56eef66b7b..a2470c0c3571 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -717,11 +717,11 @@ static void run_modeset_tests(igt_display_t *display, int howmany, bool nonblock
 			igt_plane_set_fb(plane, &fbs[1]);
 			igt_fb_set_size(&fbs[1], plane, mode->hdisplay, mode->vdisplay);
 			igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
+
+			if (fencing)
+				igt_pipe_request_out_fence(&display->pipes[i]);
 		} else
 			igt_plane_set_fb(plane, NULL);
-
-		if(fencing)
-			igt_pipe_request_out_fence(&display->pipes[i]);
 	}
 
 	/*
-- 
2.11.0

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

[PATCH i-g-t 4/5] tests/kms_atomic_transition: Do not clear in-fences after atomic commit.

[Maarten Lankhorst]

This is already done in igt_display_commit* functions.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 tests/kms_atomic_transition.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index a2470c0c3571..6e2b25da75db 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -367,14 +367,6 @@ static void configure_fencing(igt_display_t *display, enum pipe pipe)
 	}
 }
 
-static void clear_fencing(igt_display_t *display, enum pipe pipe)
-{
-	igt_plane_t *plane;
-
-	for_each_plane_on_pipe(display, pipe, plane)
-		igt_plane_set_fence_fd(plane, -1);
-}
-
 static void atomic_commit(igt_display_t *display, enum pipe pipe, unsigned int flags, void *data, bool fencing)
 {
 	if (fencing) {
@@ -383,9 +375,6 @@ static void atomic_commit(igt_display_t *display, enum pipe pipe, unsigned int f
 	}
 
 	igt_display_commit_atomic(display, flags, data);
-
-	if (fencing)
-		clear_fencing(display, pipe);
 }
 
 /*
-- 
2.11.0

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

[PATCH i-g-t 5/5] igt/kms: Do not wait for fence completion during commit

[Maarten Lankhorst]

This will make the IGT tests that use fences more useful, since they can
perform the waiting themselves when required.

To celebrate, also add plane-use-after-nonblocking-unbind-fencing,
the fence version of plane-use-after-nonblocking-unbind.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 lib/igt_kms.c                 | 19 ++++++------
 tests/kms_atomic_transition.c | 71 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 63 insertions(+), 27 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 8bf56faf41e9..6390229f1546 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1698,6 +1698,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 		pipe->plane_cursor = -1;
 		pipe->plane_primary = -1;
 		pipe->planes = NULL;
+		pipe->out_fence_fd = -1;
 
 		get_crtc_property(display->drm_fd, pipe->crtc_id,
 				    "background_color",
@@ -1913,6 +1914,9 @@ static void igt_pipe_fini(igt_pipe_t *pipe)
 
 	free(pipe->planes);
 	pipe->planes = NULL;
+
+	if (pipe->out_fence_fd != -1)
+		close(pipe->out_fence_fd);
 }
 
 static void igt_output_fini(igt_output_t *output)
@@ -2502,7 +2506,11 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
 		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, !!output);
 	}
 
-	pipe_obj->out_fence_fd = -1;
+	if (pipe_obj->out_fence_fd != -1) {
+		close(pipe_obj->out_fence_fd);
+		pipe_obj->out_fence_fd = -1;
+	}
+
 	if (pipe_obj->out_fence_requested)
 	{
 		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR,
@@ -2607,16 +2615,9 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
 		if (s != COMMIT_UNIVERSAL)
 			pipe_obj->mode_changed = false;
 
-		if (s == COMMIT_ATOMIC) {
+		if (s == COMMIT_ATOMIC && pipe_obj->out_fence_requested) {
 			pipe_obj->out_fence_requested = false;
-
-			if (pipe_obj->out_fence_fd == -1)
-				continue;
-
 			igt_assert(pipe_obj->out_fence_fd >= 0);
-			igt_assert_eq(sync_fence_wait(pipe_obj->out_fence_fd, 1000), 0);
-			close(pipe_obj->out_fence_fd);
-			pipe_obj->out_fence_fd = -1;
 		}
 
 		for_each_plane_on_pipe(display, pipe, plane) {
diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 6e2b25da75db..e22763bdf94b 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -377,6 +377,31 @@ static void atomic_commit(igt_display_t *display, enum pipe pipe, unsigned int f
 	igt_display_commit_atomic(display, flags, data);
 }
 
+static int fd_completed(int fd)
+{
+	struct pollfd pfd = { fd, POLLIN };
+	int ret;
+
+	ret = poll(&pfd, 1, 0);
+	igt_assert(ret >= 0);
+	return ret;
+}
+
+static void wait_for_transition(igt_display_t *display, enum pipe pipe, bool nonblocking, bool fencing)
+{
+	if (fencing) {
+		int fence_fd = display->pipes[pipe].out_fence_fd;
+
+		igt_assert_neq(fd_completed(fence_fd), nonblocking);
+
+		igt_assert(sync_fence_wait(fence_fd, 30000) == 0);
+	} else {
+		igt_assert_neq(fd_completed(display->drm_fd), nonblocking);
+
+		drmHandleEvent(display->drm_fd, &drm_events);
+	}
+}
+
 /*
  * 1. Set primary plane to a known fb.
  * 2. Make sure getcrtc returns the correct fb id.
@@ -393,14 +418,17 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 	struct igt_fb fb, argb_fb, sprite_fb;
 	drmModeModeInfo *mode, override_mode;
 	igt_plane_t *plane;
-	uint32_t iter_max = 1 << display->pipes[pipe].n_planes, i;
-	struct plane_parms parms[display->pipes[pipe].n_planes];
+	igt_pipe_t *pipe_obj = &display->pipes[pipe];
+	uint32_t iter_max = 1 << pipe_obj->n_planes, i;
+	struct plane_parms parms[pipe_obj->n_planes];
 	bool skip_test = false;
-	unsigned flags = DRM_MODE_PAGE_FLIP_EVENT;
+	unsigned flags = 0;
 	int ret;
 
 	if (fencing)
 		prepare_fencing(display, pipe);
+	else
+		flags |= DRM_MODE_PAGE_FLIP_EVENT;
 
 	if (nonblocking)
 		flags |= DRM_MODE_ATOMIC_NONBLOCK;
@@ -444,10 +472,10 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 		wm_setup_plane(display, pipe, iter_max - 1, parms);
 
 		if (fencing)
-			igt_pipe_request_out_fence(&display->pipes[pipe]);
+			igt_pipe_request_out_fence(pipe_obj);
 
 		ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
-		if (ret != -EINVAL || display->pipes[pipe].n_planes < 3)
+		if (ret != -EINVAL || pipe_obj->n_planes < 3)
 			break;
 
 		ret = 0;
@@ -474,25 +502,28 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 	igt_display_commit2(display, COMMIT_ATOMIC);
 
 	if (type == TRANSITION_AFTER_FREE) {
-		struct pollfd pfd = { display->drm_fd, POLLIN };
+		int fence_fd = -1;
 
 		wm_setup_plane(display, pipe, 0, parms);
 
 		atomic_commit(display, pipe, flags, (void *)(unsigned long)0, fencing);
+		if (fencing) {
+			fence_fd = pipe_obj->out_fence_fd;
+			pipe_obj->out_fence_fd = -1;
+		}
 
 		for_each_plane_on_pipe(display, pipe, plane)
 			plane->fb_changed = true;
 
 		igt_display_commit2(display, COMMIT_ATOMIC);
 
-		/*
-		 * Previous atomic commit should have completed
-		 * before this plane-only atomic commit.
-		 */
-		igt_assert_eq(poll(&pfd, 1, 0), 1);
-
-		drmHandleEvent(display->drm_fd, &drm_events);
-
+		if (fence_fd != -1) {
+			igt_assert(fd_completed(fence_fd));
+			close(fence_fd);
+		} else {
+			igt_assert(fd_completed(display->drm_fd));
+			wait_for_transition(display, pipe, false, fencing);
+		}
 		goto cleanup;
 	}
 
@@ -502,7 +533,7 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 		wm_setup_plane(display, pipe, i, parms);
 
 		atomic_commit(display, pipe, flags, (void *)(unsigned long)i, fencing);
-		drmHandleEvent(display->drm_fd, &drm_events);
+		wait_for_transition(display, pipe, nonblocking, fencing);
 
 		if (type == TRANSITION_MODESET_DISABLE) {
 			igt_output_set_pipe(output, PIPE_NONE);
@@ -510,7 +541,7 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 			wm_setup_plane(display, pipe, 0, parms);
 
 			atomic_commit(display, pipe, flags, (void *) 0UL, fencing);
-			drmHandleEvent(display->drm_fd, &drm_events);
+			wait_for_transition(display, pipe, nonblocking, fencing);
 		} else {
 			uint32_t j;
 
@@ -522,14 +553,14 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 					igt_output_override_mode(output, &override_mode);
 
 				atomic_commit(display, pipe, flags, (void *)(unsigned long) j, fencing);
-				drmHandleEvent(display->drm_fd, &drm_events);
+				wait_for_transition(display, pipe, nonblocking, fencing);
 
 				wm_setup_plane(display, pipe, i, parms);
 				if (type == TRANSITION_MODESET)
 					igt_output_override_mode(output, NULL);
 
 				atomic_commit(display, pipe, flags, (void *)(unsigned long) i, fencing);
-				drmHandleEvent(display->drm_fd, &drm_events);
+				wait_for_transition(display, pipe, nonblocking, fencing);
 			}
 		}
 	}
@@ -874,6 +905,10 @@ igt_main
 		for_each_pipe_with_valid_output(&display, pipe, output)
 			run_transition_test(&display, pipe, output, TRANSITION_AFTER_FREE, true, false);
 
+	igt_subtest("plane-use-after-nonblocking-unbind-fencing")
+		for_each_pipe_with_valid_output(&display, pipe, output)
+			run_transition_test(&display, pipe, output, TRANSITION_AFTER_FREE, true, true);
+
 	igt_subtest("plane-all-modeset-transition")
 		for_each_pipe_with_valid_output(&display, pipe, output)
 			run_transition_test(&display, pipe, output, TRANSITION_MODESET, false, false);
-- 
2.11.0

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

Re: [PATCH i-g-t 0/5] igt/kms: Make fence waiting explicit.

[Gustavo Padovan]

Hi Maarten,

2017-07-06 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>:

> I wanted to make kms_atomic_transition pass, but the nonblocking modeset
> fencing tests were bogus.
> 
> This series changes the semantics for fencing slightly. It only keeps the out fences
> in pipe_obj->out_fence_fd, it's up to the test to decide what to do with it, which
> is probably just waiting on the fd or checking if it completed.
> 
> Maarten Lankhorst (5):
>   tests/kms_atomic_transition: Add test for plane completion ordering.
>   lib/kms: Handle fence interaction correctly WRT TEST_ONLY.
>   tests/kms_atomic_transition: Only request fence on enabled pipes
>   tests/kms_atomic_transition: Do not clear in-fences after atomic
>     commit.
>   igt/kms: Do not wait for fence completion during commit
> 
>  lib/igt_kms.c                 | 41 ++++++++----------
>  tests/kms_atomic_transition.c | 99 +++++++++++++++++++++++++++++++++----------
>  2 files changed, 95 insertions(+), 45 deletions(-)

This make fencing a lot cleaner.

For the whole series:

Acked-by: Gustavo Padovan <gustavo.padovan@collabora.com>

Gustavo

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

Re: [PATCH i-g-t 0/5] igt/kms: Make fence waiting explicit.

[Maarten Lankhorst]

Op 07-07-17 om 02:15 schreef Gustavo Padovan:
> Hi Maarten,
>
> 2017-07-06 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>:
>
>> I wanted to make kms_atomic_transition pass, but the nonblocking modeset
>> fencing tests were bogus.
>>
>> This series changes the semantics for fencing slightly. It only keeps the out fences
>> in pipe_obj->out_fence_fd, it's up to the test to decide what to do with it, which
>> is probably just waiting on the fd or checking if it completed.
>>
>> Maarten Lankhorst (5):
>>   tests/kms_atomic_transition: Add test for plane completion ordering.
>>   lib/kms: Handle fence interaction correctly WRT TEST_ONLY.
>>   tests/kms_atomic_transition: Only request fence on enabled pipes
>>   tests/kms_atomic_transition: Do not clear in-fences after atomic
>>     commit.
>>   igt/kms: Do not wait for fence completion during commit
>>
>>  lib/igt_kms.c                 | 41 ++++++++----------
>>  tests/kms_atomic_transition.c | 99 +++++++++++++++++++++++++++++++++----------
>>  2 files changed, 95 insertions(+), 45 deletions(-)
> This make fencing a lot cleaner.
>
> For the whole series:
>
> Acked-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>
> Gustavo
>
Thanks, pushed. :)

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