[PATCH 1/3] drm/i915/execlists: Use rmb() to order CSB reads

[PATCH 1/3] drm/i915/execlists: Use rmb() to order CSB reads

[Chris Wilson]

We assume that the CSB is written using the normal ringbuffer
coherency protocols, as outlined in kernel/events/ring_buffer.c:

    *   (HW)                              (DRIVER)
    *
    *   if (LOAD ->data_tail) {            LOAD ->data_head
    *                      (A)             smp_rmb()       (C)
    *      STORE $data                     LOAD $data
    *      smp_wmb()       (B)             smp_mb()        (D)
    *      STORE ->data_head               STORE ->data_tail
    *   }

So we assume that the HW fulfils its ordering requirements (B), and so
we should use a complimentary rmb (C) to ensure that our read of its
WRITE pointer is completed before we start accessing the data.

The final mb (D) is implied by the uncached mmio we perform to inform
the HW of our READ pointer.

References: https://bugs.freedesktop.org/show_bug.cgi?id=105064
References: https://bugs.freedesktop.org/show_bug.cgi?id=105888
References: https://bugs.freedesktop.org/show_bug.cgi?id=106185
Fixes: 767a983ab255 ("drm/i915/execlists: Read the context-status HEAD from the HWSP")
References: 61bf9719fa17 ("drm/i915/cnl: Use mmio access to context status buffer")
Suggested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Rafael Antognolli <rafael.antognolli@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Timo Aaltonen <tjaalton@ubuntu.com>
Tested-by: Timo Aaltonen <tjaalton@ubuntu.com>
Acked-by: Michel Thierry <michel.thierry@intel.com>
Acked-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 7f98dda3c929..11170bc36456 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1008,6 +1008,7 @@ static void execlists_submission_tasklet(unsigned long data)
 
 			head = execlists->csb_head;
 			tail = READ_ONCE(buf[write_idx]);
+			rmb(); /* Hopefully paired with a wmb() in HW */
 		}
 		GEM_TRACE("%s cs-irq head=%d [%d%s], tail=%d [%d%s]\n",
 			  engine->name,
-- 
2.17.0

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

[PATCH 2/3] Revert "drm/i915/cnl: Use mmio access to context status buffer"

[Chris Wilson]

In the previous patch (to include a rmb() after readig the CSB WRITE
pointer from the HWSP) we believe we have fixed the underlying bug, and
so can re-enable using the HWSP on Cannolake.

This reverts commit 61bf9719fa17 ("drm/i915/cnl: Use mmio access to
context status buffer").

References: https://bugs.freedesktop.org/show_bug.cgi?id=105888
References: https://bugs.freedesktop.org/show_bug.cgi?id=106185
References: 61bf9719fa17 ("drm/i915/cnl: Use mmio access to context status buffer")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Rafael Antognolli <rafael.antognolli@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Timo Aaltonen <tjaalton@ubuntu.com>
Tested-by: Timo Aaltonen <tjaalton@ubuntu.com>
Acked-by: Michel Thierry <michel.thierry@intel.com>
Acked-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 70325e0824e3..8303e05b0c7d 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -470,9 +470,6 @@ static bool csb_force_mmio(struct drm_i915_private *i915)
 	if (intel_vgpu_active(i915) && !intel_vgpu_has_hwsp_emulation(i915))
 		return true;
 
-	if (IS_CANNONLAKE(i915))
-		return true;
-
 	return false;
 }
 
-- 
2.17.0

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

[PATCH 3/3] drm/i915/execlists: Relax CSB force-mmio for VT-d

[Chris Wilson]

The original switch to use CSB from the HWSP was plagued by the effort
of read ordering on VT-d; we would read the WRITE pointer from the HWSP
before it had completed writing the CSB contents. The mystery comes down
to the lack of rmb() for correct ordering with respect to the writes
from HW, and with that resolved we can remove the VT-d special casing.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 8303e05b0c7d..6bfd7e3ed152 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -458,14 +458,6 @@ static void intel_engine_init_batch_pool(struct intel_engine_cs *engine)
 
 static bool csb_force_mmio(struct drm_i915_private *i915)
 {
-	/*
-	 * IOMMU adds unpredictable latency causing the CSB write (from the
-	 * GPU into the HWSP) to only be visible some time after the interrupt
-	 * (missed breadcrumb syndrome).
-	 */
-	if (intel_vtd_active())
-		return true;
-
 	/* Older GVT emulation depends upon intercepting CSB mmio */
 	if (intel_vgpu_active(i915) && !intel_vgpu_has_hwsp_emulation(i915))
 		return true;
-- 
2.17.0

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

Re: [PATCH 3/3] drm/i915/execlists: Relax CSB force-mmio for VT-d

[Chris Wilson]

Quoting Chris Wilson (2018-05-11 13:11:47)

> The original switch to use CSB from the HWSP was plagued by the effort

s/effort/effect/

> of read ordering on VT-d; we would read the WRITE pointer from the HWSP
> before it had completed writing the CSB contents. The mystery comes down
> to the lack of rmb() for correct ordering with respect to the writes
> from HW, and with that resolved we can remove the VT-d special casing.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915/execlists: Use rmb() to order CSB reads

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/execlists: Use rmb() to order CSB reads
URL   : https://patchwork.freedesktop.org/series/43051/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
a46b4fd07172 drm/i915/execlists: Use rmb() to order CSB reads
-:32: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#32: 
References: 61bf9719fa17 ("drm/i915/cnl: Use mmio access to context status buffer")

-:32: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 61bf9719fa17 ("drm/i915/cnl: Use mmio access to context status buffer")'
#32: 
References: 61bf9719fa17 ("drm/i915/cnl: Use mmio access to context status buffer")

total: 1 errors, 1 warnings, 0 checks, 7 lines checked
e881d6833698 Revert "drm/i915/cnl: Use mmio access to context status buffer"
-:19: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#19: 
References: 61bf9719fa17 ("drm/i915/cnl: Use mmio access to context status buffer")

-:19: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 61bf9719fa17 ("drm/i915/cnl: Use mmio access to context status buffer")'
#19: 
References: 61bf9719fa17 ("drm/i915/cnl: Use mmio access to context status buffer")

total: 1 errors, 1 warnings, 0 checks, 9 lines checked
7276a19d51fc drm/i915/execlists: Relax CSB force-mmio for VT-d

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/execlists: Use rmb() to order CSB reads

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/execlists: Use rmb() to order CSB reads
URL   : https://patchwork.freedesktop.org/series/43051/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4166 -> Patchwork_8980 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8980 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8980, 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/43051/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-cnl-y3:          PASS -> DMESG-WARN (fdo#104951)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

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


== Participating hosts (41 -> 36) ==

  Missing    (5): fi-ctg-p8600 fi-byt-squawks fi-ilk-m540 fi-glk-j4005 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4166 -> Patchwork_8980

  CI_DRM_4166: 974787811b2e4047519b0f81930e9d15f0cebd01 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4475: 35f08c12aa216d5b62a5b9984b575cee6905098f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8980: 7276a19d51fcff71587ad54d14045cb4ffbd283a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4475: 3ba0657bff4216d1ec7179935590261855f1651e @ git://anongit.freedesktop.org/piglit


== Linux commits ==

7276a19d51fc drm/i915/execlists: Relax CSB force-mmio for VT-d
e881d6833698 Revert "drm/i915/cnl: Use mmio access to context status buffer"
a46b4fd07172 drm/i915/execlists: Use rmb() to order CSB reads

== Logs ==

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

✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915/execlists: Use rmb() to order CSB reads

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/execlists: Use rmb() to order CSB reads
URL   : https://patchwork.freedesktop.org/series/43051/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4166_full -> Patchwork_8980_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8980_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8980_full, 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/43051/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

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

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

    igt@gem_pwrite@big-cpu-random:
      shard-apl:          PASS -> SKIP
      shard-glk:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#106023)

    igt@kms_flip@plain-flip-ts-check-interruptible:
      shard-glk:          PASS -> FAIL (fdo#100368) +1

    igt@kms_plane@plane-panning-top-left-pipe-c-planes:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602, fdo#103313) +5

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

    igt@pm_rpm@universal-planes-dpms:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +11

    
    ==== Possible fixes ====

    igt@kms_flip@absolute-wf_vblank-interruptible:
      shard-apl:          FAIL (fdo#106087) -> PASS

    igt@kms_flip@blocking-absolute-wf_vblank-interruptible:
      shard-kbl:          FAIL -> PASS

    igt@kms_flip@dpms-vs-vblank-race-interruptible:
      shard-glk:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@modeset-vs-vblank-race-interruptible:
      shard-hsw:          FAIL (fdo#103060) -> PASS

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

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103313 https://bugs.freedesktop.org/show_bug.cgi?id=103313
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106087 https://bugs.freedesktop.org/show_bug.cgi?id=106087
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


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

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4166 -> Patchwork_8980

  CI_DRM_4166: 974787811b2e4047519b0f81930e9d15f0cebd01 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4475: 35f08c12aa216d5b62a5b9984b575cee6905098f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8980: 7276a19d51fcff71587ad54d14045cb4ffbd283a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4475: 3ba0657bff4216d1ec7179935590261855f1651e @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH 3/3] drm/i915/execlists: Relax CSB force-mmio for VT-d

[Chris Wilson]

Quoting Chris Wilson (2018-05-11 13:11:47)
> The original switch to use CSB from the HWSP was plagued by the effort
> of read ordering on VT-d; we would read the WRITE pointer from the HWSP
> before it had completed writing the CSB contents. The mystery comes down
> to the lack of rmb() for correct ordering with respect to the writes
> from HW, and with that resolved we can remove the VT-d special casing.

Mika's been able to reproduce the VT-d issue and is soak testing this
fix, so I'll leave that until he's had a chance to confirm it survives.
In the meantime, I think we are reasonably happy this is the right fix
for Cannonlake and beyond, so I've pushed the first two patches.

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

Re: [PATCH 3/3] drm/i915/execlists: Relax CSB force-mmio for VT-d

[Mika Kuoppala]

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

> Quoting Chris Wilson (2018-05-11 13:11:47)
>> The original switch to use CSB from the HWSP was plagued by the effort
>> of read ordering on VT-d; we would read the WRITE pointer from the HWSP
>> before it had completed writing the CSB contents. The mystery comes down
>> to the lack of rmb() for correct ordering with respect to the writes
>> from HW, and with that resolved we can remove the VT-d special casing.
>
> Mika's been able to reproduce the VT-d issue and is soak testing this
> fix, so I'll leave that until he's had a chance to confirm it survives.
> In the meantime, I think we are reasonably happy this is the right fix
> for Cannonlake and beyond, so I've pushed the first two patches.
>

My kbl was quite sensitive to this, sometimes failing to even
boot without the vtd backoff.

Now after weekend of gem_ctx_switch and gem_exec_whisper
I am confident for,

Tested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/execlists: Relax CSB force-mmio for VT-d

[Chris Wilson]

Quoting Mika Kuoppala (2018-05-14 09:33:23)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Quoting Chris Wilson (2018-05-11 13:11:47)
> >> The original switch to use CSB from the HWSP was plagued by the effort
> >> of read ordering on VT-d; we would read the WRITE pointer from the HWSP
> >> before it had completed writing the CSB contents. The mystery comes down
> >> to the lack of rmb() for correct ordering with respect to the writes
> >> from HW, and with that resolved we can remove the VT-d special casing.
> >
> > Mika's been able to reproduce the VT-d issue and is soak testing this
> > fix, so I'll leave that until he's had a chance to confirm it survives.
> > In the meantime, I think we are reasonably happy this is the right fix
> > for Cannonlake and beyond, so I've pushed the first two patches.
> >
> 
> My kbl was quite sensitive to this, sometimes failing to even
> boot without the vtd backoff.
> 
> Now after weekend of gem_ctx_switch and gem_exec_whisper
> I am confident for,
> 
> Tested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Thanks. The proof of the pudding is in the eating! Pushed to a wider
audience :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx