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

Message ID 1458321994-12212-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin March 18, 2016, 5:26 p.m. UTC
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(-)

Comments

Dave Gordon March 21, 2016, 3:23 p.m. UTC | #1
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.
Chris Wilson March 22, 2016, 9:28 a.m. UTC | #2
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
Dave Gordon March 22, 2016, 11:20 a.m. UTC | #3
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.
Tvrtko Ursulin March 22, 2016, 11:47 a.m. UTC | #4
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

Patch
diff mbox

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;