[Intel-gfx] [PATCH i-g-t v2] tests/i915: Exercise coherency of mmapped frame buffers
Janusz Krzysztofik
janusz.krzysztofik at linux.intel.com
Tue May 16 15:50:18 UTC 2023
Hi Andrzej,
Thanks for review.
On Tuesday, 16 May 2023 16:08:26 CEST Andrzej Hajda wrote:
> On 16.05.2023 12:05, Janusz Krzysztofik wrote:
> > Visible glitches have been observed when running graphics applications on
> > Linux under Xen hypervisor. Those observations have been confirmed with
> > failures from kms_pwrite_crc IGT test that verifies data coherency of DRM
> > frame buffer objects using hardware CRC checksums calculated by display
> > controllers, exposed to userspace via debugfs. Since not all applications
> > exhibit the issue, we need to exercise more methods than just pwrite in
> > order to identify all affected processing paths.
> >
> > Create a new test focused on exercising coherency of future scanout
> > buffers populated over mmap. Cover all available mmap methods and caching
> > modes expected to be device coherent.
> >
> > v2: Drop unused functions -- left-overs from unsuccessful negative subtest
> > attempts requiring consistent crc mismatches in non-coherent modes,
> > - since all subtests now call igt_assert_crc_equal(), move it from
> > subtest bodies to an updated and renamed helper,
> > - drop "derived from ..." info from copyrights comment (Kamil),
> > - fix order of includes (Kamil),
> > - fix whitespace (Kamil),
> > - Cc: Bhanuprakash (Kamil).
> >
> > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/7648
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > Cc: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > ---
> > tests/i915/kms_fb_coherency.c | 305 ++++++++++++++++++++++++++++++++++
> > tests/meson.build | 1 +
> > 2 files changed, 306 insertions(+)
> > create mode 100644 tests/i915/kms_fb_coherency.c
> >
> > diff --git a/tests/i915/kms_fb_coherency.c b/tests/i915/kms_fb_coherency.c
> > new file mode 100644
> > index 0000000000..b3f055c2b1
> > --- /dev/null
> > +++ b/tests/i915/kms_fb_coherency.c
> > @@ -0,0 +1,305 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2023 Intel Corporation
> > + */
> > +
> > +/**
> > + * TEST: kms_fb_coherency
> > + * Description: Exercise coherency of future scanout buffer objects
> > + */
> > +
> > +#include <errno.h>
> > +#include <limits.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +
> > +#include "igt.h"
> > +
> > +typedef struct {
> > + int drm_fd;
> > + igt_display_t display;
> > + struct igt_fb fb[2];
> > + igt_output_t *output;
> > + igt_plane_t *primary;
> > + enum pipe pipe;
> > + igt_crc_t ref_crc;
> > + igt_pipe_crc_t *pipe_crc;
> > + uint32_t devid;
> > +} data_t;
> > +
> > +static void prepare_crtc(data_t *data)
> > +{
> > + igt_display_t *display = &data->display;
> > + igt_output_t *output = data->output;
> > + drmModeModeInfo *mode;
> > +
> > + igt_display_reset(display);
> > + /* select the pipe we want to use */
> > + igt_output_set_pipe(output, data->pipe);
> > +
> > + mode = igt_output_get_mode(output);
> > +
> > + /* create a white reference fb and flip to it */
> > + igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> > + DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> > + 1.0, 1.0, 1.0, &data->fb[0]);
> > +
> > + data->primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> > +
> > + igt_plane_set_fb(data->primary, &data->fb[0]);
> > + igt_display_commit(display);
> > +
> > + if (data->pipe_crc)
> > + igt_pipe_crc_free(data->pipe_crc);
> > +
> > + data->pipe_crc = igt_pipe_crc_new(data->drm_fd, data->pipe,
> > + IGT_PIPE_CRC_SOURCE_AUTO);
> > +
> > + /* get reference crc for the white fb */
> > + igt_pipe_crc_collect_crc(data->pipe_crc, &data->ref_crc);
> > +}
> > +
> > +static struct igt_fb *prepare_fb(data_t *data)
> > +{
> > + igt_output_t *output = data->output;
> > + struct igt_fb *fb = &data->fb[1];
> > + drmModeModeInfo *mode;
> > +
> > + prepare_crtc(data);
> > +
> > + mode = igt_output_get_mode(output);
> > +
> > + /* create a non-white fb we can overwrite later */
> > + igt_create_pattern_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> > + DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR, fb);
> > +
> > + /* flip to it to make it UC/WC and fully flushed */
> > + drmModeSetPlane(data->drm_fd,
> > + data->primary->drm_plane->plane_id,
> > + output->config.crtc->crtc_id,
> > + fb->fb_id, 0,
> > + 0, 0, fb->width, fb->height,
> > + 0, 0, fb->width << 16, fb->height << 16);
> > +
> > + /* flip back the original white buffer */
> > + drmModeSetPlane(data->drm_fd,
> > + data->primary->drm_plane->plane_id,
> > + output->config.crtc->crtc_id,
> > + data->fb[0].fb_id, 0,
> > + 0, 0, fb->width, fb->height,
> > + 0, 0, fb->width << 16, fb->height << 16);
> > +
> > + if (!gem_has_lmem(data->drm_fd)) {
> > + uint32_t caching;
> > +
> > + /* make sure caching mode has become UC/WT */
> > + caching = gem_get_caching(data->drm_fd, fb->gem_handle);
> > + igt_assert(caching == I915_CACHING_NONE ||
> > + caching == I915_CACHING_DISPLAY);
> > + }
> > +
> > + return fb;
> > +}
> > +
> > +static void check_buf_crc(data_t *data, void *buf, igt_fb_t *fb)
> > +{
> > + igt_crc_t crc;
> > +
> > + /* use memset to make the mmapped fb all white */
> > + memset(buf, 0xff, fb->size);
> > + munmap(buf, fb->size);
> > +
> > + /* and flip to it */
> > + drmModeSetPlane(data->drm_fd,
> > + data->primary->drm_plane->plane_id,
> > + data->output->config.crtc->crtc_id,
> > + fb->fb_id, 0,
> > + 0, 0, fb->width, fb->height,
> > + 0, 0, fb->width << 16, fb->height << 16);
> > +
> > + /* check that the crc is as expected, which requires that caches got flushed */
> > + igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
> > + igt_assert_crc_equal(&crc, &data->ref_crc);
> > +}
> > +
> > +static void cleanup_crtc(data_t *data)
> > +{
> > + igt_display_t *display = &data->display;
> > + igt_output_t *output = data->output;
> > +
> > + igt_pipe_crc_free(data->pipe_crc);
> > + data->pipe_crc = NULL;
> > +
> > + igt_plane_set_fb(data->primary, NULL);
> > +
> > + igt_output_set_pipe(output, PIPE_ANY);
> > + igt_display_commit(display);
> > +
> > + igt_remove_fb(data->drm_fd, &data->fb[0]);
> > + igt_remove_fb(data->drm_fd, &data->fb[1]);
> > +}
> > +
> > +static void test_mmap_gtt(data_t *data)
> > +{
> > + igt_fb_t *fb;
> > + void *buf;
> > +
> > + gem_require_mappable_ggtt(data->drm_fd);
> > +
> > + fb = prepare_fb(data);
> > +
> > + buf = gem_mmap__gtt(data->drm_fd, fb->gem_handle, fb->size, PROT_WRITE);
> > +
> > + check_buf_crc(data, buf, fb);
> > +}
> > +
> > +static void test_mmap_offset_wc(data_t *data)
> > +{
> > + igt_fb_t *fb;
> > + void *buf;
> > +
> > + igt_require(gem_mmap_offset__has_wc(data->drm_fd));
> > +
> > + fb = prepare_fb(data);
> > +
> > + buf = gem_mmap_offset__wc(data->drm_fd, fb->gem_handle, 0, fb->size, PROT_WRITE);
> > +
> > + check_buf_crc(data, buf, fb);
> > +}
> > +
> > +static void test_mmap_offset_uc(data_t *data)
> > +{
> > + igt_fb_t *fb;
> > + void *buf;
> > +
> > + igt_require(gem_has_mmap_offset(data->drm_fd));
> > + igt_skip_on(gem_has_lmem(data->drm_fd));
> > +
> > + fb = prepare_fb(data);
> > +
> > + /* mmap the fb */
> > + buf = __gem_mmap_offset(data->drm_fd, fb->gem_handle, 0, fb->size, PROT_WRITE,
> > + I915_MMAP_OFFSET_UC);
> > + igt_assert(buf);
> > +
> > + check_buf_crc(data, buf, fb);
> > +}
> > +
> > +static void test_mmap_offset_fixed(data_t *data)
> > +{
> > + igt_fb_t *fb;
> > + void *buf;
> > +
> > + igt_require(gem_has_lmem(data->drm_fd));
> > +
> > + fb = prepare_fb(data);
> > +
> > + /* mmap the fb */
> > + buf = gem_mmap_offset__fixed(data->drm_fd, fb->gem_handle, 0, fb->size, PROT_WRITE);
> > +
> > + check_buf_crc(data, buf, fb);
> > +}
> > +
> > +static void test_legacy_mmap_wc(data_t *data)
> > +{
> > + igt_fb_t *fb;
> > + void *buf;
> > +
> > + igt_require(gem_has_legacy_mmap(data->drm_fd));
> > + igt_require(gem_mmap__has_wc(data->drm_fd));
>
>
> Do we need these check if they are already performed below? Just asking :)
No, not really :)
>
> > +
> > + fb = prepare_fb(data);
> > +
> > + /* mmap the fb */
> > + buf = gem_mmap__wc(data->drm_fd, fb->gem_handle, 0, fb->size, PROT_WRITE);
> > +
> > + check_buf_crc(data, buf, fb);
> > +}
> > +
> > +static void select_valid_pipe_output_combo(data_t *data)
> > +{
> > + igt_display_t *display = &data->display;
> > +
> > + for_each_pipe_with_valid_output(display, data->pipe, data->output) {
> > + igt_display_reset(display);
> > +
> > + igt_output_set_pipe(data->output, data->pipe);
> > + if (!i915_pipe_output_combo_valid(display))
> > + continue;
> > +
> > + /* one is enough */
> > + return;
>
> if (i915_pipe_output_combo_valid(display))
> return;
>
> is shorter
Indeed.
>
> > + }
> > +
> > + igt_skip("no valid crtc/connector combinations found\n");
> > +}
> > +
> > +igt_main
> > +{
> > + data_t data;
> > +
> > + igt_fixture {
> > + data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
> > +
> > + data.devid = intel_get_drm_devid(data.drm_fd);
> > +
> > + kmstest_set_vt_graphics_mode();
> > +
> > + igt_require_pipe_crc(data.drm_fd);
> > +
> > + igt_display_require(&data.display, data.drm_fd);
> > +
> > + select_valid_pipe_output_combo(&data);
> > + }
> > +
> > + /**
> > + * SUBTEST: memset-crc
> > + * Description: Use display controller CRC hardware to validate (non)coherency
> > + * of memset operations on future scanout buffer objects
> > + * mmapped with different mmap methods and different caching modes.
> > + */
> > + igt_subtest_with_dynamic("memset-crc") {
> > + if (gem_has_mappable_ggtt(data.drm_fd)) {
> > + igt_dynamic("mmap-gtt")
> > + test_mmap_gtt(&data);
> > +
> > + cleanup_crtc(&data);
> > + }
> > +
> > + if (gem_mmap_offset__has_wc(data.drm_fd)) {
>
> I wonder how these checks will work with static .testlist?
There were two options:
a) a static list of subtests, always executed and skipping if not supported,
b) a dynamically generated list of dynamic sub-subtests, based on platform
capabilities.
I think that b) will work with static .testlists just as other dynamic tests
that iterate over e.g. engines or memomry regions do -- here we "iterate" over
available mmap methods / caching modes. I've chosen b) because I think that
it produces less noise in CI, however I'm open to switching to a static list
of subtests.
>
>
> Reviewed-by: Andrzej Hajda <andrzej.hajda at intel.com>
If there are no requests for switching to a static list of subtests, nor any
other comments, I'll drop the above pointed out redundant checks from subtest
bodies, apply your continue -> return simplification, and use your R-b.
Thanks,
Janusz
>
> Regards
> Andrzej
>
> > + igt_dynamic("mmap-offset-wc")
> > + test_mmap_offset_wc(&data);
> > +
> > + cleanup_crtc(&data);
> > + }
> > +
> > + if (gem_has_mmap_offset(data.drm_fd)) {
> > + if (gem_has_lmem(data.drm_fd)) {
> > + igt_dynamic("mmap-offset-fixed")
> > + test_mmap_offset_fixed(&data);
> > +
> > + cleanup_crtc(&data);
> > +
> > + } else {
> > + igt_dynamic("mmap-offset-uc")
> > + test_mmap_offset_uc(&data);
> > +
> > + cleanup_crtc(&data);
> > + }
> > + }
> > +
> > + if (gem_has_legacy_mmap(data.drm_fd) &&
> > + gem_mmap__has_wc(data.drm_fd)) {
> > + igt_dynamic("mmap-legacy-wc")
> > + test_legacy_mmap_wc(&data);
> > +
> > + cleanup_crtc(&data);
> > + }
> > + }
> > +
> > + igt_fixture {
> > + igt_display_fini(&data.display);
> > + close(data.drm_fd);
> > + }
> > +}
> > diff --git a/tests/meson.build b/tests/meson.build
> > index 38f080f7c2..f71be1dbe5 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -224,6 +224,7 @@ i915_progs = [
> > 'kms_ccs',
> > 'kms_cdclk',
> > 'kms_draw_crc',
> > + 'kms_fb_coherency',
> > 'kms_fbcon_fbt',
> > 'kms_fence_pin_leak',
> > 'kms_flip_scaled_crc',
>
>
More information about the Intel-gfx
mailing list