mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Eric Anholt <eric@anholt.net>
To: linux-kernel@vger.kernel.org
Cc: dri-devel@lists.sourceforge.net, Eric Anholt <eric@anholt.net>
Subject: [PATCH 3/6] drm/i915: Fix lock order reversal in shmem pwrite path.
Date: Wed, 25 Mar 2009 14:45:07 -0700	[thread overview]
Message-ID: <1238017510-26784-4-git-send-email-eric@anholt.net> (raw)
In-Reply-To: <1238017510-26784-3-git-send-email-eric@anholt.net>

Like the GTT pwrite path fix, this uses an optimistic path and a
fallback to get_user_pages.  Note that this means we have to stop using
vfs_write and roll it ourselves.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/i915/i915_gem.c |  225 +++++++++++++++++++++++++++++++++++----
 1 files changed, 205 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b998d65..bdc7326 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -136,6 +136,33 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
 	return 0;
 }
 
+static inline int
+slow_shmem_copy(struct page *dst_page,
+		int dst_offset,
+		struct page *src_page,
+		int src_offset,
+		int length)
+{
+	char *dst_vaddr, *src_vaddr;
+
+	dst_vaddr = kmap_atomic(dst_page, KM_USER0);
+	if (dst_vaddr == NULL)
+		return -ENOMEM;
+
+	src_vaddr = kmap_atomic(src_page, KM_USER1);
+	if (src_vaddr == NULL) {
+		kunmap_atomic(dst_vaddr, KM_USER0);
+		return -ENOMEM;
+	}
+
+	memcpy(dst_vaddr + dst_offset, src_vaddr + src_offset, length);
+
+	kunmap_atomic(src_vaddr, KM_USER1);
+	kunmap_atomic(dst_vaddr, KM_USER0);
+
+	return 0;
+}
+
 /**
  * Reads data from the object referenced by handle.
  *
@@ -243,6 +270,23 @@ slow_kernel_write(struct io_mapping *mapping,
 	return 0;
 }
 
+static inline int
+fast_shmem_write(struct page **pages,
+		 loff_t page_base, int page_offset,
+		 char __user *data,
+		 int length)
+{
+	char __iomem *vaddr;
+
+	vaddr = kmap_atomic(pages[page_base >> PAGE_SHIFT], KM_USER0);
+	if (vaddr == NULL)
+		return -ENOMEM;
+	__copy_from_user_inatomic(vaddr + page_offset, data, length);
+	kunmap_atomic(vaddr, KM_USER0);
+
+	return 0;
+}
+
 /**
  * This is the fast pwrite path, where we copy the data directly from the
  * user into the GTT, uncached.
@@ -423,39 +467,175 @@ out_unpin_pages:
 	return ret;
 }
 
+/**
+ * This is the fast shmem pwrite path, which attempts to directly
+ * copy_from_user into the kmapped pages backing the object.
+ */
 static int
-i915_gem_shmem_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
-		      struct drm_i915_gem_pwrite *args,
-		      struct drm_file *file_priv)
+i915_gem_shmem_pwrite_fast(struct drm_device *dev, struct drm_gem_object *obj,
+			   struct drm_i915_gem_pwrite *args,
+			   struct drm_file *file_priv)
 {
+	struct drm_i915_gem_object *obj_priv = obj->driver_private;
+	ssize_t remain;
+	loff_t offset, page_base;
+	char __user *user_data;
+	int page_offset, page_length;
 	int ret;
-	loff_t offset;
-	ssize_t written;
+
+	user_data = (char __user *) (uintptr_t) args->data_ptr;
+	remain = args->size;
 
 	mutex_lock(&dev->struct_mutex);
 
+	ret = i915_gem_object_get_pages(obj);
+	if (ret != 0)
+		goto fail_unlock;
+
 	ret = i915_gem_object_set_to_cpu_domain(obj, 1);
-	if (ret) {
-		mutex_unlock(&dev->struct_mutex);
-		return ret;
+	if (ret != 0)
+		goto fail_put_pages;
+
+	obj_priv = obj->driver_private;
+	offset = args->offset;
+	obj_priv->dirty = 1;
+
+	while (remain > 0) {
+		/* Operation in this page
+		 *
+		 * page_base = page offset within aperture
+		 * page_offset = offset within page
+		 * page_length = bytes to copy for this page
+		 */
+		page_base = (offset & ~(PAGE_SIZE-1));
+		page_offset = offset & (PAGE_SIZE-1);
+		page_length = remain;
+		if ((page_offset + remain) > PAGE_SIZE)
+			page_length = PAGE_SIZE - page_offset;
+
+		ret = fast_shmem_write(obj_priv->pages,
+				       page_base, page_offset,
+				       user_data, page_length);
+		if (ret)
+			goto fail_put_pages;
+
+		remain -= page_length;
+		user_data += page_length;
+		offset += page_length;
 	}
 
+fail_put_pages:
+	i915_gem_object_put_pages(obj);
+fail_unlock:
+	mutex_unlock(&dev->struct_mutex);
+
+	return ret;
+}
+
+/**
+ * This is the fallback shmem pwrite path, which uses get_user_pages to pin
+ * the memory and maps it using kmap_atomic for copying.
+ *
+ * This avoids taking mmap_sem for faulting on the user's address while the
+ * struct_mutex is held.
+ */
+static int
+i915_gem_shmem_pwrite_slow(struct drm_device *dev, struct drm_gem_object *obj,
+			   struct drm_i915_gem_pwrite *args,
+			   struct drm_file *file_priv)
+{
+	struct drm_i915_gem_object *obj_priv = obj->driver_private;
+	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_index, shmem_page_offset;
+	int data_page_index,  data_page_offset;
+	int page_length;
+	int ret;
+	uint64_t data_ptr = 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 = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
+	if (user_pages == NULL)
+		return -ENOMEM;
+
+	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);
+	if (pinned_pages < num_pages) {
+		ret = -EFAULT;
+		goto fail_put_user_pages;
+	}
+
+	mutex_lock(&dev->struct_mutex);
+
+	ret = i915_gem_object_get_pages(obj);
+	if (ret != 0)
+		goto fail_unlock;
+
+	ret = i915_gem_object_set_to_cpu_domain(obj, 1);
+	if (ret != 0)
+		goto fail_put_pages;
+
+	obj_priv = obj->driver_private;
 	offset = args->offset;
+	obj_priv->dirty = 1;
 
-	written = vfs_write(obj->filp,
-			    (char __user *)(uintptr_t) args->data_ptr,
-			    args->size, &offset);
-	if (written != args->size) {
-		mutex_unlock(&dev->struct_mutex);
-		if (written < 0)
-			return written;
-		else
-			return -EINVAL;
+	while (remain > 0) {
+		/* Operation in this page
+		 *
+		 * shmem_page_index = page number within shmem file
+		 * 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_index = offset / PAGE_SIZE;
+		shmem_page_offset = offset & ~PAGE_MASK;
+		data_page_index = data_ptr / PAGE_SIZE - first_data_page;
+		data_page_offset = data_ptr & ~PAGE_MASK;
+
+		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;
+
+		ret = slow_shmem_copy(obj_priv->pages[shmem_page_index],
+				      shmem_page_offset,
+				      user_pages[data_page_index],
+				      data_page_offset,
+				      page_length);
+		if (ret)
+			goto fail_put_pages;
+
+		remain -= page_length;
+		data_ptr += page_length;
+		offset += page_length;
 	}
 
+fail_put_pages:
+	i915_gem_object_put_pages(obj);
+fail_unlock:
 	mutex_unlock(&dev->struct_mutex);
+fail_put_user_pages:
+	for (i = 0; i < pinned_pages; i++)
+		page_cache_release(user_pages[i]);
+	kfree(user_pages);
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -502,8 +682,13 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 			ret = i915_gem_gtt_pwrite_slow(dev, obj, args,
 						       file_priv);
 		}
-	} else
-		ret = i915_gem_shmem_pwrite(dev, obj, args, file_priv);
+	} else {
+		ret = i915_gem_shmem_pwrite_fast(dev, obj, args, file_priv);
+		if (ret == -EFAULT) {
+			ret = i915_gem_shmem_pwrite_slow(dev, obj, args,
+							 file_priv);
+		}
+	}
 
 #if WATCH_PWRITE
 	if (ret)
-- 
1.6.2.1


  reply	other threads:[~2009-03-25 21:46 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-03-25 21:45 DRM lock ordering fix series Eric Anholt
2009-03-25 21:45 ` [PATCH 1/6] drm/i915: Fix lock order reversal in GTT pwrite path Eric Anholt
2009-03-25 21:45   ` [PATCH 2/6] drm/i915: Make GEM object's page lists refcounted instead of get/free Eric Anholt
2009-03-25 21:45     ` Eric Anholt [this message]
2009-03-25 21:45       ` [PATCH 4/6] drm/i915: Fix lock order reversal in shmem pread path Eric Anholt
2009-03-25 21:45         ` [PATCH 5/6] drm/i915: Fix lock order reversal with cliprects and cmdbuf in non-DRI2 paths Eric Anholt
2009-03-25 21:45           ` [PATCH 6/6] drm/i915: Fix lock order reversal in GEM relocation entry copying Eric Anholt
2009-03-30 10:00             ` [PATCH 6/6] drm/i915: Fix lock order reversal in GEM relocation entry copying. -- makes X hang Florian Mickler
2009-03-31 19:36               ` Eric Anholt
2009-04-01  0:12                 ` Florian Mickler
2009-03-27  0:52           ` [PATCH 5/6] drm/i915: Fix lock order reversal with cliprects and cmdbuf in non-DRI2 paths Jesse Barnes
2009-03-25 23:30         ` [PATCH 4/6] drm/i915: Fix lock order reversal in shmem pread path Dave Airlie
2009-03-26  4:03           ` Keith Packard
2009-03-27  0:50         ` Jesse Barnes
2009-03-27  0:50       ` [PATCH 3/6] drm/i915: Fix lock order reversal in shmem pwrite path Jesse Barnes
2009-03-25 22:52     ` [PATCH 2/6] drm/i915: Make GEM object's page lists refcounted instead of get/free Dave Airlie
2009-03-26 19:59       ` Eric Anholt
2009-03-27  0:47     ` Jesse Barnes
2009-03-27  0:43   ` [PATCH 1/6] drm/i915: Fix lock order reversal in GTT pwrite path Jesse Barnes
2009-03-27 16:56     ` Eric Anholt
2009-03-27 17:07       ` Jesse Barnes
2009-03-28  0:54     ` Peter Zijlstra
2009-03-28  2:35       ` Jesse Barnes
2009-03-28  5:22         ` Dave Airlie
2009-03-27  9:34 ` DRM lock ordering fix series Andi Kleen
2009-03-27 16:19   ` Eric Anholt
2009-03-27 16:36     ` Eric Anholt
2009-03-27 18:10       ` Andi Kleen
2009-03-27 20:10         ` Eric Anholt
2009-03-27 21:05           ` Andi Kleen
2009-03-28  0:58           ` Peter Zijlstra
2009-03-28  1:29             ` Peter Zijlstra
2009-03-30  6:29               ` Eric Anholt
2009-03-28  8:46             ` Brice Goglin
2009-03-28 10:48               ` Peter Zijlstra
2009-03-28 12:22                 ` [RFC] x86: gup_fast() batch limit (was: DRM lock ordering fix series) Peter Zijlstra
2009-03-28 12:46                   ` Peter Zijlstra
2009-04-02 11:19                     ` Nick Piggin
2009-06-24 13:46                       ` [RFC] x86: gup_fast() batch limit Brice Goglin
2009-06-24 17:07                         ` Peter Zijlstra
2009-06-24 19:55                         ` Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1238017510-26784-4-git-send-email-eric@anholt.net \
    --to=eric@anholt.net \
    --cc=dri-devel@lists.sourceforge.net \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome