[igt-dev] ✗ Fi.CI.BAT: failure for tests: Add variable refresh rate tests (rev2)

[Kazlauskas, Nicholas]

On 1/28/19 5:41 AM, Petri Latvala wrote:
> On Fri, Jan 25, 2019 at 05:21:41PM +0000, Kazlauskas, Nicholas wrote:
>> On 1/25/19 11:43 AM, Saarinen, Jani wrote:
>>> HI,
>>>
>>>> -----Original Message-----
>>>> From: igt-dev [mailto:igt-dev-bounces at lists.freedesktop.org] On Behalf Of
>>>> Kazlauskas, Nicholas
>>>> Sent: perjantai 25. tammikuuta 2019 17.21
>>>> To: igt-dev at lists.freedesktop.org; Latvala, Petri <petri.latvala at intel.com>
>>>> Subject: Re: [igt-dev] ✗ Fi.CI.BAT: failure for tests: Add variable refresh rate tests
>>>> (rev2)
>>>>
>>>> On 1/25/19 4:57 AM, Petri Latvala wrote:
>>>>> On Thu, Jan 10, 2019 at 11:09:09AM +0000, Patchwork wrote:
>>>>>> == Series Details ==
>>>>>>
>>>>>> Series: tests: Add variable refresh rate tests (rev2)
>>>>>> URL   : https://patchwork.freedesktop.org/series/54960/
>>>>>> State : failure
>>>>>>
>>>>>> == Summary ==
>>>>>>
>>>>>> CI Bug Log - changes from IGT_4758 -> IGTPW_2212
>>>>>> ====================================================
>>>>>>
>>>>>> Summary
>>>>>> -------
>>>>>>
>>>>>>      **FAILURE**
>>>>>>
>>>>>>      Serious unknown changes coming with IGTPW_2212 absolutely need to be
>>>>>>      verified manually.
>>>>>>
>>>>>>      If you think the reported changes have nothing to do with the changes
>>>>>>      introduced in IGTPW_2212, please notify your bug team to allow them
>>>>>>      to document this new failure mode, which will reduce false positives in CI.
>>>>>>
>>>>>>      External URL:
>>>>>> https://patchwork.freedesktop.org/api/1.0/series/54960/revisions/2/
>>>>>>
>>>>>> Possible new issues
>>>>>> -------------------
>>>>>>
>>>>>>      Here are the unknown changes that may have been introduced in
>>>> IGTPW_2212:
>>>>>>
>>>>>> ### IGT changes ###
>>>>>>
>>>>>> #### Possible regressions ####
>>>>>>
>>>>>>      * igt at kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
>>>>>>        - fi-pnv-d510:        PASS -> FAIL +19
>>>>>>
>>>>>>      * igt at kms_pipe_crc_basic@hang-read-crc-pipe-b:
>>>>>>        - fi-blb-e6850:       PASS -> FAIL +18
>>>>>>
>>>>>>      * igt at kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
>>>>>>        - fi-elk-e7500:       PASS -> FAIL +19
>>>>>>
>>>>>>      * igt at kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
>>>>>>        - fi-gdg-551:         PASS -> FAIL +19
>>>>>>
>>>>>>      * igt at kms_pipe_crc_basic@read-crc-pipe-a:
>>>>>>        - fi-bwr-2160:        PASS -> FAIL +19
>>>>>>
>>>>>
>>>>>
>>>>> Just about all of these failures are this:
>>>>>
>>>>>
>>>>> (kms_cursor_legacy:2598) igt_kms-DEBUG: display: commit {
>>>>> (kms_cursor_legacy:2598) igt_kms-CRITICAL: Test assertion failure function
>>>> igt_pipe_commit, file ../lib/igt_kms.c:2780:
>>>>> (kms_cursor_legacy:2598) igt_kms-CRITICAL: Failed assertion:
>>>>> pipe->props[i]
>>>>> (kms_cursor_legacy:2598) igt_core-INFO: Stack trace:
>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #0 ../lib/igt_core.c:1472
>>>> __igt_fail_assert()
>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #1 ../lib/igt_kms.c:2782
>>>> igt_pipe_commit()
>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #2 ../lib/igt_kms.c:3313
>>>> do_display_commit()
>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #3 ../lib/igt_kms.c:3428
>>>> igt_display_commit2()
>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #4 ../tests/kms_cursor_legacy.c:522
>>>> basic_flip_cursor()
>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #5
>>>> ../tests/kms_cursor_legacy.c:1491 __real_main1358()
>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #6
>>>> ../tests/kms_cursor_legacy.c:1358 main()
>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #7 ../csu/libc-start.c:344
>>>> __libc_start_main()
>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #8 [_start+0x2a]
>>>>>
>>>>
>>>> I think that's expected.
>>>>
>>>> The vrr_enabled property is a default one in the kernel for 5.0+ but doesn't exist
>>>> before that. Since it's default and could affect other test results if left enabled
>>>> from another test I chose to add it to the list of default properties being reset. So
>>>> it was probably missing the property in those tests and failing.
>>>>
>>>> I don't know which version of the kernel was running the test but I don't think it
>>>> was 5.0 at least (considering the date it ran).
>>> Well...
>>> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2212/fi-elk-e7500/boot0.log
>>> Linux version 5.0.0-rc1-CI-CI_DRM_5390+
>>
>> Oh, thanks! I didn't know there was an easy way to view the actual dmesg
>> log.
>>
>> However, that assertion is definitely indicating that one of the pipe
>> properties (likely vrr_enabled) doesn't have a valid prop_id (likely
>> because it wasn't found).
>>
>> FWIW I don't see these failures locally when testing with the patch in
>> the tree, but I'm also still on a 4.20 tree with the VRR patches
>> applied. I'm pretty sure the VRR patches were targeting 5.0 at least so
>> those patches should be in...
> 
> 
> All the machines with the fails were old and for gen < 5 we don't have
> DRIVER_ATOMIC. vrr_enabled is an atomic-only prop?
> 
> We support testing old kernels as well so some effort needs to be put
> towards that anyway. Clearing the vrr prop when resetting to default
> is good, but sprinkle some igt_*_has_prop() around it.
> 
> 

That sounds like what's happening here - the property is only ever 
attached for atomic drivers. That check should be sufficient in solving 
the issue, thanks for the help!

Nicholas Kazlauskas