Re: [PATCH 53/55] drm/i915: Release vma when the handle is closed

Joonas Lahtinen
2016-07-28T07:16:51Z

On ke, 2016-07-27 at 11:13 +0100, Chris Wilson wrote:
> On Wed, Jul 27, 2016 at 01:00:59PM +0300, Joonas Lahtinen wrote:
> >
> > On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> > >
> > >  /**
> > >   * i915_gem_wait_ioctl - implements DRM_IOCTL_I915_GEM_WAIT
> > >   * @dev: drm device pointer
> > > @@ -2810,26 +2823,32 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
> > >   if (active && wait) {
> > >   int idx;
> > >
> > > + /* When a closed VMA is retired, it is unbound - eek.
> > > +  * In order to prevent it from being recursively closed,
> > > +  * take a pin on the vma so that the second unbind is
> > > +  * aborted.
> > > +  */
> > > + vma->pin_count++;
> > Always smells fishy. But an extra variable probably not worthy.
> >
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index ef0dc7131808..bfac2448ba04 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -3339,6 +3339,31 @@ i915_vma_retire(struct i915_gem_active *active,
> > >   return;
> > >
> > >   list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
> > > + if (unlikely(vma->closed && !vma->pin_count))
> > > + WARN_ON(i915_vma_unbind(vma));
> > So this is just an optimization to get rid of the VMA ASAP if we're
> > lucky?
> Not quite. Think of this as the active reference, this may be the last
> point at which we see the closed vma. If we don't free it now, we leak
> the VMA until the object is closed - where if we meet it we BUG.
>
> >
> > >
> > > +void i915_vma_close(struct i915_vma *vma)
> > > +{
> > > + GEM_BUG_ON(vma->closed);
> > > + vma->closed = true;
> > > +
> > > + list_del_init(&vma->obj_link);
> > > + if (!i915_vma_is_active(vma) && !vma->pin_count)
> > > + WARN_ON(__i915_vma_unbind_no_wait(vma));
> > Same here, an optimization?
> Same as above. This time we know the vma is inactive, so no outstanding
> reference.

Ok,

We have the BUG in place if something slips through the cracks, so
should be good.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas

> -Chris
>
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation