[PATCH] drm/i915: Click the ACTHD three times to go home

[PATCH] drm/i915: Click the ACTHD three times to go home

[Chris Wilson]

One POSTING_READ of ACTHD may not be enough to ensure that the seqno
write has been posted from the GPU and is now visible. So do three!

References: https://bugs.freedesktop.org/show_bug.cgi?id=97557
References: https://bugs.freedesktop.org/show_bug.cgi?id=100007
References: https://bugs.freedesktop.org/show_bug.cgi?id=100052
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ab5e2e0623b7..2b5cd6039a6e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -766,6 +766,7 @@ static void
 gen6_seqno_barrier(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
+	int loop;
 
 	/* Workaround to force correct ordering between irq and seqno writes on
 	 * ivb (and maybe also on snb) by reading from a CS register (like
@@ -783,7 +784,8 @@ gen6_seqno_barrier(struct intel_engine_cs *engine)
 	 * take the spinlock to guard against concurrent cacheline access.
 	 */
 	spin_lock_irq(&dev_priv->uncore.lock);
-	POSTING_READ_FW(RING_ACTHD(engine->mmio_base));
+	for (loop = 0; loop < 3; loop++)
+		POSTING_READ_FW(RING_ACTHD(engine->mmio_base));
 	spin_unlock_irq(&dev_priv->uncore.lock);
 }
 
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Click the ACTHD three times to go home

[Patchwork]

== Series Details ==

Series: drm/i915: Click the ACTHD three times to go home
URL   : https://patchwork.freedesktop.org/series/21876/
State : success

== Summary ==

Series 21876v1 drm/i915: Click the ACTHD three times to go home
https://patchwork.freedesktop.org/api/1.0/series/21876/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-flip-before-cursor-varying-size:
                dmesg-warn -> PASS       (fi-byt-n2820) fdo#100094

fdo#100094 https://bugs.freedesktop.org/show_bug.cgi?id=100094

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 467s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 460s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 581s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 541s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 548s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 509s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 504s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 436s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 437s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 435s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 517s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 514s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 488s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 483s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 596s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 485s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 524s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 453s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 548s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 429s

102397b5dbb1f68504739adefde2c28a5aba95b9 drm-tip: 2017y-03m-25d-00h-24m-58s UTC integration manifest
a8b28f5 drm/i915: Click the ACTHD three times to go home

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4301/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Click the ACTHD three times to go home

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> One POSTING_READ of ACTHD may not be enough to ensure that the seqno
> write has been posted from the GPU and is now visible. So do three!
>

Is this about posting read triggering or just adding enough delay
that the write gets through?

-Mika

> References: https://bugs.freedesktop.org/show_bug.cgi?id=97557
> References: https://bugs.freedesktop.org/show_bug.cgi?id=100007
> References: https://bugs.freedesktop.org/show_bug.cgi?id=100052
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ab5e2e0623b7..2b5cd6039a6e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -766,6 +766,7 @@ static void
>  gen6_seqno_barrier(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = engine->i915;
> +	int loop;
>  
>  	/* Workaround to force correct ordering between irq and seqno writes on
>  	 * ivb (and maybe also on snb) by reading from a CS register (like
> @@ -783,7 +784,8 @@ gen6_seqno_barrier(struct intel_engine_cs *engine)
>  	 * take the spinlock to guard against concurrent cacheline access.
>  	 */
>  	spin_lock_irq(&dev_priv->uncore.lock);
> -	POSTING_READ_FW(RING_ACTHD(engine->mmio_base));
> +	for (loop = 0; loop < 3; loop++)
> +		POSTING_READ_FW(RING_ACTHD(engine->mmio_base));
>  	spin_unlock_irq(&dev_priv->uncore.lock);
>  }
>  
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Click the ACTHD three times to go home

[Chris Wilson]

On Mon, Mar 27, 2017 at 01:12:30PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > One POSTING_READ of ACTHD may not be enough to ensure that the seqno
> > write has been posted from the GPU and is now visible. So do three!
> >
> 
> Is this about posting read triggering or just adding enough delay
> that the write gets through?

It's purely about a delay that we guess will cover 99.9999% of cases.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Click the ACTHD three times to go home

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Mon, Mar 27, 2017 at 01:12:30PM +0300, Mika Kuoppala wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > One POSTING_READ of ACTHD may not be enough to ensure that the seqno
>> > write has been posted from the GPU and is now visible. So do three!
>> >
>> 
>> Is this about posting read triggering or just adding enough delay
>> that the write gets through?
>
> It's purely about a delay that we guess will cover 99.9999% of cases.

Acked-by: Mika Kuoppala <mika.kuoppala@intel.com>

> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Click the ACTHD three times to go home

[Chris Wilson]

On Mon, Mar 27, 2017 at 04:19:58PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Mon, Mar 27, 2017 at 01:12:30PM +0300, Mika Kuoppala wrote:
> >> Chris Wilson <chris@chris-wilson.co.uk> writes:
> >> 
> >> > One POSTING_READ of ACTHD may not be enough to ensure that the seqno
> >> > write has been posted from the GPU and is now visible. So do three!
> >> >
> >> 
> >> Is this about posting read triggering or just adding enough delay
> >> that the write gets through?
> >
> > It's purely about a delay that we guess will cover 99.9999% of cases.
> 
> Acked-by: Mika Kuoppala <mika.kuoppala@intel.com>

Thanks, for checking over the idea. I'm going to apply it to
topic/core-for-CI for some extended soak testing and see if that
silences fi-snb-2600
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Click the ACTHD three times to go home

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Mon, Mar 27, 2017 at 04:19:58PM +0300, Mika Kuoppala wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > On Mon, Mar 27, 2017 at 01:12:30PM +0300, Mika Kuoppala wrote:
>> >> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> >> 
>> >> > One POSTING_READ of ACTHD may not be enough to ensure that the seqno
>> >> > write has been posted from the GPU and is now visible. So do three!
>> >> >
>> >> 
>> >> Is this about posting read triggering or just adding enough delay
>> >> that the write gets through?
>> >
>> > It's purely about a delay that we guess will cover 99.9999% of cases.
>> 
>> Acked-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> Thanks, for checking over the idea. I'm going to apply it to
> topic/core-for-CI for some extended soak testing and see if that
> silences fi-snb-2600

Another thing that would be worth testing is to force a latency guarantee
through pm_qos.

-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Click the ACTHD three times to go home

[Chris Wilson]

On Mon, Mar 27, 2017 at 05:00:49PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Mon, Mar 27, 2017 at 04:19:58PM +0300, Mika Kuoppala wrote:
> >> Chris Wilson <chris@chris-wilson.co.uk> writes:
> >> 
> >> > On Mon, Mar 27, 2017 at 01:12:30PM +0300, Mika Kuoppala wrote:
> >> >> Chris Wilson <chris@chris-wilson.co.uk> writes:
> >> >> 
> >> >> > One POSTING_READ of ACTHD may not be enough to ensure that the seqno
> >> >> > write has been posted from the GPU and is now visible. So do three!
> >> >> >
> >> >> 
> >> >> Is this about posting read triggering or just adding enough delay
> >> >> that the write gets through?
> >> >
> >> > It's purely about a delay that we guess will cover 99.9999% of cases.
> >> 
> >> Acked-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >
> > Thanks, for checking over the idea. I'm going to apply it to
> > topic/core-for-CI for some extended soak testing and see if that
> > silences fi-snb-2600
> 
> Another thing that would be worth testing is to force a latency guarantee
> through pm_qos.

iirc removing a pm_qos was synchronous, so for shortlived/frequent requests
they were a no-go (I tried holding pm_qos over the wait, which covers
all the irq_seqno_barriers). It bumped our client latency metrics quite
substantially.

Ugh. Super ugly would be to take pm_qos whilst irq is enabled, just we
keep that irq enabled all the time whilst the guc is active,
artificially forcing the cpus to never sleep whilst the GPU is busy.

On the other hand, they don't have seqno_barrier. Getting hacky.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Click the ACTHD three times to go home

[Chris Wilson]

On Mon, Mar 27, 2017 at 02:30:08PM +0100, Chris Wilson wrote:
> On Mon, Mar 27, 2017 at 04:19:58PM +0300, Mika Kuoppala wrote:
> > Chris Wilson <chris@chris-wilson.co.uk> writes:
> > 
> > > On Mon, Mar 27, 2017 at 01:12:30PM +0300, Mika Kuoppala wrote:
> > >> Chris Wilson <chris@chris-wilson.co.uk> writes:
> > >> 
> > >> > One POSTING_READ of ACTHD may not be enough to ensure that the seqno
> > >> > write has been posted from the GPU and is now visible. So do three!
> > >> >
> > >> 
> > >> Is this about posting read triggering or just adding enough delay
> > >> that the write gets through?
> > >
> > > It's purely about a delay that we guess will cover 99.9999% of cases.
> > 
> > Acked-by: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> Thanks, for checking over the idea. I'm going to apply it to
> topic/core-for-CI for some extended soak testing and see if that
> silences fi-snb-2600

Second run and the miss came back. Boo.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Click the ACTHD three times to go home

[Jani Nikula]

On Sat, 25 Mar 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> One POSTING_READ of ACTHD may not be enough to ensure that the seqno
> write has been posted from the GPU and is now visible. So do three!
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=97557
> References: https://bugs.freedesktop.org/show_bug.cgi?id=100007
> References: https://bugs.freedesktop.org/show_bug.cgi?id=100052
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ab5e2e0623b7..2b5cd6039a6e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -766,6 +766,7 @@ static void
>  gen6_seqno_barrier(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = engine->i915;
> +	int loop;
>  
>  	/* Workaround to force correct ordering between irq and seqno writes on
>  	 * ivb (and maybe also on snb) by reading from a CS register (like
> @@ -783,7 +784,8 @@ gen6_seqno_barrier(struct intel_engine_cs *engine)
>  	 * take the spinlock to guard against concurrent cacheline access.
>  	 */
>  	spin_lock_irq(&dev_priv->uncore.lock);
> -	POSTING_READ_FW(RING_ACTHD(engine->mmio_base));

This definitely needs the /* We do this three times for luck */ comment
from intel_display.c!

BR,
Jani.

> +	for (loop = 0; loop < 3; loop++)
> +		POSTING_READ_FW(RING_ACTHD(engine->mmio_base));
>  	spin_unlock_irq(&dev_priv->uncore.lock);
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx