Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes: > Emulate HW to track and manage ELSP queue. A set of SW ports are defined > and requests are assigned to these ports before submitting them to HW. This > helps in cleaning up incomplete requests during reset recovery easier > especially after engine reset by decoupling elsp queue management. This > will become more clear in the next patch. > > In the engine reset case we want to resume where we left-off after skipping > the incomplete batch which requires checking the elsp queue, removing > element and fixing elsp_submitted counts in some cases. Instead of directly > manipulating the elsp queue from reset path we can examine these ports, fix > up ringbuffer pointers using the incomplete request and restart submissions > again after reset. > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx> > Cc: Arun Siluvery <arun.siluvery@xxxxxxxxxxxxxxx> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Link: http://patchwork.freedesktop.org/patch/msgid/1470414607-32453-3-git-send-email-arun.siluvery@xxxxxxxxxxxxxxx > --- > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- > drivers/gpu/drm/i915/i915_gem.c | 6 +- > drivers/gpu/drm/i915/i915_gem_request.c | 11 +- > drivers/gpu/drm/i915/i915_gem_request.h | 24 +- > drivers/gpu/drm/i915/intel_lrc.c | 417 +++++++++++--------------------- > drivers/gpu/drm/i915/intel_lrc.h | 2 - > drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +- > 7 files changed, 160 insertions(+), 309 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 9bd41581b592..f1f937a64c27 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2200,7 +2200,7 @@ static int i915_execlists(struct seq_file *m, void *data) > status_pointer = I915_READ(RING_CONTEXT_STATUS_PTR(engine)); > seq_printf(m, "\tStatus pointer: 0x%08X\n", status_pointer); > > - read_pointer = engine->next_context_status_buffer; > + read_pointer = GEN8_CSB_READ_PTR(status_pointer); > write_pointer = GEN8_CSB_WRITE_PTR(status_pointer); > if (read_pointer > write_pointer) > write_pointer += GEN8_CSB_ENTRIES; > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index f4f8eaa90f2a..19715f5e698f 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2443,7 +2443,11 @@ static void i915_gem_reset_engine_cleanup(struct intel_engine_cs *engine) > /* Ensure irq handler finishes or is cancelled. */ > tasklet_kill(&engine->irq_tasklet); > > - intel_execlists_cancel_requests(engine); > + INIT_LIST_HEAD(&engine->execlist_queue); > + i915_gem_request_assign(&engine->execlist_port[0].request, > + NULL); > + i915_gem_request_assign(&engine->execlist_port[1].request, > + NULL); How about engine->reset() which would contain these? > } > > /* > diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c > index 6c2553715263..49cca4233a8e 100644 > --- a/drivers/gpu/drm/i915/i915_gem_request.c > +++ b/drivers/gpu/drm/i915/i915_gem_request.c > @@ -736,16 +736,18 @@ complete: > return ret; > } > > -static void engine_retire_requests(struct intel_engine_cs *engine) > +static bool engine_retire_requests(struct intel_engine_cs *engine) > { > struct drm_i915_gem_request *request, *next; > > list_for_each_entry_safe(request, next, &engine->request_list, link) { > if (!i915_gem_request_completed(request)) > - break; > + return false; > > i915_gem_request_retire(request); > } > + > + return true; > } > > void i915_gem_retire_requests(struct drm_i915_private *dev_priv) > @@ -759,9 +761,8 @@ void i915_gem_retire_requests(struct drm_i915_private *dev_priv) > > GEM_BUG_ON(!dev_priv->gt.awake); > > - for_each_engine(engine, dev_priv) { > - engine_retire_requests(engine); > - if (!intel_engine_is_active(engine)) > + for_each_engine_masked(engine, dev_priv, dev_priv->gt.active_engines) { > + if (engine_retire_requests(engine)) > dev_priv->gt.active_engines &= ~intel_engine_flag(engine); > } > These were already stripped to another patch. Good. > diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h > index 3496e28785e7..32123e38ef4b 100644 > --- a/drivers/gpu/drm/i915/i915_gem_request.h > +++ b/drivers/gpu/drm/i915/i915_gem_request.h > @@ -94,6 +94,9 @@ struct drm_i915_gem_request { > /** Position in the ringbuffer of the end of the whole request */ > u32 tail; > > + /** Position in the ringbuffer after the end of the whole request */ > + u32 wa_tail; > + > /** Preallocate space in the ringbuffer for the emitting the request */ > u32 reserved_space; > > @@ -130,27 +133,8 @@ struct drm_i915_gem_request { > /** process identifier submitting this request */ > struct pid *pid; > > - /** > - * The ELSP only accepts two elements at a time, so we queue > - * context/tail pairs on a given queue (ring->execlist_queue) until the > - * hardware is available. The queue serves a double purpose: we also use > - * it to keep track of the up to 2 contexts currently in the hardware > - * (usually one in execution and the other queued up by the GPU): We > - * only remove elements from the head of the queue when the hardware > - * informs us that an element has been completed. > - * > - * All accesses to the queue are mediated by a spinlock > - * (ring->execlist_lock). > - */ > - > - /** Execlist link in the submission queue.*/ > + /** Link in the execlist submission queue, guarded by execlist_lock. */ > struct list_head execlist_link; > - > - /** Execlists no. of times this request has been sent to the ELSP */ > - int elsp_submitted; > - > - /** Execlists context hardware id. */ > - unsigned int ctx_hw_id; > }; > > extern const struct fence_ops i915_fence_ops; > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 309c5d9b1c57..56b5aa8a35c4 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -156,6 +156,11 @@ > #define GEN8_CTX_STATUS_COMPLETE (1 << 4) > #define GEN8_CTX_STATUS_LITE_RESTORE (1 << 15) > > +#define GEN8_CTX_STATUS_COMPLETED_MASK \ > + (GEN8_CTX_STATUS_ACTIVE_IDLE | \ > + GEN8_CTX_STATUS_PREEMPTED | \ > + GEN8_CTX_STATUS_ELEMENT_SWITCH) > + > #define CTX_LRI_HEADER_0 0x01 > #define CTX_CONTEXT_CONTROL 0x02 > #define CTX_RING_HEAD 0x04 > @@ -263,12 +268,10 @@ logical_ring_init_platform_invariants(struct intel_engine_cs *engine) > { > struct drm_i915_private *dev_priv = engine->i915; > > - if (IS_GEN8(dev_priv) || IS_GEN9(dev_priv)) > - engine->idle_lite_restore_wa = ~0; > - > - engine->disable_lite_restore_wa = (IS_SKL_REVID(dev_priv, 0, SKL_REVID_B0) || > - IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1)) && > - (engine->id == VCS || engine->id == VCS2); > + engine->disable_lite_restore_wa = > + (IS_SKL_REVID(dev_priv, 0, SKL_REVID_B0) || > + IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1)) && > + (engine->id == VCS || engine->id == VCS2); Candidate for another cleanup patch after as both revids smells obsolete. > > engine->ctx_desc_template = GEN8_CTX_VALID; > if (IS_GEN8(dev_priv)) > @@ -328,36 +331,6 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx, > return ctx->engine[engine->id].lrc_desc; > } > > -static void execlists_elsp_write(struct drm_i915_gem_request *rq0, > - struct drm_i915_gem_request *rq1) > -{ > - > - struct intel_engine_cs *engine = rq0->engine; > - struct drm_i915_private *dev_priv = rq0->i915; > - uint64_t desc[2]; > - > - if (rq1) { > - desc[1] = intel_lr_context_descriptor(rq1->ctx, rq1->engine); > - rq1->elsp_submitted++; > - } else { > - desc[1] = 0; > - } > - > - desc[0] = intel_lr_context_descriptor(rq0->ctx, rq0->engine); > - rq0->elsp_submitted++; > - > - /* You must always write both descriptors in the order below. */ > - I915_WRITE_FW(RING_ELSP(engine), upper_32_bits(desc[1])); > - I915_WRITE_FW(RING_ELSP(engine), lower_32_bits(desc[1])); > - > - I915_WRITE_FW(RING_ELSP(engine), upper_32_bits(desc[0])); > - /* The context is automatically loaded after the following */ > - I915_WRITE_FW(RING_ELSP(engine), lower_32_bits(desc[0])); > - > - /* ELSP is a wo register, use another nearby reg for posting */ > - POSTING_READ_FW(RING_EXECLIST_STATUS_LO(engine)); > -} > - > static void > execlists_update_context_pdps(struct i915_hw_ppgtt *ppgtt, u32 *reg_state) > { > @@ -367,13 +340,12 @@ execlists_update_context_pdps(struct i915_hw_ppgtt *ppgtt, u32 *reg_state) > ASSIGN_CTX_PDP(ppgtt, reg_state, 0); > } > > -static void execlists_update_context(struct drm_i915_gem_request *rq) > +static u64 execlists_update_context(struct drm_i915_gem_request *rq) > { > - struct intel_engine_cs *engine = rq->engine; > + struct intel_context *ce = &rq->ctx->engine[rq->engine->id]; > struct i915_hw_ppgtt *ppgtt = rq->ctx->ppgtt; > - uint32_t *reg_state = rq->ctx->engine[engine->id].lrc_reg_state; > > - reg_state[CTX_RING_TAIL+1] = intel_ring_offset(rq->ring, rq->tail); > + ce->lrc_reg_state[CTX_RING_TAIL+1] = intel_ring_offset(rq->ring, rq->tail); > > /* True 32b PPGTT with dynamic page allocation: update PDP > * registers and point the unallocated PDPs to scratch page. > @@ -381,32 +353,14 @@ static void execlists_update_context(struct drm_i915_gem_request *rq) > * in 48-bit mode. > */ > if (ppgtt && !USES_FULL_48BIT_PPGTT(ppgtt->base.dev)) > - execlists_update_context_pdps(ppgtt, reg_state); > -} > - > -static void execlists_elsp_submit_contexts(struct drm_i915_gem_request *rq0, > - struct drm_i915_gem_request *rq1) > -{ > - struct drm_i915_private *dev_priv = rq0->i915; > - unsigned int fw_domains = rq0->engine->fw_domains; > - > - execlists_update_context(rq0); > - > - if (rq1) > - execlists_update_context(rq1); > + execlists_update_context_pdps(ppgtt, ce->lrc_reg_state); > > - spin_lock_irq(&dev_priv->uncore.lock); > - intel_uncore_forcewake_get__locked(dev_priv, fw_domains); > - > - execlists_elsp_write(rq0, rq1); > - > - intel_uncore_forcewake_put__locked(dev_priv, fw_domains); > - spin_unlock_irq(&dev_priv->uncore.lock); > + return ce->lrc_desc; > } > > -static inline void execlists_context_status_change( > - struct drm_i915_gem_request *rq, > - unsigned long status) > +static inline void > +execlists_context_status_change(struct drm_i915_gem_request *rq, > + unsigned long status) > { > /* > * Only used when GVT-g is enabled now. When GVT-g is disabled, > @@ -418,122 +372,104 @@ static inline void execlists_context_status_change( > atomic_notifier_call_chain(&rq->ctx->status_notifier, status, rq); > } > > -static void execlists_unqueue(struct intel_engine_cs *engine) > +static void execlists_submit_ports(struct intel_engine_cs *engine) > { > - struct drm_i915_gem_request *req0 = NULL, *req1 = NULL; > - struct drm_i915_gem_request *cursor, *tmp; > - > - assert_spin_locked(&engine->execlist_lock); > - > - /* > - * If irqs are not active generate a warning as batches that finish > - * without the irqs may get lost and a GPU Hang may occur. > - */ > - WARN_ON(!intel_irqs_enabled(engine->i915)); > - > - /* Try to read in pairs */ > - list_for_each_entry_safe(cursor, tmp, &engine->execlist_queue, > - execlist_link) { > - if (!req0) { > - req0 = cursor; > - } else if (req0->ctx == cursor->ctx) { > - /* Same ctx: ignore first request, as second request > - * will update tail past first request's workload */ > - cursor->elsp_submitted = req0->elsp_submitted; > - list_del(&req0->execlist_link); > - i915_gem_request_put(req0); > - req0 = cursor; > - } else { > - if (IS_ENABLED(CONFIG_DRM_I915_GVT)) { > - /* > - * req0 (after merged) ctx requires single > - * submission, stop picking > - */ > - if (req0->ctx->execlists_force_single_submission) > - break; > - /* > - * req0 ctx doesn't require single submission, > - * but next req ctx requires, stop picking > - */ > - if (cursor->ctx->execlists_force_single_submission) > - break; > - } > - req1 = cursor; > - WARN_ON(req1->elsp_submitted); > - break; > - } > - } > - > - if (unlikely(!req0)) > - return; > - > - execlists_context_status_change(req0, INTEL_CONTEXT_SCHEDULE_IN); > + struct drm_i915_private *dev_priv = engine->i915; > + u32 *elsp = dev_priv->regs + i915_mmio_reg_offset(RING_ELSP(engine)); > + u64 desc[2]; > > - if (req1) > - execlists_context_status_change(req1, > + if (!engine->execlist_port[0].count) > + execlists_context_status_change(engine->execlist_port[0].request, > INTEL_CONTEXT_SCHEDULE_IN); > + desc[0] = execlists_update_context(engine->execlist_port[0].request); > + engine->preempt_wa = engine->execlist_port[0].count++; > > - if (req0->elsp_submitted & engine->idle_lite_restore_wa) { > - /* > - * WaIdleLiteRestore: make sure we never cause a lite restore > - * with HEAD==TAIL. > - * > - * Apply the wa NOOPS to prevent ring:HEAD == req:TAIL as we > - * resubmit the request. See gen8_emit_request() for where we > - * prepare the padding after the end of the request. > - */ > - req0->tail += 8; > - req0->tail &= req0->ring->size - 1; > + if (engine->execlist_port[1].request) { > + GEM_BUG_ON(engine->execlist_port[1].count); > + execlists_context_status_change(engine->execlist_port[1].request, > + INTEL_CONTEXT_SCHEDULE_IN); > + desc[1] = execlists_update_context(engine->execlist_port[1].request); > + engine->execlist_port[1].count = 1; > + } else { > + desc[1] = 0; > } > > - execlists_elsp_submit_contexts(req0, req1); > + /* You must always write both descriptors in the order below. */ > + writel(upper_32_bits(desc[1]), elsp); > + writel(lower_32_bits(desc[1]), elsp); > + > + writel(upper_32_bits(desc[0]), elsp); > + /* The context is automatically loaded after the following */ > + writel(lower_32_bits(desc[0]), elsp); > } > > -static unsigned int > -execlists_check_remove_request(struct intel_engine_cs *engine, u32 ctx_id) > +static bool merge_ctx(struct i915_gem_context *prev, > + struct i915_gem_context *next) > { > - struct drm_i915_gem_request *head_req; > + if (prev != next) > + return false; > > - assert_spin_locked(&engine->execlist_lock); > + if (IS_ENABLED(CONFIG_DRM_I915_GVT) && > + prev->execlists_force_single_submission) > + return false; > > - head_req = list_first_entry_or_null(&engine->execlist_queue, > - struct drm_i915_gem_request, > - execlist_link); > - > - if (WARN_ON(!head_req || (head_req->ctx_hw_id != ctx_id))) > - return 0; > + return true; > +} > > - WARN(head_req->elsp_submitted == 0, "Never submitted head request\n"); > +static void execlists_context_unqueue(struct intel_engine_cs *engine) > +{ > + struct drm_i915_gem_request *cursor, *last; > + struct execlist_port *port = engine->execlist_port; > + bool submit = false; > + > + last = port->request; > + if (last) > + /* WaIdleLiteRestore:bdw,skl > + * Apply the wa NOOPs to prevent ring:HEAD == req:TAIL > + * as we resubmit the request. See gen8_emit_request() > + * for where we prepare the padding after the end of the > + * request. > + */ > + last->tail = last->wa_tail; > > - if (--head_req->elsp_submitted > 0) > - return 0; > + /* Try to read in pairs and fill both submission ports */ > + spin_lock(&engine->execlist_lock); > + list_for_each_entry(cursor, &engine->execlist_queue, execlist_link) { > + if (last && !merge_ctx(cursor->ctx, last->ctx)) { > + if (port != engine->execlist_port) > + break; In here you will merge ctx also for the second port? The previous version of the code was careful to only merge for the first port/request. > > - execlists_context_status_change(head_req, INTEL_CONTEXT_SCHEDULE_OUT); > + if (IS_ENABLED(CONFIG_DRM_I915_GVT) && > + cursor->ctx->execlists_force_single_submission) > + break; > > - list_del(&head_req->execlist_link); > - i915_gem_request_put(head_req); > + i915_gem_request_assign(&port->request, last); > + port++; > + } > + last = cursor; > + submit = true; > + } > + if (submit) { > + i915_gem_request_assign(&port->request, last); > + engine->execlist_queue.next = &cursor->execlist_link; > + cursor->execlist_link.prev = &engine->execlist_queue; We keep the cursor pointing to the request that will be in port[0].request? while (!list_empty(engine->execlist_queue)) and then removing from the list would have made the fill loop more complex? > + } > + spin_unlock(&engine->execlist_lock); > > - return 1; > + if (submit) > + execlists_submit_ports(engine); > } > > -static u32 > -get_context_status(struct intel_engine_cs *engine, unsigned int read_pointer, > - u32 *context_id) > +static bool execlists_elsp_idle(struct intel_engine_cs *engine) > { > - struct drm_i915_private *dev_priv = engine->i915; > - u32 status; > - > - read_pointer %= GEN8_CSB_ENTRIES; > - > - status = I915_READ_FW(RING_CONTEXT_STATUS_BUF_LO(engine, read_pointer)); > - > - if (status & GEN8_CTX_STATUS_IDLE_ACTIVE) > - return 0; > + return !engine->execlist_port[0].request; > +} > > - *context_id = I915_READ_FW(RING_CONTEXT_STATUS_BUF_HI(engine, > - read_pointer)); > +static bool execlists_elsp_ready(struct intel_engine_cs *engine) > +{ > + int idx = !engine->disable_lite_restore_wa && !engine->preempt_wa; > > - return status; > + return !engine->execlist_port[idx].request; > } > > /* > @@ -544,100 +480,63 @@ static void intel_lrc_irq_handler(unsigned long data) > { > struct intel_engine_cs *engine = (struct intel_engine_cs *)data; > struct drm_i915_private *dev_priv = engine->i915; > - u32 status_pointer; > - unsigned int read_pointer, write_pointer; > - u32 csb[GEN8_CSB_ENTRIES][2]; > - unsigned int csb_read = 0, i; > - unsigned int submit_contexts = 0; > > intel_uncore_forcewake_get(dev_priv, engine->fw_domains); > > - status_pointer = I915_READ_FW(RING_CONTEXT_STATUS_PTR(engine)); > - > - read_pointer = engine->next_context_status_buffer; > - write_pointer = GEN8_CSB_WRITE_PTR(status_pointer); > - if (read_pointer > write_pointer) > - write_pointer += GEN8_CSB_ENTRIES; > - > - while (read_pointer < write_pointer) { > - if (WARN_ON_ONCE(csb_read == GEN8_CSB_ENTRIES)) > - break; > - csb[csb_read][0] = get_context_status(engine, ++read_pointer, > - &csb[csb_read][1]); > - csb_read++; > - } > - > - engine->next_context_status_buffer = write_pointer % GEN8_CSB_ENTRIES; > - > - /* Update the read pointer to the old write pointer. Manual ringbuffer > - * management ftw </sarcasm> */ > - I915_WRITE_FW(RING_CONTEXT_STATUS_PTR(engine), > - _MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, > - engine->next_context_status_buffer << 8)); > - > - intel_uncore_forcewake_put(dev_priv, engine->fw_domains); > - > - spin_lock(&engine->execlist_lock); > - > - for (i = 0; i < csb_read; i++) { > - if (unlikely(csb[i][0] & GEN8_CTX_STATUS_PREEMPTED)) { > - if (csb[i][0] & GEN8_CTX_STATUS_LITE_RESTORE) { > - if (execlists_check_remove_request(engine, csb[i][1])) > - WARN(1, "Lite Restored request removed from queue\n"); > - } else > - WARN(1, "Preemption without Lite Restore\n"); > + if (!execlists_elsp_idle(engine)) { > + u32 *ring_mmio = > + dev_priv->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)); > + u32 *csb_mmio = > + dev_priv->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0)); > + unsigned ring, head, tail; > + > + ring = readl(ring_mmio); > + head = GEN8_CSB_READ_PTR(ring); > + tail = GEN8_CSB_WRITE_PTR(ring); > + if (tail < head) > + tail += GEN8_CSB_ENTRIES; > + > + while (head < tail) { > + unsigned idx = ++head % GEN8_CSB_ENTRIES; > + unsigned status = readl(&csb_mmio[2*idx]); > + > + if (status & GEN8_CTX_STATUS_COMPLETED_MASK) { > + GEM_BUG_ON(engine->execlist_port[0].count == 0); Would this be the safety valve if we go out of sync vrt submit vs head? > + if (--engine->execlist_port[0].count == 0) { > + GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED); > + execlists_context_status_change(engine->execlist_port[0].request, > + INTEL_CONTEXT_SCHEDULE_OUT); > + i915_gem_request_put(engine->execlist_port[0].request); > + engine->execlist_port[0] = engine->execlist_port[1]; Intention here is to always retire through port zero, and thus the refcounts matches the submitted ones? > + memset(&engine->execlist_port[1], 0, > + sizeof(engine->execlist_port[1])); > + engine->preempt_wa = false; > + } > + } > + GEM_BUG_ON(engine->execlist_port[0].count == 0 && > + !(status & GEN8_CTX_STATUS_ACTIVE_IDLE)); > } > > - if (csb[i][0] & (GEN8_CTX_STATUS_ACTIVE_IDLE | > - GEN8_CTX_STATUS_ELEMENT_SWITCH)) > - submit_contexts += > - execlists_check_remove_request(engine, csb[i][1]); > + writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, GEN8_CSB_WRITE_PTR(ring) << 8), > + ring_mmio); > } > > - if (submit_contexts) { > - if (!engine->disable_lite_restore_wa || > - (csb[i][0] & GEN8_CTX_STATUS_ACTIVE_IDLE)) > - execlists_unqueue(engine); > - } > + if (execlists_elsp_ready(engine)) > + execlists_context_unqueue(engine); > > - spin_unlock(&engine->execlist_lock); > - > - if (unlikely(submit_contexts > 2)) > - DRM_ERROR("More than two context complete events?\n"); > + intel_uncore_forcewake_put(dev_priv, engine->fw_domains); > } > > static void execlists_submit_request(struct drm_i915_gem_request *request) > { > struct intel_engine_cs *engine = request->engine; > - struct drm_i915_gem_request *cursor; > - int num_elements = 0; > > spin_lock_bh(&engine->execlist_lock); > > - list_for_each_entry(cursor, &engine->execlist_queue, execlist_link) > - if (++num_elements > 2) > - break; > - > - if (num_elements > 2) { > - struct drm_i915_gem_request *tail_req; > - > - tail_req = list_last_entry(&engine->execlist_queue, > - struct drm_i915_gem_request, > - execlist_link); > - > - if (request->ctx == tail_req->ctx) { > - WARN(tail_req->elsp_submitted != 0, > - "More than 2 already-submitted reqs queued\n"); > - list_del(&tail_req->execlist_link); > - i915_gem_request_put(tail_req); > - } > - } > - > - i915_gem_request_get(request); > list_add_tail(&request->execlist_link, &engine->execlist_queue); > - request->ctx_hw_id = request->ctx->hw_id; > - if (num_elements == 0) > - execlists_unqueue(engine); > + > + if (execlists_elsp_idle(engine)) > + tasklet_hi_schedule(&engine->irq_tasklet); > > spin_unlock_bh(&engine->execlist_lock); > } > @@ -731,6 +630,7 @@ intel_logical_ring_advance(struct drm_i915_gem_request *request) > intel_ring_emit(ring, MI_NOOP); > intel_ring_emit(ring, MI_NOOP); > intel_ring_advance(ring); > + request->wa_tail = request->ring->tail; > > /* We keep the previous context alive until we retire the following > * request. This ensures that any the context object is still pinned > @@ -743,23 +643,6 @@ intel_logical_ring_advance(struct drm_i915_gem_request *request) > return 0; > } > > -void intel_execlists_cancel_requests(struct intel_engine_cs *engine) > -{ > - struct drm_i915_gem_request *req, *tmp; > - LIST_HEAD(cancel_list); > - > - WARN_ON(!mutex_is_locked(&engine->i915->drm.struct_mutex)); > - > - spin_lock_bh(&engine->execlist_lock); > - list_replace_init(&engine->execlist_queue, &cancel_list); > - spin_unlock_bh(&engine->execlist_lock); > - > - list_for_each_entry_safe(req, tmp, &cancel_list, execlist_link) { > - list_del(&req->execlist_link); > - i915_gem_request_put(req); > - } > -} > - > static int intel_lr_context_pin(struct i915_gem_context *ctx, > struct intel_engine_cs *engine) > { > @@ -1285,7 +1168,6 @@ static void lrc_init_hws(struct intel_engine_cs *engine) > static int gen8_init_common_ring(struct intel_engine_cs *engine) > { > struct drm_i915_private *dev_priv = engine->i915; > - unsigned int next_context_status_buffer_hw; > > lrc_init_hws(engine); > > @@ -1296,32 +1178,12 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine) > I915_WRITE(RING_MODE_GEN7(engine), > _MASKED_BIT_DISABLE(GFX_REPLAY_MODE) | > _MASKED_BIT_ENABLE(GFX_RUN_LIST_ENABLE)); > - POSTING_READ(RING_MODE_GEN7(engine)); > > - /* > - * Instead of resetting the Context Status Buffer (CSB) read pointer to > - * zero, we need to read the write pointer from hardware and use its > - * value because "this register is power context save restored". > - * Effectively, these states have been observed: > - * > - * | Suspend-to-idle (freeze) | Suspend-to-RAM (mem) | > - * BDW | CSB regs not reset | CSB regs reset | > - * CHT | CSB regs not reset | CSB regs not reset | > - * SKL | ? | ? | > - * BXT | ? | ? | > - */ > - next_context_status_buffer_hw = > - GEN8_CSB_WRITE_PTR(I915_READ(RING_CONTEXT_STATUS_PTR(engine))); > - > - /* > - * When the CSB registers are reset (also after power-up / gpu reset), > - * CSB write pointer is set to all 1's, which is not valid, use '5' in > - * this special case, so the first element read is CSB[0]. > - */ > - if (next_context_status_buffer_hw == GEN8_CSB_PTR_MASK) > - next_context_status_buffer_hw = (GEN8_CSB_ENTRIES - 1); > + I915_WRITE(RING_CONTEXT_STATUS_PTR(engine), > + _MASKED_FIELD(GEN8_CSB_READ_PTR_MASK | > + GEN8_CSB_WRITE_PTR_MASK, > + 0)); > > - engine->next_context_status_buffer = next_context_status_buffer_hw; > DRM_DEBUG_DRIVER("Execlists enabled for %s\n", engine->name); > > intel_engine_init_hangcheck(engine); > @@ -1706,7 +1568,6 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine) > } > intel_lr_context_unpin(dev_priv->kernel_context, engine); > > - engine->idle_lite_restore_wa = 0; > engine->disable_lite_restore_wa = false; > engine->ctx_desc_template = 0; > > diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h > index a52cf57dbd40..4d70346500c2 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.h > +++ b/drivers/gpu/drm/i915/intel_lrc.h > @@ -97,6 +97,4 @@ int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv, > int enable_execlists); > void intel_execlists_enable_submission(struct drm_i915_private *dev_priv); > > -void intel_execlists_cancel_requests(struct intel_engine_cs *engine); > - > #endif /* _INTEL_LRC_H_ */ > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 43e545e44352..ed8a8482a7fb 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -288,11 +288,14 @@ struct intel_engine_cs { > /* Execlists */ > struct tasklet_struct irq_tasklet; > spinlock_t execlist_lock; /* used inside tasklet, use spin_lock_bh */ > + struct execlist_port { > + struct drm_i915_gem_request *request; > + unsigned int count; > + } execlist_port[2]; > struct list_head execlist_queue; > unsigned int fw_domains; > - unsigned int next_context_status_buffer; > - unsigned int idle_lite_restore_wa; > bool disable_lite_restore_wa; > + bool preempt_wa; This needs to be reset also in logical_ring_cleanup(). Overall this patch really makes the submission/retirement much more compact and simpler. The exception being the unqueuing part which is difficult due to the merging logic. Some bikescheds I made during review can be found in: https://cgit.freedesktop.org/~miku/drm-intel/commit/?h=review&id=3c806a9f6665db3a08f949224560bb039af1be1a -Mika > u32 ctx_desc_template; > > /** > -- > 2.8.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
