From MAILER-DAEMON Thu Mar 12 13:30:18 2020 From: Chris Wilson To: stable@vger.kernel.org Cc: Chris Wilson , Tvrtko Ursulin , Jani Nikula Subject: [PATCH 1/5] drm/i915: Check activity on i915_vma after confirming pin_count==0 Date: Tue, 10 Mar 2020 20:40:42 +0000 Message-Id: <20200310204046.3995087-1-chris@chris-wilson.co.uk> Sender: stable-owner@vger.kernel.org List-ID: X-Mailing-List: stable@vger.kernel.org MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit 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 Cc: Tvrtko Ursulin Reviewed-by: Tvrtko Ursulin Link: https://patchwork.freedesktop.org/patch/msgid/20200123224459.38128-1-chris@chris-wilson.co.uk (cherry picked from commit 60e94557fff1f5514c7fc4da7ddc2c7a13ffff26) Signed-off-by: Jani Nikula (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 -- 2.25.1 From MAILER-DAEMON Thu Mar 12 13:30:18 2020 From: Chris Wilson To: stable@vger.kernel.org Cc: Chris Wilson , Matthew Auld Subject: [PATCH 2/5] drm/i915/gem: Avoid parking the vma as we unbind Date: Tue, 10 Mar 2020 20:40:43 +0000 Message-Id: <20200310204046.3995087-2-chris@chris-wilson.co.uk> In-Reply-To: <20200310204046.3995087-1-chris@chris-wilson.co.uk> References: <20200310204046.3995087-1-chris@chris-wilson.co.uk> Sender: stable-owner@vger.kernel.org List-ID: X-Mailing-List: stable@vger.kernel.org MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit 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 Reviewed-by: Matthew Auld Link: https://patchwork.freedesktop.org/patch/msgid/20191203155032.3137263-1-chris@chris-wilson.co.uk (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); } -- 2.25.1 From MAILER-DAEMON Thu Mar 12 13:30:18 2020 From: Chris Wilson To: stable@vger.kernel.org Cc: Chris Wilson , Imre Deak Subject: [PATCH 3/5] drm/i915: Add a simple is-bound check before unbinding Date: Tue, 10 Mar 2020 20:40:44 +0000 Message-Id: <20200310204046.3995087-3-chris@chris-wilson.co.uk> In-Reply-To: <20200310204046.3995087-1-chris@chris-wilson.co.uk> References: <20200310204046.3995087-1-chris@chris-wilson.co.uk> Sender: stable-owner@vger.kernel.org List-ID: X-Mailing-List: stable@vger.kernel.org MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Only acquire the various atomic references required to unbind the vma if we do need to unbind the vma. Signed-off-by: Chris Wilson Acked-by: Imre Deak Link: https://patchwork.freedesktop.org/patch/msgid/20191222210256.2066451-1-chris@chris-wilson.co.uk (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; -- 2.25.1 From MAILER-DAEMON Thu Mar 12 13:30:18 2020 From: Chris Wilson To: stable@vger.kernel.org Cc: Chris Wilson , Imre Deak Subject: [PATCH 4/5] drm/i915: Introduce a vma.kref Date: Tue, 10 Mar 2020 20:40:45 +0000 Message-Id: <20200310204046.3995087-4-chris@chris-wilson.co.uk> In-Reply-To: <20200310204046.3995087-1-chris@chris-wilson.co.uk> References: <20200310204046.3995087-1-chris@chris-wilson.co.uk> Sender: stable-owner@vger.kernel.org List-ID: X-Mailing-List: stable@vger.kernel.org MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit 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 Cc: Imre Deak Acked-by: Imre Deak Link: https://patchwork.freedesktop.org/patch/msgid/20191222210256.2066451-2-chris@chris-wilson.co.uk (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) -- 2.25.1 From MAILER-DAEMON Thu Mar 12 13:30:18 2020 From: Chris Wilson To: stable@vger.kernel.org Cc: Chris Wilson , Kenneth Graunke , Tvrtko Ursulin , Matthew Auld Subject: [PATCH 5/5] drm/i915: Serialise i915_active_acquire() with __active_retire() Date: Tue, 10 Mar 2020 20:40:46 +0000 Message-Id: <20200310204046.3995087-5-chris@chris-wilson.co.uk> In-Reply-To: <20200310204046.3995087-1-chris@chris-wilson.co.uk> References: <20200310204046.3995087-1-chris@chris-wilson.co.uk> Sender: stable-owner@vger.kernel.org List-ID: X-Mailing-List: stable@vger.kernel.org MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit 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 Fixes: c9ad602feabe ("drm/i915: Split i915_active.mutex into an irq-safe spinlock for the rbtree") Signed-off-by: Chris Wilson Cc: Tvrtko Ursulin Cc: Kenneth Graunke Cc: Matthew Auld Tested-by: Kenneth Graunke Reviewed-by: Kenneth Graunke Link: https://patchwork.freedesktop.org/patch/msgid/20191205183332.801237-1-chris@chris-wilson.co.uk (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); -- 2.25.1