[PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Daniel Vetter]

Two things seem to do the trick on my ivb machine here:
- prevent the gt from powering down while waiting for seqno
  notification interrupts by grabbing the force_wake in get_irq (and
  dropping it in put_irq again).
- ordering writes from the ring's CS by reading a CS register, ACTHD
  seems to work.

Only the blt&bsd ring on ivb seem to be massively affected by this,
but for paranoia do this dance also on the render ring and on snb
(i.e. all gpus with forcewake).

Tested with Eric's glCopyPixels loop which without this patch scores a
missed irq every few seconds.

This patch needs my forcewake rework to use a spinlock instead of
dev->struct_mutex.

Cc: stable@kernel.org
Cc: Eric Anholt <eric@anholt.net>
Cc: Kenneth Graunke <kenneth@whitecape.org>
Cc: Eugeni Dodonov <eugeni.dodonov@intel.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---

For easier testing grab my -next branch at

http://cgit.freedesktop.org/~danvet/drm/log/?h=my-next

Cheers, Daniel

 drivers/gpu/drm/i915/intel_ringbuffer.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index cf6e159..dbbc565 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -631,6 +631,15 @@ render_ring_add_request(struct intel_ring_buffer *ring,
 }
 
 static u32
+gen6_ring_get_seqno(struct intel_ring_buffer *ring)
+{
+	/* Workaround for force correct ordering between irq and seqno writes on
+	 * ivb (and maybe also on snb). */
+	intel_ring_get_active_head(ring);
+	return intel_read_status_page(ring, I915_GEM_HWS_INDEX);
+}
+
+static u32
 ring_get_seqno(struct intel_ring_buffer *ring)
 {
 	return intel_read_status_page(ring, I915_GEM_HWS_INDEX);
@@ -795,6 +804,11 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
 	if (!dev->irq_enabled)
 	       return false;
 
+	/* It looks like we need to prevent the gt from suspending while waiting
+	 * for an notifiy irq, otherwise irqs seem to get lost on at least the
+	 * blt/bsd rings. */
+	gen6_gt_force_wake_get(dev_priv);
+
 	spin_lock(&ring->irq_lock);
 	if (ring->irq_refcount++ == 0) {
 		ring->irq_mask &= ~rflag;
@@ -819,6 +833,8 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
 		ironlake_disable_irq(dev_priv, gflag);
 	}
 	spin_unlock(&ring->irq_lock);
+
+	gen6_gt_force_wake_put(dev_priv);
 }
 
 static bool
@@ -1329,7 +1345,7 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
 	.write_tail		= gen6_bsd_ring_write_tail,
 	.flush			= gen6_ring_flush,
 	.add_request		= gen6_add_request,
-	.get_seqno		= ring_get_seqno,
+	.get_seqno		= gen6_ring_get_seqno,
 	.irq_get		= gen6_bsd_ring_get_irq,
 	.irq_put		= gen6_bsd_ring_put_irq,
 	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
@@ -1388,7 +1404,7 @@ static const struct intel_ring_buffer gen6_blt_ring = {
 	.write_tail		= ring_write_tail,
 	.flush			= blt_ring_flush,
 	.add_request		= gen6_add_request,
-	.get_seqno		= ring_get_seqno,
+	.get_seqno		= gen6_ring_get_seqno,
 	.irq_get		= blt_ring_get_irq,
 	.irq_put		= blt_ring_put_irq,
 	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
@@ -1410,6 +1426,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		ring->flush = gen6_render_ring_flush;
 		ring->irq_get = gen6_render_ring_get_irq;
 		ring->irq_put = gen6_render_ring_put_irq;
+		ring->get_seqno = gen6_ring_get_seqno;
 	} else if (IS_GEN5(dev)) {
 		ring->add_request = pc_render_add_request;
 		ring->get_seqno = pc_render_get_seqno;
-- 
1.7.7.3

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Eugeni Dodonov]

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

On Wed, Jan 4, 2012 at 14:52, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Two things seem to do the trick on my ivb machine here:
> - prevent the gt from powering down while waiting for seqno
>  notification interrupts by grabbing the force_wake in get_irq (and
>  dropping it in put_irq again).
> - ordering writes from the ring's CS by reading a CS register, ACTHD
>  seems to work.
>
> Only the blt&bsd ring on ivb seem to be massively affected by this,
> but for paranoia do this dance also on the render ring and on snb
> (i.e. all gpus with forcewake).
>
> Tested with Eric's glCopyPixels loop which without this patch scores a
> missed irq every few seconds.
>
> This patch needs my forcewake rework to use a spinlock instead of
> dev->struct_mutex.
>
> Cc: stable@kernel.org
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> Cc: Eugeni Dodonov <eugeni.dodonov@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>

This voodoo magic works here, no more missed irqs on my IVBs.
Tested-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>


>  static u32
> +gen6_ring_get_seqno(struct intel_ring_buffer *ring)
> +{
> +       /* Workaround for force correct ordering between irq and seqno
> writes on
> +        * ivb (and maybe also on snb). */
> +       intel_ring_get_active_head(ring);
> +       return intel_read_status_page(ring, I915_GEM_HWS_INDEX);
>

<bikeshedding>
 - the comment could be rewritten to "Workaround for force correct ordering
between irq and seqno writes on ivb (and maybe also on snb) by reading a CS
register, like ACTHD, prior reading status page".
 - we could do a 'return ring_get_seqno(ring);' instead of 'return
intel_read_status_page(ring, I915_GEM_HWS_INDEX);' to reduce the Universe
entropy :).
</bikeshedding>

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 2855 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: paper over missed irq issues with force wake vodoo

[Daniel Vetter]

Two things seem to do the trick on my ivb machine here:
- prevent the gt from powering down while waiting for seqno
  notification interrupts by grabbing the force_wake in get_irq (and
  dropping it in put_irq again).
- ordering writes from the ring's CS by reading a CS register, ACTHD
  seems to work.

Only the blt&bsd ring on ivb seem to be massively affected by this,
but for paranoia do this dance also on the render ring and on snb
(i.e. all gpus with forcewake).

Tested with Eric's glCopyPixels loop which without this patch scores a
missed irq every few seconds.

This patch needs my forcewake rework to use a spinlock instead of
dev->struct_mutex.

v2: Improve the comment per Eugeni Dodonov's suggestion.

Cc: stable@kernel.org
Cc: Eric Anholt <eric@anholt.net>
Cc: Kenneth Graunke <kenneth@whitecape.org>
Cc: Eugeni Dodonov <eugeni.dodonov@intel.com>
Tested-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index cf6e159..88499a0 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -631,6 +631,16 @@ render_ring_add_request(struct intel_ring_buffer *ring,
 }
 
 static u32
+gen6_ring_get_seqno(struct intel_ring_buffer *ring)
+{
+	/* Workaround to force correct ordering between irq and seqno writes on
+	 * ivb (and maybe also on snb) by reading from a CS register (like
+	 * ACTHD) before reading the status page. */
+	intel_ring_get_active_head(ring);
+	return intel_read_status_page(ring, I915_GEM_HWS_INDEX);
+}
+
+static u32
 ring_get_seqno(struct intel_ring_buffer *ring)
 {
 	return intel_read_status_page(ring, I915_GEM_HWS_INDEX);
@@ -795,6 +805,11 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
 	if (!dev->irq_enabled)
 	       return false;
 
+	/* It looks like we need to prevent the gt from suspending while waiting
+	 * for an notifiy irq, otherwise irqs seem to get lost on at least the
+	 * blt/bsd rings on ivb. */
+	gen6_gt_force_wake_get(dev_priv);
+
 	spin_lock(&ring->irq_lock);
 	if (ring->irq_refcount++ == 0) {
 		ring->irq_mask &= ~rflag;
@@ -819,6 +834,8 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
 		ironlake_disable_irq(dev_priv, gflag);
 	}
 	spin_unlock(&ring->irq_lock);
+
+	gen6_gt_force_wake_put(dev_priv);
 }
 
 static bool
@@ -1329,7 +1346,7 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
 	.write_tail		= gen6_bsd_ring_write_tail,
 	.flush			= gen6_ring_flush,
 	.add_request		= gen6_add_request,
-	.get_seqno		= ring_get_seqno,
+	.get_seqno		= gen6_ring_get_seqno,
 	.irq_get		= gen6_bsd_ring_get_irq,
 	.irq_put		= gen6_bsd_ring_put_irq,
 	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
@@ -1388,7 +1405,7 @@ static const struct intel_ring_buffer gen6_blt_ring = {
 	.write_tail		= ring_write_tail,
 	.flush			= blt_ring_flush,
 	.add_request		= gen6_add_request,
-	.get_seqno		= ring_get_seqno,
+	.get_seqno		= gen6_ring_get_seqno,
 	.irq_get		= blt_ring_get_irq,
 	.irq_put		= blt_ring_put_irq,
 	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
@@ -1410,6 +1427,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		ring->flush = gen6_render_ring_flush;
 		ring->irq_get = gen6_render_ring_get_irq;
 		ring->irq_put = gen6_render_ring_put_irq;
+		ring->get_seqno = gen6_ring_get_seqno;
 	} else if (IS_GEN5(dev)) {
 		ring->add_request = pc_render_add_request;
 		ring->get_seqno = pc_render_get_seqno;
-- 
1.7.7.3

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Keith Packard]

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

On Wed,  4 Jan 2012 19:40:45 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Two things seem to do the trick on my ivb machine here:
> - prevent the gt from powering down while waiting for seqno
>   notification interrupts by grabbing the force_wake in get_irq (and
>   dropping it in put_irq again).
> - ordering writes from the ring's CS by reading a CS register, ACTHD
>   seems to work.

If this works reliably, you'll deserve a medal...

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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: paper over missed irq issues with force wake vodoo

[Daniel Vetter]

On Wed, Jan 04, 2012 at 06:27:40PM -0800, Keith Packard wrote:
> On Wed,  4 Jan 2012 19:40:45 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > Two things seem to do the trick on my ivb machine here:
> > - prevent the gt from powering down while waiting for seqno
> >   notification interrupts by grabbing the force_wake in get_irq (and
> >   dropping it in put_irq again).
> > - ordering writes from the ring's CS by reading a CS register, ACTHD
> >   seems to work.
> 
> If this works reliably, you'll deserve a medal...

I've removed the HWSTAM workaround on my branch and both my ivb and snb
seem to still work. So I'm still hopeful that this actually works ;-)

Ben promised to beat on it with his machines, too, but I fear Eric is way
too busy with the finishing touches for the OGL 3.0 frenzy atm.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Eugeni Dodonov]

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

On Thu, Jan 5, 2012 at 09:13, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Jan 04, 2012 at 06:27:40PM -0800, Keith Packard wrote:
> > On Wed,  4 Jan 2012 19:40:45 +0100, Daniel Vetter <
> daniel.vetter@ffwll.ch> wrote:
> >
> > > Two things seem to do the trick on my ivb machine here:
> > > - prevent the gt from powering down while waiting for seqno
> > >   notification interrupts by grabbing the force_wake in get_irq (and
> > >   dropping it in put_irq again).
> > > - ordering writes from the ring's CS by reading a CS register, ACTHD
> > >   seems to work.
> >
> > If this works reliably, you'll deserve a medal...
>
> I've removed the HWSTAM workaround on my branch and both my ivb and snb
> seem to still work. So I'm still hopeful that this actually works ;-)
>

Yep, it seems to work even without HWSTAM, I tried this too with base on
Ben's HWSTAM disabling patches.

So I think we have a winner :).

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1454 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: rip out the HWSTAM missed irq workaround

[Daniel Vetter]

With the new ducttape of much finer quality, this seems to be no
longer necessary.

Tested on my ivb and snb machine with the usual suspects of testcases.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b9ba180..af54153 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1779,17 +1779,6 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 		INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
 
 	I915_WRITE(HWSTAM, 0xeffe);
