[PATCH 24/40] drm/i915: Defer seqno allocation until actual hardware submission time

[John.C.Harrison@Intel.com]

From: John Harrison <John.C.Harrison@Intel.com>

The seqno value is now only used for the final test for completion of
a request. It is no longer used to track the request through the
software stack. Thus it is no longer necessary to allocate the seqno
immediately with the request. Instead, it can be done lazily and left
until the request is actually sent to the hardware. This is particular
advantageous with a GPU scheduler as the requests can then be
re-ordered between their creation and their hardware submission
without having out of order seqnos.

v2: i915_add_request() can't fail!

Combine with 'drm/i915: Assign seqno at start of exec_final()'
Various bits of code during the execbuf code path need a seqno value
to be assigned to the request. This change makes this assignment
explicit at the start of submission_final() rather than relying on an
auto-generated seqno to have happened already. This is in preparation
for a future patch which changes seqno values to be assigned lazily
(during add_request).

v3: Updated to use locally cached request pointer.

Change-Id: I0d922b84c517611a79fa6c2b9e730d4fe3671d6a
For: VIZ-1587
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  1 +
 drivers/gpu/drm/i915/i915_gem.c            | 21 ++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c           | 13 +++++++++++++
 4 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5b893a6..15dee41 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2225,6 +2225,7 @@ struct drm_i915_gem_request {
 
 	/** GEM sequence number associated with this request. */
 	uint32_t seqno;
+	uint32_t reserved_seqno;
 
 	/* Unique identifier which can be used for trace points & debug */
 	uint32_t uniq;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 99e5b1d0..1fb45c2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2525,6 +2525,9 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
 
 	/* reserve 0 for non-seqno */
 	if (dev_priv->next_seqno == 0) {
+		/* Why is the full re-initialisation required? Is it only for
+		 * hardware semaphores? If so, could skip it in the case where
+		 * semaphores are disabled? */
 		int ret = i915_gem_init_seqno(dev, 0);
 		if (ret)
 			return ret;
@@ -2582,6 +2585,12 @@ void __i915_add_request(struct drm_i915_gem_request *request,
 		WARN(ret, "*_ring_flush_all_caches failed: %d!\n", ret);
 	}
 
+	/* Make the request's seqno 'live': */
+	if(!request->seqno) {
+		request->seqno = request->reserved_seqno;
+		WARN_ON(request->seqno != dev_priv->last_seqno);
+	}
+
 	/* Record the position of the start of the request so that
 	 * should we detect the updated seqno part-way through the
 	 * GPU processing the request, we never over-estimate the
@@ -2830,6 +2839,9 @@ void i915_gem_request_notify(struct intel_engine_cs *ring, bool fence_locked)
 
 	list_for_each_entry_safe(req, req_next, &ring->fence_signal_list, signal_link) {
 		if (!req->cancelled) {
+			/* How can this happen? */
+			WARN_ON(req->seqno == 0);
+
 			if (!i915_seqno_passed(seqno, req->seqno))
 				break;
 		}
@@ -3054,7 +3066,14 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
 	if (req == NULL)
 		return -ENOMEM;
 
-	ret = i915_gem_get_seqno(ring->dev, &req->seqno);
+	/*
+	 * Assign an identifier to track this request through the hardware
+	 * but don't make it live yet. It could change in the future if this
+	 * request gets overtaken. However, it still needs to be allocated
+	 * in advance because the point of submission must not fail and seqno
+	 * allocation can fail.
+	 */
+	ret = i915_gem_get_seqno(ring->dev, &req->reserved_seqno);
 	if (ret)
 		goto err;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0908699..7970958 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1249,6 +1249,19 @@ int i915_gem_ringbuffer_submission_final(struct i915_execbuffer_params *params)
 	/* The mutex must be acquired before calling this function */
 	BUG_ON(!mutex_is_locked(&params->dev->struct_mutex));
 
+	/* Make sure the request's seqno is the latest and greatest: */
+	if(req->reserved_seqno != dev_priv->last_seqno) {
+		ret = i915_gem_get_seqno(ring->dev, &req->reserved_seqno);
+		if (ret)
+			return ret;
+	}
+	/*
+	 * And make it live because some of the execbuff submission code
+	 * requires the seqno to be available up front. */
+	WARN_ON(req->seqno);
+	req->seqno = req->reserved_seqno;
+	WARN_ON(req->seqno != dev_priv->last_seqno);
+
 	ret = intel_ring_reserve_space(req);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 85bef51..d187f9a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -934,6 +934,19 @@ int intel_execlists_submission_final(struct i915_execbuffer_params *params)
 	/* The mutex must be acquired before calling this function */
 	BUG_ON(!mutex_is_locked(&params->dev->struct_mutex));
 
+	/* Make sure the request's seqno is the latest and greatest: */
+	if(req->reserved_seqno != dev_priv->last_seqno) {
+		ret = i915_gem_get_seqno(ring->dev, &req->reserved_seqno);
+		if (ret)
+			return ret;
+	}
+	/*
+	 * And make it live because some of the execbuff submission code
+	 * requires the seqno to be available up front. */
+	WARN_ON(req->seqno);
+	req->seqno = req->reserved_seqno;
+	WARN_ON(req->seqno != dev_priv->last_seqno);
+
 	ret = intel_logical_ring_reserve_space(req);
 	if (ret)
 		return ret;
-- 
1.9.1

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