[PATCH] drm/i915: Flush chipset caches after GGTT writes

[PATCH] drm/i915: Flush chipset caches after GGTT writes

[Chris Wilson]

Our I915g (early gen3, the oldest machine we have in the farm) is still
reporting occasional incoherency performing the following operations:

  1) write through GGTT (indirect write into memory)
  2) write through either CPU or WC (direct write into memory)
  3) read from GGTT (indirect read)

Instead of reporting the value from (2), the read from GGTT reports the
earlier value written via the GGTT. We have made sure that the writes are
flushed from the CPU (commit 3a32497f0dbe ("drm/i915/selftests: Provide
full mb() around clflush") and commit add00e6d896f ("drm/i915: Flush the
WCB following a WC write")), but still see the error, just less
frequently. The only remaining cache that might be affected here is a
chipset cache, so flush that as well.

Testcase: igt/drv_selftest/live_coherency #gdg
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 42d24410a98c..08266791801e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -802,7 +802,7 @@ void i915_gem_flush_ggtt_writes(struct drm_i915_private *dev_priv)
 	 * that was!).
 	 */
 
-	wmb();
+	i915_gem_chipset_flush(dev_priv);
 
 	intel_runtime_pm_get(dev_priv);
 	spin_lock_irq(&dev_priv->uncore.lock);
-- 
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: Flush chipset caches after GGTT writes

[Patchwork]

== Series Details ==

Series: drm/i915: Flush chipset caches after GGTT writes
URL   : https://patchwork.freedesktop.org/series/46681/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4501 -> Patchwork_9686 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9686 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9686, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

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

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9686:

  === IGT changes ===

    ==== Warnings ====

    igt@drv_selftest@live_guc:
      fi-kbl-7567u:       PASS -> SKIP +1

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

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

    igt@kms_chamelium@dp-edid-read:
      fi-kbl-7500u:       PASS -> FAIL (fdo#103841)

    igt@kms_flip@basic-flip-vs-dpms:
      fi-skl-6700hq:      PASS -> DMESG-WARN (fdo#105998)

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

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       DMESG-WARN (fdo#105128, fdo#107139) -> PASS

    igt@kms_busy@basic-flip-b:
      fi-skl-6700hq:      DMESG-WARN (fdo#105998) -> PASS +1

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         FAIL (fdo#104008) -> PASS

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139


== Participating hosts (46 -> 42) ==

  Additional (1): fi-cfl-8109u 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4501 -> Patchwork_9686

  CI_DRM_4501: 692d13f7b75baf0bb8c58b9784569c52d68f01e2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4559: 6d341aac2124836443ce74e8e97a4508ac8d5095 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9686: e36a071738710ccd306a78955bf07b64209d68f0 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

e36a07173871 drm/i915: Flush chipset caches after GGTT writes

== Logs ==

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

Re: [PATCH] drm/i915: Flush chipset caches after GGTT writes

[Rodrigo Vivi]

On Tue, Jul 17, 2018 at 10:26:55AM +0100, Chris Wilson wrote:
> Our I915g (early gen3, the oldest machine we have in the farm) is still
> reporting occasional incoherency performing the following operations:
> 
>   1) write through GGTT (indirect write into memory)
>   2) write through either CPU or WC (direct write into memory)
>   3) read from GGTT (indirect read)
> 
> Instead of reporting the value from (2), the read from GGTT reports the
> earlier value written via the GGTT. We have made sure that the writes are
> flushed from the CPU (commit 3a32497f0dbe ("drm/i915/selftests: Provide
> full mb() around clflush") and commit add00e6d896f ("drm/i915: Flush the
> WCB following a WC write")), but still see the error, just less
> frequently. The only remaining cache that might be affected here is a
> chipset cache, so flush that as well.
> 
> Testcase: igt/drv_selftest/live_coherency #gdg
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 42d24410a98c..08266791801e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -802,7 +802,7 @@ void i915_gem_flush_ggtt_writes(struct drm_i915_private *dev_priv)
>  	 * that was!).
>  	 */
>  
> -	wmb();
> +	i915_gem_chipset_flush(dev_priv);

this seems a void change for me... because I couldn't find the implementation
of intel_gtt_chipset_flush() so it seems that we are just replacing wmb per wmb

But I'm probably missing something here... please point me to the part that I'm
missing...

>  
>  	intel_runtime_pm_get(dev_priv);
>  	spin_lock_irq(&dev_priv->uncore.lock);
> -- 
> 2.18.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: Flush chipset caches after GGTT writes

[Chris Wilson]

Quoting Rodrigo Vivi (2018-07-17 15:32:08)
> On Tue, Jul 17, 2018 at 10:26:55AM +0100, Chris Wilson wrote:
> > Our I915g (early gen3, the oldest machine we have in the farm) is still
> > reporting occasional incoherency performing the following operations:
> > 
> >   1) write through GGTT (indirect write into memory)
> >   2) write through either CPU or WC (direct write into memory)
> >   3) read from GGTT (indirect read)
> > 
> > Instead of reporting the value from (2), the read from GGTT reports the
> > earlier value written via the GGTT. We have made sure that the writes are
> > flushed from the CPU (commit 3a32497f0dbe ("drm/i915/selftests: Provide
> > full mb() around clflush") and commit add00e6d896f ("drm/i915: Flush the
> > WCB following a WC write")), but still see the error, just less
> > frequently. The only remaining cache that might be affected here is a
> > chipset cache, so flush that as well.
> > 
> > Testcase: igt/drv_selftest/live_coherency #gdg
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 42d24410a98c..08266791801e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -802,7 +802,7 @@ void i915_gem_flush_ggtt_writes(struct drm_i915_private *dev_priv)
> >        * that was!).
> >        */
> >  
> > -     wmb();
> > +     i915_gem_chipset_flush(dev_priv);
> 
> this seems a void change for me... because I couldn't find the implementation
> of intel_gtt_chipset_flush() so it seems that we are just replacing wmb per wmb

For gen3, it triggers a write into the igfx flush page via
agp/intel-gtt.c, see i9xx_chipset_flush().
-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: Flush chipset caches after GGTT writes

[Patchwork]

== Series Details ==

Series: drm/i915: Flush chipset caches after GGTT writes
URL   : https://patchwork.freedesktop.org/series/46681/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4501_full -> Patchwork_9686_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9686_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9686_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9686_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-bsd1:
      shard-kbl:          SKIP -> PASS +1

    igt@perf_pmu@rc6:
      shard-kbl:          PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_partial_pwrite_pread@writes-after-reads:
      shard-glk:          PASS -> INCOMPLETE (fdo#103359, k.org#198133)

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
      shard-glk:          PASS -> FAIL (fdo#105703)

    
    ==== Possible fixes ====

    igt@kms_flip@plain-flip-fb-recreate:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@perf_pmu@rc6-runtime-pm-long:
      shard-hsw:          FAIL (fdo#105010) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#105010 https://bugs.freedesktop.org/show_bug.cgi?id=105010
  fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703
  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_4501 -> Patchwork_9686

  CI_DRM_4501: 692d13f7b75baf0bb8c58b9784569c52d68f01e2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4559: 6d341aac2124836443ce74e8e97a4508ac8d5095 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9686: e36a071738710ccd306a78955bf07b64209d68f0 @ 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_9686/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Flush chipset caches after GGTT writes

[Rodrigo Vivi]

On Tue, Jul 17, 2018 at 03:37:13PM +0100, Chris Wilson wrote:
> Quoting Rodrigo Vivi (2018-07-17 15:32:08)
> > On Tue, Jul 17, 2018 at 10:26:55AM +0100, Chris Wilson wrote:
> > > Our I915g (early gen3, the oldest machine we have in the farm) is still
> > > reporting occasional incoherency performing the following operations:
> > > 
> > >   1) write through GGTT (indirect write into memory)
> > >   2) write through either CPU or WC (direct write into memory)
> > >   3) read from GGTT (indirect read)
> > > 
> > > Instead of reporting the value from (2), the read from GGTT reports the
> > > earlier value written via the GGTT. We have made sure that the writes are
> > > flushed from the CPU (commit 3a32497f0dbe ("drm/i915/selftests: Provide
> > > full mb() around clflush") and commit add00e6d896f ("drm/i915: Flush the
> > > WCB following a WC write")), but still see the error, just less
> > > frequently. The only remaining cache that might be affected here is a
> > > chipset cache, so flush that as well.
> > > 
> > > Testcase: igt/drv_selftest/live_coherency #gdg
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 42d24410a98c..08266791801e 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -802,7 +802,7 @@ void i915_gem_flush_ggtt_writes(struct drm_i915_private *dev_priv)
> > >        * that was!).
> > >        */
> > >  
> > > -     wmb();
> > > +     i915_gem_chipset_flush(dev_priv);
> > 
> > this seems a void change for me... because I couldn't find the implementation
> > of intel_gtt_chipset_flush() so it seems that we are just replacing wmb per wmb
> 
> For gen3, it triggers a write into the igfx flush page via
> agp/intel-gtt.c, see i9xx_chipset_flush().

thanks for the pointer... now it makes sense

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

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

Re: [PATCH] drm/i915: Flush chipset caches after GGTT writes

[Chris Wilson]

Quoting Rodrigo Vivi (2018-07-17 16:13:41)
> On Tue, Jul 17, 2018 at 03:37:13PM +0100, Chris Wilson wrote:
> > Quoting Rodrigo Vivi (2018-07-17 15:32:08)
> > > On Tue, Jul 17, 2018 at 10:26:55AM +0100, Chris Wilson wrote:
> > > > Our I915g (early gen3, the oldest machine we have in the farm) is still
> > > > reporting occasional incoherency performing the following operations:
> > > > 
> > > >   1) write through GGTT (indirect write into memory)
> > > >   2) write through either CPU or WC (direct write into memory)
> > > >   3) read from GGTT (indirect read)
> > > > 
> > > > Instead of reporting the value from (2), the read from GGTT reports the
> > > > earlier value written via the GGTT. We have made sure that the writes are
> > > > flushed from the CPU (commit 3a32497f0dbe ("drm/i915/selftests: Provide
> > > > full mb() around clflush") and commit add00e6d896f ("drm/i915: Flush the
> > > > WCB following a WC write")), but still see the error, just less
> > > > frequently. The only remaining cache that might be affected here is a
> > > > chipset cache, so flush that as well.
> > > > 
> > > > Testcase: igt/drv_selftest/live_coherency #gdg
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > index 42d24410a98c..08266791801e 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > @@ -802,7 +802,7 @@ void i915_gem_flush_ggtt_writes(struct drm_i915_private *dev_priv)
> > > >        * that was!).
> > > >        */
> > > >  
> > > > -     wmb();
> > > > +     i915_gem_chipset_flush(dev_priv);
> > > 
> > > this seems a void change for me... because I couldn't find the implementation
> > > of intel_gtt_chipset_flush() so it seems that we are just replacing wmb per wmb
> > 
> > For gen3, it triggers a write into the igfx flush page via
> > agp/intel-gtt.c, see i9xx_chipset_flush().
> 
> thanks for the pointer... now it makes sense
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Sadly the only way to find out if this is the final piece of the puzzle
is to soak test it in CI, so in goes.

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