From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id E31716E245 for ; Thu, 9 Apr 2020 16:08:11 +0000 (UTC) Date: Thu, 9 Apr 2020 19:08:07 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Message-ID: <20200409160807.GW6112@intel.com> References: <20200407110904.13008-1-juhapekka.heikkila@gmail.com> <20200407153602.GD6112@intel.com> <7889c8d7-7aed-9ebb-7103-9fa798b4097c@gmail.com> <20200407160830.GE6112@intel.com> <0db0f75f-ea12-f04a-7bcc-254c98c9d4ad@gmail.com> <20200407171051.GI6112@intel.com> <3e40352e-dabf-2e61-4da1-66b026f87db2@gmail.com> <20200407174250.GK6112@intel.com> <387b80b3-811d-4308-2ea6-c01b058d64a8@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <387b80b3-811d-4308-2ea6-c01b058d64a8@gmail.com> Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_plane: survive cdclk caused modeset List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Juha-Pekka Heikkila Cc: igt-dev@lists.freedesktop.org List-ID: On Wed, Apr 08, 2020 at 10:08:31PM +0300, Juha-Pekka Heikkila wrote: > On 7.4.2020 20.42, Ville Syrj=E4l=E4 wrote: > > On Tue, Apr 07, 2020 at 08:24:01PM +0300, Juha-Pekka Heikkila wrote: > >> On 7.4.2020 20.10, Ville Syrj=E4l=E4 wrote: > >>> On Tue, Apr 07, 2020 at 08:07:16PM +0300, Juha-Pekka Heikkila wrote: > >>>> On 7.4.2020 19.22, Juha-Pekka Heikkila wrote: > >>>>> On 7.4.2020 19.08, Ville Syrj=E4l=E4 wrote: > >>>>>> On Tue, Apr 07, 2020 at 06:54:09PM +0300, Juha-Pekka Heikkila wrot= e: > >>>>>>> On 7.4.2020 18.36, Ville Syrj=E4l=E4 wrote: > >>>>>>>> On Tue, Apr 07, 2020 at 02:09:04PM +0300, Juha-Pekka Heikkila wr= ote: > >>>>>>>>> This change will slow this test down a bit. In mid test starting > >>>>>>>>> to use higher bpp pixel format (say 64bpp) can cause modeset. > >>>>>>>>> Use blocking commit so there's wait for modeset to happen. > >>>>>>>> > >>>>>>>> We already wait for the event the next time around. So this > >>>>>>>> doesn't make sense to me. > >>>>>>> > >>>>>>> There's those logs like this > >>>>>>> https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_470/fi-icl-guc/ig= t@kms_plane@pixel-format-pipe-c-planes-source-clamping.html > >>>>>>> > >>>>>>> > >>>>>>> where going to 64bpp pixel format will cause modeset and fail run= ning > >>>>>>> test. > >>>>>> > >>>>>> Hmm. Seems we don't have drm.debug=3D0x10 enabled so we don't see = the > >>>>>> actual debug message :( Anyways, the real bug seems to be the lack= of > >>>>>> ALLOW_MODESET. > >>>>> > >>>>> I'll try that and see how it changes situation. I originally had id= ea > >>>>> modeset will block the flip and making this commit blocking would a= lways > >>>>> force things always into correct order as current test didn't fail = every > >>>>> time for me. > >>>> > >>>> Having that commit saying > >>>> > >>>> igt_display_commit_atomic(&data->display, > >>>> DRM_MODE_ATOMIC_ALLOW_MODESET | > >>>> DRM_MODE_ATOMIC_NONBLOCK | > >>>> DRM_MODE_PAGE_FLIP_EVENT, NULL); > >>>> > >>>> I see test failing as before so it will not alone fix this issue. On= my > >>>> ICL box the test fail maybe 1/5 times, with the patch on list it nev= er > >>>> failed so far. > >>> > >>> Why does it fail? > >> > >> > >> This is failure with ALLOW_MODESET > >> --- > >> (kms_plane:31238) igt_kms-DEBUG: display: } > >> (kms_plane:31238) igt_kms-CRITICAL: Test assertion failure function > >> igt_display_commit_atomic, file ../lib/igt_kms.c:3508: > >> (kms_plane:31238) igt_kms-CRITICAL: Failed assertion: ret =3D=3D 0 > >> (kms_plane:31238) igt_kms-CRITICAL: Last errno: 16, Device or resource= busy > >> (kms_plane:31238) igt_kms-CRITICAL: error: -16 !=3D 0 > >> (kms_plane:31238) igt_core-INFO: Stack trace: > >> (kms_plane:31238) igt_core-INFO: #0 ../lib/igt_core.c:1676 > >> __igt_fail_assert() > >> (kms_plane:31238) igt_core-INFO: #1 [igt_display_commit_atomic+0x44] > >> (kms_plane:31238) igt_core-INFO: #2 ../tests/kms_plane.c:599 > >> capture_format_crcs.constprop.14() > >> (kms_plane:31238) igt_core-INFO: #3 ../tests/kms_plane.c:652 > >> test_format_plane_colors.constprop.13() > >> (kms_plane:31238) igt_core-INFO: #4 ../tests/kms_plane.c:728 > >> test_pixel_formats.constprop.9() > >> (kms_plane:31238) igt_core-INFO: #5 ../tests/kms_plane.c:950 > >> run_tests_for_pipe_plane.constprop.8() > >> (kms_plane:31238) igt_core-INFO: #6 ../tests/kms_plane.c:1019 > >> __real_main1006() > >> (kms_plane:31238) igt_core-INFO: #7 ../tests/kms_plane.c:1006 main() > >> (kms_plane:31238) igt_core-INFO: #8 ../csu/libc-start.c:344 > >> __libc_start_main() > >> (kms_plane:31238) igt_core-INFO: #9 [_start+0x2a] > >> **** END **** > >> Stack trace: > >> #0 ../lib/igt_core.c:1676 __igt_fail_assert() > >> #1 [igt_display_commit_atomic+0x44] > >> #2 ../tests/kms_plane.c:599 capture_format_crcs.constprop.14() > >> #3 ../tests/kms_plane.c:652 test_format_plane_colors.constprop.13() > >> #4 ../tests/kms_plane.c:728 test_pixel_formats.constprop.9() > >> #5 ../tests/kms_plane.c:950 run_tests_for_pipe_plane.constprop.8() > >> #6 ../tests/kms_plane.c:1019 __real_main1006() > >> #7 ../tests/kms_plane.c:1006 main() > >> #8 ../csu/libc-start.c:344 __libc_start_main() > >> #9 [_start+0x2a] > >> Subtest pixel-format-pipe-A-planes-source-clamping: FAIL (22.543s) > > = > > That doesn't tell anyone anything useful. Only kernel logs can help > > diagnose the problem. > > = > > = > >> -- > >> > >> Failure is ebusy. It seems round where it kick me out is not always the > >> same. > > = > > Hmm. Sounds like we have a race between sending the event vs. submitting > > a new commit :( > > = > > Hmm. What wasn't there a patch from Daniel to convert modeset commits to > > blocking to "fix" a race just like this? Ah, yeah it was EBUSY due to > > multiple pipes and noblocking modesets. Do you have multiple pipes > > enablad when it fails? That could certainly explain the race. > > = > = > Quickly looking there shouldn't be multiple pipes enabled. Test is one = > of those simple loops that start with for_each_plane_on_pipe(..){..} and = > different pipes are run as different tests. Can't see anything else returning -EBUSY except if flip_done is not completed, and that one should be done before we send the event. > = > >> > >>> > >>>> > >>>>> > >>>>>> > >>>>>>> I was testing this on ICL and see errors randomly, on ci those > >>>>>>> seem to be less random. Making this one commit blocking will cause > >>>>>>> modeset to settle without interrupting test, at least on my ICL. > >>>>>>> > >>>>>>> If there's a way to sort those pixel formats according to bpp and= start > >>>>>>> with highest there's no need for this. > >>>>>>> > >>>>>>>> > >>>>>>>>> > >>>>>>>>> Fixes: https://gitlab.freedesktop.org/drm/intel/issues/1214 > >>>>>>>>> Signed-off-by: Juha-Pekka Heikkila > >>>>>>>>> --- > >>>>>>>>> =A0=A0 tests/kms_plane.c | 6 ++---- > >>>>>>>>> =A0=A0 1 file changed, 2 insertions(+), 4 deletions(-) > >>>>>>>>> > >>>>>>>>> diff --git a/tests/kms_plane.c b/tests/kms_plane.c > >>>>>>>>> index 805795cd..2324fb6e 100644 > >>>>>>>>> --- a/tests/kms_plane.c > >>>>>>>>> +++ b/tests/kms_plane.c > >>>>>>>>> @@ -569,12 +569,10 @@ static void capture_format_crcs(data_t *d= ata, > >>>>>>>>> enum pipe pipe, > >>>>>>>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (data->display.is_atomic) { > >>>>>>>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 /* > >>>>>>>>> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * Use non-blocking commit= s to allow the next fb > >>>>>>>>> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * to be prepared in paral= lel while the current fb > >>>>>>>>> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * awaits to be latched. > >>>>>>>>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * Use blocking commit bec= ause there maybe > >>>>>>>>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * modeset when going to h= igher bpp pixel format. > >>>>>>>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 */ > >>>>>>>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 igt_display_commit= _atomic(&data->display, > >>>>>>>>> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0 DRM_MODE_ATOMIC_NONBLOCK | > >>>>>>>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0 DRM_MODE_PAGE_FLIP_EVENT, NULL); > >>>>>>>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 } else { > >>>>>>>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 /* > >>>>>>>>> -- = > >>>>>>>>> 2.17.1 > >>>>>>>>> > >>>>>>>>> _______________________________________________ > >>>>>>>>> igt-dev mailing list > >>>>>>>>> igt-dev@lists.freedesktop.org > >>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/igt-dev > >>>>>>>> > >>>>>> > >>>>> > >>> > > = -- = Ville Syrj=E4l=E4 Intel _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev