[i-g-t] tests: Add variable refresh rate tests

Submitted by Nicholas Kazlauskas on Jan. 9, 2019, 7:49 p.m.

Details

Message ID 20190109194932.16679-1-nicholas.kazlauskas@amd.com
State New
Headers show
Series "tests: Add variable refresh rate tests" ( rev: 2 1 ) in IGT

Not browsing as part of any series.

Commit Message

Nicholas Kazlauskas Jan. 9, 2019, 7:49 p.m.
There are 3 tests for basic variable refresh rate functionality.

The tests measure flipping at the midpoint between the current mode's
refresh rate and the minimum supported variable refresh rate.

It tests that VRR is enabled and that the difference between flip
timestamps converges to the requested rate. It also tests this under
both S3 and DPMS.

Potential ideas for future tests:
- Test behavior inside VRR range with a stepping test
- Test behavior outside of VRR range
- Multi-monitor (limited by no async pageflips in DRM atomic API)

Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 lib/igt_kms.c          |   3 +
 lib/igt_kms.h          |   2 +
 tests/Makefile.sources |   1 +
 tests/kms_vrr.c        | 417 +++++++++++++++++++++++++++++++++++++++++
 tests/meson.build      |   1 +
 5 files changed, 424 insertions(+)
 create mode 100644 tests/kms_vrr.c

Patch hide | download patch | download mbox

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 684a599c..2edf19ec 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -189,6 +189,7 @@  const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
 	[IGT_CRTC_MODE_ID] = "MODE_ID",
 	[IGT_CRTC_ACTIVE] = "ACTIVE",
 	[IGT_CRTC_OUT_FENCE_PTR] = "OUT_FENCE_PTR",
+	[IGT_CRTC_VRR_ENABLED] = "VRR_ENABLED",
 };
 
 const char * const igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
@@ -197,6 +198,7 @@  const char * const igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
 	[IGT_CONNECTOR_DPMS] = "DPMS",
 	[IGT_CONNECTOR_BROADCAST_RGB] = "Broadcast RGB",
 	[IGT_CONNECTOR_CONTENT_PROTECTION] = "Content Protection",
+	[IGT_CONNECTOR_VRR_CAPABLE] = "vrr_capable",
 };
 
 /*
@@ -1786,6 +1788,7 @@  static void igt_pipe_reset(igt_pipe_t *pipe)
 {
 	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, 0);
 	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_ACTIVE, 0);
+	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_VRR_ENABLED, 0);
 	igt_pipe_obj_clear_prop_changed(pipe, IGT_CRTC_OUT_FENCE_PTR);
 
 	pipe->out_fence_fd = -1;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 4a7c3c97..679d4e84 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -104,6 +104,7 @@  enum igt_atomic_crtc_properties {
        IGT_CRTC_MODE_ID,
        IGT_CRTC_ACTIVE,
        IGT_CRTC_OUT_FENCE_PTR,
+       IGT_CRTC_VRR_ENABLED,
        IGT_NUM_CRTC_PROPS
 };
 
@@ -121,6 +122,7 @@  enum igt_atomic_connector_properties {
        IGT_CONNECTOR_DPMS,
        IGT_CONNECTOR_BROADCAST_RGB,
        IGT_CONNECTOR_CONTENT_PROTECTION,
+       IGT_CONNECTOR_VRR_CAPABLE,
        IGT_NUM_CONNECTOR_PROPS
 };
 
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index baac7ae0..239bed59 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -88,6 +88,7 @@  TESTS_progs = \
 	kms_tv_load_detect \
 	kms_universal_plane \
 	kms_vblank \
+	kms_vrr \
 	kms_sequence \
 	meta_test \
 	perf \
diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c
new file mode 100644
index 00000000..c6b88e53
--- /dev/null
+++ b/tests/kms_vrr.c
@@ -0,0 +1,417 @@ 
+/*
+ * Copyright 2018 Advanced Micro Devices, Inc.
+ *
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#include "igt.h"
+#include "sw_sync.h"
+#include <fcntl.h>
+#include <signal.h>
+
+#define NSECS_PER_SEC (1000000000ull)
+
+/*
+ * Each test measurement step runs for ~5 seconds.
+ * This gives a decent sample size + enough time for any adaptation to occur if necessary.
+ */
+#define TEST_DURATION_NS (5000000000ull)
+
+enum {
+	TEST_NONE = 0,
+	TEST_DPMS = 1 << 0,
+	TEST_SUSPEND = 1 << 1,
+};
+
+typedef struct range {
+	unsigned int min;
+	unsigned int max;
+} range_t;
+
+typedef struct data {
+	igt_display_t display;
+	int drm_fd;
+	igt_fb_t fb0;
+	igt_fb_t fb1;
+} data_t;
+
+typedef void (*test_t)(data_t*, enum pipe, igt_output_t*, uint32_t);
+
+/* Converts a timespec structure to nanoseconds. */
+static uint64_t timespec_to_ns(struct timespec *ts)
+{
+	return ts->tv_sec * NSECS_PER_SEC + ts->tv_nsec;
+}
+
+/*
+ * Gets a vblank event from DRM and returns its timestamp in nanoseconds.
+ * This blocks until the event is received.
+ */
+static uint64_t get_vblank_event_ns(data_t *data)
+{
+	struct drm_event_vblank ev;
+
+	igt_set_timeout(1, "Waiting for vblank event\n");
+	igt_assert_eq(read(data->drm_fd, &ev, sizeof(ev)), sizeof(ev));
+	igt_reset_timeout();
+
+	return ev.tv_sec * NSECS_PER_SEC + ev.tv_usec * 1000ull;
+}
+
+/*
+ * Returns the current CLOCK_MONOTONIC time in nanoseconds.
+ * The regular IGT helpers can't be used since they default to
+ * CLOCK_MONOTONIC_RAW - which isn't what the kernel uses for its timestamps.
+ */
+static uint64_t get_time_ns(void)
+{
+	struct timespec ts;
+	memset(&ts, 0, sizeof(ts));
+	errno = 0;
+
+	if (!clock_gettime(CLOCK_MONOTONIC, &ts))
+		return timespec_to_ns(&ts);
+
+	igt_warn("Could not read monotonic time: %s\n", strerror(errno));
+	igt_fail(-errno);
+
+	return 0;
+}
+
+/* Returns the rate duration in nanoseconds for the given refresh rate. */
+static uint64_t rate_from_refresh(uint64_t refresh)
+{
+	return NSECS_PER_SEC / refresh;
+}
+
+/* Returns the min and max vrr range from the connector debugfs. */
+static range_t get_vrr_range(data_t *data, igt_output_t *output)
+{
+	char buf[256];
+	char *start_loc;
+	int fd, res;
+	range_t range;
+
+	fd = igt_debugfs_connector_dir(data->drm_fd, output->name, O_RDONLY);
+	igt_assert(fd >= 0);
+
+	res = igt_debugfs_simple_read(fd, "vrr_range", buf, sizeof(buf));
+	igt_require(res > 0);
+
+	close(fd);
+
+	igt_assert(start_loc = strstr(buf, "Min: "));
+	igt_assert_eq(sscanf(start_loc, "Min: %u", &range.min), 1);
+
+	igt_assert(start_loc = strstr(buf, "Max: "));
+	igt_assert_eq(sscanf(start_loc, "Max: %u", &range.max), 1);
+
+	return range;
+}
+
+/* Returns a suitable vrr test frequency. */
+static uint32_t get_test_rate_ns(data_t *data, igt_output_t *output)
+{
+	drmModeModeInfo *mode = igt_output_get_mode(output);
+	range_t range;
+	uint32_t vtest;
+
+	/*
+	 * The frequency with the fastest convergence speed should be
+	 * the midpoint between the current mode vfreq and the min
+	 * supported vfreq.
+	 */
+	range = get_vrr_range(data, output);
+	igt_require(mode->vrefresh > range.min);
+
+	vtest = (mode->vrefresh - range.min) / 2 + range.min;
+	igt_require(vtest < mode->vrefresh);
+
+	return rate_from_refresh(vtest);
+}
+
+/* Returns true if an output supports VRR. */
+static bool has_vrr(igt_output_t *output)
+{
+	return igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE) &&
+	       igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE);
+}
+
+/* Toggles variable refresh rate on the pipe. */
+static void set_vrr_on_pipe(data_t *data, enum pipe pipe, bool enabled)
+{
+	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED,
+				enabled);
+	igt_display_commit_atomic(&data->display, 0, NULL);
+}
+
+/* Prepare the display for testing on the given pipe. */
+static void prepare_test(data_t *data, igt_output_t *output, enum pipe pipe)
+{
+	drmModeModeInfo mode = *igt_output_get_mode(output);
+	igt_plane_t *primary;
+	cairo_t *cr;
+
+	/* Reset output */
+	igt_display_reset(&data->display);
+	igt_output_set_pipe(output, pipe);
+
+	/* Prepare resources */
+	igt_remove_fb(data->drm_fd, &data->fb1);
+	igt_remove_fb(data->drm_fd, &data->fb0);
+
+	igt_create_color_fb(data->drm_fd, mode.hdisplay, mode.vdisplay,
+			    DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
+			    0.50, 0.50, 0.50, &data->fb0);
+
+	igt_create_color_fb(data->drm_fd, mode.hdisplay, mode.vdisplay,
+			    DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
+			    0.50, 0.50, 0.50, &data->fb1);
+
+	cr = igt_get_cairo_ctx(data->drm_fd, &data->fb0);
+
+	igt_paint_color(cr, 0, 0, mode.hdisplay / 10, mode.vdisplay / 10,
+			1.00, 0.00, 0.00);
+
+	/* Take care of any required modesetting before the test begins. */
+	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, &data->fb0);
+
+	igt_display_commit_atomic(&data->display,
+				  DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
+}
+
+/* Waits for the vblank interval. Returns the vblank timestamp in ns. */
+static uint64_t
+wait_for_vblank(data_t *data, enum pipe pipe)
+{
+	drmVBlank vbl = { 0 };
+
+	vbl.request.type = kmstest_get_vbl_flag(pipe);
+	vbl.request.type |= DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
+	vbl.request.sequence = 1;
+	drmWaitVBlank(data->drm_fd, &vbl);
+
+	return get_vblank_event_ns(data);
+}
+
+/* Performs an asynchronous non-blocking page-flip on a pipe. */
+static int
+do_flip(data_t *data, enum pipe pipe_id, igt_fb_t *fb)
+{
+	igt_pipe_t *pipe = &data->display.pipes[pipe_id];
+	int ret;
+
+	igt_set_timeout(1, "Scheduling page flip\n");
+
+	/*
+	 * Only the legacy flip ioctl supports async flips.
+	 * It's also non-blocking, but returns -EBUSY if flipping too fast.
+	 * 2x monitor tests will need async flips in the atomic API.
+	 */
+	do {
+		ret = drmModePageFlip(data->drm_fd, pipe->crtc_id,
+				      fb->fb_id,
+				      DRM_MODE_PAGE_FLIP_EVENT |
+				      DRM_MODE_PAGE_FLIP_ASYNC,
+				      data);
+	} while (ret == -EBUSY);
+
+	igt_assert_eq(ret, 0);
+	igt_reset_timeout();
+
+	return 0;
+}
+
+/*
+ * Flips at the given rate and measures against the expected value.
+ * Returns the pass rate as a percentage from 0 - 100.
+ *
+ * The VRR API is quite flexible in terms of definition - the driver
+ * can arbitrarily restrict the bounds further than the absolute
+ * min and max range. But VRR is really about extending the flip
+ * to prevent stuttering or to match a source content rate.
+ *
+ * The only way to "present" at a fixed rate like userspace in a vendor
+ * neutral manner is to do it with async flips. This avoids the need
+ * to wait for next vblank and it should eventually converge at the
+ * desired rate.
+ */
+static uint32_t
+flip_and_measure(data_t *data, igt_output_t *output, enum pipe pipe,
+		 uint64_t rate_ns, uint64_t duration_ns)
+{
+	uint64_t start_ns, last_vblank_ns;
+	uint32_t total_flip = 0, total_pass = 0;
+	bool front = false;
+
+	/* Align with the vblank region to speed up convergence. */
+	last_vblank_ns = wait_for_vblank(data, pipe);
+	start_ns = get_time_ns();
+
+	for (;;) {
+		uint64_t now_ns, vblank_ns, wait_ns, target_ns;
+		int64_t diff_ns;
+
+		front = !front;
+		do_flip(data, pipe, front ? &data->fb1 : &data->fb0);
+
+		vblank_ns = get_vblank_event_ns(data);
+		diff_ns = rate_ns - (vblank_ns - last_vblank_ns);
+		last_vblank_ns = vblank_ns;
+
+		total_flip += 1;
+
+		/*
+		 * Check if the difference between the two flip timestamps
+		 * was within the required threshold from the expected rate.
+		 *
+		 * A ~50us threshold is arbitrary, but it's roughly the
+		 * difference between 144Hz and 143Hz which should give this
+		 * enough accuracy for most use cases.
+		 */
+		if (llabs(diff_ns) < 50000ll)
+			total_pass += 1;
+
+		now_ns = get_time_ns();
+		if (now_ns - start_ns > duration_ns)
+			break;
+
+		/*
+		 * Burn CPU until next timestamp, sleeping isn't accurate enough.
+		 * It's worth noting that the target timestamp is based on absolute
+		 * timestamp rather than a delta to avoid accumulation errors.
+		 */
+		diff_ns = now_ns - start_ns;
+		wait_ns = ((diff_ns + rate_ns - 1) / rate_ns) * rate_ns;
+		target_ns = start_ns + wait_ns - 10;
+
+		while (get_time_ns() < target_ns);
+	}
+
+	igt_info("Completed %u flips, %u were in threshold for %luns.\n",
+		 total_flip, total_pass, rate_ns);
+
+	return total_flip ? ((total_pass * 100) / total_flip) : 0;
+}
+
+/* Basic VRR flip functionality test - enable, measure, disable, measure */
+static void
+test_basic(data_t *data, enum pipe pipe, igt_output_t *output, uint32_t flags)
+{
+	uint64_t rate;
+	uint32_t result;
+
+	rate = get_test_rate_ns(data, output);
+
+	prepare_test(data, output, pipe);
+
+	set_vrr_on_pipe(data, pipe, 1);
+
+	/*
+	 * Do a short run with VRR, but don't check the result.
+	 * This is to make sure we were actually in the middle of
+	 * active flipping before doing the DPMS/suspend steps.
+	 */
+	flip_and_measure(data, output, pipe, rate, 250000000ull);
+
+	if (flags & TEST_DPMS) {
+		kmstest_set_connector_dpms(output->display->drm_fd,
+					   output->config.connector,
+					   DRM_MODE_DPMS_OFF);
+		kmstest_set_connector_dpms(output->display->drm_fd,
+					   output->config.connector,
+					   DRM_MODE_DPMS_ON);
+	}
+
+	if (flags & TEST_SUSPEND)
+		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
+					      SUSPEND_TEST_NONE);
+
+	result = flip_and_measure(data, output, pipe, rate, TEST_DURATION_NS);
+
+	set_vrr_on_pipe(data, pipe, 0);
+
+	/* This check is delayed until after VRR is disabled so it isn't
+	 * left enabled if the test fails. */
+	igt_assert_f(result > 75,
+		     "Target VRR on threshold not reached, result was %u%%\n",
+		     result);
+
+	result = flip_and_measure(data, output, pipe, rate, TEST_DURATION_NS);
+
+	igt_assert_f(result < 10,
+		     "Target VRR off threshold exceeded, result was %u%%\n",
+		     result);
+}
+
+/* Runs tests on outputs that are VRR capable. */
+static void
+run_vrr_test(data_t *data, test_t test, uint32_t flags)
+{
+	igt_output_t *output;
+	bool found = false;
+
+	for_each_connected_output(&data->display, output) {
+		enum pipe pipe;
+
+		if (!has_vrr(output))
+			continue;
+
+		for_each_pipe(&data->display, pipe)
+			if (igt_pipe_connector_valid(pipe, output)) {
+				test(data, pipe, output, flags);
+				found = true;
+				break;
+			}
+	}
+
+	if (!found)
+		igt_skip("No vrr capable outputs found.\n");
+}
+
+igt_main
+{
+	data_t data = { 0 };
+
+	igt_skip_on_simulation();
+
+	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_require(data.display.is_atomic);
+		igt_display_require_output(&data.display);
+	}
+
+	igt_subtest("flip-basic")
+		run_vrr_test(&data, test_basic, 0);
+
+	igt_subtest("flip-dpms")
+		run_vrr_test(&data, test_basic, TEST_DPMS);
+
+	igt_subtest("flip-suspend")
+		run_vrr_test(&data, test_basic, TEST_SUSPEND);
+
+	igt_fixture {
+		igt_display_fini(&data.display);
+	}
+}
diff --git a/tests/meson.build b/tests/meson.build
index b8a6e61b..e887f131 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -59,6 +59,7 @@  test_progs = [
 	'kms_tv_load_detect',
 	'kms_universal_plane',
 	'kms_vblank',
+	'kms_vrr',
 	'meta_test',
 	'perf',
 	'pm_backlight',

Comments

On 2019-01-09 2:49 p.m., Nicholas Kazlauskas wrote:
> There are 3 tests for basic variable refresh rate functionality.

> 

> The tests measure flipping at the midpoint between the current mode's

> refresh rate and the minimum supported variable refresh rate.

> 

> It tests that VRR is enabled and that the difference between flip

> timestamps converges to the requested rate. It also tests this under

> both S3 and DPMS.

> 

> Potential ideas for future tests:

> - Test behavior inside VRR range with a stepping test

> - Test behavior outside of VRR range

> - Multi-monitor (limited by no async pageflips in DRM atomic API)

> 

> Cc: Harry Wentland <harry.wentland@amd.com>

> Cc: Leo Li <sunpeng.li@amd.com>

> Cc: Manasi Navare <manasi.d.navare@intel.com>

> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

> ---

>  lib/igt_kms.c          |   3 +

>  lib/igt_kms.h          |   2 +

>  tests/Makefile.sources |   1 +

>  tests/kms_vrr.c        | 417 +++++++++++++++++++++++++++++++++++++++++

>  tests/meson.build      |   1 +

>  5 files changed, 424 insertions(+)

>  create mode 100644 tests/kms_vrr.c

> 

> diff --git a/lib/igt_kms.c b/lib/igt_kms.c

> index 684a599c..2edf19ec 100644

> --- a/lib/igt_kms.c

> +++ b/lib/igt_kms.c

> @@ -189,6 +189,7 @@ const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {

>  	[IGT_CRTC_MODE_ID] = "MODE_ID",

>  	[IGT_CRTC_ACTIVE] = "ACTIVE",

>  	[IGT_CRTC_OUT_FENCE_PTR] = "OUT_FENCE_PTR",

> +	[IGT_CRTC_VRR_ENABLED] = "VRR_ENABLED",

>  };

>  

>  const char * const igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {

> @@ -197,6 +198,7 @@ const char * const igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {

>  	[IGT_CONNECTOR_DPMS] = "DPMS",

>  	[IGT_CONNECTOR_BROADCAST_RGB] = "Broadcast RGB",

>  	[IGT_CONNECTOR_CONTENT_PROTECTION] = "Content Protection",

> +	[IGT_CONNECTOR_VRR_CAPABLE] = "vrr_capable",

>  };

>  

>  /*

> @@ -1786,6 +1788,7 @@ static void igt_pipe_reset(igt_pipe_t *pipe)

>  {

>  	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, 0);

>  	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_ACTIVE, 0);

> +	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_VRR_ENABLED, 0);

>  	igt_pipe_obj_clear_prop_changed(pipe, IGT_CRTC_OUT_FENCE_PTR);

>  

>  	pipe->out_fence_fd = -1;

> diff --git a/lib/igt_kms.h b/lib/igt_kms.h

> index 4a7c3c97..679d4e84 100644

> --- a/lib/igt_kms.h

> +++ b/lib/igt_kms.h

> @@ -104,6 +104,7 @@ enum igt_atomic_crtc_properties {

>         IGT_CRTC_MODE_ID,

>         IGT_CRTC_ACTIVE,

>         IGT_CRTC_OUT_FENCE_PTR,

> +       IGT_CRTC_VRR_ENABLED,

>         IGT_NUM_CRTC_PROPS

>  };

>  

> @@ -121,6 +122,7 @@ enum igt_atomic_connector_properties {

>         IGT_CONNECTOR_DPMS,

>         IGT_CONNECTOR_BROADCAST_RGB,

>         IGT_CONNECTOR_CONTENT_PROTECTION,

> +       IGT_CONNECTOR_VRR_CAPABLE,

>         IGT_NUM_CONNECTOR_PROPS

>  };

>  

> diff --git a/tests/Makefile.sources b/tests/Makefile.sources

> index baac7ae0..239bed59 100644

> --- a/tests/Makefile.sources

> +++ b/tests/Makefile.sources

> @@ -88,6 +88,7 @@ TESTS_progs = \

>  	kms_tv_load_detect \

>  	kms_universal_plane \

>  	kms_vblank \

> +	kms_vrr \

>  	kms_sequence \

>  	meta_test \

>  	perf \

> diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c

> new file mode 100644

> index 00000000..c6b88e53

> --- /dev/null

> +++ b/tests/kms_vrr.c

> @@ -0,0 +1,417 @@

> +/*

> + * Copyright 2018 Advanced Micro Devices, Inc.

> + *

> + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.

> + */

> +

> +#include "igt.h"

> +#include "sw_sync.h"

> +#include <fcntl.h>

> +#include <signal.h>

> +

> +#define NSECS_PER_SEC (1000000000ull)

> +

> +/*

> + * Each test measurement step runs for ~5 seconds.

> + * This gives a decent sample size + enough time for any adaptation to occur if necessary.

> + */

> +#define TEST_DURATION_NS (5000000000ull)

> +

> +enum {

> +	TEST_NONE = 0,

> +	TEST_DPMS = 1 << 0,

> +	TEST_SUSPEND = 1 << 1,

> +};

> +

> +typedef struct range {

> +	unsigned int min;

> +	unsigned int max;

> +} range_t;

> +

> +typedef struct data {

> +	igt_display_t display;

> +	int drm_fd;

> +	igt_fb_t fb0;

> +	igt_fb_t fb1;

> +} data_t;

> +

> +typedef void (*test_t)(data_t*, enum pipe, igt_output_t*, uint32_t);

> +

> +/* Converts a timespec structure to nanoseconds. */

> +static uint64_t timespec_to_ns(struct timespec *ts)

> +{

> +	return ts->tv_sec * NSECS_PER_SEC + ts->tv_nsec;

> +}

> +

> +/*

> + * Gets a vblank event from DRM and returns its timestamp in nanoseconds.

> + * This blocks until the event is received.

> + */

> +static uint64_t get_vblank_event_ns(data_t *data)

> +{

> +	struct drm_event_vblank ev;

> +

> +	igt_set_timeout(1, "Waiting for vblank event\n");

> +	igt_assert_eq(read(data->drm_fd, &ev, sizeof(ev)), sizeof(ev));

> +	igt_reset_timeout();

> +

> +	return ev.tv_sec * NSECS_PER_SEC + ev.tv_usec * 1000ull;

> +}

> +

> +/*

> + * Returns the current CLOCK_MONOTONIC time in nanoseconds.

> + * The regular IGT helpers can't be used since they default to

> + * CLOCK_MONOTONIC_RAW - which isn't what the kernel uses for its timestamps.

> + */

> +static uint64_t get_time_ns(void)

> +{

> +	struct timespec ts;

> +	memset(&ts, 0, sizeof(ts));

> +	errno = 0;

> +

> +	if (!clock_gettime(CLOCK_MONOTONIC, &ts))

> +		return timespec_to_ns(&ts);

> +

> +	igt_warn("Could not read monotonic time: %s\n", strerror(errno));

> +	igt_fail(-errno);

> +

> +	return 0;

> +}

> +

> +/* Returns the rate duration in nanoseconds for the given refresh rate. */

> +static uint64_t rate_from_refresh(uint64_t refresh)

> +{

> +	return NSECS_PER_SEC / refresh;

> +}

> +

> +/* Returns the min and max vrr range from the connector debugfs. */

> +static range_t get_vrr_range(data_t *data, igt_output_t *output)

> +{

> +	char buf[256];

> +	char *start_loc;

> +	int fd, res;

> +	range_t range;

> +

> +	fd = igt_debugfs_connector_dir(data->drm_fd, output->name, O_RDONLY);

> +	igt_assert(fd >= 0);

> +

> +	res = igt_debugfs_simple_read(fd, "vrr_range", buf, sizeof(buf));

> +	igt_require(res > 0);

> +

> +	close(fd);

> +

> +	igt_assert(start_loc = strstr(buf, "Min: "));

> +	igt_assert_eq(sscanf(start_loc, "Min: %u", &range.min), 1);

> +

> +	igt_assert(start_loc = strstr(buf, "Max: "));

> +	igt_assert_eq(sscanf(start_loc, "Max: %u", &range.max), 1);

> +


Would be good to see this patch on amd-gfx so reviewers can reference it.

> +	return range;

> +}

> +

> +/* Returns a suitable vrr test frequency. */

> +static uint32_t get_test_rate_ns(data_t *data, igt_output_t *output)

> +{

> +	drmModeModeInfo *mode = igt_output_get_mode(output);

> +	range_t range;

> +	uint32_t vtest;

> +

> +	/*

> +	 * The frequency with the fastest convergence speed should be

> +	 * the midpoint between the current mode vfreq and the min

> +	 * supported vfreq.

> +	 */

> +	range = get_vrr_range(data, output);

> +	igt_require(mode->vrefresh > range.min);

> +

> +	vtest = (mode->vrefresh - range.min) / 2 + range.min;

> +	igt_require(vtest < mode->vrefresh);

> +

> +	return rate_from_refresh(vtest);

> +}

> +

> +/* Returns true if an output supports VRR. */

> +static bool has_vrr(igt_output_t *output)

> +{

> +	return igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE) &&

> +	       igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE);

> +}

> +

> +/* Toggles variable refresh rate on the pipe. */

> +static void set_vrr_on_pipe(data_t *data, enum pipe pipe, bool enabled)

> +{

> +	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED,

> +				enabled);

> +	igt_display_commit_atomic(&data->display, 0, NULL);

> +}

> +

> +/* Prepare the display for testing on the given pipe. */

> +static void prepare_test(data_t *data, igt_output_t *output, enum pipe pipe)

> +{

> +	drmModeModeInfo mode = *igt_output_get_mode(output);

> +	igt_plane_t *primary;

> +	cairo_t *cr;

> +

> +	/* Reset output */

> +	igt_display_reset(&data->display);

> +	igt_output_set_pipe(output, pipe);

> +

> +	/* Prepare resources */

> +	igt_remove_fb(data->drm_fd, &data->fb1);

> +	igt_remove_fb(data->drm_fd, &data->fb0);

> +


Should we do this at the end of each test in a cleanup_test? Not sure why we're doing this here, other than cleaning up from the previous test.

Harry

> +	igt_create_color_fb(data->drm_fd, mode.hdisplay, mode.vdisplay,

> +			    DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE,

> +			    0.50, 0.50, 0.50, &data->fb0);

> +

> +	igt_create_color_fb(data->drm_fd, mode.hdisplay, mode.vdisplay,

> +			    DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE,

> +			    0.50, 0.50, 0.50, &data->fb1);

> +

> +	cr = igt_get_cairo_ctx(data->drm_fd, &data->fb0);

> +

> +	igt_paint_color(cr, 0, 0, mode.hdisplay / 10, mode.vdisplay / 10,

> +			1.00, 0.00, 0.00);

> +

> +	/* Take care of any required modesetting before the test begins. */

> +	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);

> +	igt_plane_set_fb(primary, &data->fb0);

> +

> +	igt_display_commit_atomic(&data->display,

> +				  DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);

> +}

> +

> +/* Waits for the vblank interval. Returns the vblank timestamp in ns. */

> +static uint64_t

> +wait_for_vblank(data_t *data, enum pipe pipe)

> +{

> +	drmVBlank vbl = { 0 };

> +

> +	vbl.request.type = kmstest_get_vbl_flag(pipe);

> +	vbl.request.type |= DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;

> +	vbl.request.sequence = 1;

> +	drmWaitVBlank(data->drm_fd, &vbl);

> +

> +	return get_vblank_event_ns(data);

> +}

> +

> +/* Performs an asynchronous non-blocking page-flip on a pipe. */

> +static int

> +do_flip(data_t *data, enum pipe pipe_id, igt_fb_t *fb)

> +{

> +	igt_pipe_t *pipe = &data->display.pipes[pipe_id];

> +	int ret;

> +

> +	igt_set_timeout(1, "Scheduling page flip\n");

> +

> +	/*

> +	 * Only the legacy flip ioctl supports async flips.

> +	 * It's also non-blocking, but returns -EBUSY if flipping too fast.

> +	 * 2x monitor tests will need async flips in the atomic API.

> +	 */

> +	do {

> +		ret = drmModePageFlip(data->drm_fd, pipe->crtc_id,

> +				      fb->fb_id,

> +				      DRM_MODE_PAGE_FLIP_EVENT |

> +				      DRM_MODE_PAGE_FLIP_ASYNC,

> +				      data);

> +	} while (ret == -EBUSY);

> +

> +	igt_assert_eq(ret, 0);

> +	igt_reset_timeout();

> +

> +	return 0;

> +}

> +

> +/*

> + * Flips at the given rate and measures against the expected value.

> + * Returns the pass rate as a percentage from 0 - 100.

> + *

> + * The VRR API is quite flexible in terms of definition - the driver

> + * can arbitrarily restrict the bounds further than the absolute

> + * min and max range. But VRR is really about extending the flip

> + * to prevent stuttering or to match a source content rate.

> + *

> + * The only way to "present" at a fixed rate like userspace in a vendor

> + * neutral manner is to do it with async flips. This avoids the need

> + * to wait for next vblank and it should eventually converge at the

> + * desired rate.

> + */

> +static uint32_t

> +flip_and_measure(data_t *data, igt_output_t *output, enum pipe pipe,

> +		 uint64_t rate_ns, uint64_t duration_ns)

> +{

> +	uint64_t start_ns, last_vblank_ns;

> +	uint32_t total_flip = 0, total_pass = 0;

> +	bool front = false;

> +

> +	/* Align with the vblank region to speed up convergence. */

> +	last_vblank_ns = wait_for_vblank(data, pipe);

> +	start_ns = get_time_ns();

> +

> +	for (;;) {

> +		uint64_t now_ns, vblank_ns, wait_ns, target_ns;

> +		int64_t diff_ns;

> +

> +		front = !front;

> +		do_flip(data, pipe, front ? &data->fb1 : &data->fb0);

> +

> +		vblank_ns = get_vblank_event_ns(data);

> +		diff_ns = rate_ns - (vblank_ns - last_vblank_ns);

> +		last_vblank_ns = vblank_ns;

> +

> +		total_flip += 1;

> +

> +		/*

> +		 * Check if the difference between the two flip timestamps

> +		 * was within the required threshold from the expected rate.

> +		 *

> +		 * A ~50us threshold is arbitrary, but it's roughly the

> +		 * difference between 144Hz and 143Hz which should give this

> +		 * enough accuracy for most use cases.

> +		 */

> +		if (llabs(diff_ns) < 50000ll)

> +			total_pass += 1;

> +

> +		now_ns = get_time_ns();

> +		if (now_ns - start_ns > duration_ns)

> +			break;

> +

> +		/*

> +		 * Burn CPU until next timestamp, sleeping isn't accurate enough.

> +		 * It's worth noting that the target timestamp is based on absolute

> +		 * timestamp rather than a delta to avoid accumulation errors.

> +		 */

> +		diff_ns = now_ns - start_ns;

> +		wait_ns = ((diff_ns + rate_ns - 1) / rate_ns) * rate_ns;

> +		target_ns = start_ns + wait_ns - 10;

> +

> +		while (get_time_ns() < target_ns);

> +	}

> +

> +	igt_info("Completed %u flips, %u were in threshold for %luns.\n",

> +		 total_flip, total_pass, rate_ns);

> +

> +	return total_flip ? ((total_pass * 100) / total_flip) : 0;

> +}

> +

> +/* Basic VRR flip functionality test - enable, measure, disable, measure */

> +static void

> +test_basic(data_t *data, enum pipe pipe, igt_output_t *output, uint32_t flags)

> +{

> +	uint64_t rate;

> +	uint32_t result;

> +

> +	rate = get_test_rate_ns(data, output);

> +

> +	prepare_test(data, output, pipe);

> +

> +	set_vrr_on_pipe(data, pipe, 1);

> +

> +	/*

> +	 * Do a short run with VRR, but don't check the result.

> +	 * This is to make sure we were actually in the middle of

> +	 * active flipping before doing the DPMS/suspend steps.

> +	 */

> +	flip_and_measure(data, output, pipe, rate, 250000000ull);

> +

> +	if (flags & TEST_DPMS) {

> +		kmstest_set_connector_dpms(output->display->drm_fd,

> +					   output->config.connector,

> +					   DRM_MODE_DPMS_OFF);

> +		kmstest_set_connector_dpms(output->display->drm_fd,

> +					   output->config.connector,

> +					   DRM_MODE_DPMS_ON);

> +	}

> +

> +	if (flags & TEST_SUSPEND)

> +		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,

> +					      SUSPEND_TEST_NONE);

> +

> +	result = flip_and_measure(data, output, pipe, rate, TEST_DURATION_NS);

> +

> +	set_vrr_on_pipe(data, pipe, 0);

> +

> +	/* This check is delayed until after VRR is disabled so it isn't

> +	 * left enabled if the test fails. */

> +	igt_assert_f(result > 75,

> +		     "Target VRR on threshold not reached, result was %u%%\n",

> +		     result);

> +

> +	result = flip_and_measure(data, output, pipe, rate, TEST_DURATION_NS);

> +

> +	igt_assert_f(result < 10,

> +		     "Target VRR off threshold exceeded, result was %u%%\n",

> +		     result);

> +}

> +

> +/* Runs tests on outputs that are VRR capable. */

> +static void

> +run_vrr_test(data_t *data, test_t test, uint32_t flags)

> +{

> +	igt_output_t *output;

> +	bool found = false;

> +

> +	for_each_connected_output(&data->display, output) {

> +		enum pipe pipe;

> +

> +		if (!has_vrr(output))

> +			continue;

> +

> +		for_each_pipe(&data->display, pipe)

> +			if (igt_pipe_connector_valid(pipe, output)) {

> +				test(data, pipe, output, flags);

> +				found = true;

> +				break;

> +			}

> +	}

> +

> +	if (!found)

> +		igt_skip("No vrr capable outputs found.\n");

> +}

> +

> +igt_main

> +{

> +	data_t data = { 0 };

> +

> +	igt_skip_on_simulation();

> +

> +	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_require(data.display.is_atomic);

> +		igt_display_require_output(&data.display);

> +	}

> +

> +	igt_subtest("flip-basic")

> +		run_vrr_test(&data, test_basic, 0);

> +

> +	igt_subtest("flip-dpms")

> +		run_vrr_test(&data, test_basic, TEST_DPMS);

> +

> +	igt_subtest("flip-suspend")

> +		run_vrr_test(&data, test_basic, TEST_SUSPEND);

> +

> +	igt_fixture {

> +		igt_display_fini(&data.display);

> +	}

> +}

> diff --git a/tests/meson.build b/tests/meson.build

> index b8a6e61b..e887f131 100644

> --- a/tests/meson.build

> +++ b/tests/meson.build

> @@ -59,6 +59,7 @@ test_progs = [

>  	'kms_tv_load_detect',

>  	'kms_universal_plane',

>  	'kms_vblank',

> +	'kms_vrr',

>  	'meta_test',

>  	'perf',

>  	'pm_backlight',

>
On 1/24/19 11:04 AM, Wentland, Harry wrote:
> On 2019-01-09 2:49 p.m., Nicholas Kazlauskas wrote:

>> There are 3 tests for basic variable refresh rate functionality.

>>

>> The tests measure flipping at the midpoint between the current mode's

>> refresh rate and the minimum supported variable refresh rate.

>>

>> It tests that VRR is enabled and that the difference between flip

>> timestamps converges to the requested rate. It also tests this under

>> both S3 and DPMS.

>>

>> Potential ideas for future tests:

>> - Test behavior inside VRR range with a stepping test

>> - Test behavior outside of VRR range

>> - Multi-monitor (limited by no async pageflips in DRM atomic API)

>>

>> Cc: Harry Wentland <harry.wentland@amd.com>

>> Cc: Leo Li <sunpeng.li@amd.com>

>> Cc: Manasi Navare <manasi.d.navare@intel.com>

>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

>> ---

>>   lib/igt_kms.c          |   3 +

>>   lib/igt_kms.h          |   2 +

>>   tests/Makefile.sources |   1 +

>>   tests/kms_vrr.c        | 417 +++++++++++++++++++++++++++++++++++++++++

>>   tests/meson.build      |   1 +

>>   5 files changed, 424 insertions(+)

>>   create mode 100644 tests/kms_vrr.c

>>

>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c

>> index 684a599c..2edf19ec 100644

>> --- a/lib/igt_kms.c

>> +++ b/lib/igt_kms.c

>> @@ -189,6 +189,7 @@ const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {

>>   	[IGT_CRTC_MODE_ID] = "MODE_ID",

>>   	[IGT_CRTC_ACTIVE] = "ACTIVE",

>>   	[IGT_CRTC_OUT_FENCE_PTR] = "OUT_FENCE_PTR",

>> +	[IGT_CRTC_VRR_ENABLED] = "VRR_ENABLED",

>>   };

>>   

>>   const char * const igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {

>> @@ -197,6 +198,7 @@ const char * const igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {

>>   	[IGT_CONNECTOR_DPMS] = "DPMS",

>>   	[IGT_CONNECTOR_BROADCAST_RGB] = "Broadcast RGB",

>>   	[IGT_CONNECTOR_CONTENT_PROTECTION] = "Content Protection",

>> +	[IGT_CONNECTOR_VRR_CAPABLE] = "vrr_capable",

>>   };

>>   

>>   /*

>> @@ -1786,6 +1788,7 @@ static void igt_pipe_reset(igt_pipe_t *pipe)

>>   {

>>   	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, 0);

>>   	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_ACTIVE, 0);

>> +	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_VRR_ENABLED, 0);

>>   	igt_pipe_obj_clear_prop_changed(pipe, IGT_CRTC_OUT_FENCE_PTR);

>>   

>>   	pipe->out_fence_fd = -1;

>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h

>> index 4a7c3c97..679d4e84 100644

>> --- a/lib/igt_kms.h

>> +++ b/lib/igt_kms.h

>> @@ -104,6 +104,7 @@ enum igt_atomic_crtc_properties {

>>          IGT_CRTC_MODE_ID,

>>          IGT_CRTC_ACTIVE,

>>          IGT_CRTC_OUT_FENCE_PTR,

>> +       IGT_CRTC_VRR_ENABLED,

>>          IGT_NUM_CRTC_PROPS

>>   };

>>   

>> @@ -121,6 +122,7 @@ enum igt_atomic_connector_properties {

>>          IGT_CONNECTOR_DPMS,

>>          IGT_CONNECTOR_BROADCAST_RGB,

>>          IGT_CONNECTOR_CONTENT_PROTECTION,

>> +       IGT_CONNECTOR_VRR_CAPABLE,

>>          IGT_NUM_CONNECTOR_PROPS

>>   };

>>   

>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources

>> index baac7ae0..239bed59 100644

>> --- a/tests/Makefile.sources

>> +++ b/tests/Makefile.sources

>> @@ -88,6 +88,7 @@ TESTS_progs = \

>>   	kms_tv_load_detect \

>>   	kms_universal_plane \

>>   	kms_vblank \

>> +	kms_vrr \

>>   	kms_sequence \

>>   	meta_test \

>>   	perf \

>> diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c

>> new file mode 100644

>> index 00000000..c6b88e53

>> --- /dev/null

>> +++ b/tests/kms_vrr.c

>> @@ -0,0 +1,417 @@

>> +/*

>> + * Copyright 2018 Advanced Micro Devices, Inc.

>> + *

>> + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.

>> + */

>> +

>> +#include "igt.h"

>> +#include "sw_sync.h"

>> +#include <fcntl.h>

>> +#include <signal.h>

>> +

>> +#define NSECS_PER_SEC (1000000000ull)

>> +

>> +/*

>> + * Each test measurement step runs for ~5 seconds.

>> + * This gives a decent sample size + enough time for any adaptation to occur if necessary.

>> + */

>> +#define TEST_DURATION_NS (5000000000ull)

>> +

>> +enum {

>> +	TEST_NONE = 0,

>> +	TEST_DPMS = 1 << 0,

>> +	TEST_SUSPEND = 1 << 1,

>> +};

>> +

>> +typedef struct range {

>> +	unsigned int min;

>> +	unsigned int max;

>> +} range_t;

>> +

>> +typedef struct data {

>> +	igt_display_t display;

>> +	int drm_fd;

>> +	igt_fb_t fb0;

>> +	igt_fb_t fb1;

>> +} data_t;

>> +

>> +typedef void (*test_t)(data_t*, enum pipe, igt_output_t*, uint32_t);

>> +

>> +/* Converts a timespec structure to nanoseconds. */

>> +static uint64_t timespec_to_ns(struct timespec *ts)

>> +{

>> +	return ts->tv_sec * NSECS_PER_SEC + ts->tv_nsec;

>> +}

>> +

>> +/*

>> + * Gets a vblank event from DRM and returns its timestamp in nanoseconds.

>> + * This blocks until the event is received.

>> + */

>> +static uint64_t get_vblank_event_ns(data_t *data)

>> +{

>> +	struct drm_event_vblank ev;

>> +

>> +	igt_set_timeout(1, "Waiting for vblank event\n");

>> +	igt_assert_eq(read(data->drm_fd, &ev, sizeof(ev)), sizeof(ev));

>> +	igt_reset_timeout();

>> +

>> +	return ev.tv_sec * NSECS_PER_SEC + ev.tv_usec * 1000ull;

>> +}

>> +

>> +/*

>> + * Returns the current CLOCK_MONOTONIC time in nanoseconds.

>> + * The regular IGT helpers can't be used since they default to

>> + * CLOCK_MONOTONIC_RAW - which isn't what the kernel uses for its timestamps.

>> + */

>> +static uint64_t get_time_ns(void)

>> +{

>> +	struct timespec ts;

>> +	memset(&ts, 0, sizeof(ts));

>> +	errno = 0;

>> +

>> +	if (!clock_gettime(CLOCK_MONOTONIC, &ts))

>> +		return timespec_to_ns(&ts);

>> +

>> +	igt_warn("Could not read monotonic time: %s\n", strerror(errno));

>> +	igt_fail(-errno);

>> +

>> +	return 0;

>> +}

>> +

>> +/* Returns the rate duration in nanoseconds for the given refresh rate. */

>> +static uint64_t rate_from_refresh(uint64_t refresh)

>> +{

>> +	return NSECS_PER_SEC / refresh;

>> +}

>> +

>> +/* Returns the min and max vrr range from the connector debugfs. */

>> +static range_t get_vrr_range(data_t *data, igt_output_t *output)

>> +{

>> +	char buf[256];

>> +	char *start_loc;

>> +	int fd, res;

>> +	range_t range;

>> +

>> +	fd = igt_debugfs_connector_dir(data->drm_fd, output->name, O_RDONLY);

>> +	igt_assert(fd >= 0);

>> +

>> +	res = igt_debugfs_simple_read(fd, "vrr_range", buf, sizeof(buf));

>> +	igt_require(res > 0);

>> +

>> +	close(fd);

>> +

>> +	igt_assert(start_loc = strstr(buf, "Min: "));

>> +	igt_assert_eq(sscanf(start_loc, "Min: %u", &range.min), 1);

>> +

>> +	igt_assert(start_loc = strstr(buf, "Max: "));

>> +	igt_assert_eq(sscanf(start_loc, "Max: %u", &range.max), 1);

>> +

> 

> Would be good to see this patch on amd-gfx so reviewers can reference it.


Sure. I was thinking that I'd get review on this patch first and do any 
changes necessary to that patch after, but I can see the value in having 
them both up at the same time.

> 

>> +	return range;

>> +}

>> +

>> +/* Returns a suitable vrr test frequency. */

>> +static uint32_t get_test_rate_ns(data_t *data, igt_output_t *output)

>> +{

>> +	drmModeModeInfo *mode = igt_output_get_mode(output);

>> +	range_t range;

>> +	uint32_t vtest;

>> +

>> +	/*

>> +	 * The frequency with the fastest convergence speed should be

>> +	 * the midpoint between the current mode vfreq and the min

>> +	 * supported vfreq.

>> +	 */

>> +	range = get_vrr_range(data, output);

>> +	igt_require(mode->vrefresh > range.min);

>> +

>> +	vtest = (mode->vrefresh - range.min) / 2 + range.min;

>> +	igt_require(vtest < mode->vrefresh);

>> +

>> +	return rate_from_refresh(vtest);

>> +}

>> +

>> +/* Returns true if an output supports VRR. */

>> +static bool has_vrr(igt_output_t *output)

>> +{

>> +	return igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE) &&

>> +	       igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE);

>> +}

>> +

>> +/* Toggles variable refresh rate on the pipe. */

>> +static void set_vrr_on_pipe(data_t *data, enum pipe pipe, bool enabled)

>> +{

>> +	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED,

>> +				enabled);

>> +	igt_display_commit_atomic(&data->display, 0, NULL);

>> +}

>> +

>> +/* Prepare the display for testing on the given pipe. */

>> +static void prepare_test(data_t *data, igt_output_t *output, enum pipe pipe)

>> +{

>> +	drmModeModeInfo mode = *igt_output_get_mode(output);

>> +	igt_plane_t *primary;

>> +	cairo_t *cr;

>> +

>> +	/* Reset output */

>> +	igt_display_reset(&data->display);

>> +	igt_output_set_pipe(output, pipe);

>> +

>> +	/* Prepare resources */

>> +	igt_remove_fb(data->drm_fd, &data->fb1);

>> +	igt_remove_fb(data->drm_fd, &data->fb0);

>> +

> 

> Should we do this at the end of each test in a cleanup_test? Not sure why we're doing this here, other than cleaning up from the previous test.

> 

> Harry


I felt that it was better logically grouped with the reset.

I guess this is technically leaking FBs on test success, however. I 
think there's also leaking for the cairo ctx below...

> 

>> +	igt_create_color_fb(data->drm_fd, mode.hdisplay, mode.vdisplay,

>> +			    DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE,

>> +			    0.50, 0.50, 0.50, &data->fb0);

>> +

>> +	igt_create_color_fb(data->drm_fd, mode.hdisplay, mode.vdisplay,

>> +			    DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE,

>> +			    0.50, 0.50, 0.50, &data->fb1);

>> +

>> +	cr = igt_get_cairo_ctx(data->drm_fd, &data->fb0);

>> +

>> +	igt_paint_color(cr, 0, 0, mode.hdisplay / 10, mode.vdisplay / 10,

>> +			1.00, 0.00, 0.00);

>> +

>> +	/* Take care of any required modesetting before the test begins. */

>> +	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);

>> +	igt_plane_set_fb(primary, &data->fb0);

>> +

>> +	igt_display_commit_atomic(&data->display,

>> +				  DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);


...here.

This probably needs a igt_put_cairo_ctx here.

>> +}

>> +

>> +/* Waits for the vblank interval. Returns the vblank timestamp in ns. */

>> +static uint64_t

>> +wait_for_vblank(data_t *data, enum pipe pipe)

>> +{

>> +	drmVBlank vbl = { 0 };

>> +

>> +	vbl.request.type = kmstest_get_vbl_flag(pipe);

>> +	vbl.request.type |= DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;

>> +	vbl.request.sequence = 1;

>> +	drmWaitVBlank(data->drm_fd, &vbl);

>> +

>> +	return get_vblank_event_ns(data);

>> +}

>> +

>> +/* Performs an asynchronous non-blocking page-flip on a pipe. */

>> +static int

>> +do_flip(data_t *data, enum pipe pipe_id, igt_fb_t *fb)

>> +{

>> +	igt_pipe_t *pipe = &data->display.pipes[pipe_id];

>> +	int ret;

>> +

>> +	igt_set_timeout(1, "Scheduling page flip\n");

>> +

>> +	/*

>> +	 * Only the legacy flip ioctl supports async flips.

>> +	 * It's also non-blocking, but returns -EBUSY if flipping too fast.

>> +	 * 2x monitor tests will need async flips in the atomic API.

>> +	 */

>> +	do {

>> +		ret = drmModePageFlip(data->drm_fd, pipe->crtc_id,

>> +				      fb->fb_id,

>> +				      DRM_MODE_PAGE_FLIP_EVENT |

>> +				      DRM_MODE_PAGE_FLIP_ASYNC,

>> +				      data);

>> +	} while (ret == -EBUSY);

>> +

>> +	igt_assert_eq(ret, 0);

>> +	igt_reset_timeout();

>> +

>> +	return 0;

>> +}

>> +

>> +/*

>> + * Flips at the given rate and measures against the expected value.

>> + * Returns the pass rate as a percentage from 0 - 100.

>> + *

>> + * The VRR API is quite flexible in terms of definition - the driver

>> + * can arbitrarily restrict the bounds further than the absolute

>> + * min and max range. But VRR is really about extending the flip

>> + * to prevent stuttering or to match a source content rate.

>> + *

>> + * The only way to "present" at a fixed rate like userspace in a vendor

>> + * neutral manner is to do it with async flips. This avoids the need

>> + * to wait for next vblank and it should eventually converge at the

>> + * desired rate.

>> + */

>> +static uint32_t

>> +flip_and_measure(data_t *data, igt_output_t *output, enum pipe pipe,

>> +		 uint64_t rate_ns, uint64_t duration_ns)

>> +{

>> +	uint64_t start_ns, last_vblank_ns;

>> +	uint32_t total_flip = 0, total_pass = 0;

>> +	bool front = false;

>> +

>> +	/* Align with the vblank region to speed up convergence. */

>> +	last_vblank_ns = wait_for_vblank(data, pipe);

>> +	start_ns = get_time_ns();

>> +

>> +	for (;;) {

>> +		uint64_t now_ns, vblank_ns, wait_ns, target_ns;

>> +		int64_t diff_ns;

>> +

>> +		front = !front;

>> +		do_flip(data, pipe, front ? &data->fb1 : &data->fb0);

>> +

>> +		vblank_ns = get_vblank_event_ns(data);

>> +		diff_ns = rate_ns - (vblank_ns - last_vblank_ns);

>> +		last_vblank_ns = vblank_ns;

>> +

>> +		total_flip += 1;

>> +

>> +		/*

>> +		 * Check if the difference between the two flip timestamps

>> +		 * was within the required threshold from the expected rate.

>> +		 *

>> +		 * A ~50us threshold is arbitrary, but it's roughly the

>> +		 * difference between 144Hz and 143Hz which should give this

>> +		 * enough accuracy for most use cases.

>> +		 */

>> +		if (llabs(diff_ns) < 50000ll)

>> +			total_pass += 1;

>> +

>> +		now_ns = get_time_ns();

>> +		if (now_ns - start_ns > duration_ns)

>> +			break;

>> +

>> +		/*

>> +		 * Burn CPU until next timestamp, sleeping isn't accurate enough.

>> +		 * It's worth noting that the target timestamp is based on absolute

>> +		 * timestamp rather than a delta to avoid accumulation errors.

>> +		 */

>> +		diff_ns = now_ns - start_ns;

>> +		wait_ns = ((diff_ns + rate_ns - 1) / rate_ns) * rate_ns;

>> +		target_ns = start_ns + wait_ns - 10;

>> +

>> +		while (get_time_ns() < target_ns);

>> +	}

>> +

>> +	igt_info("Completed %u flips, %u were in threshold for %luns.\n",

>> +		 total_flip, total_pass, rate_ns);

>> +

>> +	return total_flip ? ((total_pass * 100) / total_flip) : 0;

>> +}

>> +

>> +/* Basic VRR flip functionality test - enable, measure, disable, measure */

>> +static void

>> +test_basic(data_t *data, enum pipe pipe, igt_output_t *output, uint32_t flags)

>> +{

>> +	uint64_t rate;

>> +	uint32_t result;

>> +

>> +	rate = get_test_rate_ns(data, output);

>> +

>> +	prepare_test(data, output, pipe);

>> +

>> +	set_vrr_on_pipe(data, pipe, 1);

>> +

>> +	/*

>> +	 * Do a short run with VRR, but don't check the result.

>> +	 * This is to make sure we were actually in the middle of

>> +	 * active flipping before doing the DPMS/suspend steps.

>> +	 */

>> +	flip_and_measure(data, output, pipe, rate, 250000000ull);

>> +

>> +	if (flags & TEST_DPMS) {

>> +		kmstest_set_connector_dpms(output->display->drm_fd,

>> +					   output->config.connector,

>> +					   DRM_MODE_DPMS_OFF);

>> +		kmstest_set_connector_dpms(output->display->drm_fd,

>> +					   output->config.connector,

>> +					   DRM_MODE_DPMS_ON);

>> +	}

>> +

>> +	if (flags & TEST_SUSPEND)

>> +		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,

>> +					      SUSPEND_TEST_NONE);

