| Message ID | 20211129134735.628712-13-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Remove short term pins from execbuf. | expand |
On Mon, 29 Nov 2021 at 13:58, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > > We want to remove more members of i915_vma, which requires the locking to be > held more often. > > Start requiring gem object lock for i915_vma_unbind, as it's one of the > callers that may unpin pages. > > Some special care is needed when evicting, because the last reference to the > object may be held by the VMA, so after __i915_vma_unbind, vma may be garbage, > and we need to cache vma->obj before unlocking. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- <snip> > @@ -129,22 +129,47 @@ void i915_ggtt_suspend_vm(struct i915_address_space *vm) > > drm_WARN_ON(&vm->i915->drm, !vm->is_ggtt && !vm->is_dpt); > > +retry: > + i915_gem_drain_freed_objects(vm->i915); > + > mutex_lock(&vm->mutex); > > /* Skip rewriting PTE on VMA unbind. */ > open = atomic_xchg(&vm->open, 0); > > list_for_each_entry_safe(vma, vn, &vm->bound_list, vm_link) { > + struct drm_i915_gem_object *obj = vma->obj; > + > GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); > + > i915_vma_wait_for_bind(vma); > > - if (i915_vma_is_pinned(vma)) > + if (i915_vma_is_pinned(vma) || !i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) > continue; > > - if (!i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) { > - __i915_vma_evict(vma); > - drm_mm_remove_node(&vma->node); > + /* unlikely to race when GPU is idle, so no worry about slowpath.. */ > + if (!i915_gem_object_trylock(obj, NULL)) { > + atomic_set(&vm->open, open); Does this need a comment about barriers? > + > + i915_gem_object_get(obj); Should this not be kref_get_unless_zero? Assuming the vm->mutex is the only thing keeping the object alive here, won't this lead to potential uaf/double-free or something? Also should we not plonk this before the trylock? Or maybe I'm missing something here? > + mutex_unlock(&vm->mutex); > + > + i915_gem_object_lock(obj, NULL); > + open = i915_vma_unbind(vma); > + i915_gem_object_unlock(obj); > + > + GEM_WARN_ON(open); > + > + i915_gem_object_put(obj); > + goto retry; > } > + > + i915_vma_wait_for_bind(vma); We also call wait_for_bind above, is that intentional?
On 09-12-2021 14:05, Matthew Auld wrote: > On Mon, 29 Nov 2021 at 13:58, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: >> We want to remove more members of i915_vma, which requires the locking to be >> held more often. >> >> Start requiring gem object lock for i915_vma_unbind, as it's one of the >> callers that may unpin pages. >> >> Some special care is needed when evicting, because the last reference to the >> object may be held by the VMA, so after __i915_vma_unbind, vma may be garbage, >> and we need to cache vma->obj before unlocking. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- > <snip> > >> @@ -129,22 +129,47 @@ void i915_ggtt_suspend_vm(struct i915_address_space *vm) >> >> drm_WARN_ON(&vm->i915->drm, !vm->is_ggtt && !vm->is_dpt); >> >> +retry: >> + i915_gem_drain_freed_objects(vm->i915); >> + >> mutex_lock(&vm->mutex); >> >> /* Skip rewriting PTE on VMA unbind. */ >> open = atomic_xchg(&vm->open, 0); >> >> list_for_each_entry_safe(vma, vn, &vm->bound_list, vm_link) { >> + struct drm_i915_gem_object *obj = vma->obj; >> + >> GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); >> + >> i915_vma_wait_for_bind(vma); >> >> - if (i915_vma_is_pinned(vma)) >> + if (i915_vma_is_pinned(vma) || !i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) >> continue; >> >> - if (!i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) { >> - __i915_vma_evict(vma); >> - drm_mm_remove_node(&vma->node); >> + /* unlikely to race when GPU is idle, so no worry about slowpath.. */ >> + if (!i915_gem_object_trylock(obj, NULL)) { >> + atomic_set(&vm->open, open); > Does this need a comment about barriers? Not sure, it's guarded by vm->mutex. >> + >> + i915_gem_object_get(obj); > Should this not be kref_get_unless_zero? Assuming the vm->mutex is the > only thing keeping the object alive here, won't this lead to potential > uaf/double-free or something? Also should we not plonk this before the > trylock? Or maybe I'm missing something here? Normally you're correct, this is normally the case, but we drain freed objects and this path should only be run during s/r, at which point userspace should be dead, GPU idle, and we just drained all freed objects above. It would be a bug if we still found a dead object, as nothing should be running. >> + mutex_unlock(&vm->mutex); >> + >> + i915_gem_object_lock(obj, NULL); >> + open = i915_vma_unbind(vma); >> + i915_gem_object_unlock(obj); >> + >> + GEM_WARN_ON(open); >> + >> + i915_gem_object_put(obj); >> + goto retry; >> } >> + >> + i915_vma_wait_for_bind(vma); > We also call wait_for_bind above, is that intentional? Should be harmless, but first one should probably be removed. :)
On Thu, 9 Dec 2021 at 13:25, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > > On 09-12-2021 14:05, Matthew Auld wrote: > > On Mon, 29 Nov 2021 at 13:58, Maarten Lankhorst > > <maarten.lankhorst@linux.intel.com> wrote: > >> We want to remove more members of i915_vma, which requires the locking to be > >> held more often. > >> > >> Start requiring gem object lock for i915_vma_unbind, as it's one of the > >> callers that may unpin pages. > >> > >> Some special care is needed when evicting, because the last reference to the > >> object may be held by the VMA, so after __i915_vma_unbind, vma may be garbage, > >> and we need to cache vma->obj before unlocking. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > > <snip> > > > >> @@ -129,22 +129,47 @@ void i915_ggtt_suspend_vm(struct i915_address_space *vm) > >> > >> drm_WARN_ON(&vm->i915->drm, !vm->is_ggtt && !vm->is_dpt); > >> > >> +retry: > >> + i915_gem_drain_freed_objects(vm->i915); > >> + > >> mutex_lock(&vm->mutex); > >> > >> /* Skip rewriting PTE on VMA unbind. */ > >> open = atomic_xchg(&vm->open, 0); > >> > >> list_for_each_entry_safe(vma, vn, &vm->bound_list, vm_link) { > >> + struct drm_i915_gem_object *obj = vma->obj; > >> + > >> GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); > >> + > >> i915_vma_wait_for_bind(vma); > >> > >> - if (i915_vma_is_pinned(vma)) > >> + if (i915_vma_is_pinned(vma) || !i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) > >> continue; > >> > >> - if (!i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) { > >> - __i915_vma_evict(vma); > >> - drm_mm_remove_node(&vma->node); > >> + /* unlikely to race when GPU is idle, so no worry about slowpath.. */ > >> + if (!i915_gem_object_trylock(obj, NULL)) { > >> + atomic_set(&vm->open, open); > > Does this need a comment about barriers? > Not sure, it's guarded by vm->mutex. > >> + > >> + i915_gem_object_get(obj); > > Should this not be kref_get_unless_zero? Assuming the vm->mutex is the > > only thing keeping the object alive here, won't this lead to potential > > uaf/double-free or something? Also should we not plonk this before the > > trylock? Or maybe I'm missing something here? > > Normally you're correct, this is normally the case, but we drain freed objects and this path should only be run during s/r, at which point userspace should be dead, GPU idle, and we just drained all freed objects above. > > It would be a bug if we still found a dead object, as nothing should be running. Hmm, Ok. So why do we expect the trylock to ever fail here? Who else can grab it at this stage? > > >> + mutex_unlock(&vm->mutex); > >> + > >> + i915_gem_object_lock(obj, NULL); > >> + open = i915_vma_unbind(vma); > >> + i915_gem_object_unlock(obj); > >> + > >> + GEM_WARN_ON(open); > >> + > >> + i915_gem_object_put(obj); > >> + goto retry; > >> } > >> + > >> + i915_vma_wait_for_bind(vma); > > We also call wait_for_bind above, is that intentional? > > Should be harmless, but first one should probably be removed. :) >
On 09-12-2021 14:40, Matthew Auld wrote: > On Thu, 9 Dec 2021 at 13:25, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: >> On 09-12-2021 14:05, Matthew Auld wrote: >>> On Mon, 29 Nov 2021 at 13:58, Maarten Lankhorst >>> <maarten.lankhorst@linux.intel.com> wrote: >>>> We want to remove more members of i915_vma, which requires the locking to be >>>> held more often. >>>> >>>> Start requiring gem object lock for i915_vma_unbind, as it's one of the >>>> callers that may unpin pages. >>>> >>>> Some special care is needed when evicting, because the last reference to the >>>> object may be held by the VMA, so after __i915_vma_unbind, vma may be garbage, >>>> and we need to cache vma->obj before unlocking. >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>> <snip> >>> >>>> @@ -129,22 +129,47 @@ void i915_ggtt_suspend_vm(struct i915_address_space *vm) >>>> >>>> drm_WARN_ON(&vm->i915->drm, !vm->is_ggtt && !vm->is_dpt); >>>> >>>> +retry: >>>> + i915_gem_drain_freed_objects(vm->i915); >>>> + >>>> mutex_lock(&vm->mutex); >>>> >>>> /* Skip rewriting PTE on VMA unbind. */ >>>> open = atomic_xchg(&vm->open, 0); >>>> >>>> list_for_each_entry_safe(vma, vn, &vm->bound_list, vm_link) { >>>> + struct drm_i915_gem_object *obj = vma->obj; >>>> + >>>> GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); >>>> + >>>> i915_vma_wait_for_bind(vma); >>>> >>>> - if (i915_vma_is_pinned(vma)) >>>> + if (i915_vma_is_pinned(vma) || !i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) >>>> continue; >>>> >>>> - if (!i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) { >>>> - __i915_vma_evict(vma); >>>> - drm_mm_remove_node(&vma->node); >>>> + /* unlikely to race when GPU is idle, so no worry about slowpath.. */ >>>> + if (!i915_gem_object_trylock(obj, NULL)) { >>>> + atomic_set(&vm->open, open); >>> Does this need a comment about barriers? >> Not sure, it's guarded by vm->mutex. >>>> + >>>> + i915_gem_object_get(obj); >>> Should this not be kref_get_unless_zero? Assuming the vm->mutex is the >>> only thing keeping the object alive here, won't this lead to potential >>> uaf/double-free or something? Also should we not plonk this before the >>> trylock? Or maybe I'm missing something here? >> Normally you're correct, this is normally the case, but we drain freed objects and this path should only be run during s/r, at which point userspace should be dead, GPU idle, and we just drained all freed objects above. >> >> It would be a bug if we still found a dead object, as nothing should be running. > Hmm, Ok. So why do we expect the trylock to ever fail here? Who else > can grab it at this stage? It probably shouldn't, should probably be a WARN if it happens. >>>> + mutex_unlock(&vm->mutex); >>>> + >>>> + i915_gem_object_lock(obj, NULL); >>>> + open = i915_vma_unbind(vma); >>>> + i915_gem_object_unlock(obj); >>>> + >>>> + GEM_WARN_ON(open); >>>> + >>>> + i915_gem_object_put(obj); >>>> + goto retry; >>>> } >>>> + >>>> + i915_vma_wait_for_bind(vma); >>> We also call wait_for_bind above, is that intentional? >> Should be harmless, but first one should probably be removed. :) >>
On Thu, 9 Dec 2021 at 13:46, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > > On 09-12-2021 14:40, Matthew Auld wrote: > > On Thu, 9 Dec 2021 at 13:25, Maarten Lankhorst > > <maarten.lankhorst@linux.intel.com> wrote: > >> On 09-12-2021 14:05, Matthew Auld wrote: > >>> On Mon, 29 Nov 2021 at 13:58, Maarten Lankhorst > >>> <maarten.lankhorst@linux.intel.com> wrote: > >>>> We want to remove more members of i915_vma, which requires the locking to be > >>>> held more often. > >>>> > >>>> Start requiring gem object lock for i915_vma_unbind, as it's one of the > >>>> callers that may unpin pages. > >>>> > >>>> Some special care is needed when evicting, because the last reference to the > >>>> object may be held by the VMA, so after __i915_vma_unbind, vma may be garbage, > >>>> and we need to cache vma->obj before unlocking. > >>>> > >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>> --- > >>> <snip> > >>> > >>>> @@ -129,22 +129,47 @@ void i915_ggtt_suspend_vm(struct i915_address_space *vm) > >>>> > >>>> drm_WARN_ON(&vm->i915->drm, !vm->is_ggtt && !vm->is_dpt); > >>>> > >>>> +retry: > >>>> + i915_gem_drain_freed_objects(vm->i915); > >>>> + > >>>> mutex_lock(&vm->mutex); > >>>> > >>>> /* Skip rewriting PTE on VMA unbind. */ > >>>> open = atomic_xchg(&vm->open, 0); > >>>> > >>>> list_for_each_entry_safe(vma, vn, &vm->bound_list, vm_link) { > >>>> + struct drm_i915_gem_object *obj = vma->obj; > >>>> + > >>>> GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); > >>>> + > >>>> i915_vma_wait_for_bind(vma); > >>>> > >>>> - if (i915_vma_is_pinned(vma)) > >>>> + if (i915_vma_is_pinned(vma) || !i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) > >>>> continue; > >>>> > >>>> - if (!i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) { > >>>> - __i915_vma_evict(vma); > >>>> - drm_mm_remove_node(&vma->node); > >>>> + /* unlikely to race when GPU is idle, so no worry about slowpath.. */ > >>>> + if (!i915_gem_object_trylock(obj, NULL)) { > >>>> + atomic_set(&vm->open, open); > >>> Does this need a comment about barriers? > >> Not sure, it's guarded by vm->mutex. > >>>> + > >>>> + i915_gem_object_get(obj); > >>> Should this not be kref_get_unless_zero? Assuming the vm->mutex is the > >>> only thing keeping the object alive here, won't this lead to potential > >>> uaf/double-free or something? Also should we not plonk this before the > >>> trylock? Or maybe I'm missing something here? > >> Normally you're correct, this is normally the case, but we drain freed objects and this path should only be run during s/r, at which point userspace should be dead, GPU idle, and we just drained all freed objects above. > >> > >> It would be a bug if we still found a dead object, as nothing should be running. > > Hmm, Ok. So why do we expect the trylock to ever fail here? Who else > > can grab it at this stage? > It probably shouldn't, should probably be a WARN if it happens. r-b with that then. > >>>> + mutex_unlock(&vm->mutex); > >>>> + > >>>> + i915_gem_object_lock(obj, NULL); > >>>> + open = i915_vma_unbind(vma); > >>>> + i915_gem_object_unlock(obj); > >>>> + > >>>> + GEM_WARN_ON(open); > >>>> + > >>>> + i915_gem_object_put(obj); > >>>> + goto retry; > >>>> } > >>>> + > >>>> + i915_vma_wait_for_bind(vma); > >>> We also call wait_for_bind above, is that intentional? > >> Should be harmless, but first one should probably be removed. :) > >> >
diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c index 3b20f69e0240..3aec972d9382 100644 --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c @@ -47,7 +47,7 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, goto err; if (i915_vma_misplaced(vma, 0, alignment, 0)) { - ret = i915_vma_unbind(vma); + ret = i915_vma_unbind_unlocked(vma); if (ret) { vma = ERR_PTR(ret); goto err; diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c index c69c7d45aabc..0db62652e3ba 100644 --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c @@ -647,7 +647,7 @@ static int igt_mock_ppgtt_misaligned_dma(void *arg) * pages. */ for (offset = 4096; offset < page_size; offset += 4096) { - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); if (err) goto out_unpin; diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c index 8402ed925a69..8fb5be799b3c 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c @@ -318,7 +318,7 @@ static int pin_buffer(struct i915_vma *vma, u64 addr) int err; if (drm_mm_node_allocated(&vma->node) && vma->node.start != addr) { - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); if (err) 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 6d30cdfa80f3..e69e8861352d 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c @@ -165,7 +165,9 @@ static int check_partial_mapping(struct drm_i915_gem_object *obj, kunmap(p); out: + i915_gem_object_lock(obj, NULL); __i915_vma_put(vma); + i915_gem_object_unlock(obj); return err; } @@ -259,7 +261,9 @@ static int check_partial_mappings(struct drm_i915_gem_object *obj, if (err) return err; + i915_gem_object_lock(obj, NULL); __i915_vma_put(vma); + i915_gem_object_unlock(obj); if (igt_timeout(end_time, "%s: timed out after tiling=%d stride=%d\n", @@ -1349,7 +1353,9 @@ static int __igt_mmap_revoke(struct drm_i915_private *i915, * for other objects. Ergo we have to revoke the previous mmap PTE * access as it no longer points to the same object. */ + i915_gem_object_lock(obj, NULL); err = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE); + i915_gem_object_unlock(obj); if (err) { pr_err("Failed to unbind object!\n"); goto out_unmap; diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index 17e2e01b8d25..f9790f45a492 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c @@ -129,22 +129,47 @@ void i915_ggtt_suspend_vm(struct i915_address_space *vm) drm_WARN_ON(&vm->i915->drm, !vm->is_ggtt && !vm->is_dpt); +retry: + i915_gem_drain_freed_objects(vm->i915); + mutex_lock(&vm->mutex); /* Skip rewriting PTE on VMA unbind. */ open = atomic_xchg(&vm->open, 0); list_for_each_entry_safe(vma, vn, &vm->bound_list, vm_link) { + struct drm_i915_gem_object *obj = vma->obj; + GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); + i915_vma_wait_for_bind(vma); - if (i915_vma_is_pinned(vma)) + if (i915_vma_is_pinned(vma) || !i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) continue; - if (!i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) { - __i915_vma_evict(vma); - drm_mm_remove_node(&vma->node); + /* unlikely to race when GPU is idle, so no worry about slowpath.. */ + if (!i915_gem_object_trylock(obj, NULL)) { + atomic_set(&vm->open, open); + + i915_gem_object_get(obj); + mutex_unlock(&vm->mutex); + + i915_gem_object_lock(obj, NULL); + open = i915_vma_unbind(vma); + i915_gem_object_unlock(obj); + + GEM_WARN_ON(open); + + i915_gem_object_put(obj); + goto retry; } + + i915_vma_wait_for_bind(vma); + + __i915_vma_evict(vma); + drm_mm_remove_node(&vma->node); + + i915_gem_object_unlock(obj); } vm->clear_range(vm, 0, vm->total); @@ -742,11 +767,21 @@ static void ggtt_cleanup_hw(struct i915_ggtt *ggtt) atomic_set(&ggtt->vm.open, 0); flush_workqueue(ggtt->vm.i915->wq); + i915_gem_drain_freed_objects(ggtt->vm.i915); mutex_lock(&ggtt->vm.mutex); - list_for_each_entry_safe(vma, vn, &ggtt->vm.bound_list, vm_link) + list_for_each_entry_safe(vma, vn, &ggtt->vm.bound_list, vm_link) { + struct drm_i915_gem_object *obj = vma->obj; + bool trylock; + + trylock = i915_gem_object_trylock(obj, NULL); + WARN_ON(!trylock); + WARN_ON(__i915_vma_unbind(vma)); + if (trylock) + i915_gem_object_unlock(obj); + } if (drm_mm_node_allocated(&ggtt->error_capture)) drm_mm_remove_node(&ggtt->error_capture); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index e83522953cde..5ce38027e0fd 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -118,6 +118,8 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj, struct i915_vma *vma; int ret; + assert_object_held(obj); + if (list_empty(&obj->vma.list)) return 0; diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 74b88b130cd5..7261d82162af 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -1598,8 +1598,16 @@ void i915_vma_parked(struct intel_gt *gt) struct drm_i915_gem_object *obj = vma->obj; struct i915_address_space *vm = vma->vm; - INIT_LIST_HEAD(&vma->closed_link); - __i915_vma_put(vma); + if (i915_gem_object_trylock(obj, NULL)) { + INIT_LIST_HEAD(&vma->closed_link); + __i915_vma_put(vma); + i915_gem_object_unlock(obj); + } else { + /* back you go.. */ + spin_lock_irq(>->closed_lock); + list_add(&vma->closed_link, >->closed_vma); + spin_unlock_irq(>->closed_lock); + } i915_gem_object_put(obj); i915_vm_close(vm); @@ -1715,6 +1723,7 @@ int _i915_vma_move_to_active(struct i915_vma *vma, void __i915_vma_evict(struct i915_vma *vma) { GEM_BUG_ON(i915_vma_is_pinned(vma)); + assert_object_held_shared(vma->obj); if (i915_vma_is_map_and_fenceable(vma)) { /* Force a pagefault for domain tracking on next user access */ @@ -1760,6 +1769,7 @@ int __i915_vma_unbind(struct i915_vma *vma) int ret; lockdep_assert_held(&vma->vm->mutex); + assert_object_held_shared(vma->obj); if (!drm_mm_node_allocated(&vma->node)) return 0; @@ -1791,6 +1801,8 @@ int i915_vma_unbind(struct i915_vma *vma) intel_wakeref_t wakeref = 0; int err; + assert_object_held_shared(vma->obj); + /* Optimistic wait before taking the mutex */ err = i915_vma_sync(vma); if (err) @@ -1821,6 +1833,17 @@ int i915_vma_unbind(struct i915_vma *vma) return err; } +int i915_vma_unbind_unlocked(struct i915_vma *vma) +{ + int err; + + i915_gem_object_lock(vma->obj, NULL); + err = i915_vma_unbind(vma); + i915_gem_object_unlock(vma->obj); + + return err; +} + struct i915_vma *i915_vma_make_unshrinkable(struct i915_vma *vma) { i915_gem_object_make_unshrinkable(vma->obj); diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h index 32719431b3df..da69ecb1b860 100644 --- a/drivers/gpu/drm/i915/i915_vma.h +++ b/drivers/gpu/drm/i915/i915_vma.h @@ -214,6 +214,7 @@ void i915_vma_revoke_mmap(struct i915_vma *vma); void __i915_vma_evict(struct i915_vma *vma); int __i915_vma_unbind(struct i915_vma *vma); int __must_check i915_vma_unbind(struct i915_vma *vma); +int __must_check i915_vma_unbind_unlocked(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); diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c index 6b1db83119b3..de16897d3d9a 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c @@ -385,7 +385,7 @@ static void close_object_list(struct list_head *objects, vma = i915_vma_instance(obj, vm, NULL); if (!IS_ERR(vma)) - ignored = i915_vma_unbind(vma); + ignored = i915_vma_unbind_unlocked(vma); list_del(&obj->st_link); i915_gem_object_put(obj); @@ -496,7 +496,7 @@ static int fill_hole(struct i915_address_space *vm, goto err; } - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); if (err) { pr_err("%s(%s) (forward) unbind of vma.node=%llx + %llx failed with err=%d\n", __func__, p->name, vma->node.start, vma->node.size, @@ -569,7 +569,7 @@ static int fill_hole(struct i915_address_space *vm, goto err; } - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); if (err) { pr_err("%s(%s) (backward) unbind of vma.node=%llx + %llx failed with err=%d\n", __func__, p->name, vma->node.start, vma->node.size, @@ -655,7 +655,7 @@ static int walk_hole(struct i915_address_space *vm, goto err_put; } - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); if (err) { pr_err("%s unbind failed at %llx + %llx with err=%d\n", __func__, addr, vma->size, err); @@ -732,13 +732,13 @@ static int pot_hole(struct i915_address_space *vm, pr_err("%s incorrect at %llx + %llx\n", __func__, addr, vma->size); i915_vma_unpin(vma); - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); err = -EINVAL; goto err_obj; } i915_vma_unpin(vma); - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); GEM_BUG_ON(err); } @@ -832,13 +832,13 @@ static int drunk_hole(struct i915_address_space *vm, pr_err("%s incorrect at %llx + %llx\n", __func__, addr, BIT_ULL(size)); i915_vma_unpin(vma); - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); err = -EINVAL; goto err_obj; } i915_vma_unpin(vma); - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); GEM_BUG_ON(err); if (igt_timeout(end_time, @@ -906,7 +906,7 @@ static int __shrink_hole(struct i915_address_space *vm, pr_err("%s incorrect at %llx + %llx\n", __func__, addr, size); i915_vma_unpin(vma); - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); err = -EINVAL; break; } @@ -1465,7 +1465,7 @@ static int igt_gtt_reserve(void *arg) goto out; } - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); if (err) { pr_err("i915_vma_unbind failed with err=%d!\n", err); goto out; @@ -1647,7 +1647,7 @@ static int igt_gtt_insert(void *arg) GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); offset = vma->node.start; - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); if (err) { pr_err("i915_vma_unbind failed with err=%d!\n", err); goto out; diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c b/drivers/gpu/drm/i915/selftests/i915_vma.c index 5c5809dfe9b2..2c73f7448df7 100644 --- a/drivers/gpu/drm/i915/selftests/i915_vma.c +++ b/drivers/gpu/drm/i915/selftests/i915_vma.c @@ -340,7 +340,7 @@ static int igt_vma_pin1(void *arg) if (!err) { i915_vma_unpin(vma); - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); if (err) { pr_err("Failed to unbind single page from GGTT, err=%d\n", err); goto out; @@ -691,7 +691,7 @@ static int igt_vma_rotate_remap(void *arg) } i915_vma_unpin(vma); - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); if (err) { pr_err("Unbinding returned %i\n", err); goto out_object; @@ -852,7 +852,7 @@ static int igt_vma_partial(void *arg) i915_vma_unpin(vma); nvma++; - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); if (err) { pr_err("Unbinding returned %i\n", err); goto out_object; @@ -891,7 +891,7 @@ static int igt_vma_partial(void *arg) i915_vma_unpin(vma); - err = i915_vma_unbind(vma); + err = i915_vma_unbind_unlocked(vma); if (err) { pr_err("Unbinding returned %i\n", err); goto out_object;
We want to remove more members of i915_vma, which requires the locking to be held more often. Start requiring gem object lock for i915_vma_unbind, as it's one of the callers that may unpin pages. Some special care is needed when evicting, because the last reference to the object may be held by the VMA, so after __i915_vma_unbind, vma may be garbage, and we need to cache vma->obj before unlocking. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/display/intel_fb_pin.c | 2 +- .../gpu/drm/i915/gem/selftests/huge_pages.c | 2 +- .../i915/gem/selftests/i915_gem_client_blt.c | 2 +- .../drm/i915/gem/selftests/i915_gem_mman.c | 6 +++ drivers/gpu/drm/i915/gt/intel_ggtt.c | 45 ++++++++++++++++--- drivers/gpu/drm/i915/i915_gem.c | 2 + drivers/gpu/drm/i915/i915_vma.c | 27 ++++++++++- drivers/gpu/drm/i915/i915_vma.h | 1 + drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 22 ++++----- drivers/gpu/drm/i915/selftests/i915_vma.c | 8 ++-- 10 files changed, 92 insertions(+), 25 deletions(-)