Skip to contentSkip to conversation

drm/i915/gtt: Trust the uncached store to flush wcb

PATCH In review Mika Kuoppala 3 participants 7 messages May 8, 2018 – May 11, 2018

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

Mika Kuoppala

author
#0 lore raw

PATCH 1/1 drm/i915/gtt: Trust the uncached store to flush wcb

Not all architectures guarantee that uncached read will flush the write combining buffer. So marking it explicitly is recommended [1].

However we know the architecture we are operating on and can avoid wmb as the UC store will flush the wcb [2].

Omit the wmb() before invalidate as redudant.

v2: squash combining and removal (Chris)

References: http://yarchive.net/comp/linux/write_combining.html [1]
References: http://download.intel.com/design/PentiumII/applnots/24442201.pdf [2]

Cc Chris Wilson <[email protected]>
Cc Matthew Auld <[email protected]>
Signed-off-by Mika Kuoppala <[email protected]>
Reviewed-by Chris Wilson <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_gem_gtt.c +2 -4
index c879bfd9294f..2126358761a5 100644
@@ -110,7 +110,8 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma);
 
 static void gen6_ggtt_invalidate(struct drm_i915_private *dev_priv)
 {
-	/* Note that as an uncached mmio write, this should flush the
+	/*
+	 * Note that as an uncached mmio write, this will flush the
 	 * WCB of the writes into the GGTT before it triggers the invalidate.
 	 */
 	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);

@@ -2418,8 +2419,6 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 	for_each_sgt_dma(addr, sgt_iter, vma->pages)
 		gen8_set_pte(gtt_entries++, pte_encode | addr);
 
-	wmb();
-
 	/* This next bit makes the above posting read even more important. We
 	 * want to flush the TLBs only after we're certain all the PTE updates
 	 * have finished.

@@ -2460,7 +2459,6 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
 	dma_addr_t addr;
 	for_each_sgt_dma(addr, iter, vma->pages)
 		iowrite32(vm->pte_encode(addr, level, flags), &entries[i++]);
-	wmb();
 
 	/* This next bit makes the above posting read even more important. We
 	 * want to flush the TLBs only after we're certain all the PTE updates
Signature · 6 lines
2.14.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#1 lore raw

Quoting Mika Kuoppala (2018-05-08 13:03:13)

Quoted 36 lines

Not all architectures guarantee that uncached read will flush the write combining buffer. So marking it explicitly is recommended [1].

However we know the architecture we are operating on and can avoid wmb as the UC store will flush the wcb [2].

Omit the wmb() before invalidate as redudant.

v2: squash combining and removal (Chris)

References: http://yarchive.net/comp/linux/write_combining.html [1]
References: http://download.intel.com/design/PentiumII/applnots/24442201.pdf [2]

Cc Chris Wilson <[email protected]>
Cc Matthew Auld <[email protected]>
Signed-off-by Mika Kuoppala <[email protected]>
Reviewed-by Chris Wilson <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_gem_gtt.c +2 -1
index c879bfd9294f..2126358761a5 100644
@@ -110,7 +110,8 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma);
 
 static void gen6_ggtt_invalidate(struct drm_i915_private *dev_priv)
 {
-       /* Note that as an uncached mmio write, this should flush the
+       /*
+        * Note that as an uncached mmio write, this will flush the
         * WCB of the writes into the GGTT before it triggers the invalidate.
         */
        I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);

