[PATCH] drm/i915/selftests: Provide full mb() around clflush

[PATCH] drm/i915/selftests: Provide full mb() around clflush

[Chris Wilson]

clflush is an unserialised instruction and the IA manual strongly advises
you to serialise it with a mb. To be cautious, apply one before and one
after, so that it is serialised with both writes and reads without
worrying too much about the required direction.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 .../drm/i915/selftests/i915_gem_coherency.c   | 21 ++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c
index 294c58aba2c1..df44c302a9fe 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c
@@ -42,11 +42,21 @@ static int cpu_set(struct drm_i915_gem_object *obj,
 
 	page = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
 	map = kmap_atomic(page);
-	if (needs_clflush & CLFLUSH_BEFORE)
+
+	if (needs_clflush & CLFLUSH_BEFORE) {
+		mb();
 		clflush(map+offset_in_page(offset) / sizeof(*map));
+		mb();
+	}
+
 	map[offset_in_page(offset) / sizeof(*map)] = v;
-	if (needs_clflush & CLFLUSH_AFTER)
+
+	if (needs_clflush & CLFLUSH_AFTER) {
+		mb();
 		clflush(map+offset_in_page(offset) / sizeof(*map));
+		mb();
+	}
+
 	kunmap_atomic(map);
 
 	i915_gem_obj_finish_shmem_access(obj);
@@ -68,8 +78,13 @@ static int cpu_get(struct drm_i915_gem_object *obj,
 
 	page = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
 	map = kmap_atomic(page);
-	if (needs_clflush & CLFLUSH_BEFORE)
+
+	if (needs_clflush & CLFLUSH_BEFORE) {
+		mb();
 		clflush(map+offset_in_page(offset) / sizeof(*map));
+		mb();
+	}
+
 	*v = map[offset_in_page(offset) / sizeof(*map)];
 	kunmap_atomic(map);
 
-- 
2.18.0

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

✗ Fi.CI.CHECKPATCH: warning for drm/i915/selftests: Provide full mb() around clflush

[Patchwork]

== Series Details ==

Series: drm/i915/selftests: Provide full mb() around clflush
URL   : https://patchwork.freedesktop.org/series/46085/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
cdfe0eb01ba9 drm/i915/selftests: Provide full mb() around clflush
-:24: WARNING:MEMORY_BARRIER: memory barrier without comment
#24: FILE: drivers/gpu/drm/i915/selftests/i915_gem_coherency.c:47:
+		mb();

-:26: WARNING:MEMORY_BARRIER: memory barrier without comment
#26: FILE: drivers/gpu/drm/i915/selftests/i915_gem_coherency.c:49:
+		mb();

-:33: WARNING:MEMORY_BARRIER: memory barrier without comment
#33: FILE: drivers/gpu/drm/i915/selftests/i915_gem_coherency.c:55:
+		mb();

-:35: WARNING:MEMORY_BARRIER: memory barrier without comment
#35: FILE: drivers/gpu/drm/i915/selftests/i915_gem_coherency.c:57:
+		mb();

-:48: WARNING:MEMORY_BARRIER: memory barrier without comment
#48: FILE: drivers/gpu/drm/i915/selftests/i915_gem_coherency.c:83:
+		mb();

-:50: WARNING:MEMORY_BARRIER: memory barrier without comment
#50: FILE: drivers/gpu/drm/i915/selftests/i915_gem_coherency.c:85:
+		mb();

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

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

✓ Fi.CI.BAT: success for drm/i915/selftests: Provide full mb() around clflush

[Patchwork]

== Series Details ==

Series: drm/i915/selftests: Provide full mb() around clflush
URL   : https://patchwork.freedesktop.org/series/46085/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4448 -> Patchwork_9574 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

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

    igt@kms_chamelium@hdmi-hpd-fast:
      fi-kbl-7500u:       SKIP -> FAIL (fdo#102672, fdo#103841)

    igt@kms_frontbuffer_tracking@basic:
      fi-skl-guc:         NOTRUN -> FAIL (fdo#103167)

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

    
    ==== Possible fixes ====

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

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

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#102672 https://bugs.freedesktop.org/show_bug.cgi?id=102672
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139


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

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


== Build changes ==

    * Linux: CI_DRM_4448 -> Patchwork_9574

  CI_DRM_4448: e094950bd5b5767bab1d2a5c259635dc6091f8cc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4543: 366eed37c7c71217e1cb1f3be5e26358a41f0001 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9574: cdfe0eb01ba9e72cddeb359438167509754b2403 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

cdfe0eb01ba9 drm/i915/selftests: Provide full mb() around clflush

== Logs ==

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

Re: [PATCH] drm/i915/selftests: Provide full mb() around clflush

[Rodrigo Vivi]

On Fri, Jul 06, 2018 at 06:49:26PM +0100, Chris Wilson wrote:
> clflush is an unserialised instruction and the IA manual strongly advises
> you to serialise it with a mb. To be cautious, apply one before and one
> after,

my understanding is that we need one before and one after anyways,
not just a matter of being cautious for being cautious..

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


> so that it is serialised with both writes and reads without
> worrying too much about the required direction.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  .../drm/i915/selftests/i915_gem_coherency.c   | 21 ++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c
> index 294c58aba2c1..df44c302a9fe 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c
> @@ -42,11 +42,21 @@ static int cpu_set(struct drm_i915_gem_object *obj,
>  
>  	page = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
>  	map = kmap_atomic(page);
> -	if (needs_clflush & CLFLUSH_BEFORE)
> +
> +	if (needs_clflush & CLFLUSH_BEFORE) {
> +		mb();
>  		clflush(map+offset_in_page(offset) / sizeof(*map));
> +		mb();
> +	}
> +
>  	map[offset_in_page(offset) / sizeof(*map)] = v;
> -	if (needs_clflush & CLFLUSH_AFTER)
> +
> +	if (needs_clflush & CLFLUSH_AFTER) {
> +		mb();
>  		clflush(map+offset_in_page(offset) / sizeof(*map));
> +		mb();
> +	}
> +
>  	kunmap_atomic(map);
>  
>  	i915_gem_obj_finish_shmem_access(obj);
> @@ -68,8 +78,13 @@ static int cpu_get(struct drm_i915_gem_object *obj,
>  
>  	page = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
>  	map = kmap_atomic(page);
> -	if (needs_clflush & CLFLUSH_BEFORE)
> +
> +	if (needs_clflush & CLFLUSH_BEFORE) {
> +		mb();
>  		clflush(map+offset_in_page(offset) / sizeof(*map));
> +		mb();
> +	}
> +
>  	*v = map[offset_in_page(offset) / sizeof(*map)];
>  	kunmap_atomic(map);
>  
> -- 
> 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/selftests: Provide full mb() around clflush

[Chris Wilson]

Quoting Rodrigo Vivi (2018-07-06 21:23:00)
> On Fri, Jul 06, 2018 at 06:49:26PM +0100, Chris Wilson wrote:
> > clflush is an unserialised instruction and the IA manual strongly advises
> > you to serialise it with a mb. To be cautious, apply one before and one
> > after,
> 
> my understanding is that we need one before and one after anyways,
> not just a matter of being cautious for being cautious..
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

If you are really careful, you can reduce it to a mb between writes and
the clflush; and vice verse, the mb between clflush and subsequent
reads. It's all about prevent the clflush from overtaking or being
overtaken by the operations it is meant to flush or invalidate
respectively.

But since we're being paranoid here, do both.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/selftests: Provide full mb() around clflush

[Rodrigo Vivi]

On Fri, Jul 06, 2018 at 09:27:47PM +0100, Chris Wilson wrote:
> Quoting Rodrigo Vivi (2018-07-06 21:23:00)
> > On Fri, Jul 06, 2018 at 06:49:26PM +0100, Chris Wilson wrote:
> > > clflush is an unserialised instruction and the IA manual strongly advises
> > > you to serialise it with a mb. To be cautious, apply one before and one
> > > after,
> > 
> > my understanding is that we need one before and one after anyways,
> > not just a matter of being cautious for being cautious..
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> If you are really careful, you can reduce it to a mb between writes and
> the clflush; and vice verse, the mb between clflush and subsequent
> reads. It's all about prevent the clflush from overtaking or being
> overtaken by the operations it is meant to flush or invalidate
> respectively.
> 
> But since we're being paranoid here, do both.

hm... makes sense... better to be paranoid...

> -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

✓ Fi.CI.IGT: success for drm/i915/selftests: Provide full mb() around clflush

[Patchwork]

== Series Details ==

Series: drm/i915/selftests: Provide full mb() around clflush
URL   : https://patchwork.freedesktop.org/series/46085/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4448_full -> Patchwork_9574_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9574_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9574_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_9574_full:

  === IGT changes ===

    ==== Warnings ====

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

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-apl:          PASS -> FAIL (fdo#106886)

    igt@gem_workarounds@suspend-resume-context:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

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

    igt@kms_cursor_legacy@cursor-vs-flip-toggle:
      shard-hsw:          PASS -> FAIL (fdo#103355)

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

    
    ==== Possible fixes ====

    igt@gem_render_copy_redux@normal:
      shard-kbl:          INCOMPLETE (fdo#106650, fdo#103665) -> PASS

    igt@kms_flip@2x-blocking-wf_vblank:
      shard-glk:          FAIL (fdo#100368) -> PASS +1

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-glk:          FAIL -> PASS

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

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703
  fdo#106650 https://bugs.freedesktop.org/show_bug.cgi?id=106650
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  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_4448 -> Patchwork_9574

  CI_DRM_4448: e094950bd5b5767bab1d2a5c259635dc6091f8cc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4543: 366eed37c7c71217e1cb1f3be5e26358a41f0001 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9574: cdfe0eb01ba9e72cddeb359438167509754b2403 @ 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_9574/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx