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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx