* Re: [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips
2020-09-18 11:32 ` [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips Ville Syrjälä
@ 2020-09-18 11:45 ` Ville Syrjälä
2020-09-23 2:56 ` Karthik B S
2020-09-18 21:18 ` Zanoni, Paulo R
2020-09-23 2:53 ` Karthik B S
2 siblings, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2020-09-18 11:45 UTC (permalink / raw)
To: Karthik B S; +Cc: igt-dev, daniel.vetter, michel, petri.latvala
On Fri, Sep 18, 2020 at 02:32:48PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 18, 2020 at 12:13:18PM +0530, Karthik B S wrote:
> > +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;
> > +}
I guess having a generic has_drm_cap() helper might be
nice throughout igt. Not sure if we have one already.
--
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips
2020-09-18 11:45 ` Ville Syrjälä
@ 2020-09-23 2:56 ` Karthik B S
0 siblings, 0 replies; 10+ messages in thread
From: Karthik B S @ 2020-09-23 2:56 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: igt-dev, daniel.vetter, michel, petri.latvala
On 9/18/2020 5:15 PM, Ville Syrjälä wrote:
> On Fri, Sep 18, 2020 at 02:32:48PM +0300, Ville Syrjälä wrote:
>> On Fri, Sep 18, 2020 at 12:13:18PM +0530, Karthik B S wrote:
>>> +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;
>>> +}
>
> I guess having a generic has_drm_cap() helper might be
> nice throughout igt. Not sure if we have one already.
I don't see a helper for this currently. Will add this.
Thanks,
Karthik.B.S
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips
2020-09-18 11:32 ` [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips Ville Syrjälä
2020-09-18 11:45 ` Ville Syrjälä
@ 2020-09-18 21:18 ` Zanoni, Paulo R
2020-09-23 2:53 ` Karthik B S
2 siblings, 0 replies; 10+ messages in thread
From: Zanoni, Paulo R @ 2020-09-18 21:18 UTC (permalink / raw)
To: ville.syrjala@linux.intel.com, B S, Karthik
Cc: Latvala, Petri, michel@daenzer.net, igt-dev@lists.freedesktop.org,
Vetter, Daniel
On Fri, 2020-09-18 at 14:32 +0300, 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@intel.com>
> > Signed-off-by: Karthik B S <karthik.b.s@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@intel.com>
> > + * Karthik B S <karthik.b.s@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?
A long time ago in a galaxay far away, when I was developing this I was
comparing FPS numbers, so this was a lazy way to make sure the clocks
ramped up and stabilized (instead of just forcing frequencies or
something else). This is not very relevant in the context of IGT as a
test suite to prevent regressions.
>
> > +
> > + 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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips
2020-09-18 11:32 ` [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips Ville Syrjälä
2020-09-18 11:45 ` Ville Syrjälä
2020-09-18 21:18 ` Zanoni, Paulo R
@ 2020-09-23 2:53 ` Karthik B S
2020-09-23 9:55 ` Ville Syrjälä
2 siblings, 1 reply; 10+ messages in thread
From: Karthik B S @ 2020-09-23 2:53 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: michel, igt-dev, daniel.vetter, petri.latvala
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@intel.com>
>> Signed-off-by: Karthik B S <karthik.b.s@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@intel.com>
>> + * Karthik B S <karthik.b.s@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?
>> +
>> +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.
>
Sure, will update this.
>> + 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.
>
Will change the name to require_monotonic_timestamp().
>> +{
>> + 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.
>
Sure, I'll add a check for Gen 9 and Gen 10 for this.
> 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.
>
I will update this to remove the toggle logic and have explicit async
and sync flip calls.
>> + 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?
>
Will remove this as it is not relevant any more.
>> +
>> + 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.
>
Will rename it to wait_for_vblank.
>> +
>> + 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.
>
Sure. Will add a TODO here and add these changes as a 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?
>
Yes, after enabling the cursor before doing the async flip,
we were seeing the oops as you had pointed out.
Added a change accordingly in the kernel for that.
>> +}
>> +
>> +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
>
Noted.
>> + 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.
>
Noted.
> 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.
>
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?
>> +
>> + 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.
>
Sure, will fix this.
>> +}
>> +
>> +static bool has_async(int fd)
>
> has_async_flip() or has_async_page_flip() would be more descriptive.
>
Sure. Will update this.
>> +{
>> + 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.
>
Sure, will update this.
>> + drmModeResPtr res;
>> + drmModeConnectorPtr connector;
>> + int i, ret;
>> + bool async_capable;
>
> Pointless variable.
>
Will remove this 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.
>
Sure, I'll move it to a test_init() function.
> Also leaking 'res' AFAICS.
>
Will fix this.
Thanks,
Karthik.B.S
>> + }
>> +
>> + 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
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips
2020-09-23 2:53 ` Karthik B S
@ 2020-09-23 9:55 ` Ville Syrjälä
2020-09-23 14:12 ` Karthik B S
0 siblings, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2020-09-23 9:55 UTC (permalink / raw)
To: Karthik B S; +Cc: michel, igt-dev, daniel.vetter, petri.latvala
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@intel.com>
> >> Signed-off-by: Karthik B S <karthik.b.s@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@intel.com>
> >> + * Karthik B S <karthik.b.s@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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips
2020-09-23 9:55 ` Ville Syrjälä
@ 2020-09-23 14:12 ` Karthik B S
0 siblings, 0 replies; 10+ messages in thread
From: Karthik B S @ 2020-09-23 14:12 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: michel, igt-dev, daniel.vetter, petri.latvala
On 9/23/2020 3:25 PM, Ville Syrjälä wrote:
> 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@intel.com>
>>>> Signed-off-by: Karthik B S <karthik.b.s@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@intel.com>
>>>> + * Karthik B S <karthik.b.s@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>
Thanks for the inputs.
I will update this.
>> 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.
>
Got it. I misunderstood this part earlier.
Will remove the first flip in the next revision.
Thanks,
Karthik.B.S
>>>> +
>>>> + 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-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread