[PATCH 1/3] drm/i915/vma: Add a busy flag; means don't destroy

[PATCH 1/3] drm/i915/vma: Add a busy flag; means don't destroy

[Ben Widawsky]

The code has at least one place currently, and maybe more in the future
where we want to only unbind a VMA, but not destroy it. There are two
reasons for this, correctness and optimization. Using the one actual
case as an example:

1. Correctness: When doing the execbuf sequence we end up traversing the
"exec_list" looking for misaligned VMAs. At this point we'd like to
unbind the VMA, which could lead to it's destruction. In the current
code, this is "fine" because our vma destroy doesn't remove the VMA from
the exec_list. The latter fact was an oversight which was of trivial
consequence in the original PPGTT series - but now actually matters
since we have a multistage execbuffer.

2. Optimization: Unbinding a VMA is a relatively expensive operation
because it has to go and modify the PTEs. At present, therefore, it
isn't terribly interesting to try to optimize this case. However, one
could potentially do the PTE writes lazily (with full PPGTT there is no
security risk, only a correctness one) - where we don't even update PTEs
until later. Destroying and recreating the VMA in this case can be
spared with this busy flag.

"Hey, but isn't that a leak?"
As you might recall from previous patches, VMAs will be destroyed on
object freeing, and can be cheaply looked up later. Therefore the
dangling VMA isn't a problem. In the future if we end up with an
overabundance of dangling VMAs, we can potentially add some sort of
garbage collection for them.

NOTE: This patch doesn't actually fix anything. The explanation above is
the problem, this is the harness, and the fix comes next.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h | 1 +
 drivers/gpu/drm/i915/i915_gem.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1eb4d98..28c0e0a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -574,6 +574,7 @@ struct i915_vma {
 	unsigned long exec_handle;
 	struct drm_i915_gem_exec_object2 *exec_entry;
 
+	bool busy;
 };
 
 struct i915_ctx_hang_stats {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c7efb60..6d2d7df 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4156,6 +4156,9 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
 
 void i915_gem_vma_destroy(struct i915_vma *vma)
 {
+	if (vma->busy)
+		return;
+
 	WARN_ON(vma->node.allocated);
 	list_del(&vma->vma_link);
 	kfree(vma);
-- 
1.8.3.4

[PATCH 2/3] drm/i915/vma: Mark vmas busy during execbuf lookup

[Ben Widawsky]

The exec_list is a somewhat fragile list containing all the VMAs we've
looked up. It does processing in a multi-stage approach where elements
on the list are expected to not be removed for the duration of the
stages.

The next patch will fix an oversight where destroying a VMA does not
remove it from the exec_list. As such, we must mark the VMA busy for the
multi stage execbuf or else the above assertion we mentioned cannot be
guaranteed.

NOTE: This patch isn't quite tested yet. It just fixes the theoretical
issue.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 9b3b5f8..7d8c841 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -516,6 +516,8 @@ i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma)
 		i915_gem_object_unpin(obj);
 
 	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE | __EXEC_OBJECT_HAS_PIN);
+
+	vma->busy = 0;
 }
 
 static int
@@ -535,6 +537,7 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring,
 		bool need_fence, need_mappable;
 
 		vma = list_first_entry(vmas, struct i915_vma, exec_list);
+		vma->busy = true;
 		obj = vma->obj;
 		entry = vma->exec_entry;
 
-- 
1.8.3.4

[PATCH 3/3] drm/i915/vma: Remove VMA from exec_list on destroy

[Ben Widawsky]

In order to avoid confusion, and corruption as observed (in code review)
by Chris - remove the VMA from the exec_list at destroy.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6d2d7df..9f03f5d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4160,6 +4160,7 @@ void i915_gem_vma_destroy(struct i915_vma *vma)
 		return;
 
 	WARN_ON(vma->node.allocated);
+	list_del(&vma->exec_list);
 	list_del(&vma->vma_link);
 	kfree(vma);
 }
-- 
1.8.3.4

Re: [PATCH 3/3] drm/i915/vma: Remove VMA from exec_list on destroy

[Chris Wilson]

On Mon, Aug 19, 2013 at 11:14:15PM -0700, Ben Widawsky wrote:
> In order to avoid confusion, and corruption as observed (in code review)
> by Chris - remove the VMA from the exec_list at destroy.

This should be BUG_ON(!list_empty(&vma->exec_list));
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre