From: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> To: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> Cc: intel-gfx@lists.freedesktop.org, matthew.auld@intel.com, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v2 2/3] drm/i915: Remove the vma refcount Date: Wed, 2 Mar 2022 14:01:41 -0800 [thread overview] Message-ID: <20220302220139.GH25117@nvishwa1-DESK> (raw) In-Reply-To: <20220302102200.158637-3-thomas.hellstrom@linux.intel.com> On Wed, Mar 02, 2022 at 11:21:59AM +0100, Thomas Hellström wrote: >Now that i915_vma_parked() is taking the object lock on vma destruction, >and the only user of the vma refcount, i915_gem_object_unbind() >also takes the object lock, remove the vma refcount. > >Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >--- > drivers/gpu/drm/i915/i915_gem.c | 17 +++++++++++++---- > drivers/gpu/drm/i915/i915_vma.c | 14 +++----------- > drivers/gpu/drm/i915/i915_vma.h | 14 -------------- > drivers/gpu/drm/i915/i915_vma_types.h | 1 - > 4 files changed, 16 insertions(+), 30 deletions(-) > >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >index dd84ebabb50f..c26110abcc0b 100644 >--- a/drivers/gpu/drm/i915/i915_gem.c >+++ b/drivers/gpu/drm/i915/i915_gem.c >@@ -151,14 +151,25 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj, > break; > } > >+ /* >+ * Requiring the vm destructor to take the object lock >+ * before destroying a vma would help us eliminate the >+ * i915_vm_tryget() here, AND thus also the barrier stuff >+ * at the end. That's an easy fix, but sleeping locks in >+ * a kthread should generally be avoided. >+ */ > ret = -EAGAIN; > if (!i915_vm_tryget(vma->vm)) > break; > >- /* Prevent vma being freed by i915_vma_parked as we unbind */ >- vma = __i915_vma_get(vma); > spin_unlock(&obj->vma.lock); > >+ /* >+ * Since i915_vma_parked() takes the object lock >+ * before vma destruction, it won't race us here, >+ * and destroy the vma from under us. >+ */ >+ > if (vma) { > bool vm_trylock = !!(flags & I915_GEM_OBJECT_UNBIND_VM_TRYLOCK); > ret = -EBUSY; >@@ -180,8 +191,6 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj, > ret = i915_vma_unbind(vma); > } > } >- >- __i915_vma_put(vma); > } > > i915_vm_put(vma->vm); One issue still left in i915_gem_object_unbind is that it temporarily removes vmas from the obj->vma.list and adds back later as vma needs to be unbind outside the obj->vma.lock spinlock. This is an issue for other places where we iterate over the obj->vma.list. i915_debugfs_describe_obj is one such case (upcoming vm_bind will be another) that iterates over this list. What is the plan here? Do we need to take object lock while iterating over the list? But this just something I noticed and not related to this patch. This patch looks good to me. Reviewed-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> >diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c >index 91538bc38110..6fd25b39748f 100644 >--- a/drivers/gpu/drm/i915/i915_vma.c >+++ b/drivers/gpu/drm/i915/i915_vma.c >@@ -122,7 +122,6 @@ vma_create(struct drm_i915_gem_object *obj, > if (vma == NULL) > return ERR_PTR(-ENOMEM); > >- kref_init(&vma->ref); > vma->ops = &vm->vma_ops; > vma->obj = obj; > vma->size = obj->base.size; >@@ -1628,15 +1627,6 @@ void i915_vma_reopen(struct i915_vma *vma) > __i915_vma_remove_closed(vma); > } > >-void i915_vma_release(struct kref *ref) >-{ >- struct i915_vma *vma = container_of(ref, typeof(*vma), ref); >- >- i915_active_fini(&vma->active); >- GEM_WARN_ON(vma->resource); >- i915_vma_free(vma); >-} >- > static void force_unbind(struct i915_vma *vma) > { > if (!drm_mm_node_allocated(&vma->node)) >@@ -1665,7 +1655,9 @@ static void release_references(struct i915_vma *vma, bool vm_ddestroy) > if (vm_ddestroy) > i915_vm_resv_put(vma->vm); > >- __i915_vma_put(vma); >+ i915_active_fini(&vma->active); >+ GEM_WARN_ON(vma->resource); >+ i915_vma_free(vma); > } > > /** >diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h >index 67ae7341c7e0..6034991d89fe 100644 >--- a/drivers/gpu/drm/i915/i915_vma.h >+++ b/drivers/gpu/drm/i915/i915_vma.h >@@ -222,20 +222,6 @@ void i915_vma_unlink_ctx(struct i915_vma *vma); > void i915_vma_close(struct i915_vma *vma); > void i915_vma_reopen(struct i915_vma *vma); > >-static inline struct i915_vma *__i915_vma_get(struct i915_vma *vma) >-{ >- if (kref_get_unless_zero(&vma->ref)) >- return vma; >- >- return NULL; >-} >- >-void i915_vma_release(struct kref *ref); >-static inline void __i915_vma_put(struct i915_vma *vma) >-{ >- kref_put(&vma->ref, i915_vma_release); >-} >- > void i915_vma_destroy_locked(struct i915_vma *vma); > void i915_vma_destroy(struct i915_vma *vma); > >diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h >index eac36be184e5..be6e028c3b57 100644 >--- a/drivers/gpu/drm/i915/i915_vma_types.h >+++ b/drivers/gpu/drm/i915/i915_vma_types.h >@@ -211,7 +211,6 @@ struct i915_vma { > * handles (but same file) for execbuf, i.e. the number of aliases > * that exist in the ctx->handle_vmas LUT for this vma. > */ >- struct kref ref; > atomic_t open_count; > atomic_t flags; > /** >-- >2.34.1 >
WARNING: multiple messages have this Message-ID (diff)
From: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> To: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> Cc: intel-gfx@lists.freedesktop.org, matthew.auld@intel.com, dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v2 2/3] drm/i915: Remove the vma refcount Date: Wed, 2 Mar 2022 14:01:41 -0800 [thread overview] Message-ID: <20220302220139.GH25117@nvishwa1-DESK> (raw) In-Reply-To: <20220302102200.158637-3-thomas.hellstrom@linux.intel.com> On Wed, Mar 02, 2022 at 11:21:59AM +0100, Thomas Hellström wrote: >Now that i915_vma_parked() is taking the object lock on vma destruction, >and the only user of the vma refcount, i915_gem_object_unbind() >also takes the object lock, remove the vma refcount. > >Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >--- > drivers/gpu/drm/i915/i915_gem.c | 17 +++++++++++++---- > drivers/gpu/drm/i915/i915_vma.c | 14 +++----------- > drivers/gpu/drm/i915/i915_vma.h | 14 -------------- > drivers/gpu/drm/i915/i915_vma_types.h | 1 - > 4 files changed, 16 insertions(+), 30 deletions(-) > >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >index dd84ebabb50f..c26110abcc0b 100644 >--- a/drivers/gpu/drm/i915/i915_gem.c >+++ b/drivers/gpu/drm/i915/i915_gem.c >@@ -151,14 +151,25 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj, > break; > } > >+ /* >+ * Requiring the vm destructor to take the object lock >+ * before destroying a vma would help us eliminate the >+ * i915_vm_tryget() here, AND thus also the barrier stuff >+ * at the end. That's an easy fix, but sleeping locks in >+ * a kthread should generally be avoided. >+ */ > ret = -EAGAIN; > if (!i915_vm_tryget(vma->vm)) > break; > >- /* Prevent vma being freed by i915_vma_parked as we unbind */ >- vma = __i915_vma_get(vma); > spin_unlock(&obj->vma.lock); > >+ /* >+ * Since i915_vma_parked() takes the object lock >+ * before vma destruction, it won't race us here, >+ * and destroy the vma from under us. >+ */ >+ > if (vma) { > bool vm_trylock = !!(flags & I915_GEM_OBJECT_UNBIND_VM_TRYLOCK); > ret = -EBUSY; >@@ -180,8 +191,6 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj, > ret = i915_vma_unbind(vma); > } > } >- >- __i915_vma_put(vma); > } > > i915_vm_put(vma->vm); One issue still left in i915_gem_object_unbind is that it temporarily removes vmas from the obj->vma.list and adds back later as vma needs to be unbind outside the obj->vma.lock spinlock. This is an issue for other places where we iterate over the obj->vma.list. i915_debugfs_describe_obj is one such case (upcoming vm_bind will be another) that iterates over this list. What is the plan here? Do we need to take object lock while iterating over the list? But this just something I noticed and not related to this patch. This patch looks good to me. Reviewed-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> >diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c >index 91538bc38110..6fd25b39748f 100644 >--- a/drivers/gpu/drm/i915/i915_vma.c >+++ b/drivers/gpu/drm/i915/i915_vma.c >@@ -122,7 +122,6 @@ vma_create(struct drm_i915_gem_object *obj, > if (vma == NULL) > return ERR_PTR(-ENOMEM); > >- kref_init(&vma->ref); > vma->ops = &vm->vma_ops; > vma->obj = obj; > vma->size = obj->base.size; >@@ -1628,15 +1627,6 @@ void i915_vma_reopen(struct i915_vma *vma) > __i915_vma_remove_closed(vma); > } > >-void i915_vma_release(struct kref *ref) >-{ >- struct i915_vma *vma = container_of(ref, typeof(*vma), ref); >- >- i915_active_fini(&vma->active); >- GEM_WARN_ON(vma->resource); >- i915_vma_free(vma); >-} >- > static void force_unbind(struct i915_vma *vma) > { > if (!drm_mm_node_allocated(&vma->node)) >@@ -1665,7 +1655,9 @@ static void release_references(struct i915_vma *vma, bool vm_ddestroy) > if (vm_ddestroy) > i915_vm_resv_put(vma->vm); > >- __i915_vma_put(vma); >+ i915_active_fini(&vma->active); >+ GEM_WARN_ON(vma->resource); >+ i915_vma_free(vma); > } > > /** >diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h >index 67ae7341c7e0..6034991d89fe 100644 >--- a/drivers/gpu/drm/i915/i915_vma.h >+++ b/drivers/gpu/drm/i915/i915_vma.h >@@ -222,20 +222,6 @@ void i915_vma_unlink_ctx(struct i915_vma *vma); > void i915_vma_close(struct i915_vma *vma); > void i915_vma_reopen(struct i915_vma *vma); > >-static inline struct i915_vma *__i915_vma_get(struct i915_vma *vma) >-{ >- if (kref_get_unless_zero(&vma->ref)) >- return vma; >- >- return NULL; >-} >- >-void i915_vma_release(struct kref *ref); >-static inline void __i915_vma_put(struct i915_vma *vma) >-{ >- kref_put(&vma->ref, i915_vma_release); >-} >- > void i915_vma_destroy_locked(struct i915_vma *vma); > void i915_vma_destroy(struct i915_vma *vma); > >diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h >index eac36be184e5..be6e028c3b57 100644 >--- a/drivers/gpu/drm/i915/i915_vma_types.h >+++ b/drivers/gpu/drm/i915/i915_vma_types.h >@@ -211,7 +211,6 @@ struct i915_vma { > * handles (but same file) for execbuf, i.e. the number of aliases > * that exist in the ctx->handle_vmas LUT for this vma. > */ >- struct kref ref; > atomic_t open_count; > atomic_t flags; > /** >-- >2.34.1 >
next prev parent reply other threads:[~2022-03-02 21:59 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-02 10:21 [PATCH v2 0/3] vm- and vma cleanups Thomas Hellström 2022-03-02 10:21 ` [Intel-gfx] " Thomas Hellström 2022-03-02 10:21 ` [PATCH v2 1/3] drm/i915: Remove the vm open count Thomas Hellström 2022-03-02 10:21 ` [Intel-gfx] " Thomas Hellström 2022-03-02 20:33 ` Niranjana Vishwanathapura 2022-03-02 20:33 ` [Intel-gfx] " Niranjana Vishwanathapura 2022-03-03 17:38 ` Matthew Auld 2022-03-02 10:21 ` [PATCH v2 2/3] drm/i915: Remove the vma refcount Thomas Hellström 2022-03-02 10:21 ` [Intel-gfx] " Thomas Hellström 2022-03-02 22:01 ` Niranjana Vishwanathapura [this message] 2022-03-02 22:01 ` Niranjana Vishwanathapura 2022-03-03 6:29 ` Thomas Hellström 2022-03-03 6:29 ` [Intel-gfx] " Thomas Hellström 2022-03-02 10:22 ` [PATCH v2 3/3] drm/i915/gem: Remove some unnecessary code Thomas Hellström 2022-03-02 10:22 ` [Intel-gfx] " Thomas Hellström 2022-03-02 20:29 ` Niranjana Vishwanathapura 2022-03-02 20:29 ` [Intel-gfx] " Niranjana Vishwanathapura 2022-03-02 11:24 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for vm- and vma cleanups Patchwork 2022-03-02 11:25 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork 2022-03-02 11:55 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2022-03-02 19:14 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
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=20220302220139.GH25117@nvishwa1-DESK \ --to=niranjana.vishwanathapura@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=matthew.auld@intel.com \ --cc=thomas.hellstrom@linux.intel.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.