[Intel-gfx] [PATCH 14/14] drm/i915: inline enable/disable_irq into ring->get/put_irq
Now that these are properly refactored this additional indirection
doesn't really buy us anything but confusion. Hence inline them.
This duplicates the ironlake gt enable/disable code snippet, but we've
already separate ilk from gen6+ gt irq in i915_irq.c, so I think this
makes more sense.
Signed-Off-by: Daniel Vetter
---
drivers/gpu/drm/i915/intel_ringbuffer.c | 68 ---
1 files changed, 26 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 108bfd6..2ffde00 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -585,38 +585,6 @@ pc_render_get_seqno(struct intel_ring_buffer *ring)
return pc->cpu_page[0];
}
-static void
-ironlake_enable_irq(drm_i915_private_t *dev_priv, u32 mask)
-{
- dev_priv->gt_irq_mask &= ~mask;
- I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
- POSTING_READ(GTIMR);
-}
-
-static void
-ironlake_disable_irq(drm_i915_private_t *dev_priv, u32 mask)
-{
- dev_priv->gt_irq_mask |= mask;
- I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
- POSTING_READ(GTIMR);
-}
-
-static void
-i915_enable_irq(drm_i915_private_t *dev_priv, u32 mask)
-{
- dev_priv->irq_mask &= ~mask;
- I915_WRITE(IMR, dev_priv->irq_mask);
- POSTING_READ(IMR);
-}
-
-static void
-i915_disable_irq(drm_i915_private_t *dev_priv, u32 mask)
-{
- dev_priv->irq_mask |= mask;
- I915_WRITE(IMR, dev_priv->irq_mask);
- POSTING_READ(IMR);
-}
-
static bool
gen5_ring_get_irq(struct intel_ring_buffer *ring)
{
@@ -627,8 +595,11 @@ gen5_ring_get_irq(struct intel_ring_buffer *ring)
return false;
spin_lock(&ring->irq_lock);
- if (ring->irq_refcount++ == 0)
- ironlake_enable_irq(dev_priv, ring->irq_enable_mask);
+ if (ring->irq_refcount++ == 0) {
+ dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
+ I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+ POSTING_READ(GTIMR);
+ }
spin_unlock(&ring->irq_lock);
return true;
@@ -641,8 +612,11 @@ gen5_ring_put_irq(struct intel_ring_buffer *ring)
drm_i915_private_t *dev_priv = dev->dev_private;
spin_lock(&ring->irq_lock);
- if (--ring->irq_refcount == 0)
- ironlake_disable_irq(dev_priv, ring->irq_enable_mask);
+ if (--ring->irq_refcount == 0) {
+ dev_priv->gt_irq_mask |= ring->irq_enable_mask;
+ I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+ POSTING_READ(GTIMR);
+ }
spin_unlock(&ring->irq_lock);
}
@@ -656,8 +630,11 @@ i9xx_ring_get_irq(struct intel_ring_buffer *ring)
return false;
spin_lock(&ring->irq_lock);
- if (ring->irq_refcount++ == 0)
- i915_enable_irq(dev_priv, ring->irq_enable_mask);
+ if (ring->irq_refcount++ == 0) {
+ dev_priv->irq_mask &= ~ring->irq_enable_mask;
+ I915_WRITE(IMR, dev_priv->irq_mask);
+ POSTING_READ(IMR);
+ }
spin_unlock(&ring->irq_lock);
return true;
@@ -670,8 +647,11 @@ i9xx_ring_put_irq(struct intel_ring_buffer *ring)
drm_i915_private_t *dev_priv = dev->dev_private;
spin_lock(&ring->irq_lock);
- if (--ring->irq_refcount == 0)
- i915_disable_irq(dev_priv, ring->irq_enable_mask);
+ if (--ring->irq_refcount == 0) {
+ dev_priv->irq_mask |= ring->irq_enable_mask;
+ I915_WRITE(IMR, dev_priv->irq_mask);
+ POSTING_READ(IMR);
+ }
spin_unlock(&ring->irq_lock);
}
@@ -763,7 +743,9 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring)
spin_lock(&ring->irq_lock);
if (ring->irq_refcount++ == 0) {
I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
- ironlake_enable_irq(dev_priv, ring->irq_enable_mask);
+ dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
+ I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+ POSTING_READ(GTIMR);
}
spin_unlock(&ring->irq_lock);
@@ -779,7 +761,9 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring)
spin_lock(&ring->irq_lock);
if (--ring->irq_refcount == 0) {
I915_WRITE_IMR(ring, ~0);
- ironlake_disable_irq(dev_priv, ring->irq_enable_mask);
+ dev_priv->gt_irq_mask |= ring->irq_enable_mask;
+ I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+ POSTING_READ(GTIMR);
}
spin_unlock(&ring->irq_lock);
--
1.7.7.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/14] drm/i915: inline enable/disable_irq into ring->get/put_irq
On Wed, 11 Apr 2012 22:12:59 +0200 Daniel Vetter wrote: > Now that these are properly refactored this additional indirection > doesn't really buy us anything but confusion. Hence inline them. > > This duplicates the ironlake gt enable/disable code snippet, but we've > already separate ilk from gen6+ gt irq in i915_irq.c, so I think this > makes more sense. > > Signed-Off-by: Daniel Vetter Bikeshed: While doing all this, I think put/get irq is really terribly named. I was a much bigger fan of the enable disable. Also, you could use a bit of flow control to write to the correct IMR register and not duplicate functions at all. You already do the POSTING_READ so performance shouldn't matter. Something like... uint32_t imr = GEN(dev) >= 5 ? GTIMR: IMR; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/14] drm/i915: inline enable/disable_irq into ring->get/put_irq
On Thu, Apr 12, 2012 at 06:03:30PM -0700, Ben Widawsky wrote: > On Wed, 11 Apr 2012 22:12:59 +0200 > Daniel Vetter wrote: > > > Now that these are properly refactored this additional indirection > > doesn't really buy us anything but confusion. Hence inline them. > > > > This duplicates the ironlake gt enable/disable code snippet, but we've > > already separate ilk from gen6+ gt irq in i915_irq.c, so I think this > > makes more sense. > > > > Signed-Off-by: Daniel Vetter > > Bikeshed: > While doing all this, I think put/get irq is really terribly named. I > was a much bigger fan of the enable disable. Actually that can be combined with Chris' bikeshed to move the ring->put|get_irg to dev_priv->core.ring_enable/disable_irq. But I've figured that the same patch series should also move the forcewake function pointers from dev_priv->display to dev_priv->core, so this is imo a different patch series. > Also, you could use a bit of flow control to write to the correct IMR > register and not duplicate functions at all. You already do the > POSTING_READ so performance shouldn't matter. > > Something like... > > uint32_t imr = GEN(dev) >= 5 ? GTIMR: IMR; I've figured I want to ditch all IS_GEN checks from these functions. But for this case it makes some sense. Imo could be done with the follow-up though. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/14] drm/i915: inline enable/disable_irq into ring->get/put_irq
On Fri, 13 Apr 2012 11:13:53 +0200, Daniel Vetter wrote: > On Thu, Apr 12, 2012 at 06:03:30PM -0700, Ben Widawsky wrote: > > On Wed, 11 Apr 2012 22:12:59 +0200 > > Daniel Vetter wrote: > > > > > Now that these are properly refactored this additional indirection > > > doesn't really buy us anything but confusion. Hence inline them. > > > > > > This duplicates the ironlake gt enable/disable code snippet, but we've > > > already separate ilk from gen6+ gt irq in i915_irq.c, so I think this > > > makes more sense. > > > > > > Signed-Off-by: Daniel Vetter > > > > Bikeshed: > > While doing all this, I think put/get irq is really terribly named. I > > was a much bigger fan of the enable disable. > > Actually that can be combined with Chris' bikeshed to move the > ring->put|get_irg to dev_priv->core.ring_enable/disable_irq. But I've > figured that the same patch series should also move the forcewake function > pointers from dev_priv->display to dev_priv->core, so this is imo a > different patch series. Not quite, they are get/put functions, as we do have multiple waiters sharing the irq. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
