From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id BCEA310E402 for ; Tue, 1 Nov 2022 16:44:06 +0000 (UTC) Message-ID: Date: Tue, 1 Nov 2022 22:13:44 +0530 Content-Language: en-US To: , aemad References: <20221009151844.3456-1-aemad@igalia.com> <3b7df5498d9c2d5e5d1541de3da3f4f1@igalia.com> <464abb16-5fd7-8e32-f743-64bb582dfdaa@gmail.com> <696545bda7927a48462d6b982069d25f@igalia.com> From: "Modem, Bhanuprakash" In-Reply-To: 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-11-10-2022 11:05 pm, Juha-Pekka Heikkila wrote: > On 11.10.2022 20.08, aemad wrote: >> On 2022-10-10 20:52, Juha-Pekka Heikkila wrote: >>> On 10.10.2022 17.26, aemad wrote: >>>> Hi Juha, >>>> >>>> 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 >>>> >>>> I am not sure what is the invalid argument, but the failure of this >>>> test >>>> with a cursor size 32x10 is that VKMS does not support this size yet so >>>> the test unable to create a cursor FB. >>> >>> At that moment when addfb gives failure it doesn't yet relate to >>> cursor testing, it is like any other framebuffer. As other kms tests >>> work for you there maybe limitation on vkms not supporting so small >>> framebuffer or it could be on libigt there's something missing for >>> handling vkms framebuffers. >> >> yes, understood. >>> >>> I'm looking at kernel sources and I suspect you'd see in dmesg message >>> saying  "GEM object size (%zu) smaller than minimum size (%u) for >>> plane %d\n" when that addfb failed? If it is that message then >>> probably into libigt for vkms there would be needed some minimum size >>> alignment with framebuffers. >> >> I didn't check dmesg message, but I will check it. I got your point, >> Thank you. > > After sending that mail I did notice minimum framebuffer height for vkms > is 20 pixels, that's where that addfb failure likely is coming from. It > mean is this 32x10 subtest would never work on vkms and this test will > need to be fixed to skip correctly again. So, we need to skip if the device is vkms & cursor height/width < 20 px. From IGT, is there any way to identify the device is VKMS or not? - Bhanu > > /Juha-Pekka > >>> >>>> >>>> >>>>> this with vkms, normally just on i915. With i915 I cannot reproduce >>>>> that abort you're seeing. Other kms tests run on your setup? >>>> >>>> I think the abortion related to VKMS driver only, but I didn't try to >>>> run it on i915. >>>> yes, I ran other kms test but with VKMS driver. >>>> >>>>> >>>>> 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) >>>> >>>> I see, so I can check that after Modem and Swati reply. >>>> >>>>> >>>>> /Juha-Pekka >>>> >>>> BR, >>>> Alaa >>>>> >>>>> 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); >>>>>>         } >>>>>>     } >