Re: [PATCH i-g-t 4/6] i-g-t kms_plane_scaling: test scaling with tiling rotation and pixel formats

[Daniel Vetter <daniel@ffwll.ch>]

Just a quick comment at the bottom.

On Wed, Dec 13, 2017 at 03:20:50PM +0530, Vidya Srinivas wrote:
> @@ -312,23 +480,40 @@ static void test_plane_scaling(data_t *d, enum pipe pipe)
>  	igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
>  }
>  
> -igt_simple_main
> +igt_main
>  {
>  	data_t data = {};
>  	enum pipe pipe;
>  
>  	igt_skip_on_simulation();
>  
> -
> -	data.drm_fd = drm_open_driver(DRIVER_INTEL);
> -	igt_require_pipe_crc(data.drm_fd);
> -	igt_display_init(&data.display, data.drm_fd);
> -	data.devid = intel_get_drm_devid(data.drm_fd);
> +	igt_fixture {
> +		data.drm_fd = drm_open_driver(DRIVER_INTEL);
> +		kmstest_set_vt_graphics_mode();
> +		igt_require_pipe_crc(data.drm_fd);
> +		igt_display_init(&data.display, data.drm_fd);
> +		data.devid = intel_get_drm_devid(data.drm_fd);
> +		igt_require_gem(data.drm_fd);
> +	}
>  
>  	data.num_scalers = intel_gen(data.devid) >= 9 ? 2 : 0;

Hm, would be nice if we could get rid of this hard-coded platform
knowledge and autodiscover. But atm the kernel doesn't expose a
"can_scale" property unfortunately. Maybe add a FIXME comment?

Real reason I'm commenting here: You probably need to put this into the
igt_fixture too, since otherwise you're test won't run correctly when just
enumerating tests.

>  
> -	for_each_pipe_static(pipe)
> -		test_plane_scaling(&data, pipe);
> +	for_each_pipe_static(pipe) {
> +
> +		igt_subtest_f("scaler_basic_test") {
> +			test_plane_scaling(&data, pipe);
> +		}
> +
> +		igt_subtest_f("scaler_with_pixel_format") {
> +			test_scaler_with_pixel_format(&data, pipe);
> +		}
>  
> -	igt_display_fini(&data.display);
> +		igt_subtest_f("scaler_with_rotation") {
> +			test_scaler_with_rotation(&data, pipe);
> +		}
> +
> +		igt_fixture {
> +			igt_display_fini(&data.display);
> +		}
> +	}

Just a quick drive-by: You probably want to convert to subtest-based
testing as the very first patch (without any functional tests). In your
current series you add more subtests while still using igt_simple_main,
which will blow up.

The goal of a split up patch series isn't just to make review easier, but
also testing: Every single step in your series is supposed to be a fully
functional codebase. When you're not much experienced with building up
such a patch series, it's good practice to double-check that before
sending. I tend to use git rebase -x $test-script to automate that if
possible. That way you can make sure that every single step in your patch
series builds (cleanly!) and results in a working testcases (which should
only change results if your patch aims to do that, not as some
side-effect).

Cheers, 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