drivers/gpu/drm/i915/i915_gem_gtt.c static void gen8_ggtt_insert_entries(struct i915_address_space *vm, · line 2422

Quoted 6 lines
drivers/gpu/drm/i915/i915_gem_gtt.c +0 -2
@@ -2418,8 +2419,6 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
        for_each_sgt_dma(addr, sgt_iter, vma->pages)
                gen8_set_pte(gtt_entries++, pte_encode | addr);
 
-       wmb();
-
        /* This next bit makes the above posting read even more important. We

Yeah, we should rewrite this comment as well; just skip the first obsolete sentence.
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

author
#2 lore raw

PATCH 1/1 drm/i915/gtt: Trust the uncached store to flush wcb

Not all architectures guarantee that uncached read will flush the write combining buffer. So marking it explicitly is recommended [1].

However we know the architecture we are operating on and can avoid wmb as the UC store will flush the wcb [2].

Omit the wmb() before invalidate as redudant.

v2: squash combining and removal (Chris)
v3: remove obsolete comments about posting reads (Chris)

References: http://yarchive.net/comp/linux/write_combining.html [1]
References: http://download.intel.com/design/PentiumII/applnots/24442201.pdf [2]

Cc Chris Wilson <[email protected]>
Cc Matthew Auld <[email protected]>
Signed-off-by Mika Kuoppala <[email protected]>
Reviewed-by Chris Wilson <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)
drivers/gpu/drm/i915/i915_gem_gtt.c +8 -10
index c879bfd9294f..6eae9e1ed8be 100644
@@ -110,7 +110,8 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma);
 
 static void gen6_ggtt_invalidate(struct drm_i915_private *dev_priv)
 {
-	/* Note that as an uncached mmio write, this should flush the
+	/*
+	 * Note that as an uncached mmio write, this will flush the
 	 * WCB of the writes into the GGTT before it triggers the invalidate.
 	 */
 	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);

@@ -2418,11 +2419,9 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 	for_each_sgt_dma(addr, sgt_iter, vma->pages)
 		gen8_set_pte(gtt_entries++, pte_encode | addr);
 
-	wmb();
-
-	/* This next bit makes the above posting read even more important. We
-	 * want to flush the TLBs only after we're certain all the PTE updates
-	 * have finished.
+	/*
+	 * We want to flush the TLBs only after we're certain all the PTE
+	 * updates have finished.
 	 */
 	ggtt->invalidate(vm->i915);
 }

@@ -2460,11 +2459,10 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
 	dma_addr_t addr;
 	for_each_sgt_dma(addr, iter, vma->pages)
 		iowrite32(vm->pte_encode(addr, level, flags), &entries[i++]);
-	wmb();
 
-	/* This next bit makes the above posting read even more important. We
-	 * want to flush the TLBs only after we're certain all the PTE updates
-	 * have finished.
+	/*
+	 * We want to flush the TLBs only after we're certain all the PTE
+	 * updates have finished.
 	 */
 	ggtt->invalidate(vm->i915);
 }
Signature · 6 lines
2.14.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#3 lore raw
In reply to Chris Wilson
Quoted 40 lines · Chris Wilson

Quoting Mika Kuoppala (2018-05-08 13:03:13)

Quoted 39 lines

Not all architectures guarantee that uncached read will flush the write combining buffer. So marking it explicitly is recommended [1].

However we know the architecture we are operating on and can avoid wmb as the UC store will flush the wcb [2].

Omit the wmb() before invalidate as redudant.

v2: squash combining and removal (Chris)

References: http://yarchive.net/comp/linux/write_combining.html [1]
References: http://download.intel.com/design/PentiumII/applnots/24442201.pdf [2]

Cc Chris Wilson <[email protected]>
Cc Matthew Auld <[email protected]>
Signed-off-by Mika Kuoppala <[email protected]>
Reviewed-by Chris Wilson <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_gem_gtt.c +2 -1
index c879bfd9294f..2126358761a5 100644
@@ -110,7 +110,8 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma);
 
 static void gen6_ggtt_invalidate(struct drm_i915_private *dev_priv)
 {
-       /* Note that as an uncached mmio write, this should flush the
+       /*
+        * Note that as an uncached mmio write, this will flush the
         * WCB of the writes into the GGTT before it triggers the invalidate.
         */
        I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);

@@ -2418,8 +2419,6 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
        for_each_sgt_dma(addr, sgt_iter, vma->pages)
                gen8_set_pte(gtt_entries++, pte_encode | addr);
Quoted 6 lines

