[PATCH] drm/i915: Enforce read order into the hardware status page csb

[PATCH] drm/i915: Enforce read order into the hardware status page csb

[Mika Kuoppala]

Read ordering on cpu can be out of order and speculative.
In addition, the access to the hardware status page is snooped.
This raises a concern that we might use the HWSP in an unexpected
way as we may load the head of a cbs entry before we access the tail.
Concerns like that the coherency protocol is tied somehow to the
tail read snoop.

To enforce that we really do read the tail before we fetch
the csb entry pointed by head, insert a read memory barrier after we
have read the tail.

This fixes, or masks due to added latency, context status
buffer incoherence on cnl, where we see an old context status
entry still on that a csb slot.

References: https://bugs.freedesktop.org/show_bug.cgi?id=105888
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Rafael Antognolli <rafael.antognolli@intel.com>
Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c7c85134a84a..5378391e1e71 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -969,6 +969,8 @@ static void execlists_submission_tasklet(unsigned long data)
 
 			head = execlists->csb_head;
 			tail = READ_ONCE(buf[write_idx]);
+			/* Enforce ordering of reads into the HWSP */
+			rmb();
 		}
 		GEM_TRACE("%s cs-irq head=%d [%d%s], tail=%d [%d%s]\n",
 			  engine->name,
-- 
2.14.1

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

Re: [PATCH] drm/i915: Enforce read order into the hardware status page csb

[Chris Wilson]

Quoting Mika Kuoppala (2018-04-12 14:52:37)
> Read ordering on cpu can be out of order and speculative.

If it didn't include a barrier to prevent compiler mispeculation. rmb()
only applies to other CPU cores and not the GPU so this is entirely
hocus.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enforce read order into the hardware status page csb

[Chris Wilson]

Quoting Chris Wilson (2018-04-12 14:57:42)
> Quoting Mika Kuoppala (2018-04-12 14:52:37)
> > Read ordering on cpu can be out of order and speculative.
> 
> If it didn't include a barrier to prevent compiler mispeculation. rmb()
> only applies to other CPU cores and not the GPU so this is entirely
> hocus.

To put it another way this is just imposing the cost of an uncached
mmio which the entire point of using HWSP in the first place is to
avoid.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enforce read order into the hardware status page csb

[Chris Wilson]

Quoting Chris Wilson (2018-04-12 14:57:42)
> Quoting Mika Kuoppala (2018-04-12 14:52:37)
> > Read ordering on cpu can be out of order and speculative.
> 
> If it didn't include a barrier to prevent compiler mispeculation. rmb()
> only applies to other CPU cores and not the GPU so this is entirely
> hocus.

Or think of it in terms checkpatch will ask you, where is the
corresponding wmb()?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enforce read order into the hardware status page csb

[Mika Kuoppala]

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

> Quoting Mika Kuoppala (2018-04-12 14:52:37)
>> Read ordering on cpu can be out of order and speculative.
>
> If it didn't include a barrier to prevent compiler mispeculation. rmb()
> only applies to other CPU cores and not the GPU so this is entirely
> hocus.

How I understand the Documentation/memory-barrier.txt/THE THINGS CPUS
GET UP TO, the compiler memory barrier is not enough.

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

✓ Fi.CI.BAT: success for drm/i915: Enforce read order into the hardware status page csb

[Patchwork]

== Series Details ==

Series: drm/i915: Enforce read order into the hardware status page csb
URL   : https://patchwork.freedesktop.org/series/41622/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4049 -> Patchwork_8678 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         PASS -> FAIL (fdo#102575)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-elk-e7500:       PASS -> DMESG-WARN (fdo#105225) +1

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

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


== Participating hosts (35 -> 33) ==

  Additional (1): fi-cnl-y3 
  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4049 -> Patchwork_8678

  CI_DRM_4049: 3dbfa04d62f1f1214a03c3b2d30f987dccf50ab4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4426: d502f055ac4500cada758876a512ac4f14b34851 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8678: 02b77742d0823525ce798b2bcfdc4a3bec295b32 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4426: 93b35926a150e318439d2505901288594b3548f5 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

02b77742d082 drm/i915: Enforce read order into the hardware status page csb

== Logs ==

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

✓ Fi.CI.IGT: success for drm/i915: Enforce read order into the hardware status page csb

[Patchwork]

== Series Details ==

Series: drm/i915: Enforce read order into the hardware status page csb
URL   : https://patchwork.freedesktop.org/series/41622/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4049_full -> Patchwork_8678_full =

== Summary - WARNING ==

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-render:
      shard-kbl:          PASS -> SKIP +1

    igt@gem_mocs_settings@mocs-rc6-vebox:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@plain-flip-ts-check:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@pm_rpm@debugfs-read:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103313, fdo#103558) +2

    
    ==== Possible fixes ====

    igt@kms_flip@2x-dpms-vs-vblank-race-interruptible:
      shard-hsw:          FAIL (fdo#103060) -> PASS +1

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

    
  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


== Participating hosts (6 -> 4) ==

  Missing    (2): shard-glk shard-glkb 


== Build changes ==

    * Linux: CI_DRM_4049 -> Patchwork_8678

  CI_DRM_4049: 3dbfa04d62f1f1214a03c3b2d30f987dccf50ab4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4426: d502f055ac4500cada758876a512ac4f14b34851 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8678: 02b77742d0823525ce798b2bcfdc4a3bec295b32 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4426: 93b35926a150e318439d2505901288594b3548f5 @ git://anongit.freedesktop.org/piglit

== Logs ==

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