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
> +
> 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 = ¶ms_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(¶ms_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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
