[PATCH 1/2] drm/i915: Convert EFAULT into a silent SIGBUS

[PATCH 1/2] drm/i915: Convert EFAULT into a silent SIGBUS

[Chris Wilson]

EFAULT will be a possible return code where backing storage is
transient, such after it is purged by madvise. As such it is to be
expected and so should not trigger a WARN inside i915_gem_fault() but be
converted silently to SIGBUS.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d58f777903c0..f2ef2c8518ba 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1574,6 +1574,7 @@ out:
 		ret = VM_FAULT_OOM;
 		break;
 	case -ENOSPC:
+	case -EFAULT:
 		ret = VM_FAULT_SIGBUS;
 		break;
 	default:
-- 
1.9.rc1

[PATCH 2/2] drm/i915: Treat using a purged buffer as a source of EFAULT

[Chris Wilson]

Since a purged buffer is one without any associated pages, attempting to
use it should generate EFAULT rather than EINVAL, as it is not strictly
an invalid parameter.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f2ef2c8518ba..1c91a6718e89 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1733,7 +1733,7 @@ i915_gem_mmap_gtt(struct drm_file *file,
 
 	if (obj->madv != I915_MADV_WILLNEED) {
 		DRM_ERROR("Attempting to mmap a purgeable buffer\n");
-		ret = -EINVAL;
+		ret = -EFAULT;
 		goto out;
 	}
 
@@ -2087,7 +2087,7 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 
 	if (obj->madv != I915_MADV_WILLNEED) {
 		DRM_ERROR("Attempting to obtain a purgeable object\n");
-		return -EINVAL;
+		return -EFAULT;
 	}
 
 	BUG_ON(obj->pages_pin_count);
@@ -4091,7 +4091,7 @@ i915_gem_pin_ioctl(struct drm_device *dev, void *data,
 
 	if (obj->madv != I915_MADV_WILLNEED) {
 		DRM_ERROR("Attempting to pin a purgeable buffer\n");
-		ret = -EINVAL;
+		ret = -EFAULT;
 		goto out;
 	}
 
-- 
1.9.rc1

Re: [PATCH 2/2] drm/i915: Treat using a purged buffer as a source of EFAULT

[Daniel Vetter]

On Fri, Jan 31, 2014 at 11:34:58AM +0000, Chris Wilson wrote:
> Since a purged buffer is one without any associated pages, attempting to
> use it should generate EFAULT rather than EINVAL, as it is not strictly
> an invalid parameter.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Care to drop a quick testcase into i-g-t to encode this return value
change? Also, any plans to use this with userspace somehow, or what's the
motivation for this?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: Treat using a purged buffer as a source of EFAULT

[Chris Wilson]

On Tue, Feb 04, 2014 at 12:05:13PM +0100, Daniel Vetter wrote:
> On Fri, Jan 31, 2014 at 11:34:58AM +0000, Chris Wilson wrote:
> > Since a purged buffer is one without any associated pages, attempting to
> > use it should generate EFAULT rather than EINVAL, as it is not strictly
> > an invalid parameter.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Care to drop a quick testcase into i-g-t to encode this return value
> change? Also, any plans to use this with userspace somehow, or what's the
> motivation for this?

Just that we have other paths where inaccessible pages are reported back
to userspace as EFAULT, and this is another case where we could distinguish
the errno and so be more consistent.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Treat using a purged buffer as a source of EFAULT

[Daniel Vetter]

On Tue, Feb 04, 2014 at 11:36:39AM +0000, Chris Wilson wrote:
> On Tue, Feb 04, 2014 at 12:05:13PM +0100, Daniel Vetter wrote:
> > On Fri, Jan 31, 2014 at 11:34:58AM +0000, Chris Wilson wrote:
> > > Since a purged buffer is one without any associated pages, attempting to
> > > use it should generate EFAULT rather than EINVAL, as it is not strictly
> > > an invalid parameter.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Care to drop a quick testcase into i-g-t to encode this return value
> > change? Also, any plans to use this with userspace somehow, or what's the
> > motivation for this?
> 
> Just that we have other paths where inaccessible pages are reported back
> to userspace as EFAULT, and this is another case where we could distinguish
> the errno and so be more consistent.

Makes sense. I'll merge the patches once we have a bit of igt to enshrine
this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] tests: Add gem_madvise

[Chris Wilson]

Exercise that calling madvise produces expected results

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/.gitignore       |   1 +
 tests/Makefile.sources |   1 +
 tests/gem_madvise.c    | 155 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 157 insertions(+)
 create mode 100644 tests/gem_madvise.c

diff --git a/tests/.gitignore b/tests/.gitignore
index 7377275..30d738f 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -48,6 +48,7 @@ gem_hangcheck_forcewake
 gem_largeobject
 gem_linear_blits
 gem_lut_handle
+gem_madvise
 gem_media_fill
 gem_mmap
 gem_mmap_gtt
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 459c7c9..124b235 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -34,6 +34,7 @@ TESTS_progs_M = \
 	gem_flink \
 	gem_flink_race \
 	gem_linear_blits \
+	gem_madvise \
 	gem_mmap \
 	gem_mmap_gtt \
 	gem_partial_pwrite_pread \
diff --git a/tests/gem_madvise.c b/tests/gem_madvise.c
new file mode 100644
index 0000000..a7bd22c
--- /dev/null
+++ b/tests/gem_madvise.c
@@ -0,0 +1,155 @@
+/*
+ * Copyright © 2014 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:
+ *    Chris Wilson <chris@chris-wilson.co.uk>
+ *
+ */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <errno.h>
+#include <setjmp.h>
+#include <signal.h>
+
+#include "drm.h"
+#include "i915_drm.h"
+#include "drmtest.h"
+
+#define OBJECT_SIZE (1024*1024)
+
+/* Testcase: checks that the kernel reports EFAULT when trying to use purged bo
+ *
+ */
+
+static jmp_buf jmp;
+
+static void sigtrap(int sig)
+{
+	longjmp(jmp, sig);
+}
+
+static void
+dontneed_before_mmap(void)
+{
+	int fd = drm_open_any();
+	uint32_t handle;
+	char *ptr;
+
+	handle = gem_create(fd, OBJECT_SIZE);
+	gem_madvise(fd, handle, I915_MADV_DONTNEED);
+	ptr = gem_mmap(fd, handle, OBJECT_SIZE, PROT_READ | PROT_WRITE);
+	igt_assert(ptr == NULL);
+	igt_assert(errno == EFAULT);
+	close(fd);
+}
+
+static void
+dontneed_after_mmap(void)
+{
+	int fd = drm_open_any();
+	uint32_t handle;
+	char *ptr;
+
+	handle = gem_create(fd, OBJECT_SIZE);
+	ptr = gem_mmap(fd, handle, OBJECT_SIZE, PROT_READ | PROT_WRITE);
+	igt_assert(ptr != NULL);
+	gem_madvise(fd, handle, I915_MADV_DONTNEED);
+	close(fd);
+
+	signal(SIGBUS, sigtrap);
+	switch (setjmp(jmp)) {
+	case SIGBUS:
+		break;
+	case 0:
+		*ptr = 0;
+	default:
+		igt_assert(!"reached");
+		break;
+	}
+	munmap(ptr, OBJECT_SIZE);
+	signal(SIGBUS, SIG_DFL);
+}
+
+static void
+dontneed_before_pwrite(void)
+{
+	int fd = drm_open_any();
+	uint32_t buf[] = { MI_BATCH_BUFFER_END, 0 };
+	struct drm_i915_gem_pwrite gem_pwrite;
+
+	gem_pwrite.handle = gem_create(fd, OBJECT_SIZE);
+	gem_pwrite.offset = 0;
+	gem_pwrite.size = sizeof(buf);
+	gem_pwrite.data_ptr = (uintptr_t)buf;
+	gem_madvise(fd, gem_pwrite.handle, I915_MADV_DONTNEED);
+
+	igt_assert(drmIoctl(fd, DRM_IOCTL_I915_GEM_PWRITE, &gem_pwrite));
+	igt_assert(errno == EFAULT);
+
+	gem_close(fd, gem_pwrite.handle);
+	close(fd);
+}
+
+static void
+dontneed_before_exec(void)
+{
+	int fd = drm_open_any();
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 exec;
+	uint32_t buf[] = { MI_BATCH_BUFFER_END, 0 };
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	memset(&exec, 0, sizeof(exec));
+
+	exec.handle = gem_create(fd, OBJECT_SIZE);
+	gem_write(fd, exec.handle, 0, buf, sizeof(buf));
+	gem_madvise(fd, exec.handle, I915_MADV_DONTNEED);
+
+	execbuf.buffers_ptr = (uintptr_t)&exec;
+	execbuf.buffer_count = 1;
+	gem_execbuf(fd, &execbuf);
+
+	gem_close(fd, exec.handle);
+	close(fd);
+}
+
+igt_simple_main
+{
+	igt_skip_on_simulation();
+
+	igt_subtest("dontneed-before-mmap")
+		dontneed_before_mmap();
+
+	igt_subtest("dontneed-after-mmap")
+		dontneed_after_mmap();
+
+	igt_subtest("dontneed-before-pwrite")
+		dontneed_before_pwrite();
+
+	igt_subtest("dontneed-before-exec")
+		dontneed_before_exec();
+}
-- 
1.9.rc1

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

Re: [PATCH] tests: Add gem_madvise

[Daniel Vetter]

On Tue, Feb 04, 2014 at 02:14:31PM +0000, Chris Wilson wrote:
> Exercise that calling madvise produces expected results
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks a lot for the testcase and patches, all pulled in.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch