[PATCH 0/2] mmap on the dma-buf directly

[PATCH 0/2] mmap on the dma-buf directly

[Tiago Vignatti]

Hi,

I've tested these two patches (in drm-intel-nightly, but also in CrOS kernel
v3.14) and they seem just enough for what we want to do: the idea is to create
a GEM bo in one process and pass the prime handle of the it to another
process, which in turn uses the handle only to map and write. This could be
useful for Chrome OS  architecture, where the Web content ("unpriviledged
process") maps and CPU-draws a buffer, which was previously allocated in the
GPU process ("priviledged process").

I'm using a modified igt mostly to test these things. PTAL here:
https://github.com/tiagovignatti/intel-gpu-tools/commits/prime_mmap

Thank you,

Tiago


Daniel Thompson (1):
  drm: prime: Honour O_RDWR during prime-handle-to-fd

Tiago Vignatti (1):
  drm/i915: Use CPU mapping for userspace dma-buf mmap()

 drivers/gpu/drm/drm_prime.c            | 10 +++-------
 drivers/gpu/drm/i915/i915_gem_dmabuf.c | 21 ++++++++++++++++++++-
 include/uapi/drm/drm.h                 |  1 +
 3 files changed, 24 insertions(+), 8 deletions(-)

-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/i915: Use CPU mapping for userspace dma-buf mmap()

[Tiago Vignatti]

For now we're opting out devices that don't have the LLC CPU cache (mostly
"Atom" devices). Alternatively, we could build up a path to mmap them through
GTT WC (and ignore the fact that will be dead-slow for reading). Or, an even
more complex work I believe, would involve on setting up dma-buf ioctls to
allow userspace flush, controlling manually the synchronization via
begin{,end}_cpu_access.

Signed-off-by: Tiago Vignatti <tiago.vignatti@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_dmabuf.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index e9c2bfd..e6cb402 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -193,7 +193,26 @@ static void i915_gem_dmabuf_kunmap(struct dma_buf *dma_buf, unsigned long page_n
 
 static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
 {
-	return -EINVAL;
+	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
+	struct drm_device *dev = obj->base.dev;
+	int ret;
+
+	if (obj->base.size < vma->vm_end - vma->vm_start)
+		return -EINVAL;
+
+	/* On non-LLC machines we'd need to be careful cause CPU and GPU don't
+	 * share the CPU's L3 cache and coherency may hurt when CPU mapping. */
+	if (!HAS_LLC(dev))
+		return -EINVAL;
+
+	if (!obj->base.filp)
+		return -EINVAL;
+
+	ret = obj->base.filp->f_op->mmap(obj->base.filp, vma);
+	fput(vma->vm_file);
+	vma->vm_file = get_file(obj->base.filp);
+
+	return ret;
 }
 
 static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, size_t length, enum dma_data_direction direction)
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Fri, Jul 31, 2015 at 05:42:23PM -0300, Tiago Vignatti wrote:
> For now we're opting out devices that don't have the LLC CPU cache (mostly
> "Atom" devices). Alternatively, we could build up a path to mmap them through
> GTT WC (and ignore the fact that will be dead-slow for reading). Or, an even
> more complex work I believe, would involve on setting up dma-buf ioctls to
> allow userspace flush, controlling manually the synchronization via
> begin{,end}_cpu_access.
> 
> Signed-off-by: Tiago Vignatti <tiago.vignatti@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index e9c2bfd..e6cb402 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -193,7 +193,26 @@ static void i915_gem_dmabuf_kunmap(struct dma_buf *dma_buf, unsigned long page_n
>  
>  static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>  {
> -	return -EINVAL;
> +	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
> +	struct drm_device *dev = obj->base.dev;
> +	int ret;
> +
> +	if (obj->base.size < vma->vm_end - vma->vm_start)
> +		return -EINVAL;
> +
> +	/* On non-LLC machines we'd need to be careful cause CPU and GPU don't
> +	 * share the CPU's L3 cache and coherency may hurt when CPU mapping. */
> +	if (!HAS_LLC(dev))
> +		return -EINVAL;

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.

> +	if (!obj->base.filp)
> +		return -EINVAL;
> +
> +	ret = obj->base.filp->f_op->mmap(obj->base.filp, vma);
> +	fput(vma->vm_file);
> +	vma->vm_file = get_file(obj->base.filp);

Transfer owenership even if the ->mmap() fails?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Tiago Vignatti]

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.
+	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();
+}
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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();
> +}

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/i915: Use CPU mapping for userspace dma-buf mmap()

