[igt-dev] [PATCH i-g-t] tests/kms_plane: fix pixel format (-clamping) tests

[Juha-Pekka Heikkila]

On 29.10.2020 14.43, Kahola, Mika wrote:
>> -----Original Message-----
>> From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of Juha-
>> Pekka Heikkila
>> Sent: Monday, October 26, 2020 3:08 PM
>> To: igt-dev at lists.freedesktop.org
>> Subject: [igt-dev] [PATCH i-g-t] tests/kms_plane: fix pixel format (-clamping)
>> tests
>>
>> cdclk caused modesets mid test will mess up crc sequence where two
>> consecutive frames will get same crc. If there come modeset restart round
>> for given set as modeset will anyway happen only on first color.
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>> ---
>>   tests/kms_plane.c | 28 ++++++++++++++++++++++------
>>   1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/kms_plane.c b/tests/kms_plane.c index
>> e42c71cd9..4ce859fba 100644
>> --- a/tests/kms_plane.c
>> +++ b/tests/kms_plane.c
>> @@ -560,9 +560,11 @@ static void capture_format_crcs(data_t *data, enum
>> pipe pipe,
>>   	struct drm_event_vblank ev;
>>   	int i;
>>
>> +restart_round:
>>   	for (i = 0; i < data->num_colors; i++) {
>>   		const color_t *c = &data->colors[i];
>>   		struct igt_fb old_fb = *fb;
>> +		int ret;
>>
>>   		prepare_format_color(data, pipe, plane, format, modifier,
>>   				     width, height, encoding, range, c, fb); @@
>> -593,10 +595,22 @@ static void capture_format_crcs(data_t *data, enum
>> pipe pipe,
>>   			 * to be prepared in parallel while the current fb
>>   			 * awaits to be latched.
>>   			 */
>> -			igt_display_commit_atomic(&data->display,
>> -
>> DRM_MODE_ATOMIC_ALLOW_MODESET |
>> -
>> DRM_MODE_ATOMIC_NONBLOCK |
>> -
>> DRM_MODE_PAGE_FLIP_EVENT, NULL);
>> +			ret = igt_display_try_commit_atomic(&data->display,
>> +
>> DRM_MODE_ATOMIC_NONBLOCK |
>> +
>> DRM_MODE_PAGE_FLIP_EVENT, NULL);
>> +			if (ret) {
>> +				/*
>> +				 * there was needed modeset for pixel
>> format.
>> +				 * modeset here happen only on first color of
>> +				 * given set so restart round as modeset will
>> +				 * mess up crc frame sequence.
>> +				 */
>> +				igt_display_commit_atomic(&data->display,
>> +
>> DRM_MODE_ATOMIC_ALLOW_MODESET,
>> +							  NULL);
>> +				igt_remove_fb(data->drm_fd, &old_fb);
>> +				goto restart_round;
>> +			}
> 
> I tried to figure out the patch and it seems that we need to do modeset only once, right? However, there is a theoretical chance (probability maybe next to nothing) that we keep looping on restart_found. Therefore, I was thinking that we should limit the restarts to 1 or to some finite number?

You mean there would come more than one modeset? I think many tests will 
start to fail at that moment. Here fb which required modeset is left on 
screen when going for restart, there shouldn't be new modeset. Then if 
modeset itself fail that modesetting commit is not try commit so it will 
assert on failure. I don't think there is need for additional counter 
unless cdclk calculations fail but then everything is failing everywhere 
anyway I think.

> 
> Cheers,
> Mika
>>   		} else {
>>   			/*
>>   			 * Last moment to grab the previous crc @@ -815,8
>> +829,10 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
>>
>>   		igt_plane_set_fb(plane, &test_fb);
>>
>> -		ret = igt_display_try_commit_atomic(&data->display,
>> DRM_MODE_ATOMIC_TEST_ONLY, NULL);
>> -
>> +		ret = igt_display_try_commit_atomic(&data->display,
>> +
>> DRM_MODE_ATOMIC_TEST_ONLY |
>> +
>> DRM_MODE_ATOMIC_ALLOW_MODESET,
>> +						    NULL);
>>   		if (!ret) {
>>   			width = test_fb.width;
>>   			height = test_fb.height;
>> --
>> 2.28.0
>>
>> _______________________________________________
>> igt-dev mailing list
>> igt-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/igt-dev