| Message ID | 20180426174932.23127-7-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26/04/2018 18:49, Chris Wilson wrote: > When userspace is passing around swapbuffers using DRI, we frequently > have to open and close the same object in the foreign address space. > This shows itself as the same object being rebound at roughly 30fps > (with a second object also being rebound at 30fps), which involves us > having to rewrite the page tables and maintain the drm_mm range manager > every time. > > However, since the object still exists and it is only the local handle > that disappears, if we are lazy and do not unbind the VMA immediately > when the local user closes the object but defer it until the GPU is > idle, then we can reuse the same VMA binding. We still have to be > careful to mark the handle and lookup tables as closed to maintain the > uABI, just allowing the underlying VMA to be resurrected if the user is > able to access the same object from the same context again. > > If the object itself is destroyed (neither userspace keeping a handle to > it), the VMA will be reaped immediately as usual. > > In the future, this will be even more useful as instantiating a new VMA > for use on the GPU will become heavier. A nuisance indeed, so nip it in > the bud. > > v2: s/__i915_vma_final_close/i915_vma_destroy/ etc. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem.c | 4 +- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +- > drivers/gpu/drm/i915/i915_gem_gtt.c | 14 +++-- > drivers/gpu/drm/i915/i915_vma.c | 61 +++++++++++++------ > drivers/gpu/drm/i915/i915_vma.h | 6 ++ > drivers/gpu/drm/i915/selftests/huge_pages.c | 2 +- > .../gpu/drm/i915/selftests/mock_gem_device.c | 1 + > 8 files changed, 67 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index dab15b6abc3c..d4da9f941d04 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2061,6 +2061,7 @@ struct drm_i915_private { > struct list_head timelines; > > struct list_head active_rings; > + struct list_head closed_vma; > u32 active_requests; > u32 request_serial; > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 484354f25f98..5ece6ae4bdff 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -165,6 +165,7 @@ static u32 __i915_gem_park(struct drm_i915_private *i915) > i915_timelines_park(i915); > > i915_pmu_gt_parked(i915); > + i915_vma_parked(i915); > > i915->gt.awake = false; > > @@ -4795,7 +4796,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915, > &obj->vma_list, obj_link) { > GEM_BUG_ON(i915_vma_is_active(vma)); > vma->flags &= ~I915_VMA_PIN_MASK; > - i915_vma_close(vma); > + i915_vma_destroy(vma); > } > GEM_BUG_ON(!list_empty(&obj->vma_list)); > GEM_BUG_ON(!RB_EMPTY_ROOT(&obj->vma_tree)); > @@ -5598,6 +5599,7 @@ int i915_gem_init_early(struct drm_i915_private *dev_priv) > > INIT_LIST_HEAD(&dev_priv->gt.timelines); > INIT_LIST_HEAD(&dev_priv->gt.active_rings); > + INIT_LIST_HEAD(&dev_priv->gt.closed_vma); > > i915_gem_init__mm(dev_priv); > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index c74f5df3fb5a..f627a8c47c58 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -762,7 +762,8 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) > } > > /* transfer ref to ctx */ > - vma->open_count++; > + if (!vma->open_count++) > + i915_vma_reopen(vma); So only execbuf path gets to be able to reopen the VMA? I assume this is sufficient for the use case commit message describes? Other potential use cases are not interesting? > list_add(&lut->obj_link, &obj->lut_list); > list_add(&lut->ctx_link, &eb->ctx->handles_list); > lut->ctx = eb->ctx; > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index e9d828324f67..272d6bb407cc 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -2218,6 +2218,12 @@ i915_ppgtt_create(struct drm_i915_private *dev_priv, > } > > void i915_ppgtt_close(struct i915_address_space *vm) > +{ > + GEM_BUG_ON(vm->closed); > + vm->closed = true; > +} > + > +static void ppgtt_destroy_vma(struct i915_address_space *vm) > { > struct list_head *phases[] = { > &vm->active_list, > @@ -2226,15 +2232,12 @@ void i915_ppgtt_close(struct i915_address_space *vm) > NULL, > }, **phase; > > - GEM_BUG_ON(vm->closed); > vm->closed = true; There are no more references at this point so no need to mark it as closed I think. > - > for (phase = phases; *phase; phase++) { > struct i915_vma *vma, *vn; > > list_for_each_entry_safe(vma, vn, *phase, vm_link) > - if (!i915_vma_is_closed(vma)) > - i915_vma_close(vma); > + i915_vma_destroy(vma); > } > } > > @@ -2245,7 +2248,8 @@ void i915_ppgtt_release(struct kref *kref) > > trace_i915_ppgtt_release(&ppgtt->base); > > - /* vmas should already be unbound and destroyed */ > + ppgtt_destroy_vma(&ppgtt->base); > + > GEM_BUG_ON(!list_empty(&ppgtt->base.active_list)); > GEM_BUG_ON(!list_empty(&ppgtt->base.inactive_list)); > GEM_BUG_ON(!list_empty(&ppgtt->base.unbound_list)); > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > index 4bda3bd29bf5..81fa652ad78d 100644 > --- a/drivers/gpu/drm/i915/i915_vma.c > +++ b/drivers/gpu/drm/i915/i915_vma.c > @@ -46,8 +46,6 @@ i915_vma_retire(struct i915_gem_active *active, struct i915_request *rq) > > GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); > list_move_tail(&vma->vm_link, &vma->vm->inactive_list); > - if (unlikely(i915_vma_is_closed(vma) && !i915_vma_is_pinned(vma))) > - WARN_ON(i915_vma_unbind(vma)); > > GEM_BUG_ON(!i915_gem_object_is_active(obj)); > if (--obj->active_count) > @@ -232,7 +230,6 @@ i915_vma_instance(struct drm_i915_gem_object *obj, > if (!vma) > vma = vma_create(obj, vm, view); > > - GEM_BUG_ON(!IS_ERR(vma) && i915_vma_is_closed(vma)); > GEM_BUG_ON(!IS_ERR(vma) && i915_vma_compare(vma, vm, view)); > GEM_BUG_ON(!IS_ERR(vma) && vma_lookup(obj, vm, view) != vma); > return vma; > @@ -684,13 +681,31 @@ int __i915_vma_do_pin(struct i915_vma *vma, > return ret; > } > > -static void i915_vma_destroy(struct i915_vma *vma) > +void i915_vma_close(struct i915_vma *vma) > +{ > + lockdep_assert_held(&vma->vm->i915->drm.struct_mutex); > + > + GEM_BUG_ON(i915_vma_is_closed(vma)); I think the VMA code has gotten pretty messy. For instance a couple of external callers of is_vma_closed feel out of place. Like they should try to do what ever they want with the VMA, say pin it, or close it, and then that operation should either fail or handle the fact, respectively. But just another grumble at this point. > + vma->flags |= I915_VMA_CLOSED; > + > + list_add_tail(&vma->closed_link, &vma->vm->i915->gt.closed_vma); > +} I think a comment next to this function might be good, doesn't have to be long, just to mention the rationale behind lazy unbind/destroy. Just because often after refactorings and code churn it is difficult to find the commit associated with some logical piece of the code. > + > +void i915_vma_reopen(struct i915_vma *vma) > +{ > + lockdep_assert_held(&vma->vm->i915->drm.struct_mutex); > + > + if (vma->flags & I915_VMA_CLOSED) { > + vma->flags &= ~I915_VMA_CLOSED; > + list_del(&vma->closed_link); > + } > +} And then continuing the grumble, this helper wouldn't be needed. If someone had a vlaid vma reference, and tried to do something meaningful wiht it, the vma code would re-open it under the covers. > + > +static void __i915_vma_destroy(struct i915_vma *vma) > { > int i; > > GEM_BUG_ON(vma->node.allocated); > - GEM_BUG_ON(i915_vma_is_active(vma)); > - GEM_BUG_ON(!i915_vma_is_closed(vma)); > GEM_BUG_ON(vma->fence); > > for (i = 0; i < ARRAY_SIZE(vma->last_read); i++) > @@ -699,6 +714,7 @@ static void i915_vma_destroy(struct i915_vma *vma) > > list_del(&vma->obj_link); > list_del(&vma->vm_link); > + rb_erase(&vma->obj_node, &vma->obj->vma_tree); > > if (!i915_vma_is_ggtt(vma)) > i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm)); > @@ -706,15 +722,30 @@ static void i915_vma_destroy(struct i915_vma *vma) > kmem_cache_free(to_i915(vma->obj->base.dev)->vmas, vma); > } > > -void i915_vma_close(struct i915_vma *vma) > +void i915_vma_destroy(struct i915_vma *vma) > { > - GEM_BUG_ON(i915_vma_is_closed(vma)); > - vma->flags |= I915_VMA_CLOSED; > + lockdep_assert_held(&vma->vm->i915->drm.struct_mutex); > > - rb_erase(&vma->obj_node, &vma->obj->vma_tree); > + GEM_BUG_ON(i915_vma_is_active(vma)); > + GEM_BUG_ON(i915_vma_is_pinned(vma)); > + > + if (i915_vma_is_closed(vma)) > + list_del(&vma->closed_link); > > - if (!i915_vma_is_active(vma) && !i915_vma_is_pinned(vma)) > - WARN_ON(i915_vma_unbind(vma)); > + WARN_ON(i915_vma_unbind(vma)); > + __i915_vma_destroy(vma); > +} > + > +void i915_vma_parked(struct drm_i915_private *i915) > +{ > + struct i915_vma *vma, *next; > + > + list_for_each_entry_safe(vma, next, &i915->gt.closed_vma, closed_link) { > + GEM_BUG_ON(!i915_vma_is_closed(vma)); > + i915_vma_destroy(vma); > + } > + > + GEM_BUG_ON(!list_empty(&i915->gt.closed_vma)); > } > > static void __i915_vma_iounmap(struct i915_vma *vma) > @@ -804,7 +835,7 @@ int i915_vma_unbind(struct i915_vma *vma) > return -EBUSY; > > if (!drm_mm_node_allocated(&vma->node)) > - goto destroy; > + return 0; > > GEM_BUG_ON(obj->bind_count == 0); > GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj)); > @@ -841,10 +872,6 @@ int i915_vma_unbind(struct i915_vma *vma) > > i915_vma_remove(vma); > > -destroy: > - if (unlikely(i915_vma_is_closed(vma))) > - i915_vma_destroy(vma); Good this is gone, it was quite confusing why it was here. > - > return 0; > } > > diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h > index 8c5022095418..fc4294cfaa91 100644 > --- a/drivers/gpu/drm/i915/i915_vma.h > +++ b/drivers/gpu/drm/i915/i915_vma.h > @@ -119,6 +119,8 @@ struct i915_vma { > /** This vma's place in the eviction list */ > struct list_head evict_link; > > + struct list_head closed_link; > + > /** > * Used for performing relocations during execbuffer insertion. > */ > @@ -285,6 +287,8 @@ void i915_vma_revoke_mmap(struct i915_vma *vma); > 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); > > int __i915_vma_do_pin(struct i915_vma *vma, > u64 size, u64 alignment, u64 flags); > @@ -408,6 +412,8 @@ i915_vma_unpin_fence(struct i915_vma *vma) > __i915_vma_unpin_fence(vma); > } > > +void i915_vma_parked(struct drm_i915_private *i915); > + > #define for_each_until(cond) if (cond) break; else > > /** > diff --git a/drivers/gpu/drm/i915/selftests/huge_pages.c b/drivers/gpu/drm/i915/selftests/huge_pages.c > index 05bbef363fff..d7c8ef8e6764 100644 > --- a/drivers/gpu/drm/i915/selftests/huge_pages.c > +++ b/drivers/gpu/drm/i915/selftests/huge_pages.c > @@ -1091,7 +1091,7 @@ static int __igt_write_huge(struct i915_gem_context *ctx, > out_vma_unpin: > i915_vma_unpin(vma); > out_vma_close: > - i915_vma_close(vma); > + i915_vma_destroy(vma); > > return err; > } > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > index a662c0450e77..4b6622c6986a 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > @@ -226,6 +226,7 @@ struct drm_i915_private *mock_gem_device(void) > > INIT_LIST_HEAD(&i915->gt.timelines); > INIT_LIST_HEAD(&i915->gt.active_rings); > + INIT_LIST_HEAD(&i915->gt.closed_vma); > > mutex_lock(&i915->drm.struct_mutex); > mock_init_ggtt(i915); > Only two actionable things AFAIR. Then it looks OK to me. Although I would a) see if you can get Joonas to read through it - perhaps he spots something I missed, and b) ah no, won't mention any pencilling in of looking at overall vma handling in the future. Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-04-27 16:38:47) > > On 26/04/2018 18:49, Chris Wilson wrote: > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index c74f5df3fb5a..f627a8c47c58 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -762,7 +762,8 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) > > } > > > > /* transfer ref to ctx */ > > - vma->open_count++; > > + if (!vma->open_count++) > > + i915_vma_reopen(vma); > > So only execbuf path gets to be able to reopen the VMA? I assume this is > sufficient for the use case commit message describes? Other potential > use cases are not interesting? It's the only generator/consumer of user vma. Everything else is the global gtt. Think PIN_USER vs PIN_GLOBAL. > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > > index e9d828324f67..272d6bb407cc 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > @@ -2218,6 +2218,12 @@ i915_ppgtt_create(struct drm_i915_private *dev_priv, > > } > > > > void i915_ppgtt_close(struct i915_address_space *vm) > > +{ > > + GEM_BUG_ON(vm->closed); > > + vm->closed = true; > > +} > > + > > +static void ppgtt_destroy_vma(struct i915_address_space *vm) > > { > > struct list_head *phases[] = { > > &vm->active_list, > > @@ -2226,15 +2232,12 @@ void i915_ppgtt_close(struct i915_address_space *vm) > > NULL, > > }, **phase; > > > > - GEM_BUG_ON(vm->closed); > > vm->closed = true; > > There are no more references at this point so no need to mark it as > closed I think. It's a trick for a quicker i915_vma_unbind that skips the PTE updates as we know the ppgtt is being torn down. > > -static void i915_vma_destroy(struct i915_vma *vma) > > +void i915_vma_close(struct i915_vma *vma) > > +{ > > + lockdep_assert_held(&vma->vm->i915->drm.struct_mutex); > > + > > + GEM_BUG_ON(i915_vma_is_closed(vma)); > > I think the VMA code has gotten pretty messy. For instance a couple of > external callers of is_vma_closed feel out of place. Like they should > try to do what ever they want with the VMA, say pin it, or close it, and > then that operation should either fail or handle the fact, respectively. > But just another grumble at this point. But closing twice would be weird. The assert is here because the code as is would be broken if called twice. > > + vma->flags |= I915_VMA_CLOSED; > > + > > + list_add_tail(&vma->closed_link, &vma->vm->i915->gt.closed_vma); > > +} > > I think a comment next to this function might be good, doesn't have to > be long, just to mention the rationale behind lazy unbind/destroy. Just > because often after refactorings and code churn it is difficult to find > the commit associated with some logical piece of the code. > > > + > > +void i915_vma_reopen(struct i915_vma *vma) > > +{ > > + lockdep_assert_held(&vma->vm->i915->drm.struct_mutex); > > + > > + if (vma->flags & I915_VMA_CLOSED) { > > + vma->flags &= ~I915_VMA_CLOSED; > > + list_del(&vma->closed_link); > > + } > > +} > > And then continuing the grumble, this helper wouldn't be needed. If > someone had a vlaid vma reference, and tried to do something meaningful > wiht it, the vma code would re-open it under the covers. Oh no, no, no, no ;) Magically reappearing vma reeks of uabi exposure. > > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > index a662c0450e77..4b6622c6986a 100644 > > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > @@ -226,6 +226,7 @@ struct drm_i915_private *mock_gem_device(void) > > > > INIT_LIST_HEAD(&i915->gt.timelines); > > INIT_LIST_HEAD(&i915->gt.active_rings); > > + INIT_LIST_HEAD(&i915->gt.closed_vma); > > > > mutex_lock(&i915->drm.struct_mutex); > > mock_init_ggtt(i915); > > > > Only two actionable things AFAIR. Then it looks OK to me. Although I > would a) see if you can get Joonas to read through it - perhaps he spots > something I missed, and b) ah no, won't mention any pencilling in of > looking at overall vma handling in the future. You would only have to read it and execbuffer.c again :-p -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index dab15b6abc3c..d4da9f941d04 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2061,6 +2061,7 @@ struct drm_i915_private { struct list_head timelines; struct list_head active_rings; + struct list_head closed_vma; u32 active_requests; u32 request_serial; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 484354f25f98..5ece6ae4bdff 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -165,6 +165,7 @@ static u32 __i915_gem_park(struct drm_i915_private *i915) i915_timelines_park(i915); i915_pmu_gt_parked(i915); + i915_vma_parked(i915); i915->gt.awake = false; @@ -4795,7 +4796,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915, &obj->vma_list, obj_link) { GEM_BUG_ON(i915_vma_is_active(vma)); vma->flags &= ~I915_VMA_PIN_MASK; - i915_vma_close(vma); + i915_vma_destroy(vma); } GEM_BUG_ON(!list_empty(&obj->vma_list)); GEM_BUG_ON(!RB_EMPTY_ROOT(&obj->vma_tree)); @@ -5598,6 +5599,7 @@ int i915_gem_init_early(struct drm_i915_private *dev_priv) INIT_LIST_HEAD(&dev_priv->gt.timelines); INIT_LIST_HEAD(&dev_priv->gt.active_rings); + INIT_LIST_HEAD(&dev_priv->gt.closed_vma); i915_gem_init__mm(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index c74f5df3fb5a..f627a8c47c58 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -762,7 +762,8 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) } /* transfer ref to ctx */ - vma->open_count++; + if (!vma->open_count++) + i915_vma_reopen(vma); list_add(&lut->obj_link, &obj->lut_list); list_add(&lut->ctx_link, &eb->ctx->handles_list); lut->ctx = eb->ctx; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index e9d828324f67..272d6bb407cc 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2218,6 +2218,12 @@ i915_ppgtt_create(struct drm_i915_private *dev_priv, } void i915_ppgtt_close(struct i915_address_space *vm) +{ + GEM_BUG_ON(vm->closed); + vm->closed = true; +} + +static void ppgtt_destroy_vma(struct i915_address_space *vm) { struct list_head *phases[] = { &vm->active_list, @@ -2226,15 +2232,12 @@ void i915_ppgtt_close(struct i915_address_space *vm) NULL, }, **phase; - GEM_BUG_ON(vm->closed); vm->closed = true; - for (phase = phases; *phase; phase++) { struct i915_vma *vma, *vn; list_for_each_entry_safe(vma, vn, *phase, vm_link) - if (!i915_vma_is_closed(vma)) - i915_vma_close(vma); + i915_vma_destroy(vma); } } @@ -2245,7 +2248,8 @@ void i915_ppgtt_release(struct kref *kref) trace_i915_ppgtt_release(&ppgtt->base); - /* vmas should already be unbound and destroyed */ + ppgtt_destroy_vma(&ppgtt->base); + GEM_BUG_ON(!list_empty(&ppgtt->base.active_list)); GEM_BUG_ON(!list_empty(&ppgtt->base.inactive_list)); GEM_BUG_ON(!list_empty(&ppgtt->base.unbound_list)); diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 4bda3bd29bf5..81fa652ad78d 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -46,8 +46,6 @@ i915_vma_retire(struct i915_gem_active *active, struct i915_request *rq) GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); list_move_tail(&vma->vm_link, &vma->vm->inactive_list); - if (unlikely(i915_vma_is_closed(vma) && !i915_vma_is_pinned(vma))) - WARN_ON(i915_vma_unbind(vma)); GEM_BUG_ON(!i915_gem_object_is_active(obj)); if (--obj->active_count) @@ -232,7 +230,6 @@ i915_vma_instance(struct drm_i915_gem_object *obj, if (!vma) vma = vma_create(obj, vm, view); - GEM_BUG_ON(!IS_ERR(vma) && i915_vma_is_closed(vma)); GEM_BUG_ON(!IS_ERR(vma) && i915_vma_compare(vma, vm, view)); GEM_BUG_ON(!IS_ERR(vma) && vma_lookup(obj, vm, view) != vma); return vma; @@ -684,13 +681,31 @@ int __i915_vma_do_pin(struct i915_vma *vma, return ret; } -static void i915_vma_destroy(struct i915_vma *vma) +void i915_vma_close(struct i915_vma *vma) +{ + lockdep_assert_held(&vma->vm->i915->drm.struct_mutex); + + GEM_BUG_ON(i915_vma_is_closed(vma)); + vma->flags |= I915_VMA_CLOSED; + + list_add_tail(&vma->closed_link, &vma->vm->i915->gt.closed_vma); +} + +void i915_vma_reopen(struct i915_vma *vma) +{ + lockdep_assert_held(&vma->vm->i915->drm.struct_mutex); + + if (vma->flags & I915_VMA_CLOSED) { + vma->flags &= ~I915_VMA_CLOSED; + list_del(&vma->closed_link); + } +} + +static void __i915_vma_destroy(struct i915_vma *vma) { int i; GEM_BUG_ON(vma->node.allocated); - GEM_BUG_ON(i915_vma_is_active(vma)); - GEM_BUG_ON(!i915_vma_is_closed(vma)); GEM_BUG_ON(vma->fence); for (i = 0; i < ARRAY_SIZE(vma->last_read); i++) @@ -699,6 +714,7 @@ static void i915_vma_destroy(struct i915_vma *vma) list_del(&vma->obj_link); list_del(&vma->vm_link); + rb_erase(&vma->obj_node, &vma->obj->vma_tree); if (!i915_vma_is_ggtt(vma)) i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm)); @@ -706,15 +722,30 @@ static void i915_vma_destroy(struct i915_vma *vma) kmem_cache_free(to_i915(vma->obj->base.dev)->vmas, vma); } -void i915_vma_close(struct i915_vma *vma) +void i915_vma_destroy(struct i915_vma *vma) { - GEM_BUG_ON(i915_vma_is_closed(vma)); - vma->flags |= I915_VMA_CLOSED; + lockdep_assert_held(&vma->vm->i915->drm.struct_mutex); - rb_erase(&vma->obj_node, &vma->obj->vma_tree); + GEM_BUG_ON(i915_vma_is_active(vma)); + GEM_BUG_ON(i915_vma_is_pinned(vma)); + + if (i915_vma_is_closed(vma)) + list_del(&vma->closed_link); - if (!i915_vma_is_active(vma) && !i915_vma_is_pinned(vma)) - WARN_ON(i915_vma_unbind(vma)); + WARN_ON(i915_vma_unbind(vma)); + __i915_vma_destroy(vma); +} + +void i915_vma_parked(struct drm_i915_private *i915) +{ + struct i915_vma *vma, *next; + + list_for_each_entry_safe(vma, next, &i915->gt.closed_vma, closed_link) { + GEM_BUG_ON(!i915_vma_is_closed(vma)); + i915_vma_destroy(vma); + } + + GEM_BUG_ON(!list_empty(&i915->gt.closed_vma)); } static void __i915_vma_iounmap(struct i915_vma *vma) @@ -804,7 +835,7 @@ int i915_vma_unbind(struct i915_vma *vma) return -EBUSY; if (!drm_mm_node_allocated(&vma->node)) - goto destroy; + return 0; GEM_BUG_ON(obj->bind_count == 0); GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj)); @@ -841,10 +872,6 @@ int i915_vma_unbind(struct i915_vma *vma) i915_vma_remove(vma); -destroy: - if (unlikely(i915_vma_is_closed(vma))) - i915_vma_destroy(vma); - return 0; } diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h index 8c5022095418..fc4294cfaa91 100644 --- a/drivers/gpu/drm/i915/i915_vma.h +++ b/drivers/gpu/drm/i915/i915_vma.h @@ -119,6 +119,8 @@ struct i915_vma { /** This vma's place in the eviction list */ struct list_head evict_link; + struct list_head closed_link; + /** * Used for performing relocations during execbuffer insertion. */ @@ -285,6 +287,8 @@ void i915_vma_revoke_mmap(struct i915_vma *vma); 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); int __i915_vma_do_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags); @@ -408,6 +412,8 @@ i915_vma_unpin_fence(struct i915_vma *vma) __i915_vma_unpin_fence(vma); } +void i915_vma_parked(struct drm_i915_private *i915); + #define for_each_until(cond) if (cond) break; else /** diff --git a/drivers/gpu/drm/i915/selftests/huge_pages.c b/drivers/gpu/drm/i915/selftests/huge_pages.c index 05bbef363fff..d7c8ef8e6764 100644 --- a/drivers/gpu/drm/i915/selftests/huge_pages.c +++ b/drivers/gpu/drm/i915/selftests/huge_pages.c @@ -1091,7 +1091,7 @@ static int __igt_write_huge(struct i915_gem_context *ctx, out_vma_unpin: i915_vma_unpin(vma); out_vma_close: - i915_vma_close(vma); + i915_vma_destroy(vma); return err; } diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c index a662c0450e77..4b6622c6986a 100644 --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c @@ -226,6 +226,7 @@ struct drm_i915_private *mock_gem_device(void) INIT_LIST_HEAD(&i915->gt.timelines); INIT_LIST_HEAD(&i915->gt.active_rings); + INIT_LIST_HEAD(&i915->gt.closed_vma); mutex_lock(&i915->drm.struct_mutex); mock_init_ggtt(i915);
When userspace is passing around swapbuffers using DRI, we frequently have to open and close the same object in the foreign address space. This shows itself as the same object being rebound at roughly 30fps (with a second object also being rebound at 30fps), which involves us having to rewrite the page tables and maintain the drm_mm range manager every time. However, since the object still exists and it is only the local handle that disappears, if we are lazy and do not unbind the VMA immediately when the local user closes the object but defer it until the GPU is idle, then we can reuse the same VMA binding. We still have to be careful to mark the handle and lookup tables as closed to maintain the uABI, just allowing the underlying VMA to be resurrected if the user is able to access the same object from the same context again. If the object itself is destroyed (neither userspace keeping a handle to it), the VMA will be reaped immediately as usual. In the future, this will be even more useful as instantiating a new VMA for use on the GPU will become heavier. A nuisance indeed, so nip it in the bud. v2: s/__i915_vma_final_close/i915_vma_destroy/ etc. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 4 +- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +- drivers/gpu/drm/i915/i915_gem_gtt.c | 14 +++-- drivers/gpu/drm/i915/i915_vma.c | 61 +++++++++++++------ drivers/gpu/drm/i915/i915_vma.h | 6 ++ drivers/gpu/drm/i915/selftests/huge_pages.c | 2 +- .../gpu/drm/i915/selftests/mock_gem_device.c | 1 + 8 files changed, 67 insertions(+), 25 deletions(-)