[PATCH v2 0/4] Ironlake mobile GPU with VT-d, fix

[PATCH v2 0/4] Ironlake mobile GPU with VT-d, fix

[Ben Widawsky]

v1:
Idle GPU before and after unmaps.

v2: (untested)
Read GMCH_CTRL to find if VT-d is enable
Pulled David's patch into the series
Only stall for unmaps
Add warning for list_empty && !rings_idle

Ben Widawsky (3):
  drm/i915: add warning for unidle GPU
  drm/i915: Ironlake mobile GPU with VT-d fix
  drm/i915: add unidle warning back

David Woodhouse (1):
  intel-iommu: IOTLB hang workaround

 drivers/char/agp/intel-gtt.c        |   16 ++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c     |   33 ++++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_gem_gtt.c |   39 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h     |    1 +
 drivers/iommu/intel-iommu.c         |    6 ++++-
 include/drm/intel-gtt.h             |    2 +
 6 files changed, 86 insertions(+), 11 deletions(-)

-- 
1.7.6.3

[PATCH v2 1/4] drm/i915: add warning for unidle GPU

[Ben Widawsky]

When we attempt to idle the GPU we take a shortcut by checking if our
lists are empty and if so, avoid doing a synchronizing flush. The
warning will let us know if this assumption turns out to not be correct.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c |    3 ++-
 drivers/gpu/drm/i915/i915_reg.h |    1 +
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a546a71..61ce1b7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2277,7 +2277,8 @@ i915_gpu_idle(struct drm_device *dev)
 
 	lists_empty = (list_empty(&dev_priv->mm.flushing_list) &&
 		       list_empty(&dev_priv->mm.active_list));
-	if (lists_empty)
+	if (lists_empty) {
+		WARN_ON(!(I915_READ(MI_MODE) & MI_RINGS_IDLE));
 		return 0;
 
 	/* Flush everything onto the inactive list. */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 542453f..68af365 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -372,6 +372,7 @@
 
 #define MI_MODE		0x0209c
 # define VS_TIMER_DISPATCH				(1 << 6)
+# define MI_RINGS_IDLE					(1 << 9)
 # define MI_FLUSH_ENABLE				(1 << 11)
 
 #define GFX_MODE	0x02520
-- 
1.7.6.3

[PATCH v2 2/4] drm/i915: Ironlake mobile GPU with VT-d fix

[Ben Widawsky]

Under certain circumstances, an IOTLB flush never completes and the hardware
just locks up.  The fix is meant to idle the GPU both before and after
any unmap occurs.

This patch also disables the warning which is meant to detect mismatches
between when we think the command parse and arbiter should be idle, and
when it actually is.

Cc: Dave Airlie <airlied@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Keith Packard <keithp@keithp.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/char/agp/intel-gtt.c        |   16 ++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c     |   32 +++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_gem_gtt.c |   39 +++++++++++++++++++++++++++++++++++
 include/drm/intel-gtt.h             |    2 +
 4 files changed, 79 insertions(+), 10 deletions(-)

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index 8515101..47ca42f 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -923,6 +923,9 @@ static int intel_fake_agp_insert_entries(struct agp_memory *mem,
 {
 	int ret = -EINVAL;
 
+	if (intel_private.base.do_idle_maps)
+		return -ENODEV;
+
 	if (intel_private.clear_fake_agp) {
 		int start = intel_private.base.stolen_size / PAGE_SIZE;
 		int end = intel_private.base.gtt_mappable_entries;
@@ -985,6 +988,9 @@ static int intel_fake_agp_remove_entries(struct agp_memory *mem,
 	if (mem->page_count == 0)
 		return 0;
 
+	if (intel_private.base.do_idle_maps)
+		return -ENODEV;
+
 	intel_gtt_clear_range(pg_start, mem->page_count);
 
 	if (intel_private.base.needs_dmar) {
@@ -1180,8 +1186,12 @@ static void gen6_cleanup(void)
 static int i9xx_setup(void)
 {
 	u32 reg_addr;
+	u16 gmch_ctrl;
+	const unsigned short gpu_devid = intel_private.pcidev->device;
 
 	pci_read_config_dword(intel_private.pcidev, I915_MMADDR, &reg_addr);
+	pci_read_config_word(intel_private.bridge_dev,
+			     I830_GMCH_CTRL, &gmch_ctrl);
 
 	reg_addr &= 0xfff80000;
 
@@ -1211,6 +1221,12 @@ static int i9xx_setup(void)
 		intel_private.gtt_bus_addr = reg_addr + gtt_offset;
 	}
 
+	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
+	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
+	     (gmch_ctrl & G4x_GMCH_SIZE_VT_EN) &&
+	     intel_private.base.needs_dmar)
+		intel_private.base.do_idle_maps = 1;
+
 	intel_i9xx_setup_flush();
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 61ce1b7..d369e48 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2268,27 +2268,39 @@ static int i915_ring_idle(struct intel_ring_buffer *ring)
 	return i915_wait_request(ring, i915_gem_next_request_seqno(ring));
 }
 
+static int
+flush_rings(drm_i915_private_t *dev_priv)
+{
+	int i, ret;
+
+	/* Flush everything onto the inactive list. */
+	for (i = 0; i < I915_NUM_RINGS; i++) {
+		ret = i915_ring_idle(&dev_priv->ring[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 int
 i915_gpu_idle(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	bool lists_empty;
-	int ret, i;
 
 	lists_empty = (list_empty(&dev_priv->mm.flushing_list) &&
 		       list_empty(&dev_priv->mm.active_list));
 	if (lists_empty) {
-		WARN_ON(!(I915_READ(MI_MODE) & MI_RINGS_IDLE));
-		return 0;
-
-	/* Flush everything onto the inactive list. */
-	for (i = 0; i < I915_NUM_RINGS; i++) {
-		ret = i915_ring_idle(&dev_priv->ring[i]);
-		if (ret)
-			return ret;
+		/* If we require idle maps, enforce the ring is idle */
+		bool ring_idle = (I915_READ(MI_MODE) & MI_RINGS_IDLE) != 0;
+		if (!ring_idle && dev_priv->mm.gtt->do_idle_maps)
+			return flush_rings(dev_priv);
+		else
+			return 0;
 	}
 
-	return 0;
+	return flush_rings(dev_priv);
 }
 
 static int sandybridge_write_fence_reg(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7a709cd..0c6226b 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -49,10 +49,39 @@ static unsigned int cache_level_to_agp_type(struct drm_device *dev,
 	}
 }
 
+static bool do_idling(struct drm_i915_private *dev_priv)
+{
+	bool ret = dev_priv->mm.interruptible;
+
+	if (dev_priv->mm.gtt->do_idle_maps) {
+		dev_priv->mm.interruptible = false;
+		if (i915_gpu_idle(dev_priv->dev))
+			DRM_ERROR("couldn't idle GPU %d\n", ret);
+	}
+
+	return ret;
+}
+
+static void undo_idling(struct drm_i915_private *dev_priv, bool idle)
+{
+	int ret;
+	if (!dev_priv->mm.gtt->do_idle_maps)
+		return;
+
+	/*NB: since GTT mappings don't actually touch the GPU, this is not
+	 * strictly necessary.
+	 */
+	ret = i915_gpu_idle(dev_priv->dev);
+	if (ret)
+		DRM_ERROR("couldn't idle GPU %d\n", ret);
+	dev_priv->mm.interruptible = idle;
+}
+
 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;
+	bool idle;
 
 	/* First fill our portion of the GTT with scratch pages */
 	intel_gtt_clear_range(dev_priv->mm.gtt_start / PAGE_SIZE,
@@ -71,6 +100,7 @@ int i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj)
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned int agp_type = cache_level_to_agp_type(dev, obj->cache_level);
+	bool idle;
 	int ret;
 
 	if (dev_priv->mm.gtt->needs_dmar) {
@@ -100,6 +130,7 @@ void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned int agp_type = cache_level_to_agp_type(dev, cache_level);
+	bool idle;
 
 	if (dev_priv->mm.gtt->needs_dmar) {
 		BUG_ON(!obj->sg_list);
@@ -117,6 +148,12 @@ void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
 
 void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
 {
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool idle;
+
+	idle = do_idling(dev_priv);
+
 	intel_gtt_clear_range(obj->gtt_space->start >> PAGE_SHIFT,
 			      obj->base.size >> PAGE_SHIFT);
 
@@ -124,4 +161,6 @@ void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
 		intel_gtt_unmap_memory(obj->sg_list, obj->num_sg);
 		obj->sg_list = NULL;
 	}
+
+	undo_idling(dev_priv, idle);
 }
diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h
index 9e343c0..b174620 100644
--- a/include/drm/intel-gtt.h
+++ b/include/drm/intel-gtt.h
@@ -13,6 +13,8 @@ const struct intel_gtt {
 	unsigned int gtt_mappable_entries;
 	/* Whether i915 needs to use the dmar apis or not. */
 	unsigned int needs_dmar : 1;
+	/* Whether we idle the gpu before mapping/unmapping */
+	unsigned int do_idle_maps : 1;
 } *intel_gtt_get(void);
 
 void intel_gtt_chipset_flush(void);
-- 
1.7.6.3

[PATCH v2 3/4] intel-iommu: IOTLB hang workaround

[Ben Widawsky]

From: David Woodhouse <dwmw2@infradead.org>

To work around a hardware issue, we have to submit IOTLB flushes while
the graphics engine is idle. The graphics driver will go to great
lengths to ensure that it gets that right on the affected chipset(s)...
so let's not screw it over by deferring the unmap and doing it later.
That wouldn't be very helpful.

Cc: Dave Airlie <airlied@redhat.com>
Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/iommu/intel-iommu.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index c621c98..dcdc3c7 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3950,7 +3950,11 @@ static void __devinit quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
 	if (!(ggc & GGC_MEMORY_VT_ENABLED)) {
 		printk(KERN_INFO "DMAR: BIOS has allocated no shadow GTT; disabling IOMMU for graphics\n");
 		dmar_map_gfx = 0;
-	}
+	} else {
+		/* we have to ensure the gfx device is idle before we flush */
+		printk(KERN_INFO "DMAR: Disabling batched IOTLB flush on Ironlake\n");
+		intel_iommu_strict = 1;
+       }
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0040, quirk_calpella_no_shadow_gtt);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0044, quirk_calpella_no_shadow_gtt);
-- 
1.7.6.3

[PATCH v2 4/4] drm/i915: add unidle warning back

[Ben Widawsky]

The warning really does belong there even though it is not part of the
workaround. Once we rootcause how our lists and HW get out of sync, this
patch should be sucked back in.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d369e48..8aa84a4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2294,7 +2294,7 @@ i915_gpu_idle(struct drm_device *dev)
 	if (lists_empty) {
 		/* If we require idle maps, enforce the ring is idle */
 		bool ring_idle = (I915_READ(MI_MODE) & MI_RINGS_IDLE) != 0;
-		if (!ring_idle && dev_priv->mm.gtt->do_idle_maps)
+		if (WARN_ON(!ring_idle) && dev_priv->mm.gtt->do_idle_maps)
 			return flush_rings(dev_priv);
 		else
 			return 0;
-- 
1.7.6.3

[PATCH v2 2/4] drm/i915: Ironlake mobile GPU with VT-d fix

[Ben Widawsky]

Under certain circumstances, an IOTLB flush never completes and the hardware
just locks up.  The fix is meant to idle the GPU both before and after
any unmap occurs.

This patch also disables the warning which is meant to detect mismatches
between when we think the command parse and arbiter should be idle, and
when it actually is.

Cc: Dave Airlie <airlied@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Keith Packard <keithp@keithp.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/char/agp/intel-gtt.c        |   16 +++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c     |   32 +++++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_gem_gtt.c |   36 +++++++++++++++++++++++++++++++++++
 include/drm/intel-gtt.h             |    2 +
 4 files changed, 76 insertions(+), 10 deletions(-)

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index 8515101..47ca42f 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -923,6 +923,9 @@ static int intel_fake_agp_insert_entries(struct agp_memory *mem,
 {
 	int ret = -EINVAL;
 
+	if (intel_private.base.do_idle_maps)
+		return -ENODEV;
+
 	if (intel_private.clear_fake_agp) {
 		int start = intel_private.base.stolen_size / PAGE_SIZE;
 		int end = intel_private.base.gtt_mappable_entries;
@@ -985,6 +988,9 @@ static int intel_fake_agp_remove_entries(struct agp_memory *mem,
 	if (mem->page_count == 0)
 		return 0;
 
+	if (intel_private.base.do_idle_maps)
+		return -ENODEV;
+
 	intel_gtt_clear_range(pg_start, mem->page_count);
 
 	if (intel_private.base.needs_dmar) {
@@ -1180,8 +1186,12 @@ static void gen6_cleanup(void)
 static int i9xx_setup(void)
 {
 	u32 reg_addr;
+	u16 gmch_ctrl;
+	const unsigned short gpu_devid = intel_private.pcidev->device;
 
 	pci_read_config_dword(intel_private.pcidev, I915_MMADDR, &reg_addr);
+	pci_read_config_word(intel_private.bridge_dev,
+			     I830_GMCH_CTRL, &gmch_ctrl);
 
 	reg_addr &= 0xfff80000;
 
@@ -1211,6 +1221,12 @@ static int i9xx_setup(void)
 		intel_private.gtt_bus_addr = reg_addr + gtt_offset;
 	}
 
+	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
+	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
+	     (gmch_ctrl & G4x_GMCH_SIZE_VT_EN) &&
+	     intel_private.base.needs_dmar)
+		intel_private.base.do_idle_maps = 1;
+
 	intel_i9xx_setup_flush();
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 61ce1b7..d369e48 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2268,27 +2268,39 @@ static int i915_ring_idle(struct intel_ring_buffer *ring)
 	return i915_wait_request(ring, i915_gem_next_request_seqno(ring));
 }
 
+static int
+flush_rings(drm_i915_private_t *dev_priv)
+{
+	int i, ret;
+
+	/* Flush everything onto the inactive list. */
+	for (i = 0; i < I915_NUM_RINGS; i++) {
+		ret = i915_ring_idle(&dev_priv->ring[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 int
 i915_gpu_idle(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	bool lists_empty;
-	int ret, i;
 
 	lists_empty = (list_empty(&dev_priv->mm.flushing_list) &&
 		       list_empty(&dev_priv->mm.active_list));
 	if (lists_empty) {
-		WARN_ON(!(I915_READ(MI_MODE) & MI_RINGS_IDLE));
-		return 0;
-
-	/* Flush everything onto the inactive list. */
-	for (i = 0; i < I915_NUM_RINGS; i++) {
-		ret = i915_ring_idle(&dev_priv->ring[i]);
-		if (ret)
-			return ret;
+		/* If we require idle maps, enforce the ring is idle */
+		bool ring_idle = (I915_READ(MI_MODE) & MI_RINGS_IDLE) != 0;
+		if (!ring_idle && dev_priv->mm.gtt->do_idle_maps)
+			return flush_rings(dev_priv);
+		else
+			return 0;
 	}
 
-	return 0;
+	return flush_rings(dev_priv);
 }
 
 static int sandybridge_write_fence_reg(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7a709cd..905045d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -49,6 +49,34 @@ static unsigned int cache_level_to_agp_type(struct drm_device *dev,
 	}
 }
 
+static bool do_idling(struct drm_i915_private *dev_priv)
+{
+	bool ret = dev_priv->mm.interruptible;
+
+	if (dev_priv->mm.gtt->do_idle_maps) {
+		dev_priv->mm.interruptible = false;
+		if (i915_gpu_idle(dev_priv->dev))
+			DRM_ERROR("couldn't idle GPU %d\n", ret);
+	}
+
+	return ret;
+}
+
+static void undo_idling(struct drm_i915_private *dev_priv, bool idle)
+{
+	int ret;
+	if (!dev_priv->mm.gtt->do_idle_maps)
+		return;
+
+	/*NB: since GTT mappings don't actually touch the GPU, this is not
+	 * strictly necessary.
+	 */
+	ret = i915_gpu_idle(dev_priv->dev);
+	if (ret)
+		DRM_ERROR("couldn't idle GPU %d\n", ret);
+	dev_priv->mm.interruptible = idle;
+}
+
 void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -117,6 +145,12 @@ void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
 
 void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
 {
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool idle;
+
+	idle = do_idling(dev_priv);
+
 	intel_gtt_clear_range(obj->gtt_space->start >> PAGE_SHIFT,
 			      obj->base.size >> PAGE_SHIFT);
 
@@ -124,4 +158,6 @@ void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
 		intel_gtt_unmap_memory(obj->sg_list, obj->num_sg);
 		obj->sg_list = NULL;
 	}
+
+	undo_idling(dev_priv, idle);
 }
diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h
index 9e343c0..b174620 100644
--- a/include/drm/intel-gtt.h
+++ b/include/drm/intel-gtt.h
@@ -13,6 +13,8 @@ const struct intel_gtt {
 	unsigned int gtt_mappable_entries;
 	/* Whether i915 needs to use the dmar apis or not. */
 	unsigned int needs_dmar : 1;
+	/* Whether we idle the gpu before mapping/unmapping */
+	unsigned int do_idle_maps : 1;
 } *intel_gtt_get(void);
 
 void intel_gtt_chipset_flush(void);
-- 
1.7.6.3

Re: [PATCH v2 2/4] drm/i915: Ironlake mobile GPU with VT-d fix

[Daniel Vetter]

On Sat, Sep 24, 2011 at 03:23:05PM -0700, Ben Widawsky wrote:
> Under certain circumstances, an IOTLB flush never completes and the hardware
> just locks up.  The fix is meant to idle the GPU both before and after
> any unmap occurs.
> 
> This patch also disables the warning which is meant to detect mismatches
> between when we think the command parse and arbiter should be idle, and
> when it actually is.
> 
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

A few nitpicks below ...

> ---
>  drivers/char/agp/intel-gtt.c        |   16 ++++++++++++++
>  drivers/gpu/drm/i915/i915_gem.c     |   32 +++++++++++++++++++---------
>  drivers/gpu/drm/i915/i915_gem_gtt.c |   39 +++++++++++++++++++++++++++++++++++
>  include/drm/intel-gtt.h             |    2 +
>  4 files changed, 79 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index 8515101..47ca42f 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -923,6 +923,9 @@ static int intel_fake_agp_insert_entries(struct agp_memory *mem,
>  {
>  	int ret = -EINVAL;
>  
> +	if (intel_private.base.do_idle_maps)
> +		return -ENODEV;

This reminds me to kick out intel-agp support for ilk+. Upstream never
ever supported ums without gem on these, and that's the only reason we
have this agp compat code still lying around.

> +
>  	if (intel_private.clear_fake_agp) {
>  		int start = intel_private.base.stolen_size / PAGE_SIZE;
>  		int end = intel_private.base.gtt_mappable_entries;
> @@ -985,6 +988,9 @@ static int intel_fake_agp_remove_entries(struct agp_memory *mem,
>  	if (mem->page_count == 0)
>  		return 0;
>  
> +	if (intel_private.base.do_idle_maps)
> +		return -ENODEV;
> +
>  	intel_gtt_clear_range(pg_start, mem->page_count);
>  
>  	if (intel_private.base.needs_dmar) {
> @@ -1180,8 +1186,12 @@ static void gen6_cleanup(void)
>  static int i9xx_setup(void)
>  {
>  	u32 reg_addr;
> +	u16 gmch_ctrl;
> +	const unsigned short gpu_devid = intel_private.pcidev->device;
>  
>  	pci_read_config_dword(intel_private.pcidev, I915_MMADDR, &reg_addr);
> +	pci_read_config_word(intel_private.bridge_dev,
> +			     I830_GMCH_CTRL, &gmch_ctrl);
>  
>  	reg_addr &= 0xfff80000;
>  
> @@ -1211,6 +1221,12 @@ static int i9xx_setup(void)
>  		intel_private.gtt_bus_addr = reg_addr + gtt_offset;
>  	}
>  
> +	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
> +	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
> +	     (gmch_ctrl & G4x_GMCH_SIZE_VT_EN) &&
> +	     intel_private.base.needs_dmar)
> +		intel_private.base.do_idle_maps = 1;

Can you extract that horrible condition into e.g.
NEEDS_ILK_IOMMU_WORKAROUND or something like that? I think future me will
go wtf on this without the context ...

> +
>  	intel_i9xx_setup_flush();
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 61ce1b7..d369e48 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2268,27 +2268,39 @@ static int i915_ring_idle(struct intel_ring_buffer *ring)
>  	return i915_wait_request(ring, i915_gem_next_request_seqno(ring));
>  }
>  
> +static int
> +flush_rings(drm_i915_private_t *dev_priv)
> +{
> +	int i, ret;
> +
> +	/* Flush everything onto the inactive list. */
> +	for (i = 0; i < I915_NUM_RINGS; i++) {
> +		ret = i915_ring_idle(&dev_priv->ring[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  int
>  i915_gpu_idle(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	bool lists_empty;
> -	int ret, i;
>  
>  	lists_empty = (list_empty(&dev_priv->mm.flushing_list) &&
>  		       list_empty(&dev_priv->mm.active_list));
>  	if (lists_empty) {
> -		WARN_ON(!(I915_READ(MI_MODE) & MI_RINGS_IDLE));
> -		return 0;
> -
> -	/* Flush everything onto the inactive list. */
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> -		ret = i915_ring_idle(&dev_priv->ring[i]);
> -		if (ret)
> -			return ret;
> +		/* If we require idle maps, enforce the ring is idle */
> +		bool ring_idle = (I915_READ(MI_MODE) & MI_RINGS_IDLE) != 0;
> +		if (!ring_idle && dev_priv->mm.gtt->do_idle_maps)
> +			return flush_rings(dev_priv);
> +		else
> +			return 0;

Can you extract this hack into its own patch? This way
- the actual work-around would stand out more clearly
- we put more emphasive on the fact that we seem to lose track of things.

>  	}
>  
> -	return 0;
> +	return flush_rings(dev_priv);
>  }
>  
>  static int sandybridge_write_fence_reg(struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7a709cd..0c6226b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -49,10 +49,39 @@ static unsigned int cache_level_to_agp_type(struct drm_device *dev,
>  	}
>  }
>  
> +static bool do_idling(struct drm_i915_private *dev_priv)
> +{
> +	bool ret = dev_priv->mm.interruptible;
> +
> +	if (dev_priv->mm.gtt->do_idle_maps) {
> +		dev_priv->mm.interruptible = false;
> +		if (i915_gpu_idle(dev_priv->dev))
> +			DRM_ERROR("couldn't idle GPU %d\n", ret);
> +	}

I don't like the uninterruptible sleep in core gem - this is only for
modesetting where backtracking is just not really feasible. I've quickly
checked the code and I haven't seen any problems in handing back the
-ERESTARTSYS to gem_object_unbind.

> +
> +	return ret;
> +}
> +
> +static void undo_idling(struct drm_i915_private *dev_priv, bool idle)
> +{
> +	int ret;
> +	if (!dev_priv->mm.gtt->do_idle_maps)
> +		return;
> +
> +	/*NB: since GTT mappings don't actually touch the GPU, this is not
> +	 * strictly necessary.
> +	 */
> +	ret = i915_gpu_idle(dev_priv->dev);
> +	if (ret)
> +		DRM_ERROR("couldn't idle GPU %d\n", ret);
> +	dev_priv->mm.interruptible = idle;
> +}
> +
>  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;
> +	bool idle;

Patch-dirt? 2 more below ...

>  
>  	/* First fill our portion of the GTT with scratch pages */
>  	intel_gtt_clear_range(dev_priv->mm.gtt_start / PAGE_SIZE,
> @@ -71,6 +100,7 @@ int i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj)
>  	struct drm_device *dev = obj->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned int agp_type = cache_level_to_agp_type(dev, obj->cache_level);
> +	bool idle;
>  	int ret;
>  
>  	if (dev_priv->mm.gtt->needs_dmar) {
> @@ -100,6 +130,7 @@ void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
>  	struct drm_device *dev = obj->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned int agp_type = cache_level_to_agp_type(dev, cache_level);
> +	bool idle;
>  
>  	if (dev_priv->mm.gtt->needs_dmar) {
>  		BUG_ON(!obj->sg_list);
> @@ -117,6 +148,12 @@ void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
>  
>  void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
>  {
> +	struct drm_device *dev = obj->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool idle;
> +
> +	idle = do_idling(dev_priv);
> +
>  	intel_gtt_clear_range(obj->gtt_space->start >> PAGE_SHIFT,
>  			      obj->base.size >> PAGE_SHIFT);
>  
> @@ -124,4 +161,6 @@ void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
>  		intel_gtt_unmap_memory(obj->sg_list, obj->num_sg);
>  		obj->sg_list = NULL;
>  	}
> +
> +	undo_idling(dev_priv, idle);
>  }
> diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h
> index 9e343c0..b174620 100644
> --- a/include/drm/intel-gtt.h
> +++ b/include/drm/intel-gtt.h
> @@ -13,6 +13,8 @@ const struct intel_gtt {
>  	unsigned int gtt_mappable_entries;
>  	/* Whether i915 needs to use the dmar apis or not. */
>  	unsigned int needs_dmar : 1;
> +	/* Whether we idle the gpu before mapping/unmapping */
> +	unsigned int do_idle_maps : 1;
>  } *intel_gtt_get(void);
>  
>  void intel_gtt_chipset_flush(void);
> -- 
> 1.7.6.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v2 1/4] drm/i915: add warning for unidle GPU

[Daniel Vetter]

On Sat, Sep 24, 2011 at 03:23:04PM -0700, Ben Widawsky wrote:
> When we attempt to idle the GPU we take a shortcut by checking if our
> lists are empty and if so, avoid doing a synchronizing flush. The
> warning will let us know if this assumption turns out to not be correct.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I've just discussed this quickly with Chris on irc and it's probabbly
best to just kill the list_empty early bailout. gpu_idle isn't a fastpath,
so who cares. One candidate where we emit commands to the ring without
adding anything onto these lists is e.g. pageflip. There're probably
more.

Also it looks like this causes us to no properly idle the gpu on
suspend/resume. Might fix issues especially on gen2/3 where we put an
MI_WAIT in front of the flip, hence enlarging the window to up to 20 ms.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v2 1/4] drm/i915: add warning for unidle GPU

[Chris Wilson]

On Sun, 25 Sep 2011 12:16:36 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> Also it looks like this causes us to no properly idle the gpu on
> suspend/resume. Might fix issues especially on gen2/3 where we put an
> MI_WAIT in front of the flip, hence enlarging the window to up to 20 ms.

This I happen to disagree with as there is an existing bug with not
properly quiescing the GPU (on all gen) when disabling pipes that would
also be hit in this case.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 2/4] drm/i915: Ironlake mobile GPU with VT-d fix

[Ben Widawsky]

On Sun, 25 Sep 2011 11:40:14 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sat, Sep 24, 2011 at 03:23:05PM -0700, Ben Widawsky wrote:

> 
> Can you extract that horrible condition into e.g.
> NEEDS_ILK_IOMMU_WORKAROUND or something like that? I think future me
> will go wtf on this without the context ...
> 

Sure.

> > +
> >  	intel_i9xx_setup_flush();
> >  
> >  	return 0;
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c index 61ce1b7..d369e48 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2268,27 +2268,39 @@ static int i915_ring_idle(struct
> > intel_ring_buffer *ring) return i915_wait_request(ring,
> > i915_gem_next_request_seqno(ring)); }
> >  
> > +static int
> > +flush_rings(drm_i915_private_t *dev_priv)
> > +{
> > +	int i, ret;
> > +
> > +	/* Flush everything onto the inactive list. */
> > +	for (i = 0; i < I915_NUM_RINGS; i++) {
> > +		ret = i915_ring_idle(&dev_priv->ring[i]);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  int
> >  i915_gpu_idle(struct drm_device *dev)
> >  {
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> >  	bool lists_empty;
> > -	int ret, i;
> >  
> >  	lists_empty = (list_empty(&dev_priv->mm.flushing_list) &&
> >  		       list_empty(&dev_priv->mm.active_list));
> >  	if (lists_empty) {
> > -		WARN_ON(!(I915_READ(MI_MODE) & MI_RINGS_IDLE));
> > -		return 0;
> > -
> > -	/* Flush everything onto the inactive list. */
> > -	for (i = 0; i < I915_NUM_RINGS; i++) {
> > -		ret = i915_ring_idle(&dev_priv->ring[i]);
> > -		if (ret)
> > -			return ret;
> > +		/* If we require idle maps, enforce the ring is
> > idle */
> > +		bool ring_idle = (I915_READ(MI_MODE) &
> > MI_RINGS_IDLE) != 0;
> > +		if (!ring_idle && dev_priv->mm.gtt->do_idle_maps)
> > +			return flush_rings(dev_priv);
> > +		else
> > +			return 0;
> 
> Can you extract this hack into its own patch? This way
> - the actual work-around would stand out more clearly
> - we put more emphasive on the fact that we seem to lose track of
> things.
> 

>From your/Chris' comment from patch 1, I will remove this hunk
completely, and patch 1 will simply always idle the rings.

> >  	}
> >  
> > -	return 0;
> > +	return flush_rings(dev_priv);
> >  }
> >  
> >  static int sandybridge_write_fence_reg(struct drm_i915_gem_object
> > *obj, diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c index 7a709cd..0c6226b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -49,10 +49,39 @@ static unsigned int
> > cache_level_to_agp_type(struct drm_device *dev, }
> >  }
> >  
> > +static bool do_idling(struct drm_i915_private *dev_priv)
> > +{
> > +	bool ret = dev_priv->mm.interruptible;
> > +
> > +	if (dev_priv->mm.gtt->do_idle_maps) {
> > +		dev_priv->mm.interruptible = false;
> > +		if (i915_gpu_idle(dev_priv->dev))
> > +			DRM_ERROR("couldn't idle GPU %d\n", ret);
> > +	}
> 
> I don't like the uninterruptible sleep in core gem - this is only for
> modesetting where backtracking is just not really feasible. I've
> quickly checked the code and I haven't seen any problems in handing
> back the -ERESTARTSYS to gem_object_unbind.
> 

While I agree that we could probably return at idle time, we have to
guarantee that no GPU activity occurs until the unmap has completed. I
checked the code and don't really understand how it can occur, but I
believe it *can* occur as I've seen it happen in the idle function. Any
other proposal how to fix this, or any proof that the GPU can't wake up
to do display stuff while doing the unmap?

Ben

Re: [PATCH v2 2/4] drm/i915: Ironlake mobile GPU with VT-d fix

[Daniel Vetter]

On Sun, Sep 25, 2011 at 04:42:57PM -0700, Ben Widawsky wrote:
> > >  static int sandybridge_write_fence_reg(struct drm_i915_gem_object
> > > *obj, diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > b/drivers/gpu/drm/i915/i915_gem_gtt.c index 7a709cd..0c6226b 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -49,10 +49,39 @@ static unsigned int
> > > cache_level_to_agp_type(struct drm_device *dev, }
> > >  }
> > >  
> > > +static bool do_idling(struct drm_i915_private *dev_priv)
> > > +{
> > > +	bool ret = dev_priv->mm.interruptible;
> > > +
> > > +	if (dev_priv->mm.gtt->do_idle_maps) {
> > > +		dev_priv->mm.interruptible = false;
> > > +		if (i915_gpu_idle(dev_priv->dev))
> > > +			DRM_ERROR("couldn't idle GPU %d\n", ret);
> > > +	}
> > 
> > I don't like the uninterruptible sleep in core gem - this is only for
> > modesetting where backtracking is just not really feasible. I've
> > quickly checked the code and I haven't seen any problems in handing
> > back the -ERESTARTSYS to gem_object_unbind.
> > 
> 
> While I agree that we could probably return at idle time, we have to
> guarantee that no GPU activity occurs until the unmap has completed. I
> checked the code and don't really understand how it can occur, but I
> believe it *can* occur as I've seen it happen in the idle function. Any
> other proposal how to fix this, or any proof that the GPU can't wake up
> to do display stuff while doing the unmap?

Hm, looks like some misunderstanding. I understand why the gpu_idle needs
to be here, my gripes are solely with the uninterruptible sleep that you
use. I think a normal interruptible sleep with passing back the return
value to gem_unbind_object should work: By the time we're calling
gtt_unbind_object in gem_unbind_object we have not yet committed to the
unbound state, so can just bail out if the mappings are still there. And
gtt_unbind_object idles the gpu before touching the mappings, so when we
have to bail out with -ERESTARTSYS the mapping is untouched and everything
safe.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v2 2/4] drm/i915: Ironlake mobile GPU with VT-d fix

[Ben Widawsky]

On Mon, 26 Sep 2011 09:23:17 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sun, Sep 25, 2011 at 04:42:57PM -0700, Ben Widawsky wrote:
> > > >  static int sandybridge_write_fence_reg(struct drm_i915_gem_object
> > > > *obj, diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c index 7a709cd..0c6226b 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > @@ -49,10 +49,39 @@ static unsigned int
> > > > cache_level_to_agp_type(struct drm_device *dev, }
> > > >  }
> > > >  
> > > > +static bool do_idling(struct drm_i915_private *dev_priv)
> > > > +{
> > > > +	bool ret = dev_priv->mm.interruptible;
> > > > +
> > > > +	if (dev_priv->mm.gtt->do_idle_maps) {
> > > > +		dev_priv->mm.interruptible = false;
> > > > +		if (i915_gpu_idle(dev_priv->dev))
> > > > +			DRM_ERROR("couldn't idle GPU %d\n", ret);
> > > > +	}
> > > 
> > > I don't like the uninterruptible sleep in core gem - this is only for
> > > modesetting where backtracking is just not really feasible. I've
> > > quickly checked the code and I haven't seen any problems in handing
> > > back the -ERESTARTSYS to gem_object_unbind.
> > > 
> > 
> > While I agree that we could probably return at idle time, we have to
> > guarantee that no GPU activity occurs until the unmap has completed. I
> > checked the code and don't really understand how it can occur, but I
> > believe it *can* occur as I've seen it happen in the idle function. Any
> > other proposal how to fix this, or any proof that the GPU can't wake up
> > to do display stuff while doing the unmap?
> 
> Hm, looks like some misunderstanding. I understand why the gpu_idle needs
> to be here, my gripes are solely with the uninterruptible sleep that you
> use. I think a normal interruptible sleep with passing back the return
> value to gem_unbind_object should work: By the time we're calling
> gtt_unbind_object in gem_unbind_object we have not yet committed to the
> unbound state, so can just bail out if the mappings are still there. And
> gtt_unbind_object idles the gpu before touching the mappings, so when we
> have to bail out with -ERESTARTSYS the mapping is untouched and everything
> safe.
> -Daniel

Okay. I agree your suggestion works. I've changed the code to do this. I
will not listen to you for a while if someone comes in and just says
make it non interruptible, because it seems to be happening a lot
recently :P

Ben