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

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

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?

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