diff mbox

[RFC,25/25] drm/i915: Defer seqno allocation until actual hardware submission time

Message ID 1412941293-6424-1-git-send-email-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

John Harrison Oct. 10, 2014, 11:41 a.m. UTC 
From: John Harrison <John.C.Harrison@Intel.com>

For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
 drivers/gpu/drm/i915/i915_drv.h         |    5 ++++-
 drivers/gpu/drm/i915/i915_gem.c         |   28 +++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_lrc.c        |   10 ++++------
 drivers/gpu/drm/i915/intel_ringbuffer.c |   10 ++++------
 4 files changed, 39 insertions(+), 14 deletions(-)

Comments

Daniel Vetter Oct. 19, 2014, 2:17 p.m. UTC | #1 
On Fri, Oct 10, 2014 at 12:41:33PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@Intel.com

Now I'm confused ... patch 3 made it sound like having the request and the
seqno allocated at different points is a really fragile idea? Or is this
now all save with everyone using struct request? Please elaborate.

I think the idea is solid, since with the scheduler we'll probably want to
allocate the seqno even later (to avoid having to deal with out-of-order
seqnos).
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |    5 ++++-
>  drivers/gpu/drm/i915/i915_gem.c         |   28 +++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_lrc.c        |   10 ++++------
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   10 ++++------
>  4 files changed, 39 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e46c78c..d797975 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1979,6 +1979,9 @@ static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
>  	if (req->complete)
>  		return true;
>  
> +	if (req->seqno == 0)
> +		return false;
> +
>  	i915_gem_complete_requests_ring(req->ring, lazy_coherency);
>  
>  	return req->complete;
> @@ -2482,7 +2485,7 @@ i915_seqno_passed(uint32_t seq1, uint32_t seq2)
>  	return (int32_t)(seq1 - seq2) >= 0;
>  }
>  
> -int __must_check i915_gem_get_seqno(struct drm_device *dev, u32 *seqno);
> +int __must_check i915_gem_prepare_next_seqno(struct drm_device *dev);
>  int __must_check i915_gem_set_seqno(struct drm_device *dev, u32 seqno);
>  int __must_check i915_gem_object_get_fence(struct drm_i915_gem_object *obj);
>  int __must_check i915_gem_object_put_fence(struct drm_i915_gem_object *obj);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 260ef47..7db84b2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2310,12 +2310,15 @@ int i915_gem_set_seqno(struct drm_device *dev, u32 seqno)
>  }
>  
>  int
> -i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
> +i915_gem_prepare_next_seqno(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	/* 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;
> @@ -2323,6 +2326,24 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
>  		dev_priv->next_seqno = 1;
>  	}
>  
> +	return 0;
> +}
> +
> +static int
> +i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* reserve 0 for non-seqno */
> +	if (dev_priv->next_seqno == 0) {
> +		/* Should never get here! Must always call 'prepare_next' in
> +		 * advance. This code is called during request submission.
> +		 * Trying to wrap the seqno and the implicit idle() calls that
> +		 * the wrap code makes are a bad idea at this point! */
> +		DRM_ERROR("Need to wrap seqno at inopportune moment!\n");
> +		return -EBUSY;
> +	}
> +
>  	*seqno = dev_priv->last_seqno = dev_priv->next_seqno++;
>  	return 0;
>  }
> @@ -2366,6 +2387,11 @@ int __i915_add_request(struct intel_engine_cs *ring,
>  			return ret;
>  	}
>  
> +	/* Assign an identifier to track this request through the hardware: */
> +	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
> +	if (ret)
> +		return ret;
> +
>  	/* 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
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 5a75eb5..e7d4d20 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -802,6 +802,10 @@ static int logical_ring_alloc_request(struct intel_engine_cs *ring,
>  	if (ring->outstanding_lazy_request)
>  		return 0;
>  
> +	ret = i915_gem_prepare_next_seqno(ring->dev);
> +	if (ret)
> +		return ret;
> +
>  	request = kzalloc(sizeof(*request), GFP_KERNEL);
>  	if (request == NULL)
>  		return -ENOMEM;
> @@ -809,12 +813,6 @@ static int logical_ring_alloc_request(struct intel_engine_cs *ring,
>  	kref_init(&request->ref);
>  	request->ring = ring;
>  
> -	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
> -	if (ret) {
> -		kfree(request);
> -		return ret;
> -	}
> -
>  	/* Hold a reference to the context this request belongs to
>  	 * (we will need it when the time comes to emit/retire the
>  	 * request).
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 0f2719d..6a2f25d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2017,6 +2017,10 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
>  	if (ring->outstanding_lazy_request)
>  		return 0;
>  
> +	ret = i915_gem_prepare_next_seqno(ring->dev);
> +	if (ret)
> +		return ret;
> +
>  	request = kzalloc(sizeof(*request), GFP_KERNEL);
>  	if (request == NULL)
>  		return -ENOMEM;
> @@ -2024,12 +2028,6 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
>  	kref_init(&request->ref);
>  	request->ring = ring;
>  
> -	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
> -	if (ret) {
> -		kfree(request);
> -		return ret;
> -	}
> -
>  	ring->outstanding_lazy_request = request;
>  	return 0;
>  }
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e46c78c..d797975 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1979,6 +1979,9 @@  static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
 	if (req->complete)
 		return true;
 
+	if (req->seqno == 0)
+		return false;
+
 	i915_gem_complete_requests_ring(req->ring, lazy_coherency);
 
 	return req->complete;
@@ -2482,7 +2485,7 @@  i915_seqno_passed(uint32_t seq1, uint32_t seq2)
 	return (int32_t)(seq1 - seq2) >= 0;
 }
 
-int __must_check i915_gem_get_seqno(struct drm_device *dev, u32 *seqno);
+int __must_check i915_gem_prepare_next_seqno(struct drm_device *dev);
 int __must_check i915_gem_set_seqno(struct drm_device *dev, u32 seqno);
 int __must_check i915_gem_object_get_fence(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_object_put_fence(struct drm_i915_gem_object *obj);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 260ef47..7db84b2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2310,12 +2310,15 @@  int i915_gem_set_seqno(struct drm_device *dev, u32 seqno)
 }
 
 int
-i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
+i915_gem_prepare_next_seqno(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	/* 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;
@@ -2323,6 +2326,24 @@  i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
 		dev_priv->next_seqno = 1;
 	}
 
+	return 0;
+}
+
+static int
+i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* reserve 0 for non-seqno */
+	if (dev_priv->next_seqno == 0) {
+		/* Should never get here! Must always call 'prepare_next' in
+		 * advance. This code is called during request submission.
+		 * Trying to wrap the seqno and the implicit idle() calls that
+		 * the wrap code makes are a bad idea at this point! */
+		DRM_ERROR("Need to wrap seqno at inopportune moment!\n");
+		return -EBUSY;
+	}
+
 	*seqno = dev_priv->last_seqno = dev_priv->next_seqno++;
 	return 0;
 }
@@ -2366,6 +2387,11 @@  int __i915_add_request(struct intel_engine_cs *ring,
 			return ret;
 	}
 
+	/* Assign an identifier to track this request through the hardware: */
+	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
+	if (ret)
+		return ret;
+
 	/* 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
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 5a75eb5..e7d4d20 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -802,6 +802,10 @@  static int logical_ring_alloc_request(struct intel_engine_cs *ring,
 	if (ring->outstanding_lazy_request)
 		return 0;
 
+	ret = i915_gem_prepare_next_seqno(ring->dev);
+	if (ret)
+		return ret;
+
 	request = kzalloc(sizeof(*request), GFP_KERNEL);
 	if (request == NULL)
 		return -ENOMEM;
@@ -809,12 +813,6 @@  static int logical_ring_alloc_request(struct intel_engine_cs *ring,
 	kref_init(&request->ref);
 	request->ring = ring;
 
-	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
-	if (ret) {
-		kfree(request);
-		return ret;
-	}
-
 	/* Hold a reference to the context this request belongs to
 	 * (we will need it when the time comes to emit/retire the
 	 * request).
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 0f2719d..6a2f25d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2017,6 +2017,10 @@  intel_ring_alloc_request(struct intel_engine_cs *ring)
 	if (ring->outstanding_lazy_request)
 		return 0;
 
+	ret = i915_gem_prepare_next_seqno(ring->dev);
+	if (ret)
+		return ret;
+
 	request = kzalloc(sizeof(*request), GFP_KERNEL);
 	if (request == NULL)
 		return -ENOMEM;
@@ -2024,12 +2028,6 @@  intel_ring_alloc_request(struct intel_engine_cs *ring)
 	kref_init(&request->ref);
 	request->ring = ring;
 
-	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
-	if (ret) {
-		kfree(request);
-		return ret;
-	}
-
 	ring->outstanding_lazy_request = request;
 	return 0;
 }