diff mbox

[07/21] drm/i915: Track fence setup separately from fenced object lifetime

Message ID 1302945465-32115-8-git-send-email-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson April 16, 2011, 9:17 a.m. UTC 
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).

A large amount of the change is to delay updating the information about
the fence on bo until after we successfully write, or queue the write to,
the register. This avoids the complication of undoing a partial change
should we fail in pipelining the change.

Cc: Andy Whitcroft <apw@canonical.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/i915_gem.c |  155 ++++++++++++++++++++-------------------
 2 files changed, 82 insertions(+), 74 deletions(-)

Comments

Daniel Vetter April 16, 2011, 1:20 p.m. UTC | #1 
On Sat, Apr 16, 2011 at 10:17:31AM +0100, 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).
> 
> A large amount of the change is to delay updating the information about
> the fence on bo until after we successfully write, or queue the write to,
> the register. This avoids the complication of undoing a partial change
> should we fail in pipelining the change.

Reordering the accounting and consistently using setup_seqno/ring to track
fence changes looks correct.

I still see a few minor issues with the pipelined fencing code that should
be addressed in later patches:
- setup_seqno/ring is not being cleanup up anywhere and might go stale.
  The hack I've used in one of my stabs at this was to loop over all
  fences at the end of retire_requests.
- The semantics of last_fenced_seqno/ring are a bit hairy. I think ideal
  would be if the following would always hold:
  * an object may never change it's ring if last_fenced_seqno != 0. A call
    to flush fence is required to change the ring.
  * if last_fenced_seqno != 0, then always last_rendering_seqno != 0 (i.e.
    if the execbuffer fails we still move all objects with pipelined
    fencing setups to the active list).
  These two combined should give:
    last_fenced_ring != NULL implies last_fenced_ring == last_rendering_ring
  allowing us to simplify a few things.
- The obj->fenced_gpu_access optimization got killed in a previous patch.
  We could resurrect that by clearing it in process_flushing_list. With
  that change (and perhaps some movement of the assignement of
  fenced_gpu_access) we could consolidate the last_fenced_seqno/ring
  tracking.

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

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b22f11..0296967 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;
 };
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8c835de..b9515ac 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1730,6 +1730,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);
@@ -1895,7 +1897,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);
 	}
 }
@@ -2525,7 +2526,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;
@@ -2536,17 +2537,22 @@  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->last_fenced_ring, obj->last_fenced_seqno))
+		obj->last_fenced_seqno = 0;
+
 	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,
-						obj->last_fenced_seqno);
-			if (ret)
-				return ret;
-		}
+		ret = i915_wait_request(obj->last_fenced_ring,
+					obj->last_fenced_seqno);
+		if (ret)
+			return ret;
 
+		/* Since last_fence_seqno can retire much earlier than
+		 * last_rendering_seqno, we track that here for efficiency.
+		 * (With a catch-all in move_to_inactive() to prevent very
+		 * old seqno from lying around.)
+		 */
 		obj->last_fenced_seqno = 0;
-		obj->last_fenced_ring = NULL;
 	}
 
 	/* Ensure that all CPU reads are completed before installing a fence
@@ -2613,7 +2619,7 @@  i915_find_fence_reg(struct drm_device *dev,
 			first = reg;
 
 		if (!pipelined ||
-		    !reg->obj->last_fenced_ring ||
+		    !reg->obj->last_fenced_seqno ||
 		    reg->obj->last_fenced_ring == pipelined) {
 			avail = reg;
 			break;
@@ -2630,7 +2636,6 @@  i915_find_fence_reg(struct drm_device *dev,
  * i915_gem_object_get_fence - set up a fence reg for an object
  * @obj: object to map through a fence reg
  * @pipelined: ring on which to queue the change, or NULL for CPU access
- * @interruptible: must we wait uninterruptibly for the register to retire?
  *
  * When mapping objects through the GTT, userspace wants to be able to write
  * to them without having to worry about swizzling if the object is tiled.
@@ -2638,6 +2643,10 @@  i915_find_fence_reg(struct drm_device *dev,
  * This function walks the fence regs looking for a free one for @obj,
  * stealing one if it can't find any.
  *
+ * Note: if two fence registers point to the same or overlapping memory region
+ * the results are undefined. This is even more fun with asynchronous updates
+ * via the GPU!
+ *
  * It then sets up the reg based on the object's properties: address, pitch
  * and tiling format.
  */
@@ -2648,6 +2657,7 @@  i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_fence_reg *reg;
+	struct drm_i915_gem_object *old = NULL;
 	int regnum;
 	int ret;
 
