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

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

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.

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