[igt-dev] [PATCH i-g-t v2] tests/kms_async: Add test to validate asynchronous flips
Paulo Zanoni
paulo.r.zanoni at intel.com
Tue Apr 14 22:35:37 UTC 2020
Em ter, 2020-04-14 às 15:41 +0530, Karthik B S escreveu:
> 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.
>
> 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.c | 243 +++++++++++++++++++++++++++++++++++++++++
> tests/meson.build | 1 +
> 3 files changed, 245 insertions(+)
> create mode 100644 tests/kms_async.c
>
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 4e44c98c..eedf4fcb 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -29,6 +29,7 @@ TESTS_progs = \
> fbdev \
> kms_3d \
> kms_addfb_basic \
> + kms_async \
> kms_atomic \
> kms_atomic_interruptible \
> kms_atomic_transition \
> diff --git a/tests/kms_async.c b/tests/kms_async.c
> new file mode 100644
> index 00000000..6031ff4e
> --- /dev/null
> +++ b/tests/kms_async.c
Async what? :)
kms_async_flips.c is probably better IMHO since it's clearer on what
the test tests.
Also, do we have to add this to some IGT test list file if we want this
in CI? Can the IGT/CI people comment here please?
> @@ -0,0 +1,243 @@
> +/*
> + * 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 <poll.h>
> +
> +#define BUFS 4
Perhaps instead of the TODO comment below we could have something like
this in this line:
/*
* 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 10 /* TODO: May need tuning */
> +
> +IGT_TEST_DESCRIPTION("Test asynchrous page flips.");
> +
> +typedef struct {
> + int drm_fd;
> + uint32_t crtc_id;
> + struct igt_fb bufs[BUFS];
> + igt_display_t display;
> +} data_t;
> +
> +uint32_t refresh_rate;
> +
> +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 unsigned int last_ms;
> + static bool flag;
This variable serves no purpose.
> + unsigned int cur_ms, flip_interval;
> +
> + igt_assert(_data == NULL);
> +
> + cur_ms = tv_sec * 1000 + tv_usec / 1000;
> + flip_interval = cur_ms - last_ms;
> +
> + igt_debug("Flip interval: %dms, sequence = %u\n", flip_interval, sequence);
My previous comment regarding this debug statement here is still valid.
This is going to spam the logs needlessly, and possibly significantly
affect timings. It should probably be removed (or left commented out).
> +
> + if (!flag)
> + flag = 1;
> + else
> + igt_assert_f(flip_interval <= 1000 / (refresh_rate * THRESHOLD),
> + "Flip interval not significantly smaller than vblank interval\n");
> +
> + last_ms = cur_ms;
> +}
> +
> +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,
> + 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 test_async_flip(data_t *data, bool wait_for_flips)
> +{
> + int ret, frame, warm_end_frame;
> + long long int fps;
> + struct timeval start, end, diff;
> + bool warming_up = true;
> +
> + gettimeofday(&start, NULL);
> + frame = 1;
> + do {
> + int flags = DRM_MODE_PAGE_FLIP_ASYNC;
> +
> + if (wait_for_flips)
> + flags |= DRM_MODE_PAGE_FLIP_EVENT;
> +
> + ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> + data->bufs[frame % 4].fb_id,
> + flags, NULL);
> +
> + igt_assert(ret == 0 || ret == -EBUSY);
> +
> + if (wait_for_flips)
> + 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;
> + }
> +
> + frame++;
> + } while (diff.tv_sec < RUN_TIME);
> +
> + 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 bool has_async(int fd)
> +{
> + struct drm_get_cap cap = { .capability = DRM_CAP_ASYNC_PAGE_FLIP };
> +
> + ioctl(fd, DRM_IOCTL_GET_CAP, &cap);
Please use drmIoctl(). Also check the return code of the call.
Except for the little details I pointed above, this patch seems to be
in good shape now, but I'm hesitant on giving a Reviewed-by tag since
the Kernel patches that pass these tests are still under development. I
would prefer to wait them to come out before we merge this.
> + return cap.value;
> +}
> +
> +igt_main
> +{
> + data_t data;
> + drmModeResPtr res;
> + drmModeConnectorPtr connector;
> + int i, ret;
> + bool async_capable;
> +
> + igt_fixture {
> + data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
> + 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("Make sure kernel is able to accept asynchronous flips at a fast pace.");
> + 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);
> + }
> +
> + igt_describe("Wait for page flip events in between successive asynchronous flips");
> + igt_subtest("async-flip-with-page-flip-events")
> + test_async_flip(&data, true);
> + igt_describe("DO NOT wait for page flip events in between successive asynchronous flips");
> + igt_subtest("async-flip-without-page-flip-events")
> + test_async_flip(&data, false);
> +
> + 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 e882f4dc..0830b1c9 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -13,6 +13,7 @@ test_progs = [
> 'fbdev',
> 'kms_3d',
> 'kms_addfb_basic',
> + 'kms_async',
> 'kms_atomic',
> 'kms_atomic_interruptible',
> 'kms_atomic_transition',
More information about the igt-dev
mailing list