[PATCH 1/5] drm/i915: Suppress EIO during set-to-gtt

[PATCH 1/5] drm/i915: Suppress EIO during set-to-gtt

[Chris Wilson]

If we fail to flush the rendering to an object already bound to the GTT
because the hardware is edge, all is good!

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 71934dd..16591ad 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2880,12 +2880,12 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
 		return 0;
 
 	ret = i915_gem_object_flush_gpu_write_domain(obj);
-	if (ret)
+	if (ret && ret != -EIO)
 		return ret;
 
 	if (obj->pending_gpu_write || write) {
 		ret = i915_gem_object_wait_rendering(obj);
-		if (ret)
+		if (ret && ret != -EIO)
 			return ret;
 	}
 
-- 
1.7.10

[PATCH 2/5] drm/i915: Suppress EIO through fence-flushing

[Chris Wilson]

If the hardware dies without outstanding GPU fenced access, we no longer
care and can simply reuse the fence.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 16591ad..ffe0853 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2347,7 +2347,7 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj,
 		if (obj->base.write_domain & I915_GEM_GPU_DOMAINS) {
 			ret = i915_gem_flush_ring(obj->last_fenced_ring,
 						  0, obj->base.write_domain);
-			if (ret)
+			if (ret && ret != -EIO)
 				return ret;
 		}
 
@@ -2360,7 +2360,7 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj,
 			ret = i915_wait_request(obj->last_fenced_ring,
 						obj->last_fenced_seqno,
 						true);
-			if (ret)
+			if (ret && ret != -EIO)
 				return ret;
 		}
 
-- 
1.7.10

Re: [PATCH 2/5] drm/i915: Suppress EIO through fence-flushing

[Chris Wilson]

On Sat, 14 Apr 2012 09:55:48 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> If the hardware dies without outstanding GPU fenced access, we no longer
> care and can simply reuse the fence.

If the hardware dies with outstanding GPU fenced accesses, we no longer
need to care and can simply reuse the fence.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/5] drm/i915: Suppress EIO during i915_gem_idle()

[Chris Wilson]

i915_gem_idle() is called when we need to suspend the GPU. If the GPU is
already suspended by this point due to being wedged, we can safely
continue.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ffe0853..a6819c1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3576,7 +3576,7 @@ i915_gem_idle(struct drm_device *dev)
 	}
 
 	ret = i915_gpu_idle(dev, true);
-	if (ret) {
+	if (ret && ret != -EIO) {
 		mutex_unlock(&dev->struct_mutex);
 		return ret;
 	}
-- 
1.7.10

Re: [PATCH 3/5] drm/i915: Suppress EIO during i915_gem_idle()

[Daniel Vetter]

On Sat, Apr 14, 2012 at 09:55:49AM +0100, Chris Wilson wrote:
> i915_gem_idle() is called when we need to suspend the GPU. If the GPU is
> already suspended by this point due to being wedged, we can safely
> continue.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Imo the right way to fix this case here is by checking this in gpu_idle
and just not bothering to emit any flushes and request if the gpu is
wedged already.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ffe0853..a6819c1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3576,7 +3576,7 @@ i915_gem_idle(struct drm_device *dev)
>  	}
>  
>  	ret = i915_gpu_idle(dev, true);
> -	if (ret) {
> +	if (ret && ret != -EIO) {
>  		mutex_unlock(&dev->struct_mutex);
>  		return ret;
>  	}
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/5] drm/i915: Suppress EIO during i915_gem_idle()

[Chris Wilson]

On Wed, 18 Apr 2012 10:51:29 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, Apr 14, 2012 at 09:55:49AM +0100, Chris Wilson wrote:
> > i915_gem_idle() is called when we need to suspend the GPU. If the GPU is
> > already suspended by this point due to being wedged, we can safely
> > continue.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Imo the right way to fix this case here is by checking this in gpu_idle
> and just not bothering to emit any flushes and request if the gpu is
> wedged already.

Updated to fix i915_gpu_idle() instead. I don't want to add any more
checks to dev->wedged though, as that I agree will become a nightmare.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 4/5] drm/i915: Always flush tiling changes before accessing through the GTT

[Chris Wilson]

As we defer updating the fence register from set-tiling to the point of
use, we need to declare every access through the GTT as either fenced or
unfenced.

This patches fixes up a couple of freshly introduced GTT accesses which
missed the fence flush.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a6819c1..19ca320 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -400,6 +400,10 @@ i915_gem_shmem_pread(struct drm_device *dev,
 		ret = i915_gem_object_set_to_gtt_domain(obj, false);
 		if (ret)
 			return ret;
+
+		ret = i915_gem_object_put_fence(obj);
+		if (ret)
+			return ret;
 	}
 
 	offset = args->offset;
@@ -717,6 +721,10 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
 		ret = i915_gem_object_set_to_gtt_domain(obj, true);
 		if (ret)
 			return ret;
+
+		ret = i915_gem_object_put_fence(obj);
+		if (ret)
+			return ret;
 	}
 	/* Same trick applies for invalidate partially written cachelines before
 	 * writing.  */
-- 
1.7.10

Re: [PATCH 4/5] drm/i915: Always flush tiling changes before accessing through the GTT

[Daniel Vetter]

On Sat, Apr 14, 2012 at 09:55:50AM +0100, Chris Wilson wrote:
> As we defer updating the fence register from set-tiling to the point of
> use, we need to declare every access through the GTT as either fenced or
> unfenced.
> 
> This patches fixes up a couple of freshly introduced GTT accesses which
> missed the fence flush.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Actually no, this set_to_gtt is actually just a flush+wait_rendering
because the shmem pwrite/pread afters does clever in-line clflushing.
Abuse of interfaces, I know ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 4/5] drm/i915: Always flush tiling changes before accessing through the GTT

[Chris Wilson]

On Sat, 14 Apr 2012 23:54:52 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, Apr 14, 2012 at 09:55:50AM +0100, Chris Wilson wrote:
> > As we defer updating the fence register from set-tiling to the point of
> > use, we need to declare every access through the GTT as either fenced or
> > unfenced.
> > 
> > This patches fixes up a couple of freshly introduced GTT accesses which
> > missed the fence flush.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Actually no, this set_to_gtt is actually just a flush+wait_rendering
> because the shmem pwrite/pread afters does clever in-line clflushing.
> Abuse of interfaces, I know ...

Probably best to leave a note, so the next time I do a grep looking for
use of set-to-gtt without a following fence declaration, I don't submit
another patch. :-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 5/5] drm/i915: Always flush tiling changes before accessing through the GTT

[Chris Wilson]

As we defer updating the fence register from set-tiling to the point of
use, we need to declare every access through the GTT as either fenced or
unfenced.

This patches fixes an old bug in the execbuffer relocation processing
which could conceivably be hit by a pathological userspace.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 2a24d0c..1a0d54f 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -381,7 +381,11 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 		uint32_t __iomem *reloc_entry;
 		void __iomem *reloc_page;
 
-		ret = i915_gem_object_set_to_gtt_domain(obj, 1);
+		ret = i915_gem_object_set_to_gtt_domain(obj, true);
+		if (ret)
+			return ret;
+
+		ret = i915_gem_object_put_fence(obj);
 		if (ret)
 			return ret;
 
-- 
1.7.10

Re: [PATCH 5/5] drm/i915: Always flush tiling changes before accessing through the GTT

[Daniel Vetter]

On Sat, Apr 14, 2012 at 09:55:51AM +0100, Chris Wilson wrote:
> As we defer updating the fence register from set-tiling to the point of
> use, we need to declare every access through the GTT as either fenced or
> unfenced.
> 
> This patches fixes an old bug in the execbuffer relocation processing
> which could conceivably be hit by a pathological userspace.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Indeed, my git history digging got lost and given how we upload buffers I
don't think there's any chance we can hit this, so no i-g-t test
necessary.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 2a24d0c..1a0d54f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -381,7 +381,11 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
>  		uint32_t __iomem *reloc_entry;
>  		void __iomem *reloc_page;
>  
> -		ret = i915_gem_object_set_to_gtt_domain(obj, 1);
> +		ret = i915_gem_object_set_to_gtt_domain(obj, true);
> +		if (ret)
> +			return ret;
> +
> +		ret = i915_gem_object_put_fence(obj);
>  		if (ret)
>  			return ret;
>  
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 5/5] drm/i915: Always flush tiling changes before accessing through the GTT

[Daniel Vetter]

On Sun, Apr 15, 2012 at 12:00:35AM +0200, Daniel Vetter wrote:
> On Sat, Apr 14, 2012 at 09:55:51AM +0100, Chris Wilson wrote:
> > As we defer updating the fence register from set-tiling to the point of
> > use, we need to declare every access through the GTT as either fenced or
> > unfenced.
> > 
> > This patches fixes an old bug in the execbuffer relocation processing
> > which could conceivably be hit by a pathological userspace.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Indeed, my git history digging got lost and given how we upload buffers I
> don't think there's any chance we can hit this, so no i-g-t test
> necessary.
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/5] drm/i915: Suppress EIO during set-to-gtt

[Chris Wilson]

On Sat, 14 Apr 2012 09:55:47 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> If we fail to flush the rendering to an object already bound to the GTT
> because the hardware is edge, all is good!

No coffee yet. s/edge/wedged/

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/5] drm/i915: Suppress EIO during set-to-gtt

[Daniel Vetter]

On Sat, Apr 14, 2012 at 09:55:47AM +0100, Chris Wilson wrote:
> If we fail to flush the rendering to an object already bound to the GTT
> because the hardware is edge, all is good!
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

As already quickly discussed on irc, I'm not too happy about -EIO special
casing in set_to_gtt_domain, set_to_cpu_domain and flush_fence. Imo,
assuming that our hangcheck and wedging code works correctly, we should
bail out of this, the reset code should reset all the gem tracking state
and as long as we dont emit any new requests on the gpu, we should never
need to wait for one and get a -EIO in return.

Reconsidering the -EIO special case in unbind, I'm also not sure any more
that this one's right. In normal operations we should never try to unbind
an active object, which leaves us with the ilk vt-d workaround. That one
should eat the -EIO itself, imo.

Now given how well-tested that code is, I expect bugs. But imo the right
course of action is to make that code testable first before we sprinkle
-EIO handling all over the place. I've planned to resurrect my gpu hangman
this week, and I'm thinking of ways to extend that to test our -EIO/gpu
wedging code.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/5] drm/i915: Suppress EIO during set-to-gtt

[Chris Wilson]

On Wed, 18 Apr 2012 11:03:03 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> Now given how well-tested that code is, I expect bugs. But imo the right
> course of action is to make that code testable first before we sprinkle
> -EIO handling all over the place. I've planned to resurrect my gpu hangman
> this week, and I'm thinking of ways to extend that to test our -EIO/gpu
> wedging code.

I disagree with this approach. I need the driver to robust its own
failures so that we don't lose data. I know from experience that is
currently not.

Looking at the code paths required for CPU access and making them
resilient as possible is a necessary evil, imho.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre