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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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).

-- 
Ville Syrjälä
Intel