[igt-dev] [PATCH i-g-t] tests/kms_big_fb: Add retry mechanism for async flip subtests

[Karthik B S]

On 10/5/2021 4:36 PM, Ville Syrjälä wrote:
> On Tue, Oct 05, 2021 at 01:46:54PM +0300, Juha-Pekka Heikkila wrote:
>> On 5.10.2021 13.22, Ville Syrjälä wrote:
>>> On Tue, Oct 05, 2021 at 03:30:01PM +0530, Karthik B S wrote:
>>>> On 10/4/2021 9:13 PM, Ville Syrjälä wrote:
>>>>> On Mon, Oct 04, 2021 at 12:19:01PM +0300, Ville Syrjälä wrote:
>>>>>> On Mon, Oct 04, 2021 at 02:26:29PM +0530, Karthik B S wrote:
>>>>>>> Async flip subtests fail sporadically with CRC failure on CI.
>>>>>>> This is expected as these tests are not run on highest priority by the
>>>>>>> scheduler, but this creates noise on CI. Add retry mechanism to rerun
>>>>>>> the test once if failure is seen.
>>>>>>>
>>>>>>> Signed-off-by: Karthik B S <karthik.b.s at intel.com>
>>>>>>> ---
>>>>>>>     tests/i915/kms_big_fb.c | 9 +++++++++
>>>>>>>     1 file changed, 9 insertions(+)
>>>>>>>
>>>>>>> diff --git a/tests/i915/kms_big_fb.c b/tests/i915/kms_big_fb.c
>>>>>>> index 308227c9..8c09f59e 100644
>>>>>>> --- a/tests/i915/kms_big_fb.c
>>>>>>> +++ b/tests/i915/kms_big_fb.c
>>>>>>> @@ -481,6 +481,7 @@ max_hw_stride_async_flip_test(data_t *data)
>>>>>>>     		       h = data->output->config.default_mode.vdisplay;
>>>>>>>     	igt_plane_t *primary;
>>>>>>>     	igt_crc_t compare_crc, async_crc;
>>>>>>> +	bool retried = false;
>>>>>>>     
>>>>>>>     	igt_require(data->display.is_atomic);
>>>>>>>     	igt_output_set_pipe(data->output, data->pipe);
>>>>>>> @@ -513,6 +514,7 @@ max_hw_stride_async_flip_test(data_t *data)
>>>>>>>     					  INTEL_PIPE_CRC_SOURCE_AUTO);
>>>>>>>     	igt_pipe_crc_start(data->pipe_crc);
>>>>>>>     
>>>>>>> +retry:
>>>>>>>     	igt_set_timeout(5, "Async pageflipping loop got stuck!\n");
>>>>>>>     	for (int i = 0; i < 2; i++) {
>>>>>>>     		igt_plane_set_fb(primary, &data->big_fb);
>>>>>>> @@ -548,6 +550,13 @@ max_hw_stride_async_flip_test(data_t *data)
>>>>>>>     		igt_assert_f(kmstest_get_vblank(data->drm_fd, data->pipe, 0) -
>>>>>>>     			     startframe == 1, "lost frames\n");
>>>>>>>     
>>>>>>> +		/* Test is not running at real time priority, so allow one failure*/
>>>>>>> +		if (!(igt_check_crc_equal(&compare_crc, &async_crc)^(i^1)) && !retried) {
>>>>>>> +			retried = true;
>>>>>>> +			igt_reset_timeout();
>>>>>>> +			goto retry;
>>>>>>> +		}
>>>>>>> +
>>>>>> This test seems to entirely fit kms_big_fb in general. I don't
>>>>>                         ^
>>>>> 		     not
>>>>>
>>>>> is what I meant to write. Somewhat important small word.
>>>>>
>>>>>> think kms_big_fb should be testing any timing sensitive stuff.
>>>>>>
>>>>>> So I think we should change this to a form that follows the rest
>>>>>> of kms_big_fb to validate that each page flip just presents the
>>>>>> correct data on screen. The timing sensitive stuff is best left
>>>>>> for kms_async_flip.
>>>>>>
>>>>>> So this should maybe be something like: flip to a correctly sized
>>>>>> temp fb with the wrong contents and change the plane src coordinates,
>>>>>> and then async flip back to the correct fb and validate the
>>>>>> correct data is now on screen.
>>>> Thank you for the feedback.
>>>>
>>>> Is this because we can handle any timing related failures in the same
>>>> test (may be using some common mechanism)? So add a subtest for
>>>> max-hw-stride in kms_async_flips and rewrite this subtest as mentioned
>>>> above?
>>> I don't think there's anything specifically about max stride
>>> that needs timing sensitive tests. In theory kms_async_flip/crc
>>> already tests what we need, which is that there are no visual
>>> artifacts when flipping between two identical framebuffers.
>>> The only exception would be if there's some very specific
>>> hardware issue with big stride + async flip.
>> At a time there was explicit wishes from hw guys how this test was to be
>> performed with strides past 64K size. I have no idea if their worries
>> are still valid.
>>
>> Timing sensitivity comes just from the fact when async flip is made the
>> full stride flips as expected and it need to be captured with crc from
>> that flipping frame.
> kms_async_flip/crc just keeps flipping all the time, and checks
> every frame that the crc remains the same.
>
> Though atm it uses a solid color fb, so maybe not the best thing
> necessarily for detecting weird stuff. Should change it to use the
> normal pattern fb perhaps (+ the stripe on one edge we clobber/unclobber).
> Though a lot of the normal pattern fb is just black. Maybe we should
> have a some kind of standard pattern fb with a bit more of the area
> filled with interesting shapes that are easily corrupted by eg. any
> single tile fetch going into the weeds...
>
> I guess we could add a big-stride variant there too. For that we'd
> probably want to put the "what is the max supported hw stride?"
> stuff somewhere into lib/ (and extend it for all the platforms).
>
Thanks Ville, JP for the inputs. Will work on the above mentioned 
changes and resubmit the patch.

Thanks,

Karthik.B.S