drm/i915: Make use of drm_gem_object_release
diff mbox series

Message ID 20200116192047.22303-1-brian.welty@intel.com
State New
Headers show
Series
  • drm/i915: Make use of drm_gem_object_release
Related show

Commit Message

Brian Welty Jan. 16, 2020, 7:20 p.m. UTC
As i915 is using drm_gem_private_object_init, it is best to
use the inverse function for cleanup: drm_gem_object_release.
This removes need for a shmem_release and phys_release.

Signed-off-by: Brian Welty <brian.welty@intel.com>
---
Chris, the cleanup sequence in drm_gem_object_release() vs the replaced
i915 code is different, but should be okay?  Light testing didn't find
any issues.
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c | 4 +---
 drivers/gpu/drm/i915/gem/i915_gem_phys.c   | 7 -------
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c  | 9 +--------
 3 files changed, 2 insertions(+), 18 deletions(-)

Comments

Chris Wilson Jan. 16, 2020, 7:30 p.m. UTC | #1
Quoting Brian Welty (2020-01-16 19:20:47)
> As i915 is using drm_gem_private_object_init, it is best to
> use the inverse function for cleanup: drm_gem_object_release.
> This removes need for a shmem_release and phys_release.
> 
> Signed-off-by: Brian Welty <brian.welty@intel.com>
> ---
> Chris, the cleanup sequence in drm_gem_object_release() vs the replaced
> i915 code is different, but should be okay?  Light testing didn't find
> any issues.

commit 0c159ffef628fa94d0f4f9128e7f2b6f2b5e86ef
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Jul 3 19:06:01 2019 +0100

    drm/i915/gem: Defer obj->base.resv fini until RCU callback

    Since reservation_object_fini() does an immediate free, rather than
    kfree_rcu as normal, we have to delay the release until after the RCU
    grace period has elapsed (i.e. from the rcu cleanup callback) so that we
    can rely on the RCU protected access to the fences while the object is a
    zombie.

    i915_gem_busy_ioctl relies on having an RCU barrier to protect the
    reservation in order to avoid having to take a reference and strong
    memory barriers.

    v2: Order is important; only release after putting the pages!

    Fixes: c03467ba40f7 ("drm/i915/gem: Free pages before rcu-freeing the object
")
    Testcase: igt/gem_busy/close-race
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Cc: Matthew Auld <matthew.auld@intel.com>
    Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
    Link: https://patchwork.freedesktop.org/patch/msgid/20190703180601.10950-1-c
hris@chris-wilson.co.uk
Brian Welty Jan. 16, 2020, 9:27 p.m. UTC | #2
On 1/16/2020 11:30 AM, Chris Wilson wrote:
> Quoting Brian Welty (2020-01-16 19:20:47)
>> As i915 is using drm_gem_private_object_init, it is best to
>> use the inverse function for cleanup: drm_gem_object_release.
>> This removes need for a shmem_release and phys_release.
>>
>> Signed-off-by: Brian Welty <brian.welty@intel.com>
>> ---
>> Chris, the cleanup sequence in drm_gem_object_release() vs the replaced
>> i915 code is different, but should be okay?  Light testing didn't find
>> any issues.
> 
> commit 0c159ffef628fa94d0f4f9128e7f2b6f2b5e86ef
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Wed Jul 3 19:06:01 2019 +0100
> 
>     drm/i915/gem: Defer obj->base.resv fini until RCU callback
> 
>     Since reservation_object_fini() does an immediate free, rather than
>     kfree_rcu as normal, we have to delay the release until after the RCU
>     grace period has elapsed (i.e. from the rcu cleanup callback) so that we
>     can rely on the RCU protected access to the fences while the object is a
>     zombie.
> 
>     i915_gem_busy_ioctl relies on having an RCU barrier to protect the
>     reservation in order to avoid having to take a reference and strong
>     memory barriers.
> 
>     v2: Order is important; only release after putting the pages!
> 
>     Fixes: c03467ba40f7 ("drm/i915/gem: Free pages before rcu-freeing the object
> ")
>     Testcase: igt/gem_busy/close-race
>     Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>     Cc: Matthew Auld <matthew.auld@intel.com>
>     Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>     Link: https://patchwork.freedesktop.org/patch/msgid/20190703180601.10950-1-c
> hris@chris-wilson.co.uk
> 

Thanks, I didn't check the history to see this was using drm_gem_object_release in past.
But looks to be using kfree_rcu now for the free.

Are we okay now as this patch has gone in since?:

commit 96e95496b02dbf1b19a2d4ce238810572e149606
Author: Christian K├Ânig <christian.koenig@amd.com>
Date:   Tue Aug 6 13:33:12 2019 +0200

    dma-buf: fix shared fence list handling in reservation_object_copy_fences

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 46bacc82ddc4..d51838d7d2ec 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -159,7 +159,6 @@  static void __i915_gem_free_object_rcu(struct rcu_head *head)
 		container_of(head, typeof(*obj), rcu);
 	struct drm_i915_private *i915 = to_i915(obj->base.dev);
 
-	dma_resv_fini(&obj->base._resv);
 	i915_gem_object_free(obj);
 
 	GEM_BUG_ON(!atomic_read(&i915->mm.free_count));
@@ -222,8 +221,7 @@  static void __i915_gem_free_objects(struct drm_i915_private *i915,
 		if (obj->base.import_attach)
 			drm_prime_gem_destroy(&obj->base, NULL);
 
-		drm_gem_free_mmap_offset(&obj->base);
-
+		drm_gem_object_release(&obj->base);
 		if (obj->ops->release)
 			obj->ops->release(obj);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
index b1b7c1b3038a..7c19f92f256b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
@@ -134,16 +134,9 @@  i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj,
 	drm_pci_free(obj->base.dev, obj->phys_handle);
 }
 
-static void phys_release(struct drm_i915_gem_object *obj)
-{
-	fput(obj->base.filp);
-}
-
 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,
-
-	.release = phys_release,
 };
 
 int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, int align)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index a2a980d9d241..4004cfe1e28a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -418,13 +418,6 @@  shmem_pwrite(struct drm_i915_gem_object *obj,
 	return 0;
 }
 
-static void shmem_release(struct drm_i915_gem_object *obj)
-{
-	i915_gem_object_release_memory_region(obj);
-
-	fput(obj->base.filp);
-}
-
 const struct drm_i915_gem_object_ops i915_gem_shmem_ops = {
 	.flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE |
 		 I915_GEM_OBJECT_IS_SHRINKABLE,
@@ -436,7 +429,7 @@  const struct drm_i915_gem_object_ops i915_gem_shmem_ops = {
 
 	.pwrite = shmem_pwrite,
 
-	.release = shmem_release,
+	.release = i915_gem_object_release_memory_region,
 };
 
 static int __create_shmem(struct drm_i915_private *i915,