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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