-	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);
-	}
 
 	/* XXX hotplug from PCH */
 
-- 
1.7.7.3

Re: [PATCH] drm/i915: rip out the HWSTAM missed irq workaround

[Ben Widawsky]

On Thu, Jan 05, 2012 at 11:11:53PM +0100, Daniel Vetter wrote:
> With the new ducttape of much finer quality, this seems to be no
> longer necessary.
> 
> Tested on my ivb and snb machine with the usual suspects of testcases.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |   11 -----------
>  1 files changed, 0 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b9ba180..af54153 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1779,17 +1779,6 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
>  		INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
>  
>  	I915_WRITE(HWSTAM, 0xeffe);
> -	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);
> -	}
>  
>  	/* XXX hotplug from PCH */
>  


I'd prefer just reverting the old commits instead of this, but that's
just me.

Reviewed-and-Tested-by: Ben Widawsky <ben@bwidawsk.net>

Re: [PATCH] drm/i915: rip out the HWSTAM missed irq workaround

[Eugeni Dodonov]

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

On Thu, Jan 5, 2012 at 20:11, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> With the new ducttape of much finer quality, this seems to be no
> longer necessary.
>
> Tested on my ivb and snb machine with the usual suspects of testcases.
>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>

Nothing like high-quality ducttape!

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Tested-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

I wonder if we could drop the definitions of GEN6_*_HWSTAM from i915_reg.h
as well now...

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1063 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: rip out the HWSTAM missed irq workaround

[Keith Packard]

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

On Thu,  5 Jan 2012 23:11:53 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> With the new ducttape of much finer quality, this seems to be no
> longer necessary.
> 
> Tested on my ivb and snb machine with the usual suspects of testcases.

Eric suggested that unless we have evidence that the new work-around
fixes bugs on SNB, that we should continue to use the HWSTAM work-around
on that chip and use the RC6 voodoo on IVB, thus not potentially causing
regressions on SNB.

For back-porting to older kernels, that's obviously the right plan. I
think it's also the right plan for newer kernels, but I'd love to hear
alternate views on the matter.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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: rip out the HWSTAM missed irq workaround

[Daniel Vetter]

On Mon, Jan 09, 2012 at 02:00:47PM -0800, Keith Packard wrote:
> On Thu,  5 Jan 2012 23:11:53 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > With the new ducttape of much finer quality, this seems to be no
> > longer necessary.
> > 
> > Tested on my ivb and snb machine with the usual suspects of testcases.
> 
> Eric suggested that unless we have evidence that the new work-around
> fixes bugs on SNB, that we should continue to use the HWSTAM work-around
> on that chip and use the RC6 voodoo on IVB, thus not potentially causing
> regressions on SNB.
> 
> For back-porting to older kernels, that's obviously the right plan. I
> think it's also the right plan for newer kernels, but I'd love to hear
> alternate views on the matter.

I honestly don't trust my patch, so I'd like to give it as much validation
as possible. Which means:
- Shove it into -next and beat on it there. We can ship current 3.3 with
  Eric's workaround - it's not great but at least this works.
- Enable the voodoo and revert the HWSTAM w/a also on snb - there are
  orders more snb machines in the wild than pre-production ivbs. I.e. this
  hopefully greatly increases our changes to find out whether the voodoo
  really works or if it is only pretty decent, but not perfect ducttape.
- See what happens and act accordingly (maybe reinstate the HWSTAM w/a if
  it's required). If things really work out when this hits mainline,
  backport the voodoo patch, leaving the HWSTAM in place for older
  kernels.

Yep, I'm officially paranoid about this ;-) rc6, forcewake and friends
have simply blown up too often in unpredictable ways ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: rip out the HWSTAM missed irq workaround

[Keith Packard]

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

On Tue, 10 Jan 2012 00:39:52 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:

> I honestly don't trust my patch, so I'd like to give it as much validation
> as possible. Which means:
> - Shove it into -next and beat on it there. We can ship current 3.3 with
>   Eric's workaround - it's not great but at least this works.

Actually, sticking your patch in as a fix for 3.3 before RC1 means we'll
get loads more testing as more people test the RCs than will ever touch
drm-intel-next. Dave Airlie might have an opinion on whether that's
reasonable at this stage or not.

> - Enable the voodoo and revert the HWSTAM w/a also on snb - there are
>   orders more snb machines in the wild than pre-production ivbs. I.e. this
>   hopefully greatly increases our changes to find out whether the voodoo
>   really works or if it is only pretty decent, but not perfect ducttape.

I suspect that the hardware is different enough between IVB and SNB that
SNB testing won't tell us all that much though. And, we have a working
SNB driver right now, with the HWSTAM work-around in place. I'd be
perfectly happy to use HWSTAM on SNB forever and use the forcewake
voodoo only on IVB.

Yeah, having the voodoo run on SNB would get a lot more testing, but
it's not going to increase our confidence on how well it works on IVB,
which is the only place it is actually needed.

> - See what happens and act accordingly (maybe reinstate the HWSTAM w/a if
>   it's required). If things really work out when this hits mainline,
>   backport the voodoo patch, leaving the HWSTAM in place for older
>   kernels.

So, the "nice" thing about the two IVB work-arounds is that they can
co-exist in the kernel perfectly happily. We know that your voodoo
serves to keep the chip awake for a tiny interval after it has finished
drawing (essentially the time from the end of work to the interrupt ack
and forcewake disable), so it's not a significant additional power
drain.

We could leave the code for spinning in place and simply control that
with a module parameter. That would allow us to disable it now, and if
we find problems (or are particularly paranoid) we could disable it
before 3.3 ships with a 1-line patch.

> Yep, I'm officially paranoid about this ;-) rc6, forcewake and friends
> have simply blown up too often in unpredictable ways ...

We love our fancy hardware. The power savings brought about by rc6 are
impressive though; I only wish it didn't take so much software
support...

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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: rip out the HWSTAM missed irq workaround

[Daniel Vetter]

On Mon, Jan 09, 2012 at 06:09:13PM -0800, Keith Packard wrote:
> We could leave the code for spinning in place and simply control that
> with a module parameter. That would allow us to disable it now, and if
> we find problems (or are particularly paranoid) we could disable it
> before 3.3 ships with a 1-line patch.

Well, if you want to push it through fixes (I don't think it's too late
yet for such games) I agree that we want to keep around the spinning
workaround with a module option.

As Chris pointed out when submitting his rfc patch, we've now scored
missed irq issues on ivb, snb and some gen3 chips. So it looks like this
would be generally useful infrastructure. But in the case that we keep it
I have some pending review-comments on what's currently in ... ;-)

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

Re: [PATCH] drm/i915: rip out the HWSTAM missed irq workaround

[Ben Widawsky]

On Thu, Jan 05, 2012 at 11:11:53PM +0100, Daniel Vetter wrote:
> With the new ducttape of much finer quality, this seems to be no
> longer necessary.
> 
> Tested on my ivb and snb machine with the usual suspects of testcases.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |   11 -----------
>  1 files changed, 0 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b9ba180..af54153 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1779,17 +1779,6 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
>  		INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
>  
>  	I915_WRITE(HWSTAM, 0xeffe);
> -	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);
> -	}
>  
>  	/* XXX hotplug from PCH */
>  
---
This patch is a requirement for functional simulation on Gen7. It
appears to not break Gen6 afaics. Therefore:

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

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Daniel Vetter]

On Wed, Jan 04, 2012 at 07:40:45PM +0100, Daniel Vetter wrote:
> Two things seem to do the trick on my ivb machine here:
> - prevent the gt from powering down while waiting for seqno
>   notification interrupts by grabbing the force_wake in get_irq (and
>   dropping it in put_irq again).
> - ordering writes from the ring's CS by reading a CS register, ACTHD
>   seems to work.
> 
> Only the blt&bsd ring on ivb seem to be massively affected by this,
> but for paranoia do this dance also on the render ring and on snb
> (i.e. all gpus with forcewake).
> 
> Tested with Eric's glCopyPixels loop which without this patch scores a
> missed irq every few seconds.
> 
> This patch needs my forcewake rework to use a spinlock instead of
> dev->struct_mutex.
> 
> v2: Improve the comment per Eugeni Dodonov's suggestion.
> 
> Cc: stable@kernel.org
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> Cc: Eugeni Dodonov <eugeni.dodonov@intel.com>
> Tested-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>From the internal doc "SNB GT PM Programming Guide", Section 4.3.1:

"GT does not generate interrupts while in RC6 (by design)"

It talks about the PM interrupt but I think this might also apply to
interrupts in general. So I think we should apply the voodoo patch also to
snb.

My current working theory is hw engineers changed the way hwstam writes
are generated wrt to the read/write/irq pipeline on ivb and we've only
been lucky that it syncs out everything on snb before the gpu goes into
deep sleep states.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Eric Anholt]

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

On Tue, 10 Jan 2012 13:20:06 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jan 04, 2012 at 07:40:45PM +0100, Daniel Vetter wrote:
> > Two things seem to do the trick on my ivb machine here:
> > - prevent the gt from powering down while waiting for seqno
> >   notification interrupts by grabbing the force_wake in get_irq (and
> >   dropping it in put_irq again).
> > - ordering writes from the ring's CS by reading a CS register, ACTHD
> >   seems to work.
> > 
> > Only the blt&bsd ring on ivb seem to be massively affected by this,
> > but for paranoia do this dance also on the render ring and on snb
> > (i.e. all gpus with forcewake).
> > 
> > Tested with Eric's glCopyPixels loop which without this patch scores a
> > missed irq every few seconds.
> > 
> > This patch needs my forcewake rework to use a spinlock instead of
> > dev->struct_mutex.
> > 
> > v2: Improve the comment per Eugeni Dodonov's suggestion.
> > 
> > Cc: stable@kernel.org
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: Kenneth Graunke <kenneth@whitecape.org>
> > Cc: Eugeni Dodonov <eugeni.dodonov@intel.com>
> > Tested-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> > Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> From the internal doc "SNB GT PM Programming Guide", Section 4.3.1:
> 
> "GT does not generate interrupts while in RC6 (by design)"

So they've gone out of their way to build broken stuff.  Awesome.

I'd say you've found the clue here -- I'm a lot happier with going with
your patches now (and I was pretty happy with the gen7 side before).
I'd just like to not mess with gen6 unless we've got missed irq bugs
there to fix.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 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: paper over missed irq issues with force wake vodoo

[Keith Packard]

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

On Tue, 10 Jan 2012 16:51:08 -0800, Eric Anholt <eric@anholt.net> wrote:

> So they've gone out of their way to build broken stuff.  Awesome.

Well, in theory, the interrupt would be generated *before* the hardware
goes to RC6; when idle, I'm not exactly sure what the hardware would be
doing to generate interrupts.

> I'd say you've found the clue here -- I'm a lot happier with going with
> your patches now (and I was pretty happy with the gen7 side before).
> I'd just like to not mess with gen6 unless we've got missed irq bugs
> there to fix.

Yeah, knowing that there might be interrupt funnies due to RC6 goes some
way to explaining why just avoiding RC6 helps.

I wonder if any of this might explain the RC6 issues we see on SNB on
some hardware, and whether we should give this a try... I know, grasping
at straws, but still, it's about all we have at this point.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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: paper over missed irq issues with force wake vodoo

[Ben Widawsky]

On 01/10/2012 08:44 PM, Keith Packard wrote:
> On Tue, 10 Jan 2012 16:51:08 -0800, Eric Anholt <eric@anholt.net> wrote:
> 
>> So they've gone out of their way to build broken stuff.  Awesome.
> 
> Well, in theory, the interrupt would be generated *before* the hardware
> goes to RC6; when idle, I'm not exactly sure what the hardware would be
> doing to generate interrupts.

That's right. I think we need to think about the programming sequence a
bit more. The GT should be smart enough to not sleep if it has any work
pending that may generate interrupts. I don't think this by itself
explans anything.

> 
>> I'd say you've found the clue here -- I'm a lot happier with going with
>> your patches now (and I was pretty happy with the gen7 side before).
>> I'd just like to not mess with gen6 unless we've got missed irq bugs
>> there to fix.
> 
> Yeah, knowing that there might be interrupt funnies due to RC6 goes some
> way to explaining why just avoiding RC6 helps.
> 
> I wonder if any of this might explain the RC6 issues we see on SNB on
> some hardware, and whether we should give this a try... I know, grasping
> at straws, but still, it's about all we have at this point.
> 
Just as a reminder, never going into rc6 never fixed any problems, so I
still believe this is somehow timing related.

~Ben the pessimist

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Daniel Vetter]

On Tue, Jan 10, 2012 at 08:44:20PM -0800, Keith Packard wrote:
> On Tue, 10 Jan 2012 16:51:08 -0800, Eric Anholt <eric@anholt.net> wrote:
> 
> > So they've gone out of their way to build broken stuff.  Awesome.
> 
> Well, in theory, the interrupt would be generated *before* the hardware
> goes to RC6; when idle, I'm not exactly sure what the hardware would be
> doing to generate interrupts.

I agree, we still have no explanation for why the hw seems to forget to
push out the irq/seqno write before going into sleep (and then seems to
drop the irq right on the floor).

I suspect that the windows driver just grabs frocewake every time it's
interested in an interrupt and hence this irq/seqno signalling part
without forcewake wasn't ever properly validated. After all there must be
a reason for the multi-threaded forcewake stuff on windows - on Linux
(before the voodoo patch at least) we don't use forcewake at all in any
fastpath ...

Unfortunately round-trip times with vpg are a disaster and we're not
allowed to look at the windows kernel driver code to check ourselves :(
I need to again work on getting some access to it.

> > I'd say you've found the clue here -- I'm a lot happier with going with
> > your patches now (and I was pretty happy with the gen7 side before).
> > I'd just like to not mess with gen6 unless we've got missed irq bugs
> > there to fix.
> 
> Yeah, knowing that there might be interrupt funnies due to RC6 goes some
> way to explaining why just avoiding RC6 helps.
> 
> I wonder if any of this might explain the RC6 issues we see on SNB on
> some hardware, and whether we should give this a try... I know, grasping
> at straws, but still, it's about all we have at this point.

I'm already playing around with patches that grab forcewake a bit more to
work out a magic trick for semaphores vs. vt-d. Utter fail atm, everything
I try seems to blow up faster :( So we still have an elephant somewhere.

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

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Kenneth Graunke]

On 01/10/2012 04:20 AM, Daniel Vetter wrote:
> On Wed, Jan 04, 2012 at 07:40:45PM +0100, Daniel Vetter wrote:
>> Two things seem to do the trick on my ivb machine here:
>> - prevent the gt from powering down while waiting for seqno
>>    notification interrupts by grabbing the force_wake in get_irq (and
>>    dropping it in put_irq again).
>> - ordering writes from the ring's CS by reading a CS register, ACTHD
>>    seems to work.
>>
>> Only the blt&bsd ring on ivb seem to be massively affected by this,
>> but for paranoia do this dance also on the render ring and on snb
>> (i.e. all gpus with forcewake).
>>
>> Tested with Eric's glCopyPixels loop which without this patch scores a
>> missed irq every few seconds.
>>
>> This patch needs my forcewake rework to use a spinlock instead of
>> dev->struct_mutex.
>>
>> v2: Improve the comment per Eugeni Dodonov's suggestion.
>>
>> Cc: stable@kernel.org
>> Cc: Eric Anholt<eric@anholt.net>
>> Cc: Kenneth Graunke<kenneth@whitecape.org>
>> Cc: Eugeni Dodonov<eugeni.dodonov@intel.com>
>> Tested-by: Eugeni Dodonov<eugeni.dodonov@intel.com>
>> Reviewed-by: Eugeni Dodonov<eugeni.dodonov@intel.com>
>> Signed-Off-by: Daniel Vetter<daniel.vetter@ffwll.ch>
>
>  From the internal doc "SNB GT PM Programming Guide", Section 4.3.1:
>
> "GT does not generate interrupts while in RC6 (by design)"
>
> It talks about the PM interrupt but I think this might also apply to
> interrupts in general. So I think we should apply the voodoo patch also to
> snb.
>
> My current working theory is hw engineers changed the way hwstam writes
> are generated wrt to the read/write/irq pipeline on ivb and we've only
> been lucky that it syncs out everything on snb before the gpu goes into
> deep sleep states.
> -Daniel

Daniel,

This is a good find!  Your patch looked good to me in the first place, 
but with this extra bit of information, I believe it all the more.  It's 
simple and it makes a lot of sense.

FWIW,
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

I'm in favor of applying this for Gen7, and I'd be fine with applying it 
on Gen6 too.  At the very least we should experiment with it on 
Gen6---as Keith said, it might help with some of the mysterious issues 
we've seen.  Worth a try at least.

Thanks for your great work here!

--Kenneth

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Keith Packard]

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

On Wed,  4 Jan 2012 19:40:45 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Two things seem to do the trick on my ivb machine here:
> - prevent the gt from powering down while waiting for seqno
>   notification interrupts by grabbing the force_wake in get_irq (and
>   dropping it in put_irq again).
> - ordering writes from the ring's CS by reading a CS register, ACTHD
>   seems to work.

I've rebased this code on top of drm-intel-fixes and pushed it to my
forcewake-spinlock branch. I'd like to get this pushed to
drm-intel-fixes in the next couple of days, get some QA coverage and
then get it merged to master so that we can get some community testing
early in the 3.3 cycle.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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: paper over missed irq issues with force wake vodoo

[Daniel Vetter]

On Fri, Jan 13, 2012 at 08:42:00AM -0800, Keith Packard wrote:
> On Wed,  4 Jan 2012 19:40:45 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > Two things seem to do the trick on my ivb machine here:
> > - prevent the gt from powering down while waiting for seqno
> >   notification interrupts by grabbing the force_wake in get_irq (and
> >   dropping it in put_irq again).
> > - ordering writes from the ring's CS by reading a CS register, ACTHD
> >   seems to work.
> 
> I've rebased this code on top of drm-intel-fixes and pushed it to my
> forcewake-spinlock branch. I'd like to get this pushed to
> drm-intel-fixes in the next couple of days, get some QA coverage and
> then get it merged to master so that we can get some community testing
> early in the 3.3 cycle.

A few quick comments:
- I think we need to amend the commit msg of the voodoo patch with the
  piece of doc I've discovered. If you want I can send out a v3 with that.
- I think the HWSTAM revert is material for -next
- Can you post your 3 patches on intel-gfx, I think I can shoot at them a
  bit ;-)

acc101d drm/i915: Hold gt_lock across forcewake register reads

Imo this is a simple cleanup (reading forcewake-protected registers isn't
really a fast-path for us), so material for -next.

0f0e134 drm/i915: Hold gt_lock during reset

I still don't see what race you're trying to protect here, after all the
gpu just died, things are confusing anyway (and anyone accessing the gpu
in such a state should take that into account). Currently that's no one
afaics. So imo at most -next material.

176b987 drm/i915: Move reset forcewake processing to gen6_do_reset

Again this is imo just a cleanup. Furthermore the commit msg is lying a
bit because it fails to mention the fix to use the forcewake function
pointer. So the cleanup is imo for -next and the bugfix is really old,
see:


> 
> -- 
> keith.packard@intel.com



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

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Daniel Vetter]

On Sat, Jan 14, 2012 at 12:52:31AM +0100, Daniel Vetter wrote:
> On Fri, Jan 13, 2012 at 08:42:00AM -0800, Keith Packard wrote:
> > On Wed,  4 Jan 2012 19:40:45 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > 
> > > Two things seem to do the trick on my ivb machine here:
> > > - prevent the gt from powering down while waiting for seqno
> > >   notification interrupts by grabbing the force_wake in get_irq (and
> > >   dropping it in put_irq again).
> > > - ordering writes from the ring's CS by reading a CS register, ACTHD
> > >   seems to work.
> > 
> > I've rebased this code on top of drm-intel-fixes and pushed it to my
> > forcewake-spinlock branch. I'd like to get this pushed to
> > drm-intel-fixes in the next couple of days, get some QA coverage and
> > then get it merged to master so that we can get some community testing
> > early in the 3.3 cycle.
> 
> A few quick comments:
> - I think we need to amend the commit msg of the voodoo patch with the
>   piece of doc I've discovered. If you want I can send out a v3 with that.
> - I think the HWSTAM revert is material for -next
> - Can you post your 3 patches on intel-gfx, I think I can shoot at them a
>   bit ;-)
> 
> acc101d drm/i915: Hold gt_lock across forcewake register reads
> 
> Imo this is a simple cleanup (reading forcewake-protected registers isn't
> really a fast-path for us), so material for -next.
> 
> 0f0e134 drm/i915: Hold gt_lock during reset
> 
> I still don't see what race you're trying to protect here, after all the
> gpu just died, things are confusing anyway (and anyone accessing the gpu
> in such a state should take that into account). Currently that's no one
> afaics. So imo at most -next material.
> 
> 176b987 drm/i915: Move reset forcewake processing to gen6_do_reset
> 
> Again this is imo just a cleanup. Furthermore the commit msg is lying a
> bit because it fails to mention the fix to use the forcewake function
> pointer. So the cleanup is imo for -next and the bugfix is really old,
> see:

Meh, mutt mail fail, I've hit the wrong key when I've tried to look up the
reference for that patch mail. Here we go:

http://lists.freedesktop.org/archives/intel-gfx/2011-November/013668.html

And the resend:

http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg07229.html

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

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Keith Packard]

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

On Sat, 14 Jan 2012 00:52:31 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:

> - I think we need to amend the commit msg of the voodoo patch with the
>   piece of doc I've discovered. If you want I can send out a v3 with that.

Sounds good.

> - I think the HWSTAM revert is material for -next

I'd be OK with pushing all of the SNB changes to -next and leaving the
patches only affecting IVB, where we know it is necessary.

Note that this branch wasn't written with an eye to merging to -fixes,
but rather as what we want the code to look like eventually. As such, we
need to identify separately

 1) What needs to go into -fixes for 3.3.

 2) What needs to go into -next for 3.4.

> - Can you post your 3 patches on intel-gfx, I think I can shoot at them a
>   bit ;-)

Sure; it's all a work in progress.

> acc101d drm/i915: Hold gt_lock across forcewake register reads
> 
> Imo this is a simple cleanup (reading forcewake-protected registers isn't
> really a fast-path for us), so material for -next.

The 'optimization' is just a side benefit. The fix is to prevent reads
From happening without forcewake being set.

> 0f0e134 drm/i915: Hold gt_lock during reset
> 
> I still don't see what race you're trying to protect here, after all the
> gpu just died, things are confusing anyway (and anyone accessing the gpu
> in such a state should take that into account). Currently that's no one
> afaics. So imo at most -next material.

These two patches work together to ensure that no-one reads from the GPU
without forcewake being set correctly, even across reset. Mostly, I
changed the code to make it obvious that the whole read operation was
now atomic; before, I had to read a lot of code to convince myself that
the read couldn't happen without forcewake being set, except under the
reset condition. Forcing me to read code closely to prove it correct
doesn't make me happy. Having a spinlock held over the entire
section makes the whole thing obviously correct to even casual inspection.

So, I'd like these to go into -next so that when I read this code next
year, I won't have to figure all of this out again.

> 176b987 drm/i915: Move reset forcewake processing to gen6_do_reset
> 
> Again this is imo just a cleanup. Furthermore the commit msg is lying a
> bit because it fails to mention the fix to use the forcewake function
> pointer. So the cleanup is imo for -next and the bugfix is really old,
> see:

Yes, I didn't even notice that I'd fixed that bug when I moved the code...

The bugfix is, however required, and needs to be in -fixes.

So, I think for -fixes we get:

 1) The forcewake spinlock patch.

 2) The bugfix to the reset path.

 3) forcewake while waiting for interrupts for IVB only, not for SNB,
    with updated commit message.
 
This minimizes the potential for regressions in SNB (by not affecting it
at all) while fixing the IVB issues.

For -next, we should have

 1) forcewake for interrupts on IVB and SNB

 2) Removal of the HWSTAM hacks

 3) The spinlock cleanups that make me happy

I'd love to hear back from some SNB owners that the forcewake IRQ issue
resolves problems and doesn't cause any regressions. If so, we can
reconsider it for 3.3. If it doesn't fix anything, then I don't think it
should go into 3.3.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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: paper over missed irq issues with force wake vodoo

[Daniel Vetter]

On Fri, Jan 13, 2012 at 04:11:43PM -0800, Keith Packard wrote:
> On Sat, 14 Jan 2012 00:52:31 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > acc101d drm/i915: Hold gt_lock across forcewake register reads
> > 
> > Imo this is a simple cleanup (reading forcewake-protected registers isn't
> > really a fast-path for us), so material for -next.
> 
> The 'optimization' is just a side benefit. The fix is to prevent reads
> From happening without forcewake being set.

I still fail to see how you can sneak a read in there without forcewake
being asserted. And assuming I haven't understood you the last time around
we've discussed this, you've agreed.
> 
> > 0f0e134 drm/i915: Hold gt_lock during reset
> > 
> > I still don't see what race you're trying to protect here, after all the
> > gpu just died, things are confusing anyway (and anyone accessing the gpu
> > in such a state should take that into account). Currently that's no one
> > afaics. So imo at most -next material.
> 
> These two patches work together to ensure that no-one reads from the GPU
> without forcewake being set correctly, even across reset. Mostly, I
> changed the code to make it obvious that the whole read operation was
> now atomic; before, I had to read a lot of code to convince myself that
> the read couldn't happen without forcewake being set, except under the
> reset condition. Forcing me to read code closely to prove it correct
> doesn't make me happy. Having a spinlock held over the entire
> section makes the whole thing obviously correct to even casual inspection.
> 
> So, I'd like these to go into -next so that when I read this code next
> year, I won't have to figure all of this out again.

Agreed, it's not the most obvious code around ;-)
 
> > 176b987 drm/i915: Move reset forcewake processing to gen6_do_reset
> > 
> > Again this is imo just a cleanup. Furthermore the commit msg is lying a
> > bit because it fails to mention the fix to use the forcewake function
> > pointer. So the cleanup is imo for -next and the bugfix is really old,
> > see:
> 
> Yes, I didn't even notice that I'd fixed that bug when I moved the code...
> 
> The bugfix is, however required, and needs to be in -fixes.

Yeah, I've silently implied that.

> So, I think for -fixes we get:
> 
>  1) The forcewake spinlock patch.
> 
>  2) The bugfix to the reset path.
> 
>  3) forcewake while waiting for interrupts for IVB only, not for SNB,
>     with updated commit message.
>  
> This minimizes the potential for regressions in SNB (by not affecting it
> at all) while fixing the IVB issues.
> 
> For -next, we should have
> 
>  1) forcewake for interrupts on IVB and SNB
> 
>  2) Removal of the HWSTAM hacks
> 
>  3) The spinlock cleanups that make me happy
> 
> I'd love to hear back from some SNB owners that the forcewake IRQ issue
> resolves problems and doesn't cause any regressions. If so, we can
> reconsider it for 3.3. If it doesn't fix anything, then I don't think it
> should go into 3.3.

Sounds like a good merge plan. I'll wrestle the patch to add the IS_IVB
check and the documentation reference.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Keith Packard]

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

On Sat, 14 Jan 2012 01:31:40 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Jan 13, 2012 at 04:11:43PM -0800, Keith Packard wrote:
> > On Sat, 14 Jan 2012 00:52:31 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > acc101d drm/i915: Hold gt_lock across forcewake register reads
> > > 
> > > Imo this is a simple cleanup (reading forcewake-protected registers isn't
> > > really a fast-path for us), so material for -next.
> > 
> > The 'optimization' is just a side benefit. The fix is to prevent reads
> > From happening without forcewake being set.
> 
> I still fail to see how you can sneak a read in there without forcewake
> being asserted. And assuming I haven't understood you the last time around
> we've discussed this, you've agreed.

Yes, except during reset, where forcewake is cleared even if the lock
count is non-zero. Any reads happening while reset is going on will
return garbage. None of this is 'required' given the structure of the
code today, it just makes it all evident without having to go through
yet another long sequence of explanations.

I had patches to hold the spinlock across register writes too, as using
different locking for reading and writing seems like a bad plan, but I
didn't put those in because writes involve spinning to wait for the fifo
to drain, and that seemed like a bad thing to do while holding the
spinlock.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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: paper over missed irq issues with force wake vodoo

[Daniel Vetter]

On Fri, Jan 13, 2012 at 04:50:39PM -0800, Keith Packard wrote:
> On Sat, 14 Jan 2012 01:31:40 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Jan 13, 2012 at 04:11:43PM -0800, Keith Packard wrote:
> > > On Sat, 14 Jan 2012 00:52:31 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > acc101d drm/i915: Hold gt_lock across forcewake register reads
> > > > 
> > > > Imo this is a simple cleanup (reading forcewake-protected registers isn't
> > > > really a fast-path for us), so material for -next.
> > > 
> > > The 'optimization' is just a side benefit. The fix is to prevent reads
> > > From happening without forcewake being set.
> > 
> > I still fail to see how you can sneak a read in there without forcewake
> > being asserted. And assuming I haven't understood you the last time around
> > we've discussed this, you've agreed.
> 
> Yes, except during reset, where forcewake is cleared even if the lock
> count is non-zero. Any reads happening while reset is going on will
> return garbage. None of this is 'required' given the structure of the
> code today, it just makes it all evident without having to go through
> yet another long sequence of explanations.

I think that race is air-tight with your patch to rework the reset code
already. But better safe than sorry. And as I've said a good cleanup
anyway.

> I had patches to hold the spinlock across register writes too, as using
> different locking for reading and writing seems like a bad plan, but I
> didn't put those in because writes involve spinning to wait for the fifo
> to drain, and that seemed like a bad thing to do while holding the
> spinlock.

One of the reasons Chris originally shot down Ben's forcewake patches
which protected everything with a spinlock (i.e. also writes) is the
overhead. And writes to advance the ring are actually rather common. Iirc
Chris even wrote a patch to cut down on the overhead by caching the fifo
count. So I think we actually want this asymmetry in locking for
performance reasons.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Keith Packard]

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

On Sat, 14 Jan 2012 13:12:07 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:

> I think that race is air-tight with your patch to rework the reset code
> already. But better safe than sorry. And as I've said a good cleanup
> anyway.

Sounds good. I like clearer code, especially when it doesn't cost
performance. I think I'd like my version in -fixes so that we don't
change anything with -next; no reason to have two slightly different
versions out there in case one (or the other) is broken?

> One of the reasons Chris originally shot down Ben's forcewake patches
> which protected everything with a spinlock (i.e. also writes) is the
> overhead. And writes to advance the ring are actually rather common. Iirc
> Chris even wrote a patch to cut down on the overhead by caching the fifo
> count. So I think we actually want this asymmetry in locking for
> performance reasons.

Ah, that's a good reason to use different locking for each path
then. Suitable documentation, and a WARN_ON in the write path to check
for the struct_mutex should suffice to prevent mistakes in the future.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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: paper over missed irq issues with force wake vodoo

[Daniel Vetter]

On Sun, Jan 15, 2012 at 07:35, Keith Packard <keithp@keithp.com> wrote:
> On Sat, 14 Jan 2012 13:12:07 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> I think that race is air-tight with your patch to rework the reset code
>> already. But better safe than sorry. And as I've said a good cleanup
>> anyway.
>
> Sounds good. I like clearer code, especially when it doesn't cost
> performance. I think I'd like my version in -fixes so that we don't
> change anything with -next; no reason to have two slightly different
> versions out there in case one (or the other) is broken?

Having the one-liner fix as separate patch makes backporting to 3.2
simpler. But I don't care much, so if you amend your commit to mention
the small fix hidden in the cleanup and add a note that the fix needs
to go to 3.2-stable that's fine by me, too.
-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Keith Packard]

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

On Sun, 15 Jan 2012 16:03:28 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:

> Having the one-liner fix as separate patch makes backporting to 3.2
> simpler. But I don't care much, so if you amend your commit to mention
> the small fix hidden in the cleanup and add a note that the fix needs
> to go to 3.2-stable that's fine by me, too.

I'll split the one-liner out for back-port ease, although we'll probably
suggest back-porting the whole mess for anyone running IVB on 3.2.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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: paper over missed irq issues with force wake vodoo

[Ben Widawsky]

On Wed, Jan 04, 2012 at 05:52:10PM +0100, Daniel Vetter wrote:
> Two things seem to do the trick on my ivb machine here:
> - prevent the gt from powering down while waiting for seqno
>   notification interrupts by grabbing the force_wake in get_irq (and
>   dropping it in put_irq again).
> - ordering writes from the ring's CS by reading a CS register, ACTHD
>   seems to work.
> 
> Only the blt&bsd ring on ivb seem to be massively affected by this,
> but for paranoia do this dance also on the render ring and on snb
> (i.e. all gpus with forcewake).
> 
> Tested with Eric's glCopyPixels loop which without this patch scores a
> missed irq every few seconds.
> 
> This patch needs my forcewake rework to use a spinlock instead of
> dev->struct_mutex.
> 
> Cc: stable@kernel.org
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> Cc: Eugeni Dodonov <eugeni.dodonov@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-and-Tested-by: Ben Widawsky <ben@bwidawsk.net>


Please please please make it clear that we still don't understand exactly why
this works.

Ben

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Daniel Vetter]

On Fri, Jan 6, 2012 at 00:29, Ben Widawsky <ben@bwidawsk.net> wrote:
> Please please please make it clear that we still don't understand exactly why
> this works.

I've hoped that the words "voodoo" and "magic" in the headline of the
commit msg make that clear ;-)
-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Keith Packard]

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

On Thu, 5 Jan 2012 23:29:44 +0000, Ben Widawsky <ben@bwidawsk.net> wrote:

> Please please please make it clear that we still don't understand exactly why
> this works.

If the word 'voodoo' isn't sufficient, I don't know what is...

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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: paper over missed irq issues with force wake vodoo

[Keith Packard]

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

On Wed,  4 Jan 2012 17:52:10 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Two things seem to do the trick on my ivb machine here:
> - prevent the gt from powering down while waiting for seqno
>   notification interrupts by grabbing the force_wake in get_irq (and
>   dropping it in put_irq again).
> - ordering writes from the ring's CS by reading a CS register, ACTHD
>   seems to work.
> 
> Only the blt&bsd ring on ivb seem to be massively affected by this,
> but for paranoia do this dance also on the render ring and on snb
> (i.e. all gpus with forcewake).
> 
> Tested with Eric's glCopyPixels loop which without this patch scores a
> missed irq every few seconds.
> 
> This patch needs my forcewake rework to use a spinlock instead of
> dev->struct_mutex.

I've pushed these to the forcewake-spinlock branch in both my fdo and
kernel repositories. Please give that a try on IVB and SNB to make sure
things are working right.

I'd like to get these into master as well as being sent off to stable@
for 3.2.x

-- 
keith.packard@intel.com

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