[Intel-gfx] [PATCH] drm/i915: Split out aliasing-ppgtt from ggtt_bind_vma()

[Chris Wilson]

The presence of the aliasing-ppgtt (or rather its absence) causes some
confusion on old generations where we still pretend that there is a
difference between PIN_USER and PIN_GLOBAL even though we only have the
global GTT. The confusion has resulted in a bug where we do not mark the
vma as suitably bound for both types of access and so end up processing
the binding twice.

The double bind was accidentally introduced in

    commit 75d04a3773ecee617847de963ae4195d6aa74c28
    Author: Mika Kuoppala <mika.kuoppala at linux.intel.com>
    Date:   Tue Apr 28 17:56:17 2015 +0300

        drm/i915/gtt: Allocate va range only if vma is not bound

While at it implement an improved version suggested by Chris which
avoids the double-bind irrespective of what type of bind is done
first.

Note that this exact bug was already addressed in

commit d0e30adc42d979e4adc36b6c112b57337423b70c
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date:   Wed Jul 29 20:02:48 2015 +0100

        drm/i915: Mark PIN_USER binding as GLOBAL_BIND without the aliasing ppgtt

but the problem is still that originally in

commit 0875546c5318c85c13d07014af5350e9000bc9e9
Author: Daniel Vetter <daniel.vetter at ffwll.ch>
Date:   Mon Apr 20 09:04:05 2015 -0700

        drm/i915: Fix up the vma aliasing ppgtt binding

if forgotten to take into account there case where we have a
GLOBAL_BIND before a LOCAL_BIND.

Here we separate out the binding into the global GTT for machines where
we have the aliasing-ppgtt and where we do not, so that we can perform
the fixup without further confusion.

Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 62 +++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0796aa06e9f5..81f4628ae0e6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2493,7 +2493,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
 			 enum i915_cache_level cache_level,
 			 u32 flags)
 {
-	struct drm_i915_private *dev_priv = to_i915(vma->vm->dev);
 	u32 pte_flags;
 	int ret;
 
@@ -2506,26 +2505,49 @@ static int ggtt_bind_vma(struct i915_vma *vma,
 	if (vma->obj->gt_ro)
 		pte_flags |= PTE_READ_ONLY;
 
-	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
+	vma->vm->insert_entries(vma->vm,
+				vma->ggtt_view.pages,
+				vma->node.start,
+				cache_level, pte_flags);
+
+	/* Note the inconsistency here is due to absence of the
+	 * aliasing ppgtt on gen5 and earlier. Though we always
+	 * request PIN_USER for execbuffer (translated to LOCAL_BIND),
+	 * without the appgtt, we cannot honour that request and so
+	 * must substitute it with a global binding. Since we do this
+	 * behind the upper layers back, we need to explicitly set
+	 * the bound flag ourselves.
+	 */
+	vma->bound |= GLOBAL_BIND | LOCAL_BIND;
+	return 0;
+}
+
+static int agtt_bind_vma(struct i915_vma *vma,
+			 enum i915_cache_level cache_level,
+			 u32 flags)
+{
+	u32 pte_flags;
+	int ret;
+
+	ret = i915_get_ggtt_vma_pages(vma);
+	if (ret)
+		return ret;
+
+	/* Currently applicable only to VLV */
+	pte_flags = 0;
+	if (vma->obj->gt_ro)
+		pte_flags |= PTE_READ_ONLY;
+
+	if (flags & GLOBAL_BIND) {
 		vma->vm->insert_entries(vma->vm,
 					vma->ggtt_view.pages,
 					vma->node.start,
 					cache_level, pte_flags);
-
-		/* Note the inconsistency here is due to absence of the
-		 * aliasing ppgtt on gen4 and earlier. Though we always
-		 * request PIN_USER for execbuffer (translated to LOCAL_BIND),
-		 * without the appgtt, we cannot honour that request and so
-		 * must substitute it with a global binding. Since we do this
-		 * behind the upper layers back, we need to explicitly set
-		 * the bound flag ourselves.
-		 */
-		vma->bound |= GLOBAL_BIND;
-
 	}
 
-	if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
-		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
+	if (flags & LOCAL_BIND) {
+		struct i915_hw_ppgtt *appgtt =
+			to_i915(vma->vm->dev)->mm.aliasing_ppgtt;
 		appgtt->base.insert_entries(&appgtt->base,
 					    vma->ggtt_view.pages,
 					    vma->node.start,
@@ -2959,7 +2981,10 @@ static int gen8_gmch_probe(struct drm_device *dev,
 	dev_priv->gtt.base.clear_range = gen8_ggtt_clear_range;
 	dev_priv->gtt.base.insert_page = gen8_ggtt_insert_page;
 	dev_priv->gtt.base.insert_entries = gen8_ggtt_insert_entries;
-	dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
+	if (dev_priv->mm.aliasing_ppgtt)
+		dev_priv->gtt.base.bind_vma = agtt_bind_vma;
+	else
+		dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
 	dev_priv->gtt.base.unbind_vma = ggtt_unbind_vma;
 
 	return ret;
@@ -3002,7 +3027,10 @@ static int gen6_gmch_probe(struct drm_device *dev,
 	dev_priv->gtt.base.clear_range = gen6_ggtt_clear_range;
 	dev_priv->gtt.base.insert_page = gen6_ggtt_insert_page;
 	dev_priv->gtt.base.insert_entries = gen6_ggtt_insert_entries;
-	dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
+	if (dev_priv->mm.aliasing_ppgtt)
+		dev_priv->gtt.base.bind_vma = agtt_bind_vma;
+	else
+		dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
 	dev_priv->gtt.base.unbind_vma = ggtt_unbind_vma;
 
 	return ret;
-- 
2.6.1