Message ID | 1452184581-21075-4-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
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 --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,