[Intel-gfx] [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 at 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 at ffwll.ch>

> 
> ~Maarten

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch