diff mbox

drm/i915: stop using GMBUS IRQs on Gen4 chips (was Re: [3.9-rc1] irq 16: nobody cared (was [3.9-rc1] very poor interrupt responses))

Message ID alpine.LNX.2.00.1303181522460.9529@pobox.suse.cz (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Jiri Kosina March 18, 2013, 3:56 p.m. UTC 
Okay, so I think that for 3.9 we want the patch below, and if eventually 
hardware root cause / workaround is found for GM45, we can have it merged 
later.



From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] drm/i915: stop using GMBUS IRQs on Gen4 chips

Commit 28c70f162 ("drm/i915: use the gmbus irq for waits") switched to
using GMBUS irqs instead of GPIO bit-banging for chipset generations 4
and above.

It turns out though that on many systems this leads to spurious interrupts
being generated, long after the register write to disable the IRQs has been
issued.

Flushing of the register writes by POSTING_READ() directly after the register
write doesn't work either.

Disable using of GMBUS IRQs on Gen4 systems before the root cause is found and
revert back to old behavior.

Also be more careful about not issuing GMBUS4 register reads in 
gmbus_wait_hw_status() if we are not using GMBUS IRQs.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/gpu/drm/i915/intel_i2c.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

Comments

Chris Wilson March 18, 2013, 5:04 p.m. UTC | #1 
On Mon, Mar 18, 2013 at 04:56:02PM +0100, Jiri Kosina wrote:
> Okay, so I think that for 3.9 we want the patch below, and if eventually 
> hardware root cause / workaround is found for GM45, we can have it merged 
> later.
> 
> 
> 
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] drm/i915: stop using GMBUS IRQs on Gen4 chips
> 
> Commit 28c70f162 ("drm/i915: use the gmbus irq for waits") switched to
> using GMBUS irqs instead of GPIO bit-banging for chipset generations 4
> and above.
> 
> It turns out though that on many systems this leads to spurious interrupts
> being generated, long after the register write to disable the IRQs has been
> issued.
> 
> Flushing of the register writes by POSTING_READ() directly after the register
> write doesn't work either.
> 
> Disable using of GMBUS IRQs on Gen4 systems before the root cause is found and
> revert back to old behavior.
> 
> Also be more careful about not issuing GMBUS4 register reads in 
> gmbus_wait_hw_status() if we are not using GMBUS IRQs.
> 
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index acf8aec..8638036 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -58,12 +58,14 @@ to_intel_gmbus(struct i2c_adapter *i2c)
>  	return container_of(i2c, struct intel_gmbus, adapter);
>  }
>  
> +#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 5)
>  void
>  intel_i2c_reset(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS0, 0);
> -	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0);
> +	if (HAS_GMBUS_IRQ(dev))
> +		I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0);

There should not be any harm in always clearing GMBUS4, it exists on all
platforms.

>  }
>  
>  static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable)
> @@ -203,7 +205,6 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
>  	algo->data = bus;
>  }
>  
> -#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 4)
>  static int
>  gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
>  		     u32 gmbus2_status,
> @@ -214,6 +215,13 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
>  	u32 gmbus2 = 0;
>  	DEFINE_WAIT(wait);
>  
> +	if (!HAS_GMBUS_IRQ(dev_priv->dev)) {
> +		int ret;
> +		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> +				(GMBUS_SATOER | gmbus2_status),
> +				50);

This should couple up to the normal return code that chooses the
appropriate return value based on gmbus2.

How about just using:
  if (!HAS_GMBUS_IRQ(dev_priv->dev)) gmbus4_irq_en = 0;
and the existing wait loop?

> +		return ret;
> +	}
>  	/* Important: The hw handles only the first bit, so set only one! Since
>  	 * we also need to check for NAKs besides the hw ready/idle signal, we
>  	 * need to wake up periodically and check that ourselves. */
Daniel Vetter March 18, 2013, 7:21 p.m. UTC | #2 
On Mon, Mar 18, 2013 at 04:56:02PM +0100, Jiri Kosina wrote:
> Okay, so I think that for 3.9 we want the patch below, and if eventually 
> hardware root cause / workaround is found for GM45, we can have it merged 
> later.

I'd prefer if we dig into this for a bit more. I've been traveling last
week and only just now recovered from the mail backlog, hence my delays in
handling this.

Also the "nobody cared" showing up only very late after the last gmbus
transaction isn't too surprising: It only fires after 100k interrupts, and
apparently the irq handler is already a bit racy, so any interrupt might
push it over the edge. gmbus interrupts are apparently just help to expose
the race (presuming it's indeed the msi race already docuemented in the
code).
-Daniel

> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] drm/i915: stop using GMBUS IRQs on Gen4 chips
> 
> Commit 28c70f162 ("drm/i915: use the gmbus irq for waits") switched to
> using GMBUS irqs instead of GPIO bit-banging for chipset generations 4
> and above.
> 
> It turns out though that on many systems this leads to spurious interrupts
> being generated, long after the register write to disable the IRQs has been
> issued.
> 
> Flushing of the register writes by POSTING_READ() directly after the register
> write doesn't work either.
> 
> Disable using of GMBUS IRQs on Gen4 systems before the root cause is found and
> revert back to old behavior.
> 
> Also be more careful about not issuing GMBUS4 register reads in 
> gmbus_wait_hw_status() if we are not using GMBUS IRQs.
> 
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index acf8aec..8638036 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -58,12 +58,14 @@ to_intel_gmbus(struct i2c_adapter *i2c)
>  	return container_of(i2c, struct intel_gmbus, adapter);
>  }
>  
> +#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 5)
>  void
>  intel_i2c_reset(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS0, 0);
> -	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0);
> +	if (HAS_GMBUS_IRQ(dev))
> +		I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0);
>  }
>  
>  static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable)
> @@ -203,7 +205,6 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
>  	algo->data = bus;
>  }
>  
> -#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 4)
>  static int
>  gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
>  		     u32 gmbus2_status,
> @@ -214,6 +215,13 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
>  	u32 gmbus2 = 0;
>  	DEFINE_WAIT(wait);
>  
> +	if (!HAS_GMBUS_IRQ(dev_priv->dev)) {
> +		int ret;
> +		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> +				(GMBUS_SATOER | gmbus2_status),
> +				50);
> +		return ret;
> +	}
>  	/* Important: The hw handles only the first bit, so set only one! Since
>  	 * we also need to check for NAKs besides the hw ready/idle signal, we
>  	 * need to wake up periodically and check that ourselves. */
> 
> -- 
> Jiri Kosina
> SUSE Labs
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index acf8aec..8638036 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -58,12 +58,14 @@  to_intel_gmbus(struct i2c_adapter *i2c)
 	return container_of(i2c, struct intel_gmbus, adapter);
 }
 
+#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 5)
 void
 intel_i2c_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS0, 0);
-	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0);
+	if (HAS_GMBUS_IRQ(dev))
+		I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0);
 }
 
 static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable)
@@ -203,7 +205,6 @@  intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 	algo->data = bus;
 }
 
-#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 4)
 static int
 gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
 		     u32 gmbus2_status,
@@ -214,6 +215,13 @@  gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
 	u32 gmbus2 = 0;
 	DEFINE_WAIT(wait);
 
+	if (!HAS_GMBUS_IRQ(dev_priv->dev)) {
+		int ret;
+		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
+				(GMBUS_SATOER | gmbus2_status),
+				50);
+		return ret;
+	}
 	/* Important: The hw handles only the first bit, so set only one! Since
 	 * we also need to check for NAKs besides the hw ready/idle signal, we
 	 * need to wake up periodically and check that ourselves. */