[Intel-gfx] [PATCH] drm/i915: vma/ppgtt lifetime rules
Thierry, Michel
michel.thierry at intel.com
Wed Jul 30 10:19:38 CEST 2014
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Tuesday, July 29, 2014 8:20 PM
> To: Ben Widawsky
> Cc: Thierry, Michel; intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [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 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.
>
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 6656 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140730/75b7ec72/attachment.bin>
More information about the Intel-gfx
mailing list