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

[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@ffwll.ch>
Signed-off-by: Mika Kuoppala <mika.kuoppala@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

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

[Chris Wilson]

On Thu, Sep 11, 2014 at 11:39:46AM +0300, Mika Kuoppala wrote:
> 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@ffwll.ch>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@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);

vma->exec_entry = memset(&dummy_exec_entry, 0, sizeof(dummy_exec_entry);

Then you can kill the additional if (entry) checks.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Volkin, Bradley D]

On Thu, Sep 11, 2014 at 01:49:07AM -0700, Chris Wilson wrote:
> On Thu, Sep 11, 2014 at 11:39:46AM +0300, Mika Kuoppala wrote:
> > 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@ffwll.ch>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@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);
> 
> vma->exec_entry = memset(&dummy_exec_entry, 0, sizeof(dummy_exec_entry);
> 
> Then you can kill the additional if (entry) checks.

Also, don't we need to set vma->exec_entry->flags = __EXEC_OBJECT_HAS_PIN ?

Looking back at the last set of batch buffer copy patches I sent, I thought
at that time that it was necessary when doing something similar for the
shadow batch. Not sure yet how all the code has changed since then.

http://lists.freedesktop.org/archives/intel-gfx/2014-July/048707.html

Brad

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

[Chris Wilson]

On Thu, Sep 11, 2014 at 09:30:27AM -0700, Volkin, Bradley D wrote:
> On Thu, Sep 11, 2014 at 01:49:07AM -0700, Chris Wilson wrote:
> > vma->exec_entry = memset(&dummy_exec_entry, 0, sizeof(dummy_exec_entry);
> > 
> > Then you can kill the additional if (entry) checks.
> 
> Also, don't we need to set vma->exec_entry->flags = __EXEC_OBJECT_HAS_PIN ?

Yes we do. I realised that and mentioned it to Mika on IRC right after
sending :|

All the more reason for using the dummy_exec_entry/shadow_exec_entry.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre