[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 at intel.com
Tue Nov 1 16:50:56 UTC 2022
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 at 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);
>>>> }
>>>> }
>
More information about the igt-dev
mailing list