drivers/gpu/drm/i915/i915_gem_gtt.c static void gen8_ggtt_insert_entries(struct i915_address_space *vm, · line 2422

Quoted 3 lines
drivers/gpu/drm/i915/i915_gem_gtt.c +0 -2
-       wmb();
-
        /* This next bit makes the above posting read even more important. We

Yeah, we should rewrite this comment as well; just skip the first obsolete sentence.

v3 pushed. Thanks for review.

-Mika
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

CI
#4 lore raw

✗ Fi.CI.CHECKPATCH: warning for drm/i915/gtt: Trust the uncached store to flush wcb (rev2)

In reply to Mika Kuoppala
== Series Details ==

== Series Details ==

Series: drm/i915/gtt: Trust the uncached store to flush wcb (rev2)
URL : https://patchwork.freedesktop.org/series/42873/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
b634b2d79259 drm/i915/gtt: Trust the uncached store to flush wcb
-:19: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#19: 
References: http://download.intel.com/design/PentiumII/applnots/24442201.pdf [2]

total: 0 errors, 1 warnings, 0 checks, 37 lines checked

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

CI
#5 lore raw

✓ Fi.CI.BAT: success for drm/i915/gtt: Trust the uncached store to flush wcb (rev2)

In reply to Mika Kuoppala
== Series Details ==

== Series Details ==

Series: drm/i915/gtt: Trust the uncached store to flush wcb (rev2)
URL : https://patchwork.freedesktop.org/series/42873/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4157 -> Patchwork_8942 =

== Summary - WARNING ==

Minor unknown changes coming with Patchwork_8942 need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in Patchwork_8942, 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/42873/revisions/2/mbox/

== Possible new issues ==

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

=== IGT changes ===

    ==== Warnings ====

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

== Known issues ==

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

=== IGT changes ===

    ==== Issues hit ====

    igt@kms_frontbuffer_tracking@basic:
      {fi-hsw-peppy}:     PASS -> DMESG-FAIL (fdo#102614, fdo#106103)

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

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

    
    ==== Possible fixes ====

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

    igt@kms_flip@basic-flip-vs-dpms:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

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

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

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

Missing (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-skl-6700hq

== Build changes ==

* Linux: CI_DRM_4157 -> Patchwork_8942

CI_DRM_4157: 580a7e5eafe15dd21f0dfc92d860a57a404e622d @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4464: 1bb318b32db003a377da14715c7b80675a712b6b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_8942: b634b2d7925954bfb450932953aa26756dc830ea @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4464: 33e58d5583eb7ed3966a1b905f875a1dfa959f6b @ git://anongit.freedesktop.org/piglit

== Linux commits ==

b634b2d79259 drm/i915/gtt: Trust the uncached store to flush wcb

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8942/issues.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

CI
#6 lore raw

✓ Fi.CI.IGT: success for drm/i915/gtt: Trust the uncached store to flush wcb (rev2)

In reply to Mika Kuoppala
== Series Details ==

== Series Details ==

Series: drm/i915/gtt: Trust the uncached store to flush wcb (rev2)
URL : https://patchwork.freedesktop.org/series/42873/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4157_full -> Patchwork_8942_full =

== Summary - SUCCESS ==

No regressions found.

External URL: https://patchwork.freedesktop.org/api/1.0/series/42873/revisions/2/mbox/

== Known issues ==

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

=== IGT changes ===

    ==== Issues hit ====

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

    
    ==== Possible fixes ====

    igt@kms_atomic_transition@2x-modeset-transitions-nonblocking:
      shard-hsw:          DMESG-FAIL (fdo#104724) -> PASS

    igt@kms_flip@2x-plain-flip-fb-recreate-interruptible:
      shard-hsw:          FAIL (fdo#103928) -> PASS

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

    igt@kms_flip@busy-flip-interruptible:
      shard-apl:          FAIL -> PASS

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

fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724

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

No changes in participating hosts

== Build changes ==

* Linux: CI_DRM_4157 -> Patchwork_8942

CI_DRM_4157: 580a7e5eafe15dd21f0dfc92d860a57a404e622d @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4464: 1bb318b32db003a377da14715c7b80675a712b6b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_8942: b634b2d7925954bfb450932953aa26756dc830ea @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4464: 33e58d5583eb7ed3966a1b905f875a1dfa959f6b @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8942/shards.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx