Hi Kamil, Thanks for review. On Monday, 15 May 2023 22:02:51 CEST Kamil Konieczny wrote: > Hi Janusz, > > On 2023-05-15 at 10:50:20 +0200, 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. > > > > +cc kms dev Bhanuprakash Modem OK. > > > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/7648 > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxxxxxxxx> > > --- > > tests/i915/kms_fb_coherency.c | 354 ++++++++++++++++++++++++++++++++++ > > tests/meson.build | 1 + > > 2 files changed, 355 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..9223f13b05 > > --- /dev/null > > +++ b/tests/i915/kms_fb_coherency.c > > @@ -0,0 +1,354 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright © 2023 Intel Corporation > > + * > > + * Derived from tests/i915/kms_pwrite_crc.c > > + * Copyright © 2014 Intel Corporation > > imho you do not need above two lines, you already wrote this > in description. OK. > > > + */ > > + > > +/** > > + * TEST: kms_fb_coherency > > + * Description: Exercise coherency of future scanout buffer objects > > + */ > > + > > +#include "igt.h" > > Put this after sys includes below. Copy-pasted from kms_pwrite_crc.c. I'll fix it. > > > +#include <errno.h> > > +#include <limits.h> > > +#include <stdbool.h> > > +#include <stdio.h> > > +#include <string.h> > > + > > Here put any igt include(s). Yes. > > > + > > +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 igt_crc_t get_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 */ > ---------- ^ > Checks happen later, here you collect it. Right. > > > + igt_pipe_crc_collect_crc(data->pipe_crc, &crc); > > Add newline. OK. > > > + return 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]); > > + > > Remove empty line. OK. > > > +} > > + > > +static void test_mmap_gtt(data_t *data) > > +{ > > + igt_crc_t crc; > > + 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); > > + > > + crc = get_buf_crc(data, buf, fb); > > + igt_assert_crc_equal(&crc, &data->ref_crc); > > +} > > + > > +static void test_mmap_offset_wc(data_t *data) > > +{ > > + igt_crc_t crc; > > + 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); > > + > > + crc = get_buf_crc(data, buf, fb); > > + igt_assert_crc_equal(&crc, &data->ref_crc); > > +} > > + > > +static void test_mmap_offset_wb(data_t *data) > > +{ > > + igt_crc_t crc; > > + 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__cpu(data->drm_fd, fb->gem_handle, 0, fb->size, PROT_WRITE); > > + > > + crc = get_buf_crc(data, buf, fb); > > + igt_fail_on(igt_check_crc_equal(&crc, &data->ref_crc)); > > Why not use igt_assert_crc_equal ? Because I expected the opposite -- crc not equal with non-coherent caching modes in use. But please note that this function and the similar test_legacy_mmap_cpu() below are actually not used and should be dropped. I used them on trybot to verify if it is possible to get consistent results across different platforms when using non-coherent caching modes and I found it's not -- some succeeded, some were failing. Then, I decided to just drop non-coherent dynamic subtests, but forgot to drop the corresponding functions. Thanks, Janusz > > > +} > > + > > +static void test_mmap_offset_uc(data_t *data) > > +{ > > + igt_crc_t crc; > > + 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); > > + > > + crc = get_buf_crc(data, buf, fb); > > + igt_assert_crc_equal(&crc, &data->ref_crc); > > +} > > + > > +static void test_mmap_offset_fixed(data_t *data) > > +{ > > + igt_crc_t crc; > > + 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); > > + > > + crc = get_buf_crc(data, buf, fb); > > + igt_assert_crc_equal(&crc, &data->ref_crc); > > +} > > + > > +static void test_legacy_mmap_wc(data_t *data) > > +{ > > + igt_crc_t crc; > > + igt_fb_t *fb; > > + void *buf; > > + > > + igt_require(gem_has_legacy_mmap(data->drm_fd)); > > + igt_require(gem_mmap__has_wc(data->drm_fd)); > > + > > + fb = prepare_fb(data); > > + > > + /* mmap the fb */ > > + buf = gem_mmap__wc(data->drm_fd, fb->gem_handle, 0, fb->size, PROT_WRITE); > > + > > + crc = get_buf_crc(data, buf, fb); > > + igt_assert_crc_equal(&crc, &data->ref_crc); > > +} > > + > > +static void test_legacy_mmap_cpu(data_t *data) > > +{ > > + igt_crc_t crc; > > + igt_fb_t *fb; > > + void *buf; > > + > > + igt_require(gem_has_legacy_mmap(data->drm_fd)); > > + > > + fb = prepare_fb(data); > > + > > + /* mmap the fb */ > > + buf = gem_mmap__cpu(data->drm_fd, fb->gem_handle, 0, fb->size, PROT_WRITE); > > + > > + crc = get_buf_crc(data, buf, fb); > > + igt_fail_on(igt_check_crc_equal(&crc, &data->ref_crc)); > > Same here, use igt_assert_crc_equal ? > > Regards, > Kamil > > > +} > > + > > +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; > > + } > > + > > + 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)) { > > + 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 c15eb3a08c..371b501992 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', >
