[PATCH] drm/i915: Name the anonymous per-engine context struct

[PATCH] drm/i915: Name the anonymous per-engine context struct

[Tvrtko Ursulin]

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

This anonymous struct was causing a good amount of overly
verbose code. Also, if we name it and cache the pointer locally
when there are multiple accesses to it, not only the code is
more readable, but the compiler manages to generate smaller
binary.

Along the way I also shortened access to dev_priv and eliminated
some unused variables and cached some where I spotted the
opportunity.

Name for the structure, intel_context_engine, and the local
variable name were borrowed from a similar patch by Chris Wilson.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 +-
 drivers/gpu/drm/i915/intel_lrc.c | 96 +++++++++++++++++++++-------------------
 2 files changed, 51 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 00c41a4bde2a..480639c39543 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -840,7 +840,7 @@ struct intel_context {
 	} legacy_hw_ctx;
 
 	/* Execlists */
-	struct {
+	struct intel_context_engine {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
 		int pin_count;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3a23b9549f7b..c61f679a40ee 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -316,16 +316,16 @@ static void
 intel_lr_context_descriptor_update(struct intel_context *ctx,
 				   struct intel_engine_cs *engine)
 {
+	struct intel_context_engine *ce = &ctx->engine[engine->id];
 	uint64_t lrca, desc;
 
-	lrca = ctx->engine[engine->id].lrc_vma->node.start +
-	       LRC_PPHWSP_PN * PAGE_SIZE;
+	lrca = ce->lrc_vma->node.start + LRC_PPHWSP_PN * PAGE_SIZE;
 
-	desc = engine->ctx_desc_template;			   /* bits  0-11 */
+	desc = engine->ctx_desc_template;		   /* bits  0-11 */
 	desc |= lrca;					   /* bits 12-31 */
 	desc |= (lrca >> PAGE_SHIFT) << GEN8_CTX_ID_SHIFT; /* bits 32-51 */
 
-	ctx->engine[engine->id].lrc_desc = desc;
+	ce->lrc_desc = desc;
 }
 
 uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
@@ -361,8 +361,7 @@ static void execlists_elsp_write(struct drm_i915_gem_request *rq0,
 {
 
 	struct intel_engine_cs *engine = rq0->engine;
-	struct drm_device *dev = engine->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = rq0->i915;
 	uint64_t desc[2];
 
 	if (rq1) {
@@ -670,7 +669,8 @@ static int logical_ring_invalidate_all_caches(struct drm_i915_gem_request *req)
 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 intel_engine_cs *engine = req->engine;
+	const unsigned other_rings = ~intel_engine_flag(engine);
 	struct i915_vma *vma;
 	uint32_t flush_domains = 0;
 	bool flush_chipset = false;
@@ -680,7 +680,7 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
 		struct drm_i915_gem_object *obj = vma->obj;
 
 		if (obj->active & other_rings) {
-			ret = i915_gem_object_sync(obj, req->engine, &req);
+			ret = i915_gem_object_sync(obj, engine, &req);
 			if (ret)
 				return ret;
 		}
@@ -782,6 +782,7 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
 	struct intel_ringbuffer *ringbuf = request->ringbuf;
 	struct drm_i915_private *dev_priv = request->i915;
 	struct intel_engine_cs *engine = request->engine;
+	struct intel_context *ctx = request->ctx;
 
 	intel_logical_ring_advance(ringbuf);
 	request->tail = ringbuf->tail;
@@ -799,12 +800,12 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
 	if (intel_engine_stopped(engine))
 		return 0;
 
-	if (engine->last_context != request->ctx) {
+	if (engine->last_context != ctx) {
 		if (engine->last_context)
 			intel_lr_context_unpin(engine->last_context, engine);
-		if (request->ctx != request->i915->kernel_context) {
-			intel_lr_context_pin(request->ctx, engine);
-			engine->last_context = request->ctx;
+		if (ctx != dev_priv->kernel_context) {
+			intel_lr_context_pin(ctx, engine);
+			engine->last_context = ctx;
 		} else {
 			engine->last_context = NULL;
 		}
@@ -949,9 +950,8 @@ 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 = dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(params->dev);
 	struct intel_ringbuffer *ringbuf = params->ctx->engine[engine->id].ringbuf;
 	u64 exec_start;
 	int instp_mode;
@@ -1092,17 +1092,18 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
 				   struct intel_engine_cs *engine)
 {
 	struct drm_device *dev = engine->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
-	struct intel_ringbuffer *ringbuf = ctx->engine[engine->id].ringbuf;
+	struct drm_i915_private *dev_priv = ctx->i915;
+	struct intel_context_engine *ce = &ctx->engine[engine->id];
+	struct drm_i915_gem_object *ctx_obj = ce->state;
+	struct intel_ringbuffer *ringbuf = ce->ringbuf;
 	struct page *lrc_state_page;
 	uint32_t *lrc_reg_state;
 	int ret;
 
-	WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
 	ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN,
-			PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+				    PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
 	if (ret)
 		return ret;
 
@@ -1112,15 +1113,15 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
 		goto unpin_ctx_obj;
 	}
 
-	ret = intel_pin_and_map_ringbuffer_obj(engine->dev, ringbuf);
+	ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
 	if (ret)
 		goto unpin_ctx_obj;
 
-	ctx->engine[engine->id].lrc_vma = i915_gem_obj_to_ggtt(ctx_obj);
+	ce->lrc_vma = i915_gem_obj_to_ggtt(ctx_obj);
 	intel_lr_context_descriptor_update(ctx, engine);
 	lrc_reg_state = kmap(lrc_state_page);
 	lrc_reg_state[CTX_RING_BUFFER_START+1] = ringbuf->vma->node.start;
-	ctx->engine[engine->id].lrc_reg_state = lrc_reg_state;
+	ce->lrc_reg_state = lrc_reg_state;
 	ctx_obj->dirty = true;
 
 	/* Invalidate GuC TLB. */
@@ -1138,9 +1139,10 @@ unpin_ctx_obj:
 static int intel_lr_context_pin(struct intel_context *ctx,
 				struct intel_engine_cs *engine)
 {
+	struct intel_context_engine *ce = &ctx->engine[engine->id];
 	int ret = 0;
 
-	if (ctx->engine[engine->id].pin_count++ == 0) {
+	if (ce->pin_count++ == 0) {
 		ret = intel_lr_context_do_pin(ctx, engine);
 		if (ret)
 			goto reset_pin_count;
@@ -1150,23 +1152,25 @@ static int intel_lr_context_pin(struct intel_context *ctx,
 	return ret;
 
 reset_pin_count:
-	ctx->engine[engine->id].pin_count = 0;
+	ce->pin_count = 0;
 	return ret;
 }
 
 void intel_lr_context_unpin(struct intel_context *ctx,
 			    struct intel_engine_cs *engine)
 {
-	struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
+	struct intel_context_engine *ce = &ctx->engine[engine->id];
+	struct drm_i915_gem_object *ctx_obj = ce->state;
 
-	WARN_ON(!mutex_is_locked(&ctx->i915->dev->struct_mutex));
-	if (--ctx->engine[engine->id].pin_count == 0) {
-		kunmap(kmap_to_page(ctx->engine[engine->id].lrc_reg_state));
-		intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
+	WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
+
+	if (--ce->pin_count == 0) {
+		kunmap(kmap_to_page(ce->lrc_reg_state));
+		intel_unpin_ringbuffer_obj(ce->ringbuf);
 		i915_gem_object_ggtt_unpin(ctx_obj);
-		ctx->engine[engine->id].lrc_vma = NULL;
-		ctx->engine[engine->id].lrc_desc = 0;
-		ctx->engine[engine->id].lrc_reg_state = NULL;
+		ce->lrc_vma = NULL;
+		ce->lrc_desc = 0;
+		ce->lrc_reg_state = NULL;
 
 		i915_gem_context_unreference(ctx);
 	}
@@ -1177,8 +1181,7 @@ static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
 	int ret, i;
 	struct intel_engine_cs *engine = req->engine;
 	struct intel_ringbuffer *ringbuf = req->ringbuf;
-	struct drm_device *dev = engine->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = req->i915;
 	struct i915_workarounds *w = &dev_priv->workarounds;
 
 	if (w->count == 0)
@@ -2513,8 +2516,9 @@ void intel_lr_context_free(struct intel_context *ctx)
 	int i;
 
 	for (i = I915_NUM_ENGINES; --i >= 0; ) {
-		struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf;
-		struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
+		struct intel_context_engine *ce = &ctx->engine[i];
+		struct intel_ringbuffer *ringbuf = ce->ringbuf;
+		struct drm_i915_gem_object *ctx_obj = ce->state;
 
 		if (!ctx_obj)
 			continue;
@@ -2524,7 +2528,7 @@ void intel_lr_context_free(struct intel_context *ctx)
 			i915_gem_object_ggtt_unpin(ctx_obj);
 		}
 
-		WARN_ON(ctx->engine[i].pin_count);
+		WARN_ON(ce->pin_count);
 		intel_ringbuffer_free(ringbuf);
 		drm_gem_object_unreference(&ctx_obj->base);
 	}
@@ -2604,13 +2608,14 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 				    struct intel_engine_cs *engine)
 {
 	struct drm_device *dev = engine->dev;
+	struct intel_context_engine *ce = &ctx->engine[engine->id];
 	struct drm_i915_gem_object *ctx_obj;
 	uint32_t context_size;
 	struct intel_ringbuffer *ringbuf;
 	int ret;
 
 	WARN_ON(ctx->legacy_hw_ctx.rcs_state != NULL);
-	WARN_ON(ctx->engine[engine->id].state);
+	WARN_ON(ce->state);
 
 	context_size = round_up(intel_lr_context_size(engine), 4096);
 
@@ -2635,8 +2640,8 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 		goto error_ringbuf;
 	}
 
-	ctx->engine[engine->id].ringbuf = ringbuf;
-	ctx->engine[engine->id].state = ctx_obj;
+	ce->ringbuf = ringbuf;
+	ce->state = ctx_obj;
 
 	if (ctx != ctx->i915->kernel_context && engine->init_context) {
 		struct drm_i915_gem_request *req;
@@ -2663,8 +2668,8 @@ error_ringbuf:
 	intel_ringbuffer_free(ringbuf);
 error_deref_obj:
 	drm_gem_object_unreference(&ctx_obj->base);
-	ctx->engine[engine->id].ringbuf = NULL;
-	ctx->engine[engine->id].state = NULL;
+	ce->ringbuf = NULL;
+	ce->state = NULL;
 	return ret;
 }
 
@@ -2676,10 +2681,9 @@ void intel_lr_context_reset(struct drm_device *dev,
 	int i;
 
 	for_each_engine(engine, dev_priv, i) {
-		struct drm_i915_gem_object *ctx_obj =
-				ctx->engine[engine->id].state;
-		struct intel_ringbuffer *ringbuf =
-				ctx->engine[engine->id].ringbuf;
+		struct intel_context_engine *ce = &ctx->engine[engine->id];
+		struct drm_i915_gem_object *ctx_obj = ce->state;
+		struct intel_ringbuffer *ringbuf = ce->ringbuf;
 		uint32_t *reg_state;
 		struct page *page;
 
-- 
1.9.1

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

Re: [PATCH] drm/i915: Name the anonymous per-engine context struct

[Chris Wilson]

On Fri, Mar 18, 2016 at 03:00:56PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> This anonymous struct was causing a good amount of overly
> verbose code. Also, if we name it and cache the pointer locally
> when there are multiple accesses to it, not only the code is
> more readable, but the compiler manages to generate smaller
> binary.
> 
> Along the way I also shortened access to dev_priv and eliminated
> some unused variables and cached some where I spotted the
> opportunity.
> 
> Name for the structure, intel_context_engine, and the local
> variable name were borrowed from a similar patch by Chris Wilson.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

Looks like a patch I've been maintaining for an eon, and I think Mika
once did a similar thing.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

If I'm far enough away from it to be impartial?
-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

[PATCH v2] drm/i915: Name the anonymous per-engine context struct

[Tvrtko Ursulin]

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

This anonymous struct was causing a good amount of overly
verbose code. Also, if we name it and cache the pointer locally
when there are multiple accesses to it, not only the code is
more readable, but the compiler manages to generate smaller
binary.

Along the way I also shortened access to dev_priv and eliminated
some unused variables and cached some where I spotted the
opportunity.

Name for the structure, intel_context_engine, and the local
variable name were borrowed from a similar patch by Chris Wilson.

v2: Hate the engine->dev surprises, really do.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 +-
 drivers/gpu/drm/i915/intel_lrc.c | 94 +++++++++++++++++++++-------------------
 2 files changed, 50 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 00c41a4bde2a..480639c39543 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -840,7 +840,7 @@ struct intel_context {
 	} legacy_hw_ctx;
 
 	/* Execlists */
-	struct {
+	struct intel_context_engine {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
 		int pin_count;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3a23b9549f7b..e733795b57e0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -316,16 +316,16 @@ static void
 intel_lr_context_descriptor_update(struct intel_context *ctx,
 				   struct intel_engine_cs *engine)
 {
+	struct intel_context_engine *ce = &ctx->engine[engine->id];
 	uint64_t lrca, desc;
 
-	lrca = ctx->engine[engine->id].lrc_vma->node.start +
-	       LRC_PPHWSP_PN * PAGE_SIZE;
+	lrca = ce->lrc_vma->node.start + LRC_PPHWSP_PN * PAGE_SIZE;
 
-	desc = engine->ctx_desc_template;			   /* bits  0-11 */
+	desc = engine->ctx_desc_template;		   /* bits  0-11 */
 	desc |= lrca;					   /* bits 12-31 */
 	desc |= (lrca >> PAGE_SHIFT) << GEN8_CTX_ID_SHIFT; /* bits 32-51 */
 
-	ctx->engine[engine->id].lrc_desc = desc;
+	ce->lrc_desc = desc;
 }
 
 uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
@@ -361,8 +361,7 @@ static void execlists_elsp_write(struct drm_i915_gem_request *rq0,
 {
 
 	struct intel_engine_cs *engine = rq0->engine;
-	struct drm_device *dev = engine->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = rq0->i915;
 	uint64_t desc[2];
 
 	if (rq1) {
@@ -670,7 +669,8 @@ static int logical_ring_invalidate_all_caches(struct drm_i915_gem_request *req)
 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 intel_engine_cs *engine = req->engine;
+	const unsigned other_rings = ~intel_engine_flag(engine);
 	struct i915_vma *vma;
 	uint32_t flush_domains = 0;
 	bool flush_chipset = false;
@@ -680,7 +680,7 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
 		struct drm_i915_gem_object *obj = vma->obj;
 
 		if (obj->active & other_rings) {
-			ret = i915_gem_object_sync(obj, req->engine, &req);
+			ret = i915_gem_object_sync(obj, engine, &req);
 			if (ret)
 				return ret;
 		}
@@ -782,6 +782,7 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
 	struct intel_ringbuffer *ringbuf = request->ringbuf;
 	struct drm_i915_private *dev_priv = request->i915;
 	struct intel_engine_cs *engine = request->engine;
+	struct intel_context *ctx = request->ctx;
 
 	intel_logical_ring_advance(ringbuf);
 	request->tail = ringbuf->tail;
@@ -799,12 +800,12 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
 	if (intel_engine_stopped(engine))
 		return 0;
 
-	if (engine->last_context != request->ctx) {
+	if (engine->last_context != ctx) {
 		if (engine->last_context)
 			intel_lr_context_unpin(engine->last_context, engine);
-		if (request->ctx != request->i915->kernel_context) {
-			intel_lr_context_pin(request->ctx, engine);
-			engine->last_context = request->ctx;
+		if (ctx != dev_priv->kernel_context) {
+			intel_lr_context_pin(ctx, engine);
+			engine->last_context = ctx;
 		} else {
 			engine->last_context = NULL;
 		}
@@ -949,9 +950,8 @@ 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 = dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(params->dev);
 	struct intel_ringbuffer *ringbuf = params->ctx->engine[engine->id].ringbuf;
 	u64 exec_start;
 	int instp_mode;
@@ -1092,17 +1092,18 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
 				   struct intel_engine_cs *engine)
 {
 	struct drm_device *dev = engine->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
-	struct intel_ringbuffer *ringbuf = ctx->engine[engine->id].ringbuf;
+	struct drm_i915_private *dev_priv = ctx->i915;
+	struct intel_context_engine *ce = &ctx->engine[engine->id];
+	struct drm_i915_gem_object *ctx_obj = ce->state;
+	struct intel_ringbuffer *ringbuf = ce->ringbuf;
 	struct page *lrc_state_page;
 	uint32_t *lrc_reg_state;
 	int ret;
 
-	WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
 	ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN,
-			PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+				    PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
 	if (ret)
 		return ret;
 
@@ -1112,15 +1113,15 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
 		goto unpin_ctx_obj;
 	}
 
-	ret = intel_pin_and_map_ringbuffer_obj(engine->dev, ringbuf);
+	ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
 	if (ret)
 		goto unpin_ctx_obj;
 
-	ctx->engine[engine->id].lrc_vma = i915_gem_obj_to_ggtt(ctx_obj);
+	ce->lrc_vma = i915_gem_obj_to_ggtt(ctx_obj);
 	intel_lr_context_descriptor_update(ctx, engine);
 	lrc_reg_state = kmap(lrc_state_page);
 	lrc_reg_state[CTX_RING_BUFFER_START+1] = ringbuf->vma->node.start;
-	ctx->engine[engine->id].lrc_reg_state = lrc_reg_state;
+	ce->lrc_reg_state = lrc_reg_state;
 	ctx_obj->dirty = true;
 
 	/* Invalidate GuC TLB. */
@@ -1138,9 +1139,10 @@ unpin_ctx_obj:
 static int intel_lr_context_pin(struct intel_context *ctx,
 				struct intel_engine_cs *engine)
 {
+	struct intel_context_engine *ce = &ctx->engine[engine->id];
 	int ret = 0;
 
-	if (ctx->engine[engine->id].pin_count++ == 0) {
+	if (ce->pin_count++ == 0) {
 		ret = intel_lr_context_do_pin(ctx, engine);
 		if (ret)
 			goto reset_pin_count;
@@ -1150,23 +1152,25 @@ static int intel_lr_context_pin(struct intel_context *ctx,
 	return ret;
 
 reset_pin_count:
-	ctx->engine[engine->id].pin_count = 0;
+	ce->pin_count = 0;
 	return ret;
 }
 
 void intel_lr_context_unpin(struct intel_context *ctx,
 			    struct intel_engine_cs *engine)
 {
-	struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
+	struct intel_context_engine *ce = &ctx->engine[engine->id];
+	struct drm_i915_gem_object *ctx_obj = ce->state;
 
 	WARN_ON(!mutex_is_locked(&ctx->i915->dev->struct_mutex));
-	if (--ctx->engine[engine->id].pin_count == 0) {
-		kunmap(kmap_to_page(ctx->engine[engine->id].lrc_reg_state));
-		intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
+
+	if (--ce->pin_count == 0) {
+		kunmap(kmap_to_page(ce->lrc_reg_state));
+		intel_unpin_ringbuffer_obj(ce->ringbuf);
 		i915_gem_object_ggtt_unpin(ctx_obj);
-		ctx->engine[engine->id].lrc_vma = NULL;
-		ctx->engine[engine->id].lrc_desc = 0;
-		ctx->engine[engine->id].lrc_reg_state = NULL;
+		ce->lrc_vma = NULL;
+		ce->lrc_desc = 0;
+		ce->lrc_reg_state = NULL;
 
 		i915_gem_context_unreference(ctx);
 	}
@@ -1177,8 +1181,7 @@ static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
 	int ret, i;
 	struct intel_engine_cs *engine = req->engine;
 	struct intel_ringbuffer *ringbuf = req->ringbuf;
-	struct drm_device *dev = engine->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = req->i915;
 	struct i915_workarounds *w = &dev_priv->workarounds;
 
 	if (w->count == 0)
@@ -2513,8 +2516,9 @@ void intel_lr_context_free(struct intel_context *ctx)
 	int i;
 
 	for (i = I915_NUM_ENGINES; --i >= 0; ) {
-		struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf;
-		struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
+		struct intel_context_engine *ce = &ctx->engine[i];
+		struct intel_ringbuffer *ringbuf = ce->ringbuf;
+		struct drm_i915_gem_object *ctx_obj = ce->state;
 
 		if (!ctx_obj)
 			continue;
@@ -2524,7 +2528,7 @@ void intel_lr_context_free(struct intel_context *ctx)
 			i915_gem_object_ggtt_unpin(ctx_obj);
 		}
 
-		WARN_ON(ctx->engine[i].pin_count);
+		WARN_ON(ce->pin_count);
 		intel_ringbuffer_free(ringbuf);
 		drm_gem_object_unreference(&ctx_obj->base);
 	}
@@ -2604,13 +2608,14 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 				    struct intel_engine_cs *engine)
 {
 	struct drm_device *dev = engine->dev;
+	struct intel_context_engine *ce = &ctx->engine[engine->id];
 	struct drm_i915_gem_object *ctx_obj;
 	uint32_t context_size;
 	struct intel_ringbuffer *ringbuf;
 	int ret;
 
 	WARN_ON(ctx->legacy_hw_ctx.rcs_state != NULL);
-	WARN_ON(ctx->engine[engine->id].state);
+	WARN_ON(ce->state);
 
 	context_size = round_up(intel_lr_context_size(engine), 4096);
 
@@ -2635,8 +2640,8 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 		goto error_ringbuf;
 	}
 
-	ctx->engine[engine->id].ringbuf = ringbuf;
-	ctx->engine[engine->id].state = ctx_obj;
+	ce->ringbuf = ringbuf;
+	ce->state = ctx_obj;
 
 	if (ctx != ctx->i915->kernel_context && engine->init_context) {
 		struct drm_i915_gem_request *req;
@@ -2663,8 +2668,8 @@ error_ringbuf:
 	intel_ringbuffer_free(ringbuf);
 error_deref_obj:
 	drm_gem_object_unreference(&ctx_obj->base);
-	ctx->engine[engine->id].ringbuf = NULL;
-	ctx->engine[engine->id].state = NULL;
+	ce->ringbuf = NULL;
+	ce->state = NULL;
 	return ret;
 }
 
@@ -2676,10 +2681,9 @@ void intel_lr_context_reset(struct drm_device *dev,
 	int i;
 
 	for_each_engine(engine, dev_priv, i) {
-		struct drm_i915_gem_object *ctx_obj =
-				ctx->engine[engine->id].state;
-		struct intel_ringbuffer *ringbuf =
-				ctx->engine[engine->id].ringbuf;
+		struct intel_context_engine *ce = &ctx->engine[engine->id];
+		struct drm_i915_gem_object *ctx_obj = ce->state;
+		struct intel_ringbuffer *ringbuf = ce->ringbuf;
 		uint32_t *reg_state;
 		struct page *page;
 
-- 
1.9.1

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

✗ Fi.CI.BAT: warning for drm/i915: Name the anonymous per-engine context struct (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Name the anonymous per-engine context struct (rev2)
URL   : https://patchwork.freedesktop.org/series/4635/
State : warning

== Summary ==

Series 4635v2 drm/i915: Name the anonymous per-engine context struct
http://patchwork.freedesktop.org/api/1.0/series/4635/revisions/2/mbox/

Test gem_ringfill:
        Subgroup basic-default-s3:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (byt-nuc)
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (hsw-brixbox)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (ilk-hp8440p)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a:
                dmesg-warn -> PASS       (snb-x220t)
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                pass       -> DMESG-WARN (snb-dellxps)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (bsw-nuc-2)
                incomplete -> PASS       (hsw-gt2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                fail       -> DMESG-FAIL (snb-x220t)
                dmesg-warn -> PASS       (snb-dellxps)

bdw-nuci7        total:194  pass:182  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:194  pass:173  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:194  pass:156  dwarn:1   dfail:0   fail:0   skip:37 
byt-nuc          total:194  pass:159  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:194  pass:171  dwarn:1   dfail:0   fail:0   skip:22 
hsw-gt2          total:194  pass:176  dwarn:1   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:194  pass:129  dwarn:1   dfail:0   fail:0   skip:64 
ivb-t430s        total:194  pass:169  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
snb-dellxps      total:194  pass:159  dwarn:1   dfail:0   fail:0   skip:34 
snb-x220t        total:194  pass:160  dwarn:0   dfail:1   fail:0   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1649/

e7a7673e9840fe8b50a5a2894c75565ec7858a00 drm-intel-nightly: 2016y-03m-19d-10h-09m-53s UTC integration manifest
a1c5f9b1e8b9cbfdab0fb71ccf7a5a0838b56069 drm/i915: Name the anonymous per-engine context struct

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

Re: [PATCH v2] drm/i915: Name the anonymous per-engine context struct

[Dave Gordon]

On 18/03/16 17:26, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> This anonymous struct was causing a good amount of overly
> verbose code. Also, if we name it and cache the pointer locally
> when there are multiple accesses to it, not only the code is
> more readable, but the compiler manages to generate smaller
> binary.
>
> Along the way I also shortened access to dev_priv and eliminated
> some unused variables and cached some where I spotted the
> opportunity.
>
> Name for the structure, intel_context_engine, and the local
> variable name were borrowed from a similar patch by Chris Wilson.
>
> v2: Hate the engine->dev surprises, really do.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_drv.h  |  2 +-
>   drivers/gpu/drm/i915/intel_lrc.c | 94 +++++++++++++++++++++-------------------
>   2 files changed, 50 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 00c41a4bde2a..480639c39543 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -840,7 +840,7 @@ struct intel_context {
>   	} legacy_hw_ctx;
>
>   	/* Execlists */
> -	struct {
> +	struct intel_context_engine {

Good idea, I had a version of this too, derived from Chris' patch
[157/190] drm/i915: Tidy execlists by using intel_context_engine locals.

The only thing to disagree with is the actual name; it should be 
"intel_engine_context" (or some abbreviation thereof), because in 
English (and German) the noun at the *end* of a compound noun-phrase
is what it actually *is*, with all the others qualifying it. So a 
"railway bridge" is a type of bridge, not a type of railway, and
"eine Straßenbahnhaltestelle" (street-train-stopping-place => tram stop) 
is not a street.

[aside] My favourite in English is "Space Civilisation Power Struggle 
Game" (five nouns in a row!) describing a certain boxed game -- anyone 
recognise that? [/aside]

I'll post a version following that naming convention shortly ...

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

Re: [PATCH v2] drm/i915: Name the anonymous per-engine context struct

[Chris Wilson]

On Mon, Mar 21, 2016 at 03:23:57PM +0000, Dave Gordon wrote:
> On 18/03/16 17:26, Tvrtko Ursulin wrote:
> >From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> >This anonymous struct was causing a good amount of overly
> >verbose code. Also, if we name it and cache the pointer locally
> >when there are multiple accesses to it, not only the code is
> >more readable, but the compiler manages to generate smaller
> >binary.
> >
> >Along the way I also shortened access to dev_priv and eliminated
> >some unused variables and cached some where I spotted the
> >opportunity.
> >
> >Name for the structure, intel_context_engine, and the local
> >variable name were borrowed from a similar patch by Chris Wilson.
> >
> >v2: Hate the engine->dev surprises, really do.
> >
> >Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> >  drivers/gpu/drm/i915/intel_lrc.c | 94 +++++++++++++++++++++-------------------
> >  2 files changed, 50 insertions(+), 46 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index 00c41a4bde2a..480639c39543 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -840,7 +840,7 @@ struct intel_context {
> >  	} legacy_hw_ctx;
> >
> >  	/* Execlists */
> >-	struct {
> >+	struct intel_context_engine {
> 
> Good idea, I had a version of this too, derived from Chris' patch
> [157/190] drm/i915: Tidy execlists by using intel_context_engine locals.
> 
> The only thing to disagree with is the actual name; it should be
> "intel_engine_context" (or some abbreviation thereof), because in

We have been using object_subobject.
-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

Re: ✗ Fi.CI.BAT: warning for drm/i915: Name the anonymous per-engine context struct (rev2)

[Tvrtko Ursulin]

On 21/03/16 11:22, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915: Name the anonymous per-engine context struct (rev2)
> URL   : https://patchwork.freedesktop.org/series/4635/
> State : warning
>
> == Summary ==
>
> Series 4635v2 drm/i915: Name the anonymous per-engine context struct
> http://patchwork.freedesktop.org/api/1.0/series/4635/revisions/2/mbox/
>
> Test gem_ringfill:
>          Subgroup basic-default-s3:
>                  dmesg-warn -> PASS       (bsw-nuc-2)
> Test kms_flip:
>          Subgroup basic-flip-vs-dpms:
>                  pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE

Sporadic ILK pipe underruns: 
https://bugs.freedesktop.org/show_bug.cgi?id=93787

>          Subgroup basic-flip-vs-wf_vblank:
>                  fail       -> PASS       (byt-nuc)
>          Subgroup basic-plain-flip:
>                  pass       -> DMESG-WARN (hsw-brixbox)

Unrelated intermittent device suspended while HW access: 
https://bugs.freedesktop.org/show_bug.cgi?id=94349

> Test kms_force_connector_basic:
>          Subgroup force-connector-state:
>                  pass       -> SKIP       (ilk-hp8440p)
> Test kms_pipe_crc_basic:
>          Subgroup nonblocking-crc-pipe-a:
>                  dmesg-warn -> PASS       (snb-x220t)
>          Subgroup nonblocking-crc-pipe-a-frame-sequence:
>                  pass       -> DMESG-WARN (snb-dellxps)

Same.

>          Subgroup suspend-read-crc-pipe-c:
>                  pass       -> DMESG-WARN (bsw-nuc-2)


Unrelated lockdep splat: https://bugs.freedesktop.org/show_bug.cgi?id=94350

>                  incomplete -> PASS       (hsw-gt2)
> Test pm_rpm:
>          Subgroup basic-pci-d3-state:
>                  fail       -> DMESG-FAIL (snb-x220t)

Device suspended while HW access again.

>                  dmesg-warn -> PASS       (snb-dellxps)
>
> bdw-nuci7        total:194  pass:182  dwarn:0   dfail:0   fail:0   skip:12
> bdw-ultra        total:194  pass:173  dwarn:0   dfail:0   fail:0   skip:21
> bsw-nuc-2        total:194  pass:156  dwarn:1   dfail:0   fail:0   skip:37
> byt-nuc          total:194  pass:159  dwarn:0   dfail:0   fail:0   skip:35
> hsw-brixbox      total:194  pass:171  dwarn:1   dfail:0   fail:0   skip:22
> hsw-gt2          total:194  pass:176  dwarn:1   dfail:0   fail:0   skip:17
> ilk-hp8440p      total:194  pass:129  dwarn:1   dfail:0   fail:0   skip:64
> ivb-t430s        total:194  pass:169  dwarn:0   dfail:0   fail:0   skip:25
> skl-i7k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23
> snb-dellxps      total:194  pass:159  dwarn:1   dfail:0   fail:0   skip:34
> snb-x220t        total:194  pass:160  dwarn:0   dfail:1   fail:0   skip:33
>
> Results at /archive/results/CI_IGT_test/Patchwork_1649/
>
> e7a7673e9840fe8b50a5a2894c75565ec7858a00 drm-intel-nightly: 2016y-03m-19d-10h-09m-53s UTC integration manifest
> a1c5f9b1e8b9cbfdab0fb71ccf7a5a0838b56069 drm/i915: Name the anonymous per-engine context struct

So looking good.

Chris, r-b on v2? It was just a revert of a hunk which changed one 
instance of ctx->i915->dev->struct_mutex to engine->dev->struct_mutex 
which the CI reminded me is not allowed in some places.

Regards,

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

Re: ✗ Fi.CI.BAT: warning for drm/i915: Name the anonymous per-engine context struct (rev2)

[Chris Wilson]

On Tue, Mar 22, 2016 at 09:53:25AM +0000, Tvrtko Ursulin wrote:
> 
> 
> On 21/03/16 11:22, Patchwork wrote:
> >== Series Details ==
> >
> >Series: drm/i915: Name the anonymous per-engine context struct (rev2)
> >URL   : https://patchwork.freedesktop.org/series/4635/
> >State : warning
> >
> >== Summary ==
> >
> >Series 4635v2 drm/i915: Name the anonymous per-engine context struct
> >http://patchwork.freedesktop.org/api/1.0/series/4635/revisions/2/mbox/
> >
> >Test gem_ringfill:
> >         Subgroup basic-default-s3:
> >                 dmesg-warn -> PASS       (bsw-nuc-2)
> >Test kms_flip:
> >         Subgroup basic-flip-vs-dpms:
> >                 pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
> 
> Sporadic ILK pipe underruns:
> https://bugs.freedesktop.org/show_bug.cgi?id=93787
> 
> >         Subgroup basic-flip-vs-wf_vblank:
> >                 fail       -> PASS       (byt-nuc)
> >         Subgroup basic-plain-flip:
> >                 pass       -> DMESG-WARN (hsw-brixbox)
> 
> Unrelated intermittent device suspended while HW access:
> https://bugs.freedesktop.org/show_bug.cgi?id=94349
> 
> >Test kms_force_connector_basic:
> >         Subgroup force-connector-state:
> >                 pass       -> SKIP       (ilk-hp8440p)
> >Test kms_pipe_crc_basic:
> >         Subgroup nonblocking-crc-pipe-a:
> >                 dmesg-warn -> PASS       (snb-x220t)
> >         Subgroup nonblocking-crc-pipe-a-frame-sequence:
> >                 pass       -> DMESG-WARN (snb-dellxps)
> 
> Same.
> 
> >         Subgroup suspend-read-crc-pipe-c:
> >                 pass       -> DMESG-WARN (bsw-nuc-2)
> 
> 
> Unrelated lockdep splat: https://bugs.freedesktop.org/show_bug.cgi?id=94350

Is it really the same one? There should be another lockdep chain that
isn't in bugzilla...
 
> >                 incomplete -> PASS       (hsw-gt2)
> >Test pm_rpm:
> >         Subgroup basic-pci-d3-state:
> >                 fail       -> DMESG-FAIL (snb-x220t)
> 
> Device suspended while HW access again.
> 
> >                 dmesg-warn -> PASS       (snb-dellxps)
> >
> >bdw-nuci7        total:194  pass:182  dwarn:0   dfail:0   fail:0   skip:12
> >bdw-ultra        total:194  pass:173  dwarn:0   dfail:0   fail:0   skip:21
> >bsw-nuc-2        total:194  pass:156  dwarn:1   dfail:0   fail:0   skip:37
> >byt-nuc          total:194  pass:159  dwarn:0   dfail:0   fail:0   skip:35
> >hsw-brixbox      total:194  pass:171  dwarn:1   dfail:0   fail:0   skip:22
> >hsw-gt2          total:194  pass:176  dwarn:1   dfail:0   fail:0   skip:17
> >ilk-hp8440p      total:194  pass:129  dwarn:1   dfail:0   fail:0   skip:64
> >ivb-t430s        total:194  pass:169  dwarn:0   dfail:0   fail:0   skip:25
> >skl-i7k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23
> >snb-dellxps      total:194  pass:159  dwarn:1   dfail:0   fail:0   skip:34
> >snb-x220t        total:194  pass:160  dwarn:0   dfail:1   fail:0   skip:33
> >
> >Results at /archive/results/CI_IGT_test/Patchwork_1649/
> >
> >e7a7673e9840fe8b50a5a2894c75565ec7858a00 drm-intel-nightly: 2016y-03m-19d-10h-09m-53s UTC integration manifest
> >a1c5f9b1e8b9cbfdab0fb71ccf7a5a0838b56069 drm/i915: Name the anonymous per-engine context struct
> 
> So looking good.
> 
> Chris, r-b on v2? It was just a revert of a hunk which changed one
> instance of ctx->i915->dev->struct_mutex to
> engine->dev->struct_mutex which the CI reminded me is not allowed in
> some places.

That one again! One day we will get engine init/fini sorted. Yes,
-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

Re: ✗ Fi.CI.BAT: warning for drm/i915: Name the anonymous per-engine context struct (rev2)

[Tvrtko Ursulin]

On 22/03/16 10:07, Chris Wilson wrote:
> On Tue, Mar 22, 2016 at 09:53:25AM +0000, Tvrtko Ursulin wrote:
>>
>>
>> On 21/03/16 11:22, Patchwork wrote:
>>> == Series Details ==
>>>
>>> Series: drm/i915: Name the anonymous per-engine context struct (rev2)
>>> URL   : https://patchwork.freedesktop.org/series/4635/
>>> State : warning
>>>
>>> == Summary ==
>>>
>>> Series 4635v2 drm/i915: Name the anonymous per-engine context struct
>>> http://patchwork.freedesktop.org/api/1.0/series/4635/revisions/2/mbox/
>>>
>>> Test gem_ringfill:
>>>          Subgroup basic-default-s3:
>>>                  dmesg-warn -> PASS       (bsw-nuc-2)
>>> Test kms_flip:
>>>          Subgroup basic-flip-vs-dpms:
>>>                  pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
>>
>> Sporadic ILK pipe underruns:
>> https://bugs.freedesktop.org/show_bug.cgi?id=93787
>>
>>>          Subgroup basic-flip-vs-wf_vblank:
>>>                  fail       -> PASS       (byt-nuc)
>>>          Subgroup basic-plain-flip:
>>>                  pass       -> DMESG-WARN (hsw-brixbox)
>>
>> Unrelated intermittent device suspended while HW access:
>> https://bugs.freedesktop.org/show_bug.cgi?id=94349
>>
>>> Test kms_force_connector_basic:
>>>          Subgroup force-connector-state:
>>>                  pass       -> SKIP       (ilk-hp8440p)
>>> Test kms_pipe_crc_basic:
>>>          Subgroup nonblocking-crc-pipe-a:
>>>                  dmesg-warn -> PASS       (snb-x220t)
>>>          Subgroup nonblocking-crc-pipe-a-frame-sequence:
>>>                  pass       -> DMESG-WARN (snb-dellxps)
>>
>> Same.
>>
>>>          Subgroup suspend-read-crc-pipe-c:
>>>                  pass       -> DMESG-WARN (bsw-nuc-2)
>>
>>
>> Unrelated lockdep splat: https://bugs.freedesktop.org/show_bug.cgi?id=94350
>
> Is it really the same one? There should be another lockdep chain that
> isn't in bugzilla...

Looks the same to me:

Bz:

[  179.762863] rtcwake/5995 is trying to acquire lock:
[  179.762877]  (s_active#6){++++.+}, at: [<ffffffff8124ec70>] 
kernfs_remove_by_name_ns+0x40/0xa0
[  179.762878]
but task is already holding lock:
[  179.762885]  (cpu_hotplug.lock){+.+.+.}, at: [<ffffffff81078c4d>] 
cpu_hotplug_begin+0x6d/0xc0
[  179.762886]
which lock already depends on the new lock.

This CI run:

[  127.210522] rtcwake/5947 is trying to acquire lock:
[  127.210539]  (s_active#6){++++.+}, at: [<ffffffff81250740>] 
kernfs_remove_by_name_ns+0x40/0xa0
[  127.210540]
but task is already holding lock:
[  127.210549]  (cpu_hotplug.lock){+.+.+.}, at: [<ffffffff81078f5d>] 
cpu_hotplug_begin+0x6d/0xc0
[  127.210550]
which lock already depends on the new lock.

>
>>>                  incomplete -> PASS       (hsw-gt2)
>>> Test pm_rpm:
>>>          Subgroup basic-pci-d3-state:
>>>                  fail       -> DMESG-FAIL (snb-x220t)
>>
>> Device suspended while HW access again.
>>
>>>                  dmesg-warn -> PASS       (snb-dellxps)
>>>
>>> bdw-nuci7        total:194  pass:182  dwarn:0   dfail:0   fail:0   skip:12
>>> bdw-ultra        total:194  pass:173  dwarn:0   dfail:0   fail:0   skip:21
>>> bsw-nuc-2        total:194  pass:156  dwarn:1   dfail:0   fail:0   skip:37
>>> byt-nuc          total:194  pass:159  dwarn:0   dfail:0   fail:0   skip:35
>>> hsw-brixbox      total:194  pass:171  dwarn:1   dfail:0   fail:0   skip:22
>>> hsw-gt2          total:194  pass:176  dwarn:1   dfail:0   fail:0   skip:17
>>> ilk-hp8440p      total:194  pass:129  dwarn:1   dfail:0   fail:0   skip:64
>>> ivb-t430s        total:194  pass:169  dwarn:0   dfail:0   fail:0   skip:25
>>> skl-i7k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23
>>> snb-dellxps      total:194  pass:159  dwarn:1   dfail:0   fail:0   skip:34
>>> snb-x220t        total:194  pass:160  dwarn:0   dfail:1   fail:0   skip:33
>>>
>>> Results at /archive/results/CI_IGT_test/Patchwork_1649/
>>>
>>> e7a7673e9840fe8b50a5a2894c75565ec7858a00 drm-intel-nightly: 2016y-03m-19d-10h-09m-53s UTC integration manifest
>>> a1c5f9b1e8b9cbfdab0fb71ccf7a5a0838b56069 drm/i915: Name the anonymous per-engine context struct
>>
>> So looking good.
>>
>> Chris, r-b on v2? It was just a revert of a hunk which changed one
>> instance of ctx->i915->dev->struct_mutex to
>> engine->dev->struct_mutex which the CI reminded me is not allowed in
>> some places.
>
> That one again! One day we will get engine init/fini sorted. Yes,

Yes r-b, just to be really sure?

Regards,

Tvrtko

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

Re: ✗ Fi.CI.BAT: warning for drm/i915: Name the anonymous per-engine context struct (rev2)

[Chris Wilson]

On Tue, Mar 22, 2016 at 10:27:30AM +0000, Tvrtko Ursulin wrote:
> 
> On 22/03/16 10:07, Chris Wilson wrote:
> >Is it really the same one? There should be another lockdep chain that
> >isn't in bugzilla...
> 
> Looks the same to me:
> 
> Bz:
> 
> [  179.762863] rtcwake/5995 is trying to acquire lock:
> [  179.762877]  (s_active#6){++++.+}, at: [<ffffffff8124ec70>]
> kernfs_remove_by_name_ns+0x40/0xa0
> [  179.762878]
> but task is already holding lock:
> [  179.762885]  (cpu_hotplug.lock){+.+.+.}, at: [<ffffffff81078c4d>]
> cpu_hotplug_begin+0x6d/0xc0
> [  179.762886]
> which lock already depends on the new lock.
> 
> This CI run:
> 
> [  127.210522] rtcwake/5947 is trying to acquire lock:
> [  127.210539]  (s_active#6){++++.+}, at: [<ffffffff81250740>]
> kernfs_remove_by_name_ns+0x40/0xa0
> [  127.210540]
> but task is already holding lock:
> [  127.210549]  (cpu_hotplug.lock){+.+.+.}, at: [<ffffffff81078f5d>]
> cpu_hotplug_begin+0x6d/0xc0
> [  127.210550]
> which lock already depends on the new lock.

Ok, the chain I'm looking for doesn't involve the kernfs link.

> >
> >>>                 incomplete -> PASS       (hsw-gt2)
> >>>Test pm_rpm:
> >>>         Subgroup basic-pci-d3-state:
> >>>                 fail       -> DMESG-FAIL (snb-x220t)
> >>
> >>Device suspended while HW access again.
> >>
> >>>                 dmesg-warn -> PASS       (snb-dellxps)
> >>>
> >>>bdw-nuci7        total:194  pass:182  dwarn:0   dfail:0   fail:0   skip:12
> >>>bdw-ultra        total:194  pass:173  dwarn:0   dfail:0   fail:0   skip:21
> >>>bsw-nuc-2        total:194  pass:156  dwarn:1   dfail:0   fail:0   skip:37
> >>>byt-nuc          total:194  pass:159  dwarn:0   dfail:0   fail:0   skip:35
> >>>hsw-brixbox      total:194  pass:171  dwarn:1   dfail:0   fail:0   skip:22
> >>>hsw-gt2          total:194  pass:176  dwarn:1   dfail:0   fail:0   skip:17
> >>>ilk-hp8440p      total:194  pass:129  dwarn:1   dfail:0   fail:0   skip:64
> >>>ivb-t430s        total:194  pass:169  dwarn:0   dfail:0   fail:0   skip:25
> >>>skl-i7k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23
> >>>snb-dellxps      total:194  pass:159  dwarn:1   dfail:0   fail:0   skip:34
> >>>snb-x220t        total:194  pass:160  dwarn:0   dfail:1   fail:0   skip:33
> >>>
> >>>Results at /archive/results/CI_IGT_test/Patchwork_1649/
> >>>
> >>>e7a7673e9840fe8b50a5a2894c75565ec7858a00 drm-intel-nightly: 2016y-03m-19d-10h-09m-53s UTC integration manifest
> >>>a1c5f9b1e8b9cbfdab0fb71ccf7a5a0838b56069 drm/i915: Name the anonymous per-engine context struct
> >>
> >>So looking good.
> >>
> >>Chris, r-b on v2? It was just a revert of a hunk which changed one
> >>instance of ctx->i915->dev->struct_mutex to
> >>engine->dev->struct_mutex which the CI reminded me is not allowed in
> >>some places.
> >
> >That one again! One day we will get engine init/fini sorted. Yes,
> 
> Yes r-b, just to be really sure?

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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

Re: [PATCH v2] drm/i915: Name the anonymous per-engine context struct

[Dave Gordon]

On 22/03/16 09:28, Chris Wilson wrote:
> On Mon, Mar 21, 2016 at 03:23:57PM +0000, Dave Gordon wrote:
>> On 18/03/16 17:26, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> This anonymous struct was causing a good amount of overly
>>> verbose code. Also, if we name it and cache the pointer locally
>>> when there are multiple accesses to it, not only the code is
>>> more readable, but the compiler manages to generate smaller
>>> binary.
>>>
>>> Along the way I also shortened access to dev_priv and eliminated
>>> some unused variables and cached some where I spotted the
>>> opportunity.
>>>
>>> Name for the structure, intel_context_engine, and the local
>>> variable name were borrowed from a similar patch by Chris Wilson.
>>>
>>> v2: Hate the engine->dev surprises, really do.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h  |  2 +-
>>>   drivers/gpu/drm/i915/intel_lrc.c | 94 +++++++++++++++++++++-------------------
>>>   2 files changed, 50 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 00c41a4bde2a..480639c39543 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -840,7 +840,7 @@ struct intel_context {
>>>   	} legacy_hw_ctx;
>>>
>>>   	/* Execlists */
>>> -	struct {
>>> +	struct intel_context_engine {
>>
>> Good idea, I had a version of this too, derived from Chris' patch
>> [157/190] drm/i915: Tidy execlists by using intel_context_engine locals.
>>
>> The only thing to disagree with is the actual name; it should be
>> "intel_engine_context" (or some abbreviation thereof), because in
>
> We have been using object_subobject.
> -Chris

No we haven't. Examples of the above convention for structs include:

* "struct intel_device_info" - information about an intel device
* "struct i915_ctx_hang_stats" - statistics about hangs, per context
* "struct intel_uncore_funcs" - functions exported from uncore
* "struct i915_power_well_ops" - operations on power wells
* "struct i915_suspend_saved_registers" - registers saved at suspend

and for instance names:

* "struct list_head request_list" - a list of requests
* "struct i915_vma *lrc_vma" - the VMA for an LRC
* "uint32_t *lrc_reg_state" - the state of the registers in an LRC

Indeed it's quite difficult to find any compound names that do *not* 
follow this convention. Maybe your version would be more natural in a 
language where adjectives (or possessives) normally follow the noun they 
describe?

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

Re: [PATCH v2] drm/i915: Name the anonymous per-engine context struct

[Tvrtko Ursulin]

On 22/03/16 11:20, Dave Gordon wrote:
> On 22/03/16 09:28, Chris Wilson wrote:
>> On Mon, Mar 21, 2016 at 03:23:57PM +0000, Dave Gordon wrote:
>>> On 18/03/16 17:26, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> This anonymous struct was causing a good amount of overly
>>>> verbose code. Also, if we name it and cache the pointer locally
>>>> when there are multiple accesses to it, not only the code is
>>>> more readable, but the compiler manages to generate smaller
>>>> binary.
>>>>
>>>> Along the way I also shortened access to dev_priv and eliminated
>>>> some unused variables and cached some where I spotted the
>>>> opportunity.
>>>>
>>>> Name for the structure, intel_context_engine, and the local
>>>> variable name were borrowed from a similar patch by Chris Wilson.
>>>>
>>>> v2: Hate the engine->dev surprises, really do.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_drv.h  |  2 +-
>>>>   drivers/gpu/drm/i915/intel_lrc.c | 94
>>>> +++++++++++++++++++++-------------------
>>>>   2 files changed, 50 insertions(+), 46 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 00c41a4bde2a..480639c39543 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -840,7 +840,7 @@ struct intel_context {
>>>>       } legacy_hw_ctx;
>>>>
>>>>       /* Execlists */
>>>> -    struct {
>>>> +    struct intel_context_engine {
>>>
>>> Good idea, I had a version of this too, derived from Chris' patch
>>> [157/190] drm/i915: Tidy execlists by using intel_context_engine locals.
>>>
>>> The only thing to disagree with is the actual name; it should be
>>> "intel_engine_context" (or some abbreviation thereof), because in
>>
>> We have been using object_subobject.
>> -Chris
>
> No we haven't. Examples of the above convention for structs include:
>
> * "struct intel_device_info" - information about an intel device
> * "struct i915_ctx_hang_stats" - statistics about hangs, per context
> * "struct intel_uncore_funcs" - functions exported from uncore
> * "struct i915_power_well_ops" - operations on power wells
> * "struct i915_suspend_saved_registers" - registers saved at suspend
>
> and for instance names:
>
> * "struct list_head request_list" - a list of requests
> * "struct i915_vma *lrc_vma" - the VMA for an LRC
> * "uint32_t *lrc_reg_state" - the state of the registers in an LRC
>
> Indeed it's quite difficult to find any compound names that do *not*
> follow this convention. Maybe your version would be more natural in a
> language where adjectives (or possessives) normally follow the noun they
> describe?

One example is maybe:

struct drm_i915_error_state {
	struct drm_i915_error_ring

So per-ring state of the total error state, which sounds equivalent to 
per-engine state of a context.

Or:

struct intel_fbc {
	struct intel_fbc_state_cache {

More verbose version like "struct intel_context_engine_state"?

"struct intel_engine_context" reads like a context of an engine to me, 
which sounds opposite to what it should be - state of an engine for a 
context.

?

Regards,

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

[PATCH v3 1/2] drm/i915: name the anonymous per-engine context struct

[Dave Gordon]

Introduce a name for the previously anonymous per-engine structure
embedded inside the intel_context, and use local pointers to the
relevant element of this array to simplify various execlist functions.
This may improve the compiler's ability to avoid redundant dereference-
and index operations.

This version is derived from an original by Chris Wilson, detached
from the original set of patches in which it was posted and rebased
to current nightly.

Then it was updated by Tvrtko Ursulin <tvrtko.ursulin@intel.com>,
who noted:

  This anonymous struct was causing a good amount of overly verbose
  code. Also, if we name it and cache the pointer locally when there
  are multiple accesses to it, not only the code is more readable,
  but the compiler manages to generate smaller binary.

  Along the way I also shortened access to dev_priv and eliminated some
  unused variables and cached some where I spotted the opportunity.

This version uses the name "struct intel_engine_ctx" in accordance with
the English (and German) convention that the concrete noun goes at the
end of a noun-sequence, with earlier ones acting as adjectives i.e. an
"engine-context" is the context of an engine, whereas a "context-engine"
would be some sort of engine that processes contexts. Since object and
structure names are noun-like, it's more consistent to name them this way.

Originally-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h  |   2 +-
 drivers/gpu/drm/i915/intel_lrc.c | 122 +++++++++++++++++++--------------------
 2 files changed, 62 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 00c41a4..be3709f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -840,7 +840,7 @@ struct intel_context {
 	} legacy_hw_ctx;
 
 	/* Execlists */
-	struct {
+	struct intel_engine_ctx {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
 		int pin_count;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index f727822..a054a32 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -316,16 +316,16 @@ int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists
 intel_lr_context_descriptor_update(struct intel_context *ctx,
 				   struct intel_engine_cs *engine)
 {
+	struct intel_engine_ctx *ectx = &ctx->engine[engine->id];
 	uint64_t lrca, desc;
 
-	lrca = ctx->engine[engine->id].lrc_vma->node.start +
-	       LRC_PPHWSP_PN * PAGE_SIZE;
+	lrca = ectx->lrc_vma->node.start + LRC_PPHWSP_PN * PAGE_SIZE;
 
-	desc = engine->ctx_desc_template;			   /* bits  0-11 */
+	desc = engine->ctx_desc_template;		   /* bits  0-11 */
 	desc |= lrca;					   /* bits 12-31 */
 	desc |= (lrca >> PAGE_SHIFT) << GEN8_CTX_ID_SHIFT; /* bits 32-51 */
 
-	ctx->engine[engine->id].lrc_desc = desc;
+	ectx->lrc_desc = desc;
 }
 
 uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
@@ -361,8 +361,7 @@ static void execlists_elsp_write(struct drm_i915_gem_request *rq0,
 {
 
 	struct intel_engine_cs *engine = rq0->engine;
-	struct drm_device *dev = engine->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = rq0->i915;
 	uint64_t desc[2];
 
 	if (rq1) {
@@ -468,11 +467,8 @@ static void execlists_context_unqueue__locked(struct intel_engine_cs *engine)
 		 * resubmit the request. See gen8_emit_request() for where we
 		 * prepare the padding after the end of the request.
 		 */
-		struct intel_ringbuffer *ringbuf;
-
-		ringbuf = req0->ctx->engine[engine->id].ringbuf;
 		req0->tail += 8;
-		req0->tail &= ringbuf->size - 1;
+		req0->tail &= req0->ringbuf->size - 1;
 	}
 
 	execlists_submit_requests(req0, req1);
@@ -669,7 +665,8 @@ static int logical_ring_invalidate_all_caches(struct drm_i915_gem_request *req)
 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 intel_engine_cs *engine = req->engine;
+	const unsigned other_rings = ~intel_engine_flag(engine);
 	struct i915_vma *vma;
 	uint32_t flush_domains = 0;
 	bool flush_chipset = false;
@@ -679,7 +676,7 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
 		struct drm_i915_gem_object *obj = vma->obj;
 
 		if (obj->active & other_rings) {
-			ret = i915_gem_object_sync(obj, req->engine, &req);
+			ret = i915_gem_object_sync(obj, engine, &req);
 			if (ret)
 				return ret;
 		}
@@ -701,9 +698,11 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
 
 int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request)
 {
+	struct intel_engine_cs *engine = request->engine;
+	struct intel_context *ctx = request->ctx;
 	int ret = 0;
 
-	request->ringbuf = request->ctx->engine[request->engine->id].ringbuf;
+	request->ringbuf = ctx->engine[engine->id].ringbuf;
 
 	if (i915.enable_guc_submission) {
 		/*
@@ -718,8 +717,8 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 			return ret;
 	}
 
-	if (request->ctx != request->i915->kernel_context)
-		ret = intel_lr_context_pin(request->ctx, request->engine);
+	if (ctx != request->i915->kernel_context)
+		ret = intel_lr_context_pin(ctx, engine);
 
 	return ret;
 }
@@ -781,6 +780,7 @@ static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
 	struct intel_ringbuffer *ringbuf = request->ringbuf;
 	struct drm_i915_private *dev_priv = request->i915;
 	struct intel_engine_cs *engine = request->engine;
+	struct intel_context *ctx = request->ctx;
 
 	intel_logical_ring_advance(ringbuf);
 	request->tail = ringbuf->tail;
@@ -798,12 +798,12 @@ static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
 	if (intel_engine_stopped(engine))
 		return 0;
 
-	if (engine->last_context != request->ctx) {
+	if (engine->last_context != ctx) {
 		if (engine->last_context)
 			intel_lr_context_unpin(engine->last_context, engine);
-		if (request->ctx != request->i915->kernel_context) {
-			intel_lr_context_pin(request->ctx, engine);
-			engine->last_context = request->ctx;
+		if (ctx != dev_priv->kernel_context) {
+			intel_lr_context_pin(ctx, engine);
+			engine->last_context = ctx;
 		} else {
 			engine->last_context = NULL;
 		}
@@ -948,9 +948,8 @@ 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 = dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(params->dev);
 	struct intel_ringbuffer *ringbuf = params->ctx->engine[engine->id].ringbuf;
 	u64 exec_start;
 	int instp_mode;
@@ -1024,6 +1023,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
 
 void intel_execlists_retire_requests(struct intel_engine_cs *engine)
 {
+	struct intel_context *dctx = to_i915(engine->dev)->kernel_context;
 	struct drm_i915_gem_request *req, *tmp;
 	struct list_head retired_list;
 
@@ -1038,10 +1038,8 @@ void intel_execlists_retire_requests(struct intel_engine_cs *engine)
 
 	list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
 		struct intel_context *ctx = req->ctx;
-		struct drm_i915_gem_object *ctx_obj =
-				ctx->engine[engine->id].state;
 
-		if (ctx_obj && (ctx != req->i915->kernel_context))
+		if (ctx != dctx && ctx->engine[engine->id].state)
 			intel_lr_context_unpin(ctx, engine);
 
 		list_del(&req->execlist_link);
@@ -1091,17 +1089,18 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
 				   struct intel_engine_cs *engine)
 {
 	struct drm_device *dev = engine->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
-	struct intel_ringbuffer *ringbuf = ctx->engine[engine->id].ringbuf;
+	struct drm_i915_private *dev_priv = ctx->i915;
+	struct intel_engine_ctx *ectx = &ctx->engine[engine->id];
+	struct drm_i915_gem_object *ctx_obj = ectx->state;
+	struct intel_ringbuffer *ringbuf = ectx->ringbuf;
 	struct page *lrc_state_page;
 	uint32_t *lrc_reg_state;
 	int ret;
 
-	WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
 	ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN,
-			PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+				    PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
 	if (ret)
 		return ret;
 
@@ -1111,15 +1110,15 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
 		goto unpin_ctx_obj;
 	}
 
-	ret = intel_pin_and_map_ringbuffer_obj(engine->dev, ringbuf);
+	ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
 	if (ret)
 		goto unpin_ctx_obj;
 
-	ctx->engine[engine->id].lrc_vma = i915_gem_obj_to_ggtt(ctx_obj);
+	ectx->lrc_vma = i915_gem_obj_to_ggtt(ctx_obj);
 	intel_lr_context_descriptor_update(ctx, engine);
 	lrc_reg_state = kmap(lrc_state_page);
 	lrc_reg_state[CTX_RING_BUFFER_START+1] = ringbuf->vma->node.start;
-	ctx->engine[engine->id].lrc_reg_state = lrc_reg_state;
+	ectx->lrc_reg_state = lrc_reg_state;
 	ctx_obj->dirty = true;
 
 	/* Invalidate GuC TLB. */
@@ -1137,9 +1136,10 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
 static int intel_lr_context_pin(struct intel_context *ctx,
 				struct intel_engine_cs *engine)
 {
+	struct intel_engine_ctx *ectx = &ctx->engine[engine->id];
 	int ret = 0;
 
-	if (ctx->engine[engine->id].pin_count++ == 0) {
+	if (ectx->pin_count++ == 0) {
 		ret = intel_lr_context_do_pin(ctx, engine);
 		if (ret)
 			goto reset_pin_count;
@@ -1149,23 +1149,24 @@ static int intel_lr_context_pin(struct intel_context *ctx,
 	return ret;
 
 reset_pin_count:
-	ctx->engine[engine->id].pin_count = 0;
+	ectx->pin_count = 0;
 	return ret;
 }
 
 void intel_lr_context_unpin(struct intel_context *ctx,
 			    struct intel_engine_cs *engine)
 {
-	struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
+	struct intel_engine_ctx *ectx = &ctx->engine[engine->id];
 
 	WARN_ON(!mutex_is_locked(&ctx->i915->dev->struct_mutex));
-	if (--ctx->engine[engine->id].pin_count == 0) {
-		kunmap(kmap_to_page(ctx->engine[engine->id].lrc_reg_state));
-		intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
-		i915_gem_object_ggtt_unpin(ctx_obj);
-		ctx->engine[engine->id].lrc_vma = NULL;
-		ctx->engine[engine->id].lrc_desc = 0;
-		ctx->engine[engine->id].lrc_reg_state = NULL;
+
+	if (--ectx->pin_count == 0) {
+		intel_unpin_ringbuffer_obj(ectx->ringbuf);
+		i915_gem_object_ggtt_unpin(ectx->state);
+		kunmap(kmap_to_page(ectx->lrc_reg_state));
+		ectx->lrc_reg_state = NULL;
+		ectx->lrc_vma = NULL;
+		ectx->lrc_desc = 0;
 
 		i915_gem_context_unreference(ctx);
 	}
@@ -1176,8 +1177,7 @@ static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
 	int ret, i;
 	struct intel_engine_cs *engine = req->engine;
 	struct intel_ringbuffer *ringbuf = req->ringbuf;
-	struct drm_device *dev = engine->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = req->i915;
 	struct i915_workarounds *w = &dev_priv->workarounds;
 
 	if (w->count == 0)
@@ -1563,12 +1563,11 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
 
 static int gen8_init_common_ring(struct intel_engine_cs *engine)
 {
-	struct drm_device *dev = engine->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = engine->dev->dev_private;
+	struct intel_engine_ctx *ectx = &dev_priv->kernel_context->engine[engine->id];
 	unsigned int next_context_status_buffer_hw;
 
-	lrc_setup_hardware_status_page(engine,
-				       dev_priv->kernel_context->engine[engine->id].state);
+	lrc_setup_hardware_status_page(engine, ectx->state);
 
 	I915_WRITE_IMR(engine,
 		       ~(engine->irq_enable_mask | engine->irq_keep_mask));
@@ -2512,8 +2511,9 @@ void intel_lr_context_free(struct intel_context *ctx)
 	int i;
 
 	for (i = I915_NUM_ENGINES; --i >= 0; ) {
-		struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf;
-		struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
+		struct intel_engine_ctx *ectx = &ctx->engine[i];
+		struct intel_ringbuffer *ringbuf = ectx->ringbuf;
+		struct drm_i915_gem_object *ctx_obj = ectx->state;
 
 		if (!ctx_obj)
 			continue;
@@ -2523,7 +2523,7 @@ void intel_lr_context_free(struct intel_context *ctx)
 			i915_gem_object_ggtt_unpin(ctx_obj);
 		}
 
-		WARN_ON(ctx->engine[i].pin_count);
+		WARN_ON(ectx->pin_count);
 		intel_ringbuffer_free(ringbuf);
 		drm_gem_object_unreference(&ctx_obj->base);
 	}
@@ -2603,13 +2603,14 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 				    struct intel_engine_cs *engine)
 {
 	struct drm_device *dev = engine->dev;
+	struct intel_engine_ctx *ectx = &ctx->engine[engine->id];
 	struct drm_i915_gem_object *ctx_obj;
 	uint32_t context_size;
 	struct intel_ringbuffer *ringbuf;
 	int ret;
 
 	WARN_ON(ctx->legacy_hw_ctx.rcs_state != NULL);
-	WARN_ON(ctx->engine[engine->id].state);
+	WARN_ON(ectx->state);
 
 	context_size = round_up(intel_lr_context_size(engine), 4096);
 
@@ -2634,8 +2635,8 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 		goto error_ringbuf;
 	}
 
-	ctx->engine[engine->id].ringbuf = ringbuf;
-	ctx->engine[engine->id].state = ctx_obj;
+	ectx->ringbuf = ringbuf;
+	ectx->state = ctx_obj;
 
 	if (ctx != ctx->i915->kernel_context && engine->init_context) {
 		struct drm_i915_gem_request *req;
@@ -2662,23 +2663,22 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 	intel_ringbuffer_free(ringbuf);
 error_deref_obj:
 	drm_gem_object_unreference(&ctx_obj->base);
-	ctx->engine[engine->id].ringbuf = NULL;
-	ctx->engine[engine->id].state = NULL;
+	ectx->ringbuf = NULL;
+	ectx->state = NULL;
 	return ret;
 }
 
 void intel_lr_context_reset(struct drm_device *dev,
-			struct intel_context *ctx)
+			    struct intel_context *ctx)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	int i;
 
 	for_each_engine(engine, dev_priv, i) {
-		struct drm_i915_gem_object *ctx_obj =
-				ctx->engine[engine->id].state;
-		struct intel_ringbuffer *ringbuf =
-				ctx->engine[engine->id].ringbuf;
+		struct intel_engine_ctx *ectx = &ctx->engine[engine->id];
+		struct drm_i915_gem_object *ctx_obj = ectx->state;
+		struct intel_ringbuffer *ringbuf = ectx->ringbuf;
 		uint32_t *reg_state;
 		struct page *page;
 
-- 
1.9.1

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

[PATCH v3 2/2] drm/i915: tidy up a few more references to engine[]

[Dave Gordon]

Now that we have a name for the previously anonymous per-engine structure
embedded inside the intel_context for the benefit of execlist mode, we
can optimise a few more places that access this array of structures.
This may improve the compiler's ability to avoid redundant dereference
and index operations.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 13 +++++--------
 drivers/gpu/drm/i915/i915_guc_submission.c | 17 +++++++++--------
 2 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ccdca2c..ce3b5e9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1966,16 +1966,13 @@ static int i915_context_status(struct seq_file *m, void *unused)
 		if (i915.enable_execlists) {
 			seq_putc(m, '\n');
 			for_each_engine(engine, dev_priv, i) {
-				struct drm_i915_gem_object *ctx_obj =
-					ctx->engine[i].state;
-				struct intel_ringbuffer *ringbuf =
-					ctx->engine[i].ringbuf;
+				struct intel_engine_ctx *ectx = &ctx->engine[i];
 
 				seq_printf(m, "%s: ", engine->name);
-				if (ctx_obj)
-					describe_obj(m, ctx_obj);
-				if (ringbuf)
-					describe_ctx_ringbuf(m, ringbuf);
+				if (ectx->state)
+					describe_obj(m, ectx->state);
+				if (ectx->ringbuf)
+					describe_ctx_ringbuf(m, ectx->ringbuf);
 				seq_putc(m, '\n');
 			}
 		} else {
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index ae1f58d..7352023 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -392,6 +392,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 
 	for_each_engine(engine, dev_priv, i) {
 		struct guc_execlist_context *lrc = &desc.lrc[engine->guc_id];
+		struct intel_engine_ctx *ectx = &ctx->engine[i];
 		struct drm_i915_gem_object *obj;
 		uint64_t ctx_desc;
 
@@ -402,7 +403,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 		 * for now who owns a GuC client. But for future owner of GuC
 		 * client, need to make sure lrc is pinned prior to enter here.
 		 */
-		obj = ctx->engine[i].state;
+		obj = ectx->state;
 		if (!obj)
 			break;	/* XXX: continue? */
 
@@ -415,7 +416,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 		lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
 				(engine->guc_id << GUC_ELC_ENGINE_OFFSET);
 
-		obj = ctx->engine[i].ringbuf->obj;
+		obj = ectx->ringbuf->obj;
 
 		lrc->ring_begin = i915_gem_obj_ggtt_offset(obj);
 		lrc->ring_end = lrc->ring_begin + obj->base.size - 1;
@@ -987,19 +988,19 @@ int intel_guc_suspend(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc *guc = &dev_priv->guc;
-	struct intel_context *ctx;
+	struct intel_engine_ctx *ectx;
 	u32 data[3];
 
 	if (!i915.enable_guc_submission)
 		return 0;
 
-	ctx = dev_priv->kernel_context;
+	ectx = &dev_priv->kernel_context->engine[RCS];
 
 	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
 	/* any value greater than GUC_POWER_D0 */
 	data[1] = GUC_POWER_D1;
 	/* first page is shared data with GuC */
-	data[2] = i915_gem_obj_ggtt_offset(ctx->engine[RCS].state);
+	data[2] = i915_gem_obj_ggtt_offset(ectx->state);
 
 	return host2guc_action(guc, data, ARRAY_SIZE(data));
 }
@@ -1013,18 +1014,18 @@ int intel_guc_resume(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc *guc = &dev_priv->guc;
-	struct intel_context *ctx;
+	struct intel_engine_ctx *ectx;
 	u32 data[3];
 
 	if (!i915.enable_guc_submission)
 		return 0;
 
-	ctx = dev_priv->kernel_context;
+;	ectx = &dev_priv->kernel_context->engine[RCS];
 
 	data[0] = HOST2GUC_ACTION_EXIT_S_STATE;
 	data[1] = GUC_POWER_D0;
 	/* first page is shared data with GuC */
-	data[2] = i915_gem_obj_ggtt_offset(ctx->engine[RCS].state);
+	data[2] = i915_gem_obj_ggtt_offset(ectx->state);
 
 	return host2guc_action(guc, data, ARRAY_SIZE(data));
 }
-- 
1.9.1

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