| Message ID | 1450093012-14955-10-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 14/12/15 11:36, Chris Wilson wrote: > When the user closes the context mark it and the dependent address space > as closed. As we use an asynchronous destruct method, this has two purposes. > First it allows us to flag the closed context and detect internal errors if > we to create any new objects for it (as it is removed from the user's > namespace, these should be internal bugs only). And secondly, it allows > us to immediately reap stale vma. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 ++++ > drivers/gpu/drm/i915/i915_gem.c | 15 ++++++++----- > drivers/gpu/drm/i915/i915_gem_context.c | 39 +++++++++++++++++++++++++++++---- > drivers/gpu/drm/i915/i915_gem_gtt.c | 11 +++++++--- > drivers/gpu/drm/i915/i915_gem_gtt.h | 9 ++++++++ > drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +- > 6 files changed, 66 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 696469a06715..66ecd6b3df95 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -892,6 +892,8 @@ struct intel_context { > } engine[I915_NUM_RINGS]; > > struct list_head link; > + > + bool closed:1; > }; > > enum fb_op_origin { > @@ -2720,6 +2722,8 @@ int __must_check i915_vma_unbind(struct i915_vma *vma); > * _guarantee_ VMA in question is _not in use_ anywhere. > */ > int __must_check __i915_vma_unbind_no_wait(struct i915_vma *vma); > +void i915_vma_close(struct i915_vma *vma); > + > int i915_gem_object_put_pages(struct drm_i915_gem_object *obj); > void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv); > void i915_gem_release_mmap(struct drm_i915_gem_object *obj); > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 7c13c27a6470..08ea0b7eda8b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2367,12 +2367,13 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj) > return 0; > } > > -static void i915_vma_close(struct i915_vma *vma) Can't find this in this series? > +void i915_vma_close(struct i915_vma *vma) > { > RQ_BUG_ON(vma->closed); > vma->closed = true; > > list_del_init(&vma->obj_link); > + list_del_init(&vma->vm_link); > if (!vma->active) > WARN_ON(i915_vma_unbind(vma)); > } > @@ -2620,12 +2621,13 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) > return ret; > } > > - trace_i915_vma_unbind(vma); > - > - vma->vm->unbind_vma(vma); > + if (likely(!vma->vm->closed)) { > + trace_i915_vma_unbind(vma); > + vma->vm->unbind_vma(vma); > + } > vma->bound = 0; > > - list_del_init(&vma->vm_link); > + list_move_tail(&vma->vm_link, &vma->vm->unbound_list); > if (vma->is_ggtt) { > if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { > obj->map_and_fenceable = false; > @@ -2882,7 +2884,7 @@ search_free: > goto err_remove_node; > > list_move_tail(&obj->global_list, &dev_priv->mm.bound_list); > - list_add_tail(&vma->vm_link, &vm->inactive_list); > + list_move_tail(&vma->vm_link, &vm->inactive_list); > > return vma; > > @@ -3890,6 +3892,7 @@ void i915_gem_vma_destroy(struct i915_vma *vma) > if (!list_empty(&vma->exec_list)) > return; > > + list_del(&vma->vm_link); > if (!vma->is_ggtt) > i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm)); > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index c4a8a64cd1b2..9669547c7c2d 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -153,6 +153,7 @@ void i915_gem_context_free(struct kref *ctx_ref) > struct intel_context *ctx = container_of(ctx_ref, typeof(*ctx), ref); > > trace_i915_context_free(ctx); > + RQ_BUG_ON(!ctx->closed); Normal BUG_ON I think. > > if (i915.enable_execlists) > intel_lr_context_free(ctx); > @@ -209,6 +210,36 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size) > return obj; > } > > +static void i915_ppgtt_close(struct i915_address_space *vm) > +{ > + struct list_head *phases[] = { > + &vm->active_list, > + &vm->inactive_list, > + &vm->unbound_list, > + NULL, > + }, **phase; > + > + RQ_BUG_ON(vm->is_ggtt); > + RQ_BUG_ON(vm->closed); More, and elsewhere. > + vm->closed = true; > + > + for (phase = phases; *phase; phase++) { > + struct i915_vma *vma, *vn; > + > + list_for_each_entry_safe(vma, vn, *phase, vm_link) > + i915_vma_close(vma); Can't really carry on since I don't see the implementation of this. Does it wait for retirement? > + } > +} > + > +static void context_close(struct intel_context *ctx) > +{ > + RQ_BUG_ON(ctx->closed); > + ctx->closed = true; > + if (ctx->ppgtt) > + i915_ppgtt_close(&ctx->ppgtt->base); > + i915_gem_context_unreference(ctx); > +} > + > static struct intel_context * > __create_hw_context(struct drm_device *dev, > struct drm_i915_file_private *file_priv) > @@ -256,7 +287,7 @@ __create_hw_context(struct drm_device *dev, > return ctx; > > err_out: > - i915_gem_context_unreference(ctx); > + context_close(ctx); > return ERR_PTR(ret); > } > > @@ -318,7 +349,7 @@ err_unpin: > i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); > err_destroy: > idr_remove(&file_priv->context_idr, ctx->user_handle); > - i915_gem_context_unreference(ctx); > + context_close(ctx); > return ERR_PTR(ret); > } > > @@ -470,7 +501,7 @@ static int context_idr_cleanup(int id, void *p, void *data) > { > struct intel_context *ctx = p; > > - i915_gem_context_unreference(ctx); > + context_close(ctx); > return 0; > } > > @@ -894,7 +925,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, > } > > idr_remove(&ctx->file_priv->context_idr, ctx->user_handle); > - i915_gem_context_unreference(ctx); > + context_close(ctx); > mutex_unlock(&dev->struct_mutex); > > DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id); > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 9f594c33bd0a..354236d72432 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -2130,6 +2130,7 @@ static void i915_address_space_init(struct i915_address_space *vm, > vm->dev = dev_priv->dev; > INIT_LIST_HEAD(&vm->active_list); > INIT_LIST_HEAD(&vm->inactive_list); > + INIT_LIST_HEAD(&vm->unbound_list); > list_add_tail(&vm->global_link, &dev_priv->vm_list); > } > > @@ -2214,9 +2215,10 @@ void i915_ppgtt_release(struct kref *kref) > > trace_i915_ppgtt_release(&ppgtt->base); > > - /* vmas should already be unbound */ > + /* vmas should already be unbound and destroyed */ > WARN_ON(!list_empty(&ppgtt->base.active_list)); > WARN_ON(!list_empty(&ppgtt->base.inactive_list)); > + WARN_ON(!list_empty(&ppgtt->base.unbound_list)); > > list_del(&ppgtt->base.global_link); > drm_mm_takedown(&ppgtt->base.mm); > @@ -2698,7 +2700,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev, > return ret; > } > vma->bound |= GLOBAL_BIND; > - list_add_tail(&vma->vm_link, &ggtt_vm->inactive_list); > + list_move_tail(&vma->vm_link, &ggtt_vm->inactive_list); > } > > /* Clear any non-preallocated blocks */ > @@ -3252,6 +3254,8 @@ __i915_gem_vma_create(struct drm_i915_gem_object *obj, > struct i915_vma *vma; > int i; > > + RQ_BUG_ON(vm->closed); > + > if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view)) > return ERR_PTR(-EINVAL); > > @@ -3259,11 +3263,11 @@ __i915_gem_vma_create(struct drm_i915_gem_object *obj, > if (vma == NULL) > return ERR_PTR(-ENOMEM); > > - INIT_LIST_HEAD(&vma->vm_link); > INIT_LIST_HEAD(&vma->obj_link); > INIT_LIST_HEAD(&vma->exec_list); > for (i = 0; i < ARRAY_SIZE(vma->last_read); i++) > init_request_active(&vma->last_read[i], i915_vma_retire); > + list_add(&vma->vm_link, &vm->unbound_list); > vma->vm = vm; > vma->obj = obj; > vma->is_ggtt = i915_is_ggtt(vm); > @@ -3310,6 +3314,7 @@ i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj, > if (!vma) > vma = __i915_gem_vma_create(obj, ggtt, view); > > + RQ_BUG_ON(vma->closed); > return vma; > > } > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index be7e8526b219..3bd2a4f4990c 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -284,6 +284,8 @@ struct i915_address_space { > u64 start; /* Start offset always 0 for dri2 */ > u64 total; /* size addr space maps (ex. 2GB for ggtt) */ > > + bool closed; > + > struct i915_page_scratch *scratch_page; > struct i915_page_table *scratch_pt; > struct i915_page_directory *scratch_pd; > @@ -312,6 +314,13 @@ struct i915_address_space { > */ > struct list_head inactive_list; > > + /** > + * List of vma that have been unbound. > + * > + * A reference is not held on the buffer while on this list. s/buffer/object/ > + */ > + struct list_head unbound_list; > + > /* FIXME: Need a more generic return type */ > gen6_pte_t (*pte_encode)(dma_addr_t addr, > enum i915_cache_level level, > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 305d58a5f745..4a803311f5bf 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -688,7 +688,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, > } > > vma->bound |= GLOBAL_BIND; > - list_add_tail(&vma->vm_link, &ggtt->inactive_list); > + list_move_tail(&vma->vm_link, &ggtt->inactive_list); > } > > list_add_tail(&obj->global_list, &dev_priv->mm.bound_list); > Regards, Tvrtko
On 17/12/15 12:37, Tvrtko Ursulin wrote: > > Hi, > > On 14/12/15 11:36, Chris Wilson wrote: >> When the user closes the context mark it and the dependent address space >> as closed. As we use an asynchronous destruct method, this has two >> purposes. >> First it allows us to flag the closed context and detect internal >> errors if >> we to create any new objects for it (as it is removed from the user's >> namespace, these should be internal bugs only). And secondly, it allows >> us to immediately reap stale vma. >> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 4 ++++ >> drivers/gpu/drm/i915/i915_gem.c | 15 ++++++++----- >> drivers/gpu/drm/i915/i915_gem_context.c | 39 >> +++++++++++++++++++++++++++++---- >> drivers/gpu/drm/i915/i915_gem_gtt.c | 11 +++++++--- >> drivers/gpu/drm/i915/i915_gem_gtt.h | 9 ++++++++ >> drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +- >> 6 files changed, 66 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index 696469a06715..66ecd6b3df95 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -892,6 +892,8 @@ struct intel_context { >> } engine[I915_NUM_RINGS]; >> >> struct list_head link; >> + >> + bool closed:1; >> }; >> >> enum fb_op_origin { >> @@ -2720,6 +2722,8 @@ int __must_check i915_vma_unbind(struct i915_vma >> *vma); >> * _guarantee_ VMA in question is _not in use_ anywhere. >> */ >> int __must_check __i915_vma_unbind_no_wait(struct i915_vma *vma); >> +void i915_vma_close(struct i915_vma *vma); >> + >> int i915_gem_object_put_pages(struct drm_i915_gem_object *obj); >> void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv); >> void i915_gem_release_mmap(struct drm_i915_gem_object *obj); >> diff --git a/drivers/gpu/drm/i915/i915_gem.c >> b/drivers/gpu/drm/i915/i915_gem.c >> index 7c13c27a6470..08ea0b7eda8b 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -2367,12 +2367,13 @@ i915_gem_object_flush_active(struct >> drm_i915_gem_object *obj) >> return 0; >> } >> >> -static void i915_vma_close(struct i915_vma *vma) > > Can't find this in this series? Ooops it is in a different mail folder, I did not sport the break in patch numbers... Regards, Tvrtko
On Thu, Dec 17, 2015 at 12:37:13PM +0000, Tvrtko Ursulin wrote: > >-static void i915_vma_close(struct i915_vma *vma) > > Can't find this in this series? Should be the previous patch (9/11: Release vma when the handle is closed) that hooks in gem_object_close to mark each vma as closed if it is owned by the file. http://patchwork.freedesktop.org/patch/68086/ > >diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > >index c4a8a64cd1b2..9669547c7c2d 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_context.c > >+++ b/drivers/gpu/drm/i915/i915_gem_context.c > >@@ -153,6 +153,7 @@ void i915_gem_context_free(struct kref *ctx_ref) > > struct intel_context *ctx = container_of(ctx_ref, typeof(*ctx), ref); > > > > trace_i915_context_free(ctx); > >+ RQ_BUG_ON(!ctx->closed); > > Normal BUG_ON I think. You want a BUG_ON! :-p Just enable them. > >+ for (phase = phases; *phase; phase++) { > >+ struct i915_vma *vma, *vn; > >+ > >+ list_for_each_entry_safe(vma, vn, *phase, vm_link) > >+ i915_vma_close(vma); > > Can't really carry on since I don't see the implementation of this. > > Does it wait for retirement? No. i915_vma_close() uses vma tracking to defer the unbind until idle. > >+ /** > >+ * List of vma that have been unbound. > >+ * > >+ * A reference is not held on the buffer while on this list. > > s/buffer/object/ They are buffer objects! The comment was cut'n'paste. I don't think it is entirely apt to be talking about the object level active reference here anyway. But I didn't feel inclined to write something that was even more confusing. -Chris
On 17/12/15 12:48, Chris Wilson wrote: > On Thu, Dec 17, 2015 at 12:37:13PM +0000, Tvrtko Ursulin wrote: >>> -static void i915_vma_close(struct i915_vma *vma) >> >> Can't find this in this series? > > Should be the previous patch (9/11: Release vma when the handle is > closed) that hooks in gem_object_close to mark each vma as closed if it > is owned by the file. > > http://patchwork.freedesktop.org/patch/68086/ Yeah found it later, mail filtering split the thread to different folders. >>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c >>> index c4a8a64cd1b2..9669547c7c2d 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem_context.c >>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >>> @@ -153,6 +153,7 @@ void i915_gem_context_free(struct kref *ctx_ref) >>> struct intel_context *ctx = container_of(ctx_ref, typeof(*ctx), ref); >>> >>> trace_i915_context_free(ctx); >>> + RQ_BUG_ON(!ctx->closed); >> >> Normal BUG_ON I think. > > You want a BUG_ON! :-p Just enable them. No I just gave up on you seeing the light! :> Point is RQ_BUG_ON is behind CONFIG_DRM_I915_DEBUG_REQUESTS so it is wrong to use it for other things. >>> + for (phase = phases; *phase; phase++) { >>> + struct i915_vma *vma, *vn; >>> + >>> + list_for_each_entry_safe(vma, vn, *phase, vm_link) >>> + i915_vma_close(vma); >> >> Can't really carry on since I don't see the implementation of this. >> >> Does it wait for retirement? > > No. i915_vma_close() uses vma tracking to defer the unbind until idle. > >>> + /** >>> + * List of vma that have been unbound. >>> + * >>> + * A reference is not held on the buffer while on this list. >> >> s/buffer/object/ > > They are buffer objects! The comment was cut'n'paste. I don't think it > is entirely apt to be talking about the object level active reference > here anyway. But I didn't feel inclined to write something that was even > more confusing. # grep buffer i915_gem.c | grep -v ring | grep -v front | grep -v batch | grep -v execbuffer | grep -v scanout | wc -l 8 Ok, there is some precedence for the term. Tvrtko
On 14/12/15 11:36, Chris Wilson wrote: > When the user closes the context mark it and the dependent address space > as closed. As we use an asynchronous destruct method, this has two purposes. > First it allows us to flag the closed context and detect internal errors if > we to create any new objects for it (as it is removed from the user's > namespace, these should be internal bugs only). And secondly, it allows > us to immediately reap stale vma. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 ++++ > drivers/gpu/drm/i915/i915_gem.c | 15 ++++++++----- > drivers/gpu/drm/i915/i915_gem_context.c | 39 +++++++++++++++++++++++++++++---- > drivers/gpu/drm/i915/i915_gem_gtt.c | 11 +++++++--- > drivers/gpu/drm/i915/i915_gem_gtt.h | 9 ++++++++ > drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +- > 6 files changed, 66 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 696469a06715..66ecd6b3df95 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -892,6 +892,8 @@ struct intel_context { > } engine[I915_NUM_RINGS]; > > struct list_head link; > + > + bool closed:1; > }; > > enum fb_op_origin { > @@ -2720,6 +2722,8 @@ int __must_check i915_vma_unbind(struct i915_vma *vma); > * _guarantee_ VMA in question is _not in use_ anywhere. > */ > int __must_check __i915_vma_unbind_no_wait(struct i915_vma *vma); > +void i915_vma_close(struct i915_vma *vma); > + > int i915_gem_object_put_pages(struct drm_i915_gem_object *obj); > void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv); > void i915_gem_release_mmap(struct drm_i915_gem_object *obj); > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 7c13c27a6470..08ea0b7eda8b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2367,12 +2367,13 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj) > return 0; > } > > -static void i915_vma_close(struct i915_vma *vma) > +void i915_vma_close(struct i915_vma *vma) > { > RQ_BUG_ON(vma->closed); > vma->closed = true; > > list_del_init(&vma->obj_link); > + list_del_init(&vma->vm_link); > if (!vma->active) > WARN_ON(i915_vma_unbind(vma)); > } > @@ -2620,12 +2621,13 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) > return ret; > } > > - trace_i915_vma_unbind(vma); > - > - vma->vm->unbind_vma(vma); > + if (likely(!vma->vm->closed)) { > + trace_i915_vma_unbind(vma); > + vma->vm->unbind_vma(vma); > + } > vma->bound = 0; > > - list_del_init(&vma->vm_link); > + list_move_tail(&vma->vm_link, &vma->vm->unbound_list); > if (vma->is_ggtt) { > if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { > obj->map_and_fenceable = false; > @@ -2882,7 +2884,7 @@ search_free: > goto err_remove_node; > > list_move_tail(&obj->global_list, &dev_priv->mm.bound_list); > - list_add_tail(&vma->vm_link, &vm->inactive_list); > + list_move_tail(&vma->vm_link, &vm->inactive_list); > > return vma; > > @@ -3890,6 +3892,7 @@ void i915_gem_vma_destroy(struct i915_vma *vma) > if (!list_empty(&vma->exec_list)) > return; > > + list_del(&vma->vm_link); > if (!vma->is_ggtt) > i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm)); > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index c4a8a64cd1b2..9669547c7c2d 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -153,6 +153,7 @@ void i915_gem_context_free(struct kref *ctx_ref) > struct intel_context *ctx = container_of(ctx_ref, typeof(*ctx), ref); > > trace_i915_context_free(ctx); > + RQ_BUG_ON(!ctx->closed); > > if (i915.enable_execlists) > intel_lr_context_free(ctx); > @@ -209,6 +210,36 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size) > return obj; > } > > +static void i915_ppgtt_close(struct i915_address_space *vm) > +{ > + struct list_head *phases[] = { > + &vm->active_list, > + &vm->inactive_list, > + &vm->unbound_list, > + NULL, > + }, **phase; > + > + RQ_BUG_ON(vm->is_ggtt); > + RQ_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) > + i915_vma_close(vma); > + } > +} Hm so VMAs get unlinked from everywhere, but then on retire goes back to inactive. Is it not a bit weird? Why it is needed to unlink VMAs from everywhere when marking them as closed? And actually on retire objects are ahead of VMAs in the req->active_list so the last object unreference happens before the last vma is retired, which is even weirder. Am I missing something? Regards, Tvrtko
On Thu, Dec 17, 2015 at 02:15:52PM +0000, Tvrtko Ursulin wrote: > >+static void i915_ppgtt_close(struct i915_address_space *vm) > >+{ > >+ struct list_head *phases[] = { > >+ &vm->active_list, > >+ &vm->inactive_list, > >+ &vm->unbound_list, > >+ NULL, > >+ }, **phase; > >+ > >+ RQ_BUG_ON(vm->is_ggtt); > >+ RQ_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) > >+ i915_vma_close(vma); > >+ } > >+} > > Hm so VMAs get unlinked from everywhere, but then on retire goes > back to inactive. Is it not a bit weird? Very weird. In the end, I have to stop unlinking in i915_vma_close() from the vm lists. > Why it is needed to unlink VMAs from everywhere when marking them as closed? Indeed, it was just to try and keep this walk short. But I realised that this would actually also foul up the evict/shrinker (by hiding objects from them that should be thrown away). > And actually on retire objects are ahead of VMAs in the > req->active_list so the last object unreference happens before the > last vma is retired, which is even weirder. > > Am I missing something? That shouldn't happen. The i915_gem_object_retire_read is run after the i915_vma_retire. I had added some commentary to i915_vma_move_to_active() that hopefully explains the interdependences between retire callbacks (mostly to try and prevent breakage later). @@ -1075,7 +1075,13 @@ void i915_vma_move_to_active(struct i915_vma *vma, obj->dirty = 1; /* be paranoid */ - /* Add a reference if we're newly entering the active list. */ + /* Add a reference if we're newly entering the active list. + * The order in which we add operations to the retirement queue is + * vital here: mark_active adds to the start of the callback list, + * such that subsequent callbacks are called first. Therefore we + * add the active reference first and queue for it to be dropped + * *last*. + */ -Chris
On 17/12/15 14:26, Chris Wilson wrote: > On Thu, Dec 17, 2015 at 02:15:52PM +0000, Tvrtko Ursulin wrote: >>> +static void i915_ppgtt_close(struct i915_address_space *vm) >>> +{ >>> + struct list_head *phases[] = { >>> + &vm->active_list, >>> + &vm->inactive_list, >>> + &vm->unbound_list, >>> + NULL, >>> + }, **phase; >>> + >>> + RQ_BUG_ON(vm->is_ggtt); >>> + RQ_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) >>> + i915_vma_close(vma); >>> + } >>> +} >> >> Hm so VMAs get unlinked from everywhere, but then on retire goes >> back to inactive. Is it not a bit weird? > > Very weird. In the end, I have to stop unlinking in i915_vma_close() > from the vm lists. > >> Why it is needed to unlink VMAs from everywhere when marking them as closed? > > Indeed, it was just to try and keep this walk short. But I realised that > this would actually also foul up the evict/shrinker (by hiding objects > from them that should be thrown away). > >> And actually on retire objects are ahead of VMAs in the >> req->active_list so the last object unreference happens before the >> last vma is retired, which is even weirder. >> >> Am I missing something? > > That shouldn't happen. The i915_gem_object_retire_read is run after the > i915_vma_retire. > > I had added some commentary to i915_vma_move_to_active() that hopefully > explains the interdependences between retire callbacks (mostly to try > and prevent breakage later). > > @@ -1075,7 +1075,13 @@ void i915_vma_move_to_active(struct i915_vma *vma, > > obj->dirty = 1; /* be paranoid */ > > - /* Add a reference if we're newly entering the active list. */ > + /* Add a reference if we're newly entering the active list. > + * The order in which we add operations to the retirement queue is > + * vital here: mark_active adds to the start of the callback list, > + * such that subsequent callbacks are called first. Therefore we > + * add the active reference first and queue for it to be dropped > + * *last*. > + */ I don't know how I concluded active VMA is after the active object, and I specifically saw the order and list_move. Again, very good to document that, so something good at least came out of it. :) Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 696469a06715..66ecd6b3df95 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -892,6 +892,8 @@ struct intel_context { } engine[I915_NUM_RINGS]; struct list_head link; + + bool closed:1; }; enum fb_op_origin { @@ -2720,6 +2722,8 @@ int __must_check i915_vma_unbind(struct i915_vma *vma); * _guarantee_ VMA in question is _not in use_ anywhere. */ int __must_check __i915_vma_unbind_no_wait(struct i915_vma *vma); +void i915_vma_close(struct i915_vma *vma); + int i915_gem_object_put_pages(struct drm_i915_gem_object *obj); void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv); void i915_gem_release_mmap(struct drm_i915_gem_object *obj); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 7c13c27a6470..08ea0b7eda8b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2367,12 +2367,13 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj) return 0; } -static void i915_vma_close(struct i915_vma *vma) +void i915_vma_close(struct i915_vma *vma) { RQ_BUG_ON(vma->closed); vma->closed = true; list_del_init(&vma->obj_link); + list_del_init(&vma->vm_link); if (!vma->active) WARN_ON(i915_vma_unbind(vma)); } @@ -2620,12 +2621,13 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) return ret; } - trace_i915_vma_unbind(vma); - - vma->vm->unbind_vma(vma); + if (likely(!vma->vm->closed)) { + trace_i915_vma_unbind(vma); + vma->vm->unbind_vma(vma); + } vma->bound = 0; - list_del_init(&vma->vm_link); + list_move_tail(&vma->vm_link, &vma->vm->unbound_list); if (vma->is_ggtt) { if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { obj->map_and_fenceable = false; @@ -2882,7 +2884,7 @@ search_free: goto err_remove_node; list_move_tail(&obj->global_list, &dev_priv->mm.bound_list); - list_add_tail(&vma->vm_link, &vm->inactive_list); + list_move_tail(&vma->vm_link, &vm->inactive_list); return vma; @@ -3890,6 +3892,7 @@ void i915_gem_vma_destroy(struct i915_vma *vma) if (!list_empty(&vma->exec_list)) return; + list_del(&vma->vm_link); if (!vma->is_ggtt) i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm)); diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index c4a8a64cd1b2..9669547c7c2d 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -153,6 +153,7 @@ void i915_gem_context_free(struct kref *ctx_ref) struct intel_context *ctx = container_of(ctx_ref, typeof(*ctx), ref); trace_i915_context_free(ctx); + RQ_BUG_ON(!ctx->closed); if (i915.enable_execlists) intel_lr_context_free(ctx); @@ -209,6 +210,36 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size) return obj; } +static void i915_ppgtt_close(struct i915_address_space *vm) +{ + struct list_head *phases[] = { + &vm->active_list, + &vm->inactive_list, + &vm->unbound_list, + NULL, + }, **phase; + + RQ_BUG_ON(vm->is_ggtt); + RQ_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) + i915_vma_close(vma); + } +} + +static void context_close(struct intel_context *ctx) +{ + RQ_BUG_ON(ctx->closed); + ctx->closed = true; + if (ctx->ppgtt) + i915_ppgtt_close(&ctx->ppgtt->base); + i915_gem_context_unreference(ctx); +} + static struct intel_context * __create_hw_context(struct drm_device *dev, struct drm_i915_file_private *file_priv) @@ -256,7 +287,7 @@ __create_hw_context(struct drm_device *dev, return ctx; err_out: - i915_gem_context_unreference(ctx); + context_close(ctx); return ERR_PTR(ret); } @@ -318,7 +349,7 @@ err_unpin: i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); err_destroy: idr_remove(&file_priv->context_idr, ctx->user_handle); - i915_gem_context_unreference(ctx); + context_close(ctx); return ERR_PTR(ret); } @@ -470,7 +501,7 @@ static int context_idr_cleanup(int id, void *p, void *data) { struct intel_context *ctx = p; - i915_gem_context_unreference(ctx); + context_close(ctx); return 0; } @@ -894,7 +925,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, } idr_remove(&ctx->file_priv->context_idr, ctx->user_handle); - i915_gem_context_unreference(ctx); + context_close(ctx); mutex_unlock(&dev->struct_mutex); DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id); diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 9f594c33bd0a..354236d72432 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2130,6 +2130,7 @@ static void i915_address_space_init(struct i915_address_space *vm, vm->dev = dev_priv->dev; INIT_LIST_HEAD(&vm->active_list); INIT_LIST_HEAD(&vm->inactive_list); + INIT_LIST_HEAD(&vm->unbound_list); list_add_tail(&vm->global_link, &dev_priv->vm_list); } @@ -2214,9 +2215,10 @@ void i915_ppgtt_release(struct kref *kref) trace_i915_ppgtt_release(&ppgtt->base); - /* vmas should already be unbound */ + /* vmas should already be unbound and destroyed */ WARN_ON(!list_empty(&ppgtt->base.active_list)); WARN_ON(!list_empty(&ppgtt->base.inactive_list)); + WARN_ON(!list_empty(&ppgtt->base.unbound_list)); list_del(&ppgtt->base.global_link); drm_mm_takedown(&ppgtt->base.mm); @@ -2698,7 +2700,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev, return ret; } vma->bound |= GLOBAL_BIND; - list_add_tail(&vma->vm_link, &ggtt_vm->inactive_list); + list_move_tail(&vma->vm_link, &ggtt_vm->inactive_list); } /* Clear any non-preallocated blocks */ @@ -3252,6 +3254,8 @@ __i915_gem_vma_create(struct drm_i915_gem_object *obj, struct i915_vma *vma; int i; + RQ_BUG_ON(vm->closed); + if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view)) return ERR_PTR(-EINVAL); @@ -3259,11 +3263,11 @@ __i915_gem_vma_create(struct drm_i915_gem_object *obj, if (vma == NULL) return ERR_PTR(-ENOMEM); - INIT_LIST_HEAD(&vma->vm_link); INIT_LIST_HEAD(&vma->obj_link); INIT_LIST_HEAD(&vma->exec_list); for (i = 0; i < ARRAY_SIZE(vma->last_read); i++) init_request_active(&vma->last_read[i], i915_vma_retire); + list_add(&vma->vm_link, &vm->unbound_list); vma->vm = vm; vma->obj = obj; vma->is_ggtt = i915_is_ggtt(vm); @@ -3310,6 +3314,7 @@ i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj, if (!vma) vma = __i915_gem_vma_create(obj, ggtt, view); + RQ_BUG_ON(vma->closed); return vma; } diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index be7e8526b219..3bd2a4f4990c 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -284,6 +284,8 @@ struct i915_address_space { u64 start; /* Start offset always 0 for dri2 */ u64 total; /* size addr space maps (ex. 2GB for ggtt) */ + bool closed; + struct i915_page_scratch *scratch_page; struct i915_page_table *scratch_pt; struct i915_page_directory *scratch_pd; @@ -312,6 +314,13 @@ struct i915_address_space { */ struct list_head inactive_list; + /** + * List of vma that have been unbound. + * + * A reference is not held on the buffer while on this list. + */ + struct list_head unbound_list; + /* FIXME: Need a more generic return type */ gen6_pte_t (*pte_encode)(dma_addr_t addr, enum i915_cache_level level, diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 305d58a5f745..4a803311f5bf 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -688,7 +688,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, } vma->bound |= GLOBAL_BIND; - list_add_tail(&vma->vm_link, &ggtt->inactive_list); + list_move_tail(&vma->vm_link, &ggtt->inactive_list); } list_add_tail(&obj->global_list, &dev_priv->mm.bound_list);
When the user closes the context mark it and the dependent address space as closed. As we use an asynchronous destruct method, this has two purposes. First it allows us to flag the closed context and detect internal errors if we to create any new objects for it (as it is removed from the user's namespace, these should be internal bugs only). And secondly, it allows us to immediately reap stale vma. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.h | 4 ++++ drivers/gpu/drm/i915/i915_gem.c | 15 ++++++++----- drivers/gpu/drm/i915/i915_gem_context.c | 39 +++++++++++++++++++++++++++++---- drivers/gpu/drm/i915/i915_gem_gtt.c | 11 +++++++--- drivers/gpu/drm/i915/i915_gem_gtt.h | 9 ++++++++ drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +- 6 files changed, 66 insertions(+), 14 deletions(-)