[PATCH 1/2] drm/i915: Unconditionally flush any chipset buffers before execbuf

[PATCH 1/2] drm/i915: Unconditionally flush any chipset buffers before execbuf

[Chris Wilson]

If userspace is asynchronously streaming into the batch or other
execobjects, we may not flush those writes along with a change in cache
domain (as there is no change). Therefore those writes may end up in
internal chipset buffers and not visible to the GPU upon execution. We
must issue a flush command or otherwise we encounter incoherency in the
batchbuffers and the GPU executing invalid commands (i.e. hanging) quite
regularly.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90841
Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user...")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Akash Goel <akash.goel@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Tested-by: Matti Hämäläinen <ccr@tnsp.org>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 699315304748..bce587abc601 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1015,8 +1015,6 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
 {
 	const unsigned int other_rings = eb_other_engines(req);
 	struct i915_vma *vma;
-	uint32_t flush_domains = 0;
-	bool flush_chipset = false;
 	int ret;
 
 	list_for_each_entry(vma, vmas, exec_list) {
@@ -1029,16 +1027,11 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
 		}
 
 		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
-			flush_chipset |= i915_gem_clflush_object(obj, false);
-
-		flush_domains |= obj->base.write_domain;
+			i915_gem_clflush_object(obj, false);
 	}
 
-	if (flush_chipset)
-		i915_gem_chipset_flush(req->engine->i915);
-
-	if (flush_domains & I915_GEM_DOMAIN_GTT)
-		wmb();
+	/* Unconditionally flush any chipset caches (for streaming writes). */
+	i915_gem_chipset_flush(req->engine->i915);
 
 	/* Unconditionally invalidate GPU caches and TLBs. */
 	return req->engine->emit_flush(req, EMIT_INVALIDATE);
-- 
2.9.3

[PATCH 1/2] drm/i915: Unconditionally flush any chipset buffers before execbuf

[Chris Wilson]

If userspace is asynchronously streaming into the batch or other
execobjects, we may not flush those writes along with a change in cache
domain (as there is no change). Therefore those writes may end up in
internal chipset buffers and not visible to the GPU upon execution. We
must issue a flush command or otherwise we encounter incoherency in the
batchbuffers and the GPU executing invalid commands (i.e. hanging) quite
regularly.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90841
Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user...")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Akash Goel <akash.goel@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Tested-by: Matti Hämäläinen <ccr@tnsp.org>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 699315304748..bce587abc601 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1015,8 +1015,6 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
 {
 	const unsigned int other_rings = eb_other_engines(req);
 	struct i915_vma *vma;
-	uint32_t flush_domains = 0;
-	bool flush_chipset = false;
 	int ret;
 
 	list_for_each_entry(vma, vmas, exec_list) {
@@ -1029,16 +1027,11 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
 		}
 
 		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
-			flush_chipset |= i915_gem_clflush_object(obj, false);
-
-		flush_domains |= obj->base.write_domain;
+			i915_gem_clflush_object(obj, false);
 	}
 
-	if (flush_chipset)
-		i915_gem_chipset_flush(req->engine->i915);
-
-	if (flush_domains & I915_GEM_DOMAIN_GTT)
-		wmb();
+	/* Unconditionally flush any chipset caches (for streaming writes). */
+	i915_gem_chipset_flush(req->engine->i915);
 
 	/* Unconditionally invalidate GPU caches and TLBs. */
 	return req->engine->emit_flush(req, EMIT_INVALIDATE);
-- 
2.9.3

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

[PATCH 2/2] agp/intel: Flush chipset writes after updating a single PTE

[Chris Wilson]

After we update one PTE for a page, the caller expects to be able to
immediately use that through a GGTT read/write. To comply with the
callers expectations we therefore need to flush the chipset buffers
before returning.

Reported-by: Matti Hämäläinen <ccr@tnsp.org>
Fixes: d6473f566417 ("drm/i915: Add support for mapping an object page...")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Tested-by: Matti Hämäläinen <ccr@tnsp.org>
Cc: drm-intel-fixes@lists.freedesktop.org
---
 drivers/char/agp/intel-gtt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index 44311296ec02..0f7d28a98b9a 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -845,6 +845,8 @@ void intel_gtt_insert_page(dma_addr_t addr,
 			   unsigned int flags)
 {
 	intel_private.driver->write_entry(addr, pg, flags);
+	if (intel_private.driver->chipset_flush)
+		intel_private.driver->chipset_flush();
 }
 EXPORT_SYMBOL(intel_gtt_insert_page);
 
-- 
2.9.3

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

[PATCH v2] drm/i915: Fallback to single page pwrite/pread if unable to release fence

