[36/53] drm/i915: Abstract the workload submission mechanism away

Commit Message

[e-mail] June 13, 2014, 3:37 p.m. UTC

From: Oscar Mateo <[e-mail]>

As suggested by Daniel.

Signed-off-by: Oscar Mateo <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.h            | 26 ++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c            | 48 +++++++++++++++++++-----------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 25 +++++++---------
 3 files changed, 67 insertions(+), 32 deletions(-)

Comments

Daniel Vetter June 18, 2014, 8:40 p.m. UTC | #1

On Fri, Jun 13, 2014 at 04:37:54PM +0100, [e-mail] wrote:
> From: Oscar Mateo <[e-mail]>
> 
> As suggested by Daniel.
> 
> Signed-off-by: Oscar Mateo <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            | 26 ++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem.c            | 48 +++++++++++++++++++-----------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 25 +++++++---------
>  3 files changed, 67 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3e9983c..89b6d5c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1554,6 +1554,23 @@ struct drm_i915_private {
>  	/* Old ums support infrastructure, same warning applies. */
>  	struct i915_ums_state ums;
>  
> +	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
> +	struct {
> +		int (*do_execbuf) (struct drm_device *dev, struct drm_file *file,
> +				   struct intel_engine_cs *ring,
> +				   struct intel_context *ctx,
> +				   struct drm_i915_gem_execbuffer2 *args,
> +				   struct list_head *vmas,
> +				   struct drm_i915_gem_object *batch_obj,
> +				   u64 exec_start, u32 flags);
> +		int (*add_request) (struct intel_engine_cs *ring,
> +				    struct drm_file *file,
> +				    struct drm_i915_gem_object *obj,
> +				    u32 *out_seqno);

Hm, what do we need add_request for? With the clean split in command
submission I'd expect every function to know whether it'll submit to an
lrc (everything in intel_lrc.c) or whether it'll submit to a legacy ring
(existing code), so I don't see a need for an add_request vfunc.

Au contraire that looks a bit dangerous since code might get run with
execlist that assumes we have a legcy ring ...

A bit confused, but let's hope this clears up in further patches.
-Daniel

> +		int (*init_rings) (struct drm_device *dev);
> +		void (*cleanup_ring) (struct intel_engine_cs *ring);
> +	} gt;
> +
>  	/*
>  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>  	 * will be rejected. Instead look for a better place.
> @@ -2131,6 +2148,14 @@ void i915_gem_execbuffer_retire_commands(struct drm_device *dev,
>  					 struct drm_file *file,
>  					 struct intel_engine_cs *ring,
>  					 struct drm_i915_gem_object *obj);
> +int i915_gem_ringbuffer_submission(struct drm_device *dev,
> +				   struct drm_file *file,
> +				   struct intel_engine_cs *ring,
> +				   struct intel_context *ctx,
> +				   struct drm_i915_gem_execbuffer2 *args,
> +				   struct list_head *vmas,
> +				   struct drm_i915_gem_object *batch_obj,
> +				   u64 exec_start, u32 flags);
>  int i915_gem_execbuffer(struct drm_device *dev, void *data,
>  			struct drm_file *file_priv);
>  int i915_gem_execbuffer2(struct drm_device *dev, void *data,
> @@ -2281,6 +2306,7 @@ void i915_gem_reset(struct drm_device *dev);
>  bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
>  int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
>  int __must_check i915_gem_init(struct drm_device *dev);
> +int i915_gem_init_rings(struct drm_device *dev);
>  int __must_check i915_gem_init_hw(struct drm_device *dev);
>  int i915_gem_l3_remap(struct intel_engine_cs *ring, int slice);
>  void i915_gem_init_swizzling(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 69db71a..7c10540 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2310,19 +2310,16 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
>  	return 0;
>  }
>  
> -int __i915_add_request(struct intel_engine_cs *ring,
> -		       struct drm_file *file,
> -		       struct drm_i915_gem_object *obj,
> -		       u32 *out_seqno)
> +static int i915_gem_add_request(struct intel_engine_cs *ring,
> +				struct drm_file *file,
> +				struct drm_i915_gem_object *obj,
> +				u32 *out_seqno)
>  {
>  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>  	struct drm_i915_gem_request *request;
>  	u32 request_ring_position, request_start;
>  	int ret;
>  
> -	if (intel_enable_execlists(ring->dev))
> -		return intel_logical_ring_add_request(ring, file, obj, out_seqno);
> -
>  	request_start = intel_ring_get_tail(ring->buffer);
>  	/*
>  	 * Emit any outstanding flushes - execbuf can fail to emit the flush
> @@ -2403,6 +2400,16 @@ int __i915_add_request(struct intel_engine_cs *ring,
>  	return 0;
>  }
>  
> +int __i915_add_request(struct intel_engine_cs *ring,
> +		       struct drm_file *file,
> +		       struct drm_i915_gem_object *obj,
> +		       u32 *out_seqno)
> +{
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +
> +	return dev_priv->gt.add_request(ring, file, obj, out_seqno);
> +}
> +
>  static inline void
>  i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
>  {
> @@ -4627,7 +4634,7 @@ intel_enable_blt(struct drm_device *dev)
>  	return true;
>  }
>  
> -static int i915_gem_init_rings(struct drm_device *dev)
> +int i915_gem_init_rings(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
> @@ -4710,10 +4717,7 @@ i915_gem_init_hw(struct drm_device *dev)
>  
>  	i915_gem_init_swizzling(dev);
>  
> -	if (intel_enable_execlists(dev))
> -		ret = intel_logical_rings_init(dev);
> -	else
> -		ret = i915_gem_init_rings(dev);
> +	ret = dev_priv->gt.init_rings(dev);
>  	if (ret)
>  		return ret;
>  
> @@ -4751,6 +4755,18 @@ int i915_gem_init(struct drm_device *dev)
>  			DRM_DEBUG_DRIVER("allow wake ack timed out\n");
>  	}
>  
> +	if (intel_enable_execlists(dev)) {
> +		dev_priv->gt.do_execbuf = intel_execlists_submission;
> +		dev_priv->gt.add_request = intel_logical_ring_add_request;
> +		dev_priv->gt.init_rings = intel_logical_rings_init;
> +		dev_priv->gt.cleanup_ring = intel_logical_ring_cleanup;
> +	} else {
> +		dev_priv->gt.do_execbuf = i915_gem_ringbuffer_submission;
> +		dev_priv->gt.add_request = i915_gem_add_request;
> +		dev_priv->gt.init_rings = i915_gem_init_rings;
> +		dev_priv->gt.cleanup_ring = intel_cleanup_ring_buffer;
> +	}
> +
>  	i915_gem_init_userptr(dev);
>  	i915_gem_init_global_gtt(dev);
>  
> @@ -4785,12 +4801,8 @@ i915_gem_cleanup_ringbuffer(struct drm_device *dev)
>  	struct intel_engine_cs *ring;
>  	int i;
>  
> -	for_each_ring(ring, dev_priv, i) {
> -		if (intel_enable_execlists(dev))
> -			intel_logical_ring_cleanup(ring);
> -		else
> -			intel_cleanup_ring_buffer(ring);
> -	}
> +	for_each_ring(ring, dev_priv, i)
> +		dev_priv->gt.cleanup_ring(ring);
>  }
>  
>  int
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 36c7f0c..f0dd31f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1005,14 +1005,15 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
>  	return 0;
>  }
>  
> -static int
> -legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
> -			     struct intel_engine_cs *ring,
> -			     struct intel_context *ctx,
> -			     struct drm_i915_gem_execbuffer2 *args,
> -			     struct list_head *vmas,
> -			     struct drm_i915_gem_object *batch_obj,
> -			     u64 exec_start, u32 flags)
> +int
> +i915_gem_ringbuffer_submission(struct drm_device *dev,
> +			       struct drm_file *file,
> +			       struct intel_engine_cs *ring,
> +			       struct intel_context *ctx,
> +			       struct drm_i915_gem_execbuffer2 *args,
> +			       struct list_head *vmas,
> +			       struct drm_i915_gem_object *batch_obj,
> +			       u64 exec_start, u32 flags)
>  {
>  	struct drm_clip_rect *cliprects = NULL;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1379,12 +1380,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	else
>  		exec_start += i915_gem_obj_offset(batch_obj, vm);
>  
> -	if (intel_enable_execlists(dev))
> -		ret = intel_execlists_submission(dev, file, ring, ctx,
> -				args, &eb->vmas, batch_obj, exec_start, flags);
> -	else
> -		ret = legacy_ringbuffer_submission(dev, file, ring, ctx,
> -				args, &eb->vmas, batch_obj, exec_start, flags);
> +	ret = dev_priv->gt.do_execbuf(dev, file, ring, ctx, args,
> +			&eb->vmas, batch_obj, exec_start, flags);
>  	if (ret)
>  		goto err;
>  
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3e9983c..89b6d5c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1554,6 +1554,23 @@  struct drm_i915_private {
 	/* Old ums support infrastructure, same warning applies. */
 	struct i915_ums_state ums;
 
+	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
+	struct {
+		int (*do_execbuf) (struct drm_device *dev, struct drm_file *file,
+				   struct intel_engine_cs *ring,
+				   struct intel_context *ctx,
+				   struct drm_i915_gem_execbuffer2 *args,
+				   struct list_head *vmas,
+				   struct drm_i915_gem_object *batch_obj,
+				   u64 exec_start, u32 flags);
+		int (*add_request) (struct intel_engine_cs *ring,
+				    struct drm_file *file,
+				    struct drm_i915_gem_object *obj,
+				    u32 *out_seqno);
+		int (*init_rings) (struct drm_device *dev);
+		void (*cleanup_ring) (struct intel_engine_cs *ring);
+	} gt;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
@@ -2131,6 +2148,14 @@  void i915_gem_execbuffer_retire_commands(struct drm_device *dev,
 					 struct drm_file *file,
 					 struct intel_engine_cs *ring,
 					 struct drm_i915_gem_object *obj);
+int i915_gem_ringbuffer_submission(struct drm_device *dev,
+				   struct drm_file *file,
+				   struct intel_engine_cs *ring,
+				   struct intel_context *ctx,
+				   struct drm_i915_gem_execbuffer2 *args,
+				   struct list_head *vmas,
+				   struct drm_i915_gem_object *batch_obj,
+				   u64 exec_start, u32 flags);
 int i915_gem_execbuffer(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
 int i915_gem_execbuffer2(struct drm_device *dev, void *data,
@@ -2281,6 +2306,7 @@  void i915_gem_reset(struct drm_device *dev);
 bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
 int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_init(struct drm_device *dev);
+int i915_gem_init_rings(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 int i915_gem_l3_remap(struct intel_engine_cs *ring, int slice);
 void i915_gem_init_swizzling(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 69db71a..7c10540 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2310,19 +2310,16 @@  i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
 	return 0;
 }
 
-int __i915_add_request(struct intel_engine_cs *ring,
-		       struct drm_file *file,
-		       struct drm_i915_gem_object *obj,
-		       u32 *out_seqno)
+static int i915_gem_add_request(struct intel_engine_cs *ring,
+				struct drm_file *file,
+				struct drm_i915_gem_object *obj,
+				u32 *out_seqno)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 	struct drm_i915_gem_request *request;
 	u32 request_ring_position, request_start;
 	int ret;
 
-	if (intel_enable_execlists(ring->dev))
-		return intel_logical_ring_add_request(ring, file, obj, out_seqno);
-
 	request_start = intel_ring_get_tail(ring->buffer);
 	/*
 	 * Emit any outstanding flushes - execbuf can fail to emit the flush
@@ -2403,6 +2400,16 @@  int __i915_add_request(struct intel_engine_cs *ring,
 	return 0;
 }
 
+int __i915_add_request(struct intel_engine_cs *ring,
+		       struct drm_file *file,
+		       struct drm_i915_gem_object *obj,
+		       u32 *out_seqno)
+{
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+
+	return dev_priv->gt.add_request(ring, file, obj, out_seqno);
+}
+
 static inline void
 i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
 {
@@ -4627,7 +4634,7 @@  intel_enable_blt(struct drm_device *dev)
 	return true;
 }
 
-static int i915_gem_init_rings(struct drm_device *dev)
+int i915_gem_init_rings(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
@@ -4710,10 +4717,7 @@  i915_gem_init_hw(struct drm_device *dev)
 
 	i915_gem_init_swizzling(dev);
 
-	if (intel_enable_execlists(dev))
-		ret = intel_logical_rings_init(dev);
-	else
-		ret = i915_gem_init_rings(dev);
+	ret = dev_priv->gt.init_rings(dev);
 	if (ret)
 		return ret;
 
@@ -4751,6 +4755,18 @@  int i915_gem_init(struct drm_device *dev)
 			DRM_DEBUG_DRIVER("allow wake ack timed out\n");
 	}
 
+	if (intel_enable_execlists(dev)) {
+		dev_priv->gt.do_execbuf = intel_execlists_submission;
+		dev_priv->gt.add_request = intel_logical_ring_add_request;
+		dev_priv->gt.init_rings = intel_logical_rings_init;
+		dev_priv->gt.cleanup_ring = intel_logical_ring_cleanup;
+	} else {
+		dev_priv->gt.do_execbuf = i915_gem_ringbuffer_submission;
+		dev_priv->gt.add_request = i915_gem_add_request;
+		dev_priv->gt.init_rings = i915_gem_init_rings;
+		dev_priv->gt.cleanup_ring = intel_cleanup_ring_buffer;
+	}
+
 	i915_gem_init_userptr(dev);
 	i915_gem_init_global_gtt(dev);
 
@@ -4785,12 +4801,8 @@  i915_gem_cleanup_ringbuffer(struct drm_device *dev)
 	struct intel_engine_cs *ring;
 	int i;
 
-	for_each_ring(ring, dev_priv, i) {
-		if (intel_enable_execlists(dev))
-			intel_logical_ring_cleanup(ring);
-		else
-			intel_cleanup_ring_buffer(ring);
-	}
+	for_each_ring(ring, dev_priv, i)
+		dev_priv->gt.cleanup_ring(ring);
 }
 
 int
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 36c7f0c..f0dd31f 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1005,14 +1005,15 @@  i915_reset_gen7_sol_offsets(struct drm_device *dev,
 	return 0;
 }
 
-static int
-legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
-			     struct intel_engine_cs *ring,
-			     struct intel_context *ctx,
-			     struct drm_i915_gem_execbuffer2 *args,
-			     struct list_head *vmas,
-			     struct drm_i915_gem_object *batch_obj,
-			     u64 exec_start, u32 flags)
+int
+i915_gem_ringbuffer_submission(struct drm_device *dev,
+			       struct drm_file *file,
+			       struct intel_engine_cs *ring,
+			       struct intel_context *ctx,
+			       struct drm_i915_gem_execbuffer2 *args,
+			       struct list_head *vmas,
+			       struct drm_i915_gem_object *batch_obj,
+			       u64 exec_start, u32 flags)
 {
 	struct drm_clip_rect *cliprects = NULL;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1379,12 +1380,8 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	else
 		exec_start += i915_gem_obj_offset(batch_obj, vm);
 
-	if (intel_enable_execlists(dev))
-		ret = intel_execlists_submission(dev, file, ring, ctx,
-				args, &eb->vmas, batch_obj, exec_start, flags);
-	else
-		ret = legacy_ringbuffer_submission(dev, file, ring, ctx,
-				args, &eb->vmas, batch_obj, exec_start, flags);
+	ret = dev_priv->gt.do_execbuf(dev, file, ring, ctx, args,
+			&eb->vmas, batch_obj, exec_start, flags);
 	if (ret)
 		goto err;