diff mbox

[3/6] drm/i915: Avoid invariant conditionals in lrc interrupt handler

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

Commit Message

Tvrtko Ursulin Jan. 7, 2016, 4:36 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

There is no need to check on what Gen we are running on every
interrupt and every command submission. We can instead set up
some of that when engines are initialized, store it in the
engine structure and use it directly at runtime.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 36 ++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  2 ++
 2 files changed, 22 insertions(+), 16 deletions(-)

Comments

Chris Wilson Jan. 7, 2016, 4:42 p.m. UTC | #1 
On Thu, Jan 07, 2016 at 04:36:18PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> There is no need to check on what Gen we are running on every
> interrupt and every command submission. We can instead set up
> some of that when engines are initialized, store it in the
> engine structure and use it directly at runtime.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c        | 36 ++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  2 ++
>  2 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 8b6071fcd743..84977a6e6f3f 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -298,29 +298,15 @@ uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
>  				     struct intel_engine_cs *ring)
>  {
>  	struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
> -	uint64_t desc;
> +	uint64_t desc = ring->ctx_desc_template;
>  	uint64_t lrca = i915_gem_obj_ggtt_offset(ctx_obj) +
>  			LRC_PPHWSP_PN * PAGE_SIZE;
>  
>  	WARN_ON(lrca & 0xFFFFFFFF00000FFFULL);
>  
> -	desc = GEN8_CTX_VALID;
> -	desc |= GEN8_CTX_ADDRESSING_MODE(dev) << GEN8_CTX_ADDRESSING_MODE_SHIFT;
> -	if (IS_GEN8(ctx_obj->base.dev))
> -		desc |= GEN8_CTX_L3LLC_COHERENT;
> -	desc |= GEN8_CTX_PRIVILEGE;
>  	desc |= lrca;
>  	desc |= (u64)intel_execlists_ctx_id(ctx_obj) << GEN8_CTX_ID_SHIFT;
>  
> -	/* TODO: WaDisableLiteRestore when we start using semaphore
> -	 * signalling between Command Streamers */
> -	/* desc |= GEN8_CTX_FORCE_RESTORE; */
> -
> -	/* WaEnableForceRestoreInCtxtDescForVCS:skl */
> -	/* WaEnableForceRestoreInCtxtDescForVCS:bxt */
> -	if (disable_lite_restore_wa(ring))
> -		desc |= GEN8_CTX_FORCE_RESTORE;
> -
>  	return desc;
>  }
>  
> @@ -556,7 +542,7 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring)
>  		}
>  	}
>  
> -	if (disable_lite_restore_wa(ring)) {
> +	if (ring->disable_lite_restore_wa) {
>  		/* Prevent a ctx to preempt itself */
>  		if ((status & GEN8_CTX_STATUS_ACTIVE_IDLE) &&
>  		    (submit_contexts != 0))

Didn't it occur to you that this code is garbage?

> @@ -1980,6 +1966,24 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
>  		goto error;
>  	}
>  
> +	ring->disable_lite_restore_wa = disable_lite_restore_wa(ring);
> +
> +	ring->ctx_desc_template = GEN8_CTX_VALID;
> +	ring->ctx_desc_template |= GEN8_CTX_ADDRESSING_MODE(dev) <<
> +				   GEN8_CTX_ADDRESSING_MODE_SHIFT;
> +	if (IS_GEN8(dev))
> +		ring->ctx_desc_template |= GEN8_CTX_L3LLC_COHERENT;
> +	ring->ctx_desc_template |= GEN8_CTX_PRIVILEGE;
> +
> +	/* TODO: WaDisableLiteRestore when we start using semaphore
> +	 * signalling between Command Streamers */
> +	/* ring->ctx_desc_template |= GEN8_CTX_FORCE_RESTORE; */
> +
> +	/* WaEnableForceRestoreInCtxtDescForVCS:skl */
> +	/* WaEnableForceRestoreInCtxtDescForVCS:bxt */
> +	if (ring->disable_lite_restore_wa)
> +		ring->ctx_desc_template |= GEN8_CTX_FORCE_RESTORE;
> +
>  	return 0;
>  
>  error:
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 49574ffe54bc..0b91a4b77359 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -268,6 +268,8 @@ struct  intel_engine_cs {
>  	struct list_head execlist_queue;
>  	struct list_head execlist_retired_req_list;
>  	u8 next_context_status_buffer;
> +	bool disable_lite_restore_wa;

You don't need that as a separate flag. You know I sent this patch last
year?
-Chris
Tvrtko Ursulin Jan. 7, 2016, 5:16 p.m. UTC | #2 
On 07/01/16 16:42, Chris Wilson wrote:
> On Thu, Jan 07, 2016 at 04:36:18PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> There is no need to check on what Gen we are running on every
>> interrupt and every command submission. We can instead set up
>> some of that when engines are initialized, store it in the
>> engine structure and use it directly at runtime.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_lrc.c        | 36 ++++++++++++++++++---------------
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  2 ++
>>   2 files changed, 22 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 8b6071fcd743..84977a6e6f3f 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -298,29 +298,15 @@ uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
>>   				     struct intel_engine_cs *ring)
>>   {
>>   	struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
>> -	uint64_t desc;
>> +	uint64_t desc = ring->ctx_desc_template;
>>   	uint64_t lrca = i915_gem_obj_ggtt_offset(ctx_obj) +
>>   			LRC_PPHWSP_PN * PAGE_SIZE;
>>
>>   	WARN_ON(lrca & 0xFFFFFFFF00000FFFULL);
>>
>> -	desc = GEN8_CTX_VALID;
>> -	desc |= GEN8_CTX_ADDRESSING_MODE(dev) << GEN8_CTX_ADDRESSING_MODE_SHIFT;
>> -	if (IS_GEN8(ctx_obj->base.dev))
>> -		desc |= GEN8_CTX_L3LLC_COHERENT;
>> -	desc |= GEN8_CTX_PRIVILEGE;
>>   	desc |= lrca;
>>   	desc |= (u64)intel_execlists_ctx_id(ctx_obj) << GEN8_CTX_ID_SHIFT;
>>
>> -	/* TODO: WaDisableLiteRestore when we start using semaphore
>> -	 * signalling between Command Streamers */
>> -	/* desc |= GEN8_CTX_FORCE_RESTORE; */
>> -
>> -	/* WaEnableForceRestoreInCtxtDescForVCS:skl */
>> -	/* WaEnableForceRestoreInCtxtDescForVCS:bxt */
>> -	if (disable_lite_restore_wa(ring))
>> -		desc |= GEN8_CTX_FORCE_RESTORE;
>> -
>>   	return desc;
>>   }
>>
>> @@ -556,7 +542,7 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring)
>>   		}
>>   	}
>>
>> -	if (disable_lite_restore_wa(ring)) {
>> +	if (ring->disable_lite_restore_wa) {
>>   		/* Prevent a ctx to preempt itself */
>>   		if ((status & GEN8_CTX_STATUS_ACTIVE_IDLE) &&
>>   		    (submit_contexts != 0))
>
> Didn't it occur to you that this code is garbage?

No, my default position is that I don't understand it well enough. If 
you mean that the two if statements here can be merged and have a single 
execlists_context_unqueue call site, sure, but why not do it in simpler 
steps.

>
>> @@ -1980,6 +1966,24 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
>>   		goto error;
>>   	}
>>
>> +	ring->disable_lite_restore_wa = disable_lite_restore_wa(ring);
>> +
>> +	ring->ctx_desc_template = GEN8_CTX_VALID;
>> +	ring->ctx_desc_template |= GEN8_CTX_ADDRESSING_MODE(dev) <<
>> +				   GEN8_CTX_ADDRESSING_MODE_SHIFT;
>> +	if (IS_GEN8(dev))
>> +		ring->ctx_desc_template |= GEN8_CTX_L3LLC_COHERENT;
>> +	ring->ctx_desc_template |= GEN8_CTX_PRIVILEGE;
>> +
>> +	/* TODO: WaDisableLiteRestore when we start using semaphore
>> +	 * signalling between Command Streamers */
>> +	/* ring->ctx_desc_template |= GEN8_CTX_FORCE_RESTORE; */
>> +
>> +	/* WaEnableForceRestoreInCtxtDescForVCS:skl */
>> +	/* WaEnableForceRestoreInCtxtDescForVCS:bxt */
>> +	if (ring->disable_lite_restore_wa)
>> +		ring->ctx_desc_template |= GEN8_CTX_FORCE_RESTORE;
>> +
>>   	return 0;
>>
>>   error:
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 49574ffe54bc..0b91a4b77359 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -268,6 +268,8 @@ struct  intel_engine_cs {
>>   	struct list_head execlist_queue;
>>   	struct list_head execlist_retired_req_list;
>>   	u8 next_context_status_buffer;
>> +	bool disable_lite_restore_wa;
>
> You don't need that as a separate flag. You know I sent this patch last
> year?

Nope. :/

Regards,

Tvrtko
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 8b6071fcd743..84977a6e6f3f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -298,29 +298,15 @@  uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
 				     struct intel_engine_cs *ring)
 {
 	struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
-	uint64_t desc;
+	uint64_t desc = ring->ctx_desc_template;
 	uint64_t lrca = i915_gem_obj_ggtt_offset(ctx_obj) +
 			LRC_PPHWSP_PN * PAGE_SIZE;
 
 	WARN_ON(lrca & 0xFFFFFFFF00000FFFULL);
 
-	desc = GEN8_CTX_VALID;
-	desc |= GEN8_CTX_ADDRESSING_MODE(dev) << GEN8_CTX_ADDRESSING_MODE_SHIFT;
-	if (IS_GEN8(ctx_obj->base.dev))
-		desc |= GEN8_CTX_L3LLC_COHERENT;
-	desc |= GEN8_CTX_PRIVILEGE;
 	desc |= lrca;
 	desc |= (u64)intel_execlists_ctx_id(ctx_obj) << GEN8_CTX_ID_SHIFT;
 
-	/* TODO: WaDisableLiteRestore when we start using semaphore
-	 * signalling between Command Streamers */
-	/* desc |= GEN8_CTX_FORCE_RESTORE; */
-
-	/* WaEnableForceRestoreInCtxtDescForVCS:skl */
-	/* WaEnableForceRestoreInCtxtDescForVCS:bxt */
-	if (disable_lite_restore_wa(ring))
-		desc |= GEN8_CTX_FORCE_RESTORE;
-
 	return desc;
 }
 
@@ -556,7 +542,7 @@  void intel_lrc_irq_handler(struct intel_engine_cs *ring)
 		}
 	}
 
-	if (disable_lite_restore_wa(ring)) {
+	if (ring->disable_lite_restore_wa) {
 		/* Prevent a ctx to preempt itself */
 		if ((status & GEN8_CTX_STATUS_ACTIVE_IDLE) &&
 		    (submit_contexts != 0))
@@ -1980,6 +1966,24 @@  static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
 		goto error;
 	}
 
+	ring->disable_lite_restore_wa = disable_lite_restore_wa(ring);
+
+	ring->ctx_desc_template = GEN8_CTX_VALID;
+	ring->ctx_desc_template |= GEN8_CTX_ADDRESSING_MODE(dev) <<
+				   GEN8_CTX_ADDRESSING_MODE_SHIFT;
+	if (IS_GEN8(dev))
+		ring->ctx_desc_template |= GEN8_CTX_L3LLC_COHERENT;
+	ring->ctx_desc_template |= GEN8_CTX_PRIVILEGE;
+
+	/* TODO: WaDisableLiteRestore when we start using semaphore
+	 * signalling between Command Streamers */
+	/* ring->ctx_desc_template |= GEN8_CTX_FORCE_RESTORE; */
+
+	/* WaEnableForceRestoreInCtxtDescForVCS:skl */
+	/* WaEnableForceRestoreInCtxtDescForVCS:bxt */
+	if (ring->disable_lite_restore_wa)
+		ring->ctx_desc_template |= GEN8_CTX_FORCE_RESTORE;
+
 	return 0;
 
 error:
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 49574ffe54bc..0b91a4b77359 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -268,6 +268,8 @@  struct  intel_engine_cs {
 	struct list_head execlist_queue;
 	struct list_head execlist_retired_req_list;
 	u8 next_context_status_buffer;
+	bool disable_lite_restore_wa;
+	u32 ctx_desc_template;
 	u32             irq_keep_mask; /* bitmask for interrupts that should not be masked */
 	int		(*emit_request)(struct drm_i915_gem_request *request);
 	int		(*emit_flush)(struct drm_i915_gem_request *request,