[Chris Wilson]

If we cannot release the fence (for example if someone is inexplicably
trying to write into a tiled framebuffer that is currently pinned to the
display! *cough* kms_frontbuffer_tracking *cough*) fallback to using the
page-by-page pwrite/pread interface, rather than fail the syscall
entirely.

Since this is triggerable by the user (along pwrite) we have to remove
the WARN_ON(fence->pin_count).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c       | 30 ++++++++++++++++++------------
 drivers/gpu/drm/i915/i915_gem_fence.c |  2 +-
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index cb75beb7f352..6d64ee34efc6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -754,6 +754,15 @@ i915_gem_gtt_pread(struct drm_device *dev,
 	int ret;
 
 	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, PIN_MAPPABLE);
+	if (!IS_ERR(vma)) {
+		node.start = i915_ggtt_offset(vma);
+		node.allocated = false;
+		ret = i915_gem_object_put_fence(obj);
+		if (ret) {
+			i915_vma_unpin(vma);
+			vma = ERR_PTR(ret);
+		}
+	}
 	if (IS_ERR(vma)) {
 		ret = insert_mappable_node(dev_priv, &node, PAGE_SIZE);
 		if (ret)
@@ -766,12 +775,6 @@ i915_gem_gtt_pread(struct drm_device *dev,
 		}
 
 		i915_gem_object_pin_pages(obj);
-	} else {
-		node.start = i915_ggtt_offset(vma);
-		node.allocated = false;
-		ret = i915_gem_object_put_fence(obj);
-		if (ret)
-			goto out_unpin;
 	}
 
 	ret = i915_gem_object_set_to_gtt_domain(obj, false);
@@ -1058,6 +1061,15 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
 
 	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
 				       PIN_MAPPABLE | PIN_NONBLOCK);
+	if (!IS_ERR(vma)) {
+		node.start = i915_ggtt_offset(vma);
+		node.allocated = false;
+		ret = i915_gem_object_put_fence(obj);
+		if (ret) {
+			i915_vma_unpin(vma);
+			vma = ERR_PTR(ret);
+		}
+	}
 	if (IS_ERR(vma)) {
 		ret = insert_mappable_node(i915, &node, PAGE_SIZE);
 		if (ret)
@@ -1070,12 +1082,6 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
 		}
 
 		i915_gem_object_pin_pages(obj);
-	} else {
-		node.start = i915_ggtt_offset(vma);
-		node.allocated = false;
-		ret = i915_gem_object_put_fence(obj);
-		if (ret)
-			goto out_unpin;
 	}
 
 	ret = i915_gem_object_set_to_gtt_domain(obj, true);
diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c
index 334c3c4e8357..b0c6c2777725 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence.c
@@ -298,7 +298,7 @@ i915_gem_object_put_fence(struct drm_i915_gem_object *obj)
 
 	fence = &dev_priv->fence_regs[obj->fence_reg];
 
-	if (WARN_ON(fence->pin_count))
+	if (fence->pin_count)
 		return -EBUSY;
 
 	i915_gem_object_fence_lost(obj);
-- 
2.9.3

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

✓ Ro.CI.BAT: success for series starting with [1/2] drm/i915: Unconditionally flush any chipset buffers before execbuf

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Unconditionally flush any chipset buffers before execbuf
URL   : https://patchwork.freedesktop.org/series/11266/
State : success

== Summary ==

Series 11266v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/11266/revisions/1/mbox

Test kms_cursor_legacy:
        Subgroup basic-flip-vs-cursor-varying-size:
                fail       -> PASS       (ro-bdw-i5-5250u)
                dmesg-fail -> PASS       (fi-skl-i7-6700k)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-hsw-i7-4770k  total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-qkkr      total:244  pass:185  dwarn:29  dfail:0   fail:3   skip:27 
fi-skl-i7-6700k  total:244  pass:209  dwarn:4   dfail:1   fail:2   skip:28 
fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:220  dwarn:1   dfail:0   fail:0   skip:19 
ro-bdw-i7-5600u  total:240  pass:206  dwarn:1   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:195  dwarn:0   dfail:0   fail:3   skip:42 
ro-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-ilk1-i5-650   total:235  pass:174  dwarn:0   dfail:0   fail:1   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:0   fail:4   skip:14 
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1928/

ee3f71b drm-intel-nightly: 2016y-08m-18d-12h-30m-08s UTC integration manifest
e36f7d5 agp/intel: Flush chipset writes after updating a single PTE
4fe257d drm/i915: Unconditionally flush any chipset buffers before execbuf

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

