diff mbox series

[CI,2/2] drm/i915/gem: Pull phys pread/pwrite implementations to the backend

Message ID 20201105154934.16022-2-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [CI,1/2] drm/i915/gem: Allow backends to override pread implementation | expand

Commit Message

Chris Wilson Nov. 5, 2020, 3:49 p.m. UTC 
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(-)

Comments

Chris Wilson Nov. 5, 2020, 6:41 p.m. UTC | #1 
Quoting Chris Wilson (2020-11-05 15:49:34)
> Move the specialised interactions with the physical GEM object from the
> pread/pwrite ioctl handler into the phys backend.
> 

Fixes: c6790dc22312 ("drm/i915: Wean off drm_pci_alloc/drm_pci_free")
Testcase: igt/gem_pwrite/exhaustion
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Cc: stable@vger.kernel.org
-Chris
Michael J. Ruhl Nov. 9, 2020, 1:41 p.m. UTC | #2 
>-----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
Rodrigo Vivi Nov. 10, 2020, 7:03 a.m. UTC | #3 
On Thu, Nov 05, 2020 at 06:41:25PM +0000, Chris Wilson wrote:
> Quoting Chris Wilson (2020-11-05 15:49:34)
> > Move the specialised interactions with the physical GEM object from the
> > pread/pwrite ioctl handler into the phys backend.

which depends on the backend...

> > 
> 
> Fixes: c6790dc22312 ("drm/i915: Wean off drm_pci_alloc/drm_pci_free")

which was a fixes to 4.5 stable, but I could only find it into 5.4+
so...

> Testcase: igt/gem_pwrite/exhaustion
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> Cc: stable@vger.kernel.org

When cherry-picking to drm-intel-fixes, should I add

Cc: stable@vger.kernel.org # 5.4.y: c6790dc22312: drm/i915: Wean off drm_pci_alloc/drm_pci_free
Cc: stable@vger.kernel.org # 5.4.y

> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

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;
+}
+
+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);