[Intel-gfx] [PATCH 09/59] drm/i915: Merged the many do_execbuf() parameters into a structure
Tomas Elf
tomas.elf at intel.com
Tue Mar 31 09:16:54 PDT 2015
On 19/03/2015 12:30, John.C.Harrison at Intel.com wrote:
> From: John Harrison <John.C.Harrison at 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 at Intel.com>
> Reviewed-by: Tomas Elf <tomas.elf at 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 = ¶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;
> @@ -1498,6 +1501,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);
> @@ -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);
>
More information about the Intel-gfx
mailing list