Re: [PATCH 1/2] drm/i915: Unconditionally flush any chipset buffers before execbuf

[Mika Kuoppala]

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

> If userspace is asynchronously streaming into the batch or other
> execobjects, we may not flush those writes along with a change in cache
> domain (as there is no change). Therefore those writes may end up in
> internal chipset buffers and not visible to the GPU upon execution. We
> must issue a flush command or otherwise we encounter incoherency in the
> batchbuffers and the GPU executing invalid commands (i.e. hanging) quite
> regularly.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90841
> Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user...")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Akash Goel <akash.goel@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Tested-by: Matti Hämäläinen <ccr@tnsp.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 699315304748..bce587abc601 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1015,8 +1015,6 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
>  {
>  	const unsigned int other_rings = eb_other_engines(req);
>  	struct i915_vma *vma;
> -	uint32_t flush_domains = 0;
> -	bool flush_chipset = false;
>  	int ret;
>  
>  	list_for_each_entry(vma, vmas, exec_list) {
> @@ -1029,16 +1027,11 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
>  		}
>  
>  		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
> -			flush_chipset |= i915_gem_clflush_object(obj, false);
> -
> -		flush_domains |= obj->base.write_domain;
> +			i915_gem_clflush_object(obj, false);
>  	}
>  
> -	if (flush_chipset)
> -		i915_gem_chipset_flush(req->engine->i915);
> -
> -	if (flush_domains & I915_GEM_DOMAIN_GTT)
> -		wmb();

Was a bit worried about this vanishing.

But after chatting with Chris and also founding this:
https://lwn.net/Articles/174655/
[in I386 AND X86_64 SPECIFIC NOTES]

I am convinced that the uncached write to force the chipset_flush
will be strong barrier enough.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> +	/* Unconditionally flush any chipset caches (for streaming writes). */
> +	i915_gem_chipset_flush(req->engine->i915);
>  
>  	/* Unconditionally invalidate GPU caches and TLBs. */
>  	return req->engine->emit_flush(req, EMIT_INVALIDATE);
> -- 
> 2.9.3
>
> _______________________________________________
> 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

✗ Ro.CI.BAT: warning for series starting with [v2] drm/i915: Fallback to single page pwrite/pread if unable to release fence (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915: Fallback to single page pwrite/pread if unable to release fence (rev2)
URL   : https://patchwork.freedesktop.org/series/11266/
State : warning

== Summary ==

Series 11266v2 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/11266/revisions/2/mbox

Test drv_module_reload_basic:
                pass       -> SKIP       (ro-bdw-i5-5250u)
Test kms_cursor_legacy:
        Subgroup basic-flip-vs-cursor-legacy:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-flip-vs-cursor-varying-size:
                fail       -> PASS       (ro-skl3-i5-6260u)
                dmesg-fail -> PASS       (fi-skl-i7-6700k)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)

fi-hsw-i7-4770k  total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-qkkr      total:244  pass:185  dwarn:30  dfail:0   fail:2   skip:27 
fi-skl-i7-6700k  total:244  pass:209  dwarn:4   dfail:1   fail:2   skip:28 
fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:218  dwarn:3   dfail:0   fail:1   skip:18 
ro-bdw-i7-5600u  total:240  pass:207  dwarn:0   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:193  dwarn:0   dfail:0   fail:4   skip:43 
ro-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:185  dwarn:0   dfail:0   fail:0   skip:55 
ro-ilk1-i5-650   total:235  pass:174  dwarn:0   dfail:0   fail:1   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:224  dwarn:0   dfail:0   fail:2   skip:14 
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1929/

ee3f71b drm-intel-nightly: 2016y-08m-18d-12h-30m-08s UTC integration manifest
3e4cac6 agp/intel: Flush chipset writes after updating a single PTE
b76d651 drm/i915: Fallback to single page pwrite/pread if unable to release fence

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

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Unconditionally flush any chipset buffers before execbuf

[Chris Wilson]

On Thu, Aug 18, 2016 at 04:59:35PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > If userspace is asynchronously streaming into the batch or other
> > execobjects, we may not flush those writes along with a change in cache
> > domain (as there is no change). Therefore those writes may end up in
> > internal chipset buffers and not visible to the GPU upon execution. We
> > must issue a flush command or otherwise we encounter incoherency in the
> > batchbuffers and the GPU executing invalid commands (i.e. hanging) quite
> > regularly.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90841
> > Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user...")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Akash Goel <akash.goel@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Tested-by: Matti H�m�l�inen <ccr@tnsp.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++----------
> >  1 file changed, 3 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 699315304748..bce587abc601 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1015,8 +1015,6 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
> >  {
> >  	const unsigned int other_rings = eb_other_engines(req);
> >  	struct i915_vma *vma;
> > -	uint32_t flush_domains = 0;
> > -	bool flush_chipset = false;
> >  	int ret;
> >  
> >  	list_for_each_entry(vma, vmas, exec_list) {
> > @@ -1029,16 +1027,11 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
> >  		}
> >  
> >  		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
> > -			flush_chipset |= i915_gem_clflush_object(obj, false);
> > -
> > -		flush_domains |= obj->base.write_domain;
> > +			i915_gem_clflush_object(obj, false);
> >  	}
> >  
> > -	if (flush_chipset)
> > -		i915_gem_chipset_flush(req->engine->i915);
> > -
> > -	if (flush_domains & I915_GEM_DOMAIN_GTT)
> > -		wmb();
> 
> Was a bit worried about this vanishing.
> 
> But after chatting with Chris and also founding this:
> https://lwn.net/Articles/174655/
> [in I386 AND X86_64 SPECIFIC NOTES]
> 
> I am convinced that the uncached write to force the chipset_flush
> will be strong barrier enough.
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

Fwiw, the v2 that was meant to be here puts the wmb() back into
i915_gem_chipset_flush() so that we always have at least an sfence. It
should be redundant, but it shouldn't be noticeable either.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Unconditionally flush any chipset buffers before execbuf

[Chris Wilson]

On Thu, Aug 18, 2016 at 04:59:35PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > If userspace is asynchronously streaming into the batch or other
> > execobjects, we may not flush those writes along with a change in cache
> > domain (as there is no change). Therefore those writes may end up in
> > internal chipset buffers and not visible to the GPU upon execution. We
> > must issue a flush command or otherwise we encounter incoherency in the
> > batchbuffers and the GPU executing invalid commands (i.e. hanging) quite
> > regularly.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90841
> > Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user...")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Akash Goel <akash.goel@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Tested-by: Matti Hämäläinen <ccr@tnsp.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++----------
> >  1 file changed, 3 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 699315304748..bce587abc601 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1015,8 +1015,6 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
> >  {
> >  	const unsigned int other_rings = eb_other_engines(req);
> >  	struct i915_vma *vma;
> > -	uint32_t flush_domains = 0;
> > -	bool flush_chipset = false;
> >  	int ret;
> >  
> >  	list_for_each_entry(vma, vmas, exec_list) {
> > @@ -1029,16 +1027,11 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
> >  		}
> >  
> >  		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
> > -			flush_chipset |= i915_gem_clflush_object(obj, false);
> > -
> > -		flush_domains |= obj->base.write_domain;
> > +			i915_gem_clflush_object(obj, false);
> >  	}
> >  
> > -	if (flush_chipset)
> > -		i915_gem_chipset_flush(req->engine->i915);
> > -
> > -	if (flush_domains & I915_GEM_DOMAIN_GTT)
> > -		wmb();
> 
> Was a bit worried about this vanishing.
> 
> But after chatting with Chris and also founding this:
> https://lwn.net/Articles/174655/
> [in I386 AND X86_64 SPECIFIC NOTES]
> 
> I am convinced that the uncached write to force the chipset_flush
> will be strong barrier enough.
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

Fwiw, the v2 that was meant to be here puts the wmb() back into
i915_gem_chipset_flush() so that we always have at least an sfence. It
should be redundant, but it shouldn't be noticeable either.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] agp/intel: Flush chipset writes after updating a single PTE

[Mika Kuoppala]

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

> After we update one PTE for a page, the caller expects to be able to
> immediately use that through a GGTT read/write. To comply with the
> callers expectations we therefore need to flush the chipset buffers
> before returning.
>
> Reported-by: Matti Hämäläinen <ccr@tnsp.org>
> Fixes: d6473f566417 ("drm/i915: Add support for mapping an object page...")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> Cc: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Tested-by: Matti Hämäläinen <ccr@tnsp.org>
> Cc: drm-intel-fixes@lists.freedesktop.org
> ---
>  drivers/char/agp/intel-gtt.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index 44311296ec02..0f7d28a98b9a 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -845,6 +845,8 @@ void intel_gtt_insert_page(dma_addr_t addr,
>  			   unsigned int flags)
>  {
>  	intel_private.driver->write_entry(addr, pg, flags);
> +	if (intel_private.driver->chipset_flush)
> +		intel_private.driver->chipset_flush();
>  }
>  EXPORT_SYMBOL(intel_gtt_insert_page);
>  
> -- 
> 2.9.3
>
> _______________________________________________
> 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