Re: [PATCH 04/51] drm/i915: Merged the many do_execbuf() parameters into a structure

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Feb 13, 2015 at 11:48:13AM +0000, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> The do_execbuf() function takes quite a few parameters. The actual set of
> parameters is going to change with the conversion to passing requests around.
> Further, it is due to grow massively with the arrival of the GPU scheduler.
> 
> This patch simplies the prototype by passing a parameter structure instead.
> Changing the parameter set in the future is then simply a matter of
> adding/removing items to the structure.
> 
> Note that the structure does not contain absolutely everything that is passed
> in. This is because the intention is to use this structure more extensively
> later in this patch series and more especially in the GPU scheduler that is
> coming soon. The latter requires hanging on to the structure as the final
> hardware submission can be delayed until long after the execbuf IOCTL has
> returned to user land. Thus it is unsafe to put anything in the structure that
> is local to the IOCTL call itself - such as the 'args' parameter. All entries
> must be copies of data or pointers to structures that are reference counted in
> someway and guaranteed to exist for the duration of the batch buffer's life.
> 
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |   27 +++++++-------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   56 ++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_lrc.c           |   26 +++++++------
>  drivers/gpu/drm/i915/intel_lrc.h           |    9 ++---
>  4 files changed, 67 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e90b786..e6d616b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1640,6 +1640,16 @@ struct i915_workarounds {
>  	u32 count;
>  };
>  
> +struct i915_execbuffer_params {
> +	struct drm_device               *dev;
> +	struct drm_file                 *file;
> +	uint32_t                        dispatch_flags;
> +	uint32_t                        batch_obj_vm_offset;
> +	struct intel_engine_cs          *ring;
> +	struct drm_i915_gem_object      *batch_obj;
> +	struct intel_context            *ctx;
> +};

tbh I'm not a fan of parameter objects in C. C is verbose and explicit.

If we add the request then we can remove ring and ctx, which already
improves things. We also have the eb structure we use to pass around a
pile of things, which we could somewhat reuse here. What else do you plan
to add? Just want to figure out whether this is really required ...
-Daniel

> +
>  struct drm_i915_private {
>  	struct drm_device *dev;
>  	struct kmem_cache *slab;
> @@ -1891,13 +1901,9 @@ struct drm_i915_private {
>  
>  	/* 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,
> +		int (*do_execbuf)(struct i915_execbuffer_params *params,
>  				  struct drm_i915_gem_execbuffer2 *args,
> -				  struct list_head *vmas,
> -				  struct drm_i915_gem_object *batch_obj,
> -				  u64 exec_start, u32 flags);
> +				  struct list_head *vmas);
>  		int (*init_rings)(struct drm_device *dev);
>  		void (*cleanup_ring)(struct intel_engine_cs *ring);
>  		void (*stop_ring)(struct intel_engine_cs *ring);
> @@ -2622,14 +2628,9 @@ 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,
> +int i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  				   struct drm_i915_gem_execbuffer2 *args,
> -				   struct list_head *vmas,
> -				   struct drm_i915_gem_object *batch_obj,
> -				   u64 exec_start, u32 flags);
> +				   struct list_head *vmas);
>  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,
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index ec9ea45..93b0ef0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1132,17 +1132,15 @@ i915_gem_execbuffer_parse(struct intel_engine_cs *ring,
>  }
>  
>  int
> -i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
> -			       struct intel_engine_cs *ring,
> -			       struct intel_context *ctx,
> +i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  			       struct drm_i915_gem_execbuffer2 *args,
> -			       struct list_head *vmas,
> -			       struct drm_i915_gem_object *batch_obj,
> -			       u64 exec_start, u32 dispatch_flags)
> +			       struct list_head *vmas)
>  {
>  	struct drm_clip_rect *cliprects = NULL;
> +	struct drm_device *dev = params->dev;
> +	struct intel_engine_cs *ring = params->ring;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u64 exec_len;
> +	u64 exec_start, exec_len;
>  	int instp_mode;
>  	u32 instp_mask;
>  	int i, ret = 0;
> @@ -1194,7 +1192,7 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
>  	if (ret)
>  		goto error;
>  
> -	ret = i915_switch_context(ring, ctx);
> +	ret = i915_switch_context(ring, params->ctx);
>  	if (ret)
>  		goto error;
>  
> @@ -1251,12 +1249,15 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
>  	}
>  
>  	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
> -		ret = i915_reset_gen7_sol_offsets(dev, ring);
> +		ret = i915_reset_gen7_sol_offsets(params->dev, ring);
>  		if (ret)
>  			goto error;
>  	}
>  
> -	exec_len = args->batch_len;
> +	exec_len   = args->batch_len;
> +	exec_start = params->batch_obj_vm_offset +
> +		     args->batch_start_offset;
> +
>  	if (cliprects) {
>  		for (i = 0; i < args->num_cliprects; i++) {
>  			ret = i915_emit_box(ring, &cliprects[i],
> @@ -1266,22 +1267,23 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
>  
>  			ret = ring->dispatch_execbuffer(ring,
>  							exec_start, exec_len,
> -							dispatch_flags);
> +							params->dispatch_flags);
>  			if (ret)
>  				goto error;
>  		}
>  	} else {
>  		ret = ring->dispatch_execbuffer(ring,
>  						exec_start, exec_len,
> -						dispatch_flags);
> +						params->dispatch_flags);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), dispatch_flags);
> +	trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), params->dispatch_flags);
>  
>  	i915_gem_execbuffer_move_to_active(vmas, ring);
> -	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
> +	i915_gem_execbuffer_retire_commands(params->dev, params->file, ring,
> +					    params->batch_obj);
>  
>  error:
>  	kfree(cliprects);
> @@ -1351,8 +1353,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	struct intel_engine_cs *ring;
>  	struct intel_context *ctx;
>  	struct i915_address_space *vm;
> +	struct i915_execbuffer_params params_master; /* XXX: will be removed later */
> +	struct i915_execbuffer_params *params = &params_master;
>  	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
> -	u64 exec_start = args->batch_start_offset;
>  	u32 dispatch_flags;
>  	int ret;
>  	bool need_relocs;
> @@ -1445,6 +1448,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	else
>  		vm = &dev_priv->gtt.base;
>  
> +	memset(&params_master, 0x00, sizeof(params_master));
> +
>  	eb = eb_create(args);
>  	if (eb == NULL) {
>  		i915_gem_context_unreference(ctx);
> @@ -1522,13 +1527,24 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		if (ret)
>  			goto err;
>  
> -		exec_start += i915_gem_obj_ggtt_offset(batch_obj);
> +		params->batch_obj_vm_offset = i915_gem_obj_ggtt_offset(batch_obj);
>  	} else
> -		exec_start += i915_gem_obj_offset(batch_obj, vm);
> +		params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm);
>  
> -	ret = dev_priv->gt.do_execbuf(dev, file, ring, ctx, args,
> -				      &eb->vmas, batch_obj, exec_start,
> -				      dispatch_flags);
> +	/*
> +	 * Save assorted stuff away to pass through to *_submission().
> +	 * NB: This data should be 'persistent' and not local as it will
> +	 * kept around beyond the duration of the IOCTL once the GPU
> +	 * scheduler arrives.
> +	 */
> +	params->dev                     = dev;
> +	params->file                    = file;
> +	params->ring                    = ring;
> +	params->dispatch_flags          = dispatch_flags;
> +	params->batch_obj               = batch_obj;
> +	params->ctx                     = ctx;
> +
> +	ret = dev_priv->gt.do_execbuf(params, args, &eb->vmas);
>  
>  	/*
>  	 * FIXME: We crucially rely upon the active tracking for the (ppgtt)
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 762136b..ca29290 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -618,16 +618,15 @@ static int execlists_move_to_gpu(struct intel_ringbuffer *ringbuf,
>   *
>   * Return: non-zero if the submission fails.
>   */
> -int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
> -			       struct intel_engine_cs *ring,
> -			       struct intel_context *ctx,
> +int intel_execlists_submission(struct i915_execbuffer_params *params,
>  			       struct drm_i915_gem_execbuffer2 *args,
> -			       struct list_head *vmas,
> -			       struct drm_i915_gem_object *batch_obj,
> -			       u64 exec_start, u32 dispatch_flags)
> +			       struct list_head *vmas)
>  {
> +	struct drm_device       *dev = params->dev;
> +	struct intel_engine_cs  *ring = params->ring;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
> +	struct intel_ringbuffer *ringbuf = params->ctx->engine[ring->id].ringbuf;
> +	u64 exec_start;
>  	int instp_mode;
>  	u32 instp_mask;
>  	int ret;
> @@ -678,13 +677,13 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
>  		return -EINVAL;
>  	}
>  
> -	ret = execlists_move_to_gpu(ringbuf, ctx, vmas);
> +	ret = execlists_move_to_gpu(ringbuf, params->ctx, vmas);
>  	if (ret)
>  		return ret;
>  
>  	if (ring == &dev_priv->ring[RCS] &&
>  	    instp_mode != dev_priv->relative_constants_mode) {
> -		ret = intel_logical_ring_begin(ringbuf, ctx, 4);
> +		ret = intel_logical_ring_begin(ringbuf, params->ctx, 4);
>  		if (ret)
>  			return ret;
>  
> @@ -697,14 +696,17 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
>  		dev_priv->relative_constants_mode = instp_mode;
>  	}
>  
> -	ret = ring->emit_bb_start(ringbuf, ctx, exec_start, dispatch_flags);
> +	exec_start = params->batch_obj_vm_offset +
> +		     args->batch_start_offset;
> +
> +	ret = ring->emit_bb_start(ringbuf, params->ctx, exec_start, params->dispatch_flags);
>  	if (ret)
>  		return ret;
>  
> -	trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), dispatch_flags);
> +	trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), params->dispatch_flags);
>  
>  	i915_gem_execbuffer_move_to_active(vmas, ring);
> -	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
> +	i915_gem_execbuffer_retire_commands(params->dev, params->file, ring, params->batch_obj);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 3093836..ae2f3ed 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -80,13 +80,10 @@ void intel_lr_context_unpin(struct intel_engine_cs *ring,
>  
>  /* Execlists */
>  int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists);
> -int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
> -			       struct intel_engine_cs *ring,
> -			       struct intel_context *ctx,
> +struct i915_execbuffer_params;
> +int intel_execlists_submission(struct i915_execbuffer_params *params,
>  			       struct drm_i915_gem_execbuffer2 *args,
> -			       struct list_head *vmas,
> -			       struct drm_i915_gem_object *batch_obj,
> -			       u64 exec_start, u32 dispatch_flags);
> +			       struct list_head *vmas);
>  u32 intel_execlists_ctx_id(struct drm_i915_gem_object *ctx_obj);
>  
>  void intel_lrc_irq_handler(struct intel_engine_cs *ring);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx