All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/10] do_execbuffer tidy
@ 2017-01-31 13:15 Tvrtko Ursulin
  2017-01-31 13:15 ` [PATCH 01/10] drm/i915: Tidy i915_gem_do_execbuffer Tvrtko Ursulin
                   ` (11 more replies)
  0 siblings, 12 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-01-31 13:15 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I've noticed a few opportunities to improve the readability of this functions
and then kept spotting more and more which can be removed or compacted.

Eventually ended up with removing i915_execbuffer_params completely. But I think
it's OK since the plan between when it was added to now changed.

Overall reduction in source and binary, hopefully for tidier code.

         text    data     bss     dec     hex filename
      1085466   26398    2628 1114492  11017c i915.ko.0
      1085290   26398    2628 1114316  1100cc i915.ko.1

Tvrtko Ursulin (10):
  drm/i915: Tidy i915_gem_do_execbuffer
  drm/i915: Drop some unused fields from i915_execbuffer_params
  drm/i915: Tidy execbuf_submit
  drm/i915: Remove batch field from i915_execbuffer_params
  drm/i915: Nuke i915_execbuffer_params
  drm/i915: Remove some single use locals i915_gem_do_execbuffer
  drm/i915: Drop unused engine parameter from i915_gem_validate_context
  drm/i915: eb_engine_select only needs flags
  drm/i915: Pass file_priv to eb_select_engine
  drm/i915: Remove some unecessary line breaks

 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 159 +++++++++++------------------
 1 file changed, 60 insertions(+), 99 deletions(-)

-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 01/10] drm/i915: Tidy i915_gem_do_execbuffer
  2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
@ 2017-01-31 13:15 ` Tvrtko Ursulin
  2017-02-01  7:14   ` Joonas Lahtinen
  2017-01-31 13:15 ` [PATCH 02/10] drm/i915: Drop some unused fields from i915_execbuffer_params Tvrtko Ursulin
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-01-31 13:15 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Instead of sprinkling around usage and initialization of
i915_execbuffer_params we can consolidate it just before
execbuf_submit for maintability and readability.

That way we can also drop the memset since it becomes
easy to spot we initialize all the fields.

     text    data     bss     dec     hex filename
  1085466   26398    2628 1114492  11017c i915.ko.0
  1085402   26398    2628 1114428  11013c i915.ko.1

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 87 +++++++++++++++---------------
 1 file changed, 44 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 91c2393199a3..ee7b7bd17e29 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -50,14 +50,14 @@
 #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 drm_device	    *dev;
+	struct drm_file		    *file;
+	struct i915_vma		    *batch;
+	u32			    dispatch_flags;
+	u32		    	    batch_start;
+	struct intel_engine_cs	    *engine;
+	struct i915_gem_context	    *ctx;
+	struct drm_i915_gem_request *request;
 };
 
 struct eb_vmas {
@@ -1484,11 +1484,10 @@ execbuf_submit(struct i915_execbuffer_params *params,
 	}
 
 	exec_len   = args->batch_len;
-	exec_start = params->batch->node.start +
-		     params->args_batch_start_offset;
+	exec_start = params->batch->node.start + params->batch_start;
 
 	if (exec_len == 0)
-		exec_len = params->batch->size - params->args_batch_start_offset;
+		exec_len = params->batch->size - params->batch_start;
 
 	ret = params->engine->emit_bb_start(params->request,
 					    exec_start, exec_len,
@@ -1592,8 +1591,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;
+	u32 batch_start_offset;
+	struct i915_execbuffer_params params;
 	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
 	u32 dispatch_flags;
 	struct dma_fence *in_fence = NULL;
@@ -1685,8 +1686,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);
@@ -1701,7 +1700,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;
@@ -1725,24 +1724,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_offset = 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,
@@ -1763,18 +1762,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_offset = 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;
 
 		/*
@@ -1793,25 +1792,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;
@@ -1824,9 +1822,9 @@ 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;
 
@@ -1836,15 +1834,18 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	 * 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);
+	params.dev	      = dev;
+	params.file	      = file;
+	params.engine	      = engine;
+	params.dispatch_flags = dispatch_flags;
+	params.ctx	      = ctx;
+	params.batch	      = batch;
+	params.batch_start    = batch_start_offset;
+	params.request	      = req;
+
+	ret = execbuf_submit(&params, 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);
@@ -1864,7 +1865,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);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 02/10] drm/i915: Drop some unused fields from i915_execbuffer_params
  2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
  2017-01-31 13:15 ` [PATCH 01/10] drm/i915: Tidy i915_gem_do_execbuffer Tvrtko Ursulin
@ 2017-01-31 13:15 ` Tvrtko Ursulin
  2017-02-01  7:20   ` Joonas Lahtinen
  2017-01-31 13:15 ` [PATCH 03/10] drm/i915: Tidy execbuf_submit Tvrtko Ursulin
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-01-31 13:15 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

dev, file and ctx are unused.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index ee7b7bd17e29..2991751516f8 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -50,13 +50,10 @@
 #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		    	    batch_start;
 	struct intel_engine_cs	    *engine;
-	struct i915_gem_context	    *ctx;
 	struct drm_i915_gem_request *request;
 };
 
@@ -1834,11 +1831,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	 * 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;
 	params.batch	      = batch;
 	params.batch_start    = batch_start_offset;
 	params.request	      = req;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 03/10] drm/i915: Tidy execbuf_submit
  2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
  2017-01-31 13:15 ` [PATCH 01/10] drm/i915: Tidy i915_gem_do_execbuffer Tvrtko Ursulin
  2017-01-31 13:15 ` [PATCH 02/10] drm/i915: Drop some unused fields from i915_execbuffer_params Tvrtko Ursulin
@ 2017-01-31 13:15 ` Tvrtko Ursulin
  2017-02-01  7:19   ` Joonas Lahtinen
  2017-01-31 13:15 ` [PATCH 04/10] drm/i915: Remove batch field from i915_execbuffer_params Tvrtko Ursulin
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-01-31 13:15 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Use a local variable for storing the request and engine
and at the same time drop the engine field from
i915_execbuffer_params since it is available from the
request.

     text    data     bss     dec     hex filename
  1085402   26398    2628 1114428  11013c i915.ko.0
  1085354   26398    2628 1114380  11010c i915.ko.1

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 35 +++++++++++++++---------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 2991751516f8..119322eb9897 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -53,7 +53,6 @@ struct i915_execbuffer_params {
 	struct i915_vma		    *batch;
 	u32			    dispatch_flags;
 	u32		    	    batch_start;
-	struct intel_engine_cs	    *engine;
 	struct drm_i915_gem_request *request;
 };
 
@@ -1410,17 +1409,19 @@ 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;
+	struct drm_i915_gem_request *req = params->request;
+	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;
 
@@ -1430,25 +1431,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;
@@ -1457,11 +1458,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;
 
@@ -1475,7 +1476,7 @@ 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;
 	}
@@ -1486,15 +1487,14 @@ execbuf_submit(struct i915_execbuffer_params *params,
 	if (exec_len == 0)
 		exec_len = params->batch->size - params->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,
+				    params->dispatch_flags);
 	if (ret)
 		return ret;
 
-	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
+	trace_i915_gem_ring_dispatch(req, params->dispatch_flags);
 
-	i915_gem_execbuffer_move_to_active(vmas, params->request);
+	i915_gem_execbuffer_move_to_active(vmas, req);
 
 	return 0;
 }
@@ -1831,7 +1831,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	 * kept around beyond the duration of the IOCTL once the GPU
 	 * scheduler arrives.
 	 */
-	params.engine	      = engine;
 	params.dispatch_flags = dispatch_flags;
 	params.batch	      = batch;
 	params.batch_start    = batch_start_offset;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 04/10] drm/i915: Remove batch field from i915_execbuffer_params
  2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
                   ` (2 preceding siblings ...)
  2017-01-31 13:15 ` [PATCH 03/10] drm/i915: Tidy execbuf_submit Tvrtko Ursulin
@ 2017-01-31 13:15 ` Tvrtko Ursulin
  2017-01-31 13:15 ` [PATCH 05/10] drm/i915: Nuke i915_execbuffer_params Tvrtko Ursulin
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-01-31 13:15 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Also redundant since it is stored in the request.

     text    data     bss     dec     hex filename
  1085354   26398    2628 1114380  11010c i915.ko.0
  1085338   26398    2628 1114364  1100fc i915.ko.1

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 119322eb9897..afbb56196162 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -50,7 +50,6 @@
 #define BATCH_OFFSET_BIAS (256*1024)
 
 struct i915_execbuffer_params {
-	struct i915_vma		    *batch;
 	u32			    dispatch_flags;
 	u32		    	    batch_start;
 	struct drm_i915_gem_request *request;
@@ -1482,10 +1481,10 @@ execbuf_submit(struct i915_execbuffer_params *params,
 	}
 
 	exec_len   = args->batch_len;
-	exec_start = params->batch->node.start + params->batch_start;
+	exec_start = req->batch->node.start + params->batch_start;
 
 	if (exec_len == 0)
-		exec_len = params->batch->size - params->batch_start;
+		exec_len = req->batch->size - params->batch_start;
 
 	ret = engine->emit_bb_start(req, exec_start, exec_len,
 				    params->dispatch_flags);
@@ -1832,7 +1831,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	 * scheduler arrives.
 	 */
 	params.dispatch_flags = dispatch_flags;
-	params.batch	      = batch;
 	params.batch_start    = batch_start_offset;
 	params.request	      = req;
 
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 05/10] drm/i915: Nuke i915_execbuffer_params
  2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
                   ` (3 preceding siblings ...)
  2017-01-31 13:15 ` [PATCH 04/10] drm/i915: Remove batch field from i915_execbuffer_params Tvrtko Ursulin
@ 2017-01-31 13:15 ` Tvrtko Ursulin
  2017-01-31 13:15 ` [PATCH 06/10] drm/i915: Remove some single use locals i915_gem_do_execbuffer Tvrtko Ursulin
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-01-31 13:15 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Now that it only contains three parameters we can pass them
directly to execbuf_submit just as well.

No effect on generated binary, just a source reduction.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 39 ++++++++----------------------
 1 file changed, 10 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index afbb56196162..82e74db5923b 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -49,12 +49,6 @@
 
 #define BATCH_OFFSET_BIAS (256*1024)
 
-struct i915_execbuffer_params {
-	u32			    dispatch_flags;
-	u32		    	    batch_start;
-	struct drm_i915_gem_request *request;
-};
-
 struct eb_vmas {
 	struct drm_i915_private *i915;
 	struct list_head vmas;
@@ -1404,11 +1398,10 @@ 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_gem_request *req = params->request;
 	struct drm_i915_private *dev_priv = req->i915;
 	struct intel_engine_cs *engine = req->engine;
 	u64 exec_start, exec_len;
@@ -1481,17 +1474,16 @@ execbuf_submit(struct i915_execbuffer_params *params,
 	}
 
 	exec_len   = args->batch_len;
-	exec_start = req->batch->node.start + params->batch_start;
+	exec_start = req->batch->node.start + batch_start;
 
 	if (exec_len == 0)
-		exec_len = req->batch->size - params->batch_start;
+		exec_len = req->batch->size - batch_start;
 
-	ret = engine->emit_bb_start(req, 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(req, params->dispatch_flags);
+	trace_i915_gem_ring_dispatch(req, dispatch_flags);
 
 	i915_gem_execbuffer_move_to_active(vmas, req);
 
@@ -1589,8 +1581,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	struct i915_address_space *vm;
 	struct drm_i915_gem_request *req;
 	struct i915_vma *batch;
-	u32 batch_start_offset;
-	struct i915_execbuffer_params params;
+	u32 batch_start;
 	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
 	u32 dispatch_flags;
 	struct dma_fence *in_fence = NULL;
@@ -1732,7 +1723,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		goto err;
 	}
 
-	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;
 
@@ -1758,7 +1749,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			 * command parser has accepted.
 			 */
 			dispatch_flags |= I915_DISPATCH_SECURE;
-			batch_start_offset = 0;
+			batch_start = 0;
 			batch = vma;
 		}
 	}
@@ -1824,17 +1815,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	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.dispatch_flags = dispatch_flags;
-	params.batch_start    = batch_start_offset;
-	params.request	      = req;
-
-	ret = execbuf_submit(&params, args, &eb->vmas);
+	ret = execbuf_submit(req, batch_start, dispatch_flags, args, &eb->vmas);
 err_request:
 	__i915_add_request(req, ret == 0);
 	if (out_fence) {
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 06/10] drm/i915: Remove some single use locals i915_gem_do_execbuffer
  2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
                   ` (4 preceding siblings ...)
  2017-01-31 13:15 ` [PATCH 05/10] drm/i915: Nuke i915_execbuffer_params Tvrtko Ursulin
@ 2017-01-31 13:15 ` Tvrtko Ursulin
  2017-01-31 13:15 ` [PATCH 07/10] drm/i915: Drop unused engine parameter from i915_gem_validate_context Tvrtko Ursulin
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-01-31 13:15 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Remove ctx_id, ggtt and vm since they are single use.

     text    data     bss     dec     hex filename
  1085338   26398    2628 1114364  1100fc i915.ko.0
  1085290   26398    2628 1114316  1100cc i915.ko.1

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 82e74db5923b..57ae6573a37b 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1573,16 +1573,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		       struct drm_i915_gem_exec_object2 *exec)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct eb_vmas *eb;
 	struct drm_i915_gem_exec_object2 shadow_exec_entry;
 	struct intel_engine_cs *engine;
 	struct i915_gem_context *ctx;
-	struct i915_address_space *vm;
 	struct drm_i915_gem_request *req;
 	struct i915_vma *batch;
 	u32 batch_start;
-	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
 	u32 dispatch_flags;
 	struct dma_fence *in_fence = NULL;
 	struct sync_file *out_fence = NULL;
@@ -1659,7 +1656,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (ret)
 		goto pre_mutex_err;
 
-	ctx = i915_gem_validate_context(dev, file, engine, ctx_id);
+	ctx = i915_gem_validate_context(dev, file, engine,
+					i915_execbuffer2_get_context_id(*args));
 	if (IS_ERR(ctx)) {
 		mutex_unlock(&dev->struct_mutex);
 		ret = PTR_ERR(ctx);
@@ -1668,11 +1666,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	i915_gem_context_get(ctx);
 
-	if (ctx->ppgtt)
-		vm = &ctx->ppgtt->base;
-	else
-		vm = &ggtt->base;
-
 	eb = eb_create(dev_priv, args);
 	if (eb == NULL) {
 		i915_gem_context_put(ctx);
@@ -1682,7 +1675,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	}
 
 	/* Look up object handles */
-	ret = eb_lookup_vmas(eb, exec, args, vm, file);
+	ret = eb_lookup_vmas(eb, exec, args, ctx->ppgtt ? &ctx->ppgtt->base :
+			     &dev_priv->ggtt.base, file);
 	if (ret)
 		goto err;
 
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 07/10] drm/i915: Drop unused engine parameter from i915_gem_validate_context
  2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
                   ` (5 preceding siblings ...)
  2017-01-31 13:15 ` [PATCH 06/10] drm/i915: Remove some single use locals i915_gem_do_execbuffer Tvrtko Ursulin
@ 2017-01-31 13:15 ` Tvrtko Ursulin
  2017-01-31 13:15 ` [PATCH 08/10] drm/i915: eb_engine_select only needs flags Tvrtko Ursulin
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-01-31 13:15 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 57ae6573a37b..51cf3ff3e21d 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1222,7 +1222,7 @@ validate_exec_list(struct drm_device *dev,
 
 static struct i915_gem_context *
 i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
-			  struct intel_engine_cs *engine, const u32 ctx_id)
+			  const u32 ctx_id)
 {
 	struct i915_gem_context *ctx;
 
@@ -1656,7 +1656,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (ret)
 		goto pre_mutex_err;
 
-	ctx = i915_gem_validate_context(dev, file, engine,
+	ctx = i915_gem_validate_context(dev, file,
 					i915_execbuffer2_get_context_id(*args));
 	if (IS_ERR(ctx)) {
 		mutex_unlock(&dev->struct_mutex);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 08/10] drm/i915: eb_engine_select only needs flags
  2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
                   ` (6 preceding siblings ...)
  2017-01-31 13:15 ` [PATCH 07/10] drm/i915: Drop unused engine parameter from i915_gem_validate_context Tvrtko Ursulin
@ 2017-01-31 13:15 ` Tvrtko Ursulin
  2017-01-31 13:15 ` [PATCH 09/10] drm/i915: Pass file_priv to eb_select_engine Tvrtko Ursulin
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-01-31 13:15 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Not the whole args struct needs to be passed in.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 51cf3ff3e21d..ae94cc27c9ba 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1519,11 +1519,10 @@ static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
 };
 
 static struct intel_engine_cs *
-eb_select_engine(struct drm_i915_private *dev_priv,
-		 struct drm_file *file,
-		 struct drm_i915_gem_execbuffer2 *args)
+eb_select_engine(struct drm_i915_private *dev_priv, struct drm_file *file,
+		 u64 flags)
 {
-	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
+	unsigned int user_ring_id = flags & I915_EXEC_RING_MASK;
 	struct intel_engine_cs *engine;
 
 	if (user_ring_id > I915_USER_RINGS) {
@@ -1532,14 +1531,14 @@ eb_select_engine(struct drm_i915_private *dev_priv,
 	}
 
 	if ((user_ring_id != I915_EXEC_BSD) &&
-	    ((args->flags & I915_EXEC_BSD_MASK) != 0)) {
+	    ((flags & I915_EXEC_BSD_MASK) != 0)) {
 		DRM_DEBUG("execbuf with non bsd ring but with invalid "
-			  "bsd dispatch flags: %d\n", (int)(args->flags));
+			  "bsd dispatch flags: %d\n", (int)(flags));
 		return NULL;
 	}
 
 	if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) {
-		unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
+		unsigned int bsd_idx = flags & I915_EXEC_BSD_MASK;
 
 		if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
 			bsd_idx = gen8_dispatch_bsd_engine(dev_priv, file);
@@ -1604,7 +1603,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (args->flags & I915_EXEC_IS_PINNED)
 		dispatch_flags |= I915_DISPATCH_PINNED;
 
-	engine = eb_select_engine(dev_priv, file, args);
+	engine = eb_select_engine(dev_priv, file, args->flags);
 	if (!engine)
 		return -EINVAL;
 
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 09/10] drm/i915: Pass file_priv to eb_select_engine
  2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
                   ` (7 preceding siblings ...)
  2017-01-31 13:15 ` [PATCH 08/10] drm/i915: eb_engine_select only needs flags Tvrtko Ursulin
@ 2017-01-31 13:15 ` Tvrtko Ursulin
  2017-01-31 13:15 ` [PATCH 10/10] drm/i915: Remove some unecessary line breaks Tvrtko Ursulin
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-01-31 13:15 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

file_priv is what it needs to pass on to gen8_dispatch_bsd_engine so
simplify things by passing it straight away.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index ae94cc27c9ba..ff102b39c3b3 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1496,10 +1496,8 @@ execbuf_submit(struct drm_i915_gem_request *req, u32 batch_start,
  */
 static unsigned int
 gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
-			 struct drm_file *file)
+			 struct drm_i915_file_private *file_priv)
 {
-	struct drm_i915_file_private *file_priv = file->driver_priv;
-
 	/* Check whether the file_priv has already selected one ring. */
 	if ((int)file_priv->bsd_engine < 0)
 		file_priv->bsd_engine = atomic_fetch_xor(1,
@@ -1519,8 +1517,8 @@ static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
 };
 
 static struct intel_engine_cs *
-eb_select_engine(struct drm_i915_private *dev_priv, struct drm_file *file,
-		 u64 flags)
+eb_select_engine(struct drm_i915_private *dev_priv,
+		 struct drm_i915_file_private *file_priv, u64 flags)
 {
 	unsigned int user_ring_id = flags & I915_EXEC_RING_MASK;
 	struct intel_engine_cs *engine;
@@ -1541,7 +1539,7 @@ eb_select_engine(struct drm_i915_private *dev_priv, struct drm_file *file,
 		unsigned int bsd_idx = flags & I915_EXEC_BSD_MASK;
 
 		if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
-			bsd_idx = gen8_dispatch_bsd_engine(dev_priv, file);
+			bsd_idx = gen8_dispatch_bsd_engine(dev_priv, file_priv);
 		} else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
 			   bsd_idx <= I915_EXEC_BSD_RING2) {
 			bsd_idx >>= I915_EXEC_BSD_SHIFT;
@@ -1603,7 +1601,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (args->flags & I915_EXEC_IS_PINNED)
 		dispatch_flags |= I915_DISPATCH_PINNED;
 
-	engine = eb_select_engine(dev_priv, file, args->flags);
+	engine = eb_select_engine(dev_priv, file->driver_priv, args->flags);
 	if (!engine)
 		return -EINVAL;
 
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 10/10] drm/i915: Remove some unecessary line breaks
  2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
                   ` (8 preceding siblings ...)
  2017-01-31 13:15 ` [PATCH 09/10] drm/i915: Pass file_priv to eb_select_engine Tvrtko Ursulin
@ 2017-01-31 13:15 ` Tvrtko Ursulin
  2017-01-31 13:20 ` [PATCH 00/10] do_execbuffer tidy Chris Wilson
  2017-01-31 15:24 ` ✗ Fi.CI.BAT: warning for " Patchwork
  11 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-01-31 13:15 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Just spotted a few lines which fit in 80 chars and were split.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index ff102b39c3b3..18312959f961 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1066,8 +1066,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 		goto err;
 
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
-	ret = i915_gem_execbuffer_reserve(engine, &eb->vmas, ctx,
-					  &need_relocs);
+	ret = i915_gem_execbuffer_reserve(engine, &eb->vmas, ctx, &need_relocs);
 	if (ret)
 		goto err;
 
@@ -1682,8 +1681,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	/* Move the objects en-masse into the GTT, evicting if necessary. */
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
-	ret = i915_gem_execbuffer_reserve(engine, &eb->vmas, ctx,
-					  &need_relocs);
+	ret = i915_gem_execbuffer_reserve(engine, &eb->vmas, ctx, &need_relocs);
 	if (ret)
 		goto err;
 
@@ -1693,8 +1691,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (ret) {
 		if (ret == -EFAULT) {
 			ret = i915_gem_execbuffer_relocate_slow(dev, args, file,
-								engine,
-								eb, exec, ctx);
+								engine, eb,
+								exec, ctx);
 			BUG_ON(!mutex_is_locked(&dev->struct_mutex));
 		}
 		if (ret)
@@ -1719,8 +1717,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		struct i915_vma *vma;
 
 		vma = i915_gem_execbuffer_parse(engine, &shadow_exec_entry,
-						batch->obj,
-						eb,
+						batch->obj, eb,
 						args->batch_start_offset,
 						args->batch_len,
 						drm_is_current_master(file));
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 00/10] do_execbuffer tidy
  2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
                   ` (9 preceding siblings ...)
  2017-01-31 13:15 ` [PATCH 10/10] drm/i915: Remove some unecessary line breaks Tvrtko Ursulin
@ 2017-01-31 13:20 ` Chris Wilson
  2017-01-31 15:24 ` ✗ Fi.CI.BAT: warning for " Patchwork
  11 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2017-01-31 13:20 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel-gfx

On Tue, Jan 31, 2017 at 01:15:36PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> I've noticed a few opportunities to improve the readability of this functions
> and then kept spotting more and more which can be removed or compacted.
> 
> Eventually ended up with removing i915_execbuffer_params completely. But I think
> it's OK since the plan between when it was added to now changed.

Most of it looks like churn duplicating patches I've already sent. :(
We have an open performance regression bug (that has only gotten worse
with multiple timelines) that I really would like to see some progress
on.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* ✗ Fi.CI.BAT: warning for do_execbuffer tidy
  2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
                   ` (10 preceding siblings ...)
  2017-01-31 13:20 ` [PATCH 00/10] do_execbuffer tidy Chris Wilson
@ 2017-01-31 15:24 ` Patchwork
  11 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2017-01-31 15:24 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

== Series Details ==

Series: do_execbuffer tidy
URL   : https://patchwork.freedesktop.org/series/18838/
State : warning

== Summary ==

Series 18838v1 do_execbuffer tidy
https://patchwork.freedesktop.org/api/1.0/series/18838/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> DMESG-WARN (fi-snb-2520m)

fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:247  pass:225  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:78   pass:65   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:224  dwarn:0   dfail:0   fail:2   skip:21 
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:222  dwarn:4   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:247  pass:215  dwarn:1   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

a0695bf4e1c12de4863e775747fe850b92661dc6 drm-tip: 2017y-01m-31d-13h-38m-55s UTC integration manifest
db7e420 drm/i915: Remove some unecessary line breaks
c87579b drm/i915: Pass file_priv to eb_select_engine
a436b4e drm/i915: eb_engine_select only needs flags
c17110c drm/i915: Drop unused engine parameter from i915_gem_validate_context
a07e08f drm/i915: Remove some single use locals i915_gem_do_execbuffer
d6e08ae drm/i915: Nuke i915_execbuffer_params
51324f0 drm/i915: Remove batch field from i915_execbuffer_params
84b1d2e drm/i915: Tidy execbuf_submit
eb4ad7b drm/i915: Drop some unused fields from i915_execbuffer_params
5029d71 drm/i915: Tidy i915_gem_do_execbuffer

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3654/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 01/10] drm/i915: Tidy i915_gem_do_execbuffer
  2017-01-31 13:15 ` [PATCH 01/10] drm/i915: Tidy i915_gem_do_execbuffer Tvrtko Ursulin
@ 2017-02-01  7:14   ` Joonas Lahtinen
  0 siblings, 0 replies; 17+ messages in thread
From: Joonas Lahtinen @ 2017-02-01  7:14 UTC (permalink / raw)
  To: Tvrtko Ursulin, Intel-gfx

On ti, 2017-01-31 at 13:15 +0000, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Instead of sprinkling around usage and initialization of
> i915_execbuffer_params we can consolidate it just before
> execbuf_submit for maintability and readability.
> 
> That way we can also drop the memset since it becomes
> easy to spot we initialize all the fields.
> 
>      text    data     bss     dec     hex filename
>   1085466   26398    2628 1114492  11017c i915.ko.0
>   1085402   26398    2628 1114428  11013c i915.ko.1
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

<SNIP>

> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -50,14 +50,14 @@
>  #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 drm_device	    *dev;
> +	struct drm_file		    *file;
> +	struct i915_vma		    *batch;
> +	u32			    dispatch_flags;
> +	u32		    	    batch_start;
> +	struct intel_engine_cs	    *engine;
> +	struct i915_gem_context	    *ctx;
> +	struct drm_i915_gem_request *request;
>  };

You could just drop the pretty spaces totally. Otherwise, when
something gets changed, the whole struct has to be re-indented.

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

Regards, Joonas

-- 
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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 03/10] drm/i915: Tidy execbuf_submit
  2017-01-31 13:15 ` [PATCH 03/10] drm/i915: Tidy execbuf_submit Tvrtko Ursulin
@ 2017-02-01  7:19   ` Joonas Lahtinen
  0 siblings, 0 replies; 17+ messages in thread
From: Joonas Lahtinen @ 2017-02-01  7:19 UTC (permalink / raw)
  To: Tvrtko Ursulin, Intel-gfx

On ti, 2017-01-31 at 13:15 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Use a local variable for storing the request and engine
> and at the same time drop the engine field from
> i915_execbuffer_params since it is available from the
> request.
> 
>      text    data     bss     dec     hex filename
>   1085402   26398    2628 1114428  11013c i915.ko.0
>   1085354   26398    2628 1114380  11010c i915.ko.1
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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

Regards, Joonas
-- 
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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 02/10] drm/i915: Drop some unused fields from i915_execbuffer_params
  2017-01-31 13:15 ` [PATCH 02/10] drm/i915: Drop some unused fields from i915_execbuffer_params Tvrtko Ursulin
@ 2017-02-01  7:20   ` Joonas Lahtinen
  2017-02-01 16:08     ` Tvrtko Ursulin
  0 siblings, 1 reply; 17+ messages in thread
From: Joonas Lahtinen @ 2017-02-01  7:20 UTC (permalink / raw)
  To: Tvrtko Ursulin, Intel-gfx

On ti, 2017-01-31 at 13:15 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> dev, file and ctx are unused.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Could either squash this to the previous commit, or have this before
previous commit, to avoid going through removed code in previous patch
(easier for rebasing etc.)

Regards, Joonas
-- 
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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 02/10] drm/i915: Drop some unused fields from i915_execbuffer_params
  2017-02-01  7:20   ` Joonas Lahtinen
@ 2017-02-01 16:08     ` Tvrtko Ursulin
  0 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-02-01 16:08 UTC (permalink / raw)
  To: Joonas Lahtinen, Tvrtko Ursulin, Intel-gfx


On 01/02/2017 07:20, Joonas Lahtinen wrote:
> On ti, 2017-01-31 at 13:15 +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> dev, file and ctx are unused.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Could either squash this to the previous commit, or have this before
> previous commit, to avoid going through removed code in previous patch
> (easier for rebasing etc.)

Yes I realized that after I wrote it, but was too lazy to reorder. Same 
for the pretty alignment in the first patch. Always when I stumble upon 
those I am 50-50.

Will reorder it, it makes sense.

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2017-02-01 16:08 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-31 13:15 [PATCH 00/10] do_execbuffer tidy Tvrtko Ursulin
2017-01-31 13:15 ` [PATCH 01/10] drm/i915: Tidy i915_gem_do_execbuffer Tvrtko Ursulin
2017-02-01  7:14   ` Joonas Lahtinen
2017-01-31 13:15 ` [PATCH 02/10] drm/i915: Drop some unused fields from i915_execbuffer_params Tvrtko Ursulin
2017-02-01  7:20   ` Joonas Lahtinen
2017-02-01 16:08     ` Tvrtko Ursulin
2017-01-31 13:15 ` [PATCH 03/10] drm/i915: Tidy execbuf_submit Tvrtko Ursulin
2017-02-01  7:19   ` Joonas Lahtinen
2017-01-31 13:15 ` [PATCH 04/10] drm/i915: Remove batch field from i915_execbuffer_params Tvrtko Ursulin
2017-01-31 13:15 ` [PATCH 05/10] drm/i915: Nuke i915_execbuffer_params Tvrtko Ursulin
2017-01-31 13:15 ` [PATCH 06/10] drm/i915: Remove some single use locals i915_gem_do_execbuffer Tvrtko Ursulin
2017-01-31 13:15 ` [PATCH 07/10] drm/i915: Drop unused engine parameter from i915_gem_validate_context Tvrtko Ursulin
2017-01-31 13:15 ` [PATCH 08/10] drm/i915: eb_engine_select only needs flags Tvrtko Ursulin
2017-01-31 13:15 ` [PATCH 09/10] drm/i915: Pass file_priv to eb_select_engine Tvrtko Ursulin
2017-01-31 13:15 ` [PATCH 10/10] drm/i915: Remove some unecessary line breaks Tvrtko Ursulin
2017-01-31 13:20 ` [PATCH 00/10] do_execbuffer tidy Chris Wilson
2017-01-31 15:24 ` ✗ Fi.CI.BAT: warning for " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.