[Intel-gfx] [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 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);
> 
> 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