Re: [igt-dev] [PATCH i-g-t] tests/kms_cursor_crc: prevent the entire test from being aborted when a subtest fails

[aemad <aemad@igalia.com>]

On 2022-11-05 21:17, aemad wrote:
> On 2022-11-04 03:07, Modem, Bhanuprakash wrote:
>> On Fri-04-11-2022 01:06 am, aemad wrote:
>>> Hi Bhanuprakash,
>>>
>>> On 2022-11-03 18:07, aemad wrote:
>>>> On 2022-11-02 12:25, Modem, Bhanuprakash wrote:
>>>>> On Wed-02-11-2022 12:19 am, aemad wrote:
>>>>>> On 2022-11-01 17:50, Modem, Bhanuprakash wrote:
>>>>>>> Hi JP,
>>>>>>>
>>>>>>> On Tue-01-11-2022 08:41 pm, Juha-Pekka Heikkila wrote:
>>>>>>>> On 1.11.2022 13.25, aemad wrote:
>>>>>>>>> On 2022-10-10 13:16, Juha-Pekka Heikkila wrote:
>>>>>>>>>> Hi Alaa,
>>>>>>>>>>
>>>>>>>>>> what's that invalid argument you're getting on addfb? I never tried
>>>>>>>>>> this with vkms, normally just on i915. With i915 I cannot reproduce
>>>>>>>>>> that abort you're seeing. Other kms tests run on your setup?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Juha-Pekka,
>>>>>>>>>
>>>>>>>>>> Anyway, that revert you're suggesting would remove support for msm
>>>>>>>>>> driver on this test. Let's first summon those who made these changes
>>>>>>>>>> to comment. (cc'ing Modem and Swati)
>>>>>>>>>
>>>>>>>>> Could you please explain why this patch will remove MSM driver support?
>>>>>>>>>
>>>>>>>>
>>>>>>>> See in test what's done with data.vblank_wait_count variable. That is there because on msm legacy cursor is not updated asynchronously and msm cursor likely will miss even second frame. Taking that out will cause these tests reliably fail on msm.
>>>>>>>>
>>>>>>>> I was just today because of unrelated cursor issues looking into this, I still don't have patch though. As Swati/Modem didn't come back with a fix I was looking to take out all dynamic stuff, it should solve this issue.
>>>>>>>
>>>>>>> I am not sure, how this issue is related to dynamic subtests.
>>>>>>>
>>>>>>> Expecting that we need to skip the test if the device is vkms & cursor
>>>>>>> height/width < 20 px. From IGT, is there any way to identify the
>>>>>>> device is VKMS or not?
>>>>>>>
>>>>>>> Apart from this, I have floated patch with some minor cleanup [1].
>>>>>>> @ Alaa, can you please help to check if it helps?
>>>>>>
>>>>>> yes, sure I can check it.
>>>>>
>>>>> Floated a fix: https://patchwork.freedesktop.org/series/110407/
>>>>>
>>>>> I have adopted the approach from your patch (also kept your Sob) &
>>>>> fixed the usage of *-max-size tests.
>>>>>
>>>>> Please check if it works.
>>>>
>>>> Sure, I will.
>>>>
>>>> - Alaa
>>>>>
>>> Still, the max-size test doesn't include.
>>
>> Sorry, somehow I have floated a old rev :-(
> 
> Never mind, I will recheck
> 
> - Alaa
>>
>> Here is the updated fix: https://patchwork.freedesktop.org/series/110407/

I checked it, and it works as expected.

- Alaa
>>
>> - Bhanu
>>
>>>
>>> - Alaa
>>>
>>>>> - Bhanu
>>>>>
>>>>>>
>>>>>>>
>>>>>>> [1]: https://patchwork.freedesktop.org/series/110381/
>>>>>>>
>>>>>>> - Bhanu
>>>>>>>
>>>>>>>>
>>>>>>>> /Juha-Pekka
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> /Juha-Pekka
>>>>>>>>>>
>>>>>>>>>> On 9.10.2022 18.18, Alaa Emad wrote:
>>>>>>>>>>> Any test failure prevents the other tests to be run, aborting the entire test loop. This behavior was introduced by commit
>>>>>>>>>>> 9494d53d ("tests/kms_cursor_crc: Convert tests to dynamic").
>>>>>>>>>>> For instance, when running the cursor-offscreen-32x10 subtest using VKMS,
>>>>>>>>>>> the execution is aborted instead of SKIP/FAIL, as in the following output:
>>>>>>>>>>>
>>>>>>>>>>> $ ./tests/kms_cursor_crc --run-subtest cursor-offscreen-32x10
>>>>>>>>>>> (kms_cursor_crc:7447) igt_fb-CRITICAL: Test assertion failure function igt_create_fb_with_bo_size, file ../lib/igt_fb.c:2078:
>>>>>>>>>>> (kms_cursor_crc:7447) igt_fb-CRITICAL: Failed assertion: (__kms_addfb(fb->fd, fb->gem_handle, fb->width, fb->height, fb->drm_format, fb->modifier, fb->strides, fb->offsets, fb->num_planes, flags, &fb->fb_id)) == 0
>>>>>>>>>>> (kms_cursor_crc:7447) igt_fb-CRITICAL: Last errno: 22, Invalid argument
>>>>>>>>>>> Stack trace:
>>>>>>>>>>> kms_cursor_crc: ../lib/igt_core.c:1667: igt_fail: Assertion `_igt_dynamic_tests_executed < 0 || dynamic_failed_one' failed.
>>>>>>>>>>> Received signal SIGABRT.
>>>>>>>>>>>
>>>>>>>>>>> The test was aborted after convertig tests to dynamic with cursor size 32*10 that because of the assertion in the `igt_create_fb_with_bo_size` when creating cursor in `create_cursor_fb`.
>>>>>>>>>>>
>>>>>>>>>>> This happened because Within a igt_subtest_with_dynamic block, explicit failure (e.g. igt_assert) is not allowed,
>>>>>>>>>>> only dynamic subsubtests themselves will produce test results.
>>>>>>>>>>>
>>>>>>>>>>> The previous approach was running each test for all cursor sizes then move to the next test  so fix this issue
>>>>>>>>>>> by running all tests for one cursor size one by one, and check if cursor size is
>>>>>>>>>>> supported or not before running the test.
>>>>>>>>>>>
>>>>>>>>>>> Fixes: 9494d53d ("tests/kms_cursor_crc: Convert tests to dynamic")
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Alaa Emad <aemad@igalia.com>
>>>>>>>>>>> ---
>>>>>>>>>>>      tests/kms_cursor_crc.c | 136 +++++++++++++++++++++--------------------
>>>>>>>>>>>      1 file changed, 69 insertions(+), 67 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
>>>>>>>>>>> index 8d3426dd..9b620389 100644
>>>>>>>>>>> --- a/tests/kms_cursor_crc.c
>>>>>>>>>>> +++ b/tests/kms_cursor_crc.c
>>>>>>>>>>> @@ -72,7 +72,6 @@ typedef struct {
>>>>>>>>>>>          cairo_surface_t *surface;
>>>>>>>>>>>          uint32_t devid;
>>>>>>>>>>>          double alpha;
>>>>>>>>>>> -    int vblank_wait_count; /* because of msm */
>>>>>>>>>>>      } data_t;
>>>>>>>>>>>        #define TEST_DPMS (1<<0)
>>>>>>>>>>> @@ -125,12 +124,6 @@ static void cursor_disable(data_t *data)
>>>>>>>>>>>      {
>>>>>>>>>>>          igt_plane_set_fb(data->cursor, NULL);
>>>>>>>>>>>          igt_plane_set_position(data->cursor, 0, 0);
>>>>>>>>>>> -    igt_display_commit(&data->display);
>>>>>>>>>>> -
>>>>>>>>>>> -    /* do this wait here so it will not need to be added everywhere */
>>>>>>>>>>> -    igt_wait_for_vblank_count(data->drm_fd,
>>>>>>>>>>> -                  data->display.pipes[data->pipe].crtc_offset,
>>>>>>>>>>> -                  data->vblank_wait_count);
>>>>>>>>>>>      }
>>>>>>>>>>>        static bool chv_cursor_broken(data_t *data, int x)
>>>>>>>>>>> @@ -209,9 +202,8 @@ static void do_single_test(data_t *data, int x, int y, bool hw_test,
>>>>>>>>>>>              igt_display_commit(display);
>>>>>>>>>>>                /* Extra vblank wait is because nonblocking cursor ioctl */
>>>>>>>>>>> -        igt_wait_for_vblank_count(data->drm_fd,
>>>>>>>>>>> -                      display->pipes[data->pipe].crtc_offset,
>>>>>>>>>>> -                      data->vblank_wait_count);
>>>>>>>>>>> +        igt_wait_for_vblank(data->drm_fd,
>>>>>>>>>>> +                display->pipes[data->pipe].crtc_offset);
>>>>>>>>>>>                igt_pipe_crc_get_current(data->drm_fd, pipe_crc, hwcrc);
>>>>>>>>>>>      @@ -249,7 +241,11 @@ 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);
>>>>>>>>>>> +        igt_wait_for_vblank(data->drm_fd,
>>>>>>>>>>> +                display->pipes[data->pipe].crtc_offset);
>>>>>>>>>>> +
>>>>>>>>>>>              igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &crc);
>>>>>>>>>>>              igt_assert_crc_equal(&crc, hwcrc);
>>>>>>>>>>>          }
>>>>>>>>>>> @@ -267,7 +263,9 @@ static void do_fail_test(data_t *data, int x, int y, int expect)
>>>>>>>>>>>          igt_plane_set_position(data->cursor, x, y);
>>>>>>>>>>>          ret = igt_display_try_commit2(display, COMMIT_LEGACY);
>>>>>>>>>>>      +    igt_plane_set_position(data->cursor, 0, 0);
>>>>>>>>>>>          cursor_disable(data);
>>>>>>>>>>> +    igt_display_commit(display);
>>>>>>>>>>>            igt_assert_eq(ret, expect);
>>>>>>>>>>>      }
>>>>>>>>>>> @@ -689,10 +687,28 @@ static void test_rapid_movement(data_t *data)
>>>>>>>>>>>          igt_assert_lt(usec, 0.9 * 400 * 1000000 / data->refresh);
>>>>>>>>>>>      }
>>>>>>>>>>>      -static void run_size_tests(data_t *data, void (*testfunc)(data_t *),
>>>>>>>>>>> -               int w, int h)
>>>>>>>>>>> +static void run_size_tests(data_t *data, int w, int h)
>>>>>>>>>>>      {
>>>>>>>>>>>          enum pipe pipe;
>>>>>>>>>>> +    int i;
>>>>>>>>>>> +
>>>>>>>>>>> +    struct
>>>>>>>>>>> +    {
>>>>>>>>>>> +        const char *name;
>>>>>>>>>>> +        void (*testfunc)(data_t *);
>>>>>>>>>>> +        const char *desc;
>>>>>>>>>>> +    } size_tests[] = {
>>>>>>>>>>> +        {"cursor-onscreen", test_crc_onscreen,
>>>>>>>>>>> +         "Check if a given-size cursor is well-positioned inside the screen."},
>>>>>>>>>>> +        {"cursor-offscreen", test_crc_offscreen,
>>>>>>>>>>> +         "Check if a given-size cursor is well-positioned outside the screen."},
>>>>>>>>>>> +        {"cursor-sliding", test_crc_sliding,
>>>>>>>>>>> +         "Check the smooth and pixel-by-pixel given-size cursor movements on horizontal, vertical and diagonal."},
>>>>>>>>>>> +        {"cursor-random", test_crc_random,
>>>>>>>>>>> +         "Check random placement of a cursor with given size."},
>>>>>>>>>>> +        {"cursor-rapid-movement", test_rapid_movement,
>>>>>>>>>>> +         "Check the rapid update of given-size cursor movements."},
>>>>>>>>>>> +    };
>>>>>>>>>>>            if (w == 0 && h == 0) {
>>>>>>>>>>>              w = data->cursor_max_w;
>>>>>>>>>>> @@ -709,45 +725,38 @@ static void run_size_tests(data_t *data, void (*testfunc)(data_t *),
>>>>>>>>>>>              }
>>>>>>>>>>>          }
>>>>>>>>>>>      -    create_cursor_fb(data, w, h);
>>>>>>>>>>> -    if (require_cursor_size(data, w, h)) {
>>>>>>>>>>> -        igt_info("Cursor size %dx%d not supported by driver\n", w, h);
>>>>>>>>>>> -
>>>>>>>>>>> -        igt_remove_fb(data->drm_fd, &data->fb);
>>>>>>>>>>> -        return;
>>>>>>>>>>> -    }
>>>>>>>>>>> +    igt_fixture
>>>>>>>>>>> +        create_cursor_fb(data, w, h);
>>>>>>>>>>>      -    for_each_pipe(&data->display, pipe) {
>>>>>>>>>>> -        data->pipe = pipe;
>>>>>>>>>>> -        igt_dynamic_f("pipe-%s-%s",
>>>>>>>>>>> -                  kmstest_pipe_name(pipe), igt_output_name(data->output))
>>>>>>>>>>> -            run_test(data, testfunc, w, h);
>>>>>>>>>>> +    for (i = 0; i < ARRAY_SIZE(size_tests); i++)
>>>>>>>>>>> +    {
>>>>>>>>>>> +        igt_describe(size_tests[i].desc);
>>>>>>>>>>> +        igt_subtest_with_dynamic_f("%s-%dx%d", size_tests[i].name, w, h)
>>>>>>>>>>> +        {
>>>>>>>>>>> +            for_each_pipe(&data->display, pipe)
>>>>>>>>>>> +            {
>>>>>>>>>>> +                data->pipe = pipe;
>>>>>>>>>>> +                if (require_cursor_size(data, w, h))
>>>>>>>>>>> +                {
>>>>>>>>>>> +                    igt_info("Cursor size %dx%d not supported by driver\n", w, h);
>>>>>>>>>>> +                    continue;
>>>>>>>>>>> +                }
>>>>>>>>>>> +
>>>>>>>>>>> +                igt_dynamic_f("pipe-%s-%s",
>>>>>>>>>>> +                              kmstest_pipe_name(pipe), igt_output_name(data->output))
>>>>>>>>>>> +                    run_test(data, size_tests[i].testfunc, w, h);
>>>>>>>>>>> +            }
>>>>>>>>>>> +        }
>>>>>>>>>>>          }
>>>>>>>>>>>      -    igt_remove_fb(data->drm_fd, &data->fb);
>>>>>>>>>>> +    igt_fixture
>>>>>>>>>>> +        igt_remove_fb(data->drm_fd, &data->fb);
>>>>>>>>>>>      }
>>>>>>>>>>>        static void run_tests_on_pipe(data_t *data)
>>>>>>>>>>>      {
>>>>>>>>>>>          enum pipe pipe;
>>>>>>>>>>>          int cursor_size;
>>>>>>>>>>> -    int i;
>>>>>>>>>>> -    struct {
>>>>>>>>>>> -        const char *name;
>>>>>>>>>>> -        void (*testfunc)(data_t *);
>>>>>>>>>>> -        const char *desc;
>>>>>>>>>>> -    } size_tests[] = {
>>>>>>>>>>> -        { "cursor-onscreen", test_crc_onscreen,
>>>>>>>>>>> -            "Check if a given-size cursor is well-positioned inside the screen." },
>>>>>>>>>>> -        { "cursor-offscreen", test_crc_offscreen,
>>>>>>>>>>> -            "Check if a given-size cursor is well-positioned outside the screen." },
>>>>>>>>>>> -        { "cursor-sliding", test_crc_sliding,
>>>>>>>>>>> -            "Check the smooth and pixel-by-pixel given-size cursor movements on horizontal, vertical and diagonal." },
>>>>>>>>>>> -        { "cursor-random", test_crc_random,
>>>>>>>>>>> -            "Check random placement of a cursor with given size." },
>>>>>>>>>>> -        { "cursor-rapid-movement", test_rapid_movement,
>>>>>>>>>>> -            "Check the rapid update of given-size cursor movements." },
>>>>>>>>>>> -    };
>>>>>>>>>>>            igt_fixture {
>>>>>>>>>>>              data->output = igt_get_single_output_for_pipe(&data->display, pipe);
>>>>>>>>>>> @@ -848,30 +857,26 @@ static void run_tests_on_pipe(data_t *data)
>>>>>>>>>>>          igt_fixture
>>>>>>>>>>>              igt_remove_fb(data->drm_fd, &data->fb);
>>>>>>>>>>>      -    for (i = 0; i < ARRAY_SIZE(size_tests); i++) {
>>>>>>>>>>> -        igt_describe(size_tests[i].desc);
>>>>>>>>>>> -        igt_subtest_group {
>>>>>>>>>>> -            for (cursor_size = 32; cursor_size <= 512; cursor_size *= 2) {
>>>>>>>>>>> -                int w = cursor_size;
>>>>>>>>>>> -                int h = cursor_size;
>>>>>>>>>>> -
>>>>>>>>>>> -                igt_subtest_with_dynamic_f("%s-%dx%d", size_tests[i].name, w, h)
>>>>>>>>>>> -                    run_size_tests(data, size_tests[i].testfunc, w, h);
>>>>>>>>>>> -
>>>>>>>>>>> -                /*
>>>>>>>>>>> -                 * Test non-square cursors a bit on the platforms
>>>>>>>>>>> -                 * that support such things. And make it a bit more
>>>>>>>>>>> -                 * interesting by using a non-pot height.
>>>>>>>>>>> -                 */
>>>>>>>>>>> -                h /= 3;
>>>>>>>>>>> -                igt_subtest_with_dynamic_f("%s-%dx%d", size_tests[i].name, w, h)
>>>>>>>>>>> -                    run_size_tests(data, size_tests[i].testfunc, w, h);
>>>>>>>>>>> -            }
>>>>>>>>>>> +    for (cursor_size = 32; cursor_size <= 512; cursor_size *= 2)
>>>>>>>>>>> +    {
>>>>>>>>>>> +        int w = cursor_size;
>>>>>>>>>>> +        int h = cursor_size;
>>>>>>>>>>>      -            igt_subtest_with_dynamic_f("%s-max-size", size_tests[i].name)
>>>>>>>>>>> -                run_size_tests(data, size_tests[i].testfunc, 0, 0);
>>>>>>>>>>> -        }
>>>>>>>>>>> +        igt_subtest_group
>>>>>>>>>>> +            run_size_tests(data, w, h);
>>>>>>>>>>> +
>>>>>>>>>>> +        /*
>>>>>>>>>>> +         * Test non-square cursors a bit on the platforms
>>>>>>>>>>> +         * that support such things. And make it a bit more
>>>>>>>>>>> +         * interesting by using a non-pot height.
>>>>>>>>>>> +         */
>>>>>>>>>>> +        h /= 3;
>>>>>>>>>>> +
>>>>>>>>>>> +        igt_subtest_group
>>>>>>>>>>> +            run_size_tests(data, w, h);
>>>>>>>>>>>          }
>>>>>>>>>>> +
>>>>>>>>>>> +    run_size_tests(data, 0, 0);
>>>>>>>>>>>      }
>>>>>>>>>>>        static data_t data;
>>>>>>>>>>> @@ -896,8 +901,6 @@ igt_main
>>>>>>>>>>>              kmstest_set_vt_graphics_mode();
>>>>>>>>>>>                igt_require_pipe_crc(data.drm_fd);
>>>>>>>>>>> -
>>>>>>>>>>> -        data.vblank_wait_count = is_msm_device(data.drm_fd) ? 2 : 1;
>>>>>>>>>>>          }
>>>>>>>>>>>            data.cursor_max_w = cursor_width;
>>>>>>>>>>> @@ -913,6 +916,5 @@ igt_main
>>>>>>>>>>>              }
>>>>>>>>>>>                igt_display_fini(&data.display);
>>>>>>>>>>> -        close(data.drm_fd);
>>>>>>>>>>>          }
>>>>>>>>>>>      }
>>>>>>>>