All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Ironlake GPU with VT-d fix
@ 2011-09-23  0:11 Ben Widawsky
  2011-09-23  4:50 ` Keith Packard
  0 siblings, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2011-09-23  0:11 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dave Airlie, Ben Widawsky, David Woodhouse

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 map or unmap occurs.

It requires an additional IOMMU patch.

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

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index 8515101..9d939f0 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -650,6 +650,7 @@ static void intel_gtt_cleanup(void)
 static int intel_gtt_init(void)
 {
 	u32 gtt_map_size;
+	const unsigned short gpu_devid = intel_private.pcidev->device;
 	int ret;
 
 	ret = intel_private.driver->setup();
@@ -688,6 +689,11 @@ static int intel_gtt_init(void)
 
 	intel_private.base.needs_dmar = USE_PCI_DMA_API && INTEL_GTT_GEN > 2;
 
+	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
+	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
+	     intel_private.base.needs_dmar)
+		intel_private.base.do_idle_maps = 1;
+
 	ret = intel_gtt_setup_scratch_page();
 	if (ret != 0) {
 		intel_gtt_cleanup();
@@ -923,6 +929,8 @@ static int intel_fake_agp_insert_entries(struct agp_memory *mem,
 {
 	int ret = -EINVAL;
 
+	BUG_ON(intel_private.base.do_idle_maps);
+
 	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 +993,8 @@ static int intel_fake_agp_remove_entries(struct agp_memory *mem,
 	if (mem->page_count == 0)
 		return 0;
 
+	BUG_ON(intel_private.base.do_idle_maps);
+
 	intel_gtt_clear_range(pg_start, mem->page_count);
 
 	if (intel_private.base.needs_dmar) {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a546a71..785b84a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2277,7 +2277,7 @@ 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 && !!dev_priv->mm.gtt->do_idle_maps)
 		return 0;
 
 	/* Flush everything onto the inactive list. */
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7a709cd..c11ec60 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -49,10 +49,41 @@ 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;
+
+	idle = do_idling(dev_priv);
 
 	/* First fill our portion of the GTT with scratch pages */
 	intel_gtt_clear_range(dev_priv->mm.gtt_start / PAGE_SIZE,
@@ -64,6 +95,8 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 	}
 
 	intel_gtt_chipset_flush();
+
+	undo_idling(dev_priv, idle);
 }
 
 int i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj)
@@ -71,8 +104,11 @@ 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;
 
+	idle = do_idling(dev_priv);
+
 	if (dev_priv->mm.gtt->needs_dmar) {
 		ret = intel_gtt_map_memory(obj->pages,
 					   obj->base.size >> PAGE_SHIFT,
@@ -91,6 +127,7 @@ int i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj)
 				       obj->pages,
 				       agp_type);
 
+	undo_idling(dev_priv, idle);
 	return 0;
 }
 
@@ -100,6 +137,9 @@ 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;
+
+	idle = do_idling(dev_priv);
 
 	if (dev_priv->mm.gtt->needs_dmar) {
 		BUG_ON(!obj->sg_list);
@@ -113,10 +153,18 @@ void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
 				       obj->base.size >> PAGE_SHIFT,
 				       obj->pages,
 				       agp_type);
+
+	undo_idling(dev_priv, idle);
 }
 
 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 +172,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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Ironlake GPU with VT-d fix
  2011-09-23  0:11 [PATCH] drm/i915: Ironlake GPU with VT-d fix Ben Widawsky
@ 2011-09-23  4:50 ` Keith Packard
  2011-09-23  5:09   ` Ben Widawsky
  0 siblings, 1 reply; 6+ messages in thread
From: Keith Packard @ 2011-09-23  4:50 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dave Airlie, Ben Widawsky, David Woodhouse


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

On Thu, 22 Sep 2011 17:11:52 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:

> It requires an additional IOMMU patch.

Can we collect those two patches into one sequence?

> +	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
> +	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
> +	     intel_private.base.needs_dmar)
> +		intel_private.base.do_idle_maps = 1;
> +

I'd like to make this conditional on whether IOMMU is actually in use;
needs_dmar is based solely on whether the DMA_API is compiled into the
kernel and the GTT gen is > 2.

> -	if (lists_empty)
> +	if (lists_empty && !!dev_priv->mm.gtt->do_idle_maps)
>  		return 0;

Is it necessary to change the semantic of this function in cases which
aren't related to GTT remapping? Seems like you're imposing a fairly
high cost on operations which don't actually need it.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Ironlake GPU with VT-d fix
  2011-09-23  4:50 ` Keith Packard
@ 2011-09-23  5:09   ` Ben Widawsky
  2011-09-23  7:37     ` Chris Wilson
  0 siblings, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2011-09-23  5:09 UTC (permalink / raw)
  To: Keith Packard; +Cc: Dave Airlie, intel-gfx, Woodhouse, David

On Thu, 22 Sep 2011 21:50:49 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Thu, 22 Sep 2011 17:11:52 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> 
> > It requires an additional IOMMU patch.
> 
> Can we collect those two patches into one sequence?
> 

Yes. Although not sure who would do the pull request to Linus.

> > +	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
> > +	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
> > +	     intel_private.base.needs_dmar)
> > +		intel_private.base.do_idle_maps = 1;
> > +
> 
> I'd like to make this conditional on whether IOMMU is actually in use;
> needs_dmar is based solely on whether the DMA_API is compiled into the
> kernel and the GTT gen is > 2.

David did mention a way for me to detect it. I wasn't sure if there was a
deadline to get this patch out so I submitted it now. I will work on the
detection portion of it while the rest gets review. It's something like, check
if the bus address == the physical address and if not, assume the IOMMU is in
use.

> 
> > -	if (lists_empty)
> > +	if (lists_empty && !!dev_priv->mm.gtt->do_idle_maps)
> >  		return 0;
> 
> Is it necessary to change the semantic of this function in cases which
> aren't related to GTT remapping? Seems like you're imposing a fairly
> high cost on operations which don't actually need it.
> 

Not sure I follow. On that specific hunk it's apparently needed. I tried the
patch without this and we got some data back which suggested it didn't work. It
would be nice if we could reproduce this locally...

Ben

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Ironlake GPU with VT-d fix
  2011-09-23  5:09   ` Ben Widawsky
@ 2011-09-23  7:37     ` Chris Wilson
  2011-09-23 15:38       ` Ben Widawsky
  0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2011-09-23  7:37 UTC (permalink / raw)
  To: Ben Widawsky, Keith Packard; +Cc: Dave Airlie, intel-gfx, Woodhouse, David

On Thu, 22 Sep 2011 22:09:39 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Thu, 22 Sep 2011 21:50:49 -0700
> Keith Packard <keithp@keithp.com> wrote:
> 
> > On Thu, 22 Sep 2011 17:11:52 -0700, Ben Widawsky <ben@bwidawsk.net>
> > wrote:
> > 
> > > It requires an additional IOMMU patch.
> > 
> > Can we collect those two patches into one sequence?
> > 
> 
> Yes. Although not sure who would do the pull request to Linus.
> 
> > > +	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
> > > +	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
> > > +	     intel_private.base.needs_dmar)
> > > +		intel_private.base.do_idle_maps = 1;
> > > +
> > 
> > I'd like to make this conditional on whether IOMMU is actually in use;
> > needs_dmar is based solely on whether the DMA_API is compiled into the
> > kernel and the GTT gen is > 2.
> 
> David did mention a way for me to detect it. I wasn't sure if there was a
> deadline to get this patch out so I submitted it now. I will work on the
> detection portion of it while the rest gets review. It's something like, check
> if the bus address == the physical address and if not, assume the IOMMU is in
> use.

And for starters, we can check no_iommu. But really we want
intel_iommu.c to report whether or not it enabled VTd on the gfx.
 
> > 
> > > -	if (lists_empty)
> > > +	if (lists_empty && !!dev_priv->mm.gtt->do_idle_maps)
> > >  		return 0;
> > 
> > Is it necessary to change the semantic of this function in cases which
> > aren't related to GTT remapping? Seems like you're imposing a fairly
> > high cost on operations which don't actually need it.
> > 
> 
> Not sure I follow. On that specific hunk it's apparently needed. I tried the
> patch without this and we got some data back which suggested it didn't work. It
> would be nice if we could reproduce this locally...

Keith is right, there should be no reason to touch that function,
and especially not for the normal code paths.

Also we should not introduce BUG_ON() trivially into user code paths
(/dev/agp).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Ironlake GPU with VT-d fix
  2011-09-23  7:37     ` Chris Wilson
@ 2011-09-23 15:38       ` Ben Widawsky
  2011-09-23 19:03         ` Chris Wilson
  0 siblings, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2011-09-23 15:38 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Dave Airlie, Woodhouse, intel-gfx, David

On Fri, 23 Sep 2011 08:37:10 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 22 Sep 2011 22:09:39 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > On Thu, 22 Sep 2011 21:50:49 -0700
> > Keith Packard <keithp@keithp.com> wrote:
> > 
> > > On Thu, 22 Sep 2011 17:11:52 -0700, Ben Widawsky
> > > <ben@bwidawsk.net> wrote:
> > > 
> > > > It requires an additional IOMMU patch.
> > > 
> > > Can we collect those two patches into one sequence?
> > > 
> > 
> > Yes. Although not sure who would do the pull request to Linus.
> > 
> > > > +	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
> > > > +	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
> > > > +	     intel_private.base.needs_dmar)
> > > > +		intel_private.base.do_idle_maps = 1;
> > > > +
> > > 
> > > I'd like to make this conditional on whether IOMMU is actually in
> > > use; needs_dmar is based solely on whether the DMA_API is
> > > compiled into the kernel and the GTT gen is > 2.
> > 
> > David did mention a way for me to detect it. I wasn't sure if there
> > was a deadline to get this patch out so I submitted it now. I will
> > work on the detection portion of it while the rest gets review.
> > It's something like, check if the bus address == the physical
> > address and if not, assume the IOMMU is in use.
> 
> And for starters, we can check no_iommu. But really we want
> intel_iommu.c to report whether or not it enabled VTd on the gfx.
>  
> > > 
> > > > -	if (lists_empty)
> > > > +	if (lists_empty && !!dev_priv->mm.gtt->do_idle_maps)
> > > >  		return 0;
> > > 
> > > Is it necessary to change the semantic of this function in cases
> > > which aren't related to GTT remapping? Seems like you're imposing
> > > a fairly high cost on operations which don't actually need it.
> > > 
> > 
> > Not sure I follow. On that specific hunk it's apparently needed. I
> > tried the patch without this and we got some data back which
> > suggested it didn't work. It would be nice if we could reproduce
> > this locally...
> 
> Keith is right, there should be no reason to touch that function,
> and especially not for the normal code paths.

Crap... This is a bug in the patch.

> 
> Also we should not introduce BUG_ON() trivially into user code paths
> (/dev/agp).

Remove 'em then?

> -Chris
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Ironlake GPU with VT-d fix
  2011-09-23 15:38       ` Ben Widawsky
@ 2011-09-23 19:03         ` Chris Wilson
  0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2011-09-23 19:03 UTC (permalink / raw)
  To: Ben Widawsky; +Cc: Dave Airlie, Woodhouse, intel-gfx, David

On Fri, 23 Sep 2011 08:38:49 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Fri, 23 Sep 2011 08:37:10 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Thu, 22 Sep 2011 22:09:39 -0700, Ben Widawsky <ben@bwidawsk.net>
> > wrote:
> > > On Thu, 22 Sep 2011 21:50:49 -0700
> > > Keith Packard <keithp@keithp.com> wrote:
> > > 
> > > > On Thu, 22 Sep 2011 17:11:52 -0700, Ben Widawsky
> > > > <ben@bwidawsk.net> wrote:
> > > > 
> > > > > It requires an additional IOMMU patch.
> > > > 
> > > > Can we collect those two patches into one sequence?
> > > > 
> > > 
> > > Yes. Although not sure who would do the pull request to Linus.
> > > 
> > > > > +	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
> > > > > +	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
> > > > > +	     intel_private.base.needs_dmar)
> > > > > +		intel_private.base.do_idle_maps = 1;
> > > > > +
> > > > 
> > > > I'd like to make this conditional on whether IOMMU is actually in
> > > > use; needs_dmar is based solely on whether the DMA_API is
> > > > compiled into the kernel and the GTT gen is > 2.
> > > 
> > > David did mention a way for me to detect it. I wasn't sure if there
> > > was a deadline to get this patch out so I submitted it now. I will
> > > work on the detection portion of it while the rest gets review.
> > > It's something like, check if the bus address == the physical
> > > address and if not, assume the IOMMU is in use.
> > 
> > And for starters, we can check no_iommu. But really we want
> > intel_iommu.c to report whether or not it enabled VTd on the gfx.
> >  
> > > > 
> > > > > -	if (lists_empty)
> > > > > +	if (lists_empty && !!dev_priv->mm.gtt->do_idle_maps)
> > > > >  		return 0;
> > > > 
> > > > Is it necessary to change the semantic of this function in cases
> > > > which aren't related to GTT remapping? Seems like you're imposing
> > > > a fairly high cost on operations which don't actually need it.
> > > > 
> > > 
> > > Not sure I follow. On that specific hunk it's apparently needed. I
> > > tried the patch without this and we got some data back which
> > > suggested it didn't work. It would be nice if we could reproduce
> > > this locally...
> > 
> > Keith is right, there should be no reason to touch that function,
> > and especially not for the normal code paths.
> 
> Crap... This is a bug in the patch.

I'm still can't explain the necessity of forcing the i915_ring_idle()
here. If those two lists are empty, then each of the ring write/active
lists should also be empty and it will be a no-op. Hence bafflement and
time to be slightly afraid.

> > 
> > Also we should not introduce BUG_ON() trivially into user code paths
> > (/dev/agp).
> 
> Remove 'em then?

Not quite, in this case I think returning -ENODEV is the sensible
approach.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-09-23 19:03 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-23  0:11 [PATCH] drm/i915: Ironlake GPU with VT-d fix Ben Widawsky
2011-09-23  4:50 ` Keith Packard
2011-09-23  5:09   ` Ben Widawsky
2011-09-23  7:37     ` Chris Wilson
2011-09-23 15:38       ` Ben Widawsky
2011-09-23 19:03         ` Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.