[Intel-gfx,1/2] drm/i915: Use CPU mapping for userspace dma-buf mmap()
diff mbox

Message ID 55C12EF1.7080701@intel.com
State New
Headers show

Commit Message

Tiago Vignatti Aug. 4, 2015, 9:30 p.m. UTC
On 07/31/2015 06:02 PM, Chris Wilson wrote:
>
> The first problem is that llc does not guarrantee that the buffer is
> cache coherent with all aspects of the GPU. For scanout and similar
> writes need to be WC.
>
> if (obj->has_framebuffer_references) would at least catch where the fb
> is made before the mmap.
>
> Equally this buffer could then be shared with other devices and exposing
> a CPU mmap to userspace (and no flush/set-domain protocol) will result in
> corruption.

I've built an igt test to catch this corruption but it's not really 
falling there in my IvyBridge. If what you described is right (and so 
what I coded) then this test should write in the mapped buffer but not 
update the screen.

Any idea what's going on?

https://github.com/tiagovignatti/intel-gpu-tools/commit/3e130ac2b274f1a3f68855559c78cb72d0673ca2.patch


 From 3e130ac2b274f1a3f68855559c78cb72d0673ca2 Mon Sep 17 00:00:00 2001
From: Tiago Vignatti <tiago.vignatti@intel.com>
Date: Tue, 4 Aug 2015 13:38:09 -0300
Subject: [PATCH] tests: Add prime_crc for cache coherency

This program can be used to detect when the writes don't land in 
scanout, due
cache incoherency.

Run it like ./prime_crc --interactive-debug=crc

Signed-off-by: Tiago Vignatti <tiago.vignatti@intel.com>
---
  tests/.gitignore       |   1 +
  tests/Makefile.sources |   1 +
  tests/prime_crc.c      | 201 
+++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 203 insertions(+)
  create mode 100644 tests/prime_crc.c

+igt_main
+{
+	igt_fixture
+		setup_environment();
+
+	igt_subtest_f("draw-method-tiled")
+		draw_method_subtest(LOCAL_I915_FORMAT_MOD_X_TILED);
+
+	igt_fixture
+		teardown_environment();
+}

Comments

