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

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

[Feng, Boqun]

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(-)

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
-- 
1.7.4.4

Re: [PATCH] drm/i915: fix user irq miss in BSD ring on g4x

[Keith Packard]

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

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);

-- 
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

Re: [PATCH] drm/i915: fix user irq miss in BSD ring on g4x

[Feng, Boqun]

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

Re: [PATCH] drm/i915: fix user irq miss in BSD ring on g4x

[Keith Packard]

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

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.

-- 
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

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

[Feng, Boqun]

On g4x, user interrupt in BSD ring is missed.
This is because though g4x and ironlake share the same bsd_ring, 
their interrupt control interfaces have _two_ differences. 

1.different irq enable/disable functions:
On g4x are i915_enable_irq and i915_disable_irq.
On ironlake are ironlake_enable_irq and ironlake_disable_irq.
2.different irq flag: 
On g4x user interrupt flag in BSD ring on is I915_BSD_USER_INTERRUPT.
On ironlake is GT_BSD_USER_INTERRUPT

Old bsd_ring_get/put_irq call ring_get_irq and ring_get_irq.
ring_get_irq and ring_put_irq only call ironlake_enable/disable_irq.
So comes the irq miss on g4x. 

To fix this, as other rings' code do, conditionally call different 
functions(i915_enable/disable_irq and ironlake_enable/disable_irq)
and use different interrupt flags in bsd_ring_get/put_irq.

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(-)

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
-- 
1.7.4.4

Re: [PATCH] drm/i915: fix user irq miss in BSD ring on g4x

[Konstantin Belousov]

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

On Mon, May 16, 2011 at 04:02:39PM +0800, Feng, Boqun wrote:
> On g4x, user interrupt in BSD ring is missed.
> This is because though g4x and ironlake share the same bsd_ring, 
> their interrupt control interfaces have _two_ differences. 
> 
> 1.different irq enable/disable functions:
> On g4x are i915_enable_irq and i915_disable_irq.
> On ironlake are ironlake_enable_irq and ironlake_disable_irq.
> 2.different irq flag: 
> On g4x user interrupt flag in BSD ring on is I915_BSD_USER_INTERRUPT.
> On ironlake is GT_BSD_USER_INTERRUPT
> 
> Old bsd_ring_get/put_irq call ring_get_irq and ring_get_irq.
> ring_get_irq and ring_put_irq only call ironlake_enable/disable_irq.
> So comes the irq miss on g4x. 
> 
> To fix this, as other rings' code do, conditionally call different 
> functions(i915_enable/disable_irq and ironlake_enable/disable_irq)
> and use different interrupt flags in bsd_ring_get/put_irq.
> 
> Signed-off-by: Feng, Boqun <boqun.feng@intel.com>
> Reviewed-by: Xiang, Haihao <haihao.xiang@intel.com>
> Cc: stable@kernel.org
It seems that the patch removes that last use for ring_get_irq() and
ring_put_irq() ? Are the helpers still needed ?

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

Re: [PATCH] drm/i915: fix user irq miss in BSD ring on g4x

[Keith Packard]

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

On Mon, 16 May 2011 13:27:27 +0300, Konstantin Belousov <kostikbel@gmail.com> wrote:

> It seems that the patch removes that last use for ring_get_irq() and
> ring_put_irq() ? Are the helpers still needed ?

You're right -- these helpers are no longer used and are removed in the
second patch in this sequence.

-- 
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