| Message ID | 1372375867-1003-43-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 27, 2013 at 04:30:43PM -0700, Ben Widawsky wrote: > It's quite common for an object to simply be on the inactive list (and > not unbound) when we want to free the context. This of course happens > with lazy unbinding. Simply, this is needed when an object isn't fully > unbound but we want to free one VMA of the object, for whatever reason. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem.c | 28 ++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_gem_context.c | 1 + > 3 files changed, 30 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 0bc4251..9febcdd 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1674,6 +1674,7 @@ void i915_gem_free_object(struct drm_gem_object *obj); > struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj, > struct i915_address_space *vm); > void i915_gem_vma_destroy(struct i915_vma *vma); > +void i915_gem_vma_cleanup(struct i915_address_space *vm); > > int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj, > struct i915_address_space *vm, > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 12d0e61..9abc3c8 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4134,6 +4134,34 @@ void i915_gem_vma_destroy(struct i915_vma *vma) > kfree(vma); > } > > +/* This is like unbind() but without gtt considerations */ > +void i915_gem_vma_cleanup(struct i915_address_space *vm) > +{ > + struct drm_i915_private *dev_priv = vm->dev->dev_private; > + struct i915_vma *vma, *n; > + > + BUG_ON(is_i915_ggtt(vm)); > + WARN_ON(!list_empty(&vm->active_list)); > + > + list_for_each_entry_safe(vma, n, &vm->vma_list, per_vm_link) { > + struct drm_i915_gem_object *obj = vma->obj; > + > + if (WARN_ON(!i915_gem_obj_bound(obj, vm))) > + continue; > + > + i915_gem_object_unpin_pages(obj); > + > + list_del(&vma->mm_list); > + list_del(&vma->vma_link); > + drm_mm_remove_node(&vma->node); > + i915_gem_vma_destroy(vma); Is there a good reason why all of that stuff isn't included in i915_gem_vma_destroy()? It seems like it should be there. > + > + if (list_empty(&obj->vma_list)) > + list_move_tail(&obj->global_list, > + &dev_priv->mm.unbound_list); > + } > +} > + > int > i915_gem_idle(struct drm_device *dev) > { > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 988123f..c45cd5c 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -129,6 +129,7 @@ void i915_gem_context_free(struct kref *ctx_ref) > struct i915_hw_context *ctx = container_of(ctx_ref, > typeof(*ctx), ref); > > + i915_gem_vma_cleanup(&ctx->ppgtt.base); > if (ctx->ppgtt.cleanup) > ctx->ppgtt.cleanup(&ctx->ppgtt); > drm_gem_object_unreference(&ctx->obj->base); > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jul 02, 2013 at 01:59:17PM +0300, Ville Syrjälä wrote: > On Thu, Jun 27, 2013 at 04:30:43PM -0700, Ben Widawsky wrote: > > It's quite common for an object to simply be on the inactive list (and > > not unbound) when we want to free the context. This of course happens > > with lazy unbinding. Simply, this is needed when an object isn't fully > > unbound but we want to free one VMA of the object, for whatever reason. > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/i915_gem.c | 28 ++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/i915_gem_context.c | 1 + > > 3 files changed, 30 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 0bc4251..9febcdd 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1674,6 +1674,7 @@ void i915_gem_free_object(struct drm_gem_object *obj); > > struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj, > > struct i915_address_space *vm); > > void i915_gem_vma_destroy(struct i915_vma *vma); > > +void i915_gem_vma_cleanup(struct i915_address_space *vm); > > > > int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj, > > struct i915_address_space *vm, > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 12d0e61..9abc3c8 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -4134,6 +4134,34 @@ void i915_gem_vma_destroy(struct i915_vma *vma) > > kfree(vma); > > } > > > > +/* This is like unbind() but without gtt considerations */ > > +void i915_gem_vma_cleanup(struct i915_address_space *vm) > > +{ > > + struct drm_i915_private *dev_priv = vm->dev->dev_private; > > + struct i915_vma *vma, *n; > > + > > + BUG_ON(is_i915_ggtt(vm)); > > + WARN_ON(!list_empty(&vm->active_list)); > > + > > + list_for_each_entry_safe(vma, n, &vm->vma_list, per_vm_link) { > > + struct drm_i915_gem_object *obj = vma->obj; > > + > > + if (WARN_ON(!i915_gem_obj_bound(obj, vm))) > > + continue; > > + > > + i915_gem_object_unpin_pages(obj); > > + > > + list_del(&vma->mm_list); > > + list_del(&vma->vma_link); > > + drm_mm_remove_node(&vma->node); > > + i915_gem_vma_destroy(vma); > > Is there a good reason why all of that stuff isn't included in > i915_gem_vma_destroy()? It seems like it should be there. No good reason, just sloppy. The remove node ideally should only happen conditionally (ie. at unbind), but the list_del stuff should go in destroy. > > > + > > + if (list_empty(&obj->vma_list)) > > + list_move_tail(&obj->global_list, > > + &dev_priv->mm.unbound_list); > > + } > > +} > > + > > int > > i915_gem_idle(struct drm_device *dev) > > { > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > > index 988123f..c45cd5c 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -129,6 +129,7 @@ void i915_gem_context_free(struct kref *ctx_ref) > > struct i915_hw_context *ctx = container_of(ctx_ref, > > typeof(*ctx), ref); > > > > + i915_gem_vma_cleanup(&ctx->ppgtt.base); > > if (ctx->ppgtt.cleanup) > > ctx->ppgtt.cleanup(&ctx->ppgtt); > > drm_gem_object_unreference(&ctx->obj->base); > > -- > > 1.8.3.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 0bc4251..9febcdd 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1674,6 +1674,7 @@ void i915_gem_free_object(struct drm_gem_object *obj); struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj, struct i915_address_space *vm); void i915_gem_vma_destroy(struct i915_vma *vma); +void i915_gem_vma_cleanup(struct i915_address_space *vm); int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj, struct i915_address_space *vm, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 12d0e61..9abc3c8 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4134,6 +4134,34 @@ void i915_gem_vma_destroy(struct i915_vma *vma) kfree(vma); } +/* This is like unbind() but without gtt considerations */ +void i915_gem_vma_cleanup(struct i915_address_space *vm) +{ + struct drm_i915_private *dev_priv = vm->dev->dev_private; + struct i915_vma *vma, *n; + + BUG_ON(is_i915_ggtt(vm)); + WARN_ON(!list_empty(&vm->active_list)); + + list_for_each_entry_safe(vma, n, &vm->vma_list, per_vm_link) { + struct drm_i915_gem_object *obj = vma->obj; + + if (WARN_ON(!i915_gem_obj_bound(obj, vm))) + continue; + + i915_gem_object_unpin_pages(obj); + + list_del(&vma->mm_list); + list_del(&vma->vma_link); + drm_mm_remove_node(&vma->node); + i915_gem_vma_destroy(vma); + + if (list_empty(&obj->vma_list)) + list_move_tail(&obj->global_list, + &dev_priv->mm.unbound_list); + } +} + int i915_gem_idle(struct drm_device *dev) { diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 988123f..c45cd5c 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -129,6 +129,7 @@ void i915_gem_context_free(struct kref *ctx_ref) struct i915_hw_context *ctx = container_of(ctx_ref, typeof(*ctx), ref); + i915_gem_vma_cleanup(&ctx->ppgtt.base); if (ctx->ppgtt.cleanup) ctx->ppgtt.cleanup(&ctx->ppgtt); drm_gem_object_unreference(&ctx->obj->base);
It's quite common for an object to simply be on the inactive list (and not unbound) when we want to free the context. This of course happens with lazy unbinding. Simply, this is needed when an object isn't fully unbound but we want to free one VMA of the object, for whatever reason. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 28 ++++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_gem_context.c | 1 + 3 files changed, 30 insertions(+)