Re: [igt-dev] [PATCH i-g-t] tests/kms_plane: survive cdclk caused modeset

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

On Wed, Apr 08, 2020 at 10:08:31PM +0300, Juha-Pekka Heikkila wrote:
> On 7.4.2020 20.42, Ville Syrjälä wrote:
> > On Tue, Apr 07, 2020 at 08:24:01PM +0300, Juha-Pekka Heikkila wrote:
> >> On 7.4.2020 20.10, Ville Syrjälä 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älä wrote:
> >>>>>> On Tue, Apr 07, 2020 at 06:54:09PM +0300, Juha-Pekka Heikkila wrote:
> >>>>>>> On 7.4.2020 18.36, Ville Syrjälä wrote:
> >>>>>>>> On Tue, Apr 07, 2020 at 02:09:04PM +0300, Juha-Pekka Heikkila wrote:
> >>>>>>>>> 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/igt@kms_plane@pixel-format-pipe-c-planes-source-clamping.html
> >>>>>>>
> >>>>>>>
> >>>>>>> where going to 64bpp pixel format will cause modeset and fail running
> >>>>>>> test.
> >>>>>>
> >>>>>> Hmm. Seems we don't have drm.debug=0x10 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 idea
> >>>>> modeset will block the flip and making this commit blocking would always
> >>>>> 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 never
> >>>> 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 == 0
> >> (kms_plane:31238) igt_kms-CRITICAL: Last errno: 16, Device or resource busy
> >> (kms_plane:31238) igt_kms-CRITICAL: error: -16 != 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 <juhapekka.heikkila@gmail.com>
> >>>>>>>>> ---
> >>>>>>>>>      tests/kms_plane.c | 6 ++----
> >>>>>>>>>      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 *data,
> >>>>>>>>> enum pipe pipe,
> >>>>>>>>>              if (data->display.is_atomic) {
> >>>>>>>>>                  /*
> >>>>>>>>> -             * Use non-blocking commits to allow the next fb
> >>>>>>>>> -             * to be prepared in parallel while the current fb
> >>>>>>>>> -             * awaits to be latched.
> >>>>>>>>> +             * Use blocking commit because there maybe
> >>>>>>>>> +             * modeset when going to higher bpp pixel format.
> >>>>>>>>>                   */
> >>>>>>>>>                  igt_display_commit_atomic(&data->display,
> >>>>>>>>> -                          DRM_MODE_ATOMIC_NONBLOCK |
> >>>>>>>>>                                DRM_MODE_PAGE_FLIP_EVENT, NULL);
> >>>>>>>>>              } else {
> >>>>>>>>>                  /*
> >>>>>>>>> -- 
> >>>>>>>>> 2.17.1
> >>>>>>>>>
> >>>>>>>>> _______________________________________________
> >>>>>>>>> igt-dev mailing list
> >>>>>>>>> igt-dev@lists.freedesktop.org
> >>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/igt-dev
> >>>>>>>>
> >>>>>>
> >>>>>
> >>>
> > 

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev