Re: [PATCH 17/18] drm/i915: Unify legacy/execlists submit_execbuf callbacks

Joonas Lahtinen
2016-07-22T08:45:10Z

On ke, 2016-07-20 at 14:12 +0100, Chris Wilson wrote:
> Now that emitting requests is identical between legacy and execlists, we
> can use the same function to build up the ring for submitting to either
> engine. (With the exception of i915_switch_contexts(), but in time that
> will also be handled gracefully.)
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

This series craves for some T-b's.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  20 -----
>  drivers/gpu/drm/i915/i915_gem.c            |   2 -
>  drivers/gpu/drm/i915/i915_gem_context.c    |   7 +-
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  24 ++++--
>  drivers/gpu/drm/i915/intel_lrc.c           | 123 -----------------------------
>  drivers/gpu/drm/i915/intel_lrc.h           |   4 -
>  6 files changed, 21 insertions(+), 159 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3f67431577e3..f188c9a9b746 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1705,18 +1705,6 @@ 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;
> - uint64_t                        batch_obj_vm_offset;
> - struct intel_engine_cs *engine;
> - struct drm_i915_gem_object      *batch_obj;
> - struct i915_gem_context            *ctx;
> - struct drm_i915_gem_request     *request;
> -};
> -
>  /* used in computing the new watermarks state */
>  struct intel_wm_config {
>   unsigned int num_pipes_active;
> @@ -2016,9 +2004,6 @@ struct drm_i915_private {
>
>   /* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
>   struct {
> - int (*execbuf_submit)(struct i915_execbuffer_params *params,
> -       struct drm_i915_gem_execbuffer2 *args,
> -       struct list_head *vmas);
>   void (*cleanup_engine)(struct intel_engine_cs *engine);
>   void (*stop_engine)(struct intel_engine_cs *engine);
>
> @@ -2990,11 +2975,6 @@ int i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>         struct drm_file *file_priv);
>  int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
>        struct drm_file *file_priv);
> -void i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> - struct drm_i915_gem_request *req);
> -int i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
> -    struct drm_i915_gem_execbuffer2 *args,
> -    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.c b/drivers/gpu/drm/i915/i915_gem.c
> index 77d7c0b012f4..9fdecef34fa8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4531,11 +4531,9 @@ int i915_gem_init(struct drm_device *dev)
>   mutex_lock(&dev->struct_mutex);
>
>   if (!i915.enable_execlists) {
> - dev_priv->gt.execbuf_submit = i915_gem_ringbuffer_submission;
>   dev_priv->gt.cleanup_engine = intel_engine_cleanup;
>   dev_priv->gt.stop_engine = intel_engine_stop;
>   } else {
> - dev_priv->gt.execbuf_submit = intel_execlists_submission;
>   dev_priv->gt.cleanup_engine = intel_logical_ring_cleanup;
>   dev_priv->gt.stop_engine = intel_logical_ring_stop;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index e1eed0f449c6..72b21c7b7547 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -893,8 +893,9 @@ int i915_switch_context(struct drm_i915_gem_request *req)
>  {
>   struct intel_engine_cs *engine = req->engine;
>
> - WARN_ON(i915.enable_execlists);
>   lockdep_assert_held(&req->i915->drm.struct_mutex);
> + if (i915.enable_execlists)
> + return 0;
>
>   if (!req->ctx->engine[engine->id].state) {
>   struct i915_gem_context *to = req->ctx;
> @@ -942,9 +943,7 @@ int i915_gem_switch_to_kernel_context(struct drm_i915_private *dev_priv)
>   if (IS_ERR(req))
>   return PTR_ERR(req);
>
> - ret = 0;
> - if (!i915.enable_execlists)
> - ret = i915_switch_context(req);
> + ret = i915_switch_context(req);
>   i915_add_request_no_flush(req);
>   if (ret)
>   return ret;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 2d9f1f4bc058..e302477418d8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -42,6 +42,18 @@
>
>  #define BATCH_OFFSET_BIAS (256*1024)
>
> +struct i915_execbuffer_params {
> + struct drm_device               *dev;
> + struct drm_file                 *file;
> + u32  dispatch_flags;
> + u32  args_batch_start_offset;
> + u32  batch_obj_vm_offset;
> + struct intel_engine_cs          *engine;
> + struct drm_i915_gem_object      *batch_obj;
> + struct i915_gem_context         *ctx;
> + struct drm_i915_gem_request     *request;
> +};
> +
>  struct eb_vmas {
>   struct list_head vmas;
>   int and;
> @@ -1117,7 +1129,7 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
>   return ctx;
>  }
>
> -void
> +static void
>  i915_gem_execbuffer_move_to_active(struct list_head *vmas,
>      struct drm_i915_gem_request *req)
>  {
> @@ -1244,10 +1256,10 @@ err:
>   return ERR_PTR(ret);
>  }
>
> -int
> -i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
> -        struct drm_i915_gem_execbuffer2 *args,
> -        struct list_head *vmas)
> +static int
> +execbuf_submit(struct i915_execbuffer_params *params,
> +        struct drm_i915_gem_execbuffer2 *args,
> +        struct list_head *vmas)
>  {
>   struct drm_i915_private *dev_priv = params->request->i915;
>   u64 exec_start, exec_len;
> @@ -1636,7 +1648,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>   params->batch_obj               = batch_obj;
>   params->ctx                     = ctx;
>
> - ret = dev_priv->gt.execbuf_submit(params, args, &eb->vmas);
> + ret = execbuf_submit(params, args, &eb->vmas);
>  err_request:
>   i915_gem_execbuffer_retire_commands(params);
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index bce37d0d431f..8d1589f0ea7e 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -642,39 +642,6 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
>   spin_unlock_bh(&engine->execlist_lock);
>  }
>
> -static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
> -  struct list_head *vmas)
> -{
> - const unsigned other_rings = ~intel_engine_flag(req->engine);
> - struct i915_vma *vma;
> - uint32_t flush_domains = 0;
> - bool flush_chipset = false;
> - int ret;
> -
> - list_for_each_entry(vma, vmas, exec_list) {
> - struct drm_i915_gem_object *obj = vma->obj;
> -
> - if (obj->active & other_rings) {
> - ret = i915_gem_object_sync(obj, req);
> - if (ret)
> - return ret;
> - }
> -
> - if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
> - flush_chipset |= i915_gem_clflush_object(obj, false);
> -
> - flush_domains |= obj->base.write_domain;
> - }
> -
> - if (flush_domains & I915_GEM_DOMAIN_GTT)
> - wmb();
> -
> - /* Unconditionally invalidate gpu caches and ensure that we do flush
> -  * any residual writes from the previous batch.
> -  */
> - return req->engine->emit_flush(req, I915_GEM_GPU_DOMAINS, 0);
> -}
> -
>  int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request)
>  {
>   struct intel_engine_cs *engine = request->engine;
> @@ -776,96 +743,6 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
>   return 0;
>  }
>
> -/**
> - * intel_execlists_submission() - submit a batchbuffer for execution, Execlists style
> - * @params: execbuffer call parameters.
> - * @args: execbuffer call arguments.
> - * @vmas: list of vmas.
> - *
> - * This is the evil twin version of i915_gem_ringbuffer_submission. It abstracts
> - * away the submission details of the execbuffer ioctl call.
> - *
> - * Return: non-zero if the submission fails.
> - */
> -int intel_execlists_submission(struct i915_execbuffer_params *params,
> -        struct drm_i915_gem_execbuffer2 *args,
> -        struct list_head *vmas)
> -{
> - struct drm_device       *dev = params->dev;
> - struct intel_engine_cs *engine = params->engine;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_ring *ring = params->request->ring;
> - u64 exec_start;
> - int instp_mode;
> - u32 instp_mask;
> - int ret;
> -
> - instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> - instp_mask = I915_EXEC_CONSTANTS_MASK;
> - switch (instp_mode) {
> - case I915_EXEC_CONSTANTS_REL_GENERAL:
> - case I915_EXEC_CONSTANTS_ABSOLUTE:
> - case I915_EXEC_CONSTANTS_REL_SURFACE:
> - if (instp_mode != 0 && engine->id != RCS) {
> - DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
> - return -EINVAL;
> - }
> -
> - if (instp_mode != dev_priv->relative_constants_mode) {
> - if (instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
> - DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
> - return -EINVAL;
> - }
> -
> - /* The HW changed the meaning on this bit on gen6 */
> - instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
> - }
> - break;
> - default:
> - DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
> - return -EINVAL;
> - }
> -
> - if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
> - DRM_DEBUG("sol reset is gen7 only\n");
> - return -EINVAL;
> - }
> -
> - ret = execlists_move_to_gpu(params->request, vmas);
> - if (ret)
> - return ret;
> -
> - if (engine->id == RCS &&
> -     instp_mode != dev_priv->relative_constants_mode) {
> - ret = intel_ring_begin(params->request, 4);
> - if (ret)
> - return ret;
> -
> - intel_ring_emit(ring, MI_NOOP);
> - intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> - intel_ring_emit_reg(ring, INSTPM);
> - intel_ring_emit(ring, instp_mask << 16 | instp_mode);
> - intel_ring_advance(ring);
> -
> - dev_priv->relative_constants_mode = instp_mode;
> - }
> -
> - exec_start = params->batch_obj_vm_offset +
> -      args->batch_start_offset;
> -
> - ret = engine->emit_bb_start(params->request,
> -     exec_start, args->batch_len,
> -     params->dispatch_flags);
> - if (ret)
> - return ret;
> -
> - trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
> -
> - i915_gem_execbuffer_move_to_active(vmas, params->request);
> -
> - return 0;
> -}
> -
>  void intel_execlists_cancel_requests(struct intel_engine_cs *engine)
>  {
>   struct drm_i915_gem_request *req, *tmp;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 212ee7c43438..0f9c9925985c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -95,10 +95,6 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx,
>  /* Execlists */
>  int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv,
>       int enable_execlists);
> -struct i915_execbuffer_params;
> -int intel_execlists_submission(struct i915_execbuffer_params *params,
> -        struct drm_i915_gem_execbuffer2 *args,
> -        struct list_head *vmas);
>
>  void intel_execlists_cancel_requests(struct intel_engine_cs *engine);
>
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation