[igt-dev] [PATCH i-g-t] tests/kms_async: Add test to validate asynchronous flips
Karthik B S
karthik.b.s at intel.com
Wed Apr 8 06:36:21 UTC 2020
Thanks a lot for the review.
On 4/3/2020 10:39 PM, Paulo Zanoni wrote:
> Em sex, 2020-04-03 às 14:47 +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 recevied before giving the next flip,
>> and the second subtest doesn't wait for page flip events.
>>
>> The test passes if the IOCTL is succesful.
>>
>> Signed-off-by: Karthik B S <karthik.b.s at intel.com>
>> Suggested-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> I feel I deserve a little more credit than just a Suggested-by since
> this still has a lot of code I wrote :).
>
Sorry for that. I'll update in rev2. :)
>
>> ---
>> tests/Makefile.sources | 1 +
>> tests/kms_async.c | 206 +++++++++++++++++++++++++++++++++++++++++
>> tests/meson.build | 1 +
>> 3 files changed, 208 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..0e21f9f6
>> --- /dev/null
>> +++ b/tests/kms_async.c
>> @@ -0,0 +1,206 @@
>> +/*
>> + * 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 <email>
> * Karthik B S <email>
>
Noted.
>> + */
>> +
>> +#include "igt.h"
>> +#include "igt_aux.h"
>> +#include <poll.h>
>> +
>> +#define BUFS 4
>> +#define WARM_UP_TIME 2
>> +#define RUN_TIME 10
>
> Each subtest takes 12 seconds to run, by definition (sans timeouts).
> Maybe for IGT we want to make this a little faster? When I wrote this
> code I needed very precise FPS values, IGT only needs to assert things
> are working as expected.
>
> 1s WARM_UP_TIME and 4s RUN_TIME are probably very fine, and even less
> than that is also probably fine for IGT. Please play with the tunables.
>
> IGT needs to be as fast as it can while still achieving 100%
> reliability.
>
Will update this.
>> +
>> +typedef struct {
>> + int drm_fd;
>> + igt_display_t display;
>> +} 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 unsigned int last_ms;
>> + unsigned int cur_ms;
>> +
>> + igt_assert(_data == NULL);
>> +
>> + cur_ms = tv_sec * 1000 + tv_usec / 1000;
>> +
>> + igt_debug("Flip interval: %dms\n", cur_ms - last_ms);
>> +
>> + last_ms = cur_ms;
>> +
>> + igt_debug("Flip event received, fd:%d seq:%u tv_sec:%u, "
>> + "tv_usec:%u data:%p\n",
>> + fd_, sequence, tv_sec, tv_usec, _data);
>
> In my original implementation these messages were under if (0) because
> they significantly affected FPS.
>
> I think we should probably have some kind of igt_assert() here instead
> of printing stuff. This program is supposed to achieve thousands of
> frames per second even on slow hardware (I think I got 2500 on a slow
> GLK?). Any intervals that resemble the monitor refresh rate by a 10x
> error rate are probably failures.
>
Does this mean if vblank interval is 16ms, then intervals of
>1.6ms is a failure? Did I understand this right or Am I missing
something here.
>> +}
>> +
>> +static void wait_flip_event(data_t *data)
>> +{
>> + int ret;
>> + drmEventContext evctx;
>> + struct pollfd pfd;
>> + static int timeouts = 0;
>> +
>> + 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, 10000);
>
> 10s timeouts are not igt-friendly. I used a giant number because I
> wanted to read the logs. IGT doesn't need nor want 10s timeouts.
>
Will update this.
>
>> +
>> + switch (ret) {
>> + case 0:
>> + timeouts++;
>> + igt_debug("Poll timeout %d!\n", timeouts);
>
> I don't remember exactly, but I think a timeout was a failure? Maybe
> igt_assert() here?
>
>
>> + 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());
>
> Please follow the IGT coding style regarding spaces.
>
Will update this accordingly.
>> +}
>> +
>> +static void test_async_flip(data_t *data, bool wait_for_flips)
>> +{
>> + struct igt_fb bufs[BUFS];
>> + drmModeResPtr res;
>> + drmModeConnectorPtr connector;
>> + uint32_t crtc_id;
>> + int i, ret, frame, warm_end_frame;
>> + long long int fps;
>> + struct timeval start, end, diff;
>> + bool warming_up = true;
>> +
>
> From here...
>
>> + res = drmModeGetResources(data->drm_fd);
>> + igt_assert(res);
>> +
>> + kmstest_unset_all_crtcs(data->drm_fd, res);
>> +
>> + connector = find_connector_for_modeset(data);
>> + crtc_id = kmstest_find_crtc_for_connector(data->drm_fd,
>> + res, connector, 0);
>> +
>> + for (i = 0; i < BUFS; i++)
>> + make_fb(data, &bufs[i], connector, i);
>> +
>> + ret = drmModeSetCrtc(data->drm_fd, crtc_id, bufs[0].fb_id, 0, 0,
>> + &connector->connector_id, 1, &connector->modes[0]);
>> + igt_assert(ret == 0);
>
> ... to here we probably don't want to repeat for every subtest,
> especially if we add more in the future. Make them part of the fixture
> and adjust the subtests to deal with it.
>
Sure. I'll make changes to the subtests accordingly.
>> +
>> + 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, crtc_id,
>> + 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);
>> +
>> + /* 2s 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_info("fps: %lld.%03lld\n", fps / 1000, fps % 1000);
>
> We have to igt_assert() that FPS is a value significantly bigger than
> the mode refresh rate.
>
Didn't get this? FPS is expected to be bigger than RR right?
Could you please help me here, to understand what I'm missing?
>
>> +
>> + for (i = 0; i < BUFS; i++)
>> + igt_remove_fb(data->drm_fd, &bufs[i]);
>> +}
>> +
>> +igt_main
>> +{
>> + data_t data;
>> +
>> + 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);
>
> We have to SKIP in case async flips are not supported by the Kernel.
>
Could you please let me know how I can do this? Since I can't test with
a try_commit as we're using the page flip IOCTL in this test.
Any other way with which I can check this?
Thanks,
Karthik
> Thanks,
> Paulo
>
>
>> + }
>> +
>> + igt_subtest("async-flip-with-page-flip-events")
>> + test_async_flip(&data, true);
>> + igt_subtest("async-flip-without-page-flip-events")
>> + test_async_flip(&data, false);
>> +
>> + 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