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.
v2: Keep vma allocated until closed. This is useful for a later optimisation, but it is required now in order to handle potential recursion of i915_vma_unbind() by retiring itself. v3: Comments are important. Testcase: igt/gem_ppggtt/flink-and-close-vma-leak Signed-off-by: Chris Wilson <[email protected]> Cc: Tvrtko Ursulin <[email protected]> Cc: Daniele Ceraolo Spurio <[email protected] --- drivers/gpu/drm/i915/i915_drv.c | 1 + drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_gem.c | 126 ++++++++++++++++++++++-------------- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 + drivers/gpu/drm/i915/i915_gem_gtt.h | 1 + 5 files changed, 84 insertions(+), 48 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index cc831a34f7bb..2a0882647c23 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1664,6 +1664,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 9fa925389332..262d1b247344 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2673,8 +2673,8 @@ 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); /* Flags used by pin/bind&friends. */ #define PIN_MAPPABLE (1<<0) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 7e4f7f2d18e4..1f95cf39b7d2 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2385,6 +2385,30 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj) } } +static void i915_vma_close(struct i915_vma *vma) +{ + GEM_BUG_ON(vma->closed); + vma->closed = true; + + 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 @@ -2571,31 +2595,56 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj) old_write_domain); } +static void i915_vma_destroy(struct i915_vma *vma) +{ + GEM_BUG_ON(vma->node.allocated); + GEM_BUG_ON(vma->active); + GEM_BUG_ON(!vma->closed); + + list_del(&vma->vm_link); + if (!vma->is_ggtt) + i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm)); + + kmem_cache_free(to_i915(vma->obj->base.dev)->vmas, vma); +} + static int __i915_vma_unbind(struct i915_vma *vma, bool wait) { struct drm_i915_gem_object *obj = vma->obj; - int ret; + int ret, i; - if (list_empty(&vma->obj_link)) - return 0; + /* First wait upon any activity as retiring the request may + * have side-effects such as unpinning or even unbinding this vma. + */ + if (vma->active && wait) { + bool was_closed; - if (!drm_mm_node_allocated(&vma->node)) { - i915_gem_vma_destroy(vma); - return 0; + /* When a closed VMA is retired, it is unbound - eek. */ + was_closed = vma->closed; + vma->closed = false; + + for (i = 0; i < ARRAY_SIZE(vma->last_read); i++) { + ret = i915_wait_request(vma->last_read[i].request); + if (ret) + break; + } + + vma->closed = was_closed; + if (ret) + return ret; + + GEM_BUG_ON(vma->active); } if (vma->pin_count) return -EBUSY; + if (!drm_mm_node_allocated(&vma->node)) + goto destroy; + GEM_BUG_ON(obj->bind_count == 0); GEM_BUG_ON(obj->pages == NULL); - if (wait) { - ret = i915_gem_object_wait_rendering(obj, false); - if (ret) - return ret; - } - if (vma->is_ggtt && vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { i915_gem_object_finish_gtt(obj); @@ -2622,7 +2671,6 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) } drm_mm_remove_node(&vma->node); - i915_gem_vma_destroy(vma); /* Since the unbound list is global, only move to that list if * no more VMAs exist. */ @@ -2636,6 +2684,10 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) */ i915_gem_object_unpin_pages(obj); +destroy: + if (unlikely(vma->closed)) + i915_vma_destroy(vma); + return 0; } @@ -2814,7 +2866,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, if (offset & (alignment - 1) || offset + size > end) { ret = -EINVAL; - goto err_free_vma; + goto err_vma; } vma->node.start = offset; vma->node.size = size; @@ -2826,7 +2878,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, ret = drm_mm_reserve_node(&vm->mm, &vma->node); } if (ret) - goto err_free_vma; + goto err_vma; } else { if (flags & PIN_HIGH) { search_flag = DRM_MM_SEARCH_BELOW; @@ -2851,7 +2903,7 @@ search_free: if (ret == 0) goto search_free; - goto err_free_vma; + goto err_vma; } } if (WARN_ON(!i915_gem_valid_gtt_space(vma, obj->cache_level))) { @@ -2872,8 +2924,7 @@ search_free: err_remove_node: drm_mm_remove_node(&vma->node); -err_free_vma: - i915_gem_vma_destroy(vma); +err_vma: vma = ERR_PTR(ret); err_unpin: i915_gem_object_unpin_pages(obj); @@ -3808,21 +3859,18 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) trace_i915_gem_object_destroy(obj); + /* All file-owned VMA should have been released by this point through + * i915_gem_close_object(), or earlier by i915_gem_context_close(). + * However, the object may also be bound into the global GTT (e.g. + * older GPUs without per-process support, or for direct access through + * the GTT either for the user or for scanout). Those VMA still need to + * unbound now. + */ list_for_each_entry_safe(vma, next, &obj->vma_list, obj_link) { - int ret; - + GEM_BUG_ON(!i915_is_ggtt(vma->vm)); + GEM_BUG_ON(vma->active); 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); } GEM_BUG_ON(obj->bind_count); @@ -3890,22 +3938,6 @@ struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj, return NULL; } -void i915_gem_vma_destroy(struct i915_vma *vma) -{ - WARN_ON(vma->node.allocated); - - /* Keep the vma as a placeholder in the execbuffer reservation lists */ - if (!list_empty(&vma->exec_list)) - return; - - if (!vma->is_ggtt) - i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm)); - - list_del(&vma->obj_link); - - kmem_cache_free(to_i915(vma->obj->base.dev)->vmas, vma); -} - static void i915_gem_stop_ringbuffers(struct drm_device *dev) { diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index fd42b6491d28..ef093db6b8a6 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3257,6 +3257,8 @@ i915_vma_retire(struct i915_gem_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 0a7867fa5a1f..d68d5fd02923 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. -- 2.7.0.rc3 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
