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

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 9.4.2020 19.08, Ville Syrjälä wrote:
> 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 think that modeset somehow affects busyness. I can try this test w/o 
64bpp formats just to check out it will not fail otherwise with same 
kernel/machine. Now it seems when it's busy it's at the moment of modeset.

>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
> 

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev