[PATCH 0/7] gtt patches

[PATCH 0/7] gtt patches

[Daniel Vetter]

Hi all,

So the first part reworks the gtt ->bind/unbind stuff so that I can rebase the
-internal try without going berserk. 2nd patch is a bit of fluff removal, next
for do reorder i915_gem_gtt.c a bit to make more sense (I've been confused every
2nd time I've had to rebase -internal and there was a conflict in there, call me
stupid ...). Last one is my ducttape for the BUG_ON Ville hit.

Cheers, Daniel

Daniel Vetter (7):
  drm/i915: resurrect ->insert_entries/->clear_range gtt interfaces
  drm/i915: don't allow ppgtt enabling to fail anymore
  drm/i915/gtt: group sg setup/teardown functions to the end
  drm/i915/gtt: move restore_gtt_mappings to the bottom
  drm/i915/gtt: group legacy global gtt code together
  drm/i915/gtt: move generic setup code all to the bottom
  drm/i915: we can't call move_to_active before intel_ring_begin

 drivers/gpu/drm/i915/i915_dma.c            |   4 +-
 drivers/gpu/drm/i915/i915_drv.c            |   4 +-
 drivers/gpu/drm/i915/i915_drv.h            |   6 +-
 drivers/gpu/drm/i915/i915_gem.c            |   8 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  40 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 683 ++++++++++++++---------------
 6 files changed, 357 insertions(+), 388 deletions(-)

-- 
1.8.1.4

[PATCH 1/7] drm/i915: resurrect ->insert_entries/->clear_range gtt interfaces

[Daniel Vetter]

We need them in the internal tree still.

Also give them neat generic_ prefixes so that it's clear that only
gen6 is special wrt binding/unbinding a vma. And move the to the top
of the file to avoid forward declarations.

Also use the ppgtt version in the ppgtt init code already, to avoid
marking functions as dead code.

Cc: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 95 +++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e053f14..db39612 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -28,6 +28,45 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+/* Generic bind/unbind helpers. */
+static void generic_ppgtt_bind_vma(struct i915_vma *vma,
+				   enum i915_cache_level cache_level,
+				   u32 flags)
+{
+	const unsigned long entry = vma->node.start >> PAGE_SHIFT;
+
+	WARN_ON(flags);
+
+	vma->vm->insert_entries(vma->vm, vma->obj->pages, entry, cache_level);
+}
+
+static void generic_ppgtt_unbind_vma(struct i915_vma *vma)
+{
+	vma->vm->clear_range(vma->vm, vma->node.start >> PAGE_SHIFT,
+			     vma->obj->base.size >> PAGE_SHIFT);
+}
+
+static void generic_ggtt_bind_vma(struct i915_vma *vma,
+				  enum i915_cache_level cache_level,
+				  u32 unused)
+{
+	const unsigned long entry = vma->node.start >> PAGE_SHIFT;
+
+	BUG_ON(!i915_is_ggtt(vma->vm));
+
+	vma->vm->insert_entries(vma->vm, vma->obj->pages, entry, cache_level);
+	vma->obj->has_global_gtt_mapping = 1;
+}
+
+static void generic_ggtt_unbind_vma(struct i915_vma *vma)
+{
+	BUG_ON(!i915_is_ggtt(vma->vm));
+	vma->obj->has_global_gtt_mapping = 0;
+
+	vma->vm->clear_range(vma->vm, vma->node.start >> PAGE_SHIFT,
+			     vma->obj->base.size >> PAGE_SHIFT);
+}
+
 #define GEN6_PPGTT_PD_ENTRIES 512
 #define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t))
 
@@ -57,11 +96,6 @@
 #define HSW_WB_ELLC_LLC_AGE0		HSW_CACHEABILITY_CONTROL(0xb)
 #define HSW_WT_ELLC_LLC_AGE0		HSW_CACHEABILITY_CONTROL(0x6)
 
-static void gen6_ppgtt_bind_vma(struct i915_vma *vma,
-				enum i915_cache_level cache_level,
-				u32 flags);
-static void gen6_ppgtt_unbind_vma(struct i915_vma *vma);
-
 static gen6_gtt_pte_t snb_pte_encode(dma_addr_t addr,
 				     enum i915_cache_level level)
 {
@@ -337,9 +371,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	ppgtt->base.pte_encode = dev_priv->gtt.base.pte_encode;
 	ppgtt->num_pd_entries = GEN6_PPGTT_PD_ENTRIES;
 	ppgtt->enable = gen6_ppgtt_enable;
-	ppgtt->base.unbind_vma = NULL;
+	ppgtt->base.unbind_vma = generic_ppgtt_unbind_vma;
 	ppgtt->base.clear_range = gen6_ppgtt_clear_range;
-	ppgtt->base.bind_vma = NULL;
+	ppgtt->base.bind_vma = generic_ppgtt_bind_vma;
 	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
 	ppgtt->base.cleanup = gen6_ppgtt_cleanup;
 	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
@@ -437,26 +471,6 @@ void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev)
 	dev_priv->mm.aliasing_ppgtt = NULL;
 }
 
-static void __always_unused
-gen6_ppgtt_bind_vma(struct i915_vma *vma,
-		    enum i915_cache_level cache_level,
-		    u32 flags)
-{
-	const unsigned long entry = vma->node.start >> PAGE_SHIFT;
-
-	WARN_ON(flags);
-
-	gen6_ppgtt_insert_entries(vma->vm, vma->obj->pages, entry, cache_level);
-}
-
-static void __always_unused gen6_ppgtt_unbind_vma(struct i915_vma *vma)
-{
-	const unsigned long entry = vma->node.start >> PAGE_SHIFT;
-
-	gen6_ppgtt_clear_range(vma->vm, entry,
-			       vma->obj->base.size >> PAGE_SHIFT);
-}
-
 extern int intel_iommu_gfx_mapped;
 /* Certain Gen5 chipsets require require idling the GPU before
  * unmapping anything from the GTT when VT-d is enabled.
@@ -604,19 +618,6 @@ static void i915_ggtt_insert_entries(struct i915_address_space *vm,
 
 }
 
-static void i915_ggtt_bind_vma(struct i915_vma *vma,
-			       enum i915_cache_level cache_level,
-			       u32 unused)
-{
-	const unsigned long entry = vma->node.start >> PAGE_SHIFT;
-	unsigned int flags = (cache_level == I915_CACHE_NONE) ?
-		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
-
-	BUG_ON(!i915_is_ggtt(vma->vm));
-	intel_gtt_insert_sg_entries(vma->obj->pages, entry, flags);
-	vma->obj->has_global_gtt_mapping = 1;
-}
-
 static void i915_ggtt_clear_range(struct i915_address_space *vm,
 				  unsigned int first_entry,
 				  unsigned int num_entries)
@@ -624,16 +625,6 @@ static void i915_ggtt_clear_range(struct i915_address_space *vm,
 	intel_gtt_clear_range(first_entry, num_entries);
 }
 
-static void i915_ggtt_unbind_vma(struct i915_vma *vma)
-{
-	const unsigned int first = vma->node.start >> PAGE_SHIFT;
-	const unsigned int size = vma->obj->base.size >> PAGE_SHIFT;
-
-	BUG_ON(!i915_is_ggtt(vma->vm));
-	vma->obj->has_global_gtt_mapping = 0;
-	intel_gtt_clear_range(first, size);
-}
-
 static void gen6_ggtt_bind_vma(struct i915_vma *vma,
 			       enum i915_cache_level cache_level,
 			       u32 flags)
@@ -960,9 +951,9 @@ static int i915_gmch_probe(struct drm_device *dev,
 
 	dev_priv->gtt.do_idle_maps = needs_idle_maps(dev_priv->dev);
 	dev_priv->gtt.base.clear_range = i915_ggtt_clear_range;
-	dev_priv->gtt.base.unbind_vma = i915_ggtt_unbind_vma;
+	dev_priv->gtt.base.unbind_vma = generic_ggtt_unbind_vma;
 	dev_priv->gtt.base.insert_entries = i915_ggtt_insert_entries;
-	dev_priv->gtt.base.bind_vma = i915_ggtt_bind_vma;
+	dev_priv->gtt.base.bind_vma = generic_ggtt_bind_vma;
 
 	return 0;
 }
-- 
1.8.1.4

[PATCH 2/7] drm/i915: don't allow ppgtt enabling to fail anymore

[Daniel Vetter]

Trying to keep on running is poised to run into totally untested code
after a gpu reset or resume. So better just stop things from going
down the toilet.

And at driver load time we better bail out and fix whatever's breaking
ppgtt instead of ignoring it.

So there's no need for this complexity. Shovel the aliasing ppgtt cleanup
into the gen6 ggtt cleanup (there's no other platform with an aliasing
ppgtt just now). Also add the missing call to gtt.base.cleanup in
i915_load_modeset_init. Oh dear how I hate our driver load mess ;-)

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c     |  4 ++--
 drivers/gpu/drm/i915/i915_drv.c     |  4 ++--
 drivers/gpu/drm/i915/i915_drv.h     |  1 -
 drivers/gpu/drm/i915/i915_gem.c     |  8 +++-----
 drivers/gpu/drm/i915/i915_gem_gtt.c | 21 +++++++++------------
 5 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index d35de1b..cd6e8b8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1377,7 +1377,8 @@ cleanup_gem:
 	i915_gem_cleanup_ringbuffer(dev);
 	i915_gem_context_fini(dev);
 	mutex_unlock(&dev->struct_mutex);
-	i915_gem_cleanup_aliasing_ppgtt(dev);
+	dev_priv->gtt.base.cleanup(&dev_priv->gtt.base);
+
 	drm_mm_takedown(&dev_priv->gtt.base.mm);
 cleanup_power:
 	intel_display_power_put(dev, POWER_DOMAIN_VGA);
@@ -1782,7 +1783,6 @@ int i915_driver_unload(struct drm_device *dev)
 		i915_gem_cleanup_ringbuffer(dev);
 		i915_gem_context_fini(dev);
 		mutex_unlock(&dev->struct_mutex);
-		i915_gem_cleanup_aliasing_ppgtt(dev);
 		i915_gem_cleanup_stolen(dev);
 
 		if (!I915_NEED_GFX_HWS(dev))
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8ebb0d1..cdb0875 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -761,7 +761,7 @@ int i915_reset(struct drm_device *dev)
 		if (dev_priv->mm.aliasing_ppgtt) {
 			ret = dev_priv->mm.aliasing_ppgtt->enable(dev);
 			if (ret)
-				i915_gem_cleanup_aliasing_ppgtt(dev);
+				DRM_ERROR("Failed to re-enable ppgtt.\n");
 		}
 
 		/*
@@ -779,7 +779,7 @@ int i915_reset(struct drm_device *dev)
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e8ffd57..14b9697 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2105,7 +2105,6 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file);
 
 /* i915_gem_gtt.c */
-void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev);
 void i915_gem_restore_gtt_mappings(struct drm_device *dev);
 int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj);
 void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0b2bc99..932994d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4414,8 +4414,8 @@ i915_gem_init_hw(struct drm_device *dev)
 	if (dev_priv->mm.aliasing_ppgtt) {
 		ret = dev_priv->mm.aliasing_ppgtt->enable(dev);
 		if (ret) {
-			i915_gem_cleanup_aliasing_ppgtt(dev);
-			DRM_INFO("PPGTT enable failed. This is not fatal, but unexpected\n");
+			DRM_ERROR("PPGTT enabling failed\n");
+			return ret;
 		}
 	}
 
@@ -4440,10 +4440,8 @@ int i915_gem_init(struct drm_device *dev)
 
 	ret = i915_gem_init_hw(dev);
 	mutex_unlock(&dev->struct_mutex);
-	if (ret) {
-		i915_gem_cleanup_aliasing_ppgtt(dev);
+	if (ret)
 		return ret;
-	}
 
 	/* Allow hardware batchbuffers unless told otherwise, but not for KMS. */
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index db39612..a4dd8ca 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -459,18 +459,6 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
 	return ret;
 }
 
-void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
-
-	if (!ppgtt)
-		return;
-
-	ppgtt->base.cleanup(&ppgtt->base);
-	dev_priv->mm.aliasing_ppgtt = NULL;
-}
-
 extern int intel_iommu_gfx_mapped;
 /* Certain Gen5 chipsets require require idling the GPU before
  * unmapping anything from the GTT when VT-d is enabled.
@@ -928,8 +916,17 @@ static void gen6_gmch_remove(struct i915_address_space *vm)
 {
 
 	struct i915_gtt *gtt = container_of(vm, struct i915_gtt, base);
+	struct drm_i915_private *dev_priv = vm->dev->dev_private;
+	struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
+
+	if (ppgtt) {
+		ppgtt->base.cleanup(&ppgtt->base);
+		dev_priv->mm.aliasing_ppgtt = NULL;
+	}
+
 	iounmap(gtt->gsm);
 	teardown_scratch_page(vm->dev);
+
 }
 
 static int i915_gmch_probe(struct drm_device *dev,
-- 
1.8.1.4

[PATCH 3/7] drm/i915/gtt: group sg setup/teardown functions to the end

[Daniel Vetter]

They don't really have a good excuse to sit in the middle of the
platform/generic gtt code.

No functional change in here, just code movement.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h     |   5 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c | 103 ++++++++++++++++++------------------
 2 files changed, 55 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 14b9697..666aedd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2106,8 +2106,6 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 
 /* i915_gem_gtt.c */
 void i915_gem_restore_gtt_mappings(struct drm_device *dev);
-int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj);
-void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj);
 void i915_gem_init_global_gtt(struct drm_device *dev);
 void i915_gem_setup_global_gtt(struct drm_device *dev, unsigned long start,
 			       unsigned long mappable_end, unsigned long end);
@@ -2118,6 +2116,9 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev)
 		intel_gtt_chipset_flush();
 }
 
+/* SG table setup/teardown */
+int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj);
+void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj);
 
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index a4dd8ca..9cbf70c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -475,28 +475,6 @@ static inline bool needs_idle_maps(struct drm_device *dev)
 	return false;
 }
 
-static bool do_idling(struct drm_i915_private *dev_priv)
-{
-	bool ret = dev_priv->mm.interruptible;
-
-	if (unlikely(dev_priv->gtt.do_idle_maps)) {
-		dev_priv->mm.interruptible = false;
-		if (i915_gpu_idle(dev_priv->dev)) {
-			DRM_ERROR("Couldn't idle GPU\n");
-			/* Wait a bit, in hopes it avoids the hang */
-			udelay(10);
-		}
-	}
-
-	return ret;
-}
-
-static void undo_idling(struct drm_i915_private *dev_priv, bool interruptible)
-{
-	if (unlikely(dev_priv->gtt.do_idle_maps))
-		dev_priv->mm.interruptible = interruptible;
-}
-
 void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -517,19 +495,6 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 	i915_gem_chipset_flush(dev);
 }
 
-int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
-{
-	if (obj->has_dma_mapping)
-		return 0;
-
-	if (!dma_map_sg(&obj->base.dev->pdev->dev,
-			obj->pages->sgl, obj->pages->nents,
-			PCI_DMA_BIDIRECTIONAL))
-		return -ENOSPC;
-
-	return 0;
-}
-
 /*
  * Binds an object into the global gtt with the specified cache level. The object
  * will be accessible to the GPU via commands whose operands reference offsets
@@ -672,22 +637,6 @@ static void gen6_ggtt_unbind_vma(struct i915_vma *vma)
 	}
 }
 
-void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
-{
-	struct drm_device *dev = obj->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	bool interruptible;
-
-	interruptible = do_idling(dev_priv);
-
-	if (!obj->has_dma_mapping)
-		dma_unmap_sg(&dev->pdev->dev,
-			     obj->pages->sgl, obj->pages->nents,
-			     PCI_DMA_BIDIRECTIONAL);
-
-	undo_idling(dev_priv, interruptible);
-}
-
 static void i915_gtt_color_adjust(struct drm_mm_node *node,
 				  unsigned long color,
 				  unsigned long *start,
@@ -999,3 +948,55 @@ int i915_gem_gtt_init(struct drm_device *dev)
 
 	return 0;
 }
+
+/* SG table prepare/finish functions. */
+static bool do_idling(struct drm_i915_private *dev_priv)
+{
+	bool ret = dev_priv->mm.interruptible;
+
+	if (unlikely(dev_priv->gtt.do_idle_maps)) {
+		dev_priv->mm.interruptible = false;
+		if (i915_gpu_idle(dev_priv->dev)) {
+			DRM_ERROR("Couldn't idle GPU\n");
+			/* Wait a bit, in hopes it avoids the hang */
+			udelay(10);
+		}
+	}
+
+	return ret;
+}
+
+static void undo_idling(struct drm_i915_private *dev_priv, bool interruptible)
+{
+	if (unlikely(dev_priv->gtt.do_idle_maps))
+		dev_priv->mm.interruptible = interruptible;
+}
+
+int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
+{
+	if (obj->has_dma_mapping)
+		return 0;
+
+	if (!dma_map_sg(&obj->base.dev->pdev->dev,
+			obj->pages->sgl, obj->pages->nents,
+			PCI_DMA_BIDIRECTIONAL))
+		return -ENOSPC;
+
+	return 0;
+}
+
+void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool interruptible;
+
+	interruptible = do_idling(dev_priv);
+
+	if (!obj->has_dma_mapping)
+		dma_unmap_sg(&dev->pdev->dev,
+			     obj->pages->sgl, obj->pages->nents,
+			     PCI_DMA_BIDIRECTIONAL);
+
+	undo_idling(dev_priv, interruptible);
+}
-- 
1.8.1.4

[PATCH 4/7] drm/i915/gtt: move restore_gtt_mappings to the bottom

