diff mbox

drm/i915: Remove i915_execbuffer_params

Message ID 1486118024-4146-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin Feb. 3, 2017, 10:33 a.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Tidy i915_gem_do_execbuffer by removing the structure which
is under no plans to be used any longer.

This improves the redability of the code, decreases lines
of source and shrinks the binary.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 114 +++++++++++------------------
 1 file changed, 43 insertions(+), 71 deletions(-)

Comments

Chris Wilson Feb. 3, 2017, 10:46 a.m. UTC | #1 
On Fri, Feb 03, 2017 at 10:33:44AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Tidy i915_gem_do_execbuffer by removing the structure which
> is under no plans to be used any longer.
> 
> This improves the redability of the code, decreases lines
> of source and shrinks the binary.

If you put them alongside the params we pass along, it is even better.

https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=prescheduler&id=33dc0f3f6d065cf76abd9ad9778c182519fb49a8

is what I've been trying to get in for the year or so.
-Chris
Joonas Lahtinen Feb. 3, 2017, 12:45 p.m. UTC | #2 
On pe, 2017-02-03 at 10:46 +0000, Chris Wilson wrote:
> On Fri, Feb 03, 2017 at 10:33:44AM +0000, Tvrtko Ursulin wrote:
> > 
> > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Tidy i915_gem_do_execbuffer by removing the structure which
> > is under no plans to be used any longer.
> > 
> > This improves the redability of the code, decreases lines
> > of source and shrinks the binary.
> 
> If you put them alongside the params we pass along, it is even better.
> 
> https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=prescheduler&id=33dc0f3f6d065cf76abd9ad9778c182519fb49a8
> 
> is what I've been trying to get in for the year or so.

Well, the remaining bits could be moved like that, yep.

But I'd do it as a second patch on top of this, for easier review.

Regards, Joonas
Tvrtko Ursulin Feb. 8, 2017, 2:47 p.m. UTC | #3 
On 03/02/2017 12:45, Joonas Lahtinen wrote:
> On pe, 2017-02-03 at 10:46 +0000, Chris Wilson wrote:
>> On Fri, Feb 03, 2017 at 10:33:44AM +0000, Tvrtko Ursulin wrote:
>>>
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Tidy i915_gem_do_execbuffer by removing the structure which
>>> is under no plans to be used any longer.
>>>
>>> This improves the redability of the code, decreases lines
>>> of source and shrinks the binary.
>>
>> If you put them alongside the params we pass along, it is even better.
>>
>> https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=prescheduler&id=33dc0f3f6d065cf76abd9ad9778c182519fb49a8
>>
>> is what I've been trying to get in for the year or so.
>
> Well, the remaining bits could be moved like that, yep.
>
> But I'd do it as a second patch on top of this, for easier review.

I was thinking the same. If nothing else every time I stumble upon that 
poorly aligned params assignment block I feel bad so that was the 
motivation to clean it up.

Regards,

Tvrtko
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a40ade6d1c16..b5f55bf858d3 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -49,17 +49,6 @@ 
 
 #define BATCH_OFFSET_BIAS (256*1024)
 
-struct i915_execbuffer_params {
-	struct drm_device               *dev;
-	struct drm_file                 *file;
-	struct i915_vma			*batch;
-	u32				dispatch_flags;
-	u32				args_batch_start_offset;
-	struct intel_engine_cs          *engine;
-	struct i915_gem_context         *ctx;
-	struct drm_i915_gem_request     *request;
-};
-
 struct eb_vmas {
 	struct drm_i915_private *i915;
 	struct list_head vmas;
@@ -1408,21 +1397,22 @@  i915_gem_execbuffer_parse(struct intel_engine_cs *engine,
 }
 
 static int
-execbuf_submit(struct i915_execbuffer_params *params,
-	       struct drm_i915_gem_execbuffer2 *args,
+execbuf_submit(struct drm_i915_gem_request *req, u32 batch_start,
+	       u32 dispatch_flags, struct drm_i915_gem_execbuffer2 *args,
 	       struct list_head *vmas)
 {
-	struct drm_i915_private *dev_priv = params->request->i915;
+	struct drm_i915_private *dev_priv = req->i915;
+	struct intel_engine_cs *engine = req->engine;
 	u64 exec_start, exec_len;
 	int instp_mode;
 	u32 instp_mask;
 	int ret;
 
-	ret = i915_gem_execbuffer_move_to_gpu(params->request, vmas);
+	ret = i915_gem_execbuffer_move_to_gpu(req, vmas);
 	if (ret)
 		return ret;
 
-	ret = i915_switch_context(params->request);
+	ret = i915_switch_context(req);
 	if (ret)
 		return ret;
 
@@ -1432,25 +1422,25 @@  execbuf_submit(struct i915_execbuffer_params *params,
 	case I915_EXEC_CONSTANTS_REL_GENERAL:
 	case I915_EXEC_CONSTANTS_ABSOLUTE:
 	case I915_EXEC_CONSTANTS_REL_SURFACE:
-		if (instp_mode != 0 && params->engine->id != RCS) {
+		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 (INTEL_INFO(dev_priv)->gen < 4) {
+			if (INTEL_GEN(dev_priv) < 4) {
 				DRM_DEBUG("no rel constants on pre-gen4\n");
 				return -EINVAL;
 			}
 
-			if (INTEL_INFO(dev_priv)->gen > 5 &&
+			if (INTEL_GEN(dev_priv) > 5 &&
 			    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 */
-			if (INTEL_INFO(dev_priv)->gen >= 6)
+			if (INTEL_GEN(dev_priv) >= 6)
 				instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
 		}
 		break;
@@ -1459,11 +1449,11 @@  execbuf_submit(struct i915_execbuffer_params *params,
 		return -EINVAL;
 	}
 
-	if (params->engine->id == RCS &&
+	if (engine->id == RCS &&
 	    instp_mode != dev_priv->relative_constants_mode) {
-		struct intel_ring *ring = params->request->ring;
+		struct intel_ring *ring = req->ring;
 
-		ret = intel_ring_begin(params->request, 4);
+		ret = intel_ring_begin(req, 4);
 		if (ret)
 			return ret;
 
@@ -1477,27 +1467,24 @@  execbuf_submit(struct i915_execbuffer_params *params,
 	}
 
 	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
-		ret = i915_reset_gen7_sol_offsets(params->request);
+		ret = i915_reset_gen7_sol_offsets(req);
 		if (ret)
 			return ret;
 	}
 
 	exec_len   = args->batch_len;
-	exec_start = params->batch->node.start +
-		     params->args_batch_start_offset;
+	exec_start = req->batch->node.start + batch_start;
 
 	if (exec_len == 0)
-		exec_len = params->batch->size - params->args_batch_start_offset;
+		exec_len = req->batch->size - batch_start;
 
-	ret = params->engine->emit_bb_start(params->request,
-					    exec_start, exec_len,
-					    params->dispatch_flags);
+	ret = engine->emit_bb_start(req, exec_start, exec_len, dispatch_flags);
 	if (ret)
 		return ret;
 
-	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
+	trace_i915_gem_ring_dispatch(req, dispatch_flags);
 
-	i915_gem_execbuffer_move_to_active(vmas, params->request);
+	i915_gem_execbuffer_move_to_active(vmas, req);
 
 	return 0;
 }
@@ -1591,10 +1578,10 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	struct intel_engine_cs *engine;
 	struct i915_gem_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;
+	struct drm_i915_gem_request *req;
+	struct i915_vma *batch;
 	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
-	u32 dispatch_flags;
+	u32 dispatch_flags, batch_start;
 	struct dma_fence *in_fence = NULL;
 	struct sync_file *out_fence = NULL;
 	int out_fence_fd = -1;
@@ -1684,8 +1671,6 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	else
 		vm = &ggtt->base;
 
-	memset(&params_master, 0x00, sizeof(params_master));
-
 	eb = eb_create(dev_priv, args);
 	if (eb == NULL) {
 		i915_gem_context_put(ctx);
@@ -1700,7 +1685,7 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		goto err;
 
 	/* take note of the batch buffer before we might reorder the lists */
-	params->batch = eb_get_batch(eb);
+	batch = eb_get_batch(eb);
 
 	/* Move the objects en-masse into the GTT, evicting if necessary. */
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
@@ -1724,24 +1709,24 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	}
 
 	/* Set the pending read domains for the batch buffer to COMMAND */
-	if (params->batch->obj->base.pending_write_domain) {
+	if (batch->obj->base.pending_write_domain) {
 		DRM_DEBUG("Attempting to use self-modifying batch buffer\n");
 		ret = -EINVAL;
 		goto err;
 	}
-	if (args->batch_start_offset > params->batch->size ||
-	    args->batch_len > params->batch->size - args->batch_start_offset) {
+	if (args->batch_start_offset > batch->size ||
+	    args->batch_len > batch->size - args->batch_start_offset) {
 		DRM_DEBUG("Attempting to use out-of-bounds batch\n");
 		ret = -EINVAL;
 		goto err;
 	}
 
-	params->args_batch_start_offset = args->batch_start_offset;
+	batch_start = args->batch_start_offset;
 	if (engine->needs_cmd_parser && args->batch_len) {
 		struct i915_vma *vma;
 
 		vma = i915_gem_execbuffer_parse(engine, &shadow_exec_entry,
-						params->batch->obj,
+						batch->obj,
 						eb,
 						args->batch_start_offset,
 						args->batch_len,
@@ -1762,18 +1747,18 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			 * command parser has accepted.
 			 */
 			dispatch_flags |= I915_DISPATCH_SECURE;
-			params->args_batch_start_offset = 0;
-			params->batch = vma;
+			batch_start = 0;
+			batch = vma;
 		}
 	}
 
-	params->batch->obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
+	batch->obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
 
 	/* snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure
 	 * batch" bit. Hence we need to pin secure batches into the global gtt.
 	 * hsw should have this fixed, but bdw mucks it up again. */
 	if (dispatch_flags & I915_DISPATCH_SECURE) {
-		struct drm_i915_gem_object *obj = params->batch->obj;
+		struct drm_i915_gem_object *obj = batch->obj;
 		struct i915_vma *vma;
 
 		/*
@@ -1792,25 +1777,24 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			goto err;
 		}
 
-		params->batch = vma;
+		batch = vma;
 	}
 
 	/* Allocate a request for this batch buffer nice and early. */
-	params->request = i915_gem_request_alloc(engine, ctx);
-	if (IS_ERR(params->request)) {
-		ret = PTR_ERR(params->request);
+	req = i915_gem_request_alloc(engine, ctx);
+	if (IS_ERR(req)) {
+		ret = PTR_ERR(req);
 		goto err_batch_unpin;
 	}
 
 	if (in_fence) {
-		ret = i915_gem_request_await_dma_fence(params->request,
-						       in_fence);
+		ret = i915_gem_request_await_dma_fence(req, in_fence);
 		if (ret < 0)
 			goto err_request;
 	}
 
 	if (out_fence_fd != -1) {
-		out_fence = sync_file_create(&params->request->fence);
+		out_fence = sync_file_create(&req->fence);
 		if (!out_fence) {
 			ret = -ENOMEM;
 			goto err_request;
@@ -1823,27 +1807,15 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	 * inactive_list and lose its active reference. Hence we do not need
 	 * to explicitly hold another reference here.
 	 */
-	params->request->batch = params->batch;
+	req->batch = batch;
 
-	ret = i915_gem_request_add_to_client(params->request, file);
+	ret = i915_gem_request_add_to_client(req, file);
 	if (ret)
 		goto err_request;
 
-	/*
-	 * 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->engine                    = engine;
-	params->dispatch_flags          = dispatch_flags;
-	params->ctx                     = ctx;
-
-	ret = execbuf_submit(params, args, &eb->vmas);
+	ret = execbuf_submit(req, batch_start, dispatch_flags, args, &eb->vmas);
 err_request:
-	__i915_add_request(params->request, ret == 0);
+	__i915_add_request(req, ret == 0);
 	if (out_fence) {
 		if (ret == 0) {
 			fd_install(out_fence_fd, out_fence->file);
@@ -1863,7 +1835,7 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	 * active.
 	 */
 	if (dispatch_flags & I915_DISPATCH_SECURE)
-		i915_vma_unpin(params->batch);
+		i915_vma_unpin(batch);
 err:
 	/* the request owns the ref now */
 	i915_gem_context_put(ctx);