[PATCH 1/3] drm/i915: rip out old HWSTAM missed irq WA for vlv

[PATCH 1/3] drm/i915: rip out old HWSTAM missed irq WA for vlv

[Daniel Vetter]

This got copy-pasted from an older version. The newer kinds of
workarounds don't need this anymore.

Shame on me for not noticing when picking up the vlv irq patch.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 06286a2..8496fa5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1998,18 +1998,6 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(RING_IMR(GEN6_BSD_RING_BASE), 0);
 	I915_WRITE(RING_IMR(BLT_RING_BASE), 0);
 
-	if (IS_GEN6(dev) || IS_GEN7(dev)) {
-		/* Workaround stalls observed on Sandy Bridge GPUs by
-		 * making the blitter command streamer generate a
-		 * write to the Hardware Status Page for
-		 * MI_USER_INTERRUPT.  This appears to serialize the
-		 * previous seqno write out before the interrupt
-		 * happens.
-		 */
-		I915_WRITE(GEN6_BLITTER_HWSTAM, ~GEN6_BLITTER_USER_INTERRUPT);
-		I915_WRITE(GEN6_BSD_HWSTAM, ~GEN6_BSD_USER_INTERRUPT);
-	}
-
 	/* and GT */
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
-- 
1.7.9.1

[PATCH 2/3] drm/i915: use render gen to switch ring irq functions

[Daniel Vetter]

Top-level interrupt bits are usually found in the display block. It
therefore makes sense to use HAS_PCH_SPLIT in i915_irq.c

But the irq stuff in intel_ring.c only concerns itself with render
core/gt-level interrupt sources. It therefore makes more sense to
switch based on gpu gen.

Kills a vlv special case.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 98ac5c0..465a7da 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -687,7 +687,7 @@ render_ring_get_irq(struct intel_ring_buffer *ring)
 
 	spin_lock(&ring->irq_lock);
 	if (ring->irq_refcount++ == 0) {
-		if (HAS_PCH_SPLIT(dev) || IS_VALLEYVIEW(dev))
+		if (INTEL_INFO(dev)->gen >= 5)
 			ironlake_enable_irq(dev_priv,
 					    GT_PIPE_NOTIFY | GT_USER_INTERRUPT);
 		else
@@ -706,7 +706,7 @@ render_ring_put_irq(struct intel_ring_buffer *ring)
 
 	spin_lock(&ring->irq_lock);
 	if (--ring->irq_refcount == 0) {
-		if (HAS_PCH_SPLIT(dev) || IS_VALLEYVIEW(dev))
+		if (INTEL_INFO(dev)->gen >= 5)
 			ironlake_disable_irq(dev_priv,
 					     GT_USER_INTERRUPT |
 					     GT_PIPE_NOTIFY);
-- 
1.7.9.1

[PATCH 3/3] drm/i915: extract gt interrupt handler

[Daniel Vetter]

vlv, ivb and snb all share the gen6+ gt irq handling. 3 copies of the
same stuff is a bit much, so extract it into a little helper.

Now ilk has a different gt irq handling than snb, but shares the same
irq handler (due to the similar display block). So also extract the
ilk gt irq handling to clearly separate these two things.

Nice side effect of this is that we can complete Ben Widawsky's gen6+
irq bit #define cleanup and call the render irq also with the GEN6
alias. Beforehand that code was shared with ilk, and neither option
really made much sense.

As a bonus this enables the error interrupt handling lifted from the
vlv code on snb and ivb, too.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c |   66 ++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8496fa5..6d76ee5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -430,6 +430,27 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	mutex_unlock(&dev_priv->dev->struct_mutex);
 }
 
+static void snb_gt_irq_handler(struct drm_device *dev,
+			       struct drm_i915_private *dev_priv,
+			       u32 gt_iir)
+{
+
+	if (gt_iir & (GEN6_RENDER_USER_INTERRUPT |
+		      GEN6_RENDER_PIPE_CONTROL_NOTIFY_INTERRUPT))
+		notify_ring(dev, &dev_priv->ring[RCS]);
+	if (gt_iir & GEN6_BSD_USER_INTERRUPT)
+		notify_ring(dev, &dev_priv->ring[VCS]);
+	if (gt_iir & GEN6_BLITTER_USER_INTERRUPT)
+		notify_ring(dev, &dev_priv->ring[BCS]);
+
+	if (gt_iir & (GT_GEN6_BLT_CS_ERROR_INTERRUPT |
+		      GT_GEN6_BSD_CS_ERROR_INTERRUPT |
+		      GT_RENDER_CS_ERROR_INTERRUPT)) {
+		DRM_ERROR("GT error interrupt 0x%08x\n", gt_iir);
+		i915_handle_error(dev, false);
+	}
+}
+
 static irqreturn_t valleyview_irq_handler(DRM_IRQ_ARGS)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
@@ -458,19 +479,7 @@ static irqreturn_t valleyview_irq_handler(DRM_IRQ_ARGS)
 
 		ret = IRQ_HANDLED;
 
-		if (gt_iir & (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
-			notify_ring(dev, &dev_priv->ring[RCS]);
-		if (gt_iir & GT_GEN6_BSD_USER_INTERRUPT)
-			notify_ring(dev, &dev_priv->ring[VCS]);
-		if (gt_iir & GT_GEN6_BLT_USER_INTERRUPT)
-			notify_ring(dev, &dev_priv->ring[BCS]);
-
-		if (gt_iir & (GT_GEN6_BLT_CS_ERROR_INTERRUPT |
-			      GT_GEN6_BSD_CS_ERROR_INTERRUPT |
-			      GT_RENDER_CS_ERROR_INTERRUPT)) {
-			DRM_ERROR("GT error interrupt 0x%08x\n", gt_iir);
-			i915_handle_error(dev, false);
-		}
+		snb_gt_irq_handler(dev, dev_priv, gt_iir);
 
 		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 		for_each_pipe(pipe) {
@@ -618,12 +627,7 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
 				READ_BREADCRUMB(dev_priv);
 	}
 
-	if (gt_iir & (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
-		notify_ring(dev, &dev_priv->ring[RCS]);
-	if (gt_iir & GEN6_BSD_USER_INTERRUPT)
-		notify_ring(dev, &dev_priv->ring[VCS]);
-	if (gt_iir & GEN6_BLITTER_USER_INTERRUPT)
-		notify_ring(dev, &dev_priv->ring[BCS]);
+	snb_gt_irq_handler(dev, dev_priv, gt_iir);
 
 	if (de_iir & DE_GSE_IVB)
 		intel_opregion_gse_intr(dev);
@@ -675,6 +679,16 @@ done:
 	return ret;
 }
 
+static void ilk_gt_irq_handler(struct drm_device *dev,
+			       struct drm_i915_private *dev_priv,
+			       u32 gt_iir)
+{
+	if (gt_iir & (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
+		notify_ring(dev, &dev_priv->ring[RCS]);
+	if (gt_iir & GT_BSD_USER_INTERRUPT)
+		notify_ring(dev, &dev_priv->ring[VCS]);
+}
+
 static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
@@ -683,13 +697,9 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
 	u32 de_iir, gt_iir, de_ier, pch_iir, pm_iir;
 	u32 hotplug_mask;
 	struct drm_i915_master_private *master_priv;
-	u32 bsd_usr_interrupt = GT_BSD_USER_INTERRUPT;
 
 	atomic_inc(&dev_priv->irq_received);
 
-	if (IS_GEN6(dev))
-		bsd_usr_interrupt = GEN6_BSD_USER_INTERRUPT;
-
 	/* disable master interrupt before clearing iir  */
 	de_ier = I915_READ(DEIER);
 	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
@@ -718,12 +728,10 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
 				READ_BREADCRUMB(dev_priv);
 	}
 
-	if (gt_iir & (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
-		notify_ring(dev, &dev_priv->ring[RCS]);
-	if (gt_iir & bsd_usr_interrupt)
-		notify_ring(dev, &dev_priv->ring[VCS]);
-	if (gt_iir & GEN6_BLITTER_USER_INTERRUPT)
-		notify_ring(dev, &dev_priv->ring[BCS]);
+	if (IS_GEN5(dev))
+		ilk_gt_irq_handler(dev, dev_priv, gt_iir);
+	else
+		snb_gt_irq_handler(dev, dev_priv, gt_iir);
 
 	if (de_iir & DE_GSE)
 		intel_opregion_gse_intr(dev);
-- 
1.7.9.1

Re: [PATCH 3/3] drm/i915: extract gt interrupt handler

[Jesse Barnes]

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

On Fri, 30 Mar 2012 20:24:35 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> vlv, ivb and snb all share the gen6+ gt irq handling. 3 copies of the
> same stuff is a bit much, so extract it into a little helper.
> 
> Now ilk has a different gt irq handling than snb, but shares the same
> irq handler (due to the similar display block). So also extract the
> ilk gt irq handling to clearly separate these two things.
> 
> Nice side effect of this is that we can complete Ben Widawsky's gen6+
> irq bit #define cleanup and call the render irq also with the GEN6
> alias. Beforehand that code was shared with ilk, and neither option
> really made much sense.
> 
> As a bonus this enables the error interrupt handling lifted from the
> vlv code on snb and ivb, too.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Nice cleanup.  Though I don't really like the IS_GEN5 branch in
ironlake_irq_handler... might be nicer to just bite the bullet and have
a mostly duplicate snb irq handler.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 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 3/3] drm/i915: extract gt interrupt handler

[Ben Widawsky]

On Fri, 30 Mar 2012 11:28:40 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Fri, 30 Mar 2012 20:24:35 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > vlv, ivb and snb all share the gen6+ gt irq handling. 3 copies of
> > the same stuff is a bit much, so extract it into a little helper.
> > 
> > Now ilk has a different gt irq handling than snb, but shares the
> > same irq handler (due to the similar display block). So also
> > extract the ilk gt irq handling to clearly separate these two
> > things.
> > 
> > Nice side effect of this is that we can complete Ben Widawsky's
> > gen6+ irq bit #define cleanup and call the render irq also with the
> > GEN6 alias. Beforehand that code was shared with ilk, and neither
> > option really made much sense.
> > 
> > As a bonus this enables the error interrupt handling lifted from the
> > vlv code on snb and ivb, too.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Nice cleanup.  Though I don't really like the IS_GEN5 branch in
> ironlake_irq_handler... might be nicer to just bite the bullet and
> have a mostly duplicate snb irq handler.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 

I agree with Jesse. Bite the bullet, you're already +LOC (see below),
may as well give it a nice clean split.

Personally, I'd like to give you crap about the fact that your
"cleanup" had a +LOC, which came up recently regarding my ILK
context stuff.

Antagonized-by: Ben Widawsky <ben@bwidawsk.net>

Re: [PATCH 3/3] drm/i915: extract gt interrupt handler

[Daniel Vetter]

On Fri, Mar 30, 2012 at 09:31:27PM -0700, Ben Widawsky wrote:
> On Fri, 30 Mar 2012 11:28:40 -0700
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > On Fri, 30 Mar 2012 20:24:35 +0200
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > 
> > > vlv, ivb and snb all share the gen6+ gt irq handling. 3 copies of
> > > the same stuff is a bit much, so extract it into a little helper.
> > > 
> > > Now ilk has a different gt irq handling than snb, but shares the
> > > same irq handler (due to the similar display block). So also
> > > extract the ilk gt irq handling to clearly separate these two
> > > things.
> > > 
> > > Nice side effect of this is that we can complete Ben Widawsky's
> > > gen6+ irq bit #define cleanup and call the render irq also with the
> > > GEN6 alias. Beforehand that code was shared with ilk, and neither
> > > option really made much sense.
> > > 
> > > As a bonus this enables the error interrupt handling lifted from the
> > > vlv code on snb and ivb, too.
> > > 
> > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Nice cleanup.  Though I don't really like the IS_GEN5 branch in
> > ironlake_irq_handler... might be nicer to just bite the bullet and
> > have a mostly duplicate snb irq handler.
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> 
> I agree with Jesse. Bite the bullet, you're already +LOC (see below),
> may as well give it a nice clean split.

I've already replied to Jesse idea on irc, so let me repeat it here: I
considered the ugliness and dropped the idea. If things like this get
common (i.e. reusing the display block, which dictates the master irq
controller, with a different gt render core) we could introduce a vfunc.
But I don't like splitting this up just for the sake of it, because the
display controller on ironlake and snb _is_ pretty much the same thing (at
least wrt interrupt handling), the thing that's different is the gt core.

> Personally, I'd like to give you crap about the fact that your
> "cleanup" had a +LOC, which came up recently regarding my ILK
> context stuff.

Meh, the +LOC argument was just bikeshed, my real argument is that you
screw up your context abstraction by trying to wrestle it into something
that it isn't suitable for.

> Antagonized-by: Ben Widawsky <ben@bwidawsk.net>

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/3] drm/i915: extract gt interrupt handler

[Daniel Vetter]

On Fri, Mar 30, 2012 at 09:31:27PM -0700, Ben Widawsky wrote:
> On Fri, 30 Mar 2012 11:28:40 -0700
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > On Fri, 30 Mar 2012 20:24:35 +0200
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > 
> > > vlv, ivb and snb all share the gen6+ gt irq handling. 3 copies of
> > > the same stuff is a bit much, so extract it into a little helper.
> > > 
> > > Now ilk has a different gt irq handling than snb, but shares the
> > > same irq handler (due to the similar display block). So also
> > > extract the ilk gt irq handling to clearly separate these two
> > > things.
> > > 
> > > Nice side effect of this is that we can complete Ben Widawsky's
> > > gen6+ irq bit #define cleanup and call the render irq also with the
> > > GEN6 alias. Beforehand that code was shared with ilk, and neither
> > > option really made much sense.
> > > 
> > > As a bonus this enables the error interrupt handling lifted from the
> > > vlv code on snb and ivb, too.
> > > 
> > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Nice cleanup.  Though I don't really like the IS_GEN5 branch in
> > ironlake_irq_handler... might be nicer to just bite the bullet and
> > have a mostly duplicate snb irq handler.
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> 
> I agree with Jesse. Bite the bullet, you're already +LOC (see below),
> may as well give it a nice clean split.
> 
> Personally, I'd like to give you crap about the fact that your
> "cleanup" had a +LOC, which came up recently regarding my ILK
> context stuff.
> 
> Antagonized-by: Ben Widawsky <ben@bwidawsk.net>

Ok, I've picked these 3 patches up for -next, with Jesse's r-b extended to
all three (discussed quickyl on irc). Thanks for the review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48