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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx