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
