[igt-dev] [PATCH] [i-g-t] tests/kms_cursor_crc: Fix test intermittent failures on AMD gpu

[Wu, Hersen]

[AMD Official Use Only - General]

Hi Kamil,

igt_wait_for_vblank_count has been added at two locations:

1.  do_single_test for hw_test
2.  cursor_disable

As for your review:
> as for msm driver we also need to wait? Btw how it worked before for
> msm without that wait here?

By AMD GPU/Driver cursor update implementation, pipe CRC could not be generated reliably within one vblank for both HW and SW cursor.
This is why I add igt_wait_for_vblank_count at do_single_test  -- sw_test for AMD GPU only.
Without igt_wait_for_vblank_count at do_single_test for sw_test, these tests fail intermittently on AMD GPU.

I do not have msm GPU to run test. My change is guarded with is_amdgpu_device.



As for your review:
 If you need to wait 2+2 maybe set it
> already to 4 and drop this hunk?


My analysis:


With vblank_wait_count =2 for AMD GPU, cursor_disable does wait for 2 vblanks.

With igt_wait_for_vblank_count at do_single_test for sw_test.

But these waiting for 2 vblanks apply to sw test loop 0.
For loop 0, wait for 4 vblanks.

For sub-tests, test_crc_onscreen, test_crc_sliding, test_crc_random, wait for vblank are not applied since loop 1. Wait for 2 vblanks.


To simply IGT change, I leave wait for 4 blanks for loop 0.


test_crc_onscreen
{
        /* SW test */
        cursor_disable(data);
        for (int i = 0; i < ARRAY_SIZE(tests); i++)
                do_test(data, &tests[i].coords, tests[i].crc, false);
}

test_crc_sliding
{
        /* SW test */
        cursor_disable(data);
        for (i = 0; i < ARRAY_SIZE(rounds); i++) {
                do_single_test(data, i, 0, false, &rounds[i].crc[0]);
                do_single_test(data, 0, i, false, &rounds[i].crc[1]);
                do_single_test(data, i, i, false, &rounds[i].crc[2]);
        }
}

test_crc_random
{
        /* SW test */
        cursor_disable(data);
        for (i = 0; i < max; i++)
                do_single_test(data, x[i], y[i], false, &crc[i]);
}


Thanks!
Hersen


-----Original Message-----
From: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
Sent: Friday, October 13, 2023 7:49 AM
To: Kamil Konieczny <kamil.konieczny at linux.intel.com>; igt-dev at lists.freedesktop.org; Wu, Hersen <hersenxs.wu at amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira at amd.com>; Pillai, Aurabindo <Aurabindo.Pillai at amd.com>; Hung, Alex <Alex.Hung at amd.com>; Mahfooz, Hamza <Hamza.Mahfooz at amd.com>; Li, Sun peng (Leo) <Sunpeng.Li at amd.com>; markyacoub at google.com
Subject: Re: [igt-dev] [PATCH] [i-g-t] tests/kms_cursor_crc: Fix test intermittent failures on AMD gpu

On 13.10.2023 14.38, Kamil Konieczny wrote:
> Hi Hersen,
> On 2023-10-02 at 10:20:23 -0400, Hersen Wu wrote:
>> Wait for two more vblanks before reading crc on AMD gpu.
>>
>> Without waiting for two vblanks, AMD cursor updates may not
>> synchronized to the same frame of pipe, crc generated may not be
>> reliable.
>>
>> Signed-off-by: Hersen Wu <hersenxs.wu at amd.com>
>> ---
>>   tests/kms_cursor_crc.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c index
>> ba29ff65d..e3259e147 100644
>> --- a/tests/kms_cursor_crc.c
>> +++ b/tests/kms_cursor_crc.c
>> @@ -276,6 +276,15 @@ static void do_single_test(data_t *data, int x, int y, bool hw_test,
>>              restore_image(data, swbufidx, &((cursorarea){x, y, data->curw, data->curh}));
>>              igt_plane_set_fb(data->primary, &data->primary_fb[swbufidx]);
>>              igt_display_commit(display);
>> +
>> +            /* Wait for two more vblanks since cursor updates may not
>> +             * synchronized to the same frame on AMD HW
>> +             */
>> +            if (is_amdgpu_device(data->drm_fd))
> ----------- ^^^^^^^^^^^^^^^^
> Why not
>          if(data->vblank_wait_count)
>
> as for msm driver we also need to wait? Btw how it worked before for
> msm without that wait here? If you need to wait 2+2 maybe set it
> already to 4 and drop this hunk?
>

As I understood before issue with msm is different. With msm it's about async cursor settling in place while on amd its about crc settling.
Here's normal flip, nothing about cursor here, hence no point in waiting with msm. That comment above check for amd will need to be updated.

/Juha-Pekka

>> +                    igt_wait_for_vblank_count(data->drm_fd,
>> +                            display->pipes[data->pipe].crtc_offset,
>> +                            data->vblank_wait_count);
>> +
>>              igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &crc);
>>              igt_assert_crc_equal(&crc, hwcrc);
>>      }
>> @@ -1079,7 +1088,11 @@ igt_main_args("e", NULL, help_str,
>> opt_handler, NULL)
>>
>>              igt_require_pipe_crc(data.drm_fd);
>>
>> -            data.vblank_wait_count = is_msm_device(data.drm_fd) ? 2 : 1;
>> +            /* Wait for two more vblanks since cursor updates may not
>> +             * synchronized to the same frame on AMD HW
>> +             */
>> +            data.vblank_wait_count =
>> +                    (is_msm_device(data.drm_fd) || is_amdgpu_device(data.drm_fd)) ? 2
>> +: 1;
>>      }
>>
>>      data.cursor_max_w = cursor_width;
>> --
>> 2.25.1
>>