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

[Maarten Lankhorst]

Op 14-12-17 om 11:55 schreef Daniel Vetter:
> 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?
Isn't that what igt_display_try_commit_atomic + TEST_ONLY is for?

num scalers might be less if a scaler is used by the crtc as well, so not something you can skip..
> 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