Re: [PATCH 09/11] drm/i915: Release vma when the handle is closed

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi,

On 14/12/15 11:36, Chris Wilson wrote:
> In order to prevent a leak of the vma on shared objects, we need to
> hook into the object_close callback to destroy the vma on the object for
> this file. However, if we destroyed that vma immediately we may cause
> unexpected application stalls as we try to unbind a busy vma - hence we
> defer the unbind to when we retire the vma.
>
> Testcase: igt/gem_ppggtt/flink-and-close-vma-leak
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com
> ---
>   drivers/gpu/drm/i915/i915_drv.c     |  1 +
>   drivers/gpu/drm/i915/i915_drv.h     |  1 +
>   drivers/gpu/drm/i915/i915_gem.c     | 41 ++++++++++++++++++++++---------------
>   drivers/gpu/drm/i915/i915_gem_gtt.c |  2 ++
>   drivers/gpu/drm/i915/i915_gem_gtt.h |  1 +
>   5 files changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e7eef5fd6918..70979339d58a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1656,6 +1656,7 @@ static struct drm_driver driver = {
>   	.debugfs_init = i915_debugfs_init,
>   	.debugfs_cleanup = i915_debugfs_cleanup,
>   #endif
> +	.gem_close_object = i915_gem_close_object,
>   	.gem_free_object = i915_gem_free_object,
>   	.gem_vm_ops = &i915_gem_vm_ops,
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index eb775eb1c693..696469a06715 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2686,6 +2686,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>   						  size_t size);
>   struct drm_i915_gem_object *i915_gem_object_create_from_data(
>   		struct drm_device *dev, const void *data, size_t size);
> +void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file);
>   void i915_gem_free_object(struct drm_gem_object *obj);
>   void i915_gem_vma_destroy(struct i915_vma *vma);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1d21c5b79215..7c13c27a6470 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2367,6 +2367,30 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
>   	return 0;
>   }
>
> +static void i915_vma_close(struct i915_vma *vma)
> +{
> +	RQ_BUG_ON(vma->closed);

Same complaint as in the previous patch, cannot use RQ_BUG_ON. Maybe 
need GEM_BUG_ON then, don't know.

> +	vma->closed = true;

Hmmm, vma->detached? Because VMA is not really closed. And 
i915_vma_detach - it would symbolise then that VMA has been detached 
from the object and is lingering only on the VM lists.

> +
> +	list_del_init(&vma->obj_link);
> +	if (!vma->active)
> +		WARN_ON(i915_vma_unbind(vma));
> +}
> +
> +void i915_gem_close_object(struct drm_gem_object *gem,
> +			   struct drm_file *file)
> +{
> +	struct drm_i915_gem_object *obj = to_intel_bo(gem);
> +	struct drm_i915_file_private *fpriv = file->driver_priv;
> +	struct i915_vma *vma, *vn;
> +
> +	mutex_lock(&obj->base.dev->struct_mutex);
> +	list_for_each_entry_safe(vma, vn, &obj->vma_list, obj_link)
> +		if (vma->vm->file == fpriv)
> +			i915_vma_close(vma);
> +	mutex_unlock(&obj->base.dev->struct_mutex);
> +}
> +
>   /**
>    * i915_gem_wait_ioctl - implements DRM_IOCTL_I915_GEM_WAIT
>    * @DRM_IOCTL_ARGS: standard ioctl arguments
> @@ -2577,9 +2601,6 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
>   		RQ_BUG_ON(vma->active);
>   	}
>
> -	if (list_empty(&vma->obj_link))
> -		return 0;
> -
>   	if (!drm_mm_node_allocated(&vma->node)) {
>   		i915_gem_vma_destroy(vma);
>   		return 0;
> @@ -3792,20 +3813,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>   	trace_i915_gem_object_destroy(obj);
>
>   	list_for_each_entry_safe(vma, next, &obj->vma_list, obj_link) {
> -		int ret;
> -
>   		vma->pin_count = 0;
> -		ret = i915_vma_unbind(vma);
> -		if (WARN_ON(ret == -ERESTARTSYS)) {
> -			bool was_interruptible;
> -
> -			was_interruptible = dev_priv->mm.interruptible;
> -			dev_priv->mm.interruptible = false;
> -
> -			WARN_ON(i915_vma_unbind(vma));
> -
> -			dev_priv->mm.interruptible = was_interruptible;
> -		}
> +		i915_vma_close(vma);

In what circumstances can there be any VMAs still left unclosed at this 
point? I thought i915_gem_close_object would had closed them all.

>   	}
>
>   	/* Stolen objects don't hold a ref, but do hold pin count. Fix that up
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 5505603f52af..9f594c33bd0a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3240,6 +3240,8 @@ i915_vma_retire(struct drm_i915_gem_request_active *active,
>   		return;
>
>   	list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
> +	if (unlikely(vma->closed))
> +		WARN_ON(i915_vma_unbind(vma));
>   }
>
>   static struct i915_vma *
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index c2f2c62ac88d..be7e8526b219 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -190,6 +190,7 @@ struct i915_vma {
>   	unsigned int bound : 4;
>   	unsigned int active : I915_NUM_RINGS;
>   	bool is_ggtt : 1;
> +	bool closed : 1;
>
>   	/**
>   	 * Support different GGTT views into the same object.
>

Regards,

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