Re: [PATCH] drm/i915: vma/ppgtt lifetime rules

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel
> Vetter
> Sent: Tuesday, July 29, 2014 8:20 PM
> To: Ben Widawsky
> Cc: Thierry, Michel; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re:  [PATCH] drm/i915: vma/ppgtt lifetime rules
> 
> On Tue, Jul 29, 2014 at 11:44:51AM -0700, Ben Widawsky wrote:
> > 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.
> 
> Well using active would be what we want, with bound we'd have a reference
> loop: ppgtt keeps the vmas bound and the vmas keeps the ppgtt around.
> 
> But active isn't the right thing either: Currently we keep the active
> state per-bo, but have per-vma lrus, so a mix of obj/vma tracking. We've
> had lots of discussions about this and imo neither of the possible active
> approaches is really worth the fuss of converting to it, now that the code
> is in.
> 
> Assume I don't spot any hidden tricky bits after a good night's worth of
> sleep (or anyone else) I'll pull this in tomorrow. So please keep poking
> holes ...
> 
> > > 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.
> 
> Yeah, the kernel generally assumes that an atomic_t or long is
> sufficiently big for a refcount for all of memory. Maybe giant 32bit pae
> boxes violated that, but meh.
> 
> > > 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).
> 
> Hm, that's a good point since that pointers is a loop in the refcounting.
> So if we really don't need it then I agree we should ditch it. Michel,
> can you please throw that follow-up patch on top if that's all there is to
> it indeed?

Sure, I'll prepare that follow-up patch.

-Michel

> 
> Thanks, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Attachment: smime.p7s
Description: S/MIME cryptographic signature

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux