All of lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips
@ 2020-09-18  6:43 Karthik B S
  2020-09-18  7:50 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_async_flips: Add test to validate asynchronous flips (rev6) Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Karthik B S @ 2020-09-18  6:43 UTC (permalink / raw)
  To: igt-dev; +Cc: michel, daniel.vetter, petri.latvala

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
+#define SYNC_FLIP 0
+#define ASYNC_FLIP 1
+#define CURSOR_RES 64
+#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
+
+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;
+unsigned long flip_timestamp_us;
+double flip_interval;
+
+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,
+		      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)
+{
+	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.
+				 */
+				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;
+		}
+
+		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;
+
+	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);
+
+	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);
+}
+
+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,
+		      LOCAL_I915_FORMAT_MOD_X_TILED, &fb1);
+
+	igt_create_fb(data->drm_fd, width, height, DRM_FORMAT_ARGB8888,
+		      LOCAL_I915_FORMAT_MOD_Y_TILED, &fb2);
+
+	ret = drmModePageFlip(data->drm_fd, data->crtc_id,
+			      fb1.fb_id, flags, NULL);
+
+	igt_assert(ret == 0);
+
+	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);
+}
+
+static bool has_async(int fd)
+{
+	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;
+	drmModeResPtr res;
+	drmModeConnectorPtr connector;
+	int i, ret;
+	bool async_capable;
+
+	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);
+		}
+
+		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 related	[flat|nested] 10+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_async_flips: Add test to validate asynchronous flips (rev6)
  2020-09-18  6:43 [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips Karthik B S
@ 2020-09-18  7:50 ` Patchwork
  2020-09-18  8:47 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
  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ä
  2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2020-09-18  7:50 UTC (permalink / raw)
  To: Karthik B S; +Cc: igt-dev


[-- Attachment #1.1: Type: text/plain, Size: 4752 bytes --]

== Series Details ==

Series: tests/kms_async_flips: Add test to validate asynchronous flips (rev6)
URL   : https://patchwork.freedesktop.org/series/79701/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_9023 -> IGTPW_4997
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/index.html

Known issues
------------

  Here are the changes found in IGTPW_4997 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1:
    - fi-icl-u2:          [PASS][1] -> [DMESG-WARN][2] ([i915#1982]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-tgl-y:           [PASS][3] -> [DMESG-WARN][4] ([i915#402]) +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-tgl-y/igt@prime_vgem@basic-fence-flip.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/fi-tgl-y/igt@prime_vgem@basic-fence-flip.html

  
#### Possible fixes ####

  * igt@gem_flink_basic@flink-lifetime:
    - fi-tgl-y:           [DMESG-WARN][5] ([i915#402]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-tgl-y/igt@gem_flink_basic@flink-lifetime.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/fi-tgl-y/igt@gem_flink_basic@flink-lifetime.html

  * igt@i915_pm_rpm@module-reload:
    - fi-bsw-n3050:       [DMESG-WARN][7] ([i915#1982]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-bsw-n3050/igt@i915_pm_rpm@module-reload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/fi-bsw-n3050/igt@i915_pm_rpm@module-reload.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-bsw-kefka:       [DMESG-WARN][9] ([i915#1982]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1:
    - fi-icl-u2:          [DMESG-WARN][11] ([i915#1982]) -> [PASS][12] +1 similar issue
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-c:
    - fi-tgl-y:           [DMESG-WARN][13] ([i915#1982]) -> [PASS][14] +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-tgl-y/igt@kms_pipe_crc_basic@read-crc-pipe-c.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/fi-tgl-y/igt@kms_pipe_crc_basic@read-crc-pipe-c.html

  * igt@vgem_basic@unload:
    - fi-skl-guc:         [DMESG-WARN][15] ([i915#2203]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-skl-guc/igt@vgem_basic@unload.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/fi-skl-guc/igt@vgem_basic@unload.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2203]: https://gitlab.freedesktop.org/drm/intel/issues/2203
  [i915#289]: https://gitlab.freedesktop.org/drm/intel/issues/289
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402


Participating hosts (46 -> 39)
------------------------------

  Missing    (7): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-kbl-x1275 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_5787 -> IGTPW_4997

  CI-20190529: 20190529
  CI_DRM_9023: 5887fa2d8b9b7f6a278f9a1bc8642cb9d5d0279a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_4997: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/index.html
  IGT_5787: 0ec962017c8131de14e0cb038f7f76b1f17ed637 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+igt@kms_async_flips@alternate-sync-async-flip
+igt@kms_async_flips@async-flip-with-page-flip-events
+igt@kms_async_flips@invalid-async-flip
+igt@kms_async_flips@test-cursor
+igt@kms_async_flips@test-time-stamp

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/index.html

[-- Attachment #1.2: Type: text/html, Size: 5858 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
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

* [igt-dev] ✓ Fi.CI.IGT: success for tests/kms_async_flips: Add test to validate asynchronous flips (rev6)
  2020-09-18  6:43 [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips Karthik B S
  2020-09-18  7:50 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_async_flips: Add test to validate asynchronous flips (rev6) Patchwork
@ 2020-09-18  8:47 ` Patchwork
  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ä
  2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2020-09-18  8:47 UTC (permalink / raw)
  To: Karthik B S; +Cc: igt-dev


[-- Attachment #1.1: Type: text/plain, Size: 15333 bytes --]

== Series Details ==

Series: tests/kms_async_flips: Add test to validate asynchronous flips (rev6)
URL   : https://patchwork.freedesktop.org/series/79701/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_9023_full -> IGTPW_4997_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_4997_full:

### IGT changes ###

#### Possible regressions ####

  * {igt@kms_async_flips@invalid-async-flip} (NEW):
    - shard-tglb:         NOTRUN -> [SKIP][1] +4 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-tglb6/igt@kms_async_flips@invalid-async-flip.html

  * {igt@kms_async_flips@test-cursor} (NEW):
    - shard-iclb:         NOTRUN -> [SKIP][2] +4 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-iclb2/igt@kms_async_flips@test-cursor.html

  
New tests
---------

  New tests have been introduced between CI_DRM_9023_full and IGTPW_4997_full:

### New IGT tests (5) ###

  * igt@kms_async_flips@alternate-sync-async-flip:
    - Statuses : 7 skip(s)
    - Exec time: [0.0] s

  * igt@kms_async_flips@async-flip-with-page-flip-events:
    - Statuses : 7 skip(s)
    - Exec time: [0.0] s

  * igt@kms_async_flips@invalid-async-flip:
    - Statuses : 7 skip(s)
    - Exec time: [0.0] s

  * igt@kms_async_flips@test-cursor:
    - Statuses : 7 skip(s)
    - Exec time: [0.0] s

  * igt@kms_async_flips@test-time-stamp:
    - Statuses : 7 skip(s)
    - Exec time: [0.0] s

  

Known issues
------------

  Here are the changes found in IGTPW_4997_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_endless@dispatch@rcs0:
    - shard-tglb:         [PASS][3] -> [INCOMPLETE][4] ([i915#1958] / [i915#2270])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-tglb1/igt@gem_exec_endless@dispatch@rcs0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-tglb8/igt@gem_exec_endless@dispatch@rcs0.html

  * igt@gem_exec_suspend@basic-s3:
    - shard-kbl:          [PASS][5] -> [DMESG-WARN][6] ([i915#180]) +5 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-kbl4/igt@gem_exec_suspend@basic-s3.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-kbl7/igt@gem_exec_suspend@basic-s3.html

  * igt@i915_pm_rpm@system-suspend-devices:
    - shard-hsw:          [PASS][7] -> [INCOMPLETE][8] ([i915#151])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-hsw2/igt@i915_pm_rpm@system-suspend-devices.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-hsw1/igt@i915_pm_rpm@system-suspend-devices.html

  * igt@i915_selftest@live@gt_heartbeat:
    - shard-glk:          [PASS][9] -> [DMESG-FAIL][10] ([i915#541])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-glk3/igt@i915_selftest@live@gt_heartbeat.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-glk3/igt@i915_selftest@live@gt_heartbeat.html

  * igt@kms_cursor_crc@pipe-b-cursor-256x85-onscreen:
    - shard-apl:          [PASS][11] -> [FAIL][12] ([i915#1635] / [i915#54])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-apl4/igt@kms_cursor_crc@pipe-b-cursor-256x85-onscreen.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-apl8/igt@kms_cursor_crc@pipe-b-cursor-256x85-onscreen.html
    - shard-kbl:          [PASS][13] -> [FAIL][14] ([i915#54])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-kbl3/igt@kms_cursor_crc@pipe-b-cursor-256x85-onscreen.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-kbl7/igt@kms_cursor_crc@pipe-b-cursor-256x85-onscreen.html

  * igt@kms_flip@2x-flip-vs-wf_vblank@ab-vga1-hdmi-a1:
    - shard-hsw:          [PASS][15] -> [DMESG-WARN][16] ([i915#1982])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-hsw2/igt@kms_flip@2x-flip-vs-wf_vblank@ab-vga1-hdmi-a1.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-hsw6/igt@kms_flip@2x-flip-vs-wf_vblank@ab-vga1-hdmi-a1.html

  * igt@kms_frontbuffer_tracking@fbcpsr-farfromfence:
    - shard-tglb:         [PASS][17] -> [DMESG-WARN][18] ([i915#1982])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-tglb2/igt@kms_frontbuffer_tracking@fbcpsr-farfromfence.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-tglb8/igt@kms_frontbuffer_tracking@fbcpsr-farfromfence.html

  * igt@kms_psr@psr2_cursor_render:
    - shard-iclb:         [PASS][19] -> [SKIP][20] ([fdo#109441]) +3 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb2/igt@kms_psr@psr2_cursor_render.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-iclb3/igt@kms_psr@psr2_cursor_render.html

  * igt@kms_vblank@pipe-b-wait-idle:
    - shard-kbl:          [PASS][21] -> [DMESG-WARN][22] ([i915#1982]) +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-kbl2/igt@kms_vblank@pipe-b-wait-idle.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-kbl3/igt@kms_vblank@pipe-b-wait-idle.html

  * igt@prime_vgem@sync@vcs1:
    - shard-tglb:         [PASS][23] -> [INCOMPLETE][24] ([i915#409])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-tglb1/igt@prime_vgem@sync@vcs1.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-tglb8/igt@prime_vgem@sync@vcs1.html

  
#### Possible fixes ####

  * {igt@core_hotunplug@hotunbind-rebind}:
    - shard-iclb:         [DMESG-WARN][25] ([i915#1982]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb1/igt@core_hotunplug@hotunbind-rebind.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-iclb8/igt@core_hotunplug@hotunbind-rebind.html

  * igt@gen9_exec_parse@allowed-all:
    - shard-kbl:          [DMESG-WARN][27] ([i915#1436] / [i915#716]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-kbl1/igt@gen9_exec_parse@allowed-all.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-kbl1/igt@gen9_exec_parse@allowed-all.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][29] ([i915#180]) -> [PASS][30] +5 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-kbl6/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_cursor_legacy@2x-cursor-vs-flip-atomic:
    - shard-hsw:          [INCOMPLETE][31] ([CI#80]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-hsw2/igt@kms_cursor_legacy@2x-cursor-vs-flip-atomic.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-hsw7/igt@kms_cursor_legacy@2x-cursor-vs-flip-atomic.html

  * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
    - shard-hsw:          [FAIL][33] ([i915#96]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-hsw8/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-hsw7/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy.html

  * igt@kms_flip@2x-dpms-vs-vblank-race@ab-vga1-hdmi-a1:
    - shard-hsw:          [DMESG-WARN][35] ([i915#1982]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-hsw6/igt@kms_flip@2x-dpms-vs-vblank-race@ab-vga1-hdmi-a1.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-hsw5/igt@kms_flip@2x-dpms-vs-vblank-race@ab-vga1-hdmi-a1.html

  * igt@kms_flip@2x-plain-flip-fb-recreate-interruptible@ab-hdmi-a1-hdmi-a2:
    - shard-glk:          [FAIL][37] ([i915#2122]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-glk5/igt@kms_flip@2x-plain-flip-fb-recreate-interruptible@ab-hdmi-a1-hdmi-a2.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-glk6/igt@kms_flip@2x-plain-flip-fb-recreate-interruptible@ab-hdmi-a1-hdmi-a2.html

  * igt@kms_flip@flip-vs-suspend@a-edp1:
    - shard-iclb:         [INCOMPLETE][39] ([i915#2357]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb2/igt@kms_flip@flip-vs-suspend@a-edp1.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-iclb7/igt@kms_flip@flip-vs-suspend@a-edp1.html

  * igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
    - shard-kbl:          [DMESG-WARN][41] ([i915#1982]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-kbl7/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-kbl6/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-render:
    - shard-tglb:         [DMESG-WARN][43] ([i915#1982]) -> [PASS][44] +4 similar issues
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-tglb1/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-render.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-tglb1/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-render.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [SKIP][45] ([fdo#109642] / [fdo#111068]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb3/igt@kms_psr2_su@page_flip.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-iclb2/igt@kms_psr2_su@page_flip.html

  * igt@kms_psr@psr2_cursor_plane_move:
    - shard-iclb:         [SKIP][47] ([fdo#109441]) -> [PASS][48] +1 similar issue
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb3/igt@kms_psr@psr2_cursor_plane_move.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-iclb2/igt@kms_psr@psr2_cursor_plane_move.html

  * igt@kms_vblank@pipe-c-query-forked-hang:
    - shard-apl:          [DMESG-WARN][49] ([i915#1635] / [i915#1982]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-apl7/igt@kms_vblank@pipe-c-query-forked-hang.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-apl8/igt@kms_vblank@pipe-c-query-forked-hang.html

  * igt@perf@polling-parameterized:
    - shard-kbl:          [FAIL][51] ([i915#1542]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-kbl2/igt@perf@polling-parameterized.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-kbl6/igt@perf@polling-parameterized.html

  
#### Warnings ####

  * igt@i915_pm_dc@dc3co-vpb-simulation:
    - shard-tglb:         [DMESG-WARN][53] ([i915#2411]) -> [SKIP][54] ([i915#1904])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-tglb1/igt@i915_pm_dc@dc3co-vpb-simulation.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-tglb5/igt@i915_pm_dc@dc3co-vpb-simulation.html

  * igt@i915_pm_dc@dc6-psr:
    - shard-tglb:         [FAIL][55] ([i915#454]) -> [FAIL][56] ([i915#1899])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-tglb2/igt@i915_pm_dc@dc6-psr.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-tglb7/igt@i915_pm_dc@dc6-psr.html

  * igt@kms_content_protection@legacy:
    - shard-apl:          [TIMEOUT][57] ([i915#1319] / [i915#1635] / [i915#1958]) -> [TIMEOUT][58] ([i915#1319] / [i915#1635])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-apl4/igt@kms_content_protection@legacy.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-apl7/igt@kms_content_protection@legacy.html

  * igt@kms_content_protection@lic:
    - shard-apl:          [TIMEOUT][59] ([i915#1319] / [i915#1635] / [i915#1958]) -> [FAIL][60] ([fdo#110321] / [i915#1635])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-apl6/igt@kms_content_protection@lic.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-apl4/igt@kms_content_protection@lic.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-d:
    - shard-tglb:         [DMESG-WARN][61] ([i915#2411]) -> [INCOMPLETE][62] ([i915#1436] / [i915#1982])
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-tglb5/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-d.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/shard-tglb1/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-d.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [CI#80]: https://gitlab.freedesktop.org/gfx-ci/i915-infra/issues/80
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110321]: https://bugs.freedesktop.org/show_bug.cgi?id=110321
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [i915#1319]: https://gitlab.freedesktop.org/drm/intel/issues/1319
  [i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436
  [i915#151]: https://gitlab.freedesktop.org/drm/intel/issues/151
  [i915#1542]: https://gitlab.freedesktop.org/drm/intel/issues/1542
  [i915#1635]: https://gitlab.freedesktop.org/drm/intel/issues/1635
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#1899]: https://gitlab.freedesktop.org/drm/intel/issues/1899
  [i915#1904]: https://gitlab.freedesktop.org/drm/intel/issues/1904
  [i915#1958]: https://gitlab.freedesktop.org/drm/intel/issues/1958
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#2270]: https://gitlab.freedesktop.org/drm/intel/issues/2270
  [i915#2357]: https://gitlab.freedesktop.org/drm/intel/issues/2357
  [i915#2411]: https://gitlab.freedesktop.org/drm/intel/issues/2411
  [i915#409]: https://gitlab.freedesktop.org/drm/intel/issues/409
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
  [i915#541]: https://gitlab.freedesktop.org/drm/intel/issues/541
  [i915#716]: https://gitlab.freedesktop.org/drm/intel/issues/716
  [i915#96]: https://gitlab.freedesktop.org/drm/intel/issues/96


Participating hosts (11 -> 8)
------------------------------

  Missing    (3): pig-skl-6260u pig-glk-j5005 pig-icl-1065g7 


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_5787 -> IGTPW_4997
  * Piglit: piglit_4509 -> None

  CI-20190529: 20190529
  CI_DRM_9023: 5887fa2d8b9b7f6a278f9a1bc8642cb9d5d0279a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_4997: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/index.html
  IGT_5787: 0ec962017c8131de14e0cb038f7f76b1f17ed637 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4997/index.html

[-- Attachment #1.2: Type: text/html, Size: 18361 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
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  6:43 [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips Karthik B S
  2020-09-18  7:50 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_async_flips: Add test to validate asynchronous flips (rev6) Patchwork
  2020-09-18  8:47 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
@ 2020-09-18 11:32 ` Ville Syrjälä
  2020-09-18 11:45   ` Ville Syrjälä
                     ` (2 more replies)
  2 siblings, 3 replies; 10+ messages in thread
From: Ville Syrjälä @ 2020-09-18 11:32 UTC (permalink / raw)
  To: Karthik B S; +Cc: michel, igt-dev, daniel.vetter, petri.latvala

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?

> +
> +		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
_______________________________________________
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-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: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, B S, Karthik
  Cc: Latvala, Petri, michel, igt-dev, 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-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-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

end of thread, other threads:[~2020-09-23 14:13 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-18  6:43 [igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips Karthik B S
2020-09-18  7:50 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_async_flips: Add test to validate asynchronous flips (rev6) Patchwork
2020-09-18  8:47 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
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
2020-09-23  9:55     ` Ville Syrjälä
2020-09-23 14:12       ` Karthik B S

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.