[Daniel Vetter]

It's a little helper only used around for resume, so move it out
of all the setup code.

No functional change in here, just code movement.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 40 ++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 9cbf70c..560f24c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -475,26 +475,6 @@ static inline bool needs_idle_maps(struct drm_device *dev)
 	return false;
 }
 
-void i915_gem_restore_gtt_mappings(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *obj;
-
-	/* First fill our portion of the GTT with scratch pages */
-	dev_priv->gtt.base.clear_range(&dev_priv->gtt.base,
-				       dev_priv->gtt.base.start / PAGE_SIZE,
-				       dev_priv->gtt.base.total / PAGE_SIZE);
-
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
-		struct i915_vma *vma = i915_gem_obj_to_vma(obj,
-							   &dev_priv->gtt.base);
-		i915_gem_clflush_object(obj, obj->pin_display);
-		vma->vm->bind_vma(vma, obj->cache_level, 0);
-	}
-
-	i915_gem_chipset_flush(dev);
-}
-
 /*
  * Binds an object into the global gtt with the specified cache level. The object
  * will be accessible to the GPU via commands whose operands reference offsets
@@ -949,6 +929,26 @@ int i915_gem_gtt_init(struct drm_device *dev)
 	return 0;
 }
 
+void i915_gem_restore_gtt_mappings(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *obj;
+
+	/* First fill our portion of the GTT with scratch pages */
+	dev_priv->gtt.base.clear_range(&dev_priv->gtt.base,
+				       dev_priv->gtt.base.start / PAGE_SIZE,
+				       dev_priv->gtt.base.total / PAGE_SIZE);
+
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
+		struct i915_vma *vma = i915_gem_obj_to_vma(obj,
+							   &dev_priv->gtt.base);
+		i915_gem_clflush_object(obj, obj->pin_display);
+		vma->vm->bind_vma(vma, obj->cache_level, 0);
+	}
+
+	i915_gem_chipset_flush(dev);
+}
+
 /* SG table prepare/finish functions. */
 static bool do_idling(struct drm_i915_private *dev_priv)
 {
-- 
1.8.1.4

[PATCH 5/7] drm/i915/gtt: group legacy global gtt code together

[Daniel Vetter]

No functional change in here, just code movement.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 77 +++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 560f24c..095d902 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -459,28 +459,6 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
 	return ret;
 }
 
-extern int intel_iommu_gfx_mapped;
-/* Certain Gen5 chipsets require require idling the GPU before
- * unmapping anything from the GTT when VT-d is enabled.
- */
-static inline bool needs_idle_maps(struct drm_device *dev)
-{
-#ifdef CONFIG_INTEL_IOMMU
-	/* Query intel_iommu to see if we need the workaround. Presumably that
-	 * was loaded first.
-	 */
-	if (IS_GEN5(dev) && IS_MOBILE(dev) && intel_iommu_gfx_mapped)
-		return true;
-#endif
-	return false;
-}
-
-/*
- * Binds an object into the global gtt with the specified cache level. The object
- * will be accessible to the GPU via commands whose operands reference offsets
- * within the global GTT as well as accessible by the GPU through the GMADR
- * mapped BAR (dev_priv->mm.gtt->gtt).
- */
 static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
 				     struct sg_table *st,
 				     unsigned int first_entry,
@@ -539,25 +517,6 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
 }
 
 
-static void i915_ggtt_insert_entries(struct i915_address_space *vm,
-				     struct sg_table *st,
-				     unsigned int pg_start,
-				     enum i915_cache_level cache_level)
-{
-	unsigned int flags = (cache_level == I915_CACHE_NONE) ?
-		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
-
-	intel_gtt_insert_sg_entries(st, pg_start, flags);
-
-}
-
-static void i915_ggtt_clear_range(struct i915_address_space *vm,
-				  unsigned int first_entry,
-				  unsigned int num_entries)
-{
-	intel_gtt_clear_range(first_entry, num_entries);
-}
-
 static void gen6_ggtt_bind_vma(struct i915_vma *vma,
 			       enum i915_cache_level cache_level,
 			       u32 flags)
@@ -858,6 +817,42 @@ static void gen6_gmch_remove(struct i915_address_space *vm)
 
 }
 
+/* Legacy GTT support (using intel-gtt.c) */
+static void i915_ggtt_insert_entries(struct i915_address_space *vm,
+				     struct sg_table *st,
+				     unsigned int pg_start,
+				     enum i915_cache_level cache_level)
+{
+	unsigned int flags = (cache_level == I915_CACHE_NONE) ?
+		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
+
+	intel_gtt_insert_sg_entries(st, pg_start, flags);
+
+}
+
+static void i915_ggtt_clear_range(struct i915_address_space *vm,
+				  unsigned int first_entry,
+				  unsigned int num_entries)
+{
+	intel_gtt_clear_range(first_entry, num_entries);
+}
+
+extern int intel_iommu_gfx_mapped;
+/* Certain Gen5 chipsets require require idling the GPU before
+ * unmapping anything from the GTT when VT-d is enabled.
+ */
+static inline bool needs_idle_maps(struct drm_device *dev)
+{
+#ifdef CONFIG_INTEL_IOMMU
+	/* Query intel_iommu to see if we need the workaround. Presumably that
+	 * was loaded first.
+	 */
+	if (IS_GEN5(dev) && IS_MOBILE(dev) && intel_iommu_gfx_mapped)
+		return true;
+#endif
+	return false;
+}
+
 static int i915_gmch_probe(struct drm_device *dev,
 			   size_t *gtt_total,
 			   size_t *stolen,
-- 
1.8.1.4

[PATCH 6/7] drm/i915/gtt: move generic setup code all to the bottom

[Daniel Vetter]

... and splatter some header comments above the now nicely-extracted
platform support code.

No functional change in here, just code movement.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 243 ++++++++++++++++++------------------
 1 file changed, 124 insertions(+), 119 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 095d902..be374b9 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -67,10 +67,10 @@ static void generic_ggtt_unbind_vma(struct i915_vma *vma)
 			     vma->obj->base.size >> PAGE_SHIFT);
 }
 
+/* Gen6+ ppgtt code */
 #define GEN6_PPGTT_PD_ENTRIES 512
 #define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t))
 
-/* PPGTT stuff */
 #define GEN6_GTT_ADDR_ENCODE(addr)	((addr) | (((addr) >> 28) & 0xff0))
 #define HSW_GTT_ADDR_ENCODE(addr)	((addr) | (((addr) >> 28) & 0x7f0))
 
@@ -431,6 +431,7 @@ err_pt_alloc:
 	return ret;
 }
 
+/* PPGTT Setup code. */
 static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -459,6 +460,7 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
 	return ret;
 }
 
+/* Gen6+ global GTT code. */
 static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
 				     struct sg_table *st,
 				     unsigned int first_entry,
@@ -576,124 +578,6 @@ static void gen6_ggtt_unbind_vma(struct i915_vma *vma)
 	}
 }
 
-static void i915_gtt_color_adjust(struct drm_mm_node *node,
-				  unsigned long color,
-				  unsigned long *start,
-				  unsigned long *end)
-{
-	if (node->color != color)
-		*start += 4096;
-
-	if (!list_empty(&node->node_list)) {
-		node = list_entry(node->node_list.next,
-				  struct drm_mm_node,
-				  node_list);
-		if (node->allocated && node->color != color)
-			*end -= 4096;
-	}
-}
-void i915_gem_setup_global_gtt(struct drm_device *dev,
-			       unsigned long start,
-			       unsigned long mappable_end,
-			       unsigned long end)
-{
-	/* Let GEM Manage all of the aperture.
-	 *
-	 * However, leave one page at the end still bound to the scratch page.
-	 * There are a number of places where the hardware apparently prefetches
-	 * past the end of the object, and we've seen multiple hangs with the
-	 * GPU head pointer stuck in a batchbuffer bound at the last page of the
-	 * aperture.  One page should be enough to keep any prefetching inside
-	 * of the aperture.
-	 */
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_address_space *ggtt_vm = &dev_priv->gtt.base;
-	struct drm_mm_node *entry;
-	struct drm_i915_gem_object *obj;
-	unsigned long hole_start, hole_end;
-
-	BUG_ON(mappable_end > end);
-
-	/* Subtract the guard page ... */
-	drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
-	if (!HAS_LLC(dev))
-		dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
-
-	/* Mark any preallocated objects as occupied */
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
-		struct i915_vma *vma = i915_gem_obj_to_vma(obj, ggtt_vm);
-		int ret;
-		DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
-			      i915_gem_obj_ggtt_offset(obj), obj->base.size);
-
-		WARN_ON(i915_gem_obj_ggtt_bound(obj));
-		ret = drm_mm_reserve_node(&ggtt_vm->mm, &vma->node);
-		if (ret)
-			DRM_DEBUG_KMS("Reservation failed\n");
-		obj->has_global_gtt_mapping = 1;
-		list_add(&vma->vma_link, &obj->vma_list);
-	}
-
-	dev_priv->gtt.base.start = start;
-	dev_priv->gtt.base.total = end - start;
-
-	/* Clear any non-preallocated blocks */
-	drm_mm_for_each_hole(entry, &ggtt_vm->mm, hole_start, hole_end) {
-		const unsigned long count = (hole_end - hole_start) / PAGE_SIZE;
-		DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
-			      hole_start, hole_end);
-		ggtt_vm->clear_range(ggtt_vm, hole_start / PAGE_SIZE, count);
-	}
-
-	/* And finally clear the reserved guard page */
-	ggtt_vm->clear_range(ggtt_vm, end / PAGE_SIZE - 1, 1);
-}
-
-static bool
-intel_enable_ppgtt(struct drm_device *dev)
-{
-	if (i915_enable_ppgtt >= 0)
-		return i915_enable_ppgtt;
-
-#ifdef CONFIG_INTEL_IOMMU
-	/* Disable ppgtt on SNB if VT-d is on. */
-	if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped)
-		return false;
-#endif
-
-	return true;
-}
-
-void i915_gem_init_global_gtt(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long gtt_size, mappable_size;
-
-	gtt_size = dev_priv->gtt.base.total;
-	mappable_size = dev_priv->gtt.mappable_end;
-
-	if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) {
-		int ret;
-
-		if (INTEL_INFO(dev)->gen <= 7) {
-			/* PPGTT pdes are stolen from global gtt ptes, so shrink the
-			 * aperture accordingly when using aliasing ppgtt. */
-			gtt_size -= GEN6_PPGTT_PD_ENTRIES * PAGE_SIZE;
-		}
-
-		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
-
-		ret = i915_gem_init_aliasing_ppgtt(dev);
-		if (!ret)
-			return;
-
-		DRM_ERROR("Aliased PPGTT setup failed %d\n", ret);
-		drm_mm_takedown(&dev_priv->gtt.base.mm);
-		gtt_size += GEN6_PPGTT_PD_ENTRIES * PAGE_SIZE;
-	}
-	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
-}
-
 static int setup_scratch_page(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -884,6 +768,127 @@ static void i915_gmch_remove(struct i915_address_space *vm)
 	intel_gmch_remove();
 }
 
+/* Global GTT setup functions */
+static void i915_gtt_color_adjust(struct drm_mm_node *node,
+				  unsigned long color,
+				  unsigned long *start,
+				  unsigned long *end)
+{
+	if (node->color != color)
+		*start += 4096;
+
+	if (!list_empty(&node->node_list)) {
+		node = list_entry(node->node_list.next,
+				  struct drm_mm_node,
+				  node_list);
+		if (node->allocated && node->color != color)
+			*end -= 4096;
+	}
+}
+
+/* Also called in UMS mode through the gem_init ioctl. */
+void i915_gem_setup_global_gtt(struct drm_device *dev,
+			       unsigned long start,
+			       unsigned long mappable_end,
+			       unsigned long end)
+{
+	/* Let GEM Manage all of the aperture.
+	 *
+	 * However, leave one page at the end still bound to the scratch page.
+	 * There are a number of places where the hardware apparently prefetches
+	 * past the end of the object, and we've seen multiple hangs with the
+	 * GPU head pointer stuck in a batchbuffer bound at the last page of the
+	 * aperture.  One page should be enough to keep any prefetching inside
+	 * of the aperture.
+	 */
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_address_space *ggtt_vm = &dev_priv->gtt.base;
+	struct drm_mm_node *entry;
+	struct drm_i915_gem_object *obj;
+	unsigned long hole_start, hole_end;
+
+	BUG_ON(mappable_end > end);
+
+	/* Subtract the guard page ... */
+	drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
+	if (!HAS_LLC(dev))
+		dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
+
+	/* Mark any preallocated objects as occupied */
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
+		struct i915_vma *vma = i915_gem_obj_to_vma(obj, ggtt_vm);
+		int ret;
+		DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
+			      i915_gem_obj_ggtt_offset(obj), obj->base.size);
+
+		WARN_ON(i915_gem_obj_ggtt_bound(obj));
+		ret = drm_mm_reserve_node(&ggtt_vm->mm, &vma->node);
+		if (ret)
+			DRM_DEBUG_KMS("Reservation failed\n");
+		obj->has_global_gtt_mapping = 1;
+		list_add(&vma->vma_link, &obj->vma_list);
+	}
+
+	dev_priv->gtt.base.start = start;
+	dev_priv->gtt.base.total = end - start;
+
+	/* Clear any non-preallocated blocks */
+	drm_mm_for_each_hole(entry, &ggtt_vm->mm, hole_start, hole_end) {
+		const unsigned long count = (hole_end - hole_start) / PAGE_SIZE;
+		DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
+			      hole_start, hole_end);
+		ggtt_vm->clear_range(ggtt_vm, hole_start / PAGE_SIZE, count);
+	}
+
+	/* And finally clear the reserved guard page */
+	ggtt_vm->clear_range(ggtt_vm, end / PAGE_SIZE - 1, 1);
+}
+
+static bool
+intel_enable_ppgtt(struct drm_device *dev)
+{
+	if (i915_enable_ppgtt >= 0)
+		return i915_enable_ppgtt;
+
+#ifdef CONFIG_INTEL_IOMMU
+	/* Disable ppgtt on SNB if VT-d is on. */
+	if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped)
+		return false;
+#endif
+
+	return true;
+}
+
+void i915_gem_init_global_gtt(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned long gtt_size, mappable_size;
+
+	gtt_size = dev_priv->gtt.base.total;
+	mappable_size = dev_priv->gtt.mappable_end;
+
+	if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) {
+		int ret;
+
+		if (INTEL_INFO(dev)->gen <= 7) {
+			/* PPGTT pdes are stolen from global gtt ptes, so shrink the
+			 * aperture accordingly when using aliasing ppgtt. */
+			gtt_size -= GEN6_PPGTT_PD_ENTRIES * PAGE_SIZE;
+		}
+
+		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
+
+		ret = i915_gem_init_aliasing_ppgtt(dev);
+		if (!ret)
+			return;
+
+		DRM_ERROR("Aliased PPGTT setup failed %d\n", ret);
+		drm_mm_takedown(&dev_priv->gtt.base.mm);
+		gtt_size += GEN6_PPGTT_PD_ENTRIES * PAGE_SIZE;
+	}
+	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
+}
+
 int i915_gem_gtt_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.8.1.4

[PATCH 7/7] drm/i915: we can't call move_to_active before intel_ring_begin

[Daniel Vetter]

Otherwise we can die in a fire of not-yet-allocated lazy requests when
we expect them to be there:

[ 4405.463113] ------------[ cut here ]------------
[ 4405.464769] kernel BUG at /home/user/src/linux/drivers/gpu/drm/i915/intel_ringbuffer.h:268!
[ 4405.466392] invalid opcode: 0000 [#1] PREEMPT SMP
[ 4405.468007] Modules linked in: snd_hda_codec_hdmi hid_generic snd_hda_codec_realtek usbhid hid iTCO_wdt snd_hda_intel psmouse i2c_i801 snd_hda_codec pcspkr snd_hwdep i915 snd_pcm snd_page_alloc snd_timer cfbfillrect snd cfbimgblt lpc_ich cfbcopyarea soundcore drm_kms_helper mfd_core evdev wmi
[ 4405.471387] CPU: 5 PID: 9300 Comm: gem_storedw_bat Not tainted 3.12.0-rc2-drm_vbl+ #1
[ 4405.473047] Hardware name:                  /DZ77BH-55K, BIOS BHZ7710H.86A.0100.2013.0517.0942 05/17/2013
[ 4405.474712] task: ffff8800618d4b00 ti: ffff88010a806000 task.ti: ffff88010a806000
[ 4405.476370] RIP: 0010:[<ffffffffa009ffa9>]  [<ffffffffa009ffa9>] i915_vma_move_to_active+0x1b9/0x1e0 [i915]
[ 4405.478045] RSP: 0018:ffff88010a807be8  EFLAGS: 00010246
[ 4405.479689] RAX: ffff88011a681000 RBX: ffff8800b364f9c0 RCX: ffff88011902b898
[ 4405.481321] RDX: ffff8800d4a1b6e0 RSI: ffff8800d4a1a8b8 RDI: ffff88011902b840
[ 4405.482932] RBP: ffff88010a807c08 R08: 0000000000000001 R09: 0000000000000000
[ 4405.484526] R10: 0000000000000001 R11: 0000000000000000 R12: ffff8800d4a1a8b8
[ 4405.486100] R13: 0000000000000000 R14: ffff8800d4a18000 R15: ffff8800b364f9c0
[ 4405.487664] FS:  00007f36c1a738c0(0000) GS:ffff88011f340000(0000) knlGS:0000000000000000
[ 4405.489216] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 4405.490747] CR2: 00007fff1b28ea30 CR3: 0000000119e0d000 CR4: 00000000001407e0
[ 4405.492276] Stack:
[ 4405.493774]  ffff88010a807dd8 ffff8800d4a1a8b8 ffff8800d3c1c400 ffffffffa00ac060
[ 4405.495276]  ffff88010a807d28 ffffffffa00aa0db ffff88010a807cb8 ffffffff810aa4e4
[ 4405.496776]  0000000000000003 ffff880000000001 ffff8800618d50e8 ffffffff81a9da00
[ 4405.498265] Call Trace:
[ 4405.499735]  [<ffffffffa00ac060>] ? gen6_ppgtt_insert_entries+0x1a0/0x1a0 [i915]
[ 4405.501218]  [<ffffffffa00aa0db>] i915_gem_do_execbuffer.clone.18+0x65b/0x12e0 [i915]
[ 4405.502685]  [<ffffffff810aa4e4>] ? lock_release_non_nested+0xa4/0x360
[ 4405.504134]  [<ffffffffa00ab298>] i915_gem_execbuffer2+0xa8/0x290 [i915]
[ 4405.505573]  [<ffffffff813552b9>] drm_ioctl+0x419/0x5c0
[ 4405.506991]  [<ffffffff81128b12>] ? handle_mm_fault+0x352/0xa00
[ 4405.508399]  [<ffffffffa00ab1f0>] ? i915_gem_execbuffer+0x490/0x490 [i915]
[ 4405.509792]  [<ffffffff8103cd2c>] ? __do_page_fault+0x1fc/0x4b0
[ 4405.511170]  [<ffffffff81165e66>] do_vfs_ioctl+0x96/0x560
[ 4405.512533]  [<ffffffff81512163>] ? error_sti+0x5/0x6
[ 4405.513878]  [<ffffffff81511d0d>] ? retint_swapgs+0xe/0x13
[ 4405.515208]  [<ffffffff811663d1>] SyS_ioctl+0xa1/0xb0
[ 4405.516522]  [<ffffffff8129ddee>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 4405.517830]  [<ffffffff81512792>] system_call_fastpath+0x16/0x1b
[ 4405.519119] Code: f6 83 40 01 00 00 01 0f 85 11 ff ff ff b8 01 00 00 00 f0 0f c1 03 ff c0 83 f8 01 7e 10 80 8b 40 01 00 00 01 e9 f5 fe ff ff 0f 0b <0f> 0b 80 3d 61 55 08 00 01 74 e7 be 2f 00 00 00 48 c7 c7 18 03
[ 4405.520610] RIP  [<ffffffffa009ffa9>] i915_vma_move_to_active+0x1b9/0x1e0 [i915]
[ 4405.522001]  RSP <ffff88010a807be8>
[ 4405.523367] [drm:intel_crtc_set_config], [CRTC:3] [FB:50] #connectors=1 (x y) (0 0)
[ 4405.524751] [drm:intel_set_config_compute_mode_changes], computed changes for [CRTC:3], mode_changed=0, fb_changed=0
[ 4405.526152] [drm:intel_modeset_stage_output_state], [CONNECTOR:16:HDMI-A-2] to [CRTC:3]
[ 4405.531096] [drm:intel_crtc_set_config], [CRTC:3] [FB:50] #connectors=1 (x y) (0 0)
[ 4405.532491] [drm:intel_set_config_compute_mode_changes], computed changes for [CRTC:3], mode_changed=0, fb_changed=0
[ 4405.533903] [drm:intel_modeset_stage_output_state], [CONNECTOR:16:HDMI-A-2] to [CRTC:3]
[ 4405.538888] ---[ end trace 53d1b708421bb5b3 ]---

This regression has been introduced in from Ben Widawsky's ppgtt/vma
enabling patch "drm/i915: Use the new vm [un]bind functions".

This should be exercised by
igt/gem_storedw_batches_loop/secure-dispatch.

Note that this won't fix the full ordeal for real ppgtt since the
potential allocation for the batch vma could recurse into the shrinker
and wreak utter havoc with our carefully reserved buffers. So just to
the minimal fixup of restoreing the global gtt binding for aliasing
ppgtt - for now this is good enough.

The real fix involves some trickery to allocate the batch vma around
the call to eb_lookup_vmas and do all the additional buffer space
reserving for the batch in global gtt in the normal reservation step.

But that requires quite some frobbing of various pieces of code, so
definitely a 2nd step.

v2: Clarify the copy&pasta comment and update it to suit the new
location of the move_to_active call for the batch vma.

v3: Pimp the commit message a bit to explain that this isn't the full
fix.

v4: Simplify the fix to the bare minimum.

Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 40 ++++++++----------------------
 1 file changed, 11 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 88c924f..b6709d5 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1111,38 +1111,20 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	}
 	batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
 
-	/* snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure
+	/*
+	 * snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure
 	 * batch" bit. Hence we need to pin secure batches into the global gtt.
 	 * hsw should have this fixed, but let's be paranoid and do it
-	 * unconditionally for now. */
-	if (flags & I915_DISPATCH_SECURE) {
-		struct i915_address_space *ggtt = obj_to_ggtt(batch_obj);
-
-		/* Assuming all privileged batches are in the global GTT means
-		 * we need to make sure we have a global gtt offset, as well as
-		 * the PTEs mapped. As mentioned above, we can forego this on
-		 * HSW, but don't.
-		 */
-
-		ret = i915_gem_obj_ggtt_pin(batch_obj, 0, false, false);
-		if (ret)
-			goto err;
-
-		ggtt->bind_vma(i915_gem_obj_to_ggtt(batch_obj),
-			       batch_obj->cache_level,
-			       GLOBAL_BIND);
-
-		/* Since the active list is per VM, we need to make sure this
-		 * VMA ends up on the GGTT's active list to avoid premature
-		 * eviction.
-		 */
-		i915_vma_move_to_active(i915_gem_obj_to_ggtt(batch_obj), ring);
-
-		i915_gem_object_unpin(batch_obj);
+	 * unconditionally for now.
+	 *
+	 * FIXME: This won't work for real ppgtt.
+	 */
+	if (flags & I915_DISPATCH_SECURE)
+		vm->bind_vma(i915_gem_obj_to_ggtt(batch_obj),
+			     batch_obj->cache_level,
+			     GLOBAL_BIND);
 
-		exec_start += i915_gem_obj_ggtt_offset(batch_obj);
-	} else
-		exec_start += i915_gem_obj_offset(batch_obj, vm);
+	exec_start += i915_gem_obj_offset(batch_obj, vm);
 
 	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->vmas);
 	if (ret)
-- 
1.8.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/7] gtt patches

[Ben Widawsky]

On Thu, Sep 26, 2013 at 10:31:26PM +0200, Daniel Vetter wrote:
> Hi all,
> 
> So the first part reworks the gtt ->bind/unbind stuff so that I can rebase the
> -internal try without going berserk. 2nd patch is a bit of fluff removal, next
> for do reorder i915_gem_gtt.c a bit to make more sense (I've been confused every
> 2nd time I've had to rebase -internal and there was a conflict in there, call me
> stupid ...). Last one is my ducttape for the BUG_ON Ville hit.
> 
> Cheers, Daniel
> 
> Daniel Vetter (7):
>   drm/i915: resurrect ->insert_entries/->clear_range gtt interfaces
>   drm/i915: don't allow ppgtt enabling to fail anymore
>   drm/i915/gtt: group sg setup/teardown functions to the end
>   drm/i915/gtt: move restore_gtt_mappings to the bottom
>   drm/i915/gtt: group legacy global gtt code together
>   drm/i915/gtt: move generic setup code all to the bottom
>   drm/i915: we can't call move_to_active before intel_ring_begin
> 
>  drivers/gpu/drm/i915/i915_dma.c            |   4 +-
>  drivers/gpu/drm/i915/i915_drv.c            |   4 +-
>  drivers/gpu/drm/i915/i915_drv.h            |   6 +-
>  drivers/gpu/drm/i915/i915_gem.c            |   8 +-
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  40 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c        | 683 ++++++++++++++---------------
>  6 files changed, 357 insertions(+), 388 deletions(-)
> 

At this point it just seems like you're intentionally making it harder
for me to ever merge PPGTT.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 0/7] gtt patches

[Daniel Vetter]

On Fri, Sep 27, 2013 at 7:34 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> At this point it just seems like you're intentionally making it harder
> for me to ever merge PPGTT.

I have two issues with the merged patches:
1. There's a regression, and QA is meanwhile at the 3rd or so dupe
report. So it's not really an arcane corner case. And I've
specifically written a testcase for secure batch dispatching and
specifically asked you to test to make sure it catches the bugs we've
discussed, so I hope you understand I'm a bit underwhelmed that this
slept through.

2. I have a bit an issue with the currently merged code for rebasing
-internal. I only stumbled over that when I've tried to rebase
-internal and was a bit disappointed to see that despite that I've
raised this the first time your vm->bind/unbind patches showed up
nothing changed.

That's the first&last patch.

The stuff in-between is to make rebasing -internal a bit easier (while
I need to do fixups anyway) since I really botched this 1-2 times
everytime there was a conflict. I've thought that the oustanding stuff
from your series only needs to touch the ->enable callbacks in
i915_gem_gtt.c. A quick look at your ppgtt branches shows that in
addition to that there's only a now outdated cleanup patch and a
rather self-contained debugfs dumper on top. So my thinking was that
right now is an ideal time to polish i915_gem_gtt.c a bit.

But of course I'll drop cleanup patches when they conflict badly with
ongoing stuff, like I've done a few times already. But it didn't look
like this is the case here.

Now you seem to reject my patches, but I don't see any alternate
proposals from you. Furthermore to me it feels a bit the discussion
has derailed into non-constructive form a bit, so I guess this will
take a bit of time to resolve. Since I can't just hold public and
internal trees hostage until that's settled I'll drop your two vma
patches meanwhile.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/7] gtt patches

[Ben Widawsky]

On Fri, Sep 27, 2013 at 09:21:51PM +0200, Daniel Vetter wrote:
> On Fri, Sep 27, 2013 at 7:34 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > At this point it just seems like you're intentionally making it harder
> > for me to ever merge PPGTT.
> 
> I have two issues with the merged patches:
> 1. There's a regression, and QA is meanwhile at the 3rd or so dupe
> report. So it's not really an arcane corner case. And I've
> specifically written a testcase for secure batch dispatching and
> specifically asked you to test to make sure it catches the bugs we've
> discussed, so I hope you understand I'm a bit underwhelmed that this
> slept through.
> 
> 2. I have a bit an issue with the currently merged code for rebasing
> -internal. I only stumbled over that when I've tried to rebase
> -internal and was a bit disappointed to see that despite that I've
> raised this the first time your vm->bind/unbind patches showed up
> nothing changed.
> 
> That's the first&last patch.
> 
> The stuff in-between is to make rebasing -internal a bit easier (while
> I need to do fixups anyway) since I really botched this 1-2 times
> everytime there was a conflict. I've thought that the oustanding stuff
> from your series only needs to touch the ->enable callbacks in
> i915_gem_gtt.c. A quick look at your ppgtt branches shows that in
> addition to that there's only a now outdated cleanup patch and a
> rather self-contained debugfs dumper on top. So my thinking was that
> right now is an ideal time to polish i915_gem_gtt.c a bit.
> 
> But of course I'll drop cleanup patches when they conflict badly with
> ongoing stuff, like I've done a few times already. But it didn't look
> like this is the case here.
> 
> Now you seem to reject my patches, but I don't see any alternate
> proposals from you. Furthermore to me it feels a bit the discussion
> has derailed into non-constructive form a bit, so I guess this will
> take a bit of time to resolve. Since I can't just hold public and
> internal trees hostage until that's settled I'll drop your two vma
> patches meanwhile.
> 
> Cheers, Daniel

I do not plan to develop PPGTT any further. Please feel free to revert
as many patches as you'd like.

-- 
Ben Widawsky, Intel Open Source Technology Center