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

[John Harrison <John.C.Harrison@xxxxxxxxx>]

On 25/02/2015 21:52, Daniel Vetter wrote:
> On Fri, Feb 13, 2015 at 11:48:13AM +0000, John.C.Harrison@xxxxxxxxx wrote:
> > From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> >   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

The major advantage of this is when the GPU scheduler arrives. It splits 
the execbuffer code path in half - front half is all the software state 
verification and management, back half is the actual hardware writes to 
post the buffer. This structure is used to bridge the two. It contains 
all the state created by the front half that is later used by the back 
half. It is saved away with the request/scheduler node until such a time 
as the back half is called. Hence some kind of structure is required and 
it does not really make sense to add all this information to the generic 
request structure. Also, with the full pre-emptive scheduler, the 
structure grows to about twenty entries and passing that quantity of 
individual parameters to a function is just unwieldly!


> > +
> >   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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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