Re: [PATCH i-g-t 1/5] tests/kms_plane_multiple: Add TEST_ONLY flag

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 30-12-16 om 13:00 schreef Mika Kahola:
> Add TEST_ONLY flag to test atomic modesetting commits without
> actual real-life commit.
>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  tests/kms_plane_multiple.c | 79 ++++++++++++++++++++++++++++------------------
>  1 file changed, 49 insertions(+), 30 deletions(-)
>
> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
> index 5e12be4..1a77a38 100644
> --- a/tests/kms_plane_multiple.c
> +++ b/tests/kms_plane_multiple.c
> @@ -61,10 +61,12 @@ struct {
>  	int iterations;
>  	bool user_seed;
>  	int seed;
> +	bool test_only;
>  } opt = {
>  	.iterations = 64,
>  	.user_seed = false,
>  	.seed = 1,
> +	.test_only = false,
>  };
>  
>  static inline uint32_t pipe_select(int pipe)
> @@ -228,7 +230,7 @@ prepare_planes(data_t *data, enum pipe pipe, color_t *color,
>  static void
>  test_atomic_plane_position_with_output(data_t *data, enum pipe pipe,
>  				       igt_output_t *output, int max_planes,
> -				       uint64_t tiling)
> +				       uint64_t tiling, bool test_only)
>  {
>  	char buf[256];
>  	struct drm_event *e = (void *)buf;
> @@ -240,6 +242,12 @@ test_atomic_plane_position_with_output(data_t *data, enum pipe pipe,
>  	int iterations = opt.iterations < 1 ? 1 : opt.iterations;
>  	bool loop_forever;
>  	char info[256];
> +	int flags = DRM_MODE_ATOMIC_ALLOW_MODESET;
> +
> +	if (test_only)
> +		flags |= DRM_MODE_ATOMIC_TEST_ONLY;
> +	else
> +		flags |= DRM_MODE_PAGE_FLIP_EVENT;
I would only pass ALLOW_MODESET for the TEST_ONLY case.
>  	if (opt.iterations == LOOP_FOREVER) {
>  		loop_forever = true;
> @@ -256,8 +264,9 @@ test_atomic_plane_position_with_output(data_t *data, enum pipe pipe,
>  
>  	test_init(data, pipe);
>  
> -	test_grab_crc(data, output, pipe, true, &blue, tiling,
> -		      &test.reference_crc);
> +	if (!test_only)
> +		test_grab_crc(data, output, pipe, true, &blue, tiling,
> +			      &test.reference_crc);
You still want to do some kind of pipe setup, else the state of the pipe will depend on what was set before it.

I'm not sure what you want the state to be though.
>  	i = 0;
>  	while (i < iterations || loop_forever) {
> @@ -265,24 +274,27 @@ test_atomic_plane_position_with_output(data_t *data, enum pipe pipe,
>  
>  		vblank_start = get_vblank(data->display.drm_fd, pipe, DRM_VBLANK_NEXTONMISS);
>  
> -		igt_display_commit_atomic(&data->display,
> -					  DRM_MODE_PAGE_FLIP_EVENT,
> -					  &data->display);
> +		ret = igt_display_try_commit_atomic(&data->display,
> +						    flags,
> +						    &data->display);
> +		igt_assert(ret != -EINVAL);
Other failures are fine? I would leave igt_display_commit_atomic here, it catches errors for you. :)
> -		igt_set_timeout(1, "Stuck on page flip");
> +		if (!test_only) {
> +			igt_set_timeout(1, "Stuck on page flip");
>  
> -		ret = read(data->display.drm_fd, buf, sizeof(buf));
> -		igt_assert(ret >= 0);
> +			ret = read(data->display.drm_fd, buf, sizeof(buf));
> +			igt_assert(ret >= 0);
>  
> -		igt_assert_eq(get_vblank(data->display.drm_fd, pipe, 0), vblank_start + 1);
> -		igt_assert_eq(e->type, DRM_EVENT_FLIP_COMPLETE);
> -		igt_reset_timeout();
> +			igt_assert_eq(get_vblank(data->display.drm_fd, pipe, 0), vblank_start + 1);
> +			igt_assert_eq(e->type, DRM_EVENT_FLIP_COMPLETE);
> +			igt_reset_timeout();
>  
> -		n = igt_pipe_crc_get_crcs(data->pipe_crc, MAX_CRCS, &crc);
> +			n = igt_pipe_crc_get_crcs(data->pipe_crc, MAX_CRCS, &crc);
>  
> -		igt_assert_eq(n, MAX_CRCS);
> +			igt_assert_eq(n, MAX_CRCS);
>  
> -		igt_assert_crc_equal(&test.reference_crc, crc);
> +			igt_assert_crc_equal(&test.reference_crc, crc);
> +		}
>  
>  		i++;
>  	}
> @@ -345,7 +357,7 @@ test_legacy_plane_position_with_output(data_t *data, enum pipe pipe,
>  
>  static void
>  test_plane_position(data_t *data, enum pipe pipe, bool atomic, int max_planes,
> -		    uint64_t tiling)
> +		    uint64_t tiling, bool test_only)
>  {
>  	igt_output_t *output;
>  	int connected_outs;
> @@ -372,7 +384,8 @@ test_plane_position(data_t *data, enum pipe pipe, bool atomic, int max_planes,
>  			test_atomic_plane_position_with_output(data, pipe,
>  							       output,
>  							       max_planes,
> -							       tiling);
> +							       tiling,
> +							       test_only);
>  		else
>  			test_legacy_plane_position_with_output(data, pipe,
>  							       output,
> @@ -387,54 +400,55 @@ test_plane_position(data_t *data, enum pipe pipe, bool atomic, int max_planes,
>  }
>  
>  static void
> -run_tests_for_pipe_plane(data_t *data, enum pipe pipe, int max_planes)
> +run_tests_for_pipe_plane(data_t *data, enum pipe pipe, int max_planes,
> +			 bool test_only)
>  {
>  	igt_subtest_f("legacy-pipe-%s-tiling-none-planes-%d",
>  		      kmstest_pipe_name(pipe), max_planes)
>  		test_plane_position(data, pipe, false, max_planes,
> -				    LOCAL_DRM_FORMAT_MOD_NONE);
> +				    LOCAL_DRM_FORMAT_MOD_NONE, test_only);
I'm pretty sure the legacy tests don't work with TEST_ONLY..
>  	igt_subtest_f("atomic-pipe-%s-tiling-none-planes-%d",
>  		      kmstest_pipe_name(pipe), max_planes)
>  		test_plane_position(data, pipe, true, max_planes,
> -				    LOCAL_I915_FORMAT_MOD_X_TILED);
> +				    LOCAL_I915_FORMAT_MOD_X_TILED, test_only);
>  
>  	igt_subtest_f("legacy-pipe-%s-tiling-x-planes-%d",
>  		      kmstest_pipe_name(pipe), max_planes)
>  		test_plane_position(data, pipe, false, max_planes,
> -				    LOCAL_I915_FORMAT_MOD_X_TILED);
> +				    LOCAL_I915_FORMAT_MOD_X_TILED, test_only);
>  
>  	igt_subtest_f("atomic-pipe-%s-tiling-x-planes-%d",
>  		      kmstest_pipe_name(pipe), max_planes)
>  		test_plane_position(data, pipe, true, max_planes,
> -				    LOCAL_I915_FORMAT_MOD_X_TILED);
> +				    LOCAL_I915_FORMAT_MOD_X_TILED, test_only);
>  
>  	igt_subtest_f("legacy-pipe-%s-tiling-y-planes-%d",
>  		      kmstest_pipe_name(pipe), max_planes)
>  		test_plane_position(data, pipe, false, max_planes,
> -				    LOCAL_I915_FORMAT_MOD_Y_TILED);
> +				    LOCAL_I915_FORMAT_MOD_Y_TILED, test_only);
>  
>  	igt_subtest_f("atomic-pipe-%s-tiling-y-planes-%d",
>  		      kmstest_pipe_name(pipe), max_planes)
>  		test_plane_position(data, pipe, true, max_planes,
> -				    LOCAL_I915_FORMAT_MOD_Y_TILED);
> +				    LOCAL_I915_FORMAT_MOD_Y_TILED, test_only);
>  
>  	igt_subtest_f("legacy-pipe-%s-tiling-yf-planes-%d",
>  		      kmstest_pipe_name(pipe), max_planes)
>  		test_plane_position(data, pipe, false, max_planes,
> -				    LOCAL_I915_FORMAT_MOD_Yf_TILED);
> +				    LOCAL_I915_FORMAT_MOD_Yf_TILED, test_only);
>  
>  	igt_subtest_f("atomic-pipe-%s-tiling-yf-planes-%d",
>  		      kmstest_pipe_name(pipe), max_planes)
>  		test_plane_position(data, pipe, true, max_planes,
> -				    LOCAL_I915_FORMAT_MOD_Yf_TILED);
> +				    LOCAL_I915_FORMAT_MOD_Yf_TILED, test_only);
>  }
>  
>  static void
> -run_tests_for_pipe(data_t *data, enum pipe pipe)
> +run_tests_for_pipe(data_t *data, enum pipe pipe, bool test_only)
>  {
>  	for (int planes = IGT_PLANE_PRIMARY; planes < IGT_MAX_PLANES; planes++)
> -		run_tests_for_pipe_plane(data, pipe, planes);
> +		run_tests_for_pipe_plane(data, pipe, planes, test_only);
>  }
>  
>  static data_t data;
> @@ -455,6 +469,9 @@ static int opt_handler(int option, int option_index, void *input)
>  		opt.user_seed = true;
>  		opt.seed = strtol(optarg, NULL, 0);
>  		break;
> +	case 't':
> +		opt.test_only = true;
> +		break;
>  	default:
>  		igt_assert(false);
>  	}
> @@ -464,13 +481,15 @@ static int opt_handler(int option, int option_index, void *input)
>  
>  const char *help_str =
>  	"  --iterations Number of iterations for test coverage. -1 loop forever, default 64 iterations\n"
> -	"  --seed       Seed for random number generator\n";
> +	"  --seed       Seed for random number generator\n"
> +	"  --test-only  test only atomic commit\n";
>  
>  int main(int argc, char *argv[])
>  {
>  	struct option long_options[] = {
>  		{ "iterations", required_argument, NULL, 'i'},
>  		{ "seed",    required_argument, NULL, 's'},
> +		{ "test-only", no_argument, NULL, 't'},
>  		{ 0, 0, 0, 0 }
>  	};
I think it's best to just list the tests twice, once normal and once with test-only in the test name?
> @@ -489,7 +508,7 @@ int main(int argc, char *argv[])
>  	}
>  
>  	for (int pipe = 0; pipe < I915_MAX_PIPES; pipe++)
> -		run_tests_for_pipe(&data, pipe);
> +		run_tests_for_pipe(&data, pipe, opt.test_only);
>  
>  	igt_fixture {
>  		igt_display_fini(&data.display);


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