From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 756CA6F465 for ; Fri, 8 Oct 2021 10:56:44 +0000 (UTC) Message-ID: <40ce98ed-2dad-c7cb-71f4-338104205f9f@intel.com> Date: Fri, 8 Oct 2021 16:26:31 +0530 Content-Language: en-US References: <20211004085629.2796-1-karthik.b.s@intel.com> From: Karthik B S In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_big_fb: Add retry mechanism for async flip subtests List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Juha-Pekka Heikkila Cc: igt-dev@lists.freedesktop.org List-ID: 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 >>>>>>> --- >>>>>>> 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