Re: [PATCH i-g-t v2] kms_rotation_crc: 90 degree flip test is not a stress test

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 07/08/2017 16:53, Daniel Vetter wrote:
> On Fri, Aug 04, 2017 at 09:43:41AM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> To the best of my recollection the page flipping test was added
>> simply to start exercising page flips with 90/270 rotation.
>>
>> There is no need to do 60 flips which can take quite some time,
>> because we do 60 flips against each pipe and each fb geometry.
>>
>> Also, calling this a stress test is also not matching the
>> original idea of the test.
>>
>> Several changes:
>>
>> 1. Remove the stress from the name and reduce the number of
>> flips to one only.
>>
>> 2. Move the page flip before CRC collection for a more useful
>> test.
>>
>> 3. Add more flipping tests, for different rotation and sprite
>> planes.
> 
> I assume you didn't make the test overall slower with this?
> 
>> 4. Convert to table driven subtest generation.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Didn't do a full review, but sounds all reasonable. And I assume you've
> tested this at least locally (the igt patchwork CI instance doesn't do
> full runs ... yet).

Yes I've ran it on SKL. It adds more subtests so the runtime is longer, 
but it also finds new bugs.

Ideally someone with display-fu picks this up, fixes the bug(s) this 
exposes (or tells me the test is wrong), so we are not merging known 
failing tests? Or even that is OK?

Tvrtko

> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>   tests/intel-ci/extended.testlist |   2 +-
>>   tests/kms_rotation_crc.c         | 137 ++++++++++++++++++++++++---------------
>>   2 files changed, 85 insertions(+), 54 deletions(-)
>>
>> diff --git a/tests/intel-ci/extended.testlist b/tests/intel-ci/extended.testlist
>> index 17eed013f810..fb71091758c6 100644
>> --- a/tests/intel-ci/extended.testlist
>> +++ b/tests/intel-ci/extended.testlist
>> @@ -82,7 +82,7 @@ igt@gem_ringfill@vebox-bomb
>>   igt@gem_userptr_blits@stress-mm
>>   igt@gem_userptr_blits@stress-mm-invalidate-close
>>   igt@gem_userptr_blits@stress-mm-invalidate-close-overlap
>> -igt@kms_rotation_crc@primary-rotation-90-flip-stress
>> +igt@kms_rotation_crc@primary-rotation-90-flip
>>   igt@pm_rpm@gem-execbuf-stress
>>   igt@pm_rpm@gem-execbuf-stress-extra-wait
>>   igt@pm_rpm@gem-execbuf-stress-pc8
>> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
>> index 83e37f126f40..20f1adb67769 100644
>> --- a/tests/kms_rotation_crc.c
>> +++ b/tests/kms_rotation_crc.c
>> @@ -41,7 +41,7 @@ typedef struct {
>>   	int pos_y;
>>   	uint32_t override_fmt;
>>   	uint64_t override_tiling;
>> -	unsigned int flip_stress;
>> +	unsigned int flips;
>>   } data_t;
>>   
>>   static void
>> @@ -219,7 +219,7 @@ static void prepare_fbs(data_t *data, igt_output_t *output,
>>   
>>   	igt_create_fb(data->gfx_fd, w, h, pixel_format, tiling, &data->fb);
>>   
>> -	if (data->flip_stress) {
>> +	if (data->flips) {
>>   		igt_create_fb(data->gfx_fd, w, h, pixel_format, tiling, &data->fb_flip);
>>   		paint_squares(data, IGT_ROTATION_0, &data->fb_flip, 0.92);
>>   	}
>> @@ -351,15 +351,17 @@ static void test_plane_rotation(data_t *data, int plane_type)
>>   			ret = igt_display_try_commit2(display, commit);
>>   			if (data->override_fmt || data->override_tiling) {
>>   				igt_assert_eq(ret, -EINVAL);
>> -			} else {
>> -				igt_assert_eq(ret, 0);
>> -				igt_pipe_crc_collect_crc(data->pipe_crc,
>> -							  &crc_output);
>> -				igt_assert_crc_equal(&data->ref_crc,
>> -						      &crc_output);
>> +				continue;
>>   			}
>>   
>> -			flip_count = data->flip_stress;
>> +			/* Verify commit was ok. */
>> +			igt_assert_eq(ret, 0);
>> +
>> +			/*
>> +			 * If flips are requested flip away and back before
>> +			 * checking CRC.
>> +			 */
>> +			flip_count = data->flips;
>>   			while (flip_count--) {
>>   				ret = drmModePageFlip(data->gfx_fd,
>>   						      output->config.crtc->crtc_id,
>> @@ -376,6 +378,9 @@ static void test_plane_rotation(data_t *data, int plane_type)
>>   				igt_assert_eq(ret, 0);
>>   				wait_for_pageflip(data->gfx_fd);
>>   			}
>> +
>> +			igt_pipe_crc_collect_crc(data->pipe_crc, &crc_output);
>> +			igt_assert_crc_equal(&data->ref_crc, &crc_output);
>>   		}
>>   
>>   		valid_tests++;
>> @@ -569,8 +574,66 @@ err_commit:
>>   	igt_assert_eq(ret, 0);
>>   }
>>   
>> +static const char *plane_test_str(unsigned plane)
>> +{
>> +	switch (plane) {
>> +	case DRM_PLANE_TYPE_PRIMARY:
>> +		return "primary";
>> +	case DRM_PLANE_TYPE_OVERLAY:
>> +		return "sprite";
>> +	case DRM_PLANE_TYPE_CURSOR:
>> +		return "cursor";
>> +	default:
>> +		igt_assert(0);
>> +	}
>> +}
>> +
>> +static const char *rot_test_str(igt_rotation_t rot)
>> +{
>> +	switch (rot) {
>> +	case IGT_ROTATION_90:
>> +		return "90";
>> +	case IGT_ROTATION_180:
>> +		return "180";
>> +	case IGT_ROTATION_270:
>> +		return "270";
>> +	default:
>> +		igt_assert(0);
>> +	}
>> +}
>> +
>> +static const char *flip_test_str(unsigned flips)
>> +{
>> +	if (flips > 1)
>> +		return "-flip-stress";
>> +	else if (flips)
>> +		return "-flip";
>> +	else
>> +		return "";
>> +}
>> +
>>   igt_main
>>   {
>> +	struct rot_subtest {
>> +		unsigned plane;
>> +		igt_rotation_t rot;
>> +		unsigned flips;
>> +	} *subtest, subtests[] = {
>> +		{ DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_90, 0 },
>> +		{ DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_180, 0 },
>> +		{ DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_270, 0 },
>> +		{ DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_90, 1 },
>> +		{ DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_180, 1 },
>> +		{ DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_270, 1 },
>> +		{ DRM_PLANE_TYPE_OVERLAY, IGT_ROTATION_90, 0 },
>> +		{ DRM_PLANE_TYPE_OVERLAY, IGT_ROTATION_180, 0 },
>> +		{ DRM_PLANE_TYPE_OVERLAY, IGT_ROTATION_270, 0 },
>> +		{ DRM_PLANE_TYPE_OVERLAY, IGT_ROTATION_90, 1 },
>> +		{ DRM_PLANE_TYPE_OVERLAY, IGT_ROTATION_180, 1 },
>> +		{ DRM_PLANE_TYPE_OVERLAY, IGT_ROTATION_270, 1 },
>> +		{ DRM_PLANE_TYPE_CURSOR, IGT_ROTATION_180, 0 },
>> +		{ 0, 0, 0}
>> +	};
>>   	data_t data = {};
>>   	int gen = 0;
>>   
>> @@ -586,43 +649,19 @@ igt_main
>>   
>>   		igt_display_init(&data.display, data.gfx_fd);
>>   	}
>> -	igt_subtest_f("primary-rotation-180") {
>> -		data.rotation = IGT_ROTATION_180;
>> -		test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
>> -	}
>> -
>> -	igt_subtest_f("sprite-rotation-180") {
>> -		data.rotation = IGT_ROTATION_180;
>> -		test_plane_rotation(&data, DRM_PLANE_TYPE_OVERLAY);
>> -	}
>> -
>> -	igt_subtest_f("cursor-rotation-180") {
>> -		data.rotation = IGT_ROTATION_180;
>> -		test_plane_rotation(&data, DRM_PLANE_TYPE_CURSOR);
>> -	}
>> -
>> -	igt_subtest_f("primary-rotation-90") {
>> -		igt_require(gen >= 9);
>> -		data.rotation = IGT_ROTATION_90;
>> -		test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
>> -	}
>>   
>> -	igt_subtest_f("primary-rotation-270") {
>> -		igt_require(gen >= 9);
>> -		data.rotation = IGT_ROTATION_270;
>> -		test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
>> -	}
>> -
>> -	igt_subtest_f("sprite-rotation-90") {
>> -		igt_require(gen >= 9);
>> -		data.rotation = IGT_ROTATION_90;
>> -		test_plane_rotation(&data, DRM_PLANE_TYPE_OVERLAY);
>> -	}
>> -
>> -	igt_subtest_f("sprite-rotation-270") {
>> -		igt_require(gen >= 9);
>> -		data.rotation = IGT_ROTATION_270;
>> -		test_plane_rotation(&data, DRM_PLANE_TYPE_OVERLAY);
>> +	for (subtest = subtests; subtest->rot; subtest++) {
>> +		igt_subtest_f("%s-rotation-%s%s",
>> +			      plane_test_str(subtest->plane),
>> +			      rot_test_str(subtest->rot),
>> +			      flip_test_str(subtest->flips)) {
>> +			igt_require(!(subtest->rot &
>> +				    (IGT_ROTATION_90 | IGT_ROTATION_270)) ||
>> +				    gen >= 9);
>> +			data.rotation = subtest->rot;
>> +			data.flips = subtest->flips;
>> +			test_plane_rotation(&data, subtest->plane);
>> +		}
>>   	}
>>   
>>   	igt_subtest_f("sprite-rotation-90-pos-100-0") {
>> @@ -650,14 +689,6 @@ igt_main
>>   		test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
>>   	}
>>   
>> -	igt_subtest_f("primary-rotation-90-flip-stress") {
>> -		igt_require(gen >= 9);
>> -		data.override_tiling = 0;
>> -		data.flip_stress = 60;
>> -		data.rotation = IGT_ROTATION_90;
>> -		test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
>> -	}
>> -
>>   	igt_subtest_f("primary-rotation-90-Y-tiled") {
>>   		enum pipe pipe;
>>   		igt_output_t *output;
>> -- 
>> 2.9.4
>>
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx