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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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