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

Juha-Pekka Heikkila
2022-11-01T17:31:23Z

On 1.11.2022 18.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.

Well, the structure is definitely incorrect if failure in cursor
framebuffer creation will cause SIGABRT. I didn't go figuring out where
it goes wrong, I just was interested in getting tests running correctly.

In the mean time I created revert patch which probably doesn't lose
anything written on top of dynamic tests. Does this cause test runs go
correct Alaa? https://patchwork.freedesktop.org/series/110386/

/Juha-Pekka

>
> 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?
>
> [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);
>>>>>        }
>>>>>    }
>>
>