Daniel Vetter Aug. 5, 2015, 7:08 a.m. UTC | #1
On Tue, Aug 04, 2015 at 06:30:25PM -0300, Tiago Vignatti wrote:
> On 07/31/2015 06:02 PM, Chris Wilson wrote:
> >
> >The first problem is that llc does not guarrantee that the buffer is
> >cache coherent with all aspects of the GPU. For scanout and similar
> >writes need to be WC.
> >
> >if (obj->has_framebuffer_references) would at least catch where the fb
> >is made before the mmap.
> >
> >Equally this buffer could then be shared with other devices and exposing
> >a CPU mmap to userspace (and no flush/set-domain protocol) will result in
> >corruption.
> 
> I've built an igt test to catch this corruption but it's not really falling
> there in my IvyBridge. If what you described is right (and so what I coded)
> then this test should write in the mapped buffer but not update the screen.
> 
> Any idea what's going on?
> 
> https://github.com/tiagovignatti/intel-gpu-tools/commit/3e130ac2b274f1a3f68855559c78cb72d0673ca2.patch
> 
> 
> From 3e130ac2b274f1a3f68855559c78cb72d0673ca2 Mon Sep 17 00:00:00 2001
> From: Tiago Vignatti <tiago.vignatti@intel.com>
> Date: Tue, 4 Aug 2015 13:38:09 -0300
> Subject: [PATCH] tests: Add prime_crc for cache coherency
> 
> This program can be used to detect when the writes don't land in scanout,
> due
> cache incoherency.
> 
> Run it like ./prime_crc --interactive-debug=crc
> 
> Signed-off-by: Tiago Vignatti <tiago.vignatti@intel.com>
> ---
>  tests/.gitignore       |   1 +
>  tests/Makefile.sources |   1 +
>  tests/prime_crc.c      | 201
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 203 insertions(+)
>  create mode 100644 tests/prime_crc.c
> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 5bc4a58..96dbf57 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -160,6 +160,7 @@ pm_rc6_residency
>  pm_rpm
>  pm_rps
>  pm_sseu
> +prime_crc
>  prime_nv_api
>  prime_nv_pcopy
>  prime_nv_test
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 5b2072e..c05b5a7 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -90,6 +90,7 @@ TESTS_progs_M = \
>  	pm_rps \
>  	pm_rc6_residency \
>  	pm_sseu \
> +	prime_crc \
>  	prime_mmap \
>  	prime_self_import \
>  	template \
> diff --git a/tests/prime_crc.c b/tests/prime_crc.c
> new file mode 100644
> index 0000000..3474cc9
> --- /dev/null
> +++ b/tests/prime_crc.c
> @@ -0,0 +1,201 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * 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.
> + *
> + * Authors:
> + *    Tiago Vignatti <tiago.vignatti at intel.com>
> + *
> + */
> +
> +/* This program can detect when the writes don't land in scanout, due cache
> + * incoherency. */
> +
> +#include "drmtest.h"
> +#include "igt_debugfs.h"
> +#include "igt_kms.h"
> +
> +#define MAX_CONNECTORS 32
> +
> +struct modeset_params {
> +	uint32_t crtc_id;
> +	uint32_t connector_id;
> +	drmModeModeInfoPtr mode;
> +};
> +
> +int drm_fd;
> +drmModeResPtr drm_res;
> +drmModeConnectorPtr drm_connectors[MAX_CONNECTORS];
> +drm_intel_bufmgr *bufmgr;
> +igt_pipe_crc_t *pipe_crc;
> +
> +struct modeset_params ms;
> +
> +static void find_modeset_params(void)
> +{
> +	int i;
> +	uint32_t connector_id = 0, crtc_id;
> +	drmModeModeInfoPtr mode = NULL;
> +
> +	for (i = 0; i < drm_res->count_connectors; i++) {
> +		drmModeConnectorPtr c = drm_connectors[i];
> +
> +		if (c->count_modes) {
> +			connector_id = c->connector_id;
> +			mode = &c->modes[0];
> +			break;
> +		}
> +	}
> +	igt_require(connector_id);
> +
> +	crtc_id = drm_res->crtcs[0];
> +	igt_assert(crtc_id);
> +	igt_assert(mode);
> +
> +	ms.connector_id = connector_id;
> +	ms.crtc_id = crtc_id;
> +	ms.mode = mode;
> +
> +}
> +
> +#define BO_SIZE (16*1024)
> +
> +char pattern[] = {0xff, 0x00, 0x00, 0x00,
> +	0x00, 0xff, 0x00, 0x00,
> +	0x00, 0x00, 0xff, 0x00,
> +	0x00, 0x00, 0x00, 0xff};
> +
> +static void mess_with_coherency(char *ptr)
> +{
> +	off_t i;
> +
> +	for (i = 0; i < BO_SIZE; i+=sizeof(pattern)) {
> +		memcpy(ptr + i, pattern, sizeof(pattern));
> +	}
> +//	munmap(ptr, BO_SIZE);
> +//	close(dma_buf_fd);
> +}
> +
> +static char *dmabuf_mmap_framebuffer(struct igt_fb *fb)
> +{
> +	int dma_buf_fd;
> +	char *ptr = NULL;
> +
> +	dma_buf_fd = prime_handle_to_fd(drm_fd, fb->gem_handle);
> +	igt_assert(errno == 0);
> +
> +	ptr = mmap(NULL, BO_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, dma_buf_fd,
> 0);
> +	igt_assert(ptr != MAP_FAILED);
> +
> +	return ptr;
> +}
> +
> +static void get_method_crc(uint64_t tiling, igt_crc_t *crc, bool mess)
> +{
> +	struct igt_fb fb;
> +	int rc;
> +	char *ptr;
> +
> +	igt_create_fb(drm_fd, ms.mode->hdisplay, ms.mode->vdisplay,
> +		      DRM_FORMAT_XRGB8888, tiling, &fb);
> +
> +	if (mess)
> +		ptr = dmabuf_mmap_framebuffer(&fb);
> +
> +	rc = drmModeSetCrtc(drm_fd, ms.crtc_id, fb.fb_id, 0, 0,
> +			    &ms.connector_id, 1, ms.mode);
> +	igt_assert(rc == 0);
> +
> +	if (mess)
> +		mess_with_coherency(ptr);
> +
> +	igt_pipe_crc_collect_crc(pipe_crc, crc);
> +
> +	kmstest_unset_all_crtcs(drm_fd, drm_res);
> +	igt_remove_fb(drm_fd, &fb);
> +}
> +
> +static void draw_method_subtest(uint64_t tiling)
> +{
> +	igt_crc_t reference_crc, crc;
> +
> +	kmstest_unset_all_crtcs(drm_fd, drm_res);
> +
> +	find_modeset_params();
> +
> +	get_method_crc(tiling, &reference_crc, false);
> +	get_method_crc(tiling, &crc, true);
> +
> +	// XXX: IIUC if we mess up with the scanout device, through a dma-buf
> mmap'ed
> +	// pointer, then both the reference crc and the messed up one should be
> equal
> +	// because the latter wasn't flushed. That's the theory, but it's not
> what's
> +	// happening and the following is not passing.

Nah they don't have to be equal since the problem isn't that nothing goes
out to memory where the display can see it, but usually only parts of it.
I.e. you need to change your test to
- draw black screen (it starts that way so nothing to do really), grab crtc
- draw white screen and make sure you flush correctly, don't bother with
  crc (we can't test for inequality
  because collisions are too easy)
- draw black screen again without flushing, grab crc

Then assert that your two crc will be inequal (which they shouldn't be
because some cachelines will still be stuck). Maybe also add a delay
somewhere so you can see the cacheline dirt pattern, it's very
characteristic.
-Daniel

> +	igt_assert_crc_equal(&reference_crc, &crc);
> +}
> +
> +static void setup_environment(void)
> +{
> +	int i;
> +
> +	drm_fd = drm_open_any_master();
> +	igt_require(drm_fd >= 0);
> +
> +	drm_res = drmModeGetResources(drm_fd);
> +	igt_assert(drm_res->count_connectors <= MAX_CONNECTORS);
> +
> +	for (i = 0; i < drm_res->count_connectors; i++)
> +		drm_connectors[i] = drmModeGetConnector(drm_fd,
> +							drm_res->connectors[i]);
> +
> +	kmstest_set_vt_graphics_mode();
> +
> +	bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
> +	igt_assert(bufmgr);
> +	drm_intel_bufmgr_gem_enable_reuse(bufmgr);
> +
> +	pipe_crc = igt_pipe_crc_new(0, INTEL_PIPE_CRC_SOURCE_AUTO);
> +}
> +
> +static void teardown_environment(void)
> +{
> +	int i;
> +
> +	igt_pipe_crc_free(pipe_crc);
> +
> +	drm_intel_bufmgr_destroy(bufmgr);
> +
> +	for (i = 0; i < drm_res->count_connectors; i++)
> +		drmModeFreeConnector(drm_connectors[i]);
> +
> +	drmModeFreeResources(drm_res);
> +	close(drm_fd);
> +}
> +
> +igt_main
> +{
> +	igt_fixture
> +		setup_environment();
> +
> +	igt_subtest_f("draw-method-tiled")
> +		draw_method_subtest(LOCAL_I915_FORMAT_MOD_X_TILED);
> +
> +	igt_fixture
> +		teardown_environment();
> +}

Patch
diff mbox

diff --git a/tests/.gitignore b/tests/.gitignore
index 5bc4a58..96dbf57 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -160,6 +160,7 @@  pm_rc6_residency
  pm_rpm
  pm_rps
  pm_sseu
+prime_crc
  prime_nv_api
  prime_nv_pcopy
  prime_nv_test
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 5b2072e..c05b5a7 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -90,6 +90,7 @@  TESTS_progs_M = \
  	pm_rps \
  	pm_rc6_residency \
  	pm_sseu \
+	prime_crc \
  	prime_mmap \
  	prime_self_import \
  	template \
diff --git a/tests/prime_crc.c b/tests/prime_crc.c
new file mode 100644
index 0000000..3474cc9
--- /dev/null
+++ b/tests/prime_crc.c
@@ -0,0 +1,201 @@ 
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * 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.
+ *
+ * Authors:
+ *    Tiago Vignatti <tiago.vignatti at intel.com>
+ *
+ */
+
+/* This program can detect when the writes don't land in scanout, due cache
+ * incoherency. */
+
+#include "drmtest.h"
+#include "igt_debugfs.h"
+#include "igt_kms.h"
+
+#define MAX_CONNECTORS 32
+
+struct modeset_params {
+	uint32_t crtc_id;
+	uint32_t connector_id;
+	drmModeModeInfoPtr mode;
+};
+
+int drm_fd;
+drmModeResPtr drm_res;
+drmModeConnectorPtr drm_connectors[MAX_CONNECTORS];
+drm_intel_bufmgr *bufmgr;
+igt_pipe_crc_t *pipe_crc;
+
+struct modeset_params ms;
+
+static void find_modeset_params(void)
+{
+	int i;
+	uint32_t connector_id = 0, crtc_id;
+	drmModeModeInfoPtr mode = NULL;
+
+	for (i = 0; i < drm_res->count_connectors; i++) {
+		drmModeConnectorPtr c = drm_connectors[i];
+
+		if (c->count_modes) {
+			connector_id = c->connector_id;
+			mode = &c->modes[0];
+			break;
+		}
+	}
+	igt_require(connector_id);
+
+	crtc_id = drm_res->crtcs[0];
+	igt_assert(crtc_id);
+	igt_assert(mode);
+
+	ms.connector_id = connector_id;
+	ms.crtc_id = crtc_id;
+	ms.mode = mode;
+
+}
+
+#define BO_SIZE (16*1024)
+
+char pattern[] = {0xff, 0x00, 0x00, 0x00,
+	0x00, 0xff, 0x00, 0x00,
+	0x00, 0x00, 0xff, 0x00,
+	0x00, 0x00, 0x00, 0xff};
+
+static void mess_with_coherency(char *ptr)
+{
+	off_t i;
+
+	for (i = 0; i < BO_SIZE; i+=sizeof(pattern)) {
+		memcpy(ptr + i, pattern, sizeof(pattern));
+	}
+//	munmap(ptr, BO_SIZE);
+//	close(dma_buf_fd);
+}
+
+static char *dmabuf_mmap_framebuffer(struct igt_fb *fb)
+{
+	int dma_buf_fd;
+	char *ptr = NULL;
+
+	dma_buf_fd = prime_handle_to_fd(drm_fd, fb->gem_handle);
+	igt_assert(errno == 0);
+
+	ptr = mmap(NULL, BO_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, 
dma_buf_fd, 0);
+	igt_assert(ptr != MAP_FAILED);
+
+	return ptr;
+}
+
+static void get_method_crc(uint64_t tiling, igt_crc_t *crc, bool mess)
+{
+	struct igt_fb fb;
+	int rc;
+	char *ptr;
+
+	igt_create_fb(drm_fd, ms.mode->hdisplay, ms.mode->vdisplay,
+		      DRM_FORMAT_XRGB8888, tiling, &fb);
+
+	if (mess)
+		ptr = dmabuf_mmap_framebuffer(&fb);
+
+	rc = drmModeSetCrtc(drm_fd, ms.crtc_id, fb.fb_id, 0, 0,
+			    &ms.connector_id, 1, ms.mode);
+	igt_assert(rc == 0);
+
+	if (mess)
+		mess_with_coherency(ptr);
+
+	igt_pipe_crc_collect_crc(pipe_crc, crc);
+
+	kmstest_unset_all_crtcs(drm_fd, drm_res);
+	igt_remove_fb(drm_fd, &fb);
+}
+
+static void draw_method_subtest(uint64_t tiling)
+{
+	igt_crc_t reference_crc, crc;
+
+	kmstest_unset_all_crtcs(drm_fd, drm_res);
+
+	find_modeset_params();
+
+	get_method_crc(tiling, &reference_crc, false);
+	get_method_crc(tiling, &crc, true);
+
+	// XXX: IIUC if we mess up with the scanout device, through a dma-buf 
mmap'ed
+	// pointer, then both the reference crc and the messed up one should 
be equal
+	// because the latter wasn't flushed. That's the theory, but it's not 
what's
+	// happening and the following is not passing.
+	igt_assert_crc_equal(&reference_crc, &crc);
+}
+
+static void setup_environment(void)
+{
+	int i;
+
+	drm_fd = drm_open_any_master();
+	igt_require(drm_fd >= 0);
+
+	drm_res = drmModeGetResources(drm_fd);
+	igt_assert(drm_res->count_connectors <= MAX_CONNECTORS);
+
+	for (i = 0; i < drm_res->count_connectors; i++)
+		drm_connectors[i] = drmModeGetConnector(drm_fd,
+							drm_res->connectors[i]);
+
+	kmstest_set_vt_graphics_mode();
+
+	bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
+	igt_assert(bufmgr);
+	drm_intel_bufmgr_gem_enable_reuse(bufmgr);
+
+	pipe_crc = igt_pipe_crc_new(0, INTEL_PIPE_CRC_SOURCE_AUTO);
+}
+
+static void teardown_environment(void)
+{
+	int i;
+
+	igt_pipe_crc_free(pipe_crc);
+
+	drm_intel_bufmgr_destroy(bufmgr);
+
+	for (i = 0; i < drm_res->count_connectors; i++)
+		drmModeFreeConnector(drm_connectors[i]);
+
+	drmModeFreeResources(drm_res);
+	close(drm_fd);
+}
+