Nouveau - Sep 2020 - [PATCH i-g-t v5 1/5] lib/igt_core: Fix igt_assert_fd() documentation

From: Lyude Paul <lyude at redhat.com>

Nouveau has finally gotten CRC support, and at this point that support
has made its way upstream. Hooray!

So, let's start adding some relevant tests for it since nvidia's CRC
implementation has some rather interesting design choices that needed to
be worked around.

Lyude Paul (5):
  lib/igt_core: Fix igt_assert_fd() documentation
  lib/igt_core: Add igt_require_fd()
  lib/igt_debugfs: Add igt_debugfs_pipe_dir()
  lib/igt_kms: Hook up connector dithering prop
  tests: Add nouveau-crc tests

 .gitlab-ci.yml      |   2 +-
 lib/drmtest.c       |  10 ++
 lib/drmtest.h       |   2 +
 lib/igt_core.h      |  16 +-
 lib/igt_debugfs.c   |  21 +++
 lib/igt_debugfs.h   |   1 +
 lib/igt_kms.c       |   5 +
 lib/igt_kms.h       |   1 +
 tests/meson.build   |   1 +
 tests/nouveau_crc.c | 414 ++++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 470 insertions(+), 3 deletions(-)
 create mode 100644 tests/nouveau_crc.c

-- 
2.26.2

From: Lyude Paul <lyude at redhat.com>

As Petri Latvala pointed out, some of the documentation in this macro is
mistakenly copied from the other igt_assert*() macros. Let's fix that.

Signed-off-by: Lyude Paul <lyude at redhat.com>
---
 lib/igt_core.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/igt_core.h b/lib/igt_core.h
index c5871520..e74ede8b 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -966,8 +966,8 @@ void igt_describe_f(const char *fmt, ...);
  *
  * Fails (sub-) test if the given file descriptor is invalid.
  *
- * Like igt_assert(), but displays the values being compared on failure instead
- * of simply printing the stringified expression.
+ * Like igt_assert(), but displays the stringified identifier that was supposed
+ * to contain a valid fd on failure.
  */
 #define igt_assert_fd(fd) \
 	igt_assert_f(fd >= 0, "file descriptor " #fd "
failed\n");
-- 
2.26.2

From: Lyude Paul <lyude at redhat.com>

Like igt_assert_fd(), but using igt_require() instead

Changes since v1:
* Fix documentation error in igt_require_fd() - Petri Latvala

Signed-off-by: Lyude Paul <lyude at redhat.com>
---
 lib/igt_core.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/lib/igt_core.h b/lib/igt_core.h
index e74ede8b..5d835260 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -1021,6 +1021,18 @@ void igt_describe_f(const char *fmt, ...);
 	else igt_debug("Test requirement passed: %s\n", #expr); \
 } while (0)
 
+/**
+ * igt_require_fd:
+ * @fd: file descriptor
+ *
+ * Skips (sub-) test if the given file descriptor is invalid.
+ *
+ * Like igt_require(), but displays the stringified identifier that was
supposed
+ * to contain a valid fd on failure.
+ */
+#define igt_require_fd(fd) \
+	igt_require_f(fd >= 0, "file descriptor " #fd "
failed\n");
+
 /**
  * igt_skip_on_f:
  * @expr: condition to test
-- 
2.26.2

From: Lyude Paul <lyude at redhat.com>

Like igt_debugfs_connector_dir(), but for pipes instead.

Changes since v4:
* Make igt_debugfs_pipe_dir() much smaller - jcline
* Fixup docs - jcline

Signed-off-by: Lyude Paul <lyude at redhat.com>
---
 lib/igt_debugfs.c | 21 +++++++++++++++++++++
 lib/igt_debugfs.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
index 05067078..c4dab392 100644
--- a/lib/igt_debugfs.c
+++ b/lib/igt_debugfs.c
@@ -260,6 +260,27 @@ int igt_debugfs_connector_dir(int device, char *conn_name,
int mode)
 	return ret;
 }
 
+/**
+ * igt_debugfs_pipe_dir:
+ * @device: fd of the device
+ * @pipe: index of pipe
+ * @mode: mode bits as used by open()
+ *
+ * This opens the debugfs directory corresponding to the pipe index on the
+ * device for use with igt_sysfs_get() and related functions. This is just
+ * syntax sugar for igt_debugfs_open().
+ *
+ * Returns:
+ * The directory fd, or -1 on failure.
+ */
+int igt_debugfs_pipe_dir(int device, int pipe, int mode)
+{
+	char buf[128];
+
+	snprintf(buf, sizeof(buf), "crtc-%d", pipe);
+	return igt_debugfs_open(device, buf, mode);
+}
+
 /**
  * igt_debugfs_open:
  * @filename: name of the debugfs node to open
diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
index 722c5cc3..d43ba6c6 100644
--- a/lib/igt_debugfs.h
+++ b/lib/igt_debugfs.h
@@ -36,6 +36,7 @@ char *igt_debugfs_path(int device, char *path, int pathlen);
 
 int igt_debugfs_dir(int device);
 int igt_debugfs_connector_dir(int device, char *conn_name, int mode);
+int igt_debugfs_pipe_dir(int device, int pipe, int mode);
 
 int igt_debugfs_open(int fd, const char *filename, int mode);
 void __igt_debugfs_read(int fd, const char *filename, char *buf, int size);
-- 
2.26.2

From: Lyude Paul <lyude at redhat.com>

Nvidia display hardware provides a set of flexible dithering options for
CRTCs. This dithering is actually noticeable in the CRC output for all
available tap points, and can be seen as CRC values for identical frames
cycling between either 2 or 4 values repeatedly (each one of these
values is a different dithering phase applied to the source output). Of
course, this is very likely to break tests using CRC readback since we
don't expect the CRC to change if the source content hasn't changed.

So, hook up support for configuring the dithering property and reset it
to off from igt_display_reset() when applicable.

Signed-off-by: Lyude Paul <lyude at redhat.com>
---
 lib/igt_kms.c | 5 +++++
 lib/igt_kms.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 4023811a..7946555d 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -425,6 +425,7 @@ const char * const
igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
 	[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS] = "WRITEBACK_PIXEL_FORMATS",
 	[IGT_CONNECTOR_WRITEBACK_FB_ID] = "WRITEBACK_FB_ID",
 	[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR] = "WRITEBACK_OUT_FENCE_PTR",
+	[IGT_CONNECTOR_DITHERING_MODE] = "dithering mode",
 };
 
 /*
@@ -1825,6 +1826,9 @@ static void igt_output_reset(igt_output_t *output)
 		igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
 		output->writeback_out_fence_fd = -1;
 	}
+	if (igt_output_has_prop(output, IGT_CONNECTOR_DITHERING_MODE))
+		igt_output_set_prop_enum(output, IGT_CONNECTOR_DITHERING_MODE,
+					 "off");
 
 }
 
@@ -1840,6 +1844,7 @@ static void igt_output_reset(igt_output_t *output)
  * - %IGT_CONNECTOR_BROADCAST_RGB (if applicable)
  *   %IGT_CONNECTOR_CONTENT_PROTECTION (if applicable)
  *   %IGT_CONNECTOR_HDR_OUTPUT_METADATA (if applicable)
+ * - %IGT_CONNECTOR_DITHERING_MODE (if applicable)
  * - igt_output_override_mode() to default.
  *
  * For pipes:
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index f41eadaf..6ad1a7ef 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -132,6 +132,7 @@ enum igt_atomic_connector_properties {
        IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS,
        IGT_CONNECTOR_WRITEBACK_FB_ID,
        IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
+       IGT_CONNECTOR_DITHERING_MODE,
        IGT_NUM_CONNECTOR_PROPS
 };
 
-- 
2.26.2

From: Lyude Paul <lyude at redhat.com>

We're finally getting CRC support in nouveau, so let's start testing
this in igt as well! While the normal CRC capture tests are nice,
there's a number of Nvidia-specific hardware characteristics that we
need to test as well.

The most important one is known as a "notifier context flip".
Basically,
Nvidia GPUs store generated CRCs in an allocated memory region, referred
to as the notifier context, that the driver programs itself. Strangely,
this region can only hold a limited number of CRC entries, and once it
runs out of available entries the hardware simply sets an overrun bit
and stops writing any new CRC entries.

Since igt-gpu-tools doesn't really have an expectation of only being
able to grab a limited number of CRCs, we work around this in nouveau by
allocating two separate CRC notifier regions each time we start
capturing CRCs, and then flip between them whenever we get close to
filling our currently programmed notifier context. While this keeps the
number of CRC entries we lose to an absolute minimum, we are guaranteed
to lose exactly one CRC entry between context flips. Thus, we add some
tests to ensure that nouveau handles these flips correctly
(pipe-[A-F]-ctx-flip-detection), and that igt itself is also able to
handle them correctly (pipe-[A-F]-ctx-flip-skip-current-frame). Since
these tests use a debugfs interface to manually control the notifier
context flip threshold, we also add one test to ensure that any flip
thresholds we set are cleared after a single CRC capture
(ctx-flip-threshold-reset-after-capture).

In addition, we also add some simple tests to test Nvidia-specific CRC
sources.

Changes since v4:
* Don't clear the currently set display pipe at the end of each iteration of
  pipe tests, otherwise we'll accidentally leave ourselves with no
configured
  CRTCs when the test exits. Instead clear it at the start of each iteration of
  pipe tests but only if we just finished testing a different pipe.
* Get rid of outdated comment about test_ctx_flip_detection() being limited to a
  single pipe
* Clarify comments describing guarding against CRC collisions in
  test_ctx_flip_detection()
* Make a small doc blurb for test_ctx_flip_detection(), it's a rather
unusual
  and nvidia-specific behavior
* Mention in test_ctx_flip_detection() that we also explicitly check that
we've
  skipped -exactly- one frame. tl;dr more then one frame is too slow, less then
  one frame means a context flip just didn't happen when it should have.
* Also check that the flip threshold we set in
  test_ctx_flip_threshold_reset_after_capture() isn't ignored by the driver
* s/(create|destroy)_colors()/\1_crc_colors/g
Changes since v3:
* Update .gitlab-ci.yml to make nouveau exempt from the test-list-diff
  test, since all the cool kids are doing it and we don't care about
  autotools for nouveau
Changes since v2:
* Fix missing include in tests/nouveau_crc.c, this should fix builds for
  aarch64
Changes since v1:
* Fix copyright year in nouveau_crc.c

Signed-off-by: Lyude Paul <lyude at redhat.com>
---
 .gitlab-ci.yml      |   2 +-
 lib/drmtest.c       |  10 ++
 lib/drmtest.h       |   2 +
 tests/meson.build   |   1 +
 tests/nouveau_crc.c | 414 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 428 insertions(+), 1 deletion(-)
 create mode 100644 tests/nouveau_crc.c

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index d7fdbfde..e226d9d7 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -241,7 +241,7 @@ test:test-list-diff:
     - build:tests-debian-autotools
     - build:tests-debian-meson
   stage: test
-  script: diff <(sed "s/ /\n/g" meson-test-list.txt| grep -v
'vc4\|v3d\|panfrost' | sort) <(sed "s/ /\n/g"
autotools-test-list.txt | sort)
+  script: diff <(sed "s/ /\n/g" meson-test-list.txt| grep -v
'vc4\|v3d\|panfrost\|nouveau' | sort) <(sed "s/ /\n/g"
autotools-test-list.txt | sort)
 
 test:list-undocumented-tests:
   dependencies:
diff --git a/lib/drmtest.c b/lib/drmtest.c
index c732d1dd..447f5435 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -114,6 +114,11 @@ bool is_i915_device(int fd)
 	return __is_device(fd, "i915");
 }
 
+bool is_nouveau_device(int fd)
+{
+	return __is_device(fd, "nouveau");
+}
+
 bool is_vc4_device(int fd)
 {
 	return __is_device(fd, "vc4");
@@ -622,6 +627,11 @@ void igt_require_intel(int fd)
 	igt_require(is_i915_device(fd));
 }
 
+void igt_require_nouveau(int fd)
+{
+	igt_require(is_nouveau_device(fd));
+}
+
 void igt_require_vc4(int fd)
 {
 	igt_require(is_vc4_device(fd));
diff --git a/lib/drmtest.h b/lib/drmtest.h
index c56bfafa..dd4cd384 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -96,10 +96,12 @@ int __drm_open_driver_render(int chipset);
 
 void igt_require_amdgpu(int fd);
 void igt_require_intel(int fd);
+void igt_require_nouveau(int fd);
 void igt_require_vc4(int fd);
 
 bool is_amdgpu_device(int fd);
 bool is_i915_device(int fd);
+bool is_nouveau_device(int fd);
 bool is_vc4_device(int fd);
 
 /**
diff --git a/tests/meson.build b/tests/meson.build
index 515f7528..32f011e9 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -76,6 +76,7 @@ test_progs = [
 	'kms_vrr',
 	'kms_writeback',
 	'meta_test',
+	'nouveau_crc',
 	'panfrost_get_param',
 	'panfrost_gem_new',
 	'panfrost_prime',
diff --git a/tests/nouveau_crc.c b/tests/nouveau_crc.c
new file mode 100644
index 00000000..f8705b5e
--- /dev/null
+++ b/tests/nouveau_crc.c
@@ -0,0 +1,414 @@
+/*
+ * Copyright ? 2020 Red Hat 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 (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include <fcntl.h>
+#include "igt.h"
+#include "igt_sysfs.h"
+
+IGT_TEST_DESCRIPTION(
+"Tests certain aspects of CRC capture that are exclusive to nvidia
hardware, "
+"such as context flipping.");
+
+typedef struct {
+	int pipe;
+	int drm_fd;
+	int nv_crc_dir;
+	igt_display_t display;
+	igt_output_t *output;
+	igt_plane_t *primary;
+	drmModeModeInfo *mode;
+	igt_fb_t default_fb;
+} data_t;
+
+struct color_fb {
+	double r, g, b;
+	igt_crc_t crc;
+	igt_fb_t fb;
+};
+
+#define HEX_COLOR(r_, g_, b_) \
+	{ .r = (r_ / 255.0), .g = (g_ / 255.0), .b = (b_ / 255.0) }
+
+static void set_crc_flip_threshold(data_t *data, unsigned int threshold)
+{
+	igt_debug("Setting CRC notifier flip threshold to %d\n", threshold);
+	igt_assert_lt(0, igt_sysfs_printf(data->nv_crc_dir,
"flip_threshold", "%d", threshold));
+}
+
+/* Initialize each color_fb along with its respective CRC */
+static void create_crc_colors(data_t *data,
+			      struct color_fb *colors,
+			      size_t len,
+			      igt_pipe_crc_t *pipe_crc)
+{
+	char *crc_str;
+
+	igt_pipe_crc_start(pipe_crc);
+
+	for (int i = 0; i < len; i++) {
+		igt_create_color_fb(data->drm_fd,
+				    data->mode->hdisplay,
+				    data->mode->vdisplay,
+				    DRM_FORMAT_XRGB8888,
+				    LOCAL_DRM_FORMAT_MOD_NONE,
+				    colors[i].r, colors[i].g, colors[i].b,
+				    &colors[i].fb);
+
+		igt_plane_set_fb(data->primary, &colors[i].fb);
+		igt_display_commit(&data->display);
+		igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &colors[i].crc);
+
+		crc_str = igt_crc_to_string(&colors[i].crc);
+		igt_debug("CRC for frame %d of pattern: %s\n",
+			  i, crc_str);
+		free(crc_str);
+	}
+
+	igt_pipe_crc_stop(pipe_crc);
+}
+
+static void destroy_crc_colors(data_t *data, struct color_fb *colors, size_t
len)
+{
+	/* So we don't turn off the pipe if we remove it's current fb */
+	igt_plane_set_fb(data->primary, &data->default_fb);
+
+	for (int i = 0; i < len; i++)
+		igt_remove_fb(data->drm_fd, &colors[i].fb);
+}
+
+/*
+ * Nvidia GPUs store CRCs in a limited memory tregion called the CRC notifier
context. When this
+ * region fills, new CRCs are not reported. Nouveau works around this by
allocating two notifier
+ * contents, and then flips between them whenever we pass a specific threshold.
Note that even with
+ * this approach, a single frame is lost during the context flip.
+ */
+static void test_ctx_flip_detection(data_t *data)
+{
+	struct color_fb colors[] = {
+		HEX_COLOR(0xFF, 0x00, 0x18),
+		HEX_COLOR(0xFF, 0xA5, 0x2C),
+		HEX_COLOR(0xFF, 0xFF, 0x41),
+		HEX_COLOR(0x00, 0x80, 0x18),
+		HEX_COLOR(0x00, 0x00, 0xF9),
+		HEX_COLOR(0x86, 0x00, 0x7D),
+	};
+	igt_output_t *output = data->output;
+	igt_plane_t *primary = data->primary;
+	igt_pipe_crc_t *pipe_crc;
+	const int n_colors = ARRAY_SIZE(colors);
+	const int n_crcs = 20;
+	igt_crc_t *crcs = NULL;
+	int start = -1, frame, start_color = -1, i;
+	bool found_skip = false;
+
+	pipe_crc = igt_pipe_crc_new(data->drm_fd, data->pipe, "auto");
+
+	create_crc_colors(data, colors, n_colors, pipe_crc);
+
+	set_crc_flip_threshold(data, n_crcs / 2);
+	igt_pipe_crc_start(pipe_crc);
+
+	for (i = 0; i < n_crcs; i++) {
+		const int color_idx = i % n_colors;
+
+		igt_plane_set_fb(primary, &colors[color_idx].fb);
+		do_or_die(drmModePageFlip(data->drm_fd,
+					  output->config.crtc->crtc_id,
+					  colors[color_idx].fb.fb_id,
+					  DRM_MODE_PAGE_FLIP_EVENT,
+					  NULL));
+		kmstest_wait_for_pageflip(data->drm_fd);
+	}
+
+	igt_pipe_crc_get_crcs(pipe_crc, n_crcs, &crcs);
+	igt_pipe_crc_stop(pipe_crc);
+
+	/*
+	 * Guard against CRC collisions in the color framebuffers by finding the first
color in our
+	 * pattern with a CRC that differs from the last CRC. That CRC can then be
used to find the
+	 * start of the pattern
+	 */
+	for (i = 0; i < n_colors - 1; i++) {
+		if (igt_check_crc_equal(&colors[i].crc, &colors[n_colors - 1].crc))
+			continue;
+
+		igt_debug("Using frame %d of pattern for finding start\n", i);
+		start_color = i;
+		break;
+	}
+	igt_assert_lte(0, start_color);
+
+	/* Now, figure out where the pattern starts */
+	for (i = 0; i < n_crcs; i++) {
+		if (!igt_check_crc_equal(&colors[start_color].crc, &crcs[i]))
+			continue;
+
+		start = i - start_color;
+		frame = crcs[i].frame;
+		igt_debug("Pattern started on frame %d\n", frame);
+		break;
+	}
+	igt_assert_lte(0, start);
+
+	/* And finally, assert that according to the CRCs exactly all but one
+	 * frame was displayed in order. The missing frame comes from
+	 * (inevitably) losing a single CRC event when nouveau switches notifier
+	 * contexts
+	 */
+	for (i = start; i < n_crcs; i++, frame++) {
+		igt_crc_t *crc = &crcs[i];
+		char *crc_str;
+		int color_idx;
+
+		crc_str = igt_crc_to_string(crc);
+		igt_debug("CRC %d: vbl=%d val=%s\n", i, crc->frame, crc_str);
+		free(crc_str);
+
+		if (!found_skip && crc->frame != frame) {
+			igt_debug("^^^ Found expected skipped CRC %d ^^^\n",
+				  crc->frame - 1);
+			found_skip = true;
+			frame++;
+		}
+
+		/* We should never skip more then one frame, as with nouveau's current
CRC
+		 * implementation this would mean that we've lost track of which CRC
corresponds to
+		 * which frame, making our frame index unreliable. So, we also check each
frame that
+		 * comes after the skip, and ensure that it matches the colors that we
expect.
+		 */
+		if (found_skip) {
+			igt_assert_eq(crc->frame, frame);
+			color_idx = (i - start + 1) % n_colors;
+		} else {
+			color_idx = (i - start) % n_colors;
+		}
+
+		igt_assert_crc_equal(crc, &colors[color_idx].crc);
+	}
+	/* Also, if we never found a skip in the first place then something's
broken and the CRC
+	 * threshold we set was ignored by the driver, or the driver failed to flip
contexts */
+	igt_assert(found_skip);
+
+	free(crcs);
+	igt_pipe_crc_free(pipe_crc);
+	destroy_crc_colors(data, colors, ARRAY_SIZE(colors));
+}
+
+/* Test whether or not IGT is able to handle frame skips when requesting the
+ * CRC for the current frame
+ */
+static void test_ctx_flip_skip_current_frame(data_t *data)
+{
+	struct color_fb colors[] = {
+		{ .r = 1.0, .g = 0.0, .b = 0.0 },
+		{ .r = 0.0, .g = 1.0, .b = 0.0 },
+		{ .r = 0.0, .g = 0.0, .b = 1.0 },
+	};
+	igt_output_t *output = data->output;
+	igt_pipe_crc_t *pipe_crc;
+	igt_plane_t *primary = data->primary;
+	const int fd = data->drm_fd;
+	const int n_colors = ARRAY_SIZE(colors);
+	const int n_crcs = 30;
+
+	pipe_crc = igt_pipe_crc_new(fd, data->pipe, "auto");
+	create_crc_colors(data, colors, n_colors, pipe_crc);
+
+	set_crc_flip_threshold(data, 5);
+	igt_pipe_crc_start(pipe_crc);
+
+	for (int i = 0; i < n_crcs; i++) {
+		igt_crc_t crc;
+		const int color_idx = i % n_colors;
+
+		igt_plane_set_fb(primary, &colors[color_idx].fb);
+		do_or_die(drmModePageFlip(fd,
+					  output->config.crtc->crtc_id,
+					  colors[color_idx].fb.fb_id,
+					  DRM_MODE_PAGE_FLIP_EVENT,
+					  NULL));
+		kmstest_wait_for_pageflip(fd);
+
+		igt_pipe_crc_get_current(fd, pipe_crc, &crc);
+		igt_assert_crc_equal(&colors[color_idx].crc, &crc);
+	}
+
+	igt_pipe_crc_stop(pipe_crc);
+	igt_pipe_crc_free(pipe_crc);
+	destroy_crc_colors(data, colors, n_colors);
+}
+
+static void test_ctx_flip_threshold_reset_after_capture(data_t *data)
+{
+	igt_pipe_crc_t *pipe_crc;
+	const int fd = data->drm_fd;
+
+	pipe_crc = igt_pipe_crc_new(fd, data->pipe, "auto");
+
+	set_crc_flip_threshold(data, 5);
+	igt_pipe_crc_start(pipe_crc);
+	igt_assert_eq(igt_sysfs_get_u32(data->nv_crc_dir,
"flip_threshold"), 5);
+	igt_pipe_crc_stop(pipe_crc);
+
+	igt_assert_neq(igt_sysfs_get_u32(data->nv_crc_dir,
"flip_threshold"), 5);
+	igt_pipe_crc_free(pipe_crc);
+}
+
+static void test_source(data_t *data, const char *source)
+{
+	igt_pipe_crc_t *pipe_crc = igt_pipe_crc_new(data->drm_fd, data->pipe,
source);
+	igt_crc_t *crcs;
+
+	igt_pipe_crc_start(pipe_crc);
+	igt_pipe_crc_get_crcs(pipe_crc, 2, &crcs);
+	igt_pipe_crc_stop(pipe_crc);
+
+	/* The CRC shouldn't change if the source content hasn't changed */
+	igt_assert_crc_equal(&crcs[0], &crcs[1]);
+
+	igt_pipe_crc_free(pipe_crc);
+	free(crcs);
+}
+
+static void test_source_outp_inactive(data_t *data)
+{
+	struct color_fb colors[] = {
+		{ .r = 1.0, .g = 0.0, .b = 0.0 },
+		{ .r = 0.0, .g = 1.0, .b = 0.0 },
+	};
+	igt_pipe_crc_t *pipe_crc;
+	const int fd = data->drm_fd;
+	const int n_colors = ARRAY_SIZE(colors);
+
+	pipe_crc = igt_pipe_crc_new(fd, data->pipe, "outp-inactive");
+	create_crc_colors(data, colors, n_colors, pipe_crc);
+
+	/* Changing the color should not change what's outside the active raster
*/
+	igt_assert_crc_equal(&colors[0].crc, &colors[1].crc);
+
+	igt_pipe_crc_free(pipe_crc);
+	destroy_crc_colors(data, colors, n_colors);
+}
+
+data_t data = {0};
+
+#define pipe_test(name) igt_subtest_f("pipe-%s-" name,
kmstest_pipe_name(pipe))
+igt_main
+{
+	int pipe;
+
+	igt_fixture {
+		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
+		igt_require_nouveau(data.drm_fd);
+
+		kmstest_set_vt_graphics_mode();
+
+		igt_require_pipe_crc(data.drm_fd);
+		igt_display_require(&data.display, data.drm_fd);
+		igt_display_reset(&data.display);
+	}
+
+	for_each_pipe_static(pipe) {
+		igt_fixture {
+			int dir;
+
+			data.pipe = pipe;
+			igt_display_require_output_on_pipe(&data.display, pipe);
+
+			/* Disable the output from the previous iteration of pipe tests, if there is
+			 * one
+			 */
+			if (data.output) {
+				igt_output_set_pipe(data.output, PIPE_NONE);
+				igt_display_commit(&data.display);
+			}
+
+			data.output = igt_get_single_output_for_pipe(&data.display, pipe);
+			data.mode = igt_output_get_mode(data.output);
+
+			/* None of these tests need to perform modesets, just page flips. So running
+			 * display setup here is fine
+			 */
+			igt_output_set_pipe(data.output, pipe);
+			data.primary = igt_output_get_plane(data.output, 0);
+			igt_create_color_fb(data.drm_fd,
+					    data.mode->hdisplay,
+					    data.mode->vdisplay,
+					    DRM_FORMAT_XRGB8888,
+					    LOCAL_DRM_FORMAT_MOD_NONE,
+					    0.0, 0.0, 0.0,
+					    &data.default_fb);
+			igt_plane_set_fb(data.primary, &data.default_fb);
+			igt_display_commit(&data.display);
+
+			dir = igt_debugfs_pipe_dir(data.drm_fd, pipe, O_DIRECTORY);
+			igt_require_fd(dir);
+			data.nv_crc_dir = openat(dir, "nv_crc", O_DIRECTORY);
+			close(dir);
+			igt_require_fd(data.nv_crc_dir);
+		}
+
+		/* We don't need to test this on every pipe, but the setup is the same */
+		if (pipe == PIPE_A) {
+			igt_describe("Make sure that the CRC notifier context flip threshold
"
+				     "is reset to its default value after a single capture.");
+			igt_subtest("ctx-flip-threshold-reset-after-capture")
+				test_ctx_flip_threshold_reset_after_capture(&data);
+		}
+
+		igt_describe("Make sure the association between each CRC and its "
+			     "respective frame index is not broken when the driver "
+			     "performs a notifier context flip.");
+		pipe_test("ctx-flip-detection")
+			test_ctx_flip_detection(&data);
+
+		igt_describe("Make sure that igt_pipe_crc_get_current() works even
"
+			     "when the CRC for the current frame index is lost.");
+		pipe_test("ctx-flip-skip-current-frame")
+			test_ctx_flip_skip_current_frame(&data);
+
+		igt_describe("Check that basic CRC readback using the outp-complete
"
+			     "source works.");
+		pipe_test("source-outp-complete")
+			test_source(&data, "outp-complete");
+
+		igt_describe("Check that basic CRC readback using the rg source "
+			     "works.");
+		pipe_test("source-rg")
+			test_source(&data, "rg");
+
+		igt_describe("Make sure that the outp-inactive source is actually "
+			     "capturing the inactive raster.");
+		pipe_test("source-outp-inactive")
+			test_source_outp_inactive(&data);
+
+		igt_fixture {
+			igt_remove_fb(data.drm_fd, &data.default_fb);
+			close(data.nv_crc_dir);
+		}
+	}
+	igt_fixture
+		igt_display_fini(&data.display);
+
+}
-- 
2.26.2

On Wed, Sep 30, 2020 at 01:31:50PM -0400, Lyude wrote:
> From: Lyude Paul <lyude at redhat.com>
> 
> We're finally getting CRC support in nouveau, so let's start
testing
> this in igt as well! While the normal CRC capture tests are nice,
> there's a number of Nvidia-specific hardware characteristics that we
> need to test as well.
> 
> The most important one is known as a "notifier context flip".
Basically,
> Nvidia GPUs store generated CRCs in an allocated memory region, referred
> to as the notifier context, that the driver programs itself. Strangely,
> this region can only hold a limited number of CRC entries, and once it
> runs out of available entries the hardware simply sets an overrun bit
> and stops writing any new CRC entries.
> 
> Since igt-gpu-tools doesn't really have an expectation of only being
> able to grab a limited number of CRCs, we work around this in nouveau by
> allocating two separate CRC notifier regions each time we start
> capturing CRCs, and then flip between them whenever we get close to
> filling our currently programmed notifier context. While this keeps the
> number of CRC entries we lose to an absolute minimum, we are guaranteed
> to lose exactly one CRC entry between context flips. Thus, we add some
> tests to ensure that nouveau handles these flips correctly
> (pipe-[A-F]-ctx-flip-detection), and that igt itself is also able to
> handle them correctly (pipe-[A-F]-ctx-flip-skip-current-frame). Since
> these tests use a debugfs interface to manually control the notifier
> context flip threshold, we also add one test to ensure that any flip
> thresholds we set are cleared after a single CRC capture
> (ctx-flip-threshold-reset-after-capture).
> 
> In addition, we also add some simple tests to test Nvidia-specific CRC
> sources.
> 
> Changes since v4:
> * Don't clear the currently set display pipe at the end of each
iteration of
>   pipe tests, otherwise we'll accidentally leave ourselves with no
configured
>   CRTCs when the test exits. Instead clear it at the start of each
iteration of
>   pipe tests but only if we just finished testing a different pipe.
> * Get rid of outdated comment about test_ctx_flip_detection() being limited
to a
>   single pipe
> * Clarify comments describing guarding against CRC collisions in
>   test_ctx_flip_detection()
> * Make a small doc blurb for test_ctx_flip_detection(), it's a rather
unusual
>   and nvidia-specific behavior
> * Mention in test_ctx_flip_detection() that we also explicitly check that
we've
>   skipped -exactly- one frame. tl;dr more then one frame is too slow, less
then
>   one frame means a context flip just didn't happen when it should
have.
> * Also check that the flip threshold we set in
>   test_ctx_flip_threshold_reset_after_capture() isn't ignored by the
driver
> * s/(create|destroy)_colors()/\1_crc_colors/g
> Changes since v3:
> * Update .gitlab-ci.yml to make nouveau exempt from the test-list-diff
>   test, since all the cool kids are doing it and we don't care about
>   autotools for nouveau
> Changes since v2:
> * Fix missing include in tests/nouveau_crc.c, this should fix builds for
>   aarch64
> Changes since v1:
> * Fix copyright year in nouveau_crc.c
> 
> Signed-off-by: Lyude Paul <lyude at redhat.com>
> ---
>  .gitlab-ci.yml      |   2 +-
>  lib/drmtest.c       |  10 ++
>  lib/drmtest.h       |   2 +
>  tests/meson.build   |   1 +
>  tests/nouveau_crc.c | 414 ++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 428 insertions(+), 1 deletion(-)
>  create mode 100644 tests/nouveau_crc.c
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index d7fdbfde..e226d9d7 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -241,7 +241,7 @@ test:test-list-diff:
>      - build:tests-debian-autotools
>      - build:tests-debian-meson
>    stage: test
> -  script: diff <(sed "s/ /\n/g" meson-test-list.txt| grep -v
'vc4\|v3d\|panfrost' | sort) <(sed "s/ /\n/g"
autotools-test-list.txt | sort)
> +  script: diff <(sed "s/ /\n/g" meson-test-list.txt| grep -v
'vc4\|v3d\|panfrost\|nouveau' | sort) <(sed "s/ /\n/g"
autotools-test-list.txt | sort)
>  
>  test:list-undocumented-tests:
>    dependencies:
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index c732d1dd..447f5435 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -114,6 +114,11 @@ bool is_i915_device(int fd)
>  	return __is_device(fd, "i915");
>  }
>  
> +bool is_nouveau_device(int fd)
> +{
> +	return __is_device(fd, "nouveau");
> +}
> +
>  bool is_vc4_device(int fd)
>  {
>  	return __is_device(fd, "vc4");
> @@ -622,6 +627,11 @@ void igt_require_intel(int fd)
>  	igt_require(is_i915_device(fd));
>  }
>  
> +void igt_require_nouveau(int fd)
> +{
> +	igt_require(is_nouveau_device(fd));
> +}
> +
>  void igt_require_vc4(int fd)
>  {
>  	igt_require(is_vc4_device(fd));
> diff --git a/lib/drmtest.h b/lib/drmtest.h
> index c56bfafa..dd4cd384 100644
> --- a/lib/drmtest.h
> +++ b/lib/drmtest.h
> @@ -96,10 +96,12 @@ int __drm_open_driver_render(int chipset);
>  
>  void igt_require_amdgpu(int fd);
>  void igt_require_intel(int fd);
> +void igt_require_nouveau(int fd);
>  void igt_require_vc4(int fd);
>  
>  bool is_amdgpu_device(int fd);
>  bool is_i915_device(int fd);
> +bool is_nouveau_device(int fd);
>  bool is_vc4_device(int fd);
>  
>  /**
> diff --git a/tests/meson.build b/tests/meson.build
> index 515f7528..32f011e9 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -76,6 +76,7 @@ test_progs = [
>  	'kms_vrr',
>  	'kms_writeback',
>  	'meta_test',
> +	'nouveau_crc',
>  	'panfrost_get_param',
>  	'panfrost_gem_new',
>  	'panfrost_prime',
> diff --git a/tests/nouveau_crc.c b/tests/nouveau_crc.c
> new file mode 100644
> index 00000000..f8705b5e
> --- /dev/null
> +++ b/tests/nouveau_crc.c
> @@ -0,0 +1,414 @@
> +/*
> + * Copyright ? 2020 Red Hat 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 (including the
next
> + * paragraph) shall be included in all copies or substantial portions of
the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include <fcntl.h>
> +#include "igt.h"
> +#include "igt_sysfs.h"
> +
> +IGT_TEST_DESCRIPTION(
> +"Tests certain aspects of CRC capture that are exclusive to nvidia
hardware, "
> +"such as context flipping.");
> +
> +typedef struct {
> +	int pipe;
> +	int drm_fd;
> +	int nv_crc_dir;
> +	igt_display_t display;
> +	igt_output_t *output;
> +	igt_plane_t *primary;
> +	drmModeModeInfo *mode;
> +	igt_fb_t default_fb;
> +} data_t;
> +
> +struct color_fb {
> +	double r, g, b;
> +	igt_crc_t crc;
> +	igt_fb_t fb;
> +};
> +
> +#define HEX_COLOR(r_, g_, b_) \
> +	{ .r = (r_ / 255.0), .g = (g_ / 255.0), .b = (b_ / 255.0) }
> +
> +static void set_crc_flip_threshold(data_t *data, unsigned int threshold)
> +{
> +	igt_debug("Setting CRC notifier flip threshold to %d\n",
threshold);
> +	igt_assert_lt(0, igt_sysfs_printf(data->nv_crc_dir,
"flip_threshold", "%d", threshold));
> +}
> +
> +/* Initialize each color_fb along with its respective CRC */
> +static void create_crc_colors(data_t *data,
> +			      struct color_fb *colors,
> +			      size_t len,
> +			      igt_pipe_crc_t *pipe_crc)
> +{
> +	char *crc_str;
> +
> +	igt_pipe_crc_start(pipe_crc);
> +
> +	for (int i = 0; i < len; i++) {
> +		igt_create_color_fb(data->drm_fd,
> +				    data->mode->hdisplay,
> +				    data->mode->vdisplay,
> +				    DRM_FORMAT_XRGB8888,
> +				    LOCAL_DRM_FORMAT_MOD_NONE,
> +				    colors[i].r, colors[i].g, colors[i].b,
> +				    &colors[i].fb);
> +
> +		igt_plane_set_fb(data->primary, &colors[i].fb);
> +		igt_display_commit(&data->display);
> +		igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &colors[i].crc);
> +
> +		crc_str = igt_crc_to_string(&colors[i].crc);
> +		igt_debug("CRC for frame %d of pattern: %s\n",
> +			  i, crc_str);
> +		free(crc_str);
> +	}
> +
> +	igt_pipe_crc_stop(pipe_crc);
> +}
> +
> +static void destroy_crc_colors(data_t *data, struct color_fb *colors,
size_t len)
> +{
> +	/* So we don't turn off the pipe if we remove it's current fb */
> +	igt_plane_set_fb(data->primary, &data->default_fb);
> +
> +	for (int i = 0; i < len; i++)
> +		igt_remove_fb(data->drm_fd, &colors[i].fb);
> +}
> +
> +/*
> + * Nvidia GPUs store CRCs in a limited memory tregion called the CRC
notifier context. When this
s/tregion/region/. Not sure it's worth the email traffic required to fix
it, though.

On Wed, Sep 30, 2020 at 01:31:45PM -0400, Lyude wrote:
> From: Lyude Paul <lyude at redhat.com>
> 
> Nouveau has finally gotten CRC support, and at this point that support
> has made its way upstream. Hooray!
> 
> So, let's start adding some relevant tests for it since nvidia's
CRC
> implementation has some rather interesting design choices that needed to
> be worked around.
> 
> Lyude Paul (5):
>   lib/igt_core: Fix igt_assert_fd() documentation
>   lib/igt_core: Add igt_require_fd()
>   lib/igt_debugfs: Add igt_debugfs_pipe_dir()
>   lib/igt_kms: Hook up connector dithering prop
>   tests: Add nouveau-crc tests
> 
>  .gitlab-ci.yml      |   2 +-
>  lib/drmtest.c       |  10 ++
>  lib/drmtest.h       |   2 +
>  lib/igt_core.h      |  16 +-
>  lib/igt_debugfs.c   |  21 +++
>  lib/igt_debugfs.h   |   1 +
>  lib/igt_kms.c       |   5 +
>  lib/igt_kms.h       |   1 +
>  tests/meson.build   |   1 +
>  tests/nouveau_crc.c | 414 ++++++++++++++++++++++++++++++++++++++++++++
>  10 files changed, 470 insertions(+), 3 deletions(-)
>  create mode 100644 tests/nouveau_crc.c
> 
> -- 
> 2.26.2
> 
Looks good, all the tests pass now that I've removed the serial cable.
For the series:

Reviewed-by: Jeremy Cline <jcline at redhat.com>

On Wed, Sep 30, 2020 at 01:31:46PM -0400, Lyude wrote:
> From: Lyude Paul <lyude at redhat.com>
> 
> As Petri Latvala pointed out, some of the documentation in this macro is
> mistakenly copied from the other igt_assert*() macros. Let's fix that.
> 
> Signed-off-by: Lyude Paul <lyude at redhat.com>
Reviewed-by: Petri Latvala <petri.latvala at intel.com>

> ---
>  lib/igt_core.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index c5871520..e74ede8b 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -966,8 +966,8 @@ void igt_describe_f(const char *fmt, ...);
>   *
>   * Fails (sub-) test if the given file descriptor is invalid.
>   *
> - * Like igt_assert(), but displays the values being compared on failure
instead
> - * of simply printing the stringified expression.
> + * Like igt_assert(), but displays the stringified identifier that was
supposed
> + * to contain a valid fd on failure.
>   */
>  #define igt_assert_fd(fd) \
>  	igt_assert_f(fd >= 0, "file descriptor " #fd "
failed\n");
> -- 
> 2.26.2
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

On Wed, Sep 30, 2020 at 01:31:47PM -0400, Lyude wrote:
> From: Lyude Paul <lyude at redhat.com>
> 
> Like igt_assert_fd(), but using igt_require() instead
> 
> Changes since v1:
> * Fix documentation error in igt_require_fd() - Petri Latvala
> 
> Signed-off-by: Lyude Paul <lyude at redhat.com>
Reviewed-by: Petri Latvala <petri.latvala at intel.com>

> ---
>  lib/igt_core.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index e74ede8b..5d835260 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -1021,6 +1021,18 @@ void igt_describe_f(const char *fmt, ...);
>  	else igt_debug("Test requirement passed: %s\n", #expr); \
>  } while (0)
>  
> +/**
> + * igt_require_fd:
> + * @fd: file descriptor
> + *
> + * Skips (sub-) test if the given file descriptor is invalid.
> + *
> + * Like igt_require(), but displays the stringified identifier that was
supposed
> + * to contain a valid fd on failure.
> + */
> +#define igt_require_fd(fd) \
> +	igt_require_f(fd >= 0, "file descriptor " #fd "
failed\n");
> +
>  /**
>   * igt_skip_on_f:
>   * @expr: condition to test
> -- 
> 2.26.2
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev