On 26/11/2019 15:26, Chris Wilson wrote:
The current unbind + retry of i915_gem_object_ggtt_pin() allows for
someone else to sneak and claim the vma under a different placement
before the first GGTT bind is complete. Leading to confusion and
complaints all over.
Ideally we would pull the evict + rebind under the same lock, but for
now, simply try again.
Fixes: 2850748ef876 ("drm/i915: Pull i915_vma_pin under the vm->mutex")
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem.c | 39 +++++++++++++++++++--------------
drivers/gpu/drm/i915/i915_vma.c | 6 +++++
2 files changed, 28 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 61395b03443e..b0878d35ed1d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -938,33 +938,38 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
if (IS_ERR(vma))
return vma;
- if (i915_vma_misplaced(vma, size, alignment, flags)) {
- if (flags & PIN_NONBLOCK) {
- if (i915_vma_is_pinned(vma) || i915_vma_is_active(vma))
- return ERR_PTR(-ENOSPC);
-
- if (flags & PIN_MAPPABLE &&
- vma->fence_size > ggtt->mappable_end / 2)
- return ERR_PTR(-ENOSPC);
+ do {
+ if (i915_vma_misplaced(vma, size, alignment, flags)) {
+ if (flags & PIN_NONBLOCK) {
+ if (i915_vma_is_pinned(vma) ||
+ i915_vma_is_active(vma))
+ return ERR_PTR(-ENOSPC);
+
+ if (flags & PIN_MAPPABLE &&
+ vma->fence_size > ggtt->mappable_end / 2)
+ return ERR_PTR(-ENOSPC);
+ }
+
+ ret = i915_vma_unbind(vma);
+ if (ret)
+ return ERR_PTR(ret);
}
- ret = i915_vma_unbind(vma);
- if (ret)
- return ERR_PTR(ret);
- }
+ ret = i915_vma_pin(vma, size, alignment, flags | PIN_GLOBAL);
+ } while (ret == -EAGAIN); /* retry if we lost our placement */
+ if (ret)
+ return ERR_PTR(ret);
if (vma->fence && !i915_gem_object_is_tiled(obj)) {
mutex_lock(&ggtt->vm.mutex);
ret = i915_vma_revoke_fence(vma);
mutex_unlock(&ggtt->vm.mutex);
- if (ret)
+ if (ret) {
+ i915_vma_unpin(vma);
return ERR_PTR(ret);
+ }
}
- ret = i915_vma_pin(vma, size, alignment, flags | PIN_GLOBAL);
- if (ret)
- return ERR_PTR(ret);
-
return vma;
}
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index e5512f26e20a..f6e661428b02 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -905,6 +905,12 @@ int i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
__i915_vma_set_map_and_fenceable(vma);
}
+ /* Somebody else managed to gazump our placement? */
+ if (i915_vma_misplaced(vma, size, alignment, flags)) {
+ err = -EAGAIN;
+ goto err_active;
+ }
+
What about other callers? They will not be expecting this.
GEM_BUG_ON(!vma->pages);
err = i915_vma_bind(vma,
vma->obj ? vma->obj->cache_level : 0,
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