[Tiago Vignatti]

On 08/05/2015 04:08 AM, Daniel Vetter wrote:
> On Tue, Aug 04, 2015 at 06:30:25PM -0300, Tiago Vignatti wrote:
> 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.

Cool, I've got it now. The test below makes the cachelines dirt, 
requiring them to get flushed correctly -- I'll work on it now. Should 
we add that kind of test somewhere in igt BTW?

PS: I had an issue with the original kms_pwrite_crc which returns 
frequent fails. Paulo helped though and showed me that pwrite is 
currently broken: https://bugs.freedesktop.org/show_bug.cgi?id=86422

Tiago

diff --git a/tests/kms_pwrite_crc.c b/tests/kms_pwrite_crc.c
index 05b9e38..419b46d 100644
--- a/tests/kms_pwrite_crc.c
+++ b/tests/kms_pwrite_crc.c
@@ -50,6 +50,20 @@ typedef struct {
  	uint32_t devid;
  } data_t;

+static char *dmabuf_mmap_framebuffer(int drm_fd, 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, fb->size, PROT_READ | PROT_WRITE, MAP_SHARED, 
dma_buf_fd, 0);
+	igt_assert(ptr != MAP_FAILED);
+
+	return ptr;
+}
+
  static void test(data_t *data)
  {
  	igt_display_t *display = &data->display;
@@ -57,6 +71,7 @@ static void test(data_t *data)
  	struct igt_fb *fb = &data->fb[1];
  	drmModeModeInfo *mode;
  	cairo_t *cr;
+	char *ptr;
  	uint32_t caching;
  	void *buf;
  	igt_crc_t crc;
@@ -67,6 +82,8 @@ static void test(data_t *data)
  	igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
  		      DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, fb);

+	ptr = dmabuf_mmap_framebuffer(data->drm_fd, fb);
+
  	cr = igt_get_cairo_ctx(data->drm_fd, fb);
  	igt_paint_test_pattern(cr, fb->width, fb->height);
  	cairo_destroy(cr);
@@ -83,11 +100,11 @@ static void test(data_t *data)
  	caching = gem_get_caching(data->drm_fd, fb->gem_handle);
  	igt_assert(caching == I915_CACHING_NONE || caching == 
I915_CACHING_DISPLAY);

-	/* use pwrite to make the other fb all white too */
+	/* use dmabuf pointer to make the other fb all white too */
  	buf = malloc(fb->size);
  	igt_assert(buf != NULL);
  	memset(buf, 0xff, fb->size);
-	gem_write(data->drm_fd, fb->gem_handle, 0, buf, fb->size);
+	memcpy(ptr, buf, fb->size);
  	free(buf);

  	/* and flip to it */

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Use CPU mapping for userspace dma-buf mmap()

[Daniel Vetter]

On Wed, Aug 05, 2015 at 05:10:17PM -0300, Tiago Vignatti wrote:
> On 08/05/2015 04:08 AM, Daniel Vetter wrote:
> >On Tue, Aug 04, 2015 at 06:30:25PM -0300, Tiago Vignatti wrote:
> >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.
> 
> Cool, I've got it now. The test below makes the cachelines dirt, requiring
> them to get flushed correctly -- I'll work on it now. Should we add that
> kind of test somewhere in igt BTW?

Yeah if you expect me to merge dma-buf mmap with the begin/end stuff I'll
ask for an igt for it ;-)

> PS: I had an issue with the original kms_pwrite_crc which returns frequent
> fails. Paulo helped though and showed me that pwrite is currently broken:
> https://bugs.freedesktop.org/show_bug.cgi?id=86422

If you do dma-buf mmap with begin/end it should work, since in there we'll
just to manual range-based clflushing.
-Daniel


> 
> Tiago
> 
> diff --git a/tests/kms_pwrite_crc.c b/tests/kms_pwrite_crc.c
> index 05b9e38..419b46d 100644
> --- a/tests/kms_pwrite_crc.c
> +++ b/tests/kms_pwrite_crc.c
> @@ -50,6 +50,20 @@ typedef struct {
>  	uint32_t devid;
>  } data_t;
> 
> +static char *dmabuf_mmap_framebuffer(int drm_fd, 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, fb->size, PROT_READ | PROT_WRITE, MAP_SHARED, dma_buf_fd,
> 0);
> +	igt_assert(ptr != MAP_FAILED);
> +
> +	return ptr;
> +}
> +
>  static void test(data_t *data)
>  {
>  	igt_display_t *display = &data->display;
> @@ -57,6 +71,7 @@ static void test(data_t *data)
>  	struct igt_fb *fb = &data->fb[1];
>  	drmModeModeInfo *mode;
>  	cairo_t *cr;
> +	char *ptr;
>  	uint32_t caching;
>  	void *buf;
>  	igt_crc_t crc;
> @@ -67,6 +82,8 @@ static void test(data_t *data)
>  	igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
>  		      DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, fb);
> 
> +	ptr = dmabuf_mmap_framebuffer(data->drm_fd, fb);
> +
>  	cr = igt_get_cairo_ctx(data->drm_fd, fb);
>  	igt_paint_test_pattern(cr, fb->width, fb->height);
>  	cairo_destroy(cr);
> @@ -83,11 +100,11 @@ static void test(data_t *data)
>  	caching = gem_get_caching(data->drm_fd, fb->gem_handle);
>  	igt_assert(caching == I915_CACHING_NONE || caching ==
> I915_CACHING_DISPLAY);
> 
> -	/* use pwrite to make the other fb all white too */
> +	/* use dmabuf pointer to make the other fb all white too */
>  	buf = malloc(fb->size);
>  	igt_assert(buf != NULL);
>  	memset(buf, 0xff, fb->size);
> -	gem_write(data->drm_fd, fb->gem_handle, 0, buf, fb->size);
> +	memcpy(ptr, buf, fb->size);
>  	free(buf);
> 
>  	/* and flip to it */
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm: prime: Honour O_RDWR during prime-handle-to-fd

[Tiago Vignatti]

From: Daniel Thompson <daniel.thompson@linaro.org>

Currently DRM_IOCTL_PRIME_HANDLE_TO_FD rejects all flags except
(DRM|O)_CLOEXEC making it difficult (maybe impossible) for userspace
to mmap() the resulting dma-buf even when this is supported by the
DRM driver.

It is trivial to relax the restriction and permit read/write access.
This is safe because the flags are seldom touched by drm; mostly they
are passed verbatim to dma_buf calls.

v3 (Tiago): removed unused flags variable from drm_prime_handle_to_fd_ioctl.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Tiago Vignatti <tiago.vignatti@intel.com>
---
 drivers/gpu/drm/drm_prime.c | 10 +++-------
 include/uapi/drm/drm.h      |  1 +
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 27aa718..df6cdc7 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -329,7 +329,7 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
  * drm_gem_prime_export - helper library implementation of the export callback
  * @dev: drm_device to export from
  * @obj: GEM object to export
- * @flags: flags like DRM_CLOEXEC
+ * @flags: flags like DRM_CLOEXEC and DRM_RDWR
  *
  * This is the implementation of the gem_prime_export functions for GEM drivers
  * using the PRIME helpers.
@@ -628,7 +628,6 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 				 struct drm_file *file_priv)
 {
 	struct drm_prime_handle *args = data;
-	uint32_t flags;
 
 	if (!drm_core_check_feature(dev, DRIVER_PRIME))
 		return -EINVAL;
@@ -637,14 +636,11 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 		return -ENOSYS;
 
 	/* check flags are valid */
-	if (args->flags & ~DRM_CLOEXEC)
+	if (args->flags & ~(DRM_CLOEXEC | DRM_RDWR))
 		return -EINVAL;
 
-	/* we only want to pass DRM_CLOEXEC which is == O_CLOEXEC */
-	flags = args->flags & DRM_CLOEXEC;
-
 	return dev->driver->prime_handle_to_fd(dev, file_priv,
-			args->handle, flags, &args->fd);
+			args->handle, args->flags, &args->fd);
 }
 
 int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 3801584..ad8223e 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -668,6 +668,7 @@ struct drm_set_client_cap {
 	__u64 value;
 };
 
+#define DRM_RDWR O_RDWR
 #define DRM_CLOEXEC O_CLOEXEC
 struct drm_prime_handle {
 	__u32 handle;
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx