[igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Sep 18 11:32:48 UTC 2020
On Fri, Sep 18, 2020 at 12:13:18PM +0530, Karthik B S wrote:
> Asynchronous flips are issued using the page flip IOCTL.
> The test consists of two subtests. The first subtest waits for
> the page flip event to be received before giving the next flip,
> and the second subtest doesn't wait for page flip events.
>
> The test passes if the IOCTL is successful.
>
> v2: -Add authors in the test file. (Paulo)
> -Reduce the run time and timeouts to suit IGT needs. (Paulo)
> -Replace igt_debug's with igt_assert's to catch slow flips. (Paulo)
> -Follow IGT coding style regarding spaces. (Paulo)
> -Make set up code part of igt_fixture. (Paulo)
> -Skip the test if async flips are not supported. (Paulo)
> -Replace suggested-by. (Paulo)
> -Added description for test and subtests.
>
> v3: -Rename the test to kms_async_flips. (Paulo)
> -Modify the TODO comment. (Paulo)
> -Remove igt_debug in flip_handler. (Paulo)
> -Use drmIoctl() in has_async function. (Paulo)
> -Add more details in igt_assert in flip_handler. (Paulo)
> -Remove flag variable in flip_handler. (Paulo)
> -Call igt_assert in flip_handler after the warm up time.
>
> v4: -Calculate the time stamp in flip_handler from userspace, as the
> kernel will return vbl timestamps and this cannot be used
> for async flips.
> -Add a new subtest to verify that the async flip time stamp
> lies in between the previous and next vblank time stamp. (Daniel)
>
> v5: -Add test that alternates between sync and async flips. (Ville)
> -Add test to verify invalid async flips. (Ville)
> -Remove the subtest async-flip-without-page-flip-events. (Michel)
> -Remove the intel gpu restriction and make the test generic. (Michel)
>
> v6: -Change the THRESHOLD from 10 to 8 as failures are seen on CI
> on platforms <= gen10.
> -In older platforms(<= gen10), async address update bit in plane ctl
> is double buffered. Made changes in subtests to accomodate this.
> -Moved the igt_assert from flip_handler to individual subtest as we
> now have four subtests and adding conditions for the assert in
> flip handler is messy.
>
> v7: -Change flip_interval from int to float for more precision.
> -Remove the fb height change case in 'invalid' subtest as per the
> feedback received on the kernel patches.
> -Add subtest to verify legacy cursor IOCTL. (Ville)
>
> v8: -Add a cursor flip before async flip in cursor test. (Ville)
> -Make flip_interval double for more precision as failures are seen
> on older platforms on CI.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Signed-off-by: Karthik B S <karthik.b.s at intel.com>
> ---
> tests/Makefile.sources | 1 +
> tests/kms_async_flips.c | 428 ++++++++++++++++++++++++++++++++++++++++
> tests/meson.build | 1 +
> 3 files changed, 430 insertions(+)
> create mode 100644 tests/kms_async_flips.c
>
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 6ae95155..f32ea9cf 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -32,6 +32,7 @@ TESTS_progs = \
> feature_discovery \
> kms_3d \
> kms_addfb_basic \
> + kms_async_flips \
> kms_atomic \
> kms_atomic_interruptible \
> kms_atomic_transition \
> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
> new file mode 100644
> index 00000000..ef63ec45
> --- /dev/null
> +++ b/tests/kms_async_flips.c
> @@ -0,0 +1,428 @@
> +/*
> + * Copyright © 2020 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + * Paulo Zanoni <paulo.r.zanoni at intel.com>
> + * Karthik B S <karthik.b.s at intel.com>
> + */
> +
> +#include "igt.h"
> +#include "igt_aux.h"
> +#include <sys/ioctl.h>
> +#include <sys/time.h>
> +#include <poll.h>
> +
> +#define BUFS 4
I'd just truct igt_fb bufs[4], and replace all the other
"BUFS" usages with ARRAY_SIZE(bufs).
> +#define SYNC_FLIP 0
> +#define ASYNC_FLIP 1
enum
> +#define CURSOR_RES 64
Should query that from the kernel so that test is driver agnostic.
> +#define CURSOR_POS 128
> +
> +/*
> + * These constants can be tuned in case we start getting unexpected
> + * results in CI.
> + */
> +
> +#define WARM_UP_TIME 1
> +#define RUN_TIME 2
> +#define THRESHOLD 8
A rather non-descriptive name. I guess this is "min flips per frame" or
something along those lines?
> +
> +IGT_TEST_DESCRIPTION("Test asynchrous page flips.");
Check the spelling
> +
> +typedef struct {
> + int drm_fd;
> + uint32_t crtc_id;
> + struct igt_fb bufs[BUFS];
> + igt_display_t display;
> +} data_t;
> +
> +uint32_t refresh_rate;
> +unsigned long flip_timestamp_us;
> +double flip_interval;
Hmm. I wonder why these are outside the data_t?
> +
> +static drmModeConnectorPtr find_connector_for_modeset(data_t *data)
> +{
> + igt_output_t *output;
> + drmModeConnectorPtr ret = NULL;
> +
> + for_each_connected_output(&data->display, output) {
> + if (output->config.connector->count_modes > 0) {
> + ret = output->config.connector;
> + break;
> + }
> + }
> +
> + igt_assert_f(ret, "Connector NOT found\n");
> + return ret;
> +}
> +
> +static void flip_handler(int fd_, unsigned int sequence, unsigned int tv_sec,
> + unsigned int tv_usec, void *_data)
> +{
> + static double last_ms;
> + double cur_ms;
> + struct timespec ts;
> +
> + igt_assert(_data == NULL);
> +
> + clock_gettime(CLOCK_MONOTONIC, &ts);
> +
> + cur_ms = ts.tv_sec * 1000.0 + ts.tv_nsec / 1000000.0;
> +
> + flip_interval = cur_ms - last_ms;
> +
> + last_ms = cur_ms;
> +
> + flip_timestamp_us = ts.tv_sec * 1000000 + ts.tv_nsec / 1000;
> +}
> +
> +static void wait_flip_event(data_t *data)
> +{
> + int ret;
> + drmEventContext evctx;
> + struct pollfd pfd;
> +
> + evctx.version = 2;
> + evctx.vblank_handler = NULL;
> + evctx.page_flip_handler = flip_handler;
> +
> + pfd.fd = data->drm_fd;
> + pfd.events = POLLIN;
> + pfd.revents = 0;
> +
> + ret = poll(&pfd, 1, 2000);
> +
> + switch (ret) {
> + case 0:
> + igt_assert_f(0, "Flip Timeout\n");
> + break;
> + case 1:
> + ret = drmHandleEvent(data->drm_fd, &evctx);
> + igt_assert(ret == 0);
> + break;
> + default:
> + /* unexpected */
> + igt_assert(0);
> + }
> +}
> +
> +static void make_fb(data_t *data, struct igt_fb *fb,
> + drmModeConnectorPtr connector, int index)
> +{
> + uint32_t width, height;
> + int rec_width;
> +
> + width = connector->modes[0].hdisplay;
> + height = connector->modes[0].vdisplay;
> +
> + rec_width = width / (BUFS * 2);
> +
> + igt_create_fb(data->drm_fd, width, height, DRM_FORMAT_ARGB8888,
Make that XRGB8888. Otherwise some of our platforms won't support it.
> + LOCAL_I915_FORMAT_MOD_X_TILED, fb);
> + igt_draw_fill_fb(data->drm_fd, fb, 0x88);
> + igt_draw_rect_fb(data->drm_fd, NULL, NULL, fb, IGT_DRAW_MMAP_CPU,
> + rec_width * 2 + rec_width * index,
> + height / 4, rec_width,
> + height / 2, rand());
> +}
> +
> +static void has_monotonic_timestamp(int fd)
Would expect has_foo() to return a boolean. I would call this
require_monotonic_timestamp() or something. Or make this return
the cap (like the has_async() thing) and move the igt_require() to
the caller.
> +{
> + struct drm_get_cap cap = { .capability = DRM_CAP_TIMESTAMP_MONOTONIC };
> +
> + igt_assert(drmIoctl(fd, DRM_IOCTL_GET_CAP, &cap) == 0);
> +
> + igt_require_f(cap.value, "Monotonic timestamps not supported\n");
> +}
> +
> +static void test_async_flip(data_t *data, bool alternate_sync_async)
> +{
> + int ret, frame, warm_end_frame, count = 0;
> + long long int fps;
> + struct timeval start, end, diff;
> + bool toggle = SYNC_FLIP;
> + bool test_flip_interval = true;
> + bool warming_up = true;
> +
> + has_monotonic_timestamp(data->drm_fd);
> +
> + gettimeofday(&start, NULL);
> + frame = 1;
> + do {
> + int flags = DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT;
> +
> + if (alternate_sync_async) {
> + if (toggle == SYNC_FLIP) {
> + flags &= ~DRM_MODE_PAGE_FLIP_ASYNC;
> + test_flip_interval = false;
> + toggle = ASYNC_FLIP;
> + } else {
> + /* In older platforms (<= Gen10), async address update bit is double buffered.
> + * So flip timestamp can be verified only from the second flip.
> + * The first async flip just enables the async address update.
> + */
We may want to limit this stuff to just those specific platforms.
That will make sure new platforms coming in would fail the test
if this mechanism ever broke again in hw.
The logic might be easier to understand if we just did an explciit
extra async flip for the bad hw:
do {
if (sync) {
...
} else {
/* blah */
if (bad_hw) {
drmModePAgeflip(async);
wait();
}
...
}
drmModePageflip(async or sync);
...
}
Actually I might even suggest replacing this toggle stuff
with two explicit drmModePageflip() calls. One would be async,
the other would be sync. That way you don't have to keep
more than one iteration of the loop in your mind when
reading the code.
But I guess we can massage this later as well.
> + if (count == 0) {
> + count++;
> + test_flip_interval = false;
> + } else {
> + count = 0;
> + toggle = SYNC_FLIP;
> + test_flip_interval = true;
> + }
> + }
> + }
> +
> + ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> + data->bufs[frame % 4].fb_id,
> + flags, NULL);
> +
> + igt_assert(ret == 0);
> +
> + wait_flip_event(data);
> +
> + gettimeofday(&end, NULL);
> + timersub(&end, &start, &diff);
> +
> + /* 1s of warm-up time for the freq to stabilize */
> + if (warming_up && diff.tv_sec >= WARM_UP_TIME) {
> + warming_up = false;
> + warm_end_frame = frame;
> + start = end;
> + }
What's this warming thing?
> +
> + if (test_flip_interval && !warming_up) {
> + igt_assert_f(flip_interval < 1000.0 / (refresh_rate * THRESHOLD),
> + "Flip interval not significantly smaller than vblank interval\n"
> + "Flip interval: %lfms, Refresh Rate = %dHz, Threshold = %d\n",
> + flip_interval, refresh_rate, THRESHOLD);
> +
> + }
> +
> + frame++;
> + } while (diff.tv_sec < RUN_TIME);
> +
> + if (!alternate_sync_async) {
> + fps = (frame - warm_end_frame) * 1000 / RUN_TIME;
> + igt_assert_f((fps / 1000) > (refresh_rate * THRESHOLD),
> + "FPS should be significantly higher than the refresh rate\n");
> + }
> +}
> +
> +static void get_vbl_timestamp_us(data_t *data, unsigned long *vbl_time, unsigned int *seq)
> +{
> + drmVBlank wait_vbl;
> + uint32_t pipe_id_flag;
> + int pipe;
> +
> + memset(&wait_vbl, 0, sizeof(wait_vbl));
> + pipe = kmstest_get_pipe_from_crtc_id(data->drm_fd, data->crtc_id);
> + pipe_id_flag = kmstest_get_vbl_flag(pipe);
> +
> + wait_vbl.request.type = DRM_VBLANK_RELATIVE | pipe_id_flag;
> + wait_vbl.request.sequence = 1;
Looks like this is actually doing a vblank wait. So the function
name is a bit poor.
> +
> + igt_assert(drmIoctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &wait_vbl) == 0);
> + *vbl_time = wait_vbl.reply.tval_sec * 1000000 + wait_vbl.reply.tval_usec;
> + *seq = wait_vbl.reply.sequence;
> +}
> +
> +static void test_timestamp(data_t *data)
> +{
> + int flags = DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT;
> + unsigned long vbl_time, vbl_time1;
> + unsigned int seq, seq1;
> + int ret;
> +
> + has_monotonic_timestamp(data->drm_fd);
> +
> + /* In older platforms(<= gen10), async address update bit is double buffered.
> + * So flip timestamp can be verified only from the second flip.
> + * The first async flip just enables the async address update.
> + */
> + ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> + data->bufs[0].fb_id,
> + flags, NULL);
> +
> + igt_assert(ret == 0);
> +
> + wait_flip_event(data);
> +
> + get_vbl_timestamp_us(data, &vbl_time, &seq);
> +
> + ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> + data->bufs[0].fb_id,
> + flags, NULL);
> +
> + igt_assert(ret == 0);
> +
> + wait_flip_event(data);
> +
> + get_vbl_timestamp_us(data, &vbl_time1, &seq1);
This one we could just replace with a query of the current vblank
count. Would speed up the test by one frame. I guess we still want
to keep the vblank wait before this one so that we know we have enough
time during the frame to perform the async flip.
Dunno if we might want to do more than one async flip here actually.
Potentially we might want to just do as many as possible until the
vblank count changes. And then assert that we did at least a few.
The timestamps should then be identical for all the flips except the
last one.
Nothing critical I guess. Could think about these as followup.
> +
> + igt_assert_f(seq1 == seq + 1,
> + "Vblank sequence is expected to be incremented by one(%d != (%d + 1)\n", seq1, seq);
> +
> + igt_info("vbl1_timestamp = %ldus\nflip_timestamp = %ldus\nvbl2_timestamp = %ldus\n",
> + vbl_time, flip_timestamp_us, vbl_time1);
> +
> + igt_assert_f(vbl_time < flip_timestamp_us && vbl_time1 > flip_timestamp_us,
> + "Async flip time stamp is expected to be in between 2 vblank time stamps\n");
> +}
> +
> +static void test_cursor(data_t *data)
> +{
> + int flags = DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT;
> + int ret;
> + struct igt_fb cursor_fb;
> + struct drm_mode_cursor cur;
> +
> + igt_create_color_fb(data->drm_fd, CURSOR_RES, CURSOR_RES, DRM_FORMAT_ARGB8888,
> + LOCAL_DRM_FORMAT_MOD_NONE, 1., 1., 1., &cursor_fb);
> +
> + cur.flags = DRM_MODE_CURSOR_BO;
> + cur.crtc_id = data->crtc_id;
> + cur.width = CURSOR_RES;
> + cur.height = CURSOR_RES;
> + cur.handle = cursor_fb.gem_handle;
> +
> + do_ioctl(data->drm_fd, DRM_IOCTL_MODE_CURSOR, &cur);
> +
> + ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> + data->bufs[0].fb_id,
> + flags, NULL);
> +
> + igt_assert(ret == 0);
> +
> + cur.flags = DRM_MODE_CURSOR_MOVE;
> + cur.x = CURSOR_POS;
> + cur.y = CURSOR_POS;
> +
> + do_ioctl(data->drm_fd, DRM_IOCTL_MODE_CURSOR, &cur);
Did we manage to get the anticipated oops?
> +}
> +
> +static void test_invalid(data_t *data, drmModeConnectorPtr connector)
> +{
> + int flags = DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT;
> + int ret;
> + uint32_t width, height;
> + struct igt_fb fb1, fb2;
> +
> + width = connector->modes[0].hdisplay;
> + height = connector->modes[0].vdisplay;
> +
> + igt_create_fb(data->drm_fd, width, height, DRM_FORMAT_ARGB8888,
XRGB8888
> + LOCAL_I915_FORMAT_MOD_X_TILED, &fb1);
> +
> + igt_create_fb(data->drm_fd, width, height, DRM_FORMAT_ARGB8888,
And since we just want to check the X->Y transition this too should be
XRGB8888.
I guess we might want a few more of these to test the different
things; stride and pixelformat at least. Could add those as a
followup.
> + LOCAL_I915_FORMAT_MOD_Y_TILED, &fb2);
> +
> + ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> + fb1.fb_id, flags, NULL);
> +
> + igt_assert(ret == 0);
Not quite sure why we have this first page flip here?
> +
> + wait_flip_event(data);
> +
> + /* Flip with a different fb modifier which is expected to be rejected */
> + ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> + fb2.fb_id, flags, NULL);
> +
> + igt_assert(ret == -EINVAL);
We're leaking the fbs here.
> +}
> +
> +static bool has_async(int fd)
has_async_flip() or has_async_page_flip() would be more descriptive.
> +{
> + struct drm_get_cap cap = { .capability = DRM_CAP_ASYNC_PAGE_FLIP };
> +
> + igt_assert(drmIoctl(fd, DRM_IOCTL_GET_CAP, &cap) == 0);
> + return cap.value;
> +}
> +
> +igt_main
> +{
> + data_t data;
I think better make that static. IIRC there are issues with longjmp
magic otherwise.
> + drmModeResPtr res;
> + drmModeConnectorPtr connector;
> + int i, ret;
> + bool async_capable;
Pointless variable.
> +
> + igt_fixture {
> + data.drm_fd = drm_open_driver_master(DRIVER_ANY);
> + kmstest_set_vt_graphics_mode();
> + igt_display_require(&data.display, data.drm_fd);
> + igt_display_require_output(&data.display);
> +
> + async_capable = has_async(data.drm_fd);
> + igt_require_f(async_capable, "Async Flip is not supported\n");
> + }
> +
> + igt_describe("Verify the async flip functionality and the fps during async flips");
> + igt_subtest_group {
> + igt_fixture {
> + res = drmModeGetResources(data.drm_fd);
> + igt_assert(res);
> +
> + kmstest_unset_all_crtcs(data.drm_fd, res);
> +
> + connector = find_connector_for_modeset(&data);
> + data.crtc_id = kmstest_find_crtc_for_connector(data.drm_fd,
> + res, connector, 0);
> +
> + refresh_rate = connector->modes[0].vrefresh;
> +
> + for (i = 0; i < BUFS; i++)
> + make_fb(&data, &data.bufs[i], connector, i);
> +
> + ret = drmModeSetCrtc(data.drm_fd, data.crtc_id, data.bufs[0].fb_id, 0, 0,
> + &connector->connector_id, 1, &connector->modes[0]);
> + igt_assert(ret == 0);
All of that stuff could be in a function AFAICS.
Also leaking 'res' AFAICS.
> + }
> +
> + igt_describe("Wait for page flip events in between successive asynchronous flips");
> + igt_subtest("async-flip-with-page-flip-events")
> + test_async_flip(&data, false);
> +
> + igt_describe("Alternate between sync and async flips");
> + igt_subtest("alternate-sync-async-flip")
> + test_async_flip(&data, true);
> +
> + igt_describe("Verify that the async flip timestamp does not coincide with either previous or next vblank");
> + igt_subtest("test-time-stamp")
> + test_timestamp(&data);
> +
> + igt_describe("Verify that the DRM_IOCTL_MODE_CURSOR passes after async flip");
> + igt_subtest("test-cursor")
> + test_cursor(&data);
> +
> + igt_describe("Negative case to verify if changes in fb are rejected from kernel as expected");
> + igt_subtest("invalid-async-flip")
> + test_invalid(&data, connector);
> +
> + igt_fixture {
> + for (i = 0; i < BUFS; i++)
> + igt_remove_fb(data.drm_fd, &data.bufs[i]);
> + }
> + }
> +
> + igt_fixture
> + igt_display_fini(&data.display);
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index 5eb2d2fc..515f7528 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -16,6 +16,7 @@ test_progs = [
> 'feature_discovery',
> 'kms_3d',
> 'kms_addfb_basic',
> + 'kms_async_flips',
> 'kms_atomic',
> 'kms_atomic_interruptible',
> 'kms_atomic_transition',
> --
> 2.22.0
--
Ville Syrjälä
Intel
More information about the igt-dev
mailing list