[PATCH i-g-t] tests/kms_vblank: Add test to ensure DRM_CAP_CRTC_IN_VBLANK_EVENT works correctly

[PATCH i-g-t] tests/kms_vblank: Add test to ensure DRM_CAP_CRTC_IN_VBLANK_EVENT works correctly

[Maarten Lankhorst]

This was implemented correctly only on the atomic ioctl before, but
it should really be working on all 3 ioctl's involved, so ensure we
always set crtc_id correctly with a testcase.

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

diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
index 47fd10fb9078..004f0e6104ee 100644
--- a/tests/kms_vblank.c
+++ b/tests/kms_vblank.c
@@ -121,7 +121,6 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int))
 	igt_display_t *display = &data->display;
 	igt_output_t *output;
 	enum pipe p;
-	unsigned int valid_tests = 0;
 
 	for_each_pipe_with_valid_output(display, p, output) {
 		igt_hang_t hang;
@@ -170,11 +169,60 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int))
 
 		/* cleanup what prepare_crtc() has done */
 		cleanup_crtc(data, fd, output);
-		valid_tests++;
 	}
+}
+
+static void crtc_id_subtest(data_t *data, int fd)
+{
+	igt_display_t *display = &data->display;
+	igt_output_t *output;
+	enum pipe p;
+
+	for_each_pipe_with_valid_output(display, p, output) {
+		struct drm_event_vblank buf;
+		const uint32_t pipe_id_flag = kmstest_get_vbl_flag(p);
+		unsigned crtc_id, expected_crtc_id;
+		uint64_t val;
+		union drm_wait_vblank vbl;
+
+		crtc_id = display->pipes[p].crtc_id;
+		if (drmGetCap(display->drm_fd, DRM_CAP_CRTC_IN_VBLANK_EVENT, &val) == 0)
+			expected_crtc_id = crtc_id;
+		else
+			expected_crtc_id = 0;
+
+		data->pipe = p;
+		prepare_crtc(data, fd, output);
+
+		memset(&vbl, 0, sizeof(vbl));
+		vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
+		vbl.request.type |= pipe_id_flag;
+		vbl.request.sequence = 1;
+		igt_assert_eq(wait_vblank(fd, &vbl), 0);
+
+		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
+		igt_assert_eq(buf.crtc_id, expected_crtc_id);
+
+		do_or_die(drmModePageFlip(fd, crtc_id,
+					  data->primary_fb.fb_id,
+					  DRM_MODE_PAGE_FLIP_EVENT, NULL));
+
+		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
+		igt_assert_eq(buf.crtc_id, expected_crtc_id);
+
+		if (display->is_atomic) {
+			igt_plane_t *primary = igt_output_get_plane(output, 0);
+
+			igt_plane_set_fb(primary, &data->primary_fb);
+			igt_display_commit_atomic(display, DRM_MODE_PAGE_FLIP_EVENT, NULL);
 
-	igt_require_f(valid_tests,
-		      "no valid crtc/connector combinations found\n");
+			igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
+			igt_assert_eq(buf.crtc_id, expected_crtc_id);
+		}
+
+		cleanup_crtc(data, fd, output);
+		return;
+	}
 }
 
 static void accuracy(data_t *data, int fd, int nchildren)
@@ -307,8 +355,12 @@ igt_main
 		fd = drm_open_driver(DRIVER_ANY);
 		kmstest_set_vt_graphics_mode();
 		igt_display_init(&data.display, fd);
+		igt_display_require_output(&data.display);
 	}
 
+	igt_subtest("crtc-id")
+		crtc_id_subtest(&data, fd);
+
 	for (f = funcs; f->name; f++) {
 		for (m = modes; m->name; m++) {
 			if (m->flags & ~f->valid)
-- 
2.15.0

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

✗ Fi.CI.BAT: failure for tests/kms_vblank: Add test to ensure DRM_CAP_CRTC_IN_VBLANK_EVENT works correctly

[Patchwork]

== Series Details ==

Series: tests/kms_vblank: Add test to ensure DRM_CAP_CRTC_IN_VBLANK_EVENT works correctly
URL   : https://patchwork.freedesktop.org/series/34299/
State : failure

== Summary ==

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

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

Re: [PATCH i-g-t] tests/kms_vblank: Add test to ensure DRM_CAP_CRTC_IN_VBLANK_EVENT works correctly

[Daniel Vetter]

On Thu, Nov 23, 2017 at 01:26:14PM +0100, Maarten Lankhorst wrote:
> This was implemented correctly only on the atomic ioctl before, but
> it should really be working on all 3 ioctl's involved, so ensure we
> always set crtc_id correctly with a testcase.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

We seemt to completely lack these checks for the vblank ioctl and the
atomic ioctl too. Can you pls fill these gaps (probably best in kms_flip.c
and kms_atomic.c), too?

> ---
>  tests/kms_vblank.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
> index 47fd10fb9078..004f0e6104ee 100644
> --- a/tests/kms_vblank.c
> +++ b/tests/kms_vblank.c
> @@ -121,7 +121,6 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int))
>  	igt_display_t *display = &data->display;
>  	igt_output_t *output;
>  	enum pipe p;
> -	unsigned int valid_tests = 0;
>  
>  	for_each_pipe_with_valid_output(display, p, output) {
>  		igt_hang_t hang;
> @@ -170,11 +169,60 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int))
>  
>  		/* cleanup what prepare_crtc() has done */
>  		cleanup_crtc(data, fd, output);
> -		valid_tests++;
>  	}
> +}
> +
> +static void crtc_id_subtest(data_t *data, int fd)
> +{
> +	igt_display_t *display = &data->display;
> +	igt_output_t *output;
> +	enum pipe p;
> +
> +	for_each_pipe_with_valid_output(display, p, output) {
> +		struct drm_event_vblank buf;
> +		const uint32_t pipe_id_flag = kmstest_get_vbl_flag(p);
> +		unsigned crtc_id, expected_crtc_id;
> +		uint64_t val;
> +		union drm_wait_vblank vbl;
> +
> +		crtc_id = display->pipes[p].crtc_id;
> +		if (drmGetCap(display->drm_fd, DRM_CAP_CRTC_IN_VBLANK_EVENT, &val) == 0)
> +			expected_crtc_id = crtc_id;
> +		else
> +			expected_crtc_id = 0;
> +
> +		data->pipe = p;
> +		prepare_crtc(data, fd, output);
> +
> +		memset(&vbl, 0, sizeof(vbl));
> +		vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
> +		vbl.request.type |= pipe_id_flag;
> +		vbl.request.sequence = 1;
> +		igt_assert_eq(wait_vblank(fd, &vbl), 0);
> +
> +		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
> +		igt_assert_eq(buf.crtc_id, expected_crtc_id);
> +
> +		do_or_die(drmModePageFlip(fd, crtc_id,
> +					  data->primary_fb.fb_id,
> +					  DRM_MODE_PAGE_FLIP_EVENT, NULL));
> +
> +		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
> +		igt_assert_eq(buf.crtc_id, expected_crtc_id);
> +
> +		if (display->is_atomic) {
> +			igt_plane_t *primary = igt_output_get_plane(output, 0);
> +
> +			igt_plane_set_fb(primary, &data->primary_fb);
> +			igt_display_commit_atomic(display, DRM_MODE_PAGE_FLIP_EVENT, NULL);
>  
> -	igt_require_f(valid_tests,
> -		      "no valid crtc/connector combinations found\n");
> +			igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
> +			igt_assert_eq(buf.crtc_id, expected_crtc_id);
> +		}
> +
> +		cleanup_crtc(data, fd, output);
> +		return;
> +	}
>  }
>  
>  static void accuracy(data_t *data, int fd, int nchildren)
> @@ -307,8 +355,12 @@ igt_main
>  		fd = drm_open_driver(DRIVER_ANY);
>  		kmstest_set_vt_graphics_mode();
>  		igt_display_init(&data.display, fd);
> +		igt_display_require_output(&data.display);
>  	}
>  
> +	igt_subtest("crtc-id")
> +		crtc_id_subtest(&data, fd);

Either I'm stupid, or this doesn't apply on top of latest igt master.

Either way I think taking the expensive modesets out of individual tests
would be good, so that when we run entire binaries in CI, we'll benefit
from some good speedup. Which is the plan, now that machines are a bit
more stable ...
-Daniel
> +
>  	for (f = funcs; f->name; f++) {
>  		for (m = modes; m->name; m++) {
>  			if (m->flags & ~f->valid)
> -- 
> 2.15.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH i-g-t] tests/kms_vblank: Add test to ensure DRM_CAP_CRTC_IN_VBLANK_EVENT works correctly

[Maarten Lankhorst]

Op 24-11-17 om 16:03 schreef Daniel Vetter:
> On Thu, Nov 23, 2017 at 01:26:14PM +0100, Maarten Lankhorst wrote:
>> This was implemented correctly only on the atomic ioctl before, but
>> it should really be working on all 3 ioctl's involved, so ensure we
>> always set crtc_id correctly with a testcase.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> We seemt to completely lack these checks for the vblank ioctl and the
> atomic ioctl too. Can you pls fill these gaps (probably best in kms_flip.c
> and kms_atomic.c), too?
Summary is a bit out of date, but I did add them in here. :)
>> ---
>>  tests/kms_vblank.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 56 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
>> index 47fd10fb9078..004f0e6104ee 100644
>> --- a/tests/kms_vblank.c
>> +++ b/tests/kms_vblank.c
>> @@ -121,7 +121,6 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int))
>>  	igt_display_t *display = &data->display;
>>  	igt_output_t *output;
>>  	enum pipe p;
>> -	unsigned int valid_tests = 0;
>>  
>>  	for_each_pipe_with_valid_output(display, p, output) {
>>  		igt_hang_t hang;
>> @@ -170,11 +169,60 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int))
>>  
>>  		/* cleanup what prepare_crtc() has done */
>>  		cleanup_crtc(data, fd, output);
>> -		valid_tests++;
>>  	}
>> +}
>> +
>> +static void crtc_id_subtest(data_t *data, int fd)
>> +{
>> +	igt_display_t *display = &data->display;
>> +	igt_output_t *output;
>> +	enum pipe p;
>> +
>> +	for_each_pipe_with_valid_output(display, p, output) {
>> +		struct drm_event_vblank buf;
>> +		const uint32_t pipe_id_flag = kmstest_get_vbl_flag(p);
>> +		unsigned crtc_id, expected_crtc_id;
>> +		uint64_t val;
>> +		union drm_wait_vblank vbl;
>> +
>> +		crtc_id = display->pipes[p].crtc_id;
>> +		if (drmGetCap(display->drm_fd, DRM_CAP_CRTC_IN_VBLANK_EVENT, &val) == 0)
>> +			expected_crtc_id = crtc_id;
>> +		else
>> +			expected_crtc_id = 0;
>> +
>> +		data->pipe = p;
>> +		prepare_crtc(data, fd, output);
>> +
>> +		memset(&vbl, 0, sizeof(vbl));
>> +		vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
>> +		vbl.request.type |= pipe_id_flag;
>> +		vbl.request.sequence = 1;
>> +		igt_assert_eq(wait_vblank(fd, &vbl), 0);
>> +
>> +		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
>> +		igt_assert_eq(buf.crtc_id, expected_crtc_id);
>> +
>> +		do_or_die(drmModePageFlip(fd, crtc_id,
>> +					  data->primary_fb.fb_id,
>> +					  DRM_MODE_PAGE_FLIP_EVENT, NULL));
>> +
>> +		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
>> +		igt_assert_eq(buf.crtc_id, expected_crtc_id);
>> +
>> +		if (display->is_atomic) {
>> +			igt_plane_t *primary = igt_output_get_plane(output, 0);
>> +
>> +			igt_plane_set_fb(primary, &data->primary_fb);
>> +			igt_display_commit_atomic(display, DRM_MODE_PAGE_FLIP_EVENT, NULL);
>>  
>> -	igt_require_f(valid_tests,
>> -		      "no valid crtc/connector combinations found\n");
>> +			igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
>> +			igt_assert_eq(buf.crtc_id, expected_crtc_id);
>> +		}
>> +
>> +		cleanup_crtc(data, fd, output);
>> +		return;
>> +	}
>>  }
>>  
>>  static void accuracy(data_t *data, int fd, int nchildren)
>> @@ -307,8 +355,12 @@ igt_main
>>  		fd = drm_open_driver(DRIVER_ANY);
>>  		kmstest_set_vt_graphics_mode();
>>  		igt_display_init(&data.display, fd);
>> +		igt_display_require_output(&data.display);
>>  	}
>>  
>> +	igt_subtest("crtc-id")
>> +		crtc_id_subtest(&data, fd);
> Either I'm stupid, or this doesn't apply on top of latest igt master.
>
> Either way I think taking the expensive modesets out of individual tests
> would be good, so that when we run entire binaries in CI, we'll benefit
> from some good speedup. Which is the plan, now that machines are a bit
> more stable ...
This is my intention of speeding up IGT as well, tests that don't care can use
the inherited pipe/connector. It will speed up testing a lot by preventing even
performing a modeset.

I also plan to add a function to read the current state through igt_display_read_hw_state(),
then find an active pipe/connector, or just the first combination if nothing matches.
And after the end of the test keep it alive without disabling the crtc's, only removing framebuffers,
which no longer disables the crtc's on i915. :-)

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

Re: [PATCH i-g-t] tests/kms_vblank: Add test to ensure DRM_CAP_CRTC_IN_VBLANK_EVENT works correctly

[Daniel Vetter]

On Fri, Nov 24, 2017 at 04:35:10PM +0100, Maarten Lankhorst wrote:
> Op 24-11-17 om 16:03 schreef Daniel Vetter:
> > On Thu, Nov 23, 2017 at 01:26:14PM +0100, Maarten Lankhorst wrote:
> >> This was implemented correctly only on the atomic ioctl before, but
> >> it should really be working on all 3 ioctl's involved, so ensure we
> >> always set crtc_id correctly with a testcase.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > We seemt to completely lack these checks for the vblank ioctl and the
> > atomic ioctl too. Can you pls fill these gaps (probably best in kms_flip.c
> > and kms_atomic.c), too?
> Summary is a bit out of date, but I did add them in here. :)

Oh was blind.

> >> ---
> >>  tests/kms_vblank.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++----
> >>  1 file changed, 56 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
> >> index 47fd10fb9078..004f0e6104ee 100644
> >> --- a/tests/kms_vblank.c
> >> +++ b/tests/kms_vblank.c
> >> @@ -121,7 +121,6 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int))
> >>  	igt_display_t *display = &data->display;
> >>  	igt_output_t *output;
> >>  	enum pipe p;
> >> -	unsigned int valid_tests = 0;
> >>  
> >>  	for_each_pipe_with_valid_output(display, p, output) {
> >>  		igt_hang_t hang;
> >> @@ -170,11 +169,60 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int))
> >>  
> >>  		/* cleanup what prepare_crtc() has done */
> >>  		cleanup_crtc(data, fd, output);
> >> -		valid_tests++;
> >>  	}
> >> +}
> >> +
> >> +static void crtc_id_subtest(data_t *data, int fd)
> >> +{
> >> +	igt_display_t *display = &data->display;
> >> +	igt_output_t *output;
> >> +	enum pipe p;
> >> +
> >> +	for_each_pipe_with_valid_output(display, p, output) {
> >> +		struct drm_event_vblank buf;
> >> +		const uint32_t pipe_id_flag = kmstest_get_vbl_flag(p);
> >> +		unsigned crtc_id, expected_crtc_id;
> >> +		uint64_t val;
> >> +		union drm_wait_vblank vbl;
> >> +
> >> +		crtc_id = display->pipes[p].crtc_id;
> >> +		if (drmGetCap(display->drm_fd, DRM_CAP_CRTC_IN_VBLANK_EVENT, &val) == 0)
> >> +			expected_crtc_id = crtc_id;
> >> +		else
> >> +			expected_crtc_id = 0;
> >> +
> >> +		data->pipe = p;
> >> +		prepare_crtc(data, fd, output);
> >> +
> >> +		memset(&vbl, 0, sizeof(vbl));
> >> +		vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
> >> +		vbl.request.type |= pipe_id_flag;
> >> +		vbl.request.sequence = 1;
> >> +		igt_assert_eq(wait_vblank(fd, &vbl), 0);
> >> +
> >> +		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
> >> +		igt_assert_eq(buf.crtc_id, expected_crtc_id);
> >> +
> >> +		do_or_die(drmModePageFlip(fd, crtc_id,
> >> +					  data->primary_fb.fb_id,
> >> +					  DRM_MODE_PAGE_FLIP_EVENT, NULL));
> >> +
> >> +		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
> >> +		igt_assert_eq(buf.crtc_id, expected_crtc_id);
> >> +
> >> +		if (display->is_atomic) {
> >> +			igt_plane_t *primary = igt_output_get_plane(output, 0);
> >> +
> >> +			igt_plane_set_fb(primary, &data->primary_fb);
> >> +			igt_display_commit_atomic(display, DRM_MODE_PAGE_FLIP_EVENT, NULL);
> >>  
> >> -	igt_require_f(valid_tests,
> >> -		      "no valid crtc/connector combinations found\n");
> >> +			igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
> >> +			igt_assert_eq(buf.crtc_id, expected_crtc_id);
> >> +		}
> >> +
> >> +		cleanup_crtc(data, fd, output);
> >> +		return;
> >> +	}
> >>  }
> >>  
> >>  static void accuracy(data_t *data, int fd, int nchildren)
> >> @@ -307,8 +355,12 @@ igt_main
> >>  		fd = drm_open_driver(DRIVER_ANY);
> >>  		kmstest_set_vt_graphics_mode();
> >>  		igt_display_init(&data.display, fd);
> >> +		igt_display_require_output(&data.display);
> >>  	}
> >>  
> >> +	igt_subtest("crtc-id")
> >> +		crtc_id_subtest(&data, fd);
> > Either I'm stupid, or this doesn't apply on top of latest igt master.
> >
> > Either way I think taking the expensive modesets out of individual tests
> > would be good, so that when we run entire binaries in CI, we'll benefit
> > from some good speedup. Which is the plan, now that machines are a bit
> > more stable ...
> This is my intention of speeding up IGT as well, tests that don't care can use
> the inherited pipe/connector. It will speed up testing a lot by preventing even
> performing a modeset.
> 
> I also plan to add a function to read the current state through igt_display_read_hw_state(),
> then find an active pipe/connector, or just the first combination if nothing matches.
> And after the end of the test keep it alive without disabling the crtc's, only removing framebuffers,
> which no longer disables the crtc's on i915. :-)

Hm, for many things we do want to test different CRTC (e.g. this exact
test here), and then modesets are fairly unavoidable. But if we batch them
up (and create e.g. pipe-* subtests so we can batch them up without
modesets) we should get mostly there.

Either way, with my blindness rectified, and if you pimp the commit
message a bit:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> ~Maarten

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel