[4/4] drm/i915: Support for pread/pwrite from/to non shmem backed objects

Commit Message

[e-mail] Sept. 15, 2015, 8:33 a.m. UTC

From: Ankitprasad Sharma <[e-mail]>

This patch adds support for extending the pread/pwrite functionality
for objects not backed by shmem. The access will be made through
gtt interface.
This will cover prime objects as well as stolen memory backed objects
but for userptr objects it is still forbidden.

v2: Drop locks around slow_user_access, prefault the pages before
access (Chris)

v3: Rebased to the latest drm-intel-nightly (Ankit)

v4: Moved page base & offset calculations outside the copy loop,
corrected data types for size and offset variables, corrected if-else
braces format (Tvrtko/kerneldocs)

v5: Enabled pread/pwrite for all non-shmem backed objects including
without tiling restrictions (Ankit)

Testcase: igt/gem_stolen

Signed-off-by: Ankitprasad Sharma <[e-mail]>
---
 drivers/gpu/drm/i915/i915_gem.c | 128 ++++++++++++++++++++++++++++++++++------
 1 file changed, 111 insertions(+), 17 deletions(-)

Comments

Chris Wilson Sept. 15, 2015, 9:54 a.m. UTC | #1

On Tue, Sep 15, 2015 at 02:03:27PM +0530, [e-mail] wrote:
> @@ -1090,17 +1184,17 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> -	/* prime objects have no backing filp to GEM pread/pwrite
> -	 * pages from.
> -	 */
> -	if (!obj->base.filp) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
>  	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
>  
>  	ret = -EFAULT;
> +
> +	/* pwrite for non shmem backed objects */
> +	if (!obj->base.filp) {
> +		ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
> +						args->offset, args->data_ptr,
> +						true);
> +		goto out;
> +	}

There already exists a GTT write path, along with a more correct
description of its limitations.
-Chris

[e-mail] Sept. 15, 2015, 10:35 a.m. UTC | #2

On Tue, 2015-09-15 at 10:54 +0100, Chris Wilson wrote:
> On Tue, Sep 15, 2015 at 02:03:27PM +0530, [e-mail] wrote:
> > @@ -1090,17 +1184,17 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> >  		goto out;
> >  	}
> >  
> > -	/* prime objects have no backing filp to GEM pread/pwrite
> > -	 * pages from.
> > -	 */
> > -	if (!obj->base.filp) {
> > -		ret = -EINVAL;
> > -		goto out;
> > -	}
> > -
> >  	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
> >  
> >  	ret = -EFAULT;
> > +
> > +	/* pwrite for non shmem backed objects */
> > +	if (!obj->base.filp) {
> > +		ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
> > +						args->offset, args->data_ptr,
> > +						true);
> > +		goto out;
> > +	}
> 
> There already exists a GTT write path, along with a more correct
> description of its limitations.

Then it would look something like this, making i915_gem_gtt_pwrite_fast
to handle pagefaults for non-shmem backed objects

@@ -831,10 +925,16 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
                 * retry in the slow path.
                 */
-         if (fast_user_write(dev_priv->gtt.mappable, page_base,
+            if (obj->base.filp &&
+                 fast_user_write(dev_priv->gtt.mappable, page_base,
                                 page_offset, user_data, page_length)) {
                        ret = -EFAULT;
                        goto out_flush;
+            } else if (slow_user_access(dev_priv->gtt.mappable,
+                                        page_base, page_offset,
+                                       user_data, page_length, true)) {
+                       ret = -EFAULT;
+                       goto out_flush;
             }
 
Thanks
-Ankit

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 85025b1..2df37ac 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -614,6 +614,102 @@  shmem_pread_slow(struct page *page, int shmem_page_offset, int page_length,
 	return ret ? - EFAULT : 0;
 }
 
+static inline uint64_t
+slow_user_access(struct io_mapping *mapping,
+		 uint64_t page_base, int page_offset,
+		 char __user *user_data,
+		 int length, bool pwrite)
+{
+	void __iomem *vaddr_inatomic;
+	void *vaddr;
+	uint64_t unwritten;
+
+	vaddr_inatomic = io_mapping_map_wc(mapping, page_base);
+	/* We can use the cpu mem copy function because this is X86. */
+	vaddr = (void __force *)vaddr_inatomic + page_offset;
+	if (pwrite)
+		unwritten = __copy_from_user(vaddr, user_data, length);
+	else
+		unwritten = __copy_to_user(user_data, vaddr, length);
+
+	io_mapping_unmap(vaddr_inatomic);
+	return unwritten;
+}
+
+static int
+i915_gem_gtt_pread_pwrite(struct drm_device *dev,
+			  struct drm_i915_gem_object *obj, uint64_t size,
+			  uint64_t data_offset, uint64_t data_ptr, bool pwrite)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	char __user *user_data;
+	uint64_t remain;
+	uint64_t offset, page_base;
+	int page_offset, page_length, ret = 0;
+
+	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
+	if (ret)
+		goto out;
+
+	ret = i915_gem_object_set_to_gtt_domain(obj, pwrite);
+	if (ret)
+		goto out_unpin;
+
+	ret = i915_gem_object_put_fence(obj);
+	if (ret)
+		goto out_unpin;
+
+	user_data = to_user_ptr(data_ptr);
+	remain = size;
+	offset = i915_gem_obj_ggtt_offset(obj) + data_offset;
+
+	if (pwrite)
+		intel_fb_obj_invalidate(obj, ORIGIN_GTT);
+
+	mutex_unlock(&dev->struct_mutex);
+	if (!pwrite && likely(!i915.prefault_disable))
+		ret = fault_in_multipages_writeable(user_data, remain);
+
+	/*
+	 * page_offset = offset within page
+	 * page_base = page offset within aperture
+	 */
+	page_offset = offset_in_page(offset);
+	page_base = offset & PAGE_MASK;
+
+	while (remain > 0) {
+		/* page_length = bytes to copy for this page */
+		page_length = remain;
+		if ((page_offset + remain) > PAGE_SIZE)
+			page_length = PAGE_SIZE - page_offset;
+
+		/* This is a slow read/write as it tries to read from
+		 * and write to user memory which may result into page
+		 * faults
+		 */
+		ret = slow_user_access(dev_priv->gtt.mappable, page_base,
+				       page_offset, user_data,
+				       page_length, pwrite);
+
+		if (ret) {
+			ret = -EFAULT;
+			break;
+		}
+
+		remain -= page_length;
+		user_data += page_length;
+		page_base += page_length;
+		page_offset = 0;
+	}
+
+	mutex_lock(&dev->struct_mutex);
+
+out_unpin:
+	i915_gem_object_ggtt_unpin(obj);
+out:
+	return ret;
+}
+
 static int
 i915_gem_shmem_pread(struct drm_device *dev,
 		     struct drm_i915_gem_object *obj,
@@ -737,17 +833,15 @@  i915_gem_pread_ioctl(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	/* prime objects have no backing filp to GEM pread/pwrite
-	 * pages from.
-	 */
-	if (!obj->base.filp) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	trace_i915_gem_object_pread(obj, args->offset, args->size);
 
-	ret = i915_gem_shmem_pread(dev, obj, args, file);
+	/* pread for non shmem backed objects */
+	if (!obj->base.filp)
+		ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
+						args->offset, args->data_ptr,
+						false);
+	else
+		ret = i915_gem_shmem_pread(dev, obj, args, file);
 
 out:
 	drm_gem_object_unreference(&obj->base);
@@ -1090,17 +1184,17 @@  i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	/* prime objects have no backing filp to GEM pread/pwrite
-	 * pages from.
-	 */
-	if (!obj->base.filp) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
 
 	ret = -EFAULT;
+
+	/* pwrite for non shmem backed objects */
+	if (!obj->base.filp) {
+		ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
+						args->offset, args->data_ptr,
+						true);
+		goto out;
+	}
 	/* We can only do the GTT pwrite on untiled buffers, as otherwise
 	 * it would end up going through the fenced access, and we'll get
 	 * different detiling behavior between reading and writing.