Re: [PATCH 03/13] drm/i915: Avoid invariant conditionals in lrc interrupt handler

Daniel Vetter
2016-01-11T08:29:32Z

On Fri, Jan 08, 2016 at 11:29:42AM +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;
> }

tbh I'd go full monty and just cache the entire context descriptor.
-Daniel

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

--
Daniel Vetter
Software Engineer, Intel Corporation