| Message ID | 20211129134735.628712-11-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: > > i915_gem_execbuf will call i915_gem_evict_vm() after failing to pin > all objects in the first round. We are about to remove those short-term > pins, but even without those the objects are still locked. Add a special > case to allow i915_gem_evict_vm to evict locked objects as well. > > This might also allow multiple objects sharing the same resv to be evicted. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Reviewed-by: Matthew Auld <matthew.auld@intel.com> Do we need similar treatment for stuff like evict_for_node etc? > --- > drivers/gpu/drm/i915/i915_gem_evict.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c > index 24f5e3345e43..f502a617b35c 100644 > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > @@ -410,21 +410,42 @@ int i915_gem_evict_vm(struct i915_address_space *vm, struct i915_gem_ww_ctx *ww) > do { > struct i915_vma *vma, *vn; > LIST_HEAD(eviction_list); > + LIST_HEAD(locked_eviction_list); > > list_for_each_entry(vma, &vm->bound_list, vm_link) { > if (i915_vma_is_pinned(vma)) > continue; > > + /* > + * If we already own the lock, trylock fails. In case the resv > + * is shared among multiple objects, we still need the object ref. > + */ > + if (ww && (dma_resv_locking_ctx(vma->obj->base.resv) == &ww->ctx)) { > + __i915_vma_pin(vma); > + list_add(&vma->evict_link, &locked_eviction_list); > + continue; > + } > + > if (!i915_gem_object_trylock(vma->obj, ww)) > continue; > > __i915_vma_pin(vma); > list_add(&vma->evict_link, &eviction_list); > } > - if (list_empty(&eviction_list)) > + if (list_empty(&eviction_list) && list_empty(&locked_eviction_list)) > break; > > ret = 0; > + /* Unbind locked objects first, before unlocking the eviction_list */ > + list_for_each_entry_safe(vma, vn, &locked_eviction_list, evict_link) { > + __i915_vma_unpin(vma); > + > + if (ret == 0) > + ret = __i915_vma_unbind(vma); > + if (ret != -EINTR) /* "Get me out of here!" */ > + ret = 0; > + } > + > list_for_each_entry_safe(vma, vn, &eviction_list, evict_link) { > __i915_vma_unpin(vma); > if (ret == 0) > -- > 2.34.0 >
On 08-12-2021 13:07, Matthew Auld wrote: > On Mon, 29 Nov 2021 at 13:58, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: >> i915_gem_execbuf will call i915_gem_evict_vm() after failing to pin >> all objects in the first round. We are about to remove those short-term >> pins, but even without those the objects are still locked. Add a special >> case to allow i915_gem_evict_vm to evict locked objects as well. >> >> This might also allow multiple objects sharing the same resv to be evicted. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Reviewed-by: Matthew Auld <matthew.auld@intel.com> > > Do we need similar treatment for stuff like evict_for_node etc? Unfortunately not, we would risk evicting newly bound objects when we completely drop short term pinning evict_vm is the exception, because you expect it to clean up the entire vm as much as possible, and is called explictly, not as a part of pinning. :) >> --- >> drivers/gpu/drm/i915/i915_gem_evict.c | 23 ++++++++++++++++++++++- >> 1 file changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c >> index 24f5e3345e43..f502a617b35c 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_evict.c >> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c >> @@ -410,21 +410,42 @@ int i915_gem_evict_vm(struct i915_address_space *vm, struct i915_gem_ww_ctx *ww) >> do { >> struct i915_vma *vma, *vn; >> LIST_HEAD(eviction_list); >> + LIST_HEAD(locked_eviction_list); >> >> list_for_each_entry(vma, &vm->bound_list, vm_link) { >> if (i915_vma_is_pinned(vma)) >> continue; >> >> + /* >> + * If we already own the lock, trylock fails. In case the resv >> + * is shared among multiple objects, we still need the object ref. >> + */ >> + if (ww && (dma_resv_locking_ctx(vma->obj->base.resv) == &ww->ctx)) { >> + __i915_vma_pin(vma); >> + list_add(&vma->evict_link, &locked_eviction_list); >> + continue; >> + } >> + >> if (!i915_gem_object_trylock(vma->obj, ww)) >> continue; >> >> __i915_vma_pin(vma); >> list_add(&vma->evict_link, &eviction_list); >> } >> - if (list_empty(&eviction_list)) >> + if (list_empty(&eviction_list) && list_empty(&locked_eviction_list)) >> break; >> >> ret = 0; >> + /* Unbind locked objects first, before unlocking the eviction_list */ >> + list_for_each_entry_safe(vma, vn, &locked_eviction_list, evict_link) { >> + __i915_vma_unpin(vma); >> + >> + if (ret == 0) >> + ret = __i915_vma_unbind(vma); >> + if (ret != -EINTR) /* "Get me out of here!" */ >> + ret = 0; >> + } >> + >> list_for_each_entry_safe(vma, vn, &eviction_list, evict_link) { >> __i915_vma_unpin(vma); >> if (ret == 0) >> -- >> 2.34.0 >>
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index 24f5e3345e43..f502a617b35c 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -410,21 +410,42 @@ int i915_gem_evict_vm(struct i915_address_space *vm, struct i915_gem_ww_ctx *ww) do { struct i915_vma *vma, *vn; LIST_HEAD(eviction_list); + LIST_HEAD(locked_eviction_list); list_for_each_entry(vma, &vm->bound_list, vm_link) { if (i915_vma_is_pinned(vma)) continue; + /* + * If we already own the lock, trylock fails. In case the resv + * is shared among multiple objects, we still need the object ref. + */ + if (ww && (dma_resv_locking_ctx(vma->obj->base.resv) == &ww->ctx)) { + __i915_vma_pin(vma); + list_add(&vma->evict_link, &locked_eviction_list); + continue; + } + if (!i915_gem_object_trylock(vma->obj, ww)) continue; __i915_vma_pin(vma); list_add(&vma->evict_link, &eviction_list); } - if (list_empty(&eviction_list)) + if (list_empty(&eviction_list) && list_empty(&locked_eviction_list)) break; ret = 0; + /* Unbind locked objects first, before unlocking the eviction_list */ + list_for_each_entry_safe(vma, vn, &locked_eviction_list, evict_link) { + __i915_vma_unpin(vma); + + if (ret == 0) + ret = __i915_vma_unbind(vma); + if (ret != -EINTR) /* "Get me out of here!" */ + ret = 0; + } + list_for_each_entry_safe(vma, vn, &eviction_list, evict_link) { __i915_vma_unpin(vma); if (ret == 0)
i915_gem_execbuf will call i915_gem_evict_vm() after failing to pin all objects in the first round. We are about to remove those short-term pins, but even without those the objects are still locked. Add a special case to allow i915_gem_evict_vm to evict locked objects as well. This might also allow multiple objects sharing the same resv to be evicted. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/i915_gem_evict.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)