[Intel-gfx] [PATCH] drm/i915: vma/ppgtt lifetime rules

[Chris Wilson]

On Tue, Jul 29, 2014 at 01:06:40PM +0200, Daniel Vetter wrote:
> On Tue, Jul 29, 2014 at 11:08:05AM +0100, Michel Thierry wrote:
> > VMAs should take a reference of the address space they use.
> > 
> > Now, when the fd is closed, it will release the ref that the context was
> > holding, but it will still be referenced by any vmas that are still
> > active.
> > 
> > ppgtt_release() should then only be called when the last thing referencing
> > it releases the ref, and it can just call the base cleanup and free the
> > ppgtt.
> > 
> > Signed-off-by: Michel Thierry <michel.thierry at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         |  2 ++
> >  drivers/gpu/drm/i915/i915_gem.c         |  8 ++++++++
> >  drivers/gpu/drm/i915/i915_gem_context.c | 23 +++--------------------
> >  drivers/gpu/drm/i915/i915_gem_gtt.c     |  5 +++++
> >  4 files changed, 18 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2acc03f..a879a93 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2495,7 +2495,9 @@ void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
> >  
> >  /* i915_gem_context.c */
> >  #define ctx_to_ppgtt(ctx) container_of((ctx)->vm, struct i915_hw_ppgtt, base)
> > +#define vm_to_ppgtt(vm) container_of(vm, struct i915_hw_ppgtt, base)
> >  int __must_check i915_gem_context_init(struct drm_device *dev);
> > +void ppgtt_release(struct kref *kref);
> >  void i915_gem_context_fini(struct drm_device *dev);
> >  void i915_gem_context_reset(struct drm_device *dev);
> >  int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index dcd8d7b..25a32b9 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4499,12 +4499,20 @@ struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> >  
> >  void i915_gem_vma_destroy(struct i915_vma *vma)
> >  {
> > +	struct i915_address_space *vm = NULL;
> > +	struct i915_hw_ppgtt *ppgtt = NULL;
> >  	WARN_ON(vma->node.allocated);
> >  
> >  	/* Keep the vma as a placeholder in the execbuffer reservation lists */
> >  	if (!list_empty(&vma->exec_list))
> >  		return;
> >  
> > +	vm = vma->vm;
> > +	ppgtt = vm_to_ppgtt(vm);
> > +
> > +	if (ppgtt)
> > +		kref_put(&ppgtt->ref, ppgtt_release);
> 
> Hm, this has the risk that we leave unwanted vmas around for a bit longer.
> They will get cleaned up though when the final object references goes
> away, so the leak is fairly restricted. And currently we don't have a
> shrinker to just whack out vma objects even ...
> 
> It's definitely a much neater solution than what I had in mind with moving
> vmas to full-blown active tracking like we do objects. So I'm tempted to
> go with, but have a bit a lurking feeling that I'm missing something.
> 
> Chris?

I don't think that only taking the reference for whilst the vma is
active would add much extra code or complexity and being consistent
with the existing active tracking has the advantages you mention.

If we could clean up the vma handling in move_to_inactive that would
remove a major wart all by itself.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre