diff mbox

drm/i915: fix user irq miss in BSD ring on g4x

Message ID 1304397744-29312-1-git-send-email-boqun.feng@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Feng, Boqun May 3, 2011, 4:42 a.m. UTC 
On g4x, user interrupt in BSD ring is missed.
g4x and ironlake share the same bsd_ring, but their interrupt control
interfaces are different. On g4x i915_enable_irq and i915_disable_irq
are used to enable/disable irq,and user interrupt flag in BSD ring on
g4x is I915_BSD_USER_INTERRUPT.
The ring_get_irq and ring_put_irq use ironlake style interrupt control
interface. So rather than use them, expand their code and add an if-else
statement about the device version.


Signed-off-by: Feng, Boqun <boqun.feng@intel.com>
Reviewed-by: Xiang, Haihao <haihao.xiang@intel.com>
Cc: stable@kernel.org
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   29 +++++++++++++++++++++++++++--
 1 files changed, 27 insertions(+), 2 deletions(-)

Comments

Keith Packard May 13, 2011, 3:07 p.m. UTC | #1 
On Tue,  3 May 2011 12:42:24 +0800, "Feng, Boqun" <boqun.feng@intel.com> wrote:
> On g4x, user interrupt in BSD ring is missed.
> g4x and ironlake share the same bsd_ring, but their interrupt control
> interfaces are different. On g4x i915_enable_irq and i915_disable_irq
> are used to enable/disable irq,and user interrupt flag in BSD ring on
> g4x is I915_BSD_USER_INTERRUPT.
> The ring_get_irq and ring_put_irq use ironlake style interrupt control
> interface. So rather than use them, expand their code and add an if-else
> statement about the device version.

Please don't open-code ring_get_irq here. I'd suggest a cleaner fix
would be to either just conditionally call ring_get_irq, or to stick the
BSD interrupt value in dev_priv where you can get it:

        if (IS_G4X(dev))
                ring_get_irq(ring, I915_BSD_USER_INTERRUPT);
        else
                ring_get_irq(ring, GT_BSD_USER_INTERRUPT);

or
        ring_get_irq(ring, dev_priv->bsd_user_interrupt);
Feng, Boqun May 16, 2011, 4:43 a.m. UTC | #2 
Thanks for your advice. Maybe my patch log fail to _clearly_ show that there are
_two_ differences about irq control interface between gen4 and gen5

> -----Original Message-----
> From: Keith Packard [mailto:keithp@keithp.com]
> Sent: Friday, May 13, 2011 11:08 PM
> To: Feng, Boqun; intel-gfx@lists.freedesktop.org
> Cc: stable@kernel.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: fix user irq miss in BSD ring on g4x
> 
> On Tue,  3 May 2011 12:42:24 +0800, "Feng, Boqun" <boqun.feng@intel.com>
> wrote:
> > On g4x, user interrupt in BSD ring is missed.
> > g4x and ironlake share the same bsd_ring, but their interrupt control
> > interfaces are different. On g4x i915_enable_irq and i915_disable_irq
                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

1st, on gen5, we use ironlake_enable_irq and ironlake_disable_irq

> > are used to enable/disable irq,and user interrupt flag in BSD ring on
> > g4x is I915_BSD_USER_INTERRUPT.
        ~~~~~~~~~~~~~~~~~~~~~~~

2nd, on gen5, we use GT_BSD_USER_INTERRUPT

> > The ring_get_irq and ring_put_irq use ironlake style interrupt control
> > interface. So rather than use them, expand their code and add an if-else
> > statement about the device version.
> 
> Please don't open-code ring_get_irq here. I'd suggest a cleaner fix
> would be to either just conditionally call ring_get_irq, or to stick the
> BSD interrupt value in dev_priv where you can get it:
> 
>         if (IS_G4X(dev))
>                 ring_get_irq(ring, I915_BSD_USER_INTERRUPT);
>         else
>                 ring_get_irq(ring, GT_BSD_USER_INTERRUPT);
> 

So, we can't simply conditionally call ring_get_irq, we need to add a
conditionally call in ring_get_irq, too:
        if(is_G4X(dev))
                i915_enable_irq(ring, flag);
        else
                ironlake_enable_irq(ring, flag);

That's why I want to open-code ring_get_irq.

> or
>         ring_get_irq(ring, dev_priv->bsd_user_interrupt);
> 
> --
> keith.packard@intel.com

--
Feng, Boqun
Keith Packard May 16, 2011, 7:07 a.m. UTC | #3 
On Mon, 16 May 2011 12:43:23 +0800, "Feng, Boqun" <boqun.feng@intel.com> wrote:

> That's why I want to open-code ring_get_irq.

Right, I read through the code again and found that you're just
mirroring the structure of code for the other two rings, neither of
which shared the ring_get_irq/ring_put_irq functions anyways.

Thanks for the explanation; please update the commit message to include
enough of this information to make it clear too.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index e9e6f71..c4504a2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -666,12 +666,37 @@  gen6_ring_put_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
 static bool
 bsd_ring_get_irq(struct intel_ring_buffer *ring)
 {
-	return ring_get_irq(ring, GT_BSD_USER_INTERRUPT);
+	struct drm_device *dev = ring->dev;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	if (!dev->irq_enabled)
+		return false;
+
+	spin_lock(&ring->irq_lock);
+	if (ring->irq_refcount++ == 0) {
+		if (IS_G4X(dev))
+			i915_enable_irq(dev_priv, I915_BSD_USER_INTERRUPT);
+		else
+			ironlake_enable_irq(dev_priv, GT_BSD_USER_INTERRUPT);
+	}
+	spin_unlock(&ring->irq_lock);
+
+	return true;
 }
 static void
 bsd_ring_put_irq(struct intel_ring_buffer *ring)
 {
-	ring_put_irq(ring, GT_BSD_USER_INTERRUPT);
+	struct drm_device *dev = ring->dev;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	spin_lock(&ring->irq_lock);
+	if (--ring->irq_refcount == 0) {
+		if (IS_G4X(dev))
+			i915_disable_irq(dev_priv, I915_BSD_USER_INTERRUPT);
+		else
+			ironlake_disable_irq(dev_priv, GT_BSD_USER_INTERRUPT);
+	}
+	spin_unlock(&ring->irq_lock);
 }
 
 static int