[PATCH] drm/i915: Handle i915_ppgtt_put correctly

[PATCH] drm/i915: Handle i915_ppgtt_put correctly

[Michel Thierry]

Unfortunately, the gem_obj/vma relationship is not symmetrical; a gem_obj
can look up for the same vma more than once (where the ppgtt refcount is
incremented), but will free the vma only once (i915_gem_free_object).

This difference in refcount get/put means that the ppgtt is not removed
after the context and vma are destroyed, because sometimes the refcount
will never go back to zero.

Keep track of how many times a gem_obj has looked up for a given vma and
call i915_ppgtt_put accordingly. This will ensure that the ppgtt is also
removed.

OTC-Jira: VIZ-3719
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c     | 8 ++++++--
 drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++++-
 drivers/gpu/drm/i915/i915_gem_gtt.h | 1 +
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 488244a..f8e9431 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4573,8 +4573,12 @@ void i915_gem_vma_destroy(struct i915_vma *vma)
 
 	vm = vma->vm;
 
-	if (!i915_is_ggtt(vm))
-		i915_ppgtt_put(i915_vm_to_ppgtt(vm));
+	if (!i915_is_ggtt(vm)) {
+		while (vma->ppgtt_refcount > 0) {
+			i915_ppgtt_put(i915_vm_to_ppgtt(vm));
+			vma->ppgtt_refcount--;
+		}
+	}
 
 	list_del(&vma->vma_link);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 840365c..db6bbe3 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2223,6 +2223,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
 	INIT_LIST_HEAD(&vma->exec_list);
 	vma->vm = vm;
 	vma->obj = obj;
+	vma->ppgtt_refcount = 0;
 
 	switch (INTEL_INFO(vm->dev)->gen) {
 	case 8:
@@ -2267,8 +2268,10 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
 	if (!vma)
 		vma = __i915_gem_vma_create(obj, vm);
 
-	if (!i915_is_ggtt(vm))
+	if (!i915_is_ggtt(vm)) {
 		i915_ppgtt_get(i915_vm_to_ppgtt(vm));
+		vma->ppgtt_refcount++;
+	}
 
 	return vma;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 7616876..28e41d7 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -122,6 +122,7 @@ struct i915_vma {
 	struct drm_mm_node node;
 	struct drm_i915_gem_object *obj;
 	struct i915_address_space *vm;
+	int ppgtt_refcount;
 
 	/** This object's place on the active/inactive lists */
 	struct list_head mm_list;
-- 
2.0.3

Re: [PATCH] drm/i915: Handle i915_ppgtt_put correctly

[Daniel Vetter]

On Tue, Aug 19, 2014 at 10:50 AM, Michel Thierry
<michel.thierry@intel.com> wrote:
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2223,6 +2223,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
>         INIT_LIST_HEAD(&vma->exec_list);
>         vma->vm = vm;
>         vma->obj = obj;
> +       vma->ppgtt_refcount = 0;
>
>         switch (INTEL_INFO(vm->dev)->gen) {
>         case 8:
> @@ -2267,8 +2268,10 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
>         if (!vma)
>                 vma = __i915_gem_vma_create(obj, vm);
>
> -       if (!i915_is_ggtt(vm))
> +       if (!i915_is_ggtt(vm)) {
>                 i915_ppgtt_get(i915_vm_to_ppgtt(vm));
> +               vma->ppgtt_refcount++;
> +       }

Shouldn't we just move the ppgtt into the vma_create hunk (or
function) instead of this complication?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v2] drm/i915: Handle i915_ppgtt_put correctly

[Michel Thierry]

Unfortunately, the gem_obj/vma relationship is not symmetrical; a gem_obj
can look up for the same vma more than once (where the ppgtt refcount is
incremented), but will free the vma only once (i915_gem_free_object).

This difference in refcount get/put means that the ppgtt is not removed
after the context and vma are destroyed, because sometimes the refcount
will never go back to zero.

v2: Just move the ppgtt refcount into vma_create.

OTC-Jira: VIZ-3719
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 840365c..95f6df4 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2251,8 +2251,10 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
 	/* Keep GGTT vmas first to make debug easier */
 	if (i915_is_ggtt(vm))
 		list_add(&vma->vma_link, &obj->vma_list);
-	else
+	else {
 		list_add_tail(&vma->vma_link, &obj->vma_list);
+		i915_ppgtt_get(i915_vm_to_ppgtt(vm));
+	}
 
 	return vma;
 }
@@ -2267,8 +2269,5 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
 	if (!vma)
 		vma = __i915_gem_vma_create(obj, vm);
 
-	if (!i915_is_ggtt(vm))
-		i915_ppgtt_get(i915_vm_to_ppgtt(vm));
-
 	return vma;
 }
-- 
2.0.3

Re: [PATCH v2] drm/i915: Handle i915_ppgtt_put correctly

[Daniel Vetter]

On Tue, Aug 19, 2014 at 03:49:41PM +0100, Michel Thierry wrote:
> Unfortunately, the gem_obj/vma relationship is not symmetrical; a gem_obj
> can look up for the same vma more than once (where the ppgtt refcount is
> incremented), but will free the vma only once (i915_gem_free_object).
> 
> This difference in refcount get/put means that the ppgtt is not removed
> after the context and vma are destroyed, because sometimes the refcount
> will never go back to zero.
> 
> v2: Just move the ppgtt refcount into vma_create.
> 
> OTC-Jira: VIZ-3719
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch