[PATCH 0/5] Fixup pwrite/pread with gtt mmapped user addresses

[PATCH 0/5] Fixup pwrite/pread with gtt mmapped user addresses

[Daniel Vetter]

Hi all,

Original bugreport: https://bugs.freedesktop.org/show_bug.cgi?id=38115

This bug can e.g. be hit with glBufferData(dst, glBufferMap(src))

Testcase is gem_mmap_gtt.

Neglecting the trivial cleanup patch 2, patch 1 converts the possible
deadlock in an errornous -EFAULT. I've already submitted this, but
include it here again for completeness.  Patches 3-5 then fix this up.
Imo there's a bit more work left to do (like properly prefaulting,
which would have papered over this bug).  But I think this is the
minimal set required to fix the bug and I've wanted to get early
feedback on the approach and on possible race issues.

I think patch 3-5 are not cc:stable material because they're too
invasive (maybe even stuff for -next). The prefaulting patch will
be much simpler, less invasive and it essentially reduces the issue to
a tiny race, so I suggest that for backporting.

So review, comments highly appreciated.

Yours, Daniel

Daniel Vetter (5):
  io-mapping: ensure io_mapping_map_atomic _is_ atomic
  drm/i915: drop KM_USER0 argument to k(un)map_atomic
  drm/i915: fall through pwrite_gtt_slow to the shmem slow path
  drm/i915: rewrite shmem_pwrite_slow to use copy_from_user
  drm/i915: rewrite shmem_pread_slow to use copy_to_user

 drivers/gpu/drm/i915/i915_gem.c       |  324 ++++++++++++++-------------------
 drivers/gpu/drm/i915/i915_gem_debug.c |    6 +-
 include/linux/io-mapping.h            |    2 +
 3 files changed, 140 insertions(+), 192 deletions(-)

-- 
1.7.6

[PATCH 1/5] io-mapping: ensure io_mapping_map_atomic _is_ atomic

[Daniel Vetter]

For the !HAVE_ATOMIC_IOMAP case the stub functions did not call
pagefault_disable/_enable. The i915 driver relies on the map
actually being atomic, otherwise it can deadlock with it's own
pagefault handler in the gtt pwrite fastpath.

This is exercised by gem_mmap_gtt from the intel-gpu-toosl gem
testsuite.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38115
Cc: stable@kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 include/linux/io-mapping.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/linux/io-mapping.h b/include/linux/io-mapping.h
index 8cdcc2a1..6b3bef1 100644
--- a/include/linux/io-mapping.h
+++ b/include/linux/io-mapping.h
@@ -138,12 +138,14 @@ static inline void __iomem *
 io_mapping_map_atomic_wc(struct io_mapping *mapping,
 			 unsigned long offset)
 {
+	pagefault_disable();
 	return ((char __force __iomem *) mapping) + offset;
 }
 
 static inline void
 io_mapping_unmap_atomic(void __iomem *vaddr)
 {
+	pagefault_enable();
 }
 
 /* Non-atomic map/unmap */
-- 
1.7.6

[PATCH 2/5] drm/i915: drop KM_USER0 argument to k(un)map_atomic

[Daniel Vetter]

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c       |    4 ++--
 drivers/gpu/drm/i915/i915_gem_debug.c |    6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9857e9d..e0475ca 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -802,11 +802,11 @@ i915_gem_shmem_pwrite_fast(struct drm_device *dev,
 		if (IS_ERR(page))
 			return PTR_ERR(page);
 
-		vaddr = kmap_atomic(page, KM_USER0);
+		vaddr = kmap_atomic(page);
 		ret = __copy_from_user_inatomic(vaddr + page_offset,
 						user_data,
 						page_length);
-		kunmap_atomic(vaddr, KM_USER0);
+		kunmap_atomic(vaddr);
 
 		set_page_dirty(page);
 		mark_page_accessed(page);
diff --git a/drivers/gpu/drm/i915/i915_gem_debug.c b/drivers/gpu/drm/i915/i915_gem_debug.c
index 8da1899..131c6a5 100644
--- a/drivers/gpu/drm/i915/i915_gem_debug.c
+++ b/drivers/gpu/drm/i915/i915_gem_debug.c
@@ -157,7 +157,7 @@ i915_gem_object_check_coherency(struct drm_i915_gem_object *obj, int handle)
 	for (page = 0; page < obj->size / PAGE_SIZE; page++) {
 		int i;
 
-		backing_map = kmap_atomic(obj->pages[page], KM_USER0);
+		backing_map = kmap_atomic(obj->pages[page]);
 
 		if (backing_map == NULL) {
 			DRM_ERROR("failed to map backing page\n");
@@ -181,13 +181,13 @@ i915_gem_object_check_coherency(struct drm_i915_gem_object *obj, int handle)
 				}
 			}
 		}
-		kunmap_atomic(backing_map, KM_USER0);
+		kunmap_atomic(backing_map);
 		backing_map = NULL;
 	}
 
  out:
 	if (backing_map != NULL)
-		kunmap_atomic(backing_map, KM_USER0);
+		kunmap_atomic(backing_map);
 	iounmap(gtt_mapping);
 
 	/* give syslog time to catch up */
-- 
1.7.6

[PATCH 3/5] drm/i915: fall through pwrite_gtt_slow to the shmem slow path

[Daniel Vetter]

The gtt_pwrite slowpath grabs the userspace memory with
get_user_pages. This will not work for non-page backed memory, like a
gtt mmapped gem object. Hence fall throuh to the shmem paths if we hit
-EFAULT in the gtt paths.

Now the shmem paths have exactly the same problem, but this way we
only need to rearrange the code in one write path.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e0475ca..9c28d48 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1018,18 +1018,24 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 
 out_unpin:
 		i915_gem_object_unpin(obj);
-	} else {
-		ret = i915_gem_object_set_to_cpu_domain(obj, 1);
-		if (ret)
-			goto out;
 
-		ret = -EFAULT;
-		if (!i915_gem_object_needs_bit17_swizzle(obj))
-			ret = i915_gem_shmem_pwrite_fast(dev, obj, args, file);
-		if (ret == -EFAULT)
-			ret = i915_gem_shmem_pwrite_slow(dev, obj, args, file);
+		if (ret != -EFAULT)
+			goto out;
+		/* Fall through to the shmfs paths because the gtt paths might
+		 * fail with non-page-backed user pointers (e.g. gtt mappings
+		 * when moving data between textures). */
 	}
 
+	ret = i915_gem_object_set_to_cpu_domain(obj, 1);
+	if (ret)
+		goto out;
+
+	ret = -EFAULT;
+	if (!i915_gem_object_needs_bit17_swizzle(obj))
+		ret = i915_gem_shmem_pwrite_fast(dev, obj, args, file);
+	if (ret == -EFAULT)
+		ret = i915_gem_shmem_pwrite_slow(dev, obj, args, file);
+
 out:
 	drm_gem_object_unreference(&obj->base);
 unlock:
-- 
1.7.6

[PATCH 4/5] drm/i915: rewrite shmem_pwrite_slow to use copy_from_user

[Daniel Vetter]

... instead of get_user_pages, because that fails on non page-backed
user addresses like e.g. a gtt mapping of a bo.

To get there essentially copy the vfs read path into pagecache. We
can't call that right away because we have to take care of bit17
swizzling. To not deadlock with our own pagefault handler we need
to completely drop struct_mutex, reducing the atomicty-guarantees
of our userspace abi. Implications for racing with other gem ioctl:

- execbuf, pwrite, pread: Due to -EFAULT fallback to slow paths there's
  already the risk of the pwrite call not being atomic, no degration.
- read/write access to mmaps: already fully racy, no degration.
- set_tiling: Calling set_tiling while reading/writing is already
  pretty much undefined, now it just got a bit worse. set_tiling is
  only called by libdrm on unused/new bos, so no problem.
- set_domain: When changing to the gtt domain while copying (without any
  read/write access, e.g. for synchronization), we might leave unflushed
  data in the cpu caches. The clflush_object at the end of pwrite_slow
  takes care of this problem.
- truncating of purgeable objects: the shmem_read_mapping_page call could
  reinstate backing storage for truncated objects. The check at the end
  of pwrite_slow takes care of this.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |  124 +++++++++++++++++++-------------------
 1 files changed, 62 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9c28d48..c390bf8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -58,6 +58,7 @@ static void i915_gem_free_object_tail(struct drm_i915_gem_object *obj);
 
 static int i915_gem_inactive_shrink(struct shrinker *shrinker,
 				    struct shrink_control *sc);
+static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
 
 /* some bookkeeping */
 static void i915_gem_info_add_obj(struct drm_i915_private *dev_priv,
@@ -385,6 +386,32 @@ i915_gem_shmem_pread_fast(struct drm_device *dev,
 	return 0;
 }
 
+static inline int
+copy_from_user_swizzled(char __user *gpu_vaddr, int gpu_offset,
+			const char *cpu_vaddr,
+			int length)
+{
+	int ret, cpu_offset = 0;
+
+	while (length > 0) {
+		int cacheline_end = ALIGN(gpu_offset + 1, 64);
+		int this_length = min(cacheline_end - gpu_offset, length);
+		int swizzled_gpu_offset = gpu_offset ^ 64;
+
+		ret = __copy_from_user(gpu_vaddr + swizzled_gpu_offset,
+				       cpu_vaddr + cpu_offset,
+				       this_length);
+		if (ret)
+			return ret + length;
+
+		cpu_offset += this_length;
+		gpu_offset += this_length;
+		length -= this_length;
+	}
+
+	return 0;
+}
+
 /**
  * This is the fallback shmem pread path, which allocates temporary storage
  * in kernel space to copy_to_user into outside of the struct_mutex, so we
@@ -841,71 +868,36 @@ i915_gem_shmem_pwrite_slow(struct drm_device *dev,
 			   struct drm_file *file)
 {
 	struct address_space *mapping = obj->base.filp->f_path.dentry->d_inode->i_mapping;
-	struct mm_struct *mm = current->mm;
-	struct page **user_pages;
 	ssize_t remain;
-	loff_t offset, pinned_pages, i;
-	loff_t first_data_page, last_data_page, num_pages;
-	int shmem_page_offset;
-	int data_page_index,  data_page_offset;
-	int page_length;
-	int ret;
-	uint64_t data_ptr = args->data_ptr;
-	int do_bit17_swizzling;
+	loff_t offset;
+	char __user *user_data;
+	int shmem_page_offset, page_length, ret;
+	int obj_do_bit17_swizzling, page_do_bit17_swizzling;
 
+	user_data = (char __user *) (uintptr_t) args->data_ptr;
 	remain = args->size;
 
-	/* Pin the user pages containing the data.  We can't fault while
-	 * holding the struct mutex, and all of the pwrite implementations
-	 * want to hold it while dereferencing the user data.
-	 */
-	first_data_page = data_ptr / PAGE_SIZE;
-	last_data_page = (data_ptr + args->size - 1) / PAGE_SIZE;
-	num_pages = last_data_page - first_data_page + 1;
-
-	user_pages = drm_malloc_ab(num_pages, sizeof(struct page *));
-	if (user_pages == NULL)
-		return -ENOMEM;
-
-	mutex_unlock(&dev->struct_mutex);
-	down_read(&mm->mmap_sem);
-	pinned_pages = get_user_pages(current, mm, (uintptr_t)args->data_ptr,
-				      num_pages, 0, 0, user_pages, NULL);
-	up_read(&mm->mmap_sem);
-	mutex_lock(&dev->struct_mutex);
-	if (pinned_pages < num_pages) {
-		ret = -EFAULT;
-		goto out;
-	}
-
-	ret = i915_gem_object_set_to_cpu_domain(obj, 1);
-	if (ret)
-		goto out;
-
-	do_bit17_swizzling = i915_gem_object_needs_bit17_swizzle(obj);
+	obj_do_bit17_swizzling = i915_gem_object_needs_bit17_swizzle(obj);
 
 	offset = args->offset;
 	obj->dirty = 1;
 
+	mutex_unlock(&dev->struct_mutex);
+
 	while (remain > 0) {
 		struct page *page;
+		char *vaddr;
 
 		/* Operation in this page
 		 *
 		 * shmem_page_offset = offset within page in shmem file
-		 * data_page_index = page number in get_user_pages return
-		 * data_page_offset = offset with data_page_index page.
 		 * page_length = bytes to copy for this page
 		 */
 		shmem_page_offset = offset_in_page(offset);
-		data_page_index = data_ptr / PAGE_SIZE - first_data_page;
-		data_page_offset = offset_in_page(data_ptr);
 
 		page_length = remain;
 		if ((shmem_page_offset + page_length) > PAGE_SIZE)
 			page_length = PAGE_SIZE - shmem_page_offset;
-		if ((data_page_offset + page_length) > PAGE_SIZE)
-			page_length = PAGE_SIZE - data_page_offset;
 
 		page = shmem_read_mapping_page(mapping, offset >> PAGE_SHIFT);
 		if (IS_ERR(page)) {
@@ -913,34 +905,42 @@ i915_gem_shmem_pwrite_slow(struct drm_device *dev,
 			goto out;
 		}
 
-		if (do_bit17_swizzling) {
-			slow_shmem_bit17_copy(page,
-					      shmem_page_offset,
-					      user_pages[data_page_index],
-					      data_page_offset,
-					      page_length,
-					      0);
-		} else {
-			slow_shmem_copy(page,
-					shmem_page_offset,
-					user_pages[data_page_index],
-					data_page_offset,
-					page_length);
-		}
+		page_do_bit17_swizzling = (page_to_phys(page) & (1 << 17)) == 0;
+
+		vaddr = kmap(page);
+		if (obj_do_bit17_swizzling && page_do_bit17_swizzling)
+			ret = copy_from_user_swizzled(vaddr, shmem_page_offset,
+						      user_data,
+						      page_length);
+		else
+			ret = __copy_from_user(vaddr + shmem_page_offset,
+					       user_data,
+					       page_length);
+		kunmap(page);
 
 		set_page_dirty(page);
 		mark_page_accessed(page);
 		page_cache_release(page);
 
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
 		remain -= page_length;
-		data_ptr += page_length;
+		user_data += page_length;
 		offset += page_length;
 	}
 
 out:
-	for (i = 0; i < pinned_pages; i++)
-		page_cache_release(user_pages[i]);
-	drm_free_large(user_pages);
+	mutex_lock(&dev->struct_mutex);
+	/* Fixup: Kill any reinstated backing storage pages */
+	if (obj->madv == __I915_MADV_PURGED)
+		i915_gem_object_truncate(obj);
+	/* and flush dirty cachelines in case the object isn't in the cpu write
+	 * domain anymore. */
+	if (obj->base.write_domain != I915_GEM_DOMAIN_CPU)
+		i915_gem_clflush_object(obj);
 
 	return ret;
 }
-- 
1.7.6

[PATCH 5/5] drm/i915: rewrite shmem_pread_slow to use copy_to_user

[Daniel Vetter]

Like for shmem_pwrite_slow. The only difference is that because we
read data, we can leave the fetched cachelines in the cpu: In the case
that the object isn't in the cpu read domain anymore, the clflush for
the next cpu read domain invalidation will simply drop these
cachelines.

slow_shmem_bit17_copy is now ununsed, so kill it.

With this patch tests/gem_mmap_gtt now actually works.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38115
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |  172 +++++++++++++--------------------------
 1 files changed, 56 insertions(+), 116 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c390bf8..a800256 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -277,55 +277,6 @@ slow_shmem_copy(struct page *dst_page,
 	kunmap(dst_page);
 }
 
-static inline void
-slow_shmem_bit17_copy(struct page *gpu_page,
-		      int gpu_offset,
-		      struct page *cpu_page,
-		      int cpu_offset,
-		      int length,
-		      int is_read)
-{
-	char *gpu_vaddr, *cpu_vaddr;
-
-	/* Use the unswizzled path if this page isn't affected. */
-	if ((page_to_phys(gpu_page) & (1 << 17)) == 0) {
-		if (is_read)
-			return slow_shmem_copy(cpu_page, cpu_offset,
-					       gpu_page, gpu_offset, length);
-		else
-			return slow_shmem_copy(gpu_page, gpu_offset,
-					       cpu_page, cpu_offset, length);
-	}
-
-	gpu_vaddr = kmap(gpu_page);
-	cpu_vaddr = kmap(cpu_page);
-
-	/* Copy the data, XORing A6 with A17 (1). The user already knows he's
-	 * XORing with the other bits (A9 for Y, A9 and A10 for X)
-	 */
-	while (length > 0) {
-		int cacheline_end = ALIGN(gpu_offset + 1, 64);
-		int this_length = min(cacheline_end - gpu_offset, length);
-		int swizzled_gpu_offset = gpu_offset ^ 64;
-
-		if (is_read) {
-			memcpy(cpu_vaddr + cpu_offset,
-			       gpu_vaddr + swizzled_gpu_offset,
-			       this_length);
-		} else {
-			memcpy(gpu_vaddr + swizzled_gpu_offset,
-			       cpu_vaddr + cpu_offset,
-			       this_length);
-		}
-		cpu_offset += this_length;
-		gpu_offset += this_length;
-		length -= this_length;
-	}
-
-	kunmap(cpu_page);
-	kunmap(gpu_page);
-}
-
 /**
  * This is the fast shmem pread path, which attempts to copy_from_user directly
  * from the backing pages of the object to the user's address space.  On a
@@ -387,6 +338,32 @@ i915_gem_shmem_pread_fast(struct drm_device *dev,
 }
 
 static inline int
+copy_to_user_swizzled(char __user *cpu_vaddr,
+		      const char *gpu_vaddr, int gpu_offset,
+		      int length)
+{
+	int ret, cpu_offset = 0;
+
+	while (length > 0) {
+		int cacheline_end = ALIGN(gpu_offset + 1, 64);
+		int this_length = min(cacheline_end - gpu_offset, length);
+		int swizzled_gpu_offset = gpu_offset ^ 64;
+
+		ret = __copy_to_user(cpu_vaddr + cpu_offset,
+				     gpu_vaddr + swizzled_gpu_offset,
+				     this_length);
+		if (ret)
+			return ret + length;
+
+		cpu_offset += this_length;
+		gpu_offset += this_length;
+		length -= this_length;
+	}
+
+	return 0;
+}
+
+static inline int
 copy_from_user_swizzled(char __user *gpu_vaddr, int gpu_offset,
 			const char *cpu_vaddr,
 			int length)
@@ -425,72 +402,34 @@ i915_gem_shmem_pread_slow(struct drm_device *dev,
 			  struct drm_file *file)
 {
 	struct address_space *mapping = obj->base.filp->f_path.dentry->d_inode->i_mapping;
-	struct mm_struct *mm = current->mm;
-	struct page **user_pages;
+	char __user *user_data;
 	ssize_t remain;
-	loff_t offset, pinned_pages, i;
-	loff_t first_data_page, last_data_page, num_pages;
-	int shmem_page_offset;
-	int data_page_index, data_page_offset;
-	int page_length;
-	int ret;
-	uint64_t data_ptr = args->data_ptr;
-	int do_bit17_swizzling;
+	loff_t offset;
+	int shmem_page_offset, page_length, ret;
+	int obj_do_bit17_swizzling, page_do_bit17_swizzling;
 
+	user_data = (char __user *) (uintptr_t) args->data_ptr;
 	remain = args->size;
 
-	/* Pin the user pages containing the data.  We can't fault while
-	 * holding the struct mutex, yet we want to hold it while
-	 * dereferencing the user data.
-	 */
-	first_data_page = data_ptr / PAGE_SIZE;
-	last_data_page = (data_ptr + args->size - 1) / PAGE_SIZE;
-	num_pages = last_data_page - first_data_page + 1;
+	obj_do_bit17_swizzling = i915_gem_object_needs_bit17_swizzle(obj);
 
-	user_pages = drm_malloc_ab(num_pages, sizeof(struct page *));
-	if (user_pages == NULL)
-		return -ENOMEM;
+	offset = args->offset;
 
 	mutex_unlock(&dev->struct_mutex);
-	down_read(&mm->mmap_sem);
-	pinned_pages = get_user_pages(current, mm, (uintptr_t)args->data_ptr,
-				      num_pages, 1, 0, user_pages, NULL);
-	up_read(&mm->mmap_sem);
-	mutex_lock(&dev->struct_mutex);
-	if (pinned_pages < num_pages) {
-		ret = -EFAULT;
-		goto out;
-	}
-
-	ret = i915_gem_object_set_cpu_read_domain_range(obj,
-							args->offset,
-							args->size);
-	if (ret)
-		goto out;
-
-	do_bit17_swizzling = i915_gem_object_needs_bit17_swizzle(obj);
-
-	offset = args->offset;
 
 	while (remain > 0) {
 		struct page *page;
+		char *vaddr;
 
 		/* Operation in this page
 		 *
 		 * shmem_page_offset = offset within page in shmem file
-		 * data_page_index = page number in get_user_pages return
-		 * data_page_offset = offset with data_page_index page.
 		 * page_length = bytes to copy for this page
 		 */
 		shmem_page_offset = offset_in_page(offset);
-		data_page_index = data_ptr / PAGE_SIZE - first_data_page;
-		data_page_offset = offset_in_page(data_ptr);
-
 		page_length = remain;
 		if ((shmem_page_offset + page_length) > PAGE_SIZE)
 			page_length = PAGE_SIZE - shmem_page_offset;
-		if ((data_page_offset + page_length) > PAGE_SIZE)
-			page_length = PAGE_SIZE - data_page_offset;
 
 		page = shmem_read_mapping_page(mapping, offset >> PAGE_SHIFT);
 		if (IS_ERR(page)) {
@@ -498,36 +437,37 @@ i915_gem_shmem_pread_slow(struct drm_device *dev,
 			goto out;
 		}
 
-		if (do_bit17_swizzling) {
-			slow_shmem_bit17_copy(page,
-					      shmem_page_offset,
-					      user_pages[data_page_index],
-					      data_page_offset,
-					      page_length,
-					      1);
-		} else {
-			slow_shmem_copy(user_pages[data_page_index],
-					data_page_offset,
-					page,
-					shmem_page_offset,
-					page_length);
-		}
+		page_do_bit17_swizzling = (page_to_phys(page) & (1 << 17)) == 0;
+
+		vaddr = kmap(page);
+		if (obj_do_bit17_swizzling && page_do_bit17_swizzling)
+			ret = copy_to_user_swizzled(user_data,
+						    vaddr, shmem_page_offset,
+						    page_length);
+		else
+			ret = __copy_to_user(user_data,
+					     vaddr + shmem_page_offset,
+					     page_length);
+		kunmap(page);
 
 		mark_page_accessed(page);
 		page_cache_release(page);
 
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
 		remain -= page_length;
-		data_ptr += page_length;
+		user_data += page_length;
 		offset += page_length;
 	}
 
 out:
-	for (i = 0; i < pinned_pages; i++) {
-		SetPageDirty(user_pages[i]);
-		mark_page_accessed(user_pages[i]);
-		page_cache_release(user_pages[i]);
-	}
-	drm_free_large(user_pages);
+	mutex_lock(&dev->struct_mutex);
+	/* Fixup: Kill any reinstated backing storage pages */
+	if (obj->madv == __I915_MADV_PURGED)
+		i915_gem_object_truncate(obj);
 
 	return ret;
 }
-- 
1.7.6

Re: [PATCH 1/5] io-mapping: ensure io_mapping_map_atomic _is_ atomic

[Chris Wilson]

On Sat, 17 Sep 2011 20:55:45 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> For the !HAVE_ATOMIC_IOMAP case the stub functions did not call
> pagefault_disable/_enable. The i915 driver relies on the map
> actually being atomic, otherwise it can deadlock with it's own
> pagefault handler in the gtt pwrite fastpath.
> 
> This is exercised by gem_mmap_gtt from the intel-gpu-toosl gem
> testsuite.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38115
> Cc: stable@kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/5] drm/i915: drop KM_USER0 argument to k(un)map_atomic

[Chris Wilson]

On Sat, 17 Sep 2011 20:55:46 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/5] drm/i915: rewrite shmem_pwrite_slow to use copy_from_user

[Daniel Vetter]

On Sat, Sep 17, 2011 at 08:55:48PM +0200, Daniel Vetter wrote:
>  out:
> -	for (i = 0; i < pinned_pages; i++)
> -		page_cache_release(user_pages[i]);
> -	drm_free_large(user_pages);
> +	mutex_lock(&dev->struct_mutex);
> +	/* Fixup: Kill any reinstated backing storage pages */
> +	if (obj->madv == __I915_MADV_PURGED)
> +		i915_gem_object_truncate(obj);
> +	/* and flush dirty cachelines in case the object isn't in the cpu write
> +	 * domain anymore. */
> +	if (obj->base.write_domain != I915_GEM_DOMAIN_CPU)
> +		i915_gem_clflush_object(obj);

Meh, just noticed that I'm missing an intel_gtt_chipset_flush() here ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/5] drm/i915: fall through pwrite_gtt_slow to the shmem slow path

[Chris Wilson]

On Sat, 17 Sep 2011 20:55:47 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The gtt_pwrite slowpath grabs the userspace memory with
> get_user_pages. This will not work for non-page backed memory, like a
> gtt mmapped gem object. Hence fall throuh to the shmem paths if we hit
> -EFAULT in the gtt paths.
> 
> Now the shmem paths have exactly the same problem, but this way we
> only need to rearrange the code in one write path.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   24 +++++++++++++++---------
>  1 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e0475ca..9c28d48 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1018,18 +1018,24 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  
>  out_unpin:
>  		i915_gem_object_unpin(obj);
> -	} else {
> -		ret = i915_gem_object_set_to_cpu_domain(obj, 1);
> -		if (ret)
> -			goto out;
>  
> -		ret = -EFAULT;
> -		if (!i915_gem_object_needs_bit17_swizzle(obj))
> -			ret = i915_gem_shmem_pwrite_fast(dev, obj, args, file);
> -		if (ret == -EFAULT)
> -			ret = i915_gem_shmem_pwrite_slow(dev, obj, args, file);
> +		if (ret != -EFAULT)
> +			goto out;
> +		/* Fall through to the shmfs paths because the gtt paths might
> +		 * fail with non-page-backed user pointers (e.g. gtt mappings
> +		 * when moving data between textures). */
>  	}

I'd actually prefer to have an
  if (ret == -EFAULT) {
and comments here in order to mark the shmem paths as a logical unit to the
reader. Better yet would be to move the two paths (GTT/CPU) into their own
routines:

  ret = -EFAULT;
  if (can_gtt)
    ret = i915_gem_gtt_pwrite();
  /* Fallback to the slow shmem paths if we need to handle
   * GTT mapped user pointers, or otherwise can not do the upload in
   * place.
   */
  if (ret == -EFAULT)
    ret = i915_gem_cpu_pwrite();

And whilst you are here, can you incorporate this patch?
        else if (obj->gtt_space &&
+                obj->map_and_fenceable &&
                 obj->base.write_domain != I915_GEM_DOMAIN_CPU) {
                ret = i915_gem_object_pin(obj, 0, true);
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/5] drm/i915: rewrite shmem_pwrite_slow to use copy_from_user

[Chris Wilson]

On Sat, 17 Sep 2011 20:55:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> ... instead of get_user_pages, because that fails on non page-backed
> user addresses like e.g. a gtt mapping of a bo.
> 
> To get there essentially copy the vfs read path into pagecache. We
> can't call that right away because we have to take care of bit17
> swizzling. To not deadlock with our own pagefault handler we need
> to completely drop struct_mutex, reducing the atomicty-guarantees
> of our userspace abi. Implications for racing with other gem ioctl:
> 
> - execbuf, pwrite, pread: Due to -EFAULT fallback to slow paths there's
>   already the risk of the pwrite call not being atomic, no degration.
> - read/write access to mmaps: already fully racy, no degration.
> - set_tiling: Calling set_tiling while reading/writing is already
>   pretty much undefined, now it just got a bit worse. set_tiling is
>   only called by libdrm on unused/new bos, so no problem.
> - set_domain: When changing to the gtt domain while copying (without any
>   read/write access, e.g. for synchronization), we might leave unflushed
>   data in the cpu caches. The clflush_object at the end of pwrite_slow
>   takes care of this problem.
> - truncating of purgeable objects: the shmem_read_mapping_page call could
>   reinstate backing storage for truncated objects. The check at the end
>   of pwrite_slow takes care of this.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |  124 +++++++++++++++++++-------------------
>  1 files changed, 62 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9c28d48..c390bf8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -58,6 +58,7 @@ static void i915_gem_free_object_tail(struct drm_i915_gem_object *obj);
>  
>  static int i915_gem_inactive_shrink(struct shrinker *shrinker,
>  				    struct shrink_control *sc);
> +static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
>  
>  /* some bookkeeping */
>  static void i915_gem_info_add_obj(struct drm_i915_private *dev_priv,
> @@ -385,6 +386,32 @@ i915_gem_shmem_pread_fast(struct drm_device *dev,
>  	return 0;
>  }
>  
> +static inline int
> +copy_from_user_swizzled(char __user *gpu_vaddr, int gpu_offset,
__copy_from_user_swizzled as we user the fast/non-checking variant of
__copy_from_user. (Ditto for the following patch)

> +			const char *cpu_vaddr,
> +			int length)
> +{
> +	int ret, cpu_offset = 0;
> +
> +	while (length > 0) {
> +		int cacheline_end = ALIGN(gpu_offset + 1, 64);
> +		int this_length = min(cacheline_end - gpu_offset, length);
> +		int swizzled_gpu_offset = gpu_offset ^ 64;
> +
> +		ret = __copy_from_user(gpu_vaddr + swizzled_gpu_offset,
> +				       cpu_vaddr + cpu_offset,
> +				       this_length);
> +		if (ret)
> +			return ret + length;
> +
> +		cpu_offset += this_length;
> +		gpu_offset += this_length;
> +		length -= this_length;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * This is the fallback shmem pread path, which allocates temporary storage
>   * in kernel space to copy_to_user into outside of the struct_mutex, so we
> @@ -841,71 +868,36 @@ i915_gem_shmem_pwrite_slow(struct drm_device *dev,
>  			   struct drm_file *file)
>  {
>  	struct address_space *mapping = obj->base.filp->f_path.dentry->d_inode->i_mapping;
> -	struct mm_struct *mm = current->mm;
> -	struct page **user_pages;
>  	ssize_t remain;
> -	loff_t offset, pinned_pages, i;
> -	loff_t first_data_page, last_data_page, num_pages;
> -	int shmem_page_offset;
> -	int data_page_index,  data_page_offset;
> -	int page_length;
> -	int ret;
> -	uint64_t data_ptr = args->data_ptr;
> -	int do_bit17_swizzling;
> +	loff_t offset;
> +	char __user *user_data;
> +	int shmem_page_offset, page_length, ret;
> +	int obj_do_bit17_swizzling, page_do_bit17_swizzling;
>  
> +	user_data = (char __user *) (uintptr_t) args->data_ptr;
>  	remain = args->size;
>  
> -	/* Pin the user pages containing the data.  We can't fault while
> -	 * holding the struct mutex, and all of the pwrite implementations
> -	 * want to hold it while dereferencing the user data.
> -	 */
> -	first_data_page = data_ptr / PAGE_SIZE;
> -	last_data_page = (data_ptr + args->size - 1) / PAGE_SIZE;
> -	num_pages = last_data_page - first_data_page + 1;
> -
> -	user_pages = drm_malloc_ab(num_pages, sizeof(struct page *));
> -	if (user_pages == NULL)
> -		return -ENOMEM;
> -
> -	mutex_unlock(&dev->struct_mutex);
> -	down_read(&mm->mmap_sem);
> -	pinned_pages = get_user_pages(current, mm, (uintptr_t)args->data_ptr,
> -				      num_pages, 0, 0, user_pages, NULL);
> -	up_read(&mm->mmap_sem);
> -	mutex_lock(&dev->struct_mutex);
> -	if (pinned_pages < num_pages) {
> -		ret = -EFAULT;
> -		goto out;
> -	}
> -
> -	ret = i915_gem_object_set_to_cpu_domain(obj, 1);
> -	if (ret)
> -		goto out;
> -
> -	do_bit17_swizzling = i915_gem_object_needs_bit17_swizzle(obj);
> +	obj_do_bit17_swizzling = i915_gem_object_needs_bit17_swizzle(obj);
>  
>  	offset = args->offset;
>  	obj->dirty = 1;
>  
> +	mutex_unlock(&dev->struct_mutex);
> +
>  	while (remain > 0) {
>  		struct page *page;
> +		char *vaddr;
>  
>  		/* Operation in this page
>  		 *
>  		 * shmem_page_offset = offset within page in shmem file
> -		 * data_page_index = page number in get_user_pages return
> -		 * data_page_offset = offset with data_page_index page.
>  		 * page_length = bytes to copy for this page
>  		 */
>  		shmem_page_offset = offset_in_page(offset);
> -		data_page_index = data_ptr / PAGE_SIZE - first_data_page;
> -		data_page_offset = offset_in_page(data_ptr);
>  
>  		page_length = remain;
>  		if ((shmem_page_offset + page_length) > PAGE_SIZE)
>  			page_length = PAGE_SIZE - shmem_page_offset;
> -		if ((data_page_offset + page_length) > PAGE_SIZE)
> -			page_length = PAGE_SIZE - data_page_offset;
>  
>  		page = shmem_read_mapping_page(mapping, offset >> PAGE_SHIFT);
>  		if (IS_ERR(page)) {
> @@ -913,34 +905,42 @@ i915_gem_shmem_pwrite_slow(struct drm_device *dev,
>  			goto out;
>  		}
>  
> -		if (do_bit17_swizzling) {
> -			slow_shmem_bit17_copy(page,
> -					      shmem_page_offset,
> -					      user_pages[data_page_index],
> -					      data_page_offset,
> -					      page_length,
> -					      0);
> -		} else {
> -			slow_shmem_copy(page,
> -					shmem_page_offset,
> -					user_pages[data_page_index],
> -					data_page_offset,
> -					page_length);
> -		}
> +		page_do_bit17_swizzling = (page_to_phys(page) & (1 << 17)) == 0;
> +
> +		vaddr = kmap(page);
> +		if (obj_do_bit17_swizzling && page_do_bit17_swizzling)
> +			ret = copy_from_user_swizzled(vaddr, shmem_page_offset,
> +						      user_data,
> +						      page_length);
> +		else
> +			ret = __copy_from_user(vaddr + shmem_page_offset,
> +					       user_data,
> +					       page_length);
> +		kunmap(page);
>  
>  		set_page_dirty(page);
>  		mark_page_accessed(page);
>  		page_cache_release(page);
>  
> +		if (ret) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
>  		remain -= page_length;
> -		data_ptr += page_length;
> +		user_data += page_length;
>  		offset += page_length;
>  	}
>  
>  out:
> -	for (i = 0; i < pinned_pages; i++)
> -		page_cache_release(user_pages[i]);
> -	drm_free_large(user_pages);
> +	mutex_lock(&dev->struct_mutex);
> +	/* Fixup: Kill any reinstated backing storage pages */
> +	if (obj->madv == __I915_MADV_PURGED)
> +		i915_gem_object_truncate(obj);
> +	/* and flush dirty cachelines in case the object isn't in the cpu write
> +	 * domain anymore. */
> +	if (obj->base.write_domain != I915_GEM_DOMAIN_CPU)
> +		i915_gem_clflush_object(obj);

This is a little too scary. And enough for me to have doubts over the
safety of the patch.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/5] drm/i915: rewrite shmem_pread_slow to use copy_to_user

[Chris Wilson]

On Sat, 17 Sep 2011 20:55:49 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Like for shmem_pwrite_slow. The only difference is that because we
> read data, we can leave the fetched cachelines in the cpu: In the case
> that the object isn't in the cpu read domain anymore, the clflush for
> the next cpu read domain invalidation will simply drop these
> cachelines.
> 
> slow_shmem_bit17_copy is now ununsed, so kill it.
> 
> With this patch tests/gem_mmap_gtt now actually works.

I have to ask the obvious question: does this have any impact on CPU
pwrite performance?

Bring on prefaulting ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 0/5] Fixup pwrite/pread with gtt mmapped user addresses

[Ben Widawsky]

On Sat, 17 Sep 2011 20:55:44 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Hi all,
> 
> Original bugreport: https://bugs.freedesktop.org/show_bug.cgi?id=38115
> 
> This bug can e.g. be hit with glBufferData(dst, glBufferMap(src))
> 
> Testcase is gem_mmap_gtt.
> 
> Neglecting the trivial cleanup patch 2, patch 1 converts the possible
> deadlock in an errornous -EFAULT. I've already submitted this, but
> include it here again for completeness.  Patches 3-5 then fix this up.
> Imo there's a bit more work left to do (like properly prefaulting,
> which would have papered over this bug).  But I think this is the
> minimal set required to fix the bug and I've wanted to get early
> feedback on the approach and on possible race issues.
> 
> I think patch 3-5 are not cc:stable material because they're too
> invasive (maybe even stuff for -next). The prefaulting patch will
> be much simpler, less invasive and it essentially reduces the issue to
> a tiny race, so I suggest that for backporting.
> 
> So review, comments highly appreciated.
> 
> Yours, Daniel
> 
> Daniel Vetter (5):
>   io-mapping: ensure io_mapping_map_atomic _is_ atomic
>   drm/i915: drop KM_USER0 argument to k(un)map_atomic
>   drm/i915: fall through pwrite_gtt_slow to the shmem slow path
>   drm/i915: rewrite shmem_pwrite_slow to use copy_from_user
>   drm/i915: rewrite shmem_pread_slow to use copy_to_user
> 
>  drivers/gpu/drm/i915/i915_gem.c       |  324
> ++++++++++++++-------------------
> drivers/gpu/drm/i915/i915_gem_debug.c |    6 +-
> include/linux/io-mapping.h            |    2 + 3 files changed, 140
> insertions(+), 192 deletions(-)
> 

Still in process of review, but:
Tested-by: Ben Widawsky <ben@bwidawsk.net>

Re: [PATCH 4/5] drm/i915: rewrite shmem_pwrite_slow to use copy_from_user

[Daniel Vetter]

On Sat, Sep 17, 2011 at 10:00:48PM +0100, Chris Wilson wrote:
> On Sat, 17 Sep 2011 20:55:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > +static inline int
> > +copy_from_user_swizzled(char __user *gpu_vaddr, int gpu_offset,
> __copy_from_user_swizzled as we user the fast/non-checking variant of
> __copy_from_user. (Ditto for the following patch)

Good idea.
> >  out:
> > -	for (i = 0; i < pinned_pages; i++)
> > -		page_cache_release(user_pages[i]);
> > -	drm_free_large(user_pages);
> > +	mutex_lock(&dev->struct_mutex);
> > +	/* Fixup: Kill any reinstated backing storage pages */
> > +	if (obj->madv == __I915_MADV_PURGED)
> > +		i915_gem_object_truncate(obj);
> > +	/* and flush dirty cachelines in case the object isn't in the cpu write
> > +	 * domain anymore. */
> > +	if (obj->base.write_domain != I915_GEM_DOMAIN_CPU)
> > +		i915_gem_clflush_object(obj);
> 
> This is a little too scary. And enough for me to have doubts over the
> safety of the patch.

I agree in terms of scariness. Now the only other option I'm seeing is
copying the data from userspace into some temporary storage outside of
struct_mutex so that we can then savely move it to the right place under
the protection of struct_mutex. I find that approach even more
unsatisfactory.

So here's why I think this is safe:

First about not breaking the userspace abi: We already drop the
struct_mutex when falling back to the slow paths, so userspace is already
facing the risk of seein partial writes when racing with itself. So I
think anything that reads/writes data concurrently is already undefined,
so no problem. For the same reasons, set_tiling can already race with
pwrite/pread and furthermore set_tiling is only called on unused buffers
by libdrm.

This imo only leaves set_domain(GTT) without any following read/writes as
a sensible ioctl to consider. For example when one process syncs up with
gpu excution while another does some s/w fallback uploading. Now this is
obviously a nice case of userspace racing with itself, so the only thing
we have to guarantee is that the pwrite has completely when the pwrite
ioctl finished with errno=0 and is coherent wrt the domain tracking. I.e.
we're not allowed to leave unflushed dirty cachelines behind if the
write_domain is no longer the cpu domain. The clflush (and the missing
chipset flush) take care of that.

Second aspect is whether userspace can screw over the kernel. So lets
carefully check what invariants the code outside struct_mutex depends on
and what state it is mangling:

The code only needs the shmem mapping (which cannot disapear because we're
holding a ref on the gem_object which in turn is holding a ref to the
underlying shmem file for its entire lifetime) and the userspace pointers
(which is handled by copy_*_user). For correctness we also depend upon the
bit17 swizzling state, but that can only go stale if userspaces races a
set_tiling call with the ongoing pwrite. And the kernel doesn't care about
writing undefined stuff into the backing storage if that is what userspace
demands.

Also note that we already need to handle error case from
shmem_read_mapping, so in case we change our madv truncating behavior in
the future to actually truncate the underlying inode (and not just drop
the backing storage) we're still save.

So lets look at what state we're touching outside the struct_mutex: I see
to things relevant to i915/gem: We might (re-)instate backing storage
pages and we obviously pollute the cpu caches. These two things get fixed
up in case userspace tries to screw over the kernel. After these two
fixups, the bo state is consistent and we can return;

Now besides that this fixes the bug without some ugly temp storage hack, I
see a few other things in favour of this approach:
- shmem slow&fastpaths are now almost identical, code-flow wise. Unifying
  these safes quite a few lines of not-so-heavily-used code. Also this
  opens up the possibility of further cleanups and codesize reductions I
  have in mind.
- pwrite/pread now work rather similar to pagecache read/write (especially
  with the unified slow/fast paths), and I like consistency.

So in conclusion I really think this is the best way to fix this, despite
the scariness of the fixup path.

-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/5] drm/i915: fall through pwrite_gtt_slow to the shmem slow path

[Daniel Vetter]

On Sat, Sep 17, 2011 at 09:57:05PM +0100, Chris Wilson wrote:
> On Sat, 17 Sep 2011 20:55:47 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > The gtt_pwrite slowpath grabs the userspace memory with
> > get_user_pages. This will not work for non-page backed memory, like a
> > gtt mmapped gem object. Hence fall throuh to the shmem paths if we hit
> > -EFAULT in the gtt paths.
> > 
> > Now the shmem paths have exactly the same problem, but this way we
> > only need to rearrange the code in one write path.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c |   24 +++++++++++++++---------
> >  1 files changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index e0475ca..9c28d48 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1018,18 +1018,24 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> >  
> >  out_unpin:
> >  		i915_gem_object_unpin(obj);
> > -	} else {
> > -		ret = i915_gem_object_set_to_cpu_domain(obj, 1);
> > -		if (ret)
> > -			goto out;
> >  
> > -		ret = -EFAULT;
> > -		if (!i915_gem_object_needs_bit17_swizzle(obj))
> > -			ret = i915_gem_shmem_pwrite_fast(dev, obj, args, file);
> > -		if (ret == -EFAULT)
> > -			ret = i915_gem_shmem_pwrite_slow(dev, obj, args, file);
> > +		if (ret != -EFAULT)
> > +			goto out;
> > +		/* Fall through to the shmfs paths because the gtt paths might
> > +		 * fail with non-page-backed user pointers (e.g. gtt mappings
> > +		 * when moving data between textures). */
> >  	}
> 
> I'd actually prefer to have an
>   if (ret == -EFAULT) {
> and comments here in order to mark the shmem paths as a logical unit to the
> reader. Better yet would be to move the two paths (GTT/CPU) into their own
> routines:
> 
>   ret = -EFAULT;
>   if (can_gtt)
>     ret = i915_gem_gtt_pwrite();
>   /* Fallback to the slow shmem paths if we need to handle
>    * GTT mapped user pointers, or otherwise can not do the upload in
>    * place.
>    */
>   if (ret == -EFAULT)
>     ret = i915_gem_cpu_pwrite();

With my full set of pwrite/pread patches, this is essentially what the
code will look like (with the minor addition to correctly handle
phys_objects, which I've fumbled in this patch here).

> And whilst you are here, can you incorporate this patch?
>         else if (obj->gtt_space &&
> +                obj->map_and_fenceable &&
>                  obj->base.write_domain != I915_GEM_DOMAIN_CPU) {
>                 ret = i915_gem_object_pin(obj, 0, true);

Not sure this is good. On !llc machines, gtt_pwrite is much faster, so we
want to move the objects into the mappable part of the gtt to benefit from
that. Without this, they'll just stay wherever they are.

Otoh our current allocator doesn't even try to seperate mappable and
!mappable allocations, so there's some decent chance they unnecessarily
fight over each another. I'll fix up the quick hack I've sent you to
further test this.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/5] drm/i915: drop KM_USER0 argument to k(un)map_atomic

[Daniel Vetter]

Hi Keith,

This patch adapts a few leftover callers to the new kmap_atomic api. Found
while working in this area.  Please merge for -next.

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 0/5] Fixup pwrite/pread with gtt mmapped user addresses

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 491 bytes --]

On Sat, 17 Sep 2011 20:55:44 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

>   io-mapping: ensure io_mapping_map_atomic _is_ atomic
>   drm/i915: drop KM_USER0 argument to k(un)map_atomic

These have been merged.

>   drm/i915: fall through pwrite_gtt_slow to the shmem slow path
>   drm/i915: rewrite shmem_pwrite_slow to use copy_from_user
>   drm/i915: rewrite shmem_pread_slow to use copy_to_user

These don't seem to be finished yet.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 3/5] drm/i915: fall through pwrite_gtt_slow to the shmem slow path

[Chris Wilson]

On Tue, 20 Sep 2011 13:35:56 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, Sep 17, 2011 at 09:57:05PM +0100, Chris Wilson wrote:
> > And whilst you are here, can you incorporate this patch?
> >         else if (obj->gtt_space &&
> > +                obj->map_and_fenceable &&
> >                  obj->base.write_domain != I915_GEM_DOMAIN_CPU) {
> >                 ret = i915_gem_object_pin(obj, 0, true);
> 
> Not sure this is good. On !llc machines, gtt_pwrite is much faster, so we
> want to move the objects into the mappable part of the gtt to benefit from
> that. Without this, they'll just stay wherever they are.

This turns out to be a big win for machines where it avoids the pipeline
stall due to the eviction of an active page and allowing us to utilize
the full GTT for vertex data.

10% on pnv, 60% on snb for x11perf -aa10text. (Though on SNB this is
eclipsed by using LLC and a test for obj->cache_level). And it is likely
to be an improvement with geometry bound game benchmarks like openarena.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre