[igt-dev] [PATCH i-g-t v4] tests: Add nouveau-crc tests
Jeremy Cline
jcline at redhat.com
Mon Sep 28 21:36:40 UTC 2020
Hi,
On Tue, Aug 18, 2020 at 05:00:51PM -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 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 | 397 ++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 411 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 684de043..92647991 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -74,6 +74,7 @@ test_progs = [
> 'kms_vblank',
> 'kms_vrr',
> '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..05c2f4de
> --- /dev/null
> +++ b/tests/nouveau_crc.c
> @@ -0,0 +1,397 @@
> +/*
> + * 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));
> +}
> +
> +static void create_colors(data_t *data,
> + struct color_fb *colors,
> + size_t len,
> + igt_pipe_crc_t *pipe_crc)
Not to bikeshed too much, but this function seems to be more about
generating CRCs given some pre-existing colors. Maybe
create_color_crcs() would be better? And it wouldn't hurt to throw a
little docblock on it, even something as short as:
/*
* Calculate and set the CRC for each color_fb
*/
> +{
> + 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_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);
> +}
> +
> +/* Hard-coded to PIPE_A for now, we don't really need to test this on more then
> + * one head
> + */
Perhaps expand this doc-block out a bit with the details you have in the
commit:
/* Test CRCs are reported across notifier context flips
*
* Nvidia GPUs store CRCs in a limited memory region called the notifier
* context. When the region fills, new CRCs are not reported. To work
* around this, two notifier contexts are allocated and the driver flips
* between the two of them when a threshold is reached. Even with this
* approach, a single frame is lost during the context flip.
*
* This test adjusts the threshold at which the notifier context flips
* to the other context and asserts the CRCs are correctly reported with
* the exception of the frame lost during the flip.
*
* Hard-coded to PIPE_A for now, we don't really need to test this on
* more then one head
*/
I was guilty of reading the code while testing without going back to
consult the commit message and I only just now realized the test purpose
was clearly explained. It'd be nice to keep that close to the test
definition rather than in the commit log.
Also, as far as I can tell it's not actually hard-coded to PIPE_A, but
test_ctx_flip_threshold_reset_after_capture() is. Maybe I'm
mis-understanding that?
> +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_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);
While testing I was experiencing some test failures, at which point I
turned on a bunch of debug logging on the kernel side. This led to this
test reliably failing because this call timed out waiting on the pageflip
event, which is unfortunate. I'm not sure what the larger usage of this
particular function is, but having tests start to fail when verbose
logging is enabled isn't ideal.
Obviously hanging forever is also not ideal, so maybe the timeout of
this function just needs adjustment, or even just have the assertion
include a hint that perhaps debug logging is slowing things down too
much. It's not within the scope of this change so I don't think anything
needs to change in this patch, more just wanted to see what folks think
the best approach is since I spent longer than I care to admit figuring
that out and don't want others to have to suffer the same way.
> + }
> +
> + igt_pipe_crc_get_crcs(pipe_crc, n_crcs, &crcs);
> + igt_pipe_crc_stop(pipe_crc);
> +
> + /*
> + * Find the first color in our pattern with a CRC that differs from the
> + * last CRC, so we can use it to find the start of the pattern
> + */
It might be worth explicitly noting this is to avoid CRC collisions (I'm
assuming) since I had to ponder for a bit:
/*
* Guard against CRC collisions in the color framebuffers. Find the
* first color in our pattern with a CRC that differs from the last CRC,
* so we can use it 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 */
> + 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);
This consistently fails for me. The reason is that there are multiple
frames with the same color/CRC. As far as I can tell, that's perfectly
reasonable when a page flip is slow (possibly due to debug logs 😬), but
I don't know for certain what the interface expectations are.
If that assumption is correct, I guess you'll need to expect either the
same color as the previous frame, or the next color in the array after
the color in the previous frame.
> + }
> + igt_assert(found_skip);
> +
> + free(crcs);
> + igt_pipe_crc_free(pipe_crc);
> + destroy_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_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_colors(data, colors, n_colors);
> +}
> +
Maybe a short doc-block with what the test should be doing here would be
good:
/*
* Assert that after a CRC capture is started and stopped, the context
* notifier flip threshold is reset to its default value.
*/
> +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_pipe_crc_stop(pipe_crc);
> +
> + igt_assert_neq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
As it currently stands, this test could fail if the default is 5, or it
could pass if "flip_threshold" was broken and didn't report the actual
flip_threshold anyway. The default being 5 doesn't look to be something
that you necessarily need to be worried about, but it might be worth
reading the value back after setting the threshold to make sure you do
see 5.
> + igt_pipe_crc_free(pipe_crc);
> +}
> +
Same note about a docblock 🙂:
/*
* Assert that, for a given source, two consecutive CRCs are equal if
* the content hasn't changed.
*/
> +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);
> +}
> +
I'm not as certain about what exactly the "outp-inactive" source (or the
other CRC sources) is so I'm not sure what to put in the doc-block that
I'd recommend putting here.
> +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_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_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);
> + 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_output_set_pipe(data.output, PIPE_NONE);
> + igt_display_commit(&data.display);
> + igt_remove_fb(data.drm_fd, &data.default_fb);
> + close(data.nv_crc_dir);
> + }
> + }
> + igt_fixture
> + igt_display_fini(&data.display);
> +
> +}
> --
> 2.26.2
>
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
More information about the igt-dev
mailing list