[Intel-gfx] [PATCH] drm/i915: Add secure batch ggtt vma as active during execution

[Mika Kuoppala]

When PPGGT is in use, we need to bind secure batches also to ggtt.
We used to pin, exec and unpin. This trick worked as there was nothing
competing in ggtt space and the ppggt vma was used to tracking.
But this left the ggtt vma untracked and potentially it could vanish
during the batch execution.

Track ggtt vma properly by piggypacking it into the eb->vmas list
just before batch submission is made. This ensures that vma gets
into the active list properly.

Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1a0611b..06c71e9 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -217,6 +217,10 @@ i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma)
 
 	entry = vma->exec_entry;
 
+	/* Check if piggypacked ggtt batch entry */
+	if (!entry)
+		return;
+
 	if (entry->flags & __EXEC_OBJECT_HAS_FENCE)
 		i915_gem_object_unpin_fence(obj);
 
@@ -224,6 +228,8 @@ i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma)
 		vma->pin_count--;
 
 	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE | __EXEC_OBJECT_HAS_PIN);
+
+	vma->exec_entry = NULL;
 }
 
 static void eb_destroy(struct eb_vmas *eb)
@@ -970,7 +976,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 			/* update for the implicit flush after a batch */
 			obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
 		}
-		if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) {
+		if (entry && entry->flags & EXEC_OBJECT_NEEDS_FENCE) {
 			obj->last_fenced_seqno = seqno;
 			if (entry->flags & __EXEC_OBJECT_HAS_FENCE) {
 				struct drm_i915_private *dev_priv = to_i915(ring->dev);
@@ -1385,6 +1391,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	 * batch" bit. Hence we need to pin secure batches into the global gtt.
 	 * hsw should have this fixed, but bdw mucks it up again. */
 	if (flags & I915_DISPATCH_SECURE) {
+		struct i915_vma *vma;
 		/*
 		 * So on first glance it looks freaky that we pin the batch here
 		 * outside of the reservation loop. But:
@@ -1399,6 +1406,18 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		if (ret)
 			goto err;
 
+		vma = i915_gem_obj_to_ggtt(batch_obj);
+
+		/* If there is no exec_entry in this stage, ggtt vma
+		 * is not in the eb->vmas list. Piggypack our ggtt
+		 * address to the list in order for it to be added as
+		 * an active vma in do_execbuf()
+		 */
+		if (vma->exec_entry == NULL) {
+			drm_gem_object_reference(&batch_obj->base);
+			list_add_tail(&vma->exec_list, &eb->vmas);
+		}
+
 		exec_start += i915_gem_obj_ggtt_offset(batch_obj);
 	} else
 		exec_start += i915_gem_obj_offset(batch_obj, vm);
@@ -1406,14 +1425,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	ret = dev_priv->gt.do_execbuf(dev, file, ring, ctx, args,
 				      &eb->vmas, batch_obj, exec_start, flags);
 
-	/*
-	 * FIXME: We crucially rely upon the active tracking for the (ppgtt)
-	 * batch vma for correctness. For less ugly and less fragility this
-	 * needs to be adjusted to also track the ggtt batch vma properly as
-	 * active.
-	 */
-	if (flags & I915_DISPATCH_SECURE)
-		i915_gem_object_ggtt_unpin(batch_obj);
 err:
 	/* the request owns the ref now */
 	i915_gem_context_unreference(ctx);
-- 
1.9.1