Re: [PATCH i-g-t v2 2/6] kms_vblank: Reorganize subtests by pipe

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jan 10, 2018 at 11:59:34AM +0100, Maarten Lankhorst wrote:
> Op 10-01-18 om 10:04 schreef Daniel Vetter:
> > On Wed, Jan 10, 2018 at 09:57:47AM +0100, Daniel Vetter wrote:
> >> On Thu, Jan 04, 2018 at 03:12:39PM +0100, Maarten Lankhorst wrote:
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> ---
> >>>  tests/kms_vblank.c | 93 ++++++++++++++++++++++++++++--------------------------
> >>>  1 file changed, 49 insertions(+), 44 deletions(-)
> >>>
> >>> diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
> >>> index 342e4ef23b97..e51e96c7f061 100644
> >>> --- a/tests/kms_vblank.c
> >>> +++ b/tests/kms_vblank.c
> >>> @@ -118,49 +118,41 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int))
> >>>  	int nchildren =
> >>>  		data->flags & FORKED ? sysconf(_SC_NPROCESSORS_ONLN) : 1;
> >>>  	igt_display_t *display = &data->display;
> >>> -	igt_output_t *output;
> >>> -	enum pipe p;
> >>> +	igt_output_t *output = data->output;
> >>>  
> >>> -	for_each_pipe_with_valid_output(display, p, output) {
> >>> -		data->pipe = p;
> >>> -		prepare_crtc(data, fd, output);
> >>> +	prepare_crtc(data, fd, output);
> >>>  
> >>> -		igt_info("Beginning %s on pipe %s, connector %s (%d threads)\n",
> >>> -			 igt_subtest_name(),
> >>> -			 kmstest_pipe_name(data->pipe),
> >>> -			 igt_output_name(output),
> >>> -			 nchildren);
> >>> -
> >>> -		if (data->flags & BUSY) {
> >>> -			union drm_wait_vblank vbl;
> >>> -
> >>> -			memset(&vbl, 0, sizeof(vbl));
> >>> -			vbl.request.type =
> >>> -				DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
> >>> -			vbl.request.type |= kmstest_get_vbl_flag(data->pipe);
> >>> -			vbl.request.sequence = 120 + 12;
> >>> -			igt_assert_eq(wait_vblank(fd, &vbl), 0);
> >>> -		}
> >>> +	igt_info("Beginning %s on pipe %s, connector %s (%d threads)\n",
> >>> +		 igt_subtest_name(), kmstest_pipe_name(data->pipe),
> >>> +		 igt_output_name(output), nchildren);
> >>>  
> >>> -		igt_fork(child, nchildren)
> >>> -			testfunc(data, fd, nchildren);
> >>> -		igt_waitchildren();
> >>> -
> >>> -		if (data->flags & BUSY) {
> >>> -			struct drm_event_vblank buf;
> >>> -			igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
> >>> -		}
> >>> +	if (data->flags & BUSY) {
> >>> +		union drm_wait_vblank vbl;
> >>>  
> >>> -		igt_assert(poll(&(struct pollfd){fd, POLLIN}, 1, 0) == 0);
> >>> +		memset(&vbl, 0, sizeof(vbl));
> >>> +		vbl.request.type =
> >>> +			DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
> >>> +		vbl.request.type |= kmstest_get_vbl_flag(data->pipe);
> >>> +		vbl.request.sequence = 120 + 12;
> >>> +		igt_assert_eq(wait_vblank(fd, &vbl), 0);
> >>> +	}
> >>>  
> >>> -		igt_info("\n%s on pipe %s, connector %s: PASSED\n\n",
> >>> -			 igt_subtest_name(),
> >>> -			 kmstest_pipe_name(data->pipe),
> >>> -			 igt_output_name(output));
> >>> +	igt_fork(child, nchildren)
> >>> +		testfunc(data, fd, nchildren);
> >>> +	igt_waitchildren();
> >>>  
> >>> -		/* cleanup what prepare_crtc() has done */
> >>> -		cleanup_crtc(data, fd, output);
> >>> +	if (data->flags & BUSY) {
> >>> +		struct drm_event_vblank buf;
> >>> +		igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
> >>>  	}
> >>> +
> >>> +	igt_assert(poll(&(struct pollfd){fd, POLLIN}, 1, 0) == 0);
> >>> +
> >>> +	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);
> >>>  }
> >>>  
> >>>  static void crtc_id_subtest(data_t *data, int fd)
> >>> @@ -329,6 +321,8 @@ igt_main
> >>>  		{ "wait", vblank_wait, IDLE | FORKED | BUSY },
> >>>  		{ }
> >>>  	}, *f;
> >>> +	enum pipe p;
> >>> +
> >>>  	const struct {
> >>>  		const char *name;
> >>>  		unsigned int flags;
> >>> @@ -352,14 +346,25 @@ igt_main
> >>>  	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)
> >>> -				continue;
> >>> -
> >>> -			igt_subtest_f("%s-%s", f->name, m->name) {
> >>> -				data.flags = m->flags;
> >>> -				run_test(&data, fd, f->func);
> >>> +	for_each_pipe_static(p) igt_subtest_group {
> >> Imo newline and {} and more indent, I was confused for a bit looking where
> >> you have the igt_subtest_group :-)
> >>
> >> Also we need to give the cibuglog folks a headups before pushing ofc.
> >>
> >> With the bikeshed addressed:
> >>
> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >>
> >>> +		igt_fixture
> >>> +			igt_display_require_output_on_pipe(&data.display, p);
> >>> +
> >>> +		data.pipe = p;
> >>> +
> >>> +		for (f = funcs; f->name; f++) {
> >>> +			for (m = modes; m->name; m++) {
> >>> +				if (m->flags & ~f->valid)
> >>> +					continue;
> >>> +
> >>> +				igt_subtest_f("pipe-%s-%s-%s",
> >>> +					      kmstest_pipe_name(data.pipe),
> >>> +					      f->name, m->name) {
> >>> +					for_each_valid_output_on_pipe(&data.display, data.pipe, data.output) {
> >>> +						data.flags = m->flags;
> >>> +						run_test(&data, fd, f->func);
> >>> +					}
> > Ok I totally overlooked this one - looping over all output combos still
> > means tons of modesets within each subtest. How does that make stuff
> > faster/better?
> > -Daniel
> This is a temporary problem, we will need to add a
> for_single_valid_output_on_pipe macro in the future and then convert
> each caller over to it.
> 
> It's still better than running the test for a fixed amount of time,
> multiple times, which is what kms_flip did..

Well for vblank timing that actually changes depending upon mode and
output (somewhat). But maybe we just need to have 1 (and really only 1)
mode timing tests which goes through all the outputs and every mode and
makes sure the timing is accurate. But then maybe not over all the pipes.

I think kms_setmode was the start for that once, but kinda didn't go that
far.

Either way, different issue.
-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