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

[Tomas Elf <tomas.elf@intel.com>]

On 19/03/2015 12:30, 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 simplifies 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
> some way and guaranteed to exist for the duration of the batch buffer's life.
>
> v2: Rebased to newer tree and updated for changes to the command parser.
> Specifically, a code shuffle has required saving the batch start address in the
> params structure.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Reviewed-by: Tomas Elf <tomas.elf@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h            |   28 +++++++------
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |   61 ++++++++++++++++++----------
>   drivers/gpu/drm/i915/intel_lrc.c           |   26 ++++++------
>   drivers/gpu/drm/i915/intel_lrc.h           |    9 ++--
>   4 files changed, 71 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7b36d4a..6ae99ce 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1558,6 +1558,17 @@ struct i915_virtual_gpu {
>   	bool active;
>   };
>
> +struct i915_execbuffer_params {
> +	struct drm_device               *dev;
> +	struct drm_file                 *file;
> +	uint32_t                        dispatch_flags;
> +	uint32_t                        args_batch_start_offset;
> +	uint32_t                        batch_obj_vm_offset;
> +	struct intel_engine_cs          *ring;
> +	struct drm_i915_gem_object      *batch_obj;
> +	struct intel_context            *ctx;
> +};
> +
>   struct drm_i915_private {
>   	struct drm_device *dev;
>   	struct kmem_cache *slab;
> @@ -1812,13 +1823,9 @@ struct drm_i915_private {
>
>   	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
>   	struct {
> -		int (*execbuf_submit)(struct drm_device *dev, struct drm_file *file,
> -				      struct intel_engine_cs *ring,
> -				      struct intel_context *ctx,
> +		int (*execbuf_submit)(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);
> @@ -2569,14 +2576,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 2504cfd..2fffd99 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1185,17 +1185,15 @@ err:
>   }
>
>   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;
> @@ -1247,7 +1245,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;
>
> @@ -1304,12 +1302,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);

I know that I already r-b:ed this patch before but I discovered that 
this change is not necessary since dev is already defined further up. 
Just reuse that local variable and revert this change.

>   		if (ret)
>   			goto error;
>   	}
>
> -	exec_len = args->batch_len;
> +	exec_len   = args->batch_len;
> +	exec_start = params->batch_obj_vm_offset +
> +		     params->args_batch_start_offset;
> +
>   	if (cliprects) {
>   		for (i = 0; i < args->num_cliprects; i++) {
>   			ret = i915_emit_box(ring, &cliprects[i],
> @@ -1319,22 +1320,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);
> @@ -1404,8 +1406,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;
> @@ -1498,6 +1501,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);
> @@ -1567,8 +1572,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>   		if (USES_FULL_PPGTT(dev))
>   			dispatch_flags |= I915_DISPATCH_SECURE;
>
> -		exec_start = 0;
> -	}
> +		params->args_batch_start_offset = 0;
> +	} else
> +		params->args_batch_start_offset = args->batch_start_offset;
>
>   	batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
>
> @@ -1590,18 +1596,29 @@ 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);
>
>   	/* Allocate a request for this batch buffer nice and early. */
>   	ret = i915_gem_request_alloc(ring, ctx);
>   	if (ret)
>   		goto err_batch_unpin;
>
> -	ret = dev_priv->gt.execbuf_submit(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.execbuf_submit(params, args, &eb->vmas);
>
>   err_batch_unpin:
>   	/*
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index a0ce65b..c0d97e97 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -853,16 +853,15 @@ static int intel_logical_ring_begin(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;
> @@ -913,13 +912,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;
>
> @@ -932,14 +931,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 4148de0..bf137c4 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -76,13 +76,10 @@ void intel_lr_context_reset(struct drm_device *dev,
>
>   /* 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);
>

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