subject:"Re\: \[PATCH\] 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\)\)"

Re: [PATCH] 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))

2013-03-19 Thread Jiri Kosina

On Mon, 18 Mar 2013, Chris Wilson wrote:

  +#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?

I explicitly wanted to avoid touching GMBUS4 register, as the real cause 
of the failure is not clear.

But, as Yinghai Lu points out, the problem is most likely caused by 
interrupt disabling not working properly (see his very good point 
regarding DisINTx+ and INTx+ discrepancy), so zeroing the register out 
should work  and it indeed does in my case, hence the (tested) patch 
below.

I think it's a 3.9-rc material, and I am all open to debug this further 
for 3.10 so that the race is closed and gmbus irqs can be used on Gen4 
platform properly.




From: Jiri Kosina jkos...@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.

Signed-off-by: Jiri Kosina jkos...@suse.cz
---
 drivers/gpu/drm/i915/intel_i2c.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index acf8aec..9934724 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -203,7 +203,7 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
algo-data = bus;
 }
 
-#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)-gen = 4)
+#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)-gen = 5)
 static int
 gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
 u32 gmbus2_status,
@@ -214,6 +214,8 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
u32 gmbus2 = 0;
DEFINE_WAIT(wait);
 
+   if (!HAS_GMBUS_IRQ(dev_priv-dev))
+   gmbus4_irq_en = 0;
/* 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
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] 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))

2013-03-19 Thread Chris Wilson

On Tue, Mar 19, 2013 at 09:56:57AM +0100, Jiri Kosina wrote:
 On Mon, 18 Mar 2013, Chris Wilson wrote:
 
   +#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?
 
 I explicitly wanted to avoid touching GMBUS4 register, as the real cause 
 of the failure is not clear.
 
 But, as Yinghai Lu points out, the problem is most likely caused by 
 interrupt disabling not working properly (see his very good point 
 regarding DisINTx+ and INTx+ discrepancy), so zeroing the register out 
 should work  and it indeed does in my case, hence the (tested) patch 
 below.
 
 I think it's a 3.9-rc material, and I am all open to debug this further 
 for 3.10 so that the race is closed and gmbus irqs can be used on Gen4 
 platform properly.

Agreed. Using the IRQ for GMBUS is just a performance feature that can
be deferred until after we determine the root cause - and hope that the
failure is somehow peculiar to GMBUS.

Acked-by: Chris Wilson ch...@chris-wilson.co.uk
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

gm45 intel gfx can generate non-MSI irq# in MSI mode (was Re: [PATCH] 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

2013-03-19 Thread Daniel Vetter

On Tue, Mar 19, 2013 at 10:03 AM, Chris Wilson ch...@chris-wilson.co.uk wrote:
  How about just using:
if (!HAS_GMBUS_IRQ(dev_priv-dev)) gmbus4_irq_en = 0;
  and the existing wait loop?

 I explicitly wanted to avoid touching GMBUS4 register, as the real cause
 of the failure is not clear.

 But, as Yinghai Lu points out, the problem is most likely caused by
 interrupt disabling not working properly (see his very good point
 regarding DisINTx+ and INTx+ discrepancy), so zeroing the register out
 should work  and it indeed does in my case, hence the (tested) patch
 below.

 I think it's a 3.9-rc material, and I am all open to debug this further
 for 3.10 so that the race is closed and gmbus irqs can be used on Gen4
 platform properly.

 Agreed. Using the IRQ for GMBUS is just a performance feature that can
 be deferred until after we determine the root cause - and hope that the
 failure is somehow peculiar to GMBUS.

Ok, I've merged this patch. But some further investigation points at a
much more severe dragon hiding here: The MSI interrupt for the intel
gfx is commonly in the 40+ range, but the interrupt vector with the
spurious interrupts is 16. Which is the irq of the intel gfx when MSI
is disabled!

So it looks like gmbus on the intel gfx is capable of generating
non-MSI interrupts in parallel to the MSI interrupts (since apparently
gmbus still works, so we get the interrupts we expect). I have no idea
how that could happen. Hence adding a bunch of people with more clue
than me.

For reference below the updated commit message.

Cheers, Daniel

Author: Jiri Kosina jkos...@suse.cz
Date:   Tue Mar 19 09:56:57 2013 +0100

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.

Typically this results in the spurious interrupt source getting
disabled:

[9.636345] irq 16: nobody cared (try booting with the irqpoll option)
[9.637915] Pid: 4157, comm: ifup Tainted: GF
3.9.0-rc2-00341-g0863702 #422
[9.639484] Call Trace:
[9.640731]  IRQ  [8109b40d] __report_bad_irq+0x1d/0xc7
[9.640731]  [8109b7db] note_interrupt+0x15b/0x1e8
[9.640731]  [810999f7] handle_irq_event_percpu+0x1bf/0x214
[9.640731]  [81099a88] handle_irq_event+0x3c/0x5c
[9.640731]  [8109c139] handle_fasteoi_irq+0x7a/0xb0
[9.640731]  [8100400e] handle_irq+0x1a/0x24
[9.640731]  [81003d17] do_IRQ+0x48/0xaf
[9.640731]  [8142f1ea] common_interrupt+0x6a/0x6a
[9.640731]  EOI  [8142f952] ? system_call_fastpath+0x16/0x1b
[9.640731] handlers:
[9.640731] [a000d771] usb_hcd_irq [usbcore]
[9.640731] [a0306189] yenta_interrupt [yenta_socket]
[9.640731] Disabling IRQ #16

The really curious thing is now that irq 16 is _not_ the interrupt for
the i915 driver when using MSI, but it _is_ the interrupt when not
using MSI. So by all indications it seems like gmbus is able to
generate a legacy (shared) interrupt in MSI mode on some
configurations. I've tried to reproduce this and the differentiating
thing seems to be that on unaffected systems no other device uses irq
16 (which seems to be the non-MSI intel gfx interrupt on all gm45).

I have no idea how that even can happen.

To avoid tempting this elephant into a rage, just disable gmbus
interrupt support on gen 4.

v2: Improve the commit message with exact details of what's going on.
Also add a comment in the code to warn against this particular
elephant in the room.

Signed-off-by: Jiri Kosina jkos...@suse.cz (v1)
Acked-by: Chris Wilson ch...@chris-wilson.co.uk (v1)
References: https://lkml.org/lkml/2013/3/8/325
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] 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))

2013-03-18 Thread Chris Wilson

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 jkos...@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 jkos...@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. */

-- 
Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] 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))

Re: [PATCH] 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))

gm45 intel gfx can generate non-MSI irq# in MSI mode (was Re: [PATCH] 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

Re: [PATCH] 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))

4 matches

Site Navigation

Mail list logo

Footer information