[Intel-gfx] [PATCH 40/66] drm/i915: Track all VMAs per VM

[Ben Widawsky]

On Sun, Jun 30, 2013 at 05:35:00PM +0200, Daniel Vetter wrote:
> On Thu, Jun 27, 2013 at 04:30:41PM -0700, Ben Widawsky wrote:
> > This allows us to be aware of all the VMAs leftover and teardown, and is
> > useful for debug. I suspect it will prove even more useful later.
> > 
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 2 ++
> >  drivers/gpu/drm/i915/i915_gem.c | 4 ++++
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 247a124..0bc4251 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -446,6 +446,7 @@ struct i915_address_space {
> >  	struct drm_mm mm;
> >  	struct drm_device *dev;
> >  	struct list_head global_link;
> > +	struct list_head vma_list;
> 
> This one feels a bit unecessary. With the drm_mm_node embedded we already
> have a total of 4 lists:
> - The node_list in the drm_mm. There's even a for_each helper for it. This
>   lists nodes in ascending offset ordering. We only need to upcast from
>   the drm_mm_node to our vma, but due to embedded that's no problem.
> - The hole list in drm_mm. Again comes with a for_each helper included.
> - The inactive/active lists. Together they again list all vmas in a vm.
> 
> What's the new one doing that we need it so much?
> -Daniel
>

I can try to use the existing data structures to make it work. It was
really easy to do it with our own list though, a list which is really
easy to maintain, and not often traversed. So in all, I don't find the
additional list offensive. I guess a fair argument would be since we'll
have at least as many VMAs as BOs, the extra list_head is a bit
offensive. If you want to make that your tune, then I can agree with you
on that.

One reason I didn't try harder to not use this list was I felt it would
be a nice thing when we properly support page faults, though even there
I think the existing lists could probably be used.

Also, at one time, I was still use drm_mm_node *, so the upcast wasn't
possible.

> 
> >  	unsigned long start;		/* Start offset always 0 for dri2 */
> >  	size_t total;		/* size addr space maps (ex. 2GB for ggtt) */
> >  
> > @@ -556,6 +557,7 @@ struct i915_vma {
> >  	struct list_head mm_list;
> >  
> >  	struct list_head vma_link; /* Link in the object's VMA list */
> > +	struct list_head per_vm_link; /* Link in the VM's VMA list */
> >  };
> >  
> >  struct i915_ctx_hang_stats {
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index a3e8c26..5c0ad6a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4112,14 +4112,17 @@ struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
> >  
> >  	INIT_LIST_HEAD(&vma->vma_link);
> >  	INIT_LIST_HEAD(&vma->mm_list);
> > +	INIT_LIST_HEAD(&vma->per_vm_link);
> >  	vma->vm = vm;
> >  	vma->obj = obj;
> > +	list_add_tail(&vma->per_vm_link, &vm->vma_list);
> >  
> >  	return vma;
> >  }
> >  
> >  void i915_gem_vma_destroy(struct i915_vma *vma)
> >  {
> > +	list_del(&vma->per_vm_link);
> >  	WARN_ON(vma->node.allocated);
> >  	kfree(vma);
> >  }
> > @@ -4473,6 +4476,7 @@ static void i915_init_vm(struct drm_i915_private *dev_priv,
> >  	INIT_LIST_HEAD(&vm->active_list);
> >  	INIT_LIST_HEAD(&vm->inactive_list);
> >  	INIT_LIST_HEAD(&vm->global_link);
> > +	INIT_LIST_HEAD(&vm->vma_list);
> >  	list_add(&vm->global_link, &dev_priv->vm_list);
> >  }
> >  
> > -- 
> > 1.8.3.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ben Widawsky, Intel Open Source Technology Center