>> +

>> +	result = flip_and_measure(data, output, pipe, rate, TEST_DURATION_NS);

>> +

>> +	set_vrr_on_pipe(data, pipe, 0);

>> +

>> +	/* This check is delayed until after VRR is disabled so it isn't

>> +	 * left enabled if the test fails. */

>> +	igt_assert_f(result > 75,

>> +		     "Target VRR on threshold not reached, result was %u%%\n",

>> +		     result);

>> +

>> +	result = flip_and_measure(data, output, pipe, rate, TEST_DURATION_NS);

>> +

>> +	igt_assert_f(result < 10,

>> +		     "Target VRR off threshold exceeded, result was %u%%\n",

>> +		     result);

>> +}

>> +

>> +/* Runs tests on outputs that are VRR capable. */

>> +static void

>> +run_vrr_test(data_t *data, test_t test, uint32_t flags)

>> +{

>> +	igt_output_t *output;

>> +	bool found = false;

>> +

>> +	for_each_connected_output(&data->display, output) {

>> +		enum pipe pipe;

>> +

>> +		if (!has_vrr(output))

>> +			continue;

>> +

>> +		for_each_pipe(&data->display, pipe)

>> +			if (igt_pipe_connector_valid(pipe, output)) {

>> +				test(data, pipe, output, flags);

>> +				found = true;

>> +				break;

>> +			}

>> +	}

>> +

>> +	if (!found)

>> +		igt_skip("No vrr capable outputs found.\n");

>> +}

>> +

>> +igt_main

>> +{

>> +	data_t data = { 0 };

>> +

>> +	igt_skip_on_simulation();

>> +

>> +	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_require(data.display.is_atomic);

>> +		igt_display_require_output(&data.display);

>> +	}

>> +

>> +	igt_subtest("flip-basic")

>> +		run_vrr_test(&data, test_basic, 0);

>> +

>> +	igt_subtest("flip-dpms")

>> +		run_vrr_test(&data, test_basic, TEST_DPMS);

>> +

>> +	igt_subtest("flip-suspend")

>> +		run_vrr_test(&data, test_basic, TEST_SUSPEND);

>> +

>> +	igt_fixture {

>> +		igt_display_fini(&data.display);

>> +	}

>> +}

>> diff --git a/tests/meson.build b/tests/meson.build

>> index b8a6e61b..e887f131 100644

>> --- a/tests/meson.build

>> +++ b/tests/meson.build

>> @@ -59,6 +59,7 @@ test_progs = [

>>   	'kms_tv_load_detect',

>>   	'kms_universal_plane',

>>   	'kms_vblank',

>> +	'kms_vrr',

>>   	'meta_test',

>>   	'perf',

>>   	'pm_backlight',

>>


Nicholas Kazlauskas
On Thu, Jan 10, 2019 at 11:09:09AM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: tests: Add variable refresh rate tests (rev2)
> URL   : https://patchwork.freedesktop.org/series/54960/
> State : failure
> 
> == Summary ==
> 
> CI Bug Log - changes from IGT_4758 -> IGTPW_2212
> ====================================================
> 
> Summary
> -------
> 
>   **FAILURE**
> 
>   Serious unknown changes coming with IGTPW_2212 absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in IGTPW_2212, please notify your bug team to allow them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   External URL: https://patchwork.freedesktop.org/api/1.0/series/54960/revisions/2/
> 
> Possible new issues
> -------------------
> 
>   Here are the unknown changes that may have been introduced in IGTPW_2212:
> 
> ### IGT changes ###
> 
> #### Possible regressions ####
> 
>   * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
>     - fi-pnv-d510:        PASS -> FAIL +19
> 
>   * igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
>     - fi-blb-e6850:       PASS -> FAIL +18
> 
>   * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
>     - fi-elk-e7500:       PASS -> FAIL +19
> 
>   * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
>     - fi-gdg-551:         PASS -> FAIL +19
> 
>   * igt@kms_pipe_crc_basic@read-crc-pipe-a:
>     - fi-bwr-2160:        PASS -> FAIL +19
> 


Just about all of these failures are this:


(kms_cursor_legacy:2598) igt_kms-DEBUG: display: commit {
(kms_cursor_legacy:2598) igt_kms-CRITICAL: Test assertion failure function igt_pipe_commit, file ../lib/igt_kms.c:2780:
(kms_cursor_legacy:2598) igt_kms-CRITICAL: Failed assertion: pipe->props[i]
(kms_cursor_legacy:2598) igt_core-INFO: Stack trace:
(kms_cursor_legacy:2598) igt_core-INFO:   #0 ../lib/igt_core.c:1472 __igt_fail_assert()
(kms_cursor_legacy:2598) igt_core-INFO:   #1 ../lib/igt_kms.c:2782 igt_pipe_commit()
(kms_cursor_legacy:2598) igt_core-INFO:   #2 ../lib/igt_kms.c:3313 do_display_commit()
(kms_cursor_legacy:2598) igt_core-INFO:   #3 ../lib/igt_kms.c:3428 igt_display_commit2()
(kms_cursor_legacy:2598) igt_core-INFO:   #4 ../tests/kms_cursor_legacy.c:522 basic_flip_cursor()
(kms_cursor_legacy:2598) igt_core-INFO:   #5 ../tests/kms_cursor_legacy.c:1491 __real_main1358()
(kms_cursor_legacy:2598) igt_core-INFO:   #6 ../tests/kms_cursor_legacy.c:1358 main()
(kms_cursor_legacy:2598) igt_core-INFO:   #7 ../csu/libc-start.c:344 __libc_start_main()
(kms_cursor_legacy:2598) igt_core-INFO:   #8 [_start+0x2a]
On 1/25/19 4:57 AM, Petri Latvala wrote:
> On Thu, Jan 10, 2019 at 11:09:09AM +0000, Patchwork wrote:

>> == Series Details ==

>>

>> Series: tests: Add variable refresh rate tests (rev2)

>> URL   : https://patchwork.freedesktop.org/series/54960/

>> State : failure

>>

>> == Summary ==

>>

>> CI Bug Log - changes from IGT_4758 -> IGTPW_2212

>> ====================================================

>>

>> Summary

>> -------

>>

>>    **FAILURE**

>>

>>    Serious unknown changes coming with IGTPW_2212 absolutely need to be

>>    verified manually.

>>    

>>    If you think the reported changes have nothing to do with the changes

>>    introduced in IGTPW_2212, please notify your bug team to allow them

>>    to document this new failure mode, which will reduce false positives in CI.

>>

>>    External URL: https://patchwork.freedesktop.org/api/1.0/series/54960/revisions/2/

>>

>> Possible new issues

>> -------------------

>>

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

>>

>> ### IGT changes ###

>>

>> #### Possible regressions ####

>>

>>    * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:

>>      - fi-pnv-d510:        PASS -> FAIL +19

>>

>>    * igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:

>>      - fi-blb-e6850:       PASS -> FAIL +18

>>

>>    * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:

>>      - fi-elk-e7500:       PASS -> FAIL +19

>>

>>    * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:

>>      - fi-gdg-551:         PASS -> FAIL +19

>>

>>    * igt@kms_pipe_crc_basic@read-crc-pipe-a:

>>      - fi-bwr-2160:        PASS -> FAIL +19

>>

> 

> 

> Just about all of these failures are this:

> 

> 

> (kms_cursor_legacy:2598) igt_kms-DEBUG: display: commit {

> (kms_cursor_legacy:2598) igt_kms-CRITICAL: Test assertion failure function igt_pipe_commit, file ../lib/igt_kms.c:2780:

> (kms_cursor_legacy:2598) igt_kms-CRITICAL: Failed assertion: pipe->props[i]

> (kms_cursor_legacy:2598) igt_core-INFO: Stack trace:

> (kms_cursor_legacy:2598) igt_core-INFO:   #0 ../lib/igt_core.c:1472 __igt_fail_assert()

> (kms_cursor_legacy:2598) igt_core-INFO:   #1 ../lib/igt_kms.c:2782 igt_pipe_commit()

> (kms_cursor_legacy:2598) igt_core-INFO:   #2 ../lib/igt_kms.c:3313 do_display_commit()

> (kms_cursor_legacy:2598) igt_core-INFO:   #3 ../lib/igt_kms.c:3428 igt_display_commit2()

> (kms_cursor_legacy:2598) igt_core-INFO:   #4 ../tests/kms_cursor_legacy.c:522 basic_flip_cursor()

> (kms_cursor_legacy:2598) igt_core-INFO:   #5 ../tests/kms_cursor_legacy.c:1491 __real_main1358()

> (kms_cursor_legacy:2598) igt_core-INFO:   #6 ../tests/kms_cursor_legacy.c:1358 main()

> (kms_cursor_legacy:2598) igt_core-INFO:   #7 ../csu/libc-start.c:344 __libc_start_main()

> (kms_cursor_legacy:2598) igt_core-INFO:   #8 [_start+0x2a]

> 


I think that's expected.

The vrr_enabled property is a default one in the kernel for 5.0+ but 
doesn't exist before that. Since it's default and could affect other 
test results if left enabled from another test I chose to add it to the 
list of default properties being reset. So it was probably missing the 
property in those tests and failing.

I don't know which version of the kernel was running the test but I 
don't think it was 5.0 at least (considering the date it ran).

Nicholas Kazlauskas
HI, 

> -----Original Message-----

> From: igt-dev [mailto:igt-dev-bounces@lists.freedesktop.org] On Behalf Of

> Kazlauskas, Nicholas

> Sent: perjantai 25. tammikuuta 2019 17.21

> To: igt-dev@lists.freedesktop.org; Latvala, Petri <petri.latvala@intel.com>

> Subject: Re: [igt-dev] ✗ Fi.CI.BAT: failure for tests: Add variable refresh rate tests

> (rev2)

> 

> On 1/25/19 4:57 AM, Petri Latvala wrote:

> > On Thu, Jan 10, 2019 at 11:09:09AM +0000, Patchwork wrote:

> >> == Series Details ==

> >>

> >> Series: tests: Add variable refresh rate tests (rev2)

> >> URL   : https://patchwork.freedesktop.org/series/54960/

> >> State : failure

> >>

> >> == Summary ==

> >>

> >> CI Bug Log - changes from IGT_4758 -> IGTPW_2212

> >> ====================================================

> >>

> >> Summary

> >> -------

> >>

> >>    **FAILURE**

> >>

> >>    Serious unknown changes coming with IGTPW_2212 absolutely need to be

> >>    verified manually.

> >>

> >>    If you think the reported changes have nothing to do with the changes

> >>    introduced in IGTPW_2212, please notify your bug team to allow them

> >>    to document this new failure mode, which will reduce false positives in CI.

> >>

> >>    External URL:

> >> https://patchwork.freedesktop.org/api/1.0/series/54960/revisions/2/

> >>

> >> Possible new issues

> >> -------------------

> >>

> >>    Here are the unknown changes that may have been introduced in

> IGTPW_2212:

> >>

> >> ### IGT changes ###

> >>

> >> #### Possible regressions ####

> >>

> >>    * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:

> >>      - fi-pnv-d510:        PASS -> FAIL +19

> >>

> >>    * igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:

> >>      - fi-blb-e6850:       PASS -> FAIL +18

> >>

> >>    * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:

> >>      - fi-elk-e7500:       PASS -> FAIL +19

> >>

> >>    * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:

> >>      - fi-gdg-551:         PASS -> FAIL +19

> >>

> >>    * igt@kms_pipe_crc_basic@read-crc-pipe-a:

> >>      - fi-bwr-2160:        PASS -> FAIL +19

> >>

> >

> >

> > Just about all of these failures are this:

> >

> >

> > (kms_cursor_legacy:2598) igt_kms-DEBUG: display: commit {

> > (kms_cursor_legacy:2598) igt_kms-CRITICAL: Test assertion failure function

> igt_pipe_commit, file ../lib/igt_kms.c:2780:

> > (kms_cursor_legacy:2598) igt_kms-CRITICAL: Failed assertion:

> > pipe->props[i]

> > (kms_cursor_legacy:2598) igt_core-INFO: Stack trace:

> > (kms_cursor_legacy:2598) igt_core-INFO:   #0 ../lib/igt_core.c:1472

> __igt_fail_assert()

> > (kms_cursor_legacy:2598) igt_core-INFO:   #1 ../lib/igt_kms.c:2782

> igt_pipe_commit()

> > (kms_cursor_legacy:2598) igt_core-INFO:   #2 ../lib/igt_kms.c:3313

> do_display_commit()

> > (kms_cursor_legacy:2598) igt_core-INFO:   #3 ../lib/igt_kms.c:3428

> igt_display_commit2()

> > (kms_cursor_legacy:2598) igt_core-INFO:   #4 ../tests/kms_cursor_legacy.c:522

> basic_flip_cursor()

> > (kms_cursor_legacy:2598) igt_core-INFO:   #5

> ../tests/kms_cursor_legacy.c:1491 __real_main1358()

> > (kms_cursor_legacy:2598) igt_core-INFO:   #6

> ../tests/kms_cursor_legacy.c:1358 main()

> > (kms_cursor_legacy:2598) igt_core-INFO:   #7 ../csu/libc-start.c:344

> __libc_start_main()

> > (kms_cursor_legacy:2598) igt_core-INFO:   #8 [_start+0x2a]

> >

> 

> I think that's expected.

> 

> The vrr_enabled property is a default one in the kernel for 5.0+ but doesn't exist

> before that. Since it's default and could affect other test results if left enabled

> from another test I chose to add it to the list of default properties being reset. So

> it was probably missing the property in those tests and failing.

> 

> I don't know which version of the kernel was running the test but I don't think it

> was 5.0 at least (considering the date it ran).

Well...
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2212/fi-elk-e7500/boot0.log
Linux version 5.0.0-rc1-CI-CI_DRM_5390+


> 

> Nicholas Kazlauskas

> _______________________________________________

> igt-dev mailing list

> igt-dev@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/igt-dev
On 1/25/19 11:43 AM, Saarinen, Jani wrote:
> HI,

> 

>> -----Original Message-----

>> From: igt-dev [mailto:igt-dev-bounces@lists.freedesktop.org] On Behalf Of

>> Kazlauskas, Nicholas

>> Sent: perjantai 25. tammikuuta 2019 17.21

>> To: igt-dev@lists.freedesktop.org; Latvala, Petri <petri.latvala@intel.com>

>> Subject: Re: [igt-dev] ✗ Fi.CI.BAT: failure for tests: Add variable refresh rate tests

>> (rev2)

>>

>> On 1/25/19 4:57 AM, Petri Latvala wrote:

>>> On Thu, Jan 10, 2019 at 11:09:09AM +0000, Patchwork wrote:

>>>> == Series Details ==

>>>>

>>>> Series: tests: Add variable refresh rate tests (rev2)

>>>> URL   : https://patchwork.freedesktop.org/series/54960/

>>>> State : failure

>>>>

>>>> == Summary ==

>>>>

>>>> CI Bug Log - changes from IGT_4758 -> IGTPW_2212

>>>> ====================================================

>>>>

>>>> Summary

>>>> -------

>>>>

>>>>     **FAILURE**

>>>>

>>>>     Serious unknown changes coming with IGTPW_2212 absolutely need to be

>>>>     verified manually.

>>>>

>>>>     If you think the reported changes have nothing to do with the changes

>>>>     introduced in IGTPW_2212, please notify your bug team to allow them

>>>>     to document this new failure mode, which will reduce false positives in CI.

>>>>

>>>>     External URL:

>>>> https://patchwork.freedesktop.org/api/1.0/series/54960/revisions/2/

>>>>

>>>> Possible new issues

>>>> -------------------

>>>>

>>>>     Here are the unknown changes that may have been introduced in

>> IGTPW_2212:

>>>>

>>>> ### IGT changes ###

>>>>

>>>> #### Possible regressions ####

>>>>

>>>>     * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:

>>>>       - fi-pnv-d510:        PASS -> FAIL +19

>>>>

>>>>     * igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:

>>>>       - fi-blb-e6850:       PASS -> FAIL +18

>>>>

>>>>     * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:

>>>>       - fi-elk-e7500:       PASS -> FAIL +19

>>>>

>>>>     * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:

>>>>       - fi-gdg-551:         PASS -> FAIL +19

>>>>

>>>>     * igt@kms_pipe_crc_basic@read-crc-pipe-a:

>>>>       - fi-bwr-2160:        PASS -> FAIL +19

>>>>

>>>

>>>

>>> Just about all of these failures are this:

>>>

>>>

>>> (kms_cursor_legacy:2598) igt_kms-DEBUG: display: commit {

>>> (kms_cursor_legacy:2598) igt_kms-CRITICAL: Test assertion failure function

>> igt_pipe_commit, file ../lib/igt_kms.c:2780:

>>> (kms_cursor_legacy:2598) igt_kms-CRITICAL: Failed assertion:

>>> pipe->props[i]

>>> (kms_cursor_legacy:2598) igt_core-INFO: Stack trace:

>>> (kms_cursor_legacy:2598) igt_core-INFO:   #0 ../lib/igt_core.c:1472

>> __igt_fail_assert()

>>> (kms_cursor_legacy:2598) igt_core-INFO:   #1 ../lib/igt_kms.c:2782

>> igt_pipe_commit()

>>> (kms_cursor_legacy:2598) igt_core-INFO:   #2 ../lib/igt_kms.c:3313

>> do_display_commit()

>>> (kms_cursor_legacy:2598) igt_core-INFO:   #3 ../lib/igt_kms.c:3428

>> igt_display_commit2()

>>> (kms_cursor_legacy:2598) igt_core-INFO:   #4 ../tests/kms_cursor_legacy.c:522

>> basic_flip_cursor()

>>> (kms_cursor_legacy:2598) igt_core-INFO:   #5

>> ../tests/kms_cursor_legacy.c:1491 __real_main1358()

>>> (kms_cursor_legacy:2598) igt_core-INFO:   #6

>> ../tests/kms_cursor_legacy.c:1358 main()

>>> (kms_cursor_legacy:2598) igt_core-INFO:   #7 ../csu/libc-start.c:344

>> __libc_start_main()

>>> (kms_cursor_legacy:2598) igt_core-INFO:   #8 [_start+0x2a]

>>>

>>

>> I think that's expected.

>>

>> The vrr_enabled property is a default one in the kernel for 5.0+ but doesn't exist

>> before that. Since it's default and could affect other test results if left enabled

>> from another test I chose to add it to the list of default properties being reset. So

>> it was probably missing the property in those tests and failing.

>>

>> I don't know which version of the kernel was running the test but I don't think it

>> was 5.0 at least (considering the date it ran).

> Well...

> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2212/fi-elk-e7500/boot0.log

> Linux version 5.0.0-rc1-CI-CI_DRM_5390+


Oh, thanks! I didn't know there was an easy way to view the actual dmesg 
log.

However, that assertion is definitely indicating that one of the pipe 
properties (likely vrr_enabled) doesn't have a valid prop_id (likely 
because it wasn't found).

FWIW I don't see these failures locally when testing with the patch in 
the tree, but I'm also still on a 4.20 tree with the VRR patches 
applied. I'm pretty sure the VRR patches were targeting 5.0 at least so 
those patches should be in...

Nicholas Kazlauskas

> 

> 

>>

>> Nicholas Kazlauskas

>> _______________________________________________

>> igt-dev mailing list

>> igt-dev@lists.freedesktop.org

>> https://lists.freedesktop.org/mailman/listinfo/igt-dev
On Fri, Jan 25, 2019 at 05:21:41PM +0000, Kazlauskas, Nicholas wrote:
> On 1/25/19 11:43 AM, Saarinen, Jani wrote:
> > HI,
> > 
> >> -----Original Message-----
> >> From: igt-dev [mailto:igt-dev-bounces@lists.freedesktop.org] On Behalf Of
> >> Kazlauskas, Nicholas
> >> Sent: perjantai 25. tammikuuta 2019 17.21
> >> To: igt-dev@lists.freedesktop.org; Latvala, Petri <petri.latvala@intel.com>
> >> Subject: Re: [igt-dev] ✗ Fi.CI.BAT: failure for tests: Add variable refresh rate tests
> >> (rev2)
> >>
> >> On 1/25/19 4:57 AM, Petri Latvala wrote:
> >>> On Thu, Jan 10, 2019 at 11:09:09AM +0000, Patchwork wrote:
> >>>> == Series Details ==
> >>>>
> >>>> Series: tests: Add variable refresh rate tests (rev2)
> >>>> URL   : https://patchwork.freedesktop.org/series/54960/
> >>>> State : failure
> >>>>
> >>>> == Summary ==
> >>>>
> >>>> CI Bug Log - changes from IGT_4758 -> IGTPW_2212
> >>>> ====================================================
> >>>>
> >>>> Summary
> >>>> -------
> >>>>
> >>>>     **FAILURE**
> >>>>
> >>>>     Serious unknown changes coming with IGTPW_2212 absolutely need to be
> >>>>     verified manually.
> >>>>
> >>>>     If you think the reported changes have nothing to do with the changes
> >>>>     introduced in IGTPW_2212, please notify your bug team to allow them
> >>>>     to document this new failure mode, which will reduce false positives in CI.
> >>>>
> >>>>     External URL:
> >>>> https://patchwork.freedesktop.org/api/1.0/series/54960/revisions/2/
> >>>>
> >>>> Possible new issues
> >>>> -------------------
> >>>>
> >>>>     Here are the unknown changes that may have been introduced in
> >> IGTPW_2212:
> >>>>
> >>>> ### IGT changes ###
> >>>>
> >>>> #### Possible regressions ####
> >>>>
> >>>>     * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
> >>>>       - fi-pnv-d510:        PASS -> FAIL +19
> >>>>
> >>>>     * igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
> >>>>       - fi-blb-e6850:       PASS -> FAIL +18
> >>>>
> >>>>     * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
> >>>>       - fi-elk-e7500:       PASS -> FAIL +19
> >>>>
> >>>>     * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
> >>>>       - fi-gdg-551:         PASS -> FAIL +19
> >>>>
> >>>>     * igt@kms_pipe_crc_basic@read-crc-pipe-a:
> >>>>       - fi-bwr-2160:        PASS -> FAIL +19
> >>>>
> >>>
> >>>
> >>> Just about all of these failures are this:
> >>>
> >>>
> >>> (kms_cursor_legacy:2598) igt_kms-DEBUG: display: commit {
> >>> (kms_cursor_legacy:2598) igt_kms-CRITICAL: Test assertion failure function
> >> igt_pipe_commit, file ../lib/igt_kms.c:2780:
> >>> (kms_cursor_legacy:2598) igt_kms-CRITICAL: Failed assertion:
> >>> pipe->props[i]
> >>> (kms_cursor_legacy:2598) igt_core-INFO: Stack trace:
> >>> (kms_cursor_legacy:2598) igt_core-INFO:   #0 ../lib/igt_core.c:1472
> >> __igt_fail_assert()
> >>> (kms_cursor_legacy:2598) igt_core-INFO:   #1 ../lib/igt_kms.c:2782
> >> igt_pipe_commit()
> >>> (kms_cursor_legacy:2598) igt_core-INFO:   #2 ../lib/igt_kms.c:3313
> >> do_display_commit()
> >>> (kms_cursor_legacy:2598) igt_core-INFO:   #3 ../lib/igt_kms.c:3428
> >> igt_display_commit2()
> >>> (kms_cursor_legacy:2598) igt_core-INFO:   #4 ../tests/kms_cursor_legacy.c:522
> >> basic_flip_cursor()
> >>> (kms_cursor_legacy:2598) igt_core-INFO:   #5
> >> ../tests/kms_cursor_legacy.c:1491 __real_main1358()
> >>> (kms_cursor_legacy:2598) igt_core-INFO:   #6
> >> ../tests/kms_cursor_legacy.c:1358 main()
> >>> (kms_cursor_legacy:2598) igt_core-INFO:   #7 ../csu/libc-start.c:344
> >> __libc_start_main()
> >>> (kms_cursor_legacy:2598) igt_core-INFO:   #8 [_start+0x2a]
> >>>
> >>
> >> I think that's expected.
> >>
> >> The vrr_enabled property is a default one in the kernel for 5.0+ but doesn't exist
> >> before that. Since it's default and could affect other test results if left enabled
> >> from another test I chose to add it to the list of default properties being reset. So
> >> it was probably missing the property in those tests and failing.
> >>
> >> I don't know which version of the kernel was running the test but I don't think it
> >> was 5.0 at least (considering the date it ran).
> > Well...
> > https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2212/fi-elk-e7500/boot0.log
> > Linux version 5.0.0-rc1-CI-CI_DRM_5390+
> 
> Oh, thanks! I didn't know there was an easy way to view the actual dmesg 
> log.
> 
> However, that assertion is definitely indicating that one of the pipe 
> properties (likely vrr_enabled) doesn't have a valid prop_id (likely 
> because it wasn't found).
> 
> FWIW I don't see these failures locally when testing with the patch in 
> the tree, but I'm also still on a 4.20 tree with the VRR patches 
> applied. I'm pretty sure the VRR patches were targeting 5.0 at least so 
> those patches should be in...


All the machines with the fails were old and for gen < 5 we don't have
DRIVER_ATOMIC. vrr_enabled is an atomic-only prop?

We support testing old kernels as well so some effort needs to be put
towards that anyway. Clearing the vrr prop when resetting to default
is good, but sprinkle some igt_*_has_prop() around it.
On 1/28/19 5:41 AM, Petri Latvala wrote:
> On Fri, Jan 25, 2019 at 05:21:41PM +0000, Kazlauskas, Nicholas wrote:

>> On 1/25/19 11:43 AM, Saarinen, Jani wrote:

>>> HI,

>>>

>>>> -----Original Message-----

>>>> From: igt-dev [mailto:igt-dev-bounces@lists.freedesktop.org] On Behalf Of

>>>> Kazlauskas, Nicholas

>>>> Sent: perjantai 25. tammikuuta 2019 17.21

>>>> To: igt-dev@lists.freedesktop.org; Latvala, Petri <petri.latvala@intel.com>

>>>> Subject: Re: [igt-dev] ✗ Fi.CI.BAT: failure for tests: Add variable refresh rate tests

>>>> (rev2)

>>>>

>>>> On 1/25/19 4:57 AM, Petri Latvala wrote:

>>>>> On Thu, Jan 10, 2019 at 11:09:09AM +0000, Patchwork wrote:

>>>>>> == Series Details ==

>>>>>>

>>>>>> Series: tests: Add variable refresh rate tests (rev2)

>>>>>> URL   : https://patchwork.freedesktop.org/series/54960/

>>>>>> State : failure

>>>>>>

>>>>>> == Summary ==

>>>>>>

>>>>>> CI Bug Log - changes from IGT_4758 -> IGTPW_2212

>>>>>> ====================================================

>>>>>>

>>>>>> Summary

>>>>>> -------

>>>>>>

>>>>>>      **FAILURE**

>>>>>>

>>>>>>      Serious unknown changes coming with IGTPW_2212 absolutely need to be

>>>>>>      verified manually.

>>>>>>

>>>>>>      If you think the reported changes have nothing to do with the changes

>>>>>>      introduced in IGTPW_2212, please notify your bug team to allow them

>>>>>>      to document this new failure mode, which will reduce false positives in CI.

>>>>>>

>>>>>>      External URL:

>>>>>> https://patchwork.freedesktop.org/api/1.0/series/54960/revisions/2/

>>>>>>

>>>>>> Possible new issues

>>>>>> -------------------

>>>>>>

>>>>>>      Here are the unknown changes that may have been introduced in

>>>> IGTPW_2212:

>>>>>>

>>>>>> ### IGT changes ###

>>>>>>

>>>>>> #### Possible regressions ####

>>>>>>

>>>>>>      * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:

>>>>>>        - fi-pnv-d510:        PASS -> FAIL +19

>>>>>>

>>>>>>      * igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:

>>>>>>        - fi-blb-e6850:       PASS -> FAIL +18

>>>>>>

>>>>>>      * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:

>>>>>>        - fi-elk-e7500:       PASS -> FAIL +19

>>>>>>

>>>>>>      * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:

>>>>>>        - fi-gdg-551:         PASS -> FAIL +19

>>>>>>

>>>>>>      * igt@kms_pipe_crc_basic@read-crc-pipe-a:

>>>>>>        - fi-bwr-2160:        PASS -> FAIL +19

>>>>>>

>>>>>

>>>>>

>>>>> Just about all of these failures are this:

>>>>>

>>>>>

>>>>> (kms_cursor_legacy:2598) igt_kms-DEBUG: display: commit {

>>>>> (kms_cursor_legacy:2598) igt_kms-CRITICAL: Test assertion failure function

>>>> igt_pipe_commit, file ../lib/igt_kms.c:2780:

>>>>> (kms_cursor_legacy:2598) igt_kms-CRITICAL: Failed assertion:

>>>>> pipe->props[i]

>>>>> (kms_cursor_legacy:2598) igt_core-INFO: Stack trace:

>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #0 ../lib/igt_core.c:1472

>>>> __igt_fail_assert()

>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #1 ../lib/igt_kms.c:2782

>>>> igt_pipe_commit()

>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #2 ../lib/igt_kms.c:3313

>>>> do_display_commit()

>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #3 ../lib/igt_kms.c:3428

>>>> igt_display_commit2()

>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #4 ../tests/kms_cursor_legacy.c:522

>>>> basic_flip_cursor()

>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #5

>>>> ../tests/kms_cursor_legacy.c:1491 __real_main1358()

>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #6

>>>> ../tests/kms_cursor_legacy.c:1358 main()

>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #7 ../csu/libc-start.c:344

>>>> __libc_start_main()

>>>>> (kms_cursor_legacy:2598) igt_core-INFO:   #8 [_start+0x2a]

>>>>>

>>>>

>>>> I think that's expected.

>>>>

>>>> The vrr_enabled property is a default one in the kernel for 5.0+ but doesn't exist

>>>> before that. Since it's default and could affect other test results if left enabled

>>>> from another test I chose to add it to the list of default properties being reset. So

>>>> it was probably missing the property in those tests and failing.

>>>>

>>>> I don't know which version of the kernel was running the test but I don't think it

>>>> was 5.0 at least (considering the date it ran).

>>> Well...

>>> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2212/fi-elk-e7500/boot0.log

>>> Linux version 5.0.0-rc1-CI-CI_DRM_5390+

>>

>> Oh, thanks! I didn't know there was an easy way to view the actual dmesg

>> log.

>>

>> However, that assertion is definitely indicating that one of the pipe

>> properties (likely vrr_enabled) doesn't have a valid prop_id (likely

>> because it wasn't found).

>>

>> FWIW I don't see these failures locally when testing with the patch in

>> the tree, but I'm also still on a 4.20 tree with the VRR patches

>> applied. I'm pretty sure the VRR patches were targeting 5.0 at least so

>> those patches should be in...

> 

> 

> All the machines with the fails were old and for gen < 5 we don't have

> DRIVER_ATOMIC. vrr_enabled is an atomic-only prop?

> 

> We support testing old kernels as well so some effort needs to be put

> towards that anyway. Clearing the vrr prop when resetting to default

> is good, but sprinkle some igt_*_has_prop() around it.

> 

> 


That sounds like what's happening here - the property is only ever 
attached for atomic drivers. That check should be sufficient in solving 
the issue, thanks for the help!

Nicholas Kazlauskas