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@xxxxxxxxx> > > --- > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx