[igt-dev] [PATCH i-g-t v2] tests: Add variable refresh rate tests

[igt-dev] [PATCH i-g-t v2] tests: Add variable refresh rate tests

[Nicholas Kazlauskas]

There are 3 tests for basic variable refresh rate functionality.

The tests measure flipping at the average between the current mode
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 519eac79..7df2bca4 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -89,6 +89,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 e14ab2b4..fb757386 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -60,6 +60,7 @@ test_progs = [
 	'kms_tv_load_detect',
 	'kms_universal_plane',
 	'kms_vblank',
+	'kms_vrr',
 	'meta_test',
 	'perf',
 	'pm_backlight',
-- 
2.17.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2] tests: Add variable refresh rate tests

[Daniel Vetter]

> +/* 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.
> +        */

Uh, if this is also how your amdgpu userspace works we've just fucked
up the uapi for good :-/

FLIP_ASYNC = please tear

VRR = please don't strictly obey the vrefresh, but very much dont tear

Tying them together means we're deeply mixing things up.

Also amdgpu is still using it's own flip implementation, which makes
me wonder whether VRR would even work with atomic, or whether that's
also butchered ...

How I thought this stuff was supposed to work:

- VRR_ENABLED controls whether we do VRR
- since atomic is awesome you can change that on every frame
- VRR has nothing to do with ASYNC

So a) do I read this correctly b) how do we get out of this hole (and
maybe c) amdgpu really needs to remove
amdgpu_display_crtc_page_flip_target asap).

Manasi pointed this out to me, so adding a few more people here.
-Daniel

> +       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;
> +}
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2] tests: Add variable refresh rate tests

[Kazlauskas, Nicholas]

On 2020-04-14 6:52 a.m., Daniel Vetter wrote:
>> +/* 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.
>> +        */
> 
> Uh, if this is also how your amdgpu userspace works we've just fucked
> up the uapi for good :-/
> 
> FLIP_ASYNC = please tear
> 
> VRR = please don't strictly obey the vrefresh, but very much dont tear
> 
> Tying them together means we're deeply mixing things up.
> 
> Also amdgpu is still using it's own flip implementation, which makes
> me wonder whether VRR would even work with atomic, or whether that's
> also butchered ...
> 
> How I thought this stuff was supposed to work:
> 
> - VRR_ENABLED controls whether we do VRR
> - since atomic is awesome you can change that on every frame
> - VRR has nothing to do with ASYNC
> 
> So a) do I read this correctly b) how do we get out of this hole (and
> maybe c) amdgpu really needs to remove
> amdgpu_display_crtc_page_flip_target asap).
> 
> Manasi pointed this out to me, so adding a few more people here.
> -Daniel

Adaptive sync is an extended front porch that ends upon some signaling 
from the driver is almost always tied to a page flip.

FLIP_ASYNC is a mechanism to flip immediately without waiting for any 
double buffering from the driver or hardware, potentially causing tearing.

This test uses FLIP_ASYNC to precisely position when the flip occurs 
from userspace within the VBLANK to end the frame at the midpoint 
between the min and max range.

But it doesn't really need to be FLIP_ASYNC to make this work, explicit 
fencing is probably the better solution here for all drivers.

This test only really worked by accident for testing amdgpu. We have 
internal stalls in the driver to prevent an immediate flip from 
occurring more than once per frame, so if you try to flip in the 
extended front porch you end up hitting that stall and the returned 
timestamp is a frame ahead.

The other part of this test that's sort of an issue is that this is 
effectively a measurement for how fast you can immediate flip - which is 
good for catching performance regressions in the driver, and good for 
consistent VRR behavior, but not exactly related to the test.

The way FLIP_ASYNC should probably work in amdgpu is:

1) No stalls
2) Return -EBUSY if we're in the middle of a frame already, or even 
allow the commit to happen in the same frame since the hardware supports it

Not sure how userspace is equipped to handle that though.

Regards,
Nicholas Kazlauskas

> 
>> +       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;
>> +}

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2] tests: Add variable refresh rate tests

[Daniel Vetter]

On Tue, Apr 14, 2020 at 3:00 PM Kazlauskas, Nicholas
<nicholas.kazlauskas@amd.com> wrote:
>
> On 2020-04-14 6:52 a.m., Daniel Vetter wrote:
> >> +/* 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.
> >> +        */
> >
> > Uh, if this is also how your amdgpu userspace works we've just fucked
> > up the uapi for good :-/
> >
> > FLIP_ASYNC = please tear
> >
> > VRR = please don't strictly obey the vrefresh, but very much dont tear
> >
> > Tying them together means we're deeply mixing things up.
> >
> > Also amdgpu is still using it's own flip implementation, which makes
> > me wonder whether VRR would even work with atomic, or whether that's
> > also butchered ...
> >
> > How I thought this stuff was supposed to work:
> >
> > - VRR_ENABLED controls whether we do VRR
> > - since atomic is awesome you can change that on every frame
> > - VRR has nothing to do with ASYNC
> >
> > So a) do I read this correctly b) how do we get out of this hole (and
> > maybe c) amdgpu really needs to remove
> > amdgpu_display_crtc_page_flip_target asap).
> >
> > Manasi pointed this out to me, so adding a few more people here.
> > -Daniel
>
> Adaptive sync is an extended front porch that ends upon some signaling
> from the driver is almost always tied to a page flip.
>
> FLIP_ASYNC is a mechanism to flip immediately without waiting for any
> double buffering from the driver or hardware, potentially causing tearing.
>
> This test uses FLIP_ASYNC to precisely position when the flip occurs
> from userspace within the VBLANK to end the frame at the midpoint
> between the min and max range.
>
> But it doesn't really need to be FLIP_ASYNC to make this work, explicit
> fencing is probably the better solution here for all drivers.

Ok, I freaked out for a bit there, and reading amdgpu DC code didn't
really help with convincing me it's not needed - you're still using
your own page_flip_target handler instead of the one in the helpers
(written by amd people even, and for a few features before VRR), so
biggest worry I had was that VRR doesn't work without FLIP_ASYNC set.
I also looked at -amdgpu code, but I couldn't figure this out one way
or another there either (since that's not using atomic either).

> This test only really worked by accident for testing amdgpu. We have
> internal stalls in the driver to prevent an immediate flip from
> occurring more than once per frame, so if you try to flip in the
> extended front porch you end up hitting that stall and the returned
> timestamp is a frame ahead.
>
> The other part of this test that's sort of an issue is that this is
> effectively a measurement for how fast you can immediate flip - which is
> good for catching performance regressions in the driver, and good for
> consistent VRR behavior, but not exactly related to the test.

Hm yeah I think that's another reason for why this test should maybe
use atomic with vgem dma_fences that we signal exactly when needed.
Probably should be a 2nd subtest, so that we can keep the other one
working.

Plus update the comment on why exactly FLIP_ASYNC is needed with your
story above. Btw instead of FLIP_ASYNC could we use the target frame
number, set to the next frame? That should also allow us to do an
immediate flip while in the vblank.

Also I think we should have a normal flip which is somewhat late-ish
in the vblank, and make sure VRR can still hit that frame. If that
doesn't exist yet. I think with that set of tests we should be able to
exercise all the timings we want for VRR?

> The way FLIP_ASYNC should probably work in amdgpu is:
>
> 1) No stalls
> 2) Return -EBUSY if we're in the middle of a frame already, or even
> allow the commit to happen in the same frame since the hardware supports it
>
> Not sure how userspace is equipped to handle that though.

I think aside from -amdgpu there's 0 userspace out there using
FLIP_ASYNC. Plus it's not standardized for atomic drivers, so really
the semantics are super wobbly. I'm not worried about FLIP_ASYNC
itself (not the first piece of kms uapi that might be fun to unify
across drivers), the only thing that worried me is tying VRR into
FLIP_ASYNC. Looks I worried without reasons since it's just an
igt/test hack that we can change.

Thanks for your explanations and all.

Cheers, Daniel

>
> Regards,
> Nicholas Kazlauskas
>
> >
> >> +       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;
> >> +}
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2] tests: Add variable refresh rate tests

[Harry Wentland]

On 2020-04-14 2:50 p.m., Daniel Vetter wrote:
> On Tue, Apr 14, 2020 at 3:00 PM Kazlauskas, Nicholas
> <nicholas.kazlauskas@amd.com> wrote:
>>
>> On 2020-04-14 6:52 a.m., Daniel Vetter wrote:
>>>> +/* 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.
>>>> +        */
>>>
>>> Uh, if this is also how your amdgpu userspace works we've just fucked
>>> up the uapi for good :-/
>>>
>>> FLIP_ASYNC = please tear
>>>
>>> VRR = please don't strictly obey the vrefresh, but very much dont tear
>>>
>>> Tying them together means we're deeply mixing things up.
>>>
>>> Also amdgpu is still using it's own flip implementation, which makes
>>> me wonder whether VRR would even work with atomic, or whether that's
>>> also butchered ...
>>>
>>> How I thought this stuff was supposed to work:
>>>
>>> - VRR_ENABLED controls whether we do VRR
>>> - since atomic is awesome you can change that on every frame
>>> - VRR has nothing to do with ASYNC
>>>
>>> So a) do I read this correctly b) how do we get out of this hole (and
>>> maybe c) amdgpu really needs to remove
>>> amdgpu_display_crtc_page_flip_target asap).
>>>
>>> Manasi pointed this out to me, so adding a few more people here.
>>> -Daniel
>>
>> Adaptive sync is an extended front porch that ends upon some signaling
>> from the driver is almost always tied to a page flip.
>>
>> FLIP_ASYNC is a mechanism to flip immediately without waiting for any
>> double buffering from the driver or hardware, potentially causing tearing.
>>
>> This test uses FLIP_ASYNC to precisely position when the flip occurs
>> from userspace within the VBLANK to end the frame at the midpoint
>> between the min and max range.
>>
>> But it doesn't really need to be FLIP_ASYNC to make this work, explicit
>> fencing is probably the better solution here for all drivers.
> 
> Ok, I freaked out for a bit there, and reading amdgpu DC code didn't
> really help with convincing me it's not needed - you're still using
> your own page_flip_target handler instead of the one in the helpers
> (written by amd people even, and for a few features before VRR), so
> biggest worry I had was that VRR doesn't work without FLIP_ASYNC set.
> I also looked at -amdgpu code, but I couldn't figure this out one way
> or another there either (since that's not using atomic either).
> 
>> This test only really worked by accident for testing amdgpu. We have
>> internal stalls in the driver to prevent an immediate flip from
>> occurring more than once per frame, so if you try to flip in the
>> extended front porch you end up hitting that stall and the returned
>> timestamp is a frame ahead.
>>
>> The other part of this test that's sort of an issue is that this is
>> effectively a measurement for how fast you can immediate flip - which is
>> good for catching performance regressions in the driver, and good for
>> consistent VRR behavior, but not exactly related to the test.
> 
> Hm yeah I think that's another reason for why this test should maybe
> use atomic with vgem dma_fences that we signal exactly when needed.
> Probably should be a 2nd subtest, so that we can keep the other one
> working.
> 
> Plus update the comment on why exactly FLIP_ASYNC is needed with your
> story above. Btw instead of FLIP_ASYNC could we use the target frame
> number, set to the next frame? That should also allow us to do an
> immediate flip while in the vblank.
> 
> Also I think we should have a normal flip which is somewhat late-ish
> in the vblank, and make sure VRR can still hit that frame. If that
> doesn't exist yet. I think with that set of tests we should be able to
> exercise all the timings we want for VRR?
> 
>> The way FLIP_ASYNC should probably work in amdgpu is:
>>
>> 1) No stalls
>> 2) Return -EBUSY if we're in the middle of a frame already, or even
>> allow the commit to happen in the same frame since the hardware supports it
>>
>> Not sure how userspace is equipped to handle that though.
> 
> I think aside from -amdgpu there's 0 userspace out there using
> FLIP_ASYNC. Plus it's not standardized for atomic drivers, so really
> the semantics are super wobbly. I'm not worried about FLIP_ASYNC
> itself (not the first piece of kms uapi that might be fun to unify
> across drivers), the only thing that worried me is tying VRR into
> FLIP_ASYNC. Looks I worried without reasons since it's just an
> igt/test hack that we can change.
> 

Thanks, Nick, and Daniel, for hashing this out a bit.

I still don't fully understand flip semantics in DRM or where they're
defined.

One thing that is problematic for some use-cases, and I'm not sure if
this is unique to amdgpu, is that a normal atomic flip (not ASYNC) will
wait until HW latches (starts scanning out) the new address before it
returns. This will stall the renderer.

A better approach, and an approach that I understand Windows is taking,
is to return immediately from the driver call after programming the new
address. AMD HW at least will guarantee that this address is scanned out
only on the following frame and won't tear (unless an immediate flip is
explicitly requested). This will let usermode render and flip at a rate
it desires and ensures that only the flip programmed closest to the
start of scanout will be actually scanned out.

Cheers,
Harry

> Thanks for your explanations and all.
> 
> Cheers, Daniel
> 
>>
>> Regards,
>> Nicholas Kazlauskas
>>
>>>
>>>> +       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;
>>>> +}
>>
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2] tests: Add variable refresh rate tests

[Manasi Navare]

On Tue, Apr 14, 2020 at 08:50:29PM +0200, Daniel Vetter wrote:
> On Tue, Apr 14, 2020 at 3:00 PM Kazlauskas, Nicholas
> <nicholas.kazlauskas@amd.com> wrote:
> >
> > On 2020-04-14 6:52 a.m., Daniel Vetter wrote:
> > >> +/* 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.
> > >> +        */
> > >
> > > Uh, if this is also how your amdgpu userspace works we've just fucked
> > > up the uapi for good :-/
> > >
> > > FLIP_ASYNC = please tear
> > >
> > > VRR = please don't strictly obey the vrefresh, but very much dont tear
> > >
> > > Tying them together means we're deeply mixing things up.
> > >
> > > Also amdgpu is still using it's own flip implementation, which makes
> > > me wonder whether VRR would even work with atomic, or whether that's
> > > also butchered ...
> > >
> > > How I thought this stuff was supposed to work:
> > >
> > > - VRR_ENABLED controls whether we do VRR
> > > - since atomic is awesome you can change that on every frame
> > > - VRR has nothing to do with ASYNC
> > >
> > > So a) do I read this correctly b) how do we get out of this hole (and
> > > maybe c) amdgpu really needs to remove
> > > amdgpu_display_crtc_page_flip_target asap).
> > >
> > > Manasi pointed this out to me, so adding a few more people here.
> > > -Daniel
> >
> > Adaptive sync is an extended front porch that ends upon some signaling
> > from the driver is almost always tied to a page flip.
> >
> > FLIP_ASYNC is a mechanism to flip immediately without waiting for any
> > double buffering from the driver or hardware, potentially causing tearing.
> >
> > This test uses FLIP_ASYNC to precisely position when the flip occurs
> > from userspace within the VBLANK to end the frame at the midpoint
> > between the min and max range.
> >
> > But it doesn't really need to be FLIP_ASYNC to make this work, explicit
> > fencing is probably the better solution here for all drivers.
> 
> Ok, I freaked out for a bit there, and reading amdgpu DC code didn't
> really help with convincing me it's not needed - you're still using
> your own page_flip_target handler instead of the one in the helpers
> (written by amd people even, and for a few features before VRR), so
> biggest worry I had was that VRR doesn't work without FLIP_ASYNC set.
> I also looked at -amdgpu code, but I couldn't figure this out one way
> or another there either (since that's not using atomic either).
> 
> > This test only really worked by accident for testing amdgpu. We have
> > internal stalls in the driver to prevent an immediate flip from
> > occurring more than once per frame, so if you try to flip in the
> > extended front porch you end up hitting that stall and the returned
> > timestamp is a frame ahead.
> >
> > The other part of this test that's sort of an issue is that this is
> > effectively a measurement for how fast you can immediate flip - which is
> > good for catching performance regressions in the driver, and good for
> > consistent VRR behavior, but not exactly related to the test.
> 
> Hm yeah I think that's another reason for why this test should maybe
> use atomic with vgem dma_fences that we signal exactly when needed.
> Probably should be a 2nd subtest, so that we can keep the other one
> working.
> 
> Plus update the comment on why exactly FLIP_ASYNC is needed with your
> story above. Btw instead of FLIP_ASYNC could we use the target frame
> number, set to the next frame? That should also allow us to do an
> immediate flip while in the vblank.
> 
> Also I think we should have a normal flip which is somewhat late-ish
> in the vblank, and make sure VRR can still hit that frame. If that
> doesn't exist yet. I think with that set of tests we should be able to
> exercise all the timings we want for VRR?
>

Yes I think we can use the normal flip here.
And the way it really works on Intel HW is that, it has its own flip decision boundary
where if the flip request occurs in the Vactive of the previous frame then it waits
to flip unil the flip decision boundary hits which is vmin + the framestart_delay +
the pipeline fill delay and if the flip request occurs anywhere in between vmin and vmax
then the flip happens immediately thus terminating the vblank there.

Do we still need to use the vgem_dma_fences? Could you elaborate a little bit on
what signalling is expected to be achieved with the fences and how that can be used
in combination with the regular atomic commit and flip request and the page flip event captures?

Manasi
 
> > The way FLIP_ASYNC should probably work in amdgpu is:
> >
> > 1) No stalls
> > 2) Return -EBUSY if we're in the middle of a frame already, or even
> > allow the commit to happen in the same frame since the hardware supports it
> >
> > Not sure how userspace is equipped to handle that though.
> 
> I think aside from -amdgpu there's 0 userspace out there using
> FLIP_ASYNC. Plus it's not standardized for atomic drivers, so really
> the semantics are super wobbly. I'm not worried about FLIP_ASYNC
> itself (not the first piece of kms uapi that might be fun to unify
> across drivers), the only thing that worried me is tying VRR into
> FLIP_ASYNC. Looks I worried without reasons since it's just an
> igt/test hack that we can change.
> 
> Thanks for your explanations and all.
> 
> Cheers, Daniel
> 
> >
> > Regards,
> > Nicholas Kazlauskas
> >
> > >
> > >> +       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;
> > >> +}
> >
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2] tests: Add variable refresh rate tests

[Daniel Vetter]

On Tue, Apr 14, 2020 at 03:17:20PM -0400, Harry Wentland wrote:
> 
> 
> On 2020-04-14 2:50 p.m., Daniel Vetter wrote:
> > On Tue, Apr 14, 2020 at 3:00 PM Kazlauskas, Nicholas
> > <nicholas.kazlauskas@amd.com> wrote:
> >>
> >> On 2020-04-14 6:52 a.m., Daniel Vetter wrote:
> >>>> +/* 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.
> >>>> +        */
> >>>
> >>> Uh, if this is also how your amdgpu userspace works we've just fucked
> >>> up the uapi for good :-/
> >>>
> >>> FLIP_ASYNC = please tear
> >>>
> >>> VRR = please don't strictly obey the vrefresh, but very much dont tear
> >>>
> >>> Tying them together means we're deeply mixing things up.
> >>>
> >>> Also amdgpu is still using it's own flip implementation, which makes
> >>> me wonder whether VRR would even work with atomic, or whether that's
> >>> also butchered ...
> >>>
> >>> How I thought this stuff was supposed to work:
> >>>
> >>> - VRR_ENABLED controls whether we do VRR
> >>> - since atomic is awesome you can change that on every frame
> >>> - VRR has nothing to do with ASYNC
> >>>
> >>> So a) do I read this correctly b) how do we get out of this hole (and
> >>> maybe c) amdgpu really needs to remove
> >>> amdgpu_display_crtc_page_flip_target asap).
> >>>
> >>> Manasi pointed this out to me, so adding a few more people here.
> >>> -Daniel
> >>
> >> Adaptive sync is an extended front porch that ends upon some signaling
> >> from the driver is almost always tied to a page flip.
> >>
> >> FLIP_ASYNC is a mechanism to flip immediately without waiting for any
> >> double buffering from the driver or hardware, potentially causing tearing.
> >>
> >> This test uses FLIP_ASYNC to precisely position when the flip occurs
> >> from userspace within the VBLANK to end the frame at the midpoint
> >> between the min and max range.
> >>
> >> But it doesn't really need to be FLIP_ASYNC to make this work, explicit
> >> fencing is probably the better solution here for all drivers.
> > 
> > Ok, I freaked out for a bit there, and reading amdgpu DC code didn't
> > really help with convincing me it's not needed - you're still using
> > your own page_flip_target handler instead of the one in the helpers
> > (written by amd people even, and for a few features before VRR), so
> > biggest worry I had was that VRR doesn't work without FLIP_ASYNC set.
> > I also looked at -amdgpu code, but I couldn't figure this out one way
> > or another there either (since that's not using atomic either).
> > 
> >> This test only really worked by accident for testing amdgpu. We have
> >> internal stalls in the driver to prevent an immediate flip from
> >> occurring more than once per frame, so if you try to flip in the
> >> extended front porch you end up hitting that stall and the returned
> >> timestamp is a frame ahead.
> >>
> >> The other part of this test that's sort of an issue is that this is
> >> effectively a measurement for how fast you can immediate flip - which is
> >> good for catching performance regressions in the driver, and good for
> >> consistent VRR behavior, but not exactly related to the test.
> > 
> > Hm yeah I think that's another reason for why this test should maybe
> > use atomic with vgem dma_fences that we signal exactly when needed.
> > Probably should be a 2nd subtest, so that we can keep the other one
> > working.
> > 
> > Plus update the comment on why exactly FLIP_ASYNC is needed with your
> > story above. Btw instead of FLIP_ASYNC could we use the target frame
> > number, set to the next frame? That should also allow us to do an
> > immediate flip while in the vblank.
> > 
> > Also I think we should have a normal flip which is somewhat late-ish
> > in the vblank, and make sure VRR can still hit that frame. If that
> > doesn't exist yet. I think with that set of tests we should be able to
> > exercise all the timings we want for VRR?
> > 
> >> The way FLIP_ASYNC should probably work in amdgpu is:
> >>
> >> 1) No stalls
> >> 2) Return -EBUSY if we're in the middle of a frame already, or even
> >> allow the commit to happen in the same frame since the hardware supports it
> >>
> >> Not sure how userspace is equipped to handle that though.
> > 
> > I think aside from -amdgpu there's 0 userspace out there using
> > FLIP_ASYNC. Plus it's not standardized for atomic drivers, so really
> > the semantics are super wobbly. I'm not worried about FLIP_ASYNC
> > itself (not the first piece of kms uapi that might be fun to unify
> > across drivers), the only thing that worried me is tying VRR into
> > FLIP_ASYNC. Looks I worried without reasons since it's just an
> > igt/test hack that we can change.

Aside: Big part of my confusion was that I ended up looking at non-DC
code. Would be real nice if we could garbage collect at least some of that
stuff ...

> Thanks, Nick, and Daniel, for hashing this out a bit.
> 
> I still don't fully understand flip semantics in DRM or where they're
> defined.
> 
> One thing that is problematic for some use-cases, and I'm not sure if
> this is unique to amdgpu, is that a normal atomic flip (not ASYNC) will
> wait until HW latches (starts scanning out) the new address before it
> returns. This will stall the renderer.

So for the blocking case we ... block.

For the non-blocking case the work item indeeds blocks until scanout has
started. And we need to, because the next thing we do afterwards is unpin
the buffers. But, and this is important in how atomic works, at least with
the nonblocking helpers: These workers are overlapping, so the next one
will already start before that's all finished. Furthermore if you block
you rendering on a flip, that's definitely not something atomic core nor
helpers does.

So not sure what you're doing, and what exactly is getting blocked.

> A better approach, and an approach that I understand Windows is taking,
> is to return immediately from the driver call after programming the new
> address. AMD HW at least will guarantee that this address is scanned out
> only on the following frame and won't tear (unless an immediate flip is
> explicitly requested). This will let usermode render and flip at a rate
> it desires and ensures that only the flip programmed closest to the
> start of scanout will be actually scanned out.

Ah that one, that's mailbox mode. Simply not yet implemented. There have
been patches floating around about this idea since forever, some even
landed (but not all). The generic cursor implementation at least works
like this, and would be fairly easy to extend. It's really hard to extend
this to all of atomic (Ville had some proof-of-concepts, but even that was
limited), but just mailbox flip mode shouldn't be hard to roll out.

It's just no one has yet done the work (uapi + igt + userspace) to make it
happen.
-Daniel

> 
> Cheers,
> Harry
> 
> > Thanks for your explanations and all.
> > 
> > Cheers, Daniel
> > 
> >>
> >> Regards,
> >> Nicholas Kazlauskas
> >>
> >>>
> >>>> +       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;
> >>>> +}
> >>
> > 
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2] tests: Add variable refresh rate tests

[Harry Wentland]

On 2020-04-15 9:00 a.m., Daniel Vetter wrote:
> On Tue, Apr 14, 2020 at 03:17:20PM -0400, Harry Wentland wrote:
>>
>>
>> On 2020-04-14 2:50 p.m., Daniel Vetter wrote:
>>> On Tue, Apr 14, 2020 at 3:00 PM Kazlauskas, Nicholas
>>> <nicholas.kazlauskas@amd.com> wrote:
>>>>
>>>> On 2020-04-14 6:52 a.m., Daniel Vetter wrote:
>>>>>> +/* 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.
>>>>>> +        */
>>>>>
>>>>> Uh, if this is also how your amdgpu userspace works we've just fucked
>>>>> up the uapi for good :-/
>>>>>
>>>>> FLIP_ASYNC = please tear
>>>>>
>>>>> VRR = please don't strictly obey the vrefresh, but very much dont tear
>>>>>
>>>>> Tying them together means we're deeply mixing things up.
>>>>>
>>>>> Also amdgpu is still using it's own flip implementation, which makes
>>>>> me wonder whether VRR would even work with atomic, or whether that's
>>>>> also butchered ...
>>>>>
>>>>> How I thought this stuff was supposed to work:
>>>>>
>>>>> - VRR_ENABLED controls whether we do VRR
>>>>> - since atomic is awesome you can change that on every frame
>>>>> - VRR has nothing to do with ASYNC
>>>>>
>>>>> So a) do I read this correctly b) how do we get out of this hole (and
>>>>> maybe c) amdgpu really needs to remove
>>>>> amdgpu_display_crtc_page_flip_target asap).
>>>>>
>>>>> Manasi pointed this out to me, so adding a few more people here.
>>>>> -Daniel
>>>>
>>>> Adaptive sync is an extended front porch that ends upon some signaling
>>>> from the driver is almost always tied to a page flip.
>>>>
>>>> FLIP_ASYNC is a mechanism to flip immediately without waiting for any
>>>> double buffering from the driver or hardware, potentially causing tearing.
>>>>
>>>> This test uses FLIP_ASYNC to precisely position when the flip occurs
>>>> from userspace within the VBLANK to end the frame at the midpoint
>>>> between the min and max range.
>>>>
>>>> But it doesn't really need to be FLIP_ASYNC to make this work, explicit
>>>> fencing is probably the better solution here for all drivers.
>>>
>>> Ok, I freaked out for a bit there, and reading amdgpu DC code didn't
>>> really help with convincing me it's not needed - you're still using
>>> your own page_flip_target handler instead of the one in the helpers
>>> (written by amd people even, and for a few features before VRR), so
>>> biggest worry I had was that VRR doesn't work without FLIP_ASYNC set.
>>> I also looked at -amdgpu code, but I couldn't figure this out one way
>>> or another there either (since that's not using atomic either).
>>>
>>>> This test only really worked by accident for testing amdgpu. We have
>>>> internal stalls in the driver to prevent an immediate flip from
>>>> occurring more than once per frame, so if you try to flip in the
>>>> extended front porch you end up hitting that stall and the returned
>>>> timestamp is a frame ahead.
>>>>
>>>> The other part of this test that's sort of an issue is that this is
>>>> effectively a measurement for how fast you can immediate flip - which is
>>>> good for catching performance regressions in the driver, and good for
>>>> consistent VRR behavior, but not exactly related to the test.
>>>
>>> Hm yeah I think that's another reason for why this test should maybe
>>> use atomic with vgem dma_fences that we signal exactly when needed.
>>> Probably should be a 2nd subtest, so that we can keep the other one
>>> working.
>>>
>>> Plus update the comment on why exactly FLIP_ASYNC is needed with your
>>> story above. Btw instead of FLIP_ASYNC could we use the target frame
>>> number, set to the next frame? That should also allow us to do an
>>> immediate flip while in the vblank.
>>>
>>> Also I think we should have a normal flip which is somewhat late-ish
>>> in the vblank, and make sure VRR can still hit that frame. If that
>>> doesn't exist yet. I think with that set of tests we should be able to
>>> exercise all the timings we want for VRR?
>>>
>>>> The way FLIP_ASYNC should probably work in amdgpu is:
>>>>
>>>> 1) No stalls
>>>> 2) Return -EBUSY if we're in the middle of a frame already, or even
>>>> allow the commit to happen in the same frame since the hardware supports it
>>>>
>>>> Not sure how userspace is equipped to handle that though.
>>>
>>> I think aside from -amdgpu there's 0 userspace out there using
>>> FLIP_ASYNC. Plus it's not standardized for atomic drivers, so really
>>> the semantics are super wobbly. I'm not worried about FLIP_ASYNC
>>> itself (not the first piece of kms uapi that might be fun to unify
>>> across drivers), the only thing that worried me is tying VRR into
>>> FLIP_ASYNC. Looks I worried without reasons since it's just an
>>> igt/test hack that we can change.
> 
> Aside: Big part of my confusion was that I ended up looking at non-DC
> code. Would be real nice if we could garbage collect at least some of that
> stuff ...
> 
>> Thanks, Nick, and Daniel, for hashing this out a bit.
>>
>> I still don't fully understand flip semantics in DRM or where they're
>> defined.
>>
>> One thing that is problematic for some use-cases, and I'm not sure if
>> this is unique to amdgpu, is that a normal atomic flip (not ASYNC) will
>> wait until HW latches (starts scanning out) the new address before it
>> returns. This will stall the renderer.
> 
> So for the blocking case we ... block.
> 
> For the non-blocking case the work item indeeds blocks until scanout has
> started. And we need to, because the next thing we do afterwards is unpin
> the buffers. But, and this is important in how atomic works, at least with
> the nonblocking helpers: These workers are overlapping, so the next one
> will already start before that's all finished. Furthermore if you block
> you rendering on a flip, that's definitely not something atomic core nor
> helpers does.
> 
> So not sure what you're doing, and what exactly is getting blocked.
> 
>> A better approach, and an approach that I understand Windows is taking,
>> is to return immediately from the driver call after programming the new
>> address. AMD HW at least will guarantee that this address is scanned out
>> only on the following frame and won't tear (unless an immediate flip is
>> explicitly requested). This will let usermode render and flip at a rate
>> it desires and ensures that only the flip programmed closest to the
>> start of scanout will be actually scanned out.
> 
> Ah that one, that's mailbox mode. Simply not yet implemented. There have
> been patches floating around about this idea since forever, some even
> landed (but not all). The generic cursor implementation at least works
> like this, and would be fairly easy to extend. It's really hard to extend
> this to all of atomic (Ville had some proof-of-concepts, but even that was
> limited), but just mailbox flip mode shouldn't be hard to roll out.
> 
> It's just no one has yet done the work (uapi + igt + userspace) to make it
> happen.

Thanks for teaching me a new term.

Good to hear there has been some work on this before. I gotta look for
Ville's patches sometime.

Harry

> -Daniel
> 
>>
>> Cheers,
>> Harry
>>
>>> Thanks for your explanations and all.
>>>
>>> Cheers, Daniel
>>>
>>>>
>>>> Regards,
>>>> Nicholas Kazlauskas
>>>>
>>>>>
>>>>>> +       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;
>>>>>> +}
>>>>
>>>
>>>
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev