[Intel-gfx] [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 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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre