[Intel-gfx] [PATCH 42/66] drm/i915: Clean up VMAs before freeing

[Ben Widawsky]

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 at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ben Widawsky, Intel Open Source Technology Center