From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by gabe.freedesktop.org (Postfix) with ESMTPS id A070610E404 for ; Tue, 1 Nov 2022 16:51:18 +0000 (UTC) Message-ID: Date: Tue, 1 Nov 2022 22:20:56 +0530 Content-Language: en-US To: , aemad References: <20221009151844.3456-1-aemad@igalia.com> <31ec4c5751bfda7c1f174b9e0c898280@igalia.com> <3703439d-a975-3756-00b1-a925d16f9e9a@gmail.com> From: "Modem, Bhanuprakash" In-Reply-To: <3703439d-a975-3756-00b1-a925d16f9e9a@gmail.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_cursor_crc: prevent the entire test from being aborted when a subtest fails List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: igt-dev@lists.freedesktop.org, petri.latvala@intel.com Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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 >>>> --- >>>>    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); >>>>        } >>>>    } >