intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* Pipelined fence fixes
@ 2011-03-18 22:35 Chris Wilson
  2011-03-18 22:35 ` [PATCH 1/4] drm/i915: Track fence setup separately from fenced object lifetime Chris Wilson
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Chris Wilson @ 2011-03-18 22:35 UTC (permalink / raw)
  To: intel-gfx

Spurred on by the tiling corruption caused by "disabling" the pipelined
fencing, I think I finally fixed the GPU hangs plaguing the
implementation. (It's a matter of timing and making sure that is
sufficient serialisation between the CPU and GPU writes to the fence
registers, but not too much...)

This is quite hairy code, but is required to fix the stalls introduced
to prevent the tiling corruption.
-Chris

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/4] drm/i915: Track fence setup separately from fenced object lifetime
  2011-03-18 22:35 Pipelined fence fixes Chris Wilson
@ 2011-03-18 22:35 ` Chris Wilson
  2011-03-19 22:35   ` Daniel Vetter
  2011-03-18 22:35 ` [PATCH 2/4] drm/i915: Invalidate fenced read domains upon flush Chris Wilson
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2011-03-18 22:35 UTC (permalink / raw)
  To: intel-gfx; +Cc: Andy Whitcroft

This fixes a bookkeeping error causing an OOPS whilst waiting for an
object to finish using a fence. Now we can simply wait for the fence to
be written independent of the objects currently inhabiting it (past,
present and future).

Cc: Andy Whitcroft <apw@canonical.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |    2 +-
 drivers/gpu/drm/i915/i915_gem.c |   50 ++++++++++++++++++---------------------
 2 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4496505..bcbcb53 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -129,6 +129,7 @@ struct drm_i915_master_private {
 struct drm_i915_fence_reg {
 	struct list_head lru_list;
 	struct drm_i915_gem_object *obj;
+	struct intel_ring_buffer *setup_ring;
 	uint32_t setup_seqno;
 };
 
@@ -818,7 +819,6 @@ struct drm_i915_gem_object {
 
 	/** Breadcrumb of last fenced GPU access to the buffer. */
 	uint32_t last_fenced_seqno;
-	struct intel_ring_buffer *last_fenced_ring;
 
 	/** Current tiling stride for the object, if it's tiled. */
 	uint32_t stride;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c5dfb59..ef84c13 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1640,7 +1640,6 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
 		BUG_ON(obj->fence_reg == I915_FENCE_REG_NONE);
 
 		obj->last_fenced_seqno = seqno;
-		obj->last_fenced_ring = ring;
 
 		reg = &dev_priv->fence_regs[obj->fence_reg];
 		list_move_tail(&reg->lru_list, &dev_priv->mm.fence_list);
@@ -1684,6 +1683,8 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 	i915_gem_object_move_off_active(obj);
 	obj->fenced_gpu_access = false;
 
+	obj->last_fenced_seqno = 0;
+
 	obj->active = 0;
 	obj->pending_gpu_write = false;
 	drm_gem_object_unreference(&obj->base);
@@ -1849,7 +1850,6 @@ static void i915_gem_reset_fences(struct drm_device *dev)
 		reg->obj->fence_reg = I915_FENCE_REG_NONE;
 		reg->obj->fenced_gpu_access = false;
 		reg->obj->last_fenced_seqno = 0;
-		reg->obj->last_fenced_ring = NULL;
 		i915_gem_clear_fence_reg(dev, reg);
 	}
 }
@@ -2450,7 +2450,7 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj,
 
 	if (obj->fenced_gpu_access) {
 		if (obj->base.write_domain & I915_GEM_GPU_DOMAINS) {
-			ret = i915_gem_flush_ring(obj->last_fenced_ring,
+			ret = i915_gem_flush_ring(obj->ring,
 						  0, obj->base.write_domain);
 			if (ret)
 				return ret;
@@ -2459,17 +2459,15 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj,
 		obj->fenced_gpu_access = false;
 	}
 
-	if (obj->last_fenced_seqno && pipelined != obj->last_fenced_ring) {
-		if (!ring_passed_seqno(obj->last_fenced_ring,
-				       obj->last_fenced_seqno)) {
-			ret = i915_wait_request(obj->last_fenced_ring,
+	if (obj->last_fenced_seqno && pipelined != obj->ring) {
+		if (!ring_passed_seqno(obj->ring, obj->last_fenced_seqno)) {
+			ret = i915_wait_request(obj->ring,
 						obj->last_fenced_seqno);
 			if (ret)
 				return ret;
 		}
 
 		obj->last_fenced_seqno = 0;
-		obj->last_fenced_ring = NULL;
 	}
 
 	/* Ensure that all CPU reads are completed before installing a fence
@@ -2536,8 +2534,8 @@ i915_find_fence_reg(struct drm_device *dev,
 			first = reg;
 
 		if (!pipelined ||
-		    !reg->obj->last_fenced_ring ||
-		    reg->obj->last_fenced_ring == pipelined) {
+		    !reg->obj->last_fenced_seqno ||
+		    reg->obj->ring == pipelined) {
 			avail = reg;
 			break;
 		}
@@ -2589,30 +2587,23 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 			if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
 				pipelined = NULL;
 
-			if (pipelined) {
-				reg->setup_seqno =
-					i915_gem_next_request_seqno(pipelined);
-				obj->last_fenced_seqno = reg->setup_seqno;
-				obj->last_fenced_ring = pipelined;
-			}
-
 			goto update;
 		}
 
 		if (!pipelined) {
 			if (reg->setup_seqno) {
-				if (!ring_passed_seqno(obj->last_fenced_ring,
+				if (!ring_passed_seqno(reg->setup_ring,
 						       reg->setup_seqno)) {
-					ret = i915_wait_request(obj->last_fenced_ring,
+					ret = i915_wait_request(reg->setup_ring,
 								reg->setup_seqno);
 					if (ret)
 						return ret;
 				}
 
 				reg->setup_seqno = 0;
+				reg->setup_ring = NULL;
 			}
-		} else if (obj->last_fenced_ring &&
-			   obj->last_fenced_ring != pipelined) {
+		} else if (obj->last_fenced_seqno && obj->ring != pipelined) {
 			ret = i915_gem_object_flush_fence(obj, pipelined);
 			if (ret)
 				return ret;
@@ -2647,9 +2638,13 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 			pipelined = NULL;
 
 		old->fence_reg = I915_FENCE_REG_NONE;
-		old->last_fenced_ring = pipelined;
-		old->last_fenced_seqno =
-			pipelined ? i915_gem_next_request_seqno(pipelined) : 0;
+		old->last_fenced_seqno = 0;
+		if (pipelined) {
+			old->last_fenced_seqno =
+				i915_gem_next_request_seqno(pipelined);
+			i915_gem_object_move_to_active(old, pipelined,
+						       old->last_fenced_seqno);
+		}
 
 		drm_gem_object_unreference(&old->base);
 	} else if (obj->last_fenced_seqno == 0)
@@ -2658,13 +2653,13 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 	reg->obj = obj;
 	list_move_tail(&reg->lru_list, &dev_priv->mm.fence_list);
 	obj->fence_reg = reg - dev_priv->fence_regs;
-	obj->last_fenced_ring = pipelined;
 
+update:
 	reg->setup_seqno =
 		pipelined ? i915_gem_next_request_seqno(pipelined) : 0;
-	obj->last_fenced_seqno = reg->setup_seqno;
+	reg->setup_ring = pipelined;
 
-update:
+	obj->last_fenced_seqno = reg->setup_seqno;
 	obj->tiling_changed = false;
 	switch (INTEL_INFO(dev)->gen) {
 	case 6:
@@ -2721,6 +2716,7 @@ i915_gem_clear_fence_reg(struct drm_device *dev,
 	list_del_init(&reg->lru_list);
 	reg->obj = NULL;
 	reg->setup_seqno = 0;
+	reg->setup_ring = NULL;
 }
 
 /**
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/4] drm/i915: Invalidate fenced read domains upon flush
  2011-03-18 22:35 Pipelined fence fixes Chris Wilson
  2011-03-18 22:35 ` [PATCH 1/4] drm/i915: Track fence setup separately from fenced object lifetime Chris Wilson
@ 2011-03-18 22:35 ` Chris Wilson
  2011-03-19 22:49   ` Daniel Vetter
  2011-03-18 22:35 ` [PATCH 3/4] drm/i915: Cleanup handling of last_fenced_seqno Chris Wilson
  2011-03-18 22:35 ` [PATCH 4/4] drm/i915: Prevent fence-reuse stalls Chris Wilson
  3 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2011-03-18 22:35 UTC (permalink / raw)
  To: intel-gfx

Whenever we finish reading an object through a fence, for safety we
clear any GPU read domain and so invalidate any TLBs associated with
the fenced region upon its next use.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ef84c13..5201f82 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2456,6 +2456,8 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj,
 				return ret;
 		}
 
+		 /* Invalidate the GPU TLBs for any future reads */
+		obj->base.read_domains &= ~I915_GEM_GPU_DOMAINS;
 		obj->fenced_gpu_access = false;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 7ff7f93..8367fc9 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -172,9 +172,8 @@ i915_gem_object_set_to_gpu_domain(struct drm_i915_gem_object *obj,
 	 * write domain
 	 */
 	if (obj->base.write_domain &&
-	    (((obj->base.write_domain != obj->base.pending_read_domains ||
-	       obj->ring != ring)) ||
-	     (obj->fenced_gpu_access && !obj->pending_fenced_gpu_access))) {
+	    (obj->base.write_domain != obj->base.pending_read_domains ||
+	     obj->ring != ring)) {
 		flush_domains |= obj->base.write_domain;
 		invalidate_domains |=
 			obj->base.pending_read_domains & ~obj->base.write_domain;
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/4] drm/i915: Cleanup handling of last_fenced_seqno
  2011-03-18 22:35 Pipelined fence fixes Chris Wilson
  2011-03-18 22:35 ` [PATCH 1/4] drm/i915: Track fence setup separately from fenced object lifetime Chris Wilson
  2011-03-18 22:35 ` [PATCH 2/4] drm/i915: Invalidate fenced read domains upon flush Chris Wilson
@ 2011-03-18 22:35 ` Chris Wilson
  2011-03-19 22:55   ` Daniel Vetter
  2011-03-18 22:35 ` [PATCH 4/4] drm/i915: Prevent fence-reuse stalls Chris Wilson
  3 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2011-03-18 22:35 UTC (permalink / raw)
  To: intel-gfx; +Cc: Andy Whitcroft

Cc: Andy Whitcroft <apw@canonical.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   29 +++++++++++++++++------------
 1 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5201f82..2dbf8f7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2461,13 +2461,15 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj,
 		obj->fenced_gpu_access = false;
 	}
 
+	if (obj->last_fenced_seqno &&
+	    ring_passed_seqno(obj->ring, obj->last_fenced_seqno))
+		obj->last_fenced_seqno = 0;
+
 	if (obj->last_fenced_seqno && pipelined != obj->ring) {
-		if (!ring_passed_seqno(obj->ring, obj->last_fenced_seqno)) {
-			ret = i915_wait_request(obj->ring,
-						obj->last_fenced_seqno);
-			if (ret)
-				return ret;
-		}
+		ret = i915_wait_request(obj->ring,
+					obj->last_fenced_seqno);
+		if (ret)
+			return ret;
 
 		obj->last_fenced_seqno = 0;
 	}
@@ -2586,9 +2588,6 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 			if (ret)
 				return ret;
 
-			if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
-				pipelined = NULL;
-
 			goto update;
 		}
 
@@ -2606,9 +2605,12 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 				reg->setup_ring = NULL;
 			}
 		} else if (obj->last_fenced_seqno && obj->ring != pipelined) {
-			ret = i915_gem_object_flush_fence(obj, pipelined);
+			ret = i915_wait_request(obj->ring,
+						obj->last_fenced_seqno);
 			if (ret)
 				return ret;
+
+			obj->last_fenced_seqno = 0;
 		}
 
 		return 0;
@@ -2648,15 +2650,18 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 						       old->last_fenced_seqno);
 		}
 
+		obj->last_fenced_seqno = old->last_fenced_seqno;
 		drm_gem_object_unreference(&old->base);
-	} else if (obj->last_fenced_seqno == 0)
-		pipelined = NULL;
+	}
 
 	reg->obj = obj;
 	list_move_tail(&reg->lru_list, &dev_priv->mm.fence_list);
 	obj->fence_reg = reg - dev_priv->fence_regs;
 
 update:
+	if (obj->last_fenced_seqno == 0)
+		pipelined = NULL;
+
 	reg->setup_seqno =
 		pipelined ? i915_gem_next_request_seqno(pipelined) : 0;
 	reg->setup_ring = pipelined;
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/4] drm/i915: Prevent fence-reuse stalls
  2011-03-18 22:35 Pipelined fence fixes Chris Wilson
                   ` (2 preceding siblings ...)
  2011-03-18 22:35 ` [PATCH 3/4] drm/i915: Cleanup handling of last_fenced_seqno Chris Wilson
@ 2011-03-18 22:35 ` Chris Wilson
  2011-03-19 22:57   ` Daniel Vetter
  3 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2011-03-18 22:35 UTC (permalink / raw)
  To: intel-gfx; +Cc: Andy Whitcroft

With the last bug preventing fence pipelining fixed (or at least the
last known bug), re-enable pipelining to avoid stalling on fence
acquisition between batches.

Cc: Andy Whitcroft <apw@canonical.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2dbf8f7..78dba7a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2575,9 +2575,6 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 	struct drm_i915_fence_reg *reg;
 	int ret;
 
-	/* XXX disable pipelining. There are bugs. Shocking. */
-	pipelined = NULL;
-
 	/* Just update our place in the LRU if our fence is getting reused. */
 	if (obj->fence_reg != I915_FENCE_REG_NONE) {
 		reg = &dev_priv->fence_regs[obj->fence_reg];
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] drm/i915: Track fence setup separately from fenced object lifetime
  2011-03-18 22:35 ` [PATCH 1/4] drm/i915: Track fence setup separately from fenced object lifetime Chris Wilson
@ 2011-03-19 22:35   ` Daniel Vetter
  2011-03-19 22:42     ` Chris Wilson
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Vetter @ 2011-03-19 22:35 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Andy Whitcroft, intel-gfx

On Fri, Mar 18, 2011 at 10:35:16PM +0000, Chris Wilson wrote:
> This fixes a bookkeeping error causing an OOPS whilst waiting for an
> object to finish using a fence. Now we can simply wait for the fence to
> be written independent of the objects currently inhabiting it (past,
> present and future).

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Ah, this patch addresses my update label comment ;) Otherwise I think
tracking fences more as independent objects definitely results in clearer
semantics. A tiny nitpick below.

> @@ -2647,9 +2638,13 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
>  			pipelined = NULL;
>  
>  		old->fence_reg = I915_FENCE_REG_NONE;
> -		old->last_fenced_ring = pipelined;
> -		old->last_fenced_seqno =
> -			pipelined ? i915_gem_next_request_seqno(pipelined) : 0;
> +		old->last_fenced_seqno = 0;
> +		if (pipelined) {
> +			old->last_fenced_seqno =
> +				i915_gem_next_request_seqno(pipelined);
> +			i915_gem_object_move_to_active(old, pipelined,
> +						       old->last_fenced_seqno);
> +		}
>  
>  		drm_gem_object_unreference(&old->base);
>  	} else if (obj->last_fenced_seqno == 0)

This special case of the last_fenced_seqno tracking slightly annoys me. I
_think_ the flush_ring in flush_fence does already take care of this (but
I'm not too shure, and this is definitely the safe option).
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] drm/i915: Track fence setup separately from fenced object lifetime
  2011-03-19 22:35   ` Daniel Vetter
@ 2011-03-19 22:42     ` Chris Wilson
  2011-03-19 23:11       ` Daniel Vetter
  0 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2011-03-19 22:42 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Andy Whitcroft, intel-gfx

On Sat, 19 Mar 2011 23:35:55 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > @@ -2647,9 +2638,13 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
> >  			pipelined = NULL;
> >  
> >  		old->fence_reg = I915_FENCE_REG_NONE;
> > -		old->last_fenced_ring = pipelined;
> > -		old->last_fenced_seqno =
> > -			pipelined ? i915_gem_next_request_seqno(pipelined) : 0;
> > +		old->last_fenced_seqno = 0;
> > +		if (pipelined) {
> > +			old->last_fenced_seqno =
> > +				i915_gem_next_request_seqno(pipelined);
> > +			i915_gem_object_move_to_active(old, pipelined,
> > +						       old->last_fenced_seqno);
> > +		}
> >  
> >  		drm_gem_object_unreference(&old->base);
> >  	} else if (obj->last_fenced_seqno == 0)
> 
> This special case of the last_fenced_seqno tracking slightly annoys me. I
> _think_ the flush_ring in flush_fence does already take care of this (but
> I'm not too shure, and this is definitely the safe option).

It's meant to be an optimisation where we note that even though this might
be a pipelined request, the object does not have any outstanding GPU
fenced access and so we can write the fence register immediately.

Worth a comment after cleaning it up (see the later patch).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] drm/i915: Invalidate fenced read domains upon flush
  2011-03-18 22:35 ` [PATCH 2/4] drm/i915: Invalidate fenced read domains upon flush Chris Wilson
@ 2011-03-19 22:49   ` Daniel Vetter
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2011-03-19 22:49 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Mar 18, 2011 at 10:35:17PM +0000, Chris Wilson wrote:
> Whenever we finish reading an object through a fence, for safety we
> clear any GPU read domain and so invalidate any TLBs associated with
> the fenced region upon its next use.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] drm/i915: Cleanup handling of last_fenced_seqno
  2011-03-18 22:35 ` [PATCH 3/4] drm/i915: Cleanup handling of last_fenced_seqno Chris Wilson
@ 2011-03-19 22:55   ` Daniel Vetter
  2011-03-19 23:09     ` Chris Wilson
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Vetter @ 2011-03-19 22:55 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Andy Whitcroft, intel-gfx

A few nitpicks below.

> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5201f82..2dbf8f7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2461,13 +2461,15 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj,
>  		obj->fenced_gpu_access = false;
>  	}
>  
> +	if (obj->last_fenced_seqno &&
> +	    ring_passed_seqno(obj->ring, obj->last_fenced_seqno))
> +		obj->last_fenced_seqno = 0;

This only avoids running the request retiring logic. Without this there are a
few more things to simplify, I think:

>  	if (obj->last_fenced_seqno && pipelined != obj->ring) {
> -		if (!ring_passed_seqno(obj->ring, obj->last_fenced_seqno)) {
> -			ret = i915_wait_request(obj->ring,
> -						obj->last_fenced_seqno);
> -			if (ret)
> -				return ret;
> -		}
> +		ret = i915_wait_request(obj->ring,
> +					obj->last_fenced_seqno);
> +		if (ret)
> +			return ret;
>  
>  		obj->last_fenced_seqno = 0;

Can't we move that (and the other copies) to move_off_active?

> @@ -2648,15 +2650,18 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
>  						       old->last_fenced_seqno);
>  		}

Above here is the imo unnecessary clause mentioned in a previous patch.

>  
> +		obj->last_fenced_seqno = old->last_fenced_seqno;
>  		drm_gem_object_unreference(&old->base);
> -	} else if (obj->last_fenced_seqno == 0)
> -		pipelined = NULL;
> +	}
>  
>  	reg->obj = obj;
>  	list_move_tail(&reg->lru_list, &dev_priv->mm.fence_list);
>  	obj->fence_reg = reg - dev_priv->fence_regs;
>  
>  update:
> +	if (obj->last_fenced_seqno == 0)
> +		pipelined = NULL;
> +
>  	reg->setup_seqno =
>  		pipelined ? i915_gem_next_request_seqno(pipelined) : 0;
>  	reg->setup_ring = pipelined;
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] drm/i915: Prevent fence-reuse stalls
  2011-03-18 22:35 ` [PATCH 4/4] drm/i915: Prevent fence-reuse stalls Chris Wilson
@ 2011-03-19 22:57   ` Daniel Vetter
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2011-03-19 22:57 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Andy Whitcroft, intel-gfx

On Fri, Mar 18, 2011 at 10:35:19PM +0000, Chris Wilson wrote:
> With the last bug preventing fence pipelining fixed (or at least the
> last known bug), re-enable pipelining to avoid stalling on fence
> acquisition between batches.

\o/ ... the cleaned-up get_fence code is definitely much less scary, so
I'm all for a new round lets-try-to-upset-gen3-gpus.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] drm/i915: Cleanup handling of last_fenced_seqno
  2011-03-19 22:55   ` Daniel Vetter
@ 2011-03-19 23:09     ` Chris Wilson
  0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2011-03-19 23:09 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Andy Whitcroft, intel-gfx

On Sat, 19 Mar 2011 23:55:11 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> A few nitpicks below.
> 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 5201f82..2dbf8f7 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2461,13 +2461,15 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj,
> >  		obj->fenced_gpu_access = false;
> >  	}
> >  
> > +	if (obj->last_fenced_seqno &&
> > +	    ring_passed_seqno(obj->ring, obj->last_fenced_seqno))
> > +		obj->last_fenced_seqno = 0;
> 
> This only avoids running the request retiring logic. Without this there are a
> few more things to simplify, I think:

Not strictly. We only do i915_wait_request() if changing rings, yet we
want to check if the object has any outstanding fences for an
optimisation later (writing the fence register immediately whenever
possible).
 
> >  	if (obj->last_fenced_seqno && pipelined != obj->ring) {
> > -		if (!ring_passed_seqno(obj->ring, obj->last_fenced_seqno)) {
> > -			ret = i915_wait_request(obj->ring,
> > -						obj->last_fenced_seqno);
> > -			if (ret)
> > -				return ret;
> > -		}
> > +		ret = i915_wait_request(obj->ring,
> > +					obj->last_fenced_seqno);
> > +		if (ret)
> > +			return ret;
> >  
> >  		obj->last_fenced_seqno = 0;
> 
> Can't we move that (and the other copies) to move_off_active?

No, because the obj may remain active even after the wait.

> 
> > @@ -2648,15 +2650,18 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
> >  						       old->last_fenced_seqno);
> >  		}
> 
> Above here is the imo unnecessary clause mentioned in a previous patch.

ECONTEXT?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] drm/i915: Track fence setup separately from fenced object lifetime
  2011-03-19 22:42     ` Chris Wilson
@ 2011-03-19 23:11       ` Daniel Vetter
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2011-03-19 23:11 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Andy Whitcroft, intel-gfx

On Sat, Mar 19, 2011 at 10:42:10PM +0000, Chris Wilson wrote:
> It's meant to be an optimisation where we note that even though this might
> be a pipelined request, the object does not have any outstanding GPU
> fenced access and so we can write the fence register immediately.

Some more thinking cleared stuff up here: last_fenced_seqno can retire
much earlier that last_seqno, so we have to ad-hoc track that for
efficiency.

I still like a

	obj->last_fenced_seqno = 0;

in move_off_active, just to ensure that we don't have terribly old seqnos
lying around.

> Worth a comment after cleaning it up (see the later patch).

So scrap these review comments from me for that patch and perhaps add a
small comment instead as to why we have to track this by hand.

> -Chris

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2011-03-19 23:11 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-18 22:35 Pipelined fence fixes Chris Wilson
2011-03-18 22:35 ` [PATCH 1/4] drm/i915: Track fence setup separately from fenced object lifetime Chris Wilson
2011-03-19 22:35   ` Daniel Vetter
2011-03-19 22:42     ` Chris Wilson
2011-03-19 23:11       ` Daniel Vetter
2011-03-18 22:35 ` [PATCH 2/4] drm/i915: Invalidate fenced read domains upon flush Chris Wilson
2011-03-19 22:49   ` Daniel Vetter
2011-03-18 22:35 ` [PATCH 3/4] drm/i915: Cleanup handling of last_fenced_seqno Chris Wilson
2011-03-19 22:55   ` Daniel Vetter
2011-03-19 23:09     ` Chris Wilson
2011-03-18 22:35 ` [PATCH 4/4] drm/i915: Prevent fence-reuse stalls Chris Wilson
2011-03-19 22:57   ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).