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

[Ben Widawsky]

On Tue, Jul 29, 2014 at 11:32:07AM -0700, Ben Widawsky 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);
> > +
> >  	list_del(&vma->vma_link);
> >  
> >  	kfree(vma);
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 5b5af6c..59272f9 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -108,30 +108,13 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
> >  		return;
> >  	}
> >  
> > -	/*
> > -	 * Make sure vmas are unbound before we take down the drm_mm
> > -	 *
> > -	 * FIXME: Proper refcounting should take care of this, this shouldn't be
> > -	 * needed at all.
> > -	 */
> > -	if (!list_empty(&vm->active_list)) {
> > -		struct i915_vma *vma;
> > -
> > -		list_for_each_entry(vma, &vm->active_list, mm_list)
> > -			if (WARN_ON(list_empty(&vma->vma_link) ||
> > -				    list_is_singular(&vma->vma_link)))
> > -				break;
> > -
> > -		i915_gem_evict_vm(&ppgtt->base, true);
> > -	} else {
> > -		i915_gem_retire_requests(dev);
> > -		i915_gem_evict_vm(&ppgtt->base, false);
> > -	}
> > +	/* vmas should already be unbound */
> > +	WARN_ON(!list_empty(&vm->active_list));
> >  
> >  	ppgtt->base.cleanup(&ppgtt->base);
> >  }
> >  
> > -static void ppgtt_release(struct kref *kref)
> > +void ppgtt_release(struct kref *kref)
> >  {
> >  	struct i915_hw_ppgtt *ppgtt =
> >  		container_of(kref, struct i915_hw_ppgtt, ref);
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 1411613..90c3d0f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2159,10 +2159,15 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> >  				  struct i915_address_space *vm)
> >  {
> >  	struct i915_vma *vma;
> > +	struct i915_hw_ppgtt *ppgtt = NULL;
> >  
> >  	vma = i915_gem_obj_to_vma(obj, vm);
> >  	if (!vma)
> >  		vma = __i915_gem_vma_create(obj, vm);
> >  
> > +	ppgtt = vm_to_ppgtt(vm);
> > +	if (ppgtt)
> > +		kref_get(&ppgtt->ref);
> > +
> >  	return vma;
> >  }
> 
> Seems like a reasonable fix to me, and it's simpler than Chris' request
> to ref when active. Logically his makes more sense, but it's not quite
> as simple to review. Actually active isn't the right thing to track IMO,
> bound is what you want to track.

I didn't mean to imply this is anything but a semantic difference with
the driver as it exists today.

> I think if anything, do that one as a
> patch on top, it does address the problem mentioned below. I also wonder
> given the recent reset lolz how this behaves there; and I'd make sure to
> thoroughly test the various reset/hang conditions.
> 
> Also, this puts a hard limit on the maximum number of VMAs in an address
> space based on sizeof(atomic_t). (unless you do what I said above) I
> doubt it's a problem in the near future, but it should be documented
> somewhere and I hope kref will explode loudly if you overflow it.

Also, on second thought, this is a don't care.

> 
> For future safety, I'd recommend dropping vm->ctx at this point. It
> shouldn't be hard at all to get rid of (quick grep shows only debugfs).

-- 
Ben Widawsky, Intel Open Source Technology Center