Re: [Intel-gfx] [CI 2/2] drm/i915/gem: Pull phys pread/pwrite implementations to the backend

["Ruhl, Michael J" <michael.j.ruhl@intel.com>]

>-----Original Message-----
>From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Chris
>Wilson
>Sent: Thursday, November 5, 2020 10:50 AM
>To: intel-gfx@lists.freedesktop.org
>Subject: [Intel-gfx] [CI 2/2] drm/i915/gem: Pull phys pread/pwrite
>implementations to the backend
>
>Move the specialised interactions with the physical GEM object from the
>pread/pwrite ioctl handler into the phys backend.
>
>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>---
> drivers/gpu/drm/i915/gem/i915_gem_phys.c | 55
>++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem.c          | 26 -----------
> 2 files changed, 55 insertions(+), 26 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
>b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
>index 28147aab47b9..3a4dfe2ef1da 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
>@@ -134,6 +134,58 @@ i915_gem_object_put_pages_phys(struct
>drm_i915_gem_object *obj,
> 			  vaddr, dma);
> }
>
>+static int
>+phys_pwrite(struct drm_i915_gem_object *obj,
>+	    const struct drm_i915_gem_pwrite *args)
>+{
>+	void *vaddr = sg_page(obj->mm.pages->sgl) + args->offset;
>+	char __user *user_data = u64_to_user_ptr(args->data_ptr);
>+	int err;
>+
>+	err = i915_gem_object_wait(obj,
>+				   I915_WAIT_INTERRUPTIBLE |
>+				   I915_WAIT_ALL,
>+				   MAX_SCHEDULE_TIMEOUT);
>+	if (err)
>+		return err;
>+
>+	/*
>+	 * We manually control the domain here and pretend that it
>+	 * remains coherent i.e. in the GTT domain, like shmem_pwrite.
>+	 */
>+	i915_gem_object_invalidate_frontbuffer(obj, ORIGIN_CPU);
>+
>+	if (copy_from_user(vaddr, user_data, args->size))
>+		return -EFAULT;
>+
>+	drm_clflush_virt_range(vaddr, args->size);
>+	intel_gt_chipset_flush(&to_i915(obj->base.dev)->gt);
>+
>+	i915_gem_object_flush_frontbuffer(obj, ORIGIN_CPU);
>+	return 0;

So the original code path (through pwrite_ioctl()) includes a pin/unpin.

That doesn't need to be done here?

Thanks,

Mike


>+}
>+
>+static int
>+phys_pread(struct drm_i915_gem_object *obj,
>+	   const struct drm_i915_gem_pread *args)
>+{
>+	void *vaddr = sg_page(obj->mm.pages->sgl) + args->offset;
>+	char __user *user_data = u64_to_user_ptr(args->data_ptr);
>+	int err;
>+
>+	err = i915_gem_object_wait(obj,
>+				   I915_WAIT_INTERRUPTIBLE,
>+				   MAX_SCHEDULE_TIMEOUT);
>+	if (err)
>+		return err;
>+
>+	drm_clflush_virt_range(vaddr, args->size);
>+	if (copy_to_user(user_data, vaddr, args->size))
>+		return -EFAULT;
>+
>+	return 0;
>+}
>+
> static void phys_release(struct drm_i915_gem_object *obj)
> {
> 	fput(obj->base.filp);
>@@ -144,6 +196,9 @@ static const struct drm_i915_gem_object_ops
>i915_gem_phys_ops = {
> 	.get_pages = i915_gem_object_get_pages_phys,
> 	.put_pages = i915_gem_object_put_pages_phys,
>
>+	.pread  = phys_pread,
>+	.pwrite = phys_pwrite,
>+
> 	.release = phys_release,
> };
>
>diff --git a/drivers/gpu/drm/i915/i915_gem.c
>b/drivers/gpu/drm/i915/i915_gem.c
>index d58fe1ddc3e1..58276694c848 100644
>--- a/drivers/gpu/drm/i915/i915_gem.c
>+++ b/drivers/gpu/drm/i915/i915_gem.c
>@@ -179,30 +179,6 @@ int i915_gem_object_unbind(struct
>drm_i915_gem_object *obj,
> 	return ret;
> }
>
>-static int
>-i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
>-		     struct drm_i915_gem_pwrite *args,
>-		     struct drm_file *file)
>-{
>-	void *vaddr = sg_page(obj->mm.pages->sgl) + args->offset;
>-	char __user *user_data = u64_to_user_ptr(args->data_ptr);
>-
>-	/*
>-	 * We manually control the domain here and pretend that it
>-	 * remains coherent i.e. in the GTT domain, like shmem_pwrite.
>-	 */
>-	i915_gem_object_invalidate_frontbuffer(obj, ORIGIN_CPU);
>-
>-	if (copy_from_user(vaddr, user_data, args->size))
>-		return -EFAULT;
>-
>-	drm_clflush_virt_range(vaddr, args->size);
>-	intel_gt_chipset_flush(&to_i915(obj->base.dev)->gt);
>-
>-	i915_gem_object_flush_frontbuffer(obj, ORIGIN_CPU);
>-	return 0;
>-}
>-
> static int
> i915_gem_create(struct drm_file *file,
> 		struct intel_memory_region *mr,
>@@ -872,8 +848,6 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void
>*data,
> 	if (ret == -EFAULT || ret == -ENOSPC) {
> 		if (i915_gem_object_has_struct_page(obj))
> 			ret = i915_gem_shmem_pwrite(obj, args);
>-		else
>-			ret = i915_gem_phys_pwrite(obj, args, file);
> 	}
>
> 	i915_gem_object_unpin_pages(obj);
>--
>2.20.1
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx