Missing byt full-ppgtt massage

Missing byt full-ppgtt massage

[Chris Wilson]

None of you never noticed I didn't send this patch from my tree that I
found helped byt full-ppgtt. I can't believe none of you are psychic.
-Chris


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

[PATCH] drm/i915/ringbuffer: Delay after invalidating gen6+ xcs

[Chris Wilson]

During stress testing of full-ppgtt (on Baytrail at least), we found
that the invalidation around a context/mm switch was insufficient (writes
would go astray). Adding a second MI_FLUSH_DW barrier prevents this, but
it is unclear as to whether this is merely a delaying tactic or if it is
truly serialising with the TLB invalidation. Either way, it is
empirically required.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107715
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 101 +++++++++++-------------
 1 file changed, 47 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index d40f55a8dc34..952b6269bab0 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1944,40 +1944,62 @@ static void gen6_bsd_submit_request(struct i915_request *request)
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
 
-static int gen6_bsd_ring_flush(struct i915_request *rq, u32 mode)
+static int emit_mi_flush_dw(struct i915_request *rq, u32 mode, u32 invflags)
 {
 	u32 cmd, *cs;
 
-	cs = intel_ring_begin(rq, 4);
-	if (IS_ERR(cs))
-		return PTR_ERR(cs);
+	do {
+		cs = intel_ring_begin(rq, 4);
+		if (IS_ERR(cs))
+			return PTR_ERR(cs);
 
-	cmd = MI_FLUSH_DW;
+		cmd = MI_FLUSH_DW;
 
-	/* We always require a command barrier so that subsequent
-	 * commands, such as breadcrumb interrupts, are strictly ordered
-	 * wrt the contents of the write cache being flushed to memory
-	 * (and thus being coherent from the CPU).
-	 */
-	cmd |= MI_FLUSH_DW_STORE_INDEX | MI_FLUSH_DW_OP_STOREDW;
+		/*
+		 * We always require a command barrier so that subsequent
+		 * commands, such as breadcrumb interrupts, are strictly ordered
+		 * wrt the contents of the write cache being flushed to memory
+		 * (and thus being coherent from the CPU).
+		 */
+		cmd |= MI_FLUSH_DW_STORE_INDEX | MI_FLUSH_DW_OP_STOREDW;
 
-	/*
-	 * Bspec vol 1c.5 - video engine command streamer:
-	 * "If ENABLED, all TLBs will be invalidated once the flush
-	 * operation is complete. This bit is only valid when the
-	 * Post-Sync Operation field is a value of 1h or 3h."
-	 */
-	if (mode & EMIT_INVALIDATE)
-		cmd |= MI_INVALIDATE_TLB | MI_INVALIDATE_BSD;
+		/*
+		 * Bspec vol 1c.3 - blitter engine command streamer:
+		 * "If ENABLED, all TLBs will be invalidated once the flush
+		 * operation is complete. This bit is only valid when the
+		 * Post-Sync Operation field is a value of 1h or 3h."
+		 */
+		if (mode & EMIT_INVALIDATE)
+			cmd |= invflags;
+		*cs++ = cmd;
+		*cs++ = I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT;
+		*cs++ = 0;
+		*cs++ = MI_NOOP;
+		intel_ring_advance(rq, cs);
+
+		/*
+		 * Not only do we need a full barrier (post-sync write) after
+		 * invalidating the TLBs, but we need to wait a little bit
+		 * longer. Whether this is merely delaying us, or the
+		 * subsequent flush is a key part of serialising with the
+		 * post-sync op, this extra pass appears vital before a
+		 * mm switch!
+		 */
+		if (!(mode & EMIT_INVALIDATE))
+			break;
+
+		mode &= ~EMIT_INVALIDATE;
+	} while (1);
 
-	*cs++ = cmd;
-	*cs++ = I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT;
-	*cs++ = 0;
-	*cs++ = MI_NOOP;
-	intel_ring_advance(rq, cs);
 	return 0;
 }
 
+static int gen6_bsd_ring_flush(struct i915_request *rq, u32 mode)
+{
+	return emit_mi_flush_dw(rq, mode,
+				MI_INVALIDATE_TLB | MI_INVALIDATE_BSD);
+}
+
 static int
 hsw_emit_bb_start(struct i915_request *rq,
 		  u64 offset, u32 len,
@@ -2022,36 +2044,7 @@ gen6_emit_bb_start(struct i915_request *rq,
 
 static int gen6_ring_flush(struct i915_request *rq, u32 mode)
 {
-	u32 cmd, *cs;
-
-	cs = intel_ring_begin(rq, 4);
-	if (IS_ERR(cs))
-		return PTR_ERR(cs);
-
-	cmd = MI_FLUSH_DW;
-
-	/* We always require a command barrier so that subsequent
-	 * commands, such as breadcrumb interrupts, are strictly ordered
-	 * wrt the contents of the write cache being flushed to memory
-	 * (and thus being coherent from the CPU).
-	 */
-	cmd |= MI_FLUSH_DW_STORE_INDEX | MI_FLUSH_DW_OP_STOREDW;
-
-	/*
-	 * Bspec vol 1c.3 - blitter engine command streamer:
-	 * "If ENABLED, all TLBs will be invalidated once the flush
-	 * operation is complete. This bit is only valid when the
-	 * Post-Sync Operation field is a value of 1h or 3h."
-	 */
-	if (mode & EMIT_INVALIDATE)
-		cmd |= MI_INVALIDATE_TLB;
-	*cs++ = cmd;
-	*cs++ = I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT;
-	*cs++ = 0;
-	*cs++ = MI_NOOP;
-	intel_ring_advance(rq, cs);
-
-	return 0;
+	return emit_mi_flush_dw(rq, mode, MI_INVALIDATE_TLB);
 }
 
 static void intel_ring_init_semaphores(struct drm_i915_private *dev_priv,
-- 
2.18.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/ringbuffer: Delay after invalidating gen6+ xcs

[Patchwork]

== Series Details ==

Series: drm/i915/ringbuffer: Delay after invalidating gen6+ xcs
URL   : https://patchwork.freedesktop.org/series/48813/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4718 -> Patchwork_10035 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48813/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10035 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    {igt@amdgpu/amd_basic@userptr}:
      {fi-kbl-8809g}:     PASS -> INCOMPLETE (fdo#107402)

    igt@drv_selftest@live_hangcheck:
      fi-kbl-7560u:       PASS -> DMESG-FAIL (fdo#106947, fdo#106560)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      {fi-cfl-8109u}:     DMESG-FAIL (fdo#106560) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         FAIL (fdo#107336) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107402 https://bugs.freedesktop.org/show_bug.cgi?id=107402


== Participating hosts (54 -> 48) ==

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-glk-j4005 


== Build changes ==

    * Linux: CI_DRM_4718 -> Patchwork_10035

  CI_DRM_4718: c7398fd19cef9b11c79af7292109507b6be075c4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4611: b966dd93a30f41581fe1dbf9bc1c4a29b552ca05 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10035: a0e065a6828aff0fff71319364dfbfd44e76dab7 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a0e065a6828a drm/i915/ringbuffer: Delay after invalidating gen6+ xcs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10035/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Missing byt full-ppgtt massage

[Rodrigo Vivi]

On Tue, Aug 28, 2018 at 06:04:28PM +0100, Chris Wilson wrote:
> None of you never noticed I didn't send this patch from my tree that I
> found helped byt full-ppgtt. I can't believe none of you are psychic.

I noticed nothing... :P
what tree or patch are we missing that helps full ppgtt on byt?

> -Chris
> 
> 
> _______________________________________________
> 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: Missing byt full-ppgtt massage

[Chris Wilson]

Quoting Rodrigo Vivi (2018-08-28 21:14:14)
> On Tue, Aug 28, 2018 at 06:04:28PM +0100, Chris Wilson wrote:
> > None of you never noticed I didn't send this patch from my tree that I
> > found helped byt full-ppgtt. I can't believe none of you are psychic.
> 
> I noticed nothing... :P
> what tree or patch are we missing that helps full ppgtt on byt?

My tree of 269 patches, but for now the patch in reply to this cover
letter...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915/ringbuffer: Delay after invalidating gen6+ xcs

[Patchwork]

== Series Details ==

Series: drm/i915/ringbuffer: Delay after invalidating gen6+ xcs
URL   : https://patchwork.freedesktop.org/series/48813/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4718_full -> Patchwork_10035_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_10035_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    igt@kms_vblank@pipe-b-accuracy-idle:
      shard-glk:          PASS -> INCOMPLETE (k.org#198133, fdo#103359)

    
    ==== Possible fixes ====

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-glk:          FAIL (fdo#105363) -> PASS

    
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4718 -> Patchwork_10035

  CI_DRM_4718: c7398fd19cef9b11c79af7292109507b6be075c4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4611: b966dd93a30f41581fe1dbf9bc1c4a29b552ca05 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10035: a0e065a6828aff0fff71319364dfbfd44e76dab7 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10035/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/ringbuffer: Delay after invalidating gen6+ xcs

[Ville Syrjälä]

On Tue, Aug 28, 2018 at 06:04:29PM +0100, Chris Wilson wrote:
> During stress testing of full-ppgtt (on Baytrail at least), we found
> that the invalidation around a context/mm switch was insufficient (writes
> would go astray). Adding a second MI_FLUSH_DW barrier prevents this, but
> it is unclear as to whether this is merely a delaying tactic or if it is
> truly serialising with the TLB invalidation. Either way, it is
> empirically required.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107715
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 101 +++++++++++-------------
>  1 file changed, 47 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d40f55a8dc34..952b6269bab0 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1944,40 +1944,62 @@ static void gen6_bsd_submit_request(struct i915_request *request)
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  }
>  
> -static int gen6_bsd_ring_flush(struct i915_request *rq, u32 mode)
> +static int emit_mi_flush_dw(struct i915_request *rq, u32 mode, u32 invflags)
>  {
>  	u32 cmd, *cs;
>  
> -	cs = intel_ring_begin(rq, 4);
> -	if (IS_ERR(cs))
> -		return PTR_ERR(cs);
> +	do {
> +		cs = intel_ring_begin(rq, 4);
> +		if (IS_ERR(cs))
> +			return PTR_ERR(cs);
>  
> -	cmd = MI_FLUSH_DW;
> +		cmd = MI_FLUSH_DW;
>  
> -	/* We always require a command barrier so that subsequent
> -	 * commands, such as breadcrumb interrupts, are strictly ordered
> -	 * wrt the contents of the write cache being flushed to memory
> -	 * (and thus being coherent from the CPU).
> -	 */
> -	cmd |= MI_FLUSH_DW_STORE_INDEX | MI_FLUSH_DW_OP_STOREDW;
> +		/*
> +		 * We always require a command barrier so that subsequent
> +		 * commands, such as breadcrumb interrupts, are strictly ordered
> +		 * wrt the contents of the write cache being flushed to memory
> +		 * (and thus being coherent from the CPU).
> +		 */
> +		cmd |= MI_FLUSH_DW_STORE_INDEX | MI_FLUSH_DW_OP_STOREDW;
>  
> -	/*
> -	 * Bspec vol 1c.5 - video engine command streamer:
> -	 * "If ENABLED, all TLBs will be invalidated once the flush
> -	 * operation is complete. This bit is only valid when the
> -	 * Post-Sync Operation field is a value of 1h or 3h."
> -	 */
> -	if (mode & EMIT_INVALIDATE)
> -		cmd |= MI_INVALIDATE_TLB | MI_INVALIDATE_BSD;
> +		/*
> +		 * Bspec vol 1c.3 - blitter engine command streamer:
> +		 * "If ENABLED, all TLBs will be invalidated once the flush
> +		 * operation is complete. This bit is only valid when the
> +		 * Post-Sync Operation field is a value of 1h or 3h."
> +		 */
> +		if (mode & EMIT_INVALIDATE)
> +			cmd |= invflags;
> +		*cs++ = cmd;
> +		*cs++ = I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT;
> +		*cs++ = 0;
> +		*cs++ = MI_NOOP;
> +		intel_ring_advance(rq, cs);
> +
> +		/*
> +		 * Not only do we need a full barrier (post-sync write) after
> +		 * invalidating the TLBs, but we need to wait a little bit
> +		 * longer. Whether this is merely delaying us, or the
> +		 * subsequent flush is a key part of serialising with the
> +		 * post-sync op, this extra pass appears vital before a
> +		 * mm switch!
> +		 */
> +		if (!(mode & EMIT_INVALIDATE))
> +			break;
> +
> +		mode &= ~EMIT_INVALIDATE;
> +	} while (1);

I find the loop thingy somewhat hard to read. I'd probably have
written it as something like

{
	if (mode & EMIT_INVALIDATE)
		mi_flush(INVALIDATE_TLB);
	mi_flush(0);
}

Either way it seems to do what it says so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
> -	*cs++ = cmd;
> -	*cs++ = I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT;
> -	*cs++ = 0;
> -	*cs++ = MI_NOOP;
> -	intel_ring_advance(rq, cs);
>  	return 0;
>  }
>  
> +static int gen6_bsd_ring_flush(struct i915_request *rq, u32 mode)
> +{
> +	return emit_mi_flush_dw(rq, mode,
> +				MI_INVALIDATE_TLB | MI_INVALIDATE_BSD);
> +}
> +
>  static int
>  hsw_emit_bb_start(struct i915_request *rq,
>  		  u64 offset, u32 len,
> @@ -2022,36 +2044,7 @@ gen6_emit_bb_start(struct i915_request *rq,
>  
>  static int gen6_ring_flush(struct i915_request *rq, u32 mode)
>  {
> -	u32 cmd, *cs;
> -
> -	cs = intel_ring_begin(rq, 4);
> -	if (IS_ERR(cs))
> -		return PTR_ERR(cs);
> -
> -	cmd = MI_FLUSH_DW;
> -
> -	/* We always require a command barrier so that subsequent
> -	 * commands, such as breadcrumb interrupts, are strictly ordered
> -	 * wrt the contents of the write cache being flushed to memory
> -	 * (and thus being coherent from the CPU).
> -	 */
> -	cmd |= MI_FLUSH_DW_STORE_INDEX | MI_FLUSH_DW_OP_STOREDW;
> -
> -	/*
> -	 * Bspec vol 1c.3 - blitter engine command streamer:
> -	 * "If ENABLED, all TLBs will be invalidated once the flush
> -	 * operation is complete. This bit is only valid when the
> -	 * Post-Sync Operation field is a value of 1h or 3h."
> -	 */
> -	if (mode & EMIT_INVALIDATE)
> -		cmd |= MI_INVALIDATE_TLB;
> -	*cs++ = cmd;
> -	*cs++ = I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT;
> -	*cs++ = 0;
> -	*cs++ = MI_NOOP;
> -	intel_ring_advance(rq, cs);
> -
> -	return 0;
> +	return emit_mi_flush_dw(rq, mode, MI_INVALIDATE_TLB);
>  }
>  
>  static void intel_ring_init_semaphores(struct drm_i915_private *dev_priv,
> -- 
> 2.18.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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