| Message ID | 20200805122231.23313-15-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Replace obj->mm.lock with reservation_ww_class | expand |
On 05/08/2020 13:22, Chris Wilson wrote: > Directly seralise the atomic pinning with evicting the vma from unbind > with a pair of coupled cmpxchg to avoid fighting over vm->mutex. Assumption being bind/unbind should never contend and create a busy-spinny section? And motivation being.. ? > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_vma.c | 45 ++++++++++----------------------- > 1 file changed, 14 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > index dbe11b349175..17ce0bce318e 100644 > --- a/drivers/gpu/drm/i915/i915_vma.c > +++ b/drivers/gpu/drm/i915/i915_vma.c > @@ -742,12 +742,10 @@ i915_vma_detach(struct i915_vma *vma) > > bool i915_vma_pin_inplace(struct i915_vma *vma, unsigned int flags) > { > - unsigned int bound; > - bool pinned = true; > + unsigned int bound = atomic_read(&vma->flags); > > GEM_BUG_ON(flags & ~I915_VMA_BIND_MASK); > > - bound = atomic_read(&vma->flags); > do { > if (unlikely(flags & ~bound)) > return false; > @@ -755,34 +753,10 @@ bool i915_vma_pin_inplace(struct i915_vma *vma, unsigned int flags) > if (unlikely(bound & (I915_VMA_OVERFLOW | I915_VMA_ERROR))) > return false; > > - if (!(bound & I915_VMA_PIN_MASK)) > - goto unpinned; > - > GEM_BUG_ON(((bound + 1) & I915_VMA_PIN_MASK) == 0); > } while (!atomic_try_cmpxchg(&vma->flags, &bound, bound + 1)); > > return true; > - > -unpinned: > - /* > - * If pin_count==0, but we are bound, check under the lock to avoid > - * racing with a concurrent i915_vma_unbind(). > - */ > - mutex_lock(&vma->vm->mutex); > - do { > - if (unlikely(bound & (I915_VMA_OVERFLOW | I915_VMA_ERROR))) { > - pinned = false; > - break; > - } > - > - if (unlikely(flags & ~bound)) { > - pinned = false; > - break; > - } > - } while (!atomic_try_cmpxchg(&vma->flags, &bound, bound + 1)); > - mutex_unlock(&vma->vm->mutex); > - > - return pinned; > } > > static int vma_get_pages(struct i915_vma *vma) > @@ -1292,6 +1266,7 @@ void __i915_vma_evict(struct i915_vma *vma) > > int __i915_vma_unbind(struct i915_vma *vma) > { > + unsigned int bound; > int ret; > > lockdep_assert_held(&vma->vm->mutex); > @@ -1299,10 +1274,18 @@ int __i915_vma_unbind(struct i915_vma *vma) > if (!drm_mm_node_allocated(&vma->node)) > return 0; > > - if (i915_vma_is_pinned(vma)) { > - vma_print_allocator(vma, "is pinned"); > - return -EAGAIN; > - } > + /* Serialise with i915_vma_pin_inplace() */ > + bound = atomic_read(&vma->flags); > + do { > + if (unlikely(bound & I915_VMA_PIN_MASK)) { > + vma_print_allocator(vma, "is pinned"); > + return -EAGAIN; > + } > + > + if (unlikely(bound & I915_VMA_ERROR)) > + break; > + } while (!atomic_try_cmpxchg(&vma->flags, > + &bound, bound | I915_VMA_ERROR)); Using the error flag is somehow critical for this scheme to work? Can you please explain in the comment and/or commit message? > > /* > * After confirming that no one else is pinning this vma, wait for > Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index dbe11b349175..17ce0bce318e 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -742,12 +742,10 @@ i915_vma_detach(struct i915_vma *vma) bool i915_vma_pin_inplace(struct i915_vma *vma, unsigned int flags) { - unsigned int bound; - bool pinned = true; + unsigned int bound = atomic_read(&vma->flags); GEM_BUG_ON(flags & ~I915_VMA_BIND_MASK); - bound = atomic_read(&vma->flags); do { if (unlikely(flags & ~bound)) return false; @@ -755,34 +753,10 @@ bool i915_vma_pin_inplace(struct i915_vma *vma, unsigned int flags) if (unlikely(bound & (I915_VMA_OVERFLOW | I915_VMA_ERROR))) return false; - if (!(bound & I915_VMA_PIN_MASK)) - goto unpinned; - GEM_BUG_ON(((bound + 1) & I915_VMA_PIN_MASK) == 0); } while (!atomic_try_cmpxchg(&vma->flags, &bound, bound + 1)); return true; - -unpinned: - /* - * If pin_count==0, but we are bound, check under the lock to avoid - * racing with a concurrent i915_vma_unbind(). - */ - mutex_lock(&vma->vm->mutex); - do { - if (unlikely(bound & (I915_VMA_OVERFLOW | I915_VMA_ERROR))) { - pinned = false; - break; - } - - if (unlikely(flags & ~bound)) { - pinned = false; - break; - } - } while (!atomic_try_cmpxchg(&vma->flags, &bound, bound + 1)); - mutex_unlock(&vma->vm->mutex); - - return pinned; } static int vma_get_pages(struct i915_vma *vma) @@ -1292,6 +1266,7 @@ void __i915_vma_evict(struct i915_vma *vma) int __i915_vma_unbind(struct i915_vma *vma) { + unsigned int bound; int ret; lockdep_assert_held(&vma->vm->mutex); @@ -1299,10 +1274,18 @@ int __i915_vma_unbind(struct i915_vma *vma) if (!drm_mm_node_allocated(&vma->node)) return 0; - if (i915_vma_is_pinned(vma)) { - vma_print_allocator(vma, "is pinned"); - return -EAGAIN; - } + /* Serialise with i915_vma_pin_inplace() */ + bound = atomic_read(&vma->flags); + do { + if (unlikely(bound & I915_VMA_PIN_MASK)) { + vma_print_allocator(vma, "is pinned"); + return -EAGAIN; + } + + if (unlikely(bound & I915_VMA_ERROR)) + break; + } while (!atomic_try_cmpxchg(&vma->flags, + &bound, bound | I915_VMA_ERROR)); /* * After confirming that no one else is pinning this vma, wait for
Directly seralise the atomic pinning with evicting the vma from unbind with a pair of coupled cmpxchg to avoid fighting over vm->mutex. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_vma.c | 45 ++++++++++----------------------- 1 file changed, 14 insertions(+), 31 deletions(-)