@@ -2657,45 +2667,21 @@  i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 	/* 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];
-		list_move_tail(&reg->lru_list, &dev_priv->mm.fence_list);
-
-		if (obj->tiling_changed) {
-			ret = i915_gem_object_flush_fence(obj, pipelined);
-			if (ret)
-				return ret;
-
-			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;
-			}
 
+		if (obj->tiling_changed)
 			goto update;
-		}
-
-		if (!pipelined) {
-			if (reg->setup_seqno) {
-				if (!ring_passed_seqno(obj->last_fenced_ring,
-						       reg->setup_seqno)) {
-					ret = i915_wait_request(obj->last_fenced_ring,
-								reg->setup_seqno);
-					if (ret)
-						return ret;
-				}
 
-				reg->setup_seqno = 0;
-			}
-		} else if (obj->last_fenced_ring &&
-			   obj->last_fenced_ring != pipelined) {
-			ret = i915_gem_object_flush_fence(obj, pipelined);
+		if (reg->setup_seqno && pipelined != reg->setup_ring) {
+			ret = i915_wait_request(reg->setup_ring,
+						reg->setup_seqno);
 			if (ret)
 				return ret;
+
+			reg->setup_ring = 0;
+			reg->setup_seqno = 0;
 		}
 
+		list_move_tail(&reg->lru_list, &dev_priv->mm.fence_list);
 		return 0;
 	}
 
@@ -2703,47 +2689,43 @@  i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 	if (reg == NULL)
 		return -ENOSPC;
 
-	ret = i915_gem_object_flush_fence(obj, pipelined);
-	if (ret)
-		return ret;
-
-	if (reg->obj) {
-		struct drm_i915_gem_object *old = reg->obj;
-
+	if ((old = reg->obj)) {
 		drm_gem_object_reference(&old->base);
 
 		if (old->tiling_mode)
 			i915_gem_release_mmap(old);
 
-		ret = i915_gem_object_flush_fence(old, pipelined);
-		if (ret) {
-			drm_gem_object_unreference(&old->base);
-			return ret;
-		}
+		ret = i915_gem_object_flush_fence(old, NULL); //pipelined);
+		if (ret)
+			goto err;
 
-		if (old->last_fenced_seqno == 0 && obj->last_fenced_seqno == 0)
-			pipelined = NULL;
+		/* Mark the fence register as in-use if pipelined */
+		reg->setup_ring = old->last_fenced_ring;
+		reg->setup_seqno = old->last_fenced_seqno;
+	}
 
-		old->fence_reg = I915_FENCE_REG_NONE;
-		old->last_fenced_ring = pipelined;
-		old->last_fenced_seqno =
-			pipelined ? i915_gem_next_request_seqno(pipelined) : 0;
+update:
+	ret = i915_gem_object_flush_fence(obj, pipelined);
+	if (ret)
+		goto err;
 
-		drm_gem_object_unreference(&old->base);
-	} else if (obj->last_fenced_seqno == 0)
-		pipelined = NULL;
+	if (reg->setup_seqno && pipelined != reg->setup_ring) {
+		ret = i915_wait_request(reg->setup_ring,
+					reg->setup_seqno);
+		if (ret)
+			goto err;
 
-	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;
+		reg->setup_ring = 0;
+		reg->setup_seqno = 0;
+	}
 
-	reg->setup_seqno =
-		pipelined ? i915_gem_next_request_seqno(pipelined) : 0;
-	obj->last_fenced_seqno = reg->setup_seqno;
+	/* If we had a pipelined request, but there is no pending GPU access or
+	 * update to a fence register for this memory region, we can write
+	 * the new fence register immediately.
+	 */
+	if (obj->last_fenced_seqno == 0 && reg->setup_seqno == 0)
+		pipelined = NULL;
 
-update:
-	obj->tiling_changed = false;
 	regnum = reg - dev_priv->fence_regs;
 	switch (INTEL_INFO(dev)->gen) {
 	case 6:
@@ -2760,7 +2742,31 @@  update:
 		ret = i830_write_fence_reg(obj, pipelined, regnum);
 		break;
 	}
+	if (ret)
+		goto err;
+
+	if (pipelined) {
+		reg->setup_seqno = i915_gem_next_request_seqno(pipelined);
+		reg->setup_ring = pipelined;
+		if (old) {
+			old->last_fenced_ring = pipelined;
+			old->last_fenced_seqno = reg->setup_seqno;
+		}
+	}
+
+	if (old) {
+		old->fence_reg = I915_FENCE_REG_NONE;
+		drm_gem_object_unreference(&old->base);
+	}
+
+	list_move_tail(&reg->lru_list, &dev_priv->mm.fence_list);
+	reg->obj = obj;
+	obj->fence_reg = regnum;
+	obj->tiling_changed = false;
+	return 0;
 
+err:
+	drm_gem_object_unreference(&old->base);
 	return ret;
 }
 
@@ -2799,6 +2805,7 @@  i915_gem_clear_fence_reg(struct drm_device *dev,
 
 	list_del_init(&reg->lru_list);
 	reg->obj = NULL;
+	reg->setup_ring = 0;
 	reg->setup_seqno = 0;
 }