Only assert that the i915_vma is now idle if and only if no other pins are present. If another user has the i915_vma pinned, they may submit more work to the i915_vma skipping the vm->mutex used to serialise the unbind. We need to wait again, if we want to continue and unbind this vma.
However, if we own the i915_vma (we hold the vm->mutex for the unbind and the pin_count is 0), we can assert that the vma remains idle as we unbind.
Fixes: 2850748ef876 ("drm/i915: Pull i915_vma_pin under the vm->mutex") Closes: https://gitlab.freedesktop.org/drm/intel/issues/530 Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20200123224459.38128-1-chris@c... (cherry picked from commit 60e94557fff1f5514c7fc4da7ddc2c7a13ffff26) Signed-off-by: Jani Nikula jani.nikula@intel.com (cherry picked from commit e4edd4fcbf4daf9d4319bef0bfaf350cb672239a) --- drivers/gpu/drm/i915/i915_vma.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 01c822256b39..7c7e152cc5ff 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -1149,16 +1149,26 @@ int __i915_vma_unbind(struct i915_vma *vma) if (ret) return ret;
- GEM_BUG_ON(i915_vma_is_active(vma)); if (i915_vma_is_pinned(vma)) { vma_print_allocator(vma, "is pinned"); return -EBUSY; }
- GEM_BUG_ON(i915_vma_is_active(vma)); + /* + * After confirming that no one else is pinning this vma, wait for + * any laggards who may have crept in during the wait (through + * a residual pin skipping the vm->mutex) to complete. + */ + ret = i915_vma_sync(vma); + if (ret) + return ret; + if (!drm_mm_node_allocated(&vma->node)) return 0;
+ GEM_BUG_ON(i915_vma_is_pinned(vma)); + GEM_BUG_ON(i915_vma_is_active(vma)); + if (i915_vma_is_map_and_fenceable(vma)) { /* * Check that we have flushed all writes through the GGTT
In order to avoid keeping a reference on the i915_vma (which is long overdue!) we have to coordinate all the possible lifetimes and only use the vma while we know it is alive. In this episode, we are reminded that while idle, the closed vma are destroyed. So if the GT idles while we are working with the vma, the vma itself becomes invalid.
First class i915_vma here we come, but in the meantime keep piling on the straw.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Reviewed-by: Matthew Auld matthew.auld@intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20191203155032.3137263-1-chris... (cherry picked from commit cb6c3d45f948f8f184687a23fea30017d01e892f) --- drivers/gpu/drm/i915/i915_gem.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 3f07948ea4da..f7c52b437f6a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -128,18 +128,33 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj, struct i915_vma, obj_link))) { struct i915_address_space *vm = vma->vm; + bool awake = false;
- ret = -EBUSY; + ret = -EAGAIN; if (!i915_vm_tryopen(vm)) break;
+ /* Prevent vma being freed by i915_vma_parked as we unbind */ + if (intel_gt_pm_get_if_awake(vm->gt)) { + awake = true; + } else { + if (i915_vma_is_closed(vma)) { + spin_unlock(&obj->vma.lock); + goto err_vm; + } + } + list_move_tail(&vma->obj_link, &still_in_list); spin_unlock(&obj->vma.lock);
+ ret = -EBUSY; if (flags & I915_GEM_OBJECT_UNBIND_ACTIVE || !i915_vma_is_active(vma)) ret = i915_vma_unbind(vma);
+ if (awake) + intel_gt_pm_put(vm->gt); +err_vm: i915_vm_close(vm); spin_lock(&obj->vma.lock); }
Only acquire the various atomic references required to unbind the vma if we do need to unbind the vma.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Acked-by: Imre Deak imre.deak@intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20191222210256.2066451-1-chris... (cherry picked from commit f5af1659d809e264d619e5f483fd8f47bced3b6a) --- drivers/gpu/drm/i915/i915_gem.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f7c52b437f6a..998b67e3466e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -130,6 +130,10 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj, struct i915_address_space *vm = vma->vm; bool awake = false;
+ list_move_tail(&vma->obj_link, &still_in_list); + if (!i915_vma_is_bound(vma, I915_VMA_BIND_MASK)) + continue; + ret = -EAGAIN; if (!i915_vm_tryopen(vm)) break; @@ -144,7 +148,6 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj, } }
- list_move_tail(&vma->obj_link, &still_in_list); spin_unlock(&obj->vma.lock);
ret = -EBUSY;
Start introducing a kref on i915_vma in order to protect the vma unbind (i915_gem_object_unbind) from a parallel destruction (i915_vma_parked). Later, we will use the refcount to manage all access and turn i915_vma into a first class container.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Imre Deak imre.deak@intel.com Acked-by: Imre Deak imre.deak@intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20191222210256.2066451-2-chris... (cherry picked from commit 76f9764cc3d538435262dea885bf69fac2415402) --- drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 +- .../gpu/drm/i915/gem/selftests/huge_pages.c | 3 +-- .../drm/i915/gem/selftests/i915_gem_mman.c | 4 +-- drivers/gpu/drm/i915/i915_gem.c | 26 +++++++------------ drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++-- drivers/gpu/drm/i915/i915_vma.c | 9 ++++--- drivers/gpu/drm/i915/i915_vma.h | 25 +++++++++++++++--- 7 files changed, 44 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index a596548c07bf..b6937469ffd3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -174,7 +174,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915, GEM_BUG_ON(vma->obj != obj); spin_unlock(&obj->vma.lock);
- i915_vma_destroy(vma); + __i915_vma_put(vma);
spin_lock(&obj->vma.lock); } diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c index 688c49a24f32..bd1e2c12de63 100644 --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c @@ -1110,8 +1110,7 @@ static int __igt_write_huge(struct intel_context *ce, out_vma_unpin: i915_vma_unpin(vma); out_vma_close: - i915_vma_destroy(vma); - + __i915_vma_put(vma); return err; }
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c index 29b2077b73d2..d226e55df8b2 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c @@ -161,7 +161,7 @@ static int check_partial_mapping(struct drm_i915_gem_object *obj, kunmap(p);
out: - i915_vma_destroy(vma); + __i915_vma_put(vma); return err; }
@@ -255,7 +255,7 @@ static int check_partial_mappings(struct drm_i915_gem_object *obj, if (err) return err;
- i915_vma_destroy(vma); + __i915_vma_put(vma);
if (igt_timeout(end_time, "%s: timed out after tiling=%d stride=%d\n", diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 998b67e3466e..67a8e7408e67 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -128,7 +128,6 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj, struct i915_vma, obj_link))) { struct i915_address_space *vm = vma->vm; - bool awake = false;
list_move_tail(&vma->obj_link, &still_in_list); if (!i915_vma_is_bound(vma, I915_VMA_BIND_MASK)) @@ -139,25 +138,18 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj, break;
/* Prevent vma being freed by i915_vma_parked as we unbind */ - if (intel_gt_pm_get_if_awake(vm->gt)) { - awake = true; - } else { - if (i915_vma_is_closed(vma)) { - spin_unlock(&obj->vma.lock); - goto err_vm; - } - } - + vma = __i915_vma_get(vma); spin_unlock(&obj->vma.lock);
- ret = -EBUSY; - if (flags & I915_GEM_OBJECT_UNBIND_ACTIVE || - !i915_vma_is_active(vma)) - ret = i915_vma_unbind(vma); + if (vma) { + ret = -EBUSY; + if (flags & I915_GEM_OBJECT_UNBIND_ACTIVE || + !i915_vma_is_active(vma)) + ret = i915_vma_unbind(vma); + + __i915_vma_put(vma); + }
- if (awake) - intel_gt_pm_put(vm->gt); -err_vm: i915_vm_close(vm); spin_lock(&obj->vma.lock); } diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 44727806dfd7..dd2c20f7d4d2 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -522,7 +522,7 @@ void __i915_vm_close(struct i915_address_space *vm)
atomic_and(~I915_VMA_PIN_MASK, &vma->flags); WARN_ON(__i915_vma_unbind(vma)); - i915_vma_destroy(vma); + __i915_vma_put(vma);
i915_gem_object_put(obj); } @@ -1790,7 +1790,7 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm) { struct gen6_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
- i915_vma_destroy(ppgtt->vma); + __i915_vma_put(ppgtt->vma);
gen6_ppgtt_free_pd(ppgtt); free_scratch(vm); @@ -1878,6 +1878,7 @@ static struct i915_vma *pd_vma_create(struct gen6_ppgtt *ppgtt, int size)
i915_active_init(&vma->active, NULL, NULL);
+ kref_init(&vma->ref); mutex_init(&vma->pages_mutex); vma->vm = i915_vm_get(&ggtt->vm); vma->ops = &pd_vma_ops; diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 7c7e152cc5ff..5309872442bc 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -112,6 +112,7 @@ vma_create(struct drm_i915_gem_object *obj, if (vma == NULL) return ERR_PTR(-ENOMEM);
+ kref_init(&vma->ref); mutex_init(&vma->pages_mutex); vma->vm = i915_vm_get(vm); vma->ops = &vm->vma_ops; @@ -978,8 +979,10 @@ void i915_vma_reopen(struct i915_vma *vma) __i915_vma_remove_closed(vma); }
-void i915_vma_destroy(struct i915_vma *vma) +void i915_vma_release(struct kref *ref) { + struct i915_vma *vma = container_of(ref, typeof(*vma), ref); + if (drm_mm_node_allocated(&vma->node)) { mutex_lock(&vma->vm->mutex); atomic_and(~I915_VMA_PIN_MASK, &vma->flags); @@ -1027,7 +1030,7 @@ void i915_vma_parked(struct intel_gt *gt) spin_unlock_irq(>->closed_lock);
if (obj) { - i915_vma_destroy(vma); + __i915_vma_put(vma); i915_gem_object_put(obj); }
@@ -1202,7 +1205,7 @@ int __i915_vma_unbind(struct i915_vma *vma) i915_vma_detach(vma); vma_unbind_pages(vma);
- drm_mm_remove_node(&vma->node); /* pairs with i915_vma_destroy() */ + drm_mm_remove_node(&vma->node); /* pairs with i915_vma_release() */ return 0; }
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h index 465932813bc5..ce1db908ad69 100644 --- a/drivers/gpu/drm/i915/i915_vma.h +++ b/drivers/gpu/drm/i915/i915_vma.h @@ -51,14 +51,19 @@ enum i915_cache_level; */ struct i915_vma { struct drm_mm_node node; - struct drm_i915_gem_object *obj; + struct i915_address_space *vm; const struct i915_vma_ops *ops; - struct i915_fence_reg *fence; + + struct drm_i915_gem_object *obj; struct dma_resv *resv; /** Alias of obj->resv */ + struct sg_table *pages; void __iomem *iomap; void *private; /* owned by creator */ + + struct i915_fence_reg *fence; + u64 size; u64 display_alignment; struct i915_page_sizes page_sizes; @@ -71,6 +76,7 @@ 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; /** @@ -333,7 +339,20 @@ int __must_check i915_vma_unbind(struct i915_vma *vma); 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); -void i915_vma_destroy(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); +}
#define assert_vma_held(vma) dma_resv_assert_held((vma)->resv)
As __active_retire() does it's final atomic_dec() under the ref->tree_lock spinlock, in order to prevent ourselves from reusing the ref->cache and ref->tree as they are being destroyed, we need to serialise with the retirement during i915_active_acquire().
[ +0.000005] kernel BUG at drivers/gpu/drm/i915/i915_active.c:157! [ +0.000011] invalid opcode: 0000 [#1] SMP [ +0.000004] CPU: 7 PID: 188 Comm: kworker/u16:4 Not tainted 5.4.0-rc8-03070-gac5e57322614 #89 [ +0.000002] Hardware name: Razer Razer Blade Stealth 13 Late 2019/LY320, BIOS 1.02 09/10/2019 [ +0.000082] Workqueue: events_unbound active_work [i915] [ +0.000059] RIP: 0010:__active_retire+0x115/0x120 [i915] [ +0.000003] Code: 75 28 48 8b 3d 8c 6e 1a 00 48 89 ee e8 e4 5f a5 c0 48 8b 44 24 10 65 48 33 04 25 28 00 00 00 75 0f 48 83 c4 18 5b 5d 41 5c c3 <0f> 0b 0f 0b 0f 0b e8 a0 90 87 c0 0f 1f 44 00 00 48 8b 3d 54 6e 1a [ +0.000002] RSP: 0018:ffffb833003f7e48 EFLAGS: 00010286 [ +0.000003] RAX: ffff8d6e8d726d00 RBX: ffff8d6f9db4e840 RCX: 0000000000000000 [ +0.000001] RDX: ffffffff82605930 RSI: ffff8d6f9adc4908 RDI: ffff8d6e96cefe28 [ +0.000002] RBP: ffff8d6e96cefe00 R08: 0000000000000000 R09: ffff8d6f9ffe9a50 [ +0.000002] R10: 0000000000000048 R11: 0000000000000018 R12: ffff8d6f9adc4930 [ +0.000001] R13: ffff8d6f9e04fb00 R14: 0000000000000000 R15: ffff8d6f9adc4988 [ +0.000002] FS: 0000000000000000(0000) GS:ffff8d6f9ffc0000(0000) knlGS:0000000000000000 [ +0.000002] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ +0.000002] CR2: 000055eb5a34cf10 CR3: 000000018d609002 CR4: 0000000000760ee0 [ +0.000002] PKRU: 55555554 [ +0.000001] Call Trace: [ +0.000010] process_one_work+0x1aa/0x350 [ +0.000004] worker_thread+0x4d/0x3a0 [ +0.000004] kthread+0xfb/0x130 [ +0.000004] ? process_one_work+0x350/0x350 [ +0.000003] ? kthread_park+0x90/0x90 [ +0.000005] ret_from_fork+0x1f/0x40
Reported-by: Kenneth Graunke kenneth@whitecape.org Fixes: c9ad602feabe ("drm/i915: Split i915_active.mutex into an irq-safe spinlock for the rbtree") Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com Cc: Kenneth Graunke kenneth@whitecape.org Cc: Matthew Auld matthew.auld@intel.com Tested-by: Kenneth Graunke kenneth@whitecape.org Reviewed-by: Kenneth Graunke kenneth@whitecape.org Link: https://patchwork.freedesktop.org/patch/msgid/20191205183332.801237-1-chris@... (cherry picked from commit bbca083de291a03ffe1a1eb0832a0d74f8b64898) --- drivers/gpu/drm/i915/i915_active.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c index a19e7d89bc8a..378b52d1ab74 100644 --- a/drivers/gpu/drm/i915/i915_active.c +++ b/drivers/gpu/drm/i915/i915_active.c @@ -91,10 +91,9 @@ static void debug_active_init(struct i915_active *ref)
static void debug_active_activate(struct i915_active *ref) { - spin_lock_irq(&ref->tree_lock); + lockdep_assert_held(&ref->tree_lock); if (!atomic_read(&ref->count)) /* before the first inc */ debug_object_activate(ref, &active_debug_desc); - spin_unlock_irq(&ref->tree_lock); }
static void debug_active_deactivate(struct i915_active *ref) @@ -407,8 +406,10 @@ int i915_active_acquire(struct i915_active *ref) if (!atomic_read(&ref->count) && ref->active) err = ref->active(ref); if (!err) { + spin_lock_irq(&ref->tree_lock); /* vs __active_retire() */ debug_active_activate(ref); atomic_inc(&ref->count); + spin_unlock_irq(&ref->tree_lock); }
mutex_unlock(&ref->mutex);
linux-stable-mirror@lists.linaro.org
-
Chris Wilson