[PATCH v4 0/4] ILK VT-d fix

[PATCH v4 0/4] ILK VT-d fix

[Ben Widawsky]

Tested-by: Ben Widawsky <ben@bwidawsk.net>
I ran the airlied test on it for 4 minutes. It normally hangs in <1
minute.

Changes from the last version:
Rebased on the new patches from David Woodhouse
Use the new global to determine if we need workaround.
Idling can fail now, instead of being uninterruptible.
No more undo idling (we can add it back later if we need it).

Ben Widawsky (2):
  drm/i915: Remove early exit on i915_gpu_idle
  drm/i915: ILK + VT-d workaround

David Woodhouse (2):
  intel-iommu: Workaround IOTLB hang on Ironlake GPU
  intel-iommu: Export a flag indicating that the IOMMU is used for
    iGFX.

 drivers/char/agp/intel-gtt.c        |   29 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |    2 +-
 drivers/gpu/drm/i915/i915_gem.c     |   10 +++-------
 drivers/gpu/drm/i915/i915_gem_gtt.c |   19 ++++++++++++++++++-
 drivers/iommu/intel-iommu.c         |   31 ++++++++++++++++++++-----------
 include/drm/intel-gtt.h             |    2 ++
 6 files changed, 73 insertions(+), 20 deletions(-)

-- 
1.7.7

[PATCH 1/4] intel-iommu: Workaround IOTLB hang on Ironlake GPU

[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 (we hope) 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.

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..ff26603 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 if (dmar_map_gfx) {
+		/* 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.7

[PATCH 2/4] intel-iommu: Export a flag indicating that the IOMMU is used for iGFX.

[Ben Widawsky]

From: David Woodhouse <David.Woodhouse@intel.com>

We really don't want this to work in the general case; device drivers
*shouldn't* care whether they are behind an IOMMU or not. But the
integrated graphics is a special case, because the IOMMU and the GTT are
all kind of smashed into one and generally horrifically buggy, so it's
reasonable for the graphics driver to want to know when the IOMMU is
active for the graphics hardware.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/iommu/intel-iommu.c |   25 +++++++++++++++----------
 1 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index ff26603..1e184c1 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -404,6 +404,9 @@ static int dmar_forcedac;
 static int intel_iommu_strict;
 static int intel_iommu_superpage = 1;
 
+int intel_iommu_gfx_mapped;
+EXPORT_SYMBOL_GPL(intel_iommu_gfx_mapped);
+
 #define DUMMY_DEVICE_DOMAIN_INFO ((struct device_domain_info *)(-1))
 static DEFINE_SPINLOCK(device_domain_lock);
 static LIST_HEAD(device_domain_list);
@@ -3226,9 +3229,6 @@ static void __init init_no_remapping_devices(void)
 		}
 	}
 
-	if (dmar_map_gfx)
-		return;
-
 	for_each_drhd_unit(drhd) {
 		int i;
 		if (drhd->ignored || drhd->include_all)
@@ -3236,18 +3236,23 @@ static void __init init_no_remapping_devices(void)
 
 		for (i = 0; i < drhd->devices_cnt; i++)
 			if (drhd->devices[i] &&
-				!IS_GFX_DEVICE(drhd->devices[i]))
+			    !IS_GFX_DEVICE(drhd->devices[i]))
 				break;
 
 		if (i < drhd->devices_cnt)
 			continue;
 
-		/* bypass IOMMU if it is just for gfx devices */
-		drhd->ignored = 1;
-		for (i = 0; i < drhd->devices_cnt; i++) {
-			if (!drhd->devices[i])
-				continue;
-			drhd->devices[i]->dev.archdata.iommu = DUMMY_DEVICE_DOMAIN_INFO;
+		/* This IOMMU has *only* gfx devices. Either bypass it or
+		   set the gfx_mapped flag, as appropriate */
+		if (dmar_map_gfx) {
+			intel_iommu_gfx_mapped = 1;
+		} else {
+			drhd->ignored = 1;
+			for (i = 0; i < drhd->devices_cnt; i++) {
+				if (!drhd->devices[i])
+					continue;
+				drhd->devices[i]->dev.archdata.iommu = DUMMY_DEVICE_DOMAIN_INFO;
+			}
 		}
 	}
 }
-- 
1.7.7

[PATCH 3/4] drm/i915: Remove early exit on i915_gpu_idle

[Ben Widawsky]

[Description from: Daniel Vetter]
I've just discussed this quickly with Chris on irc and it's probably
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 are
probably more.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f0f885f..a925141 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2191,14 +2191,8 @@ 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)
-		return 0;
-
 	/* Flush everything onto the inactive list. */
 	for (i = 0; i < I915_NUM_RINGS; i++) {
 		ret = i915_ring_idle(&dev_priv->ring[i]);
-- 
1.7.7

Re: [PATCH 3/4] drm/i915: Remove early exit on i915_gpu_idle

[Daniel Vetter]

On Sat, Oct 15, 2011 at 01:47:15PM -0700, Ben Widawsky wrote:
> [Description from: Daniel Vetter]
> I've just discussed this quickly with Chris on irc and it's probably
> 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 are
> probably more.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (for the commitmsg)
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/4] drm/i915: Remove early exit on i915_gpu_idle

[Chris Wilson]

On Sat, 15 Oct 2011 13:47:15 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> [Description from: Daniel Vetter]
> I've just discussed this quickly with Chris on irc and it's probably
> 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 are
> probably more.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 4/4] drm/i915: ILK + VT-d workaround

[Ben Widawsky]

Idle the GPU before doing any unmaps. We know if VT-d is in use through
an exported variable from iommu code.

This should avoid a known HW issue.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/char/agp/intel-gtt.c        |   28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |    2 +-
 drivers/gpu/drm/i915/i915_gem.c     |    4 +++-
 drivers/gpu/drm/i915/i915_gem_gtt.c |   19 ++++++++++++++++++-
 include/drm/intel-gtt.h             |    2 ++
 5 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index 8515101..80a7ed0 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) {
@@ -1177,6 +1183,25 @@ static void gen6_cleanup(void)
 {
 }
 
+/* Certain Gen5 chipsets require require idling the GPU before
+ * unmapping anything from the GTT when VT-d is enabled.
+ */
+extern int intel_iommu_gfx_mapped;
+static inline int needs_idle_maps(void)
+{
+	const unsigned short gpu_devid = intel_private.pcidev->device;
+
+	/* Query intel_iommu to see if we need the workaround. Presumably that
+	 * was loaded first.
+	 */
+	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
+	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
+	     intel_iommu_gfx_mapped)
+		return 1;
+
+	return 0;
+}
+
 static int i9xx_setup(void)
 {
 	u32 reg_addr;
@@ -1211,6 +1236,9 @@ static int i9xx_setup(void)
 		intel_private.gtt_bus_addr = reg_addr + gtt_offset;
 	}
 
+	if (needs_idle_maps());
+		intel_private.base.do_idle_maps = 1;
+
 	intel_i9xx_setup_flush();
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 15c0ca5..3aa8612 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1219,7 +1219,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev);
 int __must_check i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj);
 void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
 				enum i915_cache_level cache_level);
-void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj);
+int i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj);
 
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct drm_device *dev, int min_size,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a925141..89d22eb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2130,7 +2130,9 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
 
 	trace_i915_gem_object_unbind(obj);
 
-	i915_gem_gtt_unbind_object(obj);
+	ret = i915_gem_gtt_unbind_object(obj);
+	if (ret == -ERESTARTSYS)
+		return ret;
 	i915_gem_object_put_pages_gtt(obj);
 
 	list_del_init(&obj->gtt_list);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7a709cd..1df6395 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -49,6 +49,15 @@ static unsigned int cache_level_to_agp_type(struct drm_device *dev,
 	}
 }
 
+static bool do_idling(struct drm_i915_private *dev_priv)
+{
+	if (dev_priv->mm.gtt->do_idle_maps)
+		if (i915_gpu_idle(dev_priv->dev))
+			return false;
+
+	return true;
+}
+
 void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -115,8 +124,14 @@ void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
 				       agp_type);
 }
 
-void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
+int 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;
+
+	if (do_idling(dev_priv) == false)
+		return -ERESTARTSYS;
+
 	intel_gtt_clear_range(obj->gtt_space->start >> PAGE_SHIFT,
 			      obj->base.size >> PAGE_SHIFT);
 
@@ -124,4 +139,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;
 	}
+
+	return 0;
 }
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.7

Re: [PATCH 4/4] drm/i915: ILK + VT-d workaround

[Daniel Vetter]

On Sat, Oct 15, 2011 at 01:47:16PM -0700, Ben Widawsky wrote:
> Idle the GPU before doing any unmaps. We know if VT-d is in use through
> an exported variable from iommu code.
> 
> This should avoid a known HW issue.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/char/agp/intel-gtt.c        |   28 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h     |    2 +-
>  drivers/gpu/drm/i915/i915_gem.c     |    4 +++-
>  drivers/gpu/drm/i915/i915_gem_gtt.c |   19 ++++++++++++++++++-
>  include/drm/intel-gtt.h             |    2 ++
>  5 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index 8515101..80a7ed0 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;
> +

This reminds me to kill the fake agp driver fro gen5+ with fire (we only
need it for ums code). Infrastructure is half-way there, but not quite yet
...

>  	intel_gtt_clear_range(pg_start, mem->page_count);
>  
>  	if (intel_private.base.needs_dmar) {
> @@ -1177,6 +1183,25 @@ static void gen6_cleanup(void)
>  {
>  }
>  
> +/* Certain Gen5 chipsets require require idling the GPU before
> + * unmapping anything from the GTT when VT-d is enabled.
> + */
> +extern int intel_iommu_gfx_mapped;
> +static inline int needs_idle_maps(void)
> +{
> +	const unsigned short gpu_devid = intel_private.pcidev->device;
> +
> +	/* Query intel_iommu to see if we need the workaround. Presumably that
> +	 * was loaded first.
> +	 */
> +	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
> +	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
> +	     intel_iommu_gfx_mapped)
> +		return 1;
> +
> +	return 0;
> +}
> +
>  static int i9xx_setup(void)
>  {
>  	u32 reg_addr;
> @@ -1211,6 +1236,9 @@ static int i9xx_setup(void)
>  		intel_private.gtt_bus_addr = reg_addr + gtt_offset;
>  	}
>  
> +	if (needs_idle_maps());
> +		intel_private.base.do_idle_maps = 1;
> +
>  	intel_i9xx_setup_flush();
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 15c0ca5..3aa8612 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1219,7 +1219,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev);
>  int __must_check i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj);
>  void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
>  				enum i915_cache_level cache_level);
> -void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj);
> +int i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj);
>  
>  /* i915_gem_evict.c */
>  int __must_check i915_gem_evict_something(struct drm_device *dev, int min_size,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a925141..89d22eb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2130,7 +2130,9 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
>  
>  	trace_i915_gem_object_unbind(obj);
>  
> -	i915_gem_gtt_unbind_object(obj);
> +	ret = i915_gem_gtt_unbind_object(obj);
> +	if (ret == -ERESTARTSYS)
> +		return ret;
>  	i915_gem_object_put_pages_gtt(obj);
>  
>  	list_del_init(&obj->gtt_list);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7a709cd..1df6395 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -49,6 +49,15 @@ static unsigned int cache_level_to_agp_type(struct drm_device *dev,
>  	}
>  }
>  
> +static bool do_idling(struct drm_i915_private *dev_priv)
> +{
> +	if (dev_priv->mm.gtt->do_idle_maps)
> +		if (i915_gpu_idle(dev_priv->dev))
> +			return false;
> +
> +	return true;
> +}
> +
>  void i915_gem_restore_gtt_mappings(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -115,8 +124,14 @@ void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
>  				       agp_type);
>  }
>  
> -void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
> +int 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;
> +
> +	if (do_idling(dev_priv) == false)
> +		return -ERESTARTSYS;

That's a bit a lie, gpu_idle can also return -ENOMEM, -EAGAIN and -EIO. I
think you could just inline do_idling to avoid passing back the retval
twice ...
		
> +
>  	intel_gtt_clear_range(obj->gtt_space->start >> PAGE_SHIFT,
>  			      obj->base.size >> PAGE_SHIFT);
>  
> @@ -124,4 +139,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;
>  	}
> +
> +	return 0;
>  }
> 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.7
> 
> _______________________________________________
> 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 4/4] drm/i915: ILK + VT-d workaround

[Chris Wilson]

On Sat, 15 Oct 2011 13:47:16 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Idle the GPU before doing any unmaps. We know if VT-d is in use through
> an exported variable from iommu code.
> 
> This should avoid a known HW issue.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Just one minor comment below, but
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> ---
>  drivers/char/agp/intel-gtt.c        |   28 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h     |    2 +-
>  drivers/gpu/drm/i915/i915_gem.c     |    4 +++-
>  drivers/gpu/drm/i915/i915_gem_gtt.c |   19 ++++++++++++++++++-
>  include/drm/intel-gtt.h             |    2 ++
>  5 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index 8515101..80a7ed0 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) {
> @@ -1177,6 +1183,25 @@ static void gen6_cleanup(void)
>  {
>  }
>  
> +/* Certain Gen5 chipsets require require idling the GPU before
> + * unmapping anything from the GTT when VT-d is enabled.
> + */
> +extern int intel_iommu_gfx_mapped;
> +static inline int needs_idle_maps(void)
> +{
> +	const unsigned short gpu_devid = intel_private.pcidev->device;
> +
> +	/* Query intel_iommu to see if we need the workaround. Presumably that
> +	 * was loaded first.
> +	 */
> +	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
> +	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
> +	     intel_iommu_gfx_mapped)
> +		return 1;
> +
> +	return 0;
> +}
> +
>  static int i9xx_setup(void)
>  {
>  	u32 reg_addr;
> @@ -1211,6 +1236,9 @@ static int i9xx_setup(void)
>  		intel_private.gtt_bus_addr = reg_addr + gtt_offset;
>  	}
>  
> +	if (needs_idle_maps());
> +		intel_private.base.do_idle_maps = 1;
> +
>  	intel_i9xx_setup_flush();
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 15c0ca5..3aa8612 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1219,7 +1219,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev);
>  int __must_check i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj);
>  void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
>  				enum i915_cache_level cache_level);
> -void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj);
> +int i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj);

Make this a __must_check, if we forget to handle the error from unbind,
it will undoubtably lead to great confusion and an upset chip/kernel.
Admittedly today there is only callsite, but useful defence against
future callers making a mistake.
>  
>  /* i915_gem_evict.c */
>  int __must_check i915_gem_evict_something(struct drm_device *dev, int min_size,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a925141..89d22eb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2130,7 +2130,9 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
>  
>  	trace_i915_gem_object_unbind(obj);
>  
> -	i915_gem_gtt_unbind_object(obj);
> +	ret = i915_gem_gtt_unbind_object(obj);
> +	if (ret == -ERESTARTSYS)
> +		return ret;
>  	i915_gem_object_put_pages_gtt(obj);
>  
>  	list_del_init(&obj->gtt_list);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7a709cd..1df6395 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -49,6 +49,15 @@ static unsigned int cache_level_to_agp_type(struct drm_device *dev,
>  	}
>  }
>  
> +static bool do_idling(struct drm_i915_private *dev_priv)
> +{
> +	if (dev_priv->mm.gtt->do_idle_maps)
> +		if (i915_gpu_idle(dev_priv->dev))
> +			return false;
> +
> +	return true;
> +}
> +
>  void i915_gem_restore_gtt_mappings(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -115,8 +124,14 @@ void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
>  				       agp_type);
>  }
>  
> -void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
> +int 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;
> +
> +	if (do_idling(dev_priv) == false)
> +		return -ERESTARTSYS;
> +
>  	intel_gtt_clear_range(obj->gtt_space->start >> PAGE_SHIFT,
>  			      obj->base.size >> PAGE_SHIFT);
>  
> @@ -124,4 +139,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;
>  	}
> +
> +	return 0;
>  }
> 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.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v4 0/4] ILK VT-d fix

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 396 bytes --]

On Mon, 24 Oct 2011 08:14:45 +0000, "Woodhouse, David" <david.woodhouse@intel.com> wrote:

> The IOMMU patches are now in Linus' tree and thus will be in the 3.1
> release. Are we going to push the graphics patches in time for the 3.1
> release too?

The graphics patches are in the 3.2 pull request; they'll be available
for the 3.1 stable series though.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 0/4] v5: ILK vt-d fix, more like v3 now

[Ben Widawsky]

I think Daniel caved in and agrees that making the idle cause the GPU to
be uninterruptible was the correct approach. So this patch is pretty
much the same as v3, except it has David's new global to determine the
IOMMU status. In other words, disregard v4.

For the series:
Tested-by: Ben Widawsky <ben@bwidawsk.net>

Ben Widawsky (2):
  drm/i915: Remove early exit on i915_gpu_idle
  drm/i915: ILK + VT-d workaround

David Woodhouse (2):
  intel-iommu: Workaround IOTLB hang on Ironlake GPU
  intel-iommu: Export a flag indicating that the IOMMU is used for
    iGFX.

 drivers/char/agp/intel-gtt.c        |   28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c     |    6 ------
 drivers/gpu/drm/i915/i915_gem_gtt.c |   30 ++++++++++++++++++++++++++++++
 drivers/iommu/intel-iommu.c         |   31 ++++++++++++++++++++-----------
 include/drm/intel-gtt.h             |    2 ++
 5 files changed, 80 insertions(+), 17 deletions(-)

-- 
1.7.7

[PATCH 3/4] drm/i915: Remove early exit on i915_gpu_idle

[Ben Widawsky]

[Description from: Daniel Vetter]
I've just discussed this quickly with Chris on irc and it's probably
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 are
probably more.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f0f885f..a925141 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2191,14 +2191,8 @@ 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)
-		return 0;
-
 	/* Flush everything onto the inactive list. */
 	for (i = 0; i < I915_NUM_RINGS; i++) {
 		ret = i915_ring_idle(&dev_priv->ring[i]);
-- 
1.7.7