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

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>,
	Vidya Srinivas <vidya.srinivas@intel.com>
Cc: daniel.vetter@intel.com, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH i-g-t 4/6] i-g-t kms_plane_scaling: test scaling with tiling rotation and pixel formats
Date: Tue, 9 Jan 2018 13:33:54 +0100	[thread overview]
Message-ID: <e4e58524-e51c-8fb0-a30f-3f40208f8966@linux.intel.com> (raw)
In-Reply-To: <20171214105504.GI26573@phenom.ffwll.local>

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