[Intel-gfx] [PATCH] drm/i915: Fix up the vma aliasing ppgtt binding

[Chris Wilson]

On Fri, Apr 24, 2015 at 12:14:17PM +0100, Chris Wilson wrote:
> On Mon, Apr 20, 2015 at 09:04:05AM -0700, Daniel Vetter wrote:
> > Currently we have the problem that the decision whether ptes need to
> > be (re)written is splattered all over the codebase. Move all that into
> > i915_vma_bind. This needs a few changes:
> > - Just reuse the PIN_* flags for i915_vma_bind and do the conversion
> >   to vma->bound in there to avoid duplicating the conversion code all
> >   over.
> > - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
> >   around) explicit, add PIN_USER for that.
> > - Two callers want to update ptes, give them a PIN_UPDATE for that.
> > 
> > Of course we still want to avoid double-binding, but that should be
> > taken care of:
> > - A ppgtt vma will only ever see PIN_USER, so no issue with
> >   double-binding.
> > - A ggtt vma with aliasing ppgtt needs both types of binding, and we
> >   track that properly now.
> > - A ggtt vma without aliasing ppgtt could be bound twice. In the
> >   lower-level ->bind_vma functions hence unconditionally set
> >   GLOBAL_BIND when writing the ggtt ptes.
> > 
> > There's still a bit room for cleanup, but that's for follow-up
> > patches.
> > 
> > v2: Fixup fumbles.
> > 
> > v3: s/PIN_EXECBUF/PIN_USER/ for clearer meaning, suggested by Chris.
> > 
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> 
> I'm getting lots of GPU hangs from this patch...

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 91aade7..e3841f8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3066,8 +3066,8 @@ int i915_vma_unbind(struct i915_vma *vma)
 	}
 
 	trace_i915_vma_unbind(vma);
-
 	vma->vm->unbind_vma(vma);
+	vma->bound = 0;
 
 	list_del_init(&vma->mm_list);
 	if (i915_is_ggtt(vma->vm)) {
@@ -3588,7 +3588,6 @@ search_free:
 	if (ret)
 		goto err_remove_node;
 
-	trace_i915_vma_bind(vma, flags);
 	ret = i915_vma_bind(vma, obj->cache_level, flags);
 	if (ret)
 		goto err_finish_gtt;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index cfdc8c6..45d74da 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1563,9 +1563,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		 * be in ggtt still end up in the aliasing ppgtt. remove
 		 * this check when that is fixed.
 		 */
-		if (USES_FULL_PPGTT(dev))
-			dispatch_flags |= I915_DISPATCH_SECURE;
-
+		dispatch_flags |= I915_DISPATCH_SECURE;
 		exec_start = 0;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 9e06180..bbeb6c3 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1949,8 +1949,6 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
 
 	BUG_ON(!i915_is_ggtt(vma->vm));
 	intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
-
-	vma->bound |= GLOBAL_BIND;
 }
 
 static void i915_ggtt_clear_range(struct i915_address_space *vm,
@@ -2813,9 +2811,12 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
 int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 		  u32 flags)
 {
-	u32 bind_flags = 0;
+	u32 bind_flags;
 	int ret;
 
+	if (WARN_ON(flags == 0))
+		return -EINVAL;
+
 	if (vma->vm->allocate_va_range) {
 		trace_i915_va_alloc(vma->vm, vma->node.start,
 				    vma->node.size,
@@ -2834,6 +2835,7 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 			return 0;
 	}
 
+	bind_flags = 0;
 	if (flags & PIN_GLOBAL)
 		bind_flags |= GLOBAL_BIND;
 	if (flags & PIN_USER)
@@ -2844,10 +2846,11 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 	else
 		bind_flags &= ~vma->bound;
 
-	if (bind_flags)
+	if (bind_flags) {
+		trace_i915_vma_bind(vma, bind_flags);
 		vma->vm->bind_vma(vma, cache_level, bind_flags);
-
-	vma->bound |= bind_flags;
+		vma->bound |= bind_flags;
+	}
 
 	return 0;
 }

-- 
Chris Wilson, Intel Open Source Technology Centre