[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
Wed Sep 23 09:55:06 UTC 2020
On Wed, Sep 23, 2020 at 08:23:44AM +0530, Karthik B S wrote:
>
>
> On 9/18/2020 5:02 PM, Ville Syrjälä wrote:
> > 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).
>
> Thanks for the review.
> Sure, I'll change this.
> >
> >> +#define SYNC_FLIP 0
> >> +#define ASYNC_FLIP 1
> >
> > enum
> >
>
> Will update this.
>
> >> +#define CURSOR_RES 64
> >
> > Should query that from the kernel so that test is driver agnostic.
> >
>
> Sure. Will change this.
> >> +#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?
> >
>
> Sure. I will change this to MIN_FLIPS_PER_FRAME.
> >> +
> >> +IGT_TEST_DESCRIPTION("Test asynchrous page flips.");
> >
> > Check the spelling
> >
>
> Will fix this.
> >> +
> >> +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?
> >
>
> Will move the refresh_rate variable inside data_t.
> The other two are getting updated in flip_handler. Hence kept them
> global. Would you suggest making data_t global. Or any other way I can
> handle this better?
It should be possible to pass arbitrary data to the event handler.
<snip>
> Sure. Will add a TODO here and add these changes as a follow up.
> >> + 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?
> >
>
> First to have an async flip with xtiled buffer and then follow it up
> with an async flip with a y-tiled buffer, so the second one is expected
> to fail. Could this be done in one flip? Am I missing something here?
What does the fist async flip achieve for us? Nothing AFAICS. We must
already be scanning out a similar fb or else the first async flip would
also fail.
If it's possible that we are not already scanning out a similar fb to
the first one then we should do a full modeset/atomic commit to make
it so. Otherwise the second async flip is going to be testing random
things.
> >> +
> >> + 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);
> >> +
--
Ville Syrjälä
Intel
More information about the igt-dev
mailing list