* [PATCH 2/9] drm/i915/execlists: Pull submit after dequeue under timeline lock
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
@ 2018-06-28 12:33 ` Chris Wilson
2018-06-28 12:33 ` [PATCH 3/9] drm/i915/execlists: Pull CSB reset under the timeline.lock Chris Wilson
` (13 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2018-06-28 12:33 UTC (permalink / raw)
To: intel-gfx
In the next patch, we will begin processing the CSB from inside the
submission path (underneath an irqsoff section, and even from inside
interrupt handlers). This means that updating the execlists->port[] will
no longer be serialised by the tasklet but needs to be locked by the
engine->timeline.lock instead. Pull dequeue and submit under the same
lock for protection. (An alternate future plan is to keep the in/out
arrays separate for concurrent processing and reduced lock coverage.)
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/intel_lrc.c | 32 ++++++++++++--------------------
1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2b21a6596360..009db92b67d7 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -567,7 +567,7 @@ static void complete_preempt_context(struct intel_engine_execlists *execlists)
execlists_clear_active(execlists, EXECLISTS_ACTIVE_PREEMPT);
}
-static bool __execlists_dequeue(struct intel_engine_cs *engine)
+static void __execlists_dequeue(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
struct execlist_port *port = execlists->port;
@@ -622,11 +622,11 @@ static bool __execlists_dequeue(struct intel_engine_cs *engine)
* the HW to indicate that it has had a chance to respond.
*/
if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
- return false;
+ return;
if (need_preempt(engine, last, execlists->queue_priority)) {
inject_preempt_context(engine);
- return false;
+ return;
}
/*
@@ -651,7 +651,7 @@ static bool __execlists_dequeue(struct intel_engine_cs *engine)
* priorities of the ports haven't been switch.
*/
if (port_count(&port[1]))
- return false;
+ return;
/*
* WaIdleLiteRestore:bdw,skl
@@ -751,8 +751,10 @@ static bool __execlists_dequeue(struct intel_engine_cs *engine)
port != execlists->port ? rq_prio(last) : INT_MIN;
execlists->first = rb;
- if (submit)
+ if (submit) {
port_assign(port, last);
+ execlists_submit_ports(engine);
+ }
/* We must always keep the beast fed if we have work piled up */
GEM_BUG_ON(execlists->first && !port_isset(execlists->port));
@@ -761,24 +763,19 @@ static bool __execlists_dequeue(struct intel_engine_cs *engine)
if (last)
execlists_user_begin(execlists, execlists->port);
- return submit;
+ /* If the engine is now idle, so should be the flag; and vice versa. */
+ GEM_BUG_ON(execlists_is_active(&engine->execlists,
+ EXECLISTS_ACTIVE_USER) ==
+ !port_isset(engine->execlists.port));
}
static void execlists_dequeue(struct intel_engine_cs *engine)
{
- struct intel_engine_execlists * const execlists = &engine->execlists;
unsigned long flags;
- bool submit;
spin_lock_irqsave(&engine->timeline.lock, flags);
- submit = __execlists_dequeue(engine);
+ __execlists_dequeue(engine);
spin_unlock_irqrestore(&engine->timeline.lock, flags);
-
- if (submit)
- execlists_submit_ports(engine);
-
- GEM_BUG_ON(port_isset(execlists->port) &&
- !execlists_is_active(execlists, EXECLISTS_ACTIVE_USER));
}
void
@@ -1161,11 +1158,6 @@ static void execlists_submission_tasklet(unsigned long data)
if (!execlists_is_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT))
execlists_dequeue(engine);
-
- /* If the engine is now idle, so should be the flag; and vice versa. */
- GEM_BUG_ON(execlists_is_active(&engine->execlists,
- EXECLISTS_ACTIVE_USER) ==
- !port_isset(engine->execlists.port));
}
static void queue_request(struct intel_engine_cs *engine,
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 3/9] drm/i915/execlists: Pull CSB reset under the timeline.lock
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
2018-06-28 12:33 ` [PATCH 2/9] drm/i915/execlists: Pull submit after dequeue under timeline lock Chris Wilson
@ 2018-06-28 12:33 ` Chris Wilson
2018-06-28 12:33 ` [PATCH 4/9] drm/i915/execlists: Process one CSB update at a time Chris Wilson
` (12 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2018-06-28 12:33 UTC (permalink / raw)
To: intel-gfx
In the following patch, we will process the CSB events under the
timeline.lock and not serialised by the tasklet. This also means that we
will need to protect access to common variables such as
execlists->csb_head with the timeline.lock during reset.
v2: Move sync_irq to avoid deadlocks between taking timeline.lock from
our interrupt handler.
v3: Kill off the synchronize_hardirq as it raises more questions than
answered; now we use the timeline.lock entirely for CSB serialisation
between the irq and elsewhere, we don't need to be so heavy handed with
flushing
v4: Treat request cancellation (wedging after failed reset) similarly
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/intel_lrc.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 009db92b67d7..4b31e8f42aeb 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -871,7 +871,6 @@ static void reset_irq(struct intel_engine_cs *engine)
{
/* Mark all CS interrupts as complete */
smp_store_mb(engine->execlists.active, 0);
- synchronize_hardirq(engine->i915->drm.irq);
clear_gtiir(engine);
@@ -908,14 +907,12 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
* submission's irq state, we also wish to remind ourselves that
* it is irq state.)
*/
- local_irq_save(flags);
+ spin_lock_irqsave(&engine->timeline.lock, flags);
/* Cancel the requests on the HW and clear the ELSP tracker. */
execlists_cancel_port_requests(execlists);
reset_irq(engine);
- spin_lock(&engine->timeline.lock);
-
/* Mark all executing requests as skipped. */
list_for_each_entry(rq, &engine->timeline.requests, link) {
GEM_BUG_ON(!rq->global_seqno);
@@ -949,9 +946,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
execlists->first = NULL;
GEM_BUG_ON(port_isset(execlists->port));
- spin_unlock(&engine->timeline.lock);
-
- local_irq_restore(flags);
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
}
static void process_csb(struct intel_engine_cs *engine)
@@ -1969,8 +1964,7 @@ static void execlists_reset(struct intel_engine_cs *engine,
engine->name, request ? request->global_seqno : 0,
intel_engine_get_seqno(engine));
- /* See execlists_cancel_requests() for the irq/spinlock split. */
- local_irq_save(flags);
+ spin_lock_irqsave(&engine->timeline.lock, flags);
/*
* Catch up with any missed context-switch interrupts.
@@ -1985,14 +1979,12 @@ static void execlists_reset(struct intel_engine_cs *engine,
reset_irq(engine);
/* Push back any incomplete requests for replay after the reset. */
- spin_lock(&engine->timeline.lock);
__unwind_incomplete_requests(engine);
- spin_unlock(&engine->timeline.lock);
/* Following the reset, we need to reload the CSB read/write pointers */
engine->execlists.csb_head = GEN8_CSB_ENTRIES - 1;
- local_irq_restore(flags);
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
/*
* If the request was innocent, we leave the request in the ELSP
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 4/9] drm/i915/execlists: Process one CSB update at a time
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
2018-06-28 12:33 ` [PATCH 2/9] drm/i915/execlists: Pull submit after dequeue under timeline lock Chris Wilson
2018-06-28 12:33 ` [PATCH 3/9] drm/i915/execlists: Pull CSB reset under the timeline.lock Chris Wilson
@ 2018-06-28 12:33 ` Chris Wilson
2018-06-28 12:33 ` [PATCH 5/9] drm/i915/execlists: Unify CSB access pointers Chris Wilson
` (11 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2018-06-28 12:33 UTC (permalink / raw)
To: intel-gfx
In the next patch, we will process the CSB events directly from the
submission path, rather than only after a CS interrupt. Hence, we will
no longer have the need for a loop until the has-interrupt bit is clear,
and in the meantime can remove that small optimisation.
v2: Tvrtko pointed out it was safer to unconditionally kick the tasklet
after each irq, when assuming that the tasklet is called for each irq.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_irq.c | 7 +-
drivers/gpu/drm/i915/intel_lrc.c | 278 +++++++++++++++----------------
2 files changed, 141 insertions(+), 144 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e83fcedcbf1d..97418efec719 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1482,9 +1482,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
bool tasklet = false;
if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
- if (READ_ONCE(engine->execlists.active))
- tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
- &engine->irq_posted);
+ if (READ_ONCE(engine->execlists.active)) {
+ set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
+ tasklet = true;
+ }
}
if (iir & GT_RENDER_USER_INTERRUPT) {
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 4b31e8f42aeb..91656eb2f2db 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -954,166 +954,162 @@ static void process_csb(struct intel_engine_cs *engine)
struct intel_engine_execlists * const execlists = &engine->execlists;
struct execlist_port *port = execlists->port;
struct drm_i915_private *i915 = engine->i915;
+
+ /* The HWSP contains a (cacheable) mirror of the CSB */
+ const u32 *buf =
+ &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
+ unsigned int head, tail;
bool fw = false;
- do {
- /* The HWSP contains a (cacheable) mirror of the CSB */
- const u32 *buf =
- &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
- unsigned int head, tail;
-
- /* Clear before reading to catch new interrupts */
- clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
- smp_mb__after_atomic();
-
- if (unlikely(execlists->csb_use_mmio)) {
- if (!fw) {
- intel_uncore_forcewake_get(i915, execlists->fw_domains);
- fw = true;
- }
+ /* Clear before reading to catch new interrupts */
+ clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
+ smp_mb__after_atomic();
- buf = (u32 * __force)
- (i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0)));
+ if (unlikely(execlists->csb_use_mmio)) {
+ intel_uncore_forcewake_get(i915, execlists->fw_domains);
+ fw = true;
- head = readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
- tail = GEN8_CSB_WRITE_PTR(head);
- head = GEN8_CSB_READ_PTR(head);
- execlists->csb_head = head;
- } else {
- const int write_idx =
- intel_hws_csb_write_index(i915) -
- I915_HWS_CSB_BUF0_INDEX;
+ buf = (u32 * __force)
+ (i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0)));
- head = execlists->csb_head;
- tail = READ_ONCE(buf[write_idx]);
- rmb(); /* Hopefully paired with a wmb() in HW */
- }
- GEM_TRACE("%s cs-irq head=%d [%d%s], tail=%d [%d%s]\n",
- engine->name,
- head, GEN8_CSB_READ_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?",
- tail, GEN8_CSB_WRITE_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?");
+ head = readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
+ tail = GEN8_CSB_WRITE_PTR(head);
+ head = GEN8_CSB_READ_PTR(head);
+ execlists->csb_head = head;
+ } else {
+ const int write_idx =
+ intel_hws_csb_write_index(i915) -
+ I915_HWS_CSB_BUF0_INDEX;
- while (head != tail) {
- struct i915_request *rq;
- unsigned int status;
- unsigned int count;
+ head = execlists->csb_head;
+ tail = READ_ONCE(buf[write_idx]);
+ rmb(); /* Hopefully paired with a wmb() in HW */
+ }
+ GEM_TRACE("%s cs-irq head=%d [%d%s], tail=%d [%d%s]\n",
+ engine->name,
+ head, GEN8_CSB_READ_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?",
+ tail, GEN8_CSB_WRITE_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?");
- if (++head == GEN8_CSB_ENTRIES)
- head = 0;
+ while (head != tail) {
+ struct i915_request *rq;
+ unsigned int status;
+ unsigned int count;
- /*
- * We are flying near dragons again.
- *
- * We hold a reference to the request in execlist_port[]
- * but no more than that. We are operating in softirq
- * context and so cannot hold any mutex or sleep. That
- * prevents us stopping the requests we are processing
- * in port[] from being retired simultaneously (the
- * breadcrumb will be complete before we see the
- * context-switch). As we only hold the reference to the
- * request, any pointer chasing underneath the request
- * is subject to a potential use-after-free. Thus we
- * store all of the bookkeeping within port[] as
- * required, and avoid using unguarded pointers beneath
- * request itself. The same applies to the atomic
- * status notifier.
- */
+ if (++head == GEN8_CSB_ENTRIES)
+ head = 0;
- status = READ_ONCE(buf[2 * head]); /* maybe mmio! */
- GEM_TRACE("%s csb[%d]: status=0x%08x:0x%08x, active=0x%x\n",
- engine->name, head,
- status, buf[2*head + 1],
- execlists->active);
-
- if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
- GEN8_CTX_STATUS_PREEMPTED))
- execlists_set_active(execlists,
- EXECLISTS_ACTIVE_HWACK);
- if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
- execlists_clear_active(execlists,
- EXECLISTS_ACTIVE_HWACK);
-
- if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
- continue;
+ /*
+ * We are flying near dragons again.
+ *
+ * We hold a reference to the request in execlist_port[]
+ * but no more than that. We are operating in softirq
+ * context and so cannot hold any mutex or sleep. That
+ * prevents us stopping the requests we are processing
+ * in port[] from being retired simultaneously (the
+ * breadcrumb will be complete before we see the
+ * context-switch). As we only hold the reference to the
+ * request, any pointer chasing underneath the request
+ * is subject to a potential use-after-free. Thus we
+ * store all of the bookkeeping within port[] as
+ * required, and avoid using unguarded pointers beneath
+ * request itself. The same applies to the atomic
+ * status notifier.
+ */
- /* We should never get a COMPLETED | IDLE_ACTIVE! */
- GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
+ status = READ_ONCE(buf[2 * head]); /* maybe mmio! */
+ GEM_TRACE("%s csb[%d]: status=0x%08x:0x%08x, active=0x%x\n",
+ engine->name, head,
+ status, buf[2*head + 1],
+ execlists->active);
+
+ if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
+ GEN8_CTX_STATUS_PREEMPTED))
+ execlists_set_active(execlists,
+ EXECLISTS_ACTIVE_HWACK);
+ if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
+ execlists_clear_active(execlists,
+ EXECLISTS_ACTIVE_HWACK);
+
+ if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
+ continue;
- if (status & GEN8_CTX_STATUS_COMPLETE &&
- buf[2*head + 1] == execlists->preempt_complete_status) {
- GEM_TRACE("%s preempt-idle\n", engine->name);
- complete_preempt_context(execlists);
- continue;
- }
+ /* We should never get a COMPLETED | IDLE_ACTIVE! */
+ GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
- if (status & GEN8_CTX_STATUS_PREEMPTED &&
- execlists_is_active(execlists,
- EXECLISTS_ACTIVE_PREEMPT))
- continue;
+ if (status & GEN8_CTX_STATUS_COMPLETE &&
+ buf[2*head + 1] == execlists->preempt_complete_status) {
+ GEM_TRACE("%s preempt-idle\n", engine->name);
+ complete_preempt_context(execlists);
+ continue;
+ }
- GEM_BUG_ON(!execlists_is_active(execlists,
- EXECLISTS_ACTIVE_USER));
+ if (status & GEN8_CTX_STATUS_PREEMPTED &&
+ execlists_is_active(execlists,
+ EXECLISTS_ACTIVE_PREEMPT))
+ continue;
- rq = port_unpack(port, &count);
- GEM_TRACE("%s out[0]: ctx=%d.%d, global=%d (fence %llx:%d) (current %d), prio=%d\n",
- engine->name,
- port->context_id, count,
- rq ? rq->global_seqno : 0,
- rq ? rq->fence.context : 0,
- rq ? rq->fence.seqno : 0,
- intel_engine_get_seqno(engine),
- rq ? rq_prio(rq) : 0);
+ GEM_BUG_ON(!execlists_is_active(execlists,
+ EXECLISTS_ACTIVE_USER));
- /* Check the context/desc id for this event matches */
- GEM_DEBUG_BUG_ON(buf[2 * head + 1] != port->context_id);
+ rq = port_unpack(port, &count);
+ GEM_TRACE("%s out[0]: ctx=%d.%d, global=%d (fence %llx:%d) (current %d), prio=%d\n",
+ engine->name,
+ port->context_id, count,
+ rq ? rq->global_seqno : 0,
+ rq ? rq->fence.context : 0,
+ rq ? rq->fence.seqno : 0,
+ intel_engine_get_seqno(engine),
+ rq ? rq_prio(rq) : 0);
+
+ /* Check the context/desc id for this event matches */
+ GEM_DEBUG_BUG_ON(buf[2 * head + 1] != port->context_id);
+
+ GEM_BUG_ON(count == 0);
+ if (--count == 0) {
+ /*
+ * On the final event corresponding to the
+ * submission of this context, we expect either
+ * an element-switch event or a completion
+ * event (and on completion, the active-idle
+ * marker). No more preemptions, lite-restore
+ * or otherwise.
+ */
+ GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);
+ GEM_BUG_ON(port_isset(&port[1]) &&
+ !(status & GEN8_CTX_STATUS_ELEMENT_SWITCH));
+ GEM_BUG_ON(!port_isset(&port[1]) &&
+ !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
- GEM_BUG_ON(count == 0);
- if (--count == 0) {
- /*
- * On the final event corresponding to the
- * submission of this context, we expect either
- * an element-switch event or a completion
- * event (and on completion, the active-idle
- * marker). No more preemptions, lite-restore
- * or otherwise.
- */
- GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);
- GEM_BUG_ON(port_isset(&port[1]) &&
- !(status & GEN8_CTX_STATUS_ELEMENT_SWITCH));
- GEM_BUG_ON(!port_isset(&port[1]) &&
- !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
+ /*
+ * We rely on the hardware being strongly
+ * ordered, that the breadcrumb write is
+ * coherent (visible from the CPU) before the
+ * user interrupt and CSB is processed.
+ */
+ GEM_BUG_ON(!i915_request_completed(rq));
- /*
- * We rely on the hardware being strongly
- * ordered, that the breadcrumb write is
- * coherent (visible from the CPU) before the
- * user interrupt and CSB is processed.
- */
- GEM_BUG_ON(!i915_request_completed(rq));
-
- execlists_context_schedule_out(rq,
- INTEL_CONTEXT_SCHEDULE_OUT);
- i915_request_put(rq);
-
- GEM_TRACE("%s completed ctx=%d\n",
- engine->name, port->context_id);
-
- port = execlists_port_complete(execlists, port);
- if (port_isset(port))
- execlists_user_begin(execlists, port);
- else
- execlists_user_end(execlists);
- } else {
- port_set(port, port_pack(rq, count));
- }
- }
+ execlists_context_schedule_out(rq,
+ INTEL_CONTEXT_SCHEDULE_OUT);
+ i915_request_put(rq);
- if (head != execlists->csb_head) {
- execlists->csb_head = head;
- writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
- i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
+ GEM_TRACE("%s completed ctx=%d\n",
+ engine->name, port->context_id);
+
+ port = execlists_port_complete(execlists, port);
+ if (port_isset(port))
+ execlists_user_begin(execlists, port);
+ else
+ execlists_user_end(execlists);
+ } else {
+ port_set(port, port_pack(rq, count));
}
- } while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted));
+ }
+
+ if (head != execlists->csb_head) {
+ execlists->csb_head = head;
+ writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
+ i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
+ }
if (unlikely(fw))
intel_uncore_forcewake_put(i915, execlists->fw_domains);
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 5/9] drm/i915/execlists: Unify CSB access pointers
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
` (2 preceding siblings ...)
2018-06-28 12:33 ` [PATCH 4/9] drm/i915/execlists: Process one CSB update at a time Chris Wilson
@ 2018-06-28 12:33 ` Chris Wilson
2018-06-28 12:33 ` [PATCH 6/9] drm/i915/execlists: Reset CSB write pointer after reset Chris Wilson
` (10 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2018-06-28 12:33 UTC (permalink / raw)
To: intel-gfx
Following the removal of the last workarounds, the only CSB mmio access
is for the old vGPU interface. The mmio registers presented by vGPU do
not require forcewake and can be treated as ordinary volatile memory,
i.e. they behave just like the HWSP access just at a different location.
We can reduce the CSB access to a set of read/write/buffer pointers and
treat the various paths identically and not worry about forcewake.
(Forcewake is nightmare for worstcase latency, and we want to process
this all with irqsoff -- no latency allowed!)
v2: Comments, comments, comments. Well, 2 bonus comments.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/intel_engine_cs.c | 12 ---
drivers/gpu/drm/i915/intel_lrc.c | 133 ++++++++++++------------
drivers/gpu/drm/i915/intel_ringbuffer.h | 23 ++--
3 files changed, 82 insertions(+), 86 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index d3264bd6e9dc..7209c22798e6 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -25,7 +25,6 @@
#include <drm/drm_print.h>
#include "i915_drv.h"
-#include "i915_vgpu.h"
#include "intel_ringbuffer.h"
#include "intel_lrc.h"
@@ -456,21 +455,10 @@ static void intel_engine_init_batch_pool(struct intel_engine_cs *engine)
i915_gem_batch_pool_init(&engine->batch_pool, engine);
}
-static bool csb_force_mmio(struct drm_i915_private *i915)
-{
- /* Older GVT emulation depends upon intercepting CSB mmio */
- if (intel_vgpu_active(i915) && !intel_vgpu_has_hwsp_emulation(i915))
- return true;
-
- return false;
-}
-
static void intel_engine_init_execlist(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
- execlists->csb_use_mmio = csb_force_mmio(engine->i915);
-
execlists->port_mask = 1;
BUILD_BUG_ON_NOT_POWER_OF_2(execlists_num_ports(execlists));
GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 91656eb2f2db..8531a5b6f6ff 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -137,6 +137,7 @@
#include <drm/i915_drm.h>
#include "i915_drv.h"
#include "i915_gem_render_state.h"
+#include "i915_vgpu.h"
#include "intel_lrc_reg.h"
#include "intel_mocs.h"
#include "intel_workarounds.h"
@@ -953,44 +954,40 @@ static void process_csb(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
struct execlist_port *port = execlists->port;
- struct drm_i915_private *i915 = engine->i915;
-
- /* The HWSP contains a (cacheable) mirror of the CSB */
- const u32 *buf =
- &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
- unsigned int head, tail;
- bool fw = false;
+ const u32 * const buf = execlists->csb_status;
+ u8 head, tail;
/* Clear before reading to catch new interrupts */
clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
smp_mb__after_atomic();
- if (unlikely(execlists->csb_use_mmio)) {
- intel_uncore_forcewake_get(i915, execlists->fw_domains);
- fw = true;
-
- buf = (u32 * __force)
- (i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0)));
-
- head = readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
- tail = GEN8_CSB_WRITE_PTR(head);
- head = GEN8_CSB_READ_PTR(head);
- execlists->csb_head = head;
- } else {
- const int write_idx =
- intel_hws_csb_write_index(i915) -
- I915_HWS_CSB_BUF0_INDEX;
+ /*
+ * Note that csb_write, csb_status may be either in HWSP or mmio.
+ * When reading from the csb_write mmio register, we have to be
+ * careful to only use the GEN8_CSB_WRITE_PTR portion, which is
+ * the low 4bits. As it happens we know the next 4bits are always
+ * zero and so we can simply masked off the low u8 of the register
+ * and treat it identically to reading from the HWSP (without having
+ * to use explicit shifting and masking, and probably bifurcating
+ * the code to handle the legacy mmio read).
+ */
+ head = execlists->csb_head;
+ tail = READ_ONCE(*execlists->csb_write);
+ GEM_TRACE("%s cs-irq head=%d, tail=%d\n", engine->name, head, tail);
+ if (unlikely(head == tail))
+ return;
- head = execlists->csb_head;
- tail = READ_ONCE(buf[write_idx]);
- rmb(); /* Hopefully paired with a wmb() in HW */
- }
- GEM_TRACE("%s cs-irq head=%d [%d%s], tail=%d [%d%s]\n",
- engine->name,
- head, GEN8_CSB_READ_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?",
- tail, GEN8_CSB_WRITE_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?");
+ /*
+ * Hopefully paired with a wmb() in HW!
+ *
+ * We must complete the read of the write pointer before any reads
+ * from the CSB, so that we do not see stale values. Without an rmb
+ * (lfence) the HW may speculatively perform the CSB[] reads *before*
+ * we perform the READ_ONCE(*csb_write).
+ */
+ rmb();
- while (head != tail) {
+ do {
struct i915_request *rq;
unsigned int status;
unsigned int count;
@@ -1016,12 +1013,12 @@ static void process_csb(struct intel_engine_cs *engine)
* status notifier.
*/
- status = READ_ONCE(buf[2 * head]); /* maybe mmio! */
GEM_TRACE("%s csb[%d]: status=0x%08x:0x%08x, active=0x%x\n",
engine->name, head,
- status, buf[2*head + 1],
+ buf[2 * head + 0], buf[2 * head + 1],
execlists->active);
+ status = buf[2 * head];
if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
GEN8_CTX_STATUS_PREEMPTED))
execlists_set_active(execlists,
@@ -1103,16 +1100,11 @@ static void process_csb(struct intel_engine_cs *engine)
} else {
port_set(port, port_pack(rq, count));
}
- }
-
- if (head != execlists->csb_head) {
- execlists->csb_head = head;
- writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
- i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
- }
+ } while (head != tail);
- if (unlikely(fw))
- intel_uncore_forcewake_put(i915, execlists->fw_domains);
+ writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
+ execlists->csb_read);
+ execlists->csb_head = head;
}
/*
@@ -2430,28 +2422,11 @@ logical_ring_default_irqs(struct intel_engine_cs *engine)
static void
logical_ring_setup(struct intel_engine_cs *engine)
{
- struct drm_i915_private *dev_priv = engine->i915;
- enum forcewake_domains fw_domains;
-
intel_engine_setup_common(engine);
/* Intentionally left blank. */
engine->buffer = NULL;
- fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
- RING_ELSP(engine),
- FW_REG_WRITE);
-
- fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
- RING_CONTEXT_STATUS_PTR(engine),
- FW_REG_READ | FW_REG_WRITE);
-
- fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
- RING_CONTEXT_STATUS_BUF_BASE(engine),
- FW_REG_READ);
-
- engine->execlists.fw_domains = fw_domains;
-
tasklet_init(&engine->execlists.tasklet,
execlists_submission_tasklet, (unsigned long)engine);
@@ -2459,34 +2434,56 @@ logical_ring_setup(struct intel_engine_cs *engine)
logical_ring_default_irqs(engine);
}
+static bool csb_force_mmio(struct drm_i915_private *i915)
+{
+ /* Older GVT emulation depends upon intercepting CSB mmio */
+ return intel_vgpu_active(i915) && !intel_vgpu_has_hwsp_emulation(i915);
+}
+
static int logical_ring_init(struct intel_engine_cs *engine)
{
+ struct drm_i915_private *i915 = engine->i915;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
int ret;
ret = intel_engine_init_common(engine);
if (ret)
goto error;
- if (HAS_LOGICAL_RING_ELSQ(engine->i915)) {
- engine->execlists.submit_reg = engine->i915->regs +
+ if (HAS_LOGICAL_RING_ELSQ(i915)) {
+ execlists->submit_reg = i915->regs +
i915_mmio_reg_offset(RING_EXECLIST_SQ_CONTENTS(engine));
- engine->execlists.ctrl_reg = engine->i915->regs +
+ execlists->ctrl_reg = i915->regs +
i915_mmio_reg_offset(RING_EXECLIST_CONTROL(engine));
} else {
- engine->execlists.submit_reg = engine->i915->regs +
+ execlists->submit_reg = i915->regs +
i915_mmio_reg_offset(RING_ELSP(engine));
}
- engine->execlists.preempt_complete_status = ~0u;
- if (engine->i915->preempt_context) {
+ execlists->preempt_complete_status = ~0u;
+ if (i915->preempt_context) {
struct intel_context *ce =
- to_intel_context(engine->i915->preempt_context, engine);
+ to_intel_context(i915->preempt_context, engine);
- engine->execlists.preempt_complete_status =
+ execlists->preempt_complete_status =
upper_32_bits(ce->lrc_desc);
}
- engine->execlists.csb_head = GEN8_CSB_ENTRIES - 1;
+ execlists->csb_head = GEN8_CSB_ENTRIES - 1;
+ execlists->csb_read =
+ i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine));
+ if (csb_force_mmio(i915)) {
+ execlists->csb_status = (u32 __force *)
+ (i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0)));
+
+ execlists->csb_write = (u32 __force *)execlists->csb_read;
+ } else {
+ execlists->csb_status =
+ &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
+
+ execlists->csb_write =
+ &engine->status_page.page_addr[intel_hws_csb_write_index(i915)];
+ }
return 0;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index a0bc7a8222b4..80fde36561fb 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -300,24 +300,35 @@ struct intel_engine_execlists {
struct rb_node *first;
/**
- * @fw_domains: forcewake domains for irq tasklet
+ * @csb_read: control register for Context Switch buffer
+ *
+ * Note this register is always in mmio.
*/
- unsigned int fw_domains;
+ u32 __iomem *csb_read;
/**
- * @csb_head: context status buffer head
+ * @csb_write: control register for Context Switch buffer
+ *
+ * Note this register may be either mmio or HWSP shadow.
*/
- unsigned int csb_head;
+ u32 *csb_write;
/**
- * @csb_use_mmio: access csb through mmio, instead of hwsp
+ * @csb_status: status array for Context Switch buffer
+ *
+ * Note these register may be either mmio or HWSP shadow.
*/
- bool csb_use_mmio;
+ u32 *csb_status;
/**
* @preempt_complete_status: expected CSB upon completing preemption
*/
u32 preempt_complete_status;
+
+ /**
+ * @csb_head: context status buffer head
+ */
+ u8 csb_head;
};
#define INTEL_ENGINE_CS_MAX_NAME 8
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 6/9] drm/i915/execlists: Reset CSB write pointer after reset
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
` (3 preceding siblings ...)
2018-06-28 12:33 ` [PATCH 5/9] drm/i915/execlists: Unify CSB access pointers Chris Wilson
@ 2018-06-28 12:33 ` Chris Wilson
2018-06-28 12:33 ` [PATCH 7/9] drm/i915/execlists: Stop storing the CSB read pointer in the mmio register Chris Wilson
` (9 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2018-06-28 12:33 UTC (permalink / raw)
To: intel-gfx
On HW reset, the HW clears the write pointer (to 0). But since it also
writes its first CSB entry to slot 0, we need to reset the write pointer
back to the element before (so the first entry we read is 0).
This is required for the next patch, where we trust the CSB completely!
v2: Use _MASKED_FIELD
v3: Store the reset value, so that we differentiate between mmio/hwsp
transparently and without pretense.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/intel_lrc.c | 23 +++++++++++++++++++++--
drivers/gpu/drm/i915/intel_ringbuffer.h | 9 +++++++++
2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 8531a5b6f6ff..f597a12f705b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -884,6 +884,21 @@ static void reset_irq(struct intel_engine_cs *engine)
clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
}
+static void reset_csb_pointers(struct intel_engine_execlists *execlists)
+{
+ /*
+ * After a reset, the HW starts writing into CSB entry [0]. We
+ * therefore have to set our HEAD pointer back one entry so that
+ * the *first* entry we check is entry 0. To complicate this further,
+ * as we don't wait for the first interrupt after reset, we have to
+ * fake the HW write to point back to the last entry so that our
+ * inline comparison of our cached head position against the last HW
+ * write works even before the first interrupt.
+ */
+ execlists->csb_head = execlists->csb_write_reset;
+ WRITE_ONCE(*execlists->csb_write, execlists->csb_write_reset);
+}
+
static void execlists_cancel_requests(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
@@ -1970,7 +1985,7 @@ static void execlists_reset(struct intel_engine_cs *engine,
__unwind_incomplete_requests(engine);
/* Following the reset, we need to reload the CSB read/write pointers */
- engine->execlists.csb_head = GEN8_CSB_ENTRIES - 1;
+ reset_csb_pointers(&engine->execlists);
spin_unlock_irqrestore(&engine->timeline.lock, flags);
@@ -2469,7 +2484,6 @@ static int logical_ring_init(struct intel_engine_cs *engine)
upper_32_bits(ce->lrc_desc);
}
- execlists->csb_head = GEN8_CSB_ENTRIES - 1;
execlists->csb_read =
i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine));
if (csb_force_mmio(i915)) {
@@ -2477,13 +2491,18 @@ static int logical_ring_init(struct intel_engine_cs *engine)
(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0)));
execlists->csb_write = (u32 __force *)execlists->csb_read;
+ execlists->csb_write_reset =
+ _MASKED_FIELD(GEN8_CSB_WRITE_PTR_MASK,
+ GEN8_CSB_ENTRIES - 1);
} else {
execlists->csb_status =
&engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
execlists->csb_write =
&engine->status_page.page_addr[intel_hws_csb_write_index(i915)];
+ execlists->csb_write_reset = GEN8_CSB_ENTRIES - 1;
}
+ reset_csb_pointers(execlists);
return 0;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 80fde36561fb..62e3db6f6f8d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -325,6 +325,15 @@ struct intel_engine_execlists {
*/
u32 preempt_complete_status;
+ /**
+ * @csb_write_reset: reset value for CSB write pointer
+ *
+ * As the CSB write pointer maybe either in HWSP or as a field
+ * inside an mmio register, we want to reprogram it slightly
+ * differently to avoid later confusion.
+ */
+ u32 csb_write_reset;
+
/**
* @csb_head: context status buffer head
*/
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 7/9] drm/i915/execlists: Stop storing the CSB read pointer in the mmio register
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
` (4 preceding siblings ...)
2018-06-28 12:33 ` [PATCH 6/9] drm/i915/execlists: Reset CSB write pointer after reset Chris Wilson
@ 2018-06-28 12:33 ` Chris Wilson
2018-06-28 12:33 ` [PATCH 8/9] drm/i915/execlists: Trust the CSB Chris Wilson
` (8 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2018-06-28 12:33 UTC (permalink / raw)
To: intel-gfx
As we now never read back our current head position from the CSB
pointers register, and the HW itself doesn't use it to prevent
overwriting unread CSB entries, we do not need to keep updating the
register. As it turns out this register is not listed as being shadowed,
and so requires forcewake -- but we haven't been taking forcewake around
it so the writes has probably been regularly dropped. Fortuitously, we
only read the value after a reset where it did not matter, and zero was
the right answer (well, close enough).
Mika pointed out that this was how we used to do it (accidentally!)
before he fixed it in commit cc53699b25b5 ("drm/i915: Use masked write
for Context Status Buffer Pointer").
References: cc53699b25b5 ("drm/i915: Use masked write for Context Status Buffer Pointer")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/intel_lrc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index f597a12f705b..14be53035610 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1117,8 +1117,6 @@ static void process_csb(struct intel_engine_cs *engine)
}
} while (head != tail);
- writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
- execlists->csb_read);
execlists->csb_head = head;
}
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 8/9] drm/i915/execlists: Trust the CSB
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
` (5 preceding siblings ...)
2018-06-28 12:33 ` [PATCH 7/9] drm/i915/execlists: Stop storing the CSB read pointer in the mmio register Chris Wilson
@ 2018-06-28 12:33 ` Chris Wilson
2018-06-28 13:04 ` Tvrtko Ursulin
2018-06-28 12:33 ` [PATCH 9/9] drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd) Chris Wilson
` (7 subsequent siblings)
14 siblings, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2018-06-28 12:33 UTC (permalink / raw)
To: intel-gfx
Now that we use the CSB stored in the CPU friendly HWSP, we do not need
to track interrupts for when the mmio CSB registers are valid and can
just check where we read up to last from the cached HWSP. This means we
can forgo the atomic bit tracking from interrupt, and in the next patch
it means we can check the CSB at any time.
v2: Change the splitting inside reset_prepare, we only want to lose
testing the interrupt in this patch, the next patch requires the change
in locking
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_irq.c | 11 +++-------
drivers/gpu/drm/i915/intel_engine_cs.c | 8 ++-----
drivers/gpu/drm/i915/intel_lrc.c | 29 ++++---------------------
drivers/gpu/drm/i915/intel_ringbuffer.h | 1 -
4 files changed, 9 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 97418efec719..cb91b213aa67 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1478,15 +1478,10 @@ static void snb_gt_irq_handler(struct drm_i915_private *dev_priv,
static void
gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
{
- struct intel_engine_execlists * const execlists = &engine->execlists;
bool tasklet = false;
- if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
- if (READ_ONCE(engine->execlists.active)) {
- set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
- tasklet = true;
- }
- }
+ if (iir & GT_CONTEXT_SWITCH_INTERRUPT)
+ tasklet = true;
if (iir & GT_RENDER_USER_INTERRUPT) {
notify_ring(engine);
@@ -1494,7 +1489,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
}
if (tasklet)
- tasklet_hi_schedule(&execlists->tasklet);
+ tasklet_hi_schedule(&engine->execlists.tasklet);
}
static void gen8_gt_irq_ack(struct drm_i915_private *i915,
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 7209c22798e6..ace93958689e 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1353,12 +1353,10 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine,
ptr = I915_READ(RING_CONTEXT_STATUS_PTR(engine));
read = GEN8_CSB_READ_PTR(ptr);
write = GEN8_CSB_WRITE_PTR(ptr);
- drm_printf(m, "\tExeclist CSB read %d [%d cached], write %d [%d from hws], interrupt posted? %s, tasklet queued? %s (%s)\n",
+ drm_printf(m, "\tExeclist CSB read %d [%d cached], write %d [%d from hws], tasklet queued? %s (%s)\n",
read, execlists->csb_head,
write,
intel_read_status_page(engine, intel_hws_csb_write_index(engine->i915)),
- yesno(test_bit(ENGINE_IRQ_EXECLIST,
- &engine->irq_posted)),
yesno(test_bit(TASKLET_STATE_SCHED,
&engine->execlists.tasklet.state)),
enableddisabled(!atomic_read(&engine->execlists.tasklet.count)));
@@ -1570,11 +1568,9 @@ void intel_engine_dump(struct intel_engine_cs *engine,
spin_unlock(&b->rb_lock);
local_irq_restore(flags);
- drm_printf(m, "IRQ? 0x%lx (breadcrumbs? %s) (execlists? %s)\n",
+ drm_printf(m, "IRQ? 0x%lx (breadcrumbs? %s)\n",
engine->irq_posted,
yesno(test_bit(ENGINE_IRQ_BREADCRUMB,
- &engine->irq_posted)),
- yesno(test_bit(ENGINE_IRQ_EXECLIST,
&engine->irq_posted)));
drm_printf(m, "HWSP:\n");
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 14be53035610..7f8b29684d9d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -874,14 +874,6 @@ static void reset_irq(struct intel_engine_cs *engine)
smp_store_mb(engine->execlists.active, 0);
clear_gtiir(engine);
-
- /*
- * The port is checked prior to scheduling a tasklet, but
- * just in case we have suspended the tasklet to do the
- * wedging make sure that when it wakes, it decides there
- * is no work to do by clearing the irq_posted bit.
- */
- clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
}
static void reset_csb_pointers(struct intel_engine_execlists *execlists)
@@ -972,10 +964,6 @@ static void process_csb(struct intel_engine_cs *engine)
const u32 * const buf = execlists->csb_status;
u8 head, tail;
- /* Clear before reading to catch new interrupts */
- clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
- smp_mb__after_atomic();
-
/*
* Note that csb_write, csb_status may be either in HWSP or mmio.
* When reading from the csb_write mmio register, we have to be
@@ -1128,11 +1116,10 @@ static void execlists_submission_tasklet(unsigned long data)
{
struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
- GEM_TRACE("%s awake?=%d, active=%x, irq-posted?=%d\n",
+ GEM_TRACE("%s awake?=%d, active=%x\n",
engine->name,
engine->i915->gt.awake,
- engine->execlists.active,
- test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted));
+ engine->execlists.active);
/*
* We can skip acquiring intel_runtime_pm_get() here as it was taken
@@ -1144,14 +1131,7 @@ static void execlists_submission_tasklet(unsigned long data)
*/
GEM_BUG_ON(!engine->i915->gt.awake);
- /*
- * Prefer doing test_and_clear_bit() as a two stage operation to avoid
- * imposing the cost of a locked atomic transaction when submitting a
- * new request (outside of the context-switch interrupt).
- */
- if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
- process_csb(engine);
-
+ process_csb(engine);
if (!execlists_is_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT))
execlists_dequeue(engine);
}
@@ -1919,8 +1899,7 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
* and avoid blaming an innocent request if the stall was due to the
* preemption itself.
*/
- if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
- process_csb(engine);
+ process_csb(engine);
/*
* The last active request can then be no later than the last request
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 62e3db6f6f8d..e1ee1ca9ac16 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -368,7 +368,6 @@ struct intel_engine_cs {
atomic_t irq_count;
unsigned long irq_posted;
#define ENGINE_IRQ_BREADCRUMB 0
-#define ENGINE_IRQ_EXECLIST 1
/* Rather than have every client wait upon all user interrupts,
* with the herd waking after every interrupt and each doing the
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 8/9] drm/i915/execlists: Trust the CSB
2018-06-28 12:33 ` [PATCH 8/9] drm/i915/execlists: Trust the CSB Chris Wilson
@ 2018-06-28 13:04 ` Tvrtko Ursulin
0 siblings, 0 replies; 24+ messages in thread
From: Tvrtko Ursulin @ 2018-06-28 13:04 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 28/06/2018 13:33, Chris Wilson wrote:
> Now that we use the CSB stored in the CPU friendly HWSP, we do not need
> to track interrupts for when the mmio CSB registers are valid and can
> just check where we read up to last from the cached HWSP. This means we
> can forgo the atomic bit tracking from interrupt, and in the next patch
> it means we can check the CSB at any time.
>
> v2: Change the splitting inside reset_prepare, we only want to lose
> testing the interrupt in this patch, the next patch requires the change
> in locking
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 11 +++-------
> drivers/gpu/drm/i915/intel_engine_cs.c | 8 ++-----
> drivers/gpu/drm/i915/intel_lrc.c | 29 ++++---------------------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 -
> 4 files changed, 9 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 97418efec719..cb91b213aa67 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1478,15 +1478,10 @@ static void snb_gt_irq_handler(struct drm_i915_private *dev_priv,
> static void
> gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
> {
> - struct intel_engine_execlists * const execlists = &engine->execlists;
> bool tasklet = false;
>
> - if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
> - if (READ_ONCE(engine->execlists.active)) {
> - set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> - tasklet = true;
> - }
> - }
> + if (iir & GT_CONTEXT_SWITCH_INTERRUPT)
> + tasklet = true;
>
> if (iir & GT_RENDER_USER_INTERRUPT) {
> notify_ring(engine);
> @@ -1494,7 +1489,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
> }
>
> if (tasklet)
> - tasklet_hi_schedule(&execlists->tasklet);
> + tasklet_hi_schedule(&engine->execlists.tasklet);
> }
>
> static void gen8_gt_irq_ack(struct drm_i915_private *i915,
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 7209c22798e6..ace93958689e 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1353,12 +1353,10 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine,
> ptr = I915_READ(RING_CONTEXT_STATUS_PTR(engine));
> read = GEN8_CSB_READ_PTR(ptr);
> write = GEN8_CSB_WRITE_PTR(ptr);
> - drm_printf(m, "\tExeclist CSB read %d [%d cached], write %d [%d from hws], interrupt posted? %s, tasklet queued? %s (%s)\n",
> + drm_printf(m, "\tExeclist CSB read %d [%d cached], write %d [%d from hws], tasklet queued? %s (%s)\n",
> read, execlists->csb_head,
> write,
> intel_read_status_page(engine, intel_hws_csb_write_index(engine->i915)),
> - yesno(test_bit(ENGINE_IRQ_EXECLIST,
> - &engine->irq_posted)),
> yesno(test_bit(TASKLET_STATE_SCHED,
> &engine->execlists.tasklet.state)),
> enableddisabled(!atomic_read(&engine->execlists.tasklet.count)));
> @@ -1570,11 +1568,9 @@ void intel_engine_dump(struct intel_engine_cs *engine,
> spin_unlock(&b->rb_lock);
> local_irq_restore(flags);
>
> - drm_printf(m, "IRQ? 0x%lx (breadcrumbs? %s) (execlists? %s)\n",
> + drm_printf(m, "IRQ? 0x%lx (breadcrumbs? %s)\n",
> engine->irq_posted,
> yesno(test_bit(ENGINE_IRQ_BREADCRUMB,
> - &engine->irq_posted)),
> - yesno(test_bit(ENGINE_IRQ_EXECLIST,
> &engine->irq_posted)));
>
> drm_printf(m, "HWSP:\n");
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 14be53035610..7f8b29684d9d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -874,14 +874,6 @@ static void reset_irq(struct intel_engine_cs *engine)
> smp_store_mb(engine->execlists.active, 0);
>
> clear_gtiir(engine);
> -
> - /*
> - * The port is checked prior to scheduling a tasklet, but
> - * just in case we have suspended the tasklet to do the
> - * wedging make sure that when it wakes, it decides there
> - * is no work to do by clearing the irq_posted bit.
> - */
> - clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> }
>
> static void reset_csb_pointers(struct intel_engine_execlists *execlists)
> @@ -972,10 +964,6 @@ static void process_csb(struct intel_engine_cs *engine)
> const u32 * const buf = execlists->csb_status;
> u8 head, tail;
>
> - /* Clear before reading to catch new interrupts */
> - clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> - smp_mb__after_atomic();
> -
> /*
> * Note that csb_write, csb_status may be either in HWSP or mmio.
> * When reading from the csb_write mmio register, we have to be
> @@ -1128,11 +1116,10 @@ static void execlists_submission_tasklet(unsigned long data)
> {
> struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
>
> - GEM_TRACE("%s awake?=%d, active=%x, irq-posted?=%d\n",
> + GEM_TRACE("%s awake?=%d, active=%x\n",
> engine->name,
> engine->i915->gt.awake,
> - engine->execlists.active,
> - test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted));
> + engine->execlists.active);
>
> /*
> * We can skip acquiring intel_runtime_pm_get() here as it was taken
> @@ -1144,14 +1131,7 @@ static void execlists_submission_tasklet(unsigned long data)
> */
> GEM_BUG_ON(!engine->i915->gt.awake);
>
> - /*
> - * Prefer doing test_and_clear_bit() as a two stage operation to avoid
> - * imposing the cost of a locked atomic transaction when submitting a
> - * new request (outside of the context-switch interrupt).
> - */
> - if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
> - process_csb(engine);
> -
> + process_csb(engine);
I'd probably add a newline here.
> if (!execlists_is_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT))
> execlists_dequeue(engine);
> }
> @@ -1919,8 +1899,7 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
> * and avoid blaming an innocent request if the stall was due to the
> * preemption itself.
> */
> - if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
> - process_csb(engine);
> + process_csb(engine);
>
> /*
> * The last active request can then be no later than the last request
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 62e3db6f6f8d..e1ee1ca9ac16 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -368,7 +368,6 @@ struct intel_engine_cs {
> atomic_t irq_count;
> unsigned long irq_posted;
> #define ENGINE_IRQ_BREADCRUMB 0
> -#define ENGINE_IRQ_EXECLIST 1
>
> /* Rather than have every client wait upon all user interrupts,
> * with the herd waking after every interrupt and each doing the
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 9/9] drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
` (6 preceding siblings ...)
2018-06-28 12:33 ` [PATCH 8/9] drm/i915/execlists: Trust the CSB Chris Wilson
@ 2018-06-28 12:33 ` Chris Wilson
2018-06-28 13:11 ` [PATCH v7] " Chris Wilson
2018-06-28 13:33 ` Chris Wilson
2018-06-28 12:50 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts Patchwork
` (6 subsequent siblings)
14 siblings, 2 replies; 24+ messages in thread
From: Chris Wilson @ 2018-06-28 12:33 UTC (permalink / raw)
To: intel-gfx
Back in commit 27af5eea54d1 ("drm/i915: Move execlists irq handler to a
bottom half"), we came to the conclusion that running our CSB processing
and ELSP submission from inside the irq handler was a bad idea. A really
bad idea as we could impose nearly 1s latency on other users of the
system, on average! Deferring our work to a tasklet allowed us to do the
processing with irqs enabled, reducing the impact to an average of about
50us.
We have since eradicated the use of forcewaked mmio from inside the CSB
processing and ELSP submission, bringing the impact down to around 5us
(on Kabylake); an order of magnitude better than our measurements 2
years ago on Broadwell and only about 2x worse on average than the
gem_syslatency on an unladen system.
In this iteration of the tasklet-vs-direct submission debate, we seek a
compromise where by we submit new requests immediately to the HW but
defer processing the CS interrupt onto a tasklet. We gain the advantage
of low-latency and ksoftirqd avoidance when waking up the HW, while
avoiding the system-wide starvation of our CS irq-storms.
Comparing the impact on the maximum latency observed (that is the time
stolen from an RT process) over a 120s interval, repeated several times
(using gem_syslatency, similar to RT's cyclictest) while the system is
fully laden with i915 nops, we see that direct submission an actually
improve the worse case.
Maximum latency in microseconds of a third party RT thread
(gem_syslatency -t 120 -f 2)
x Always using tasklets (a couple of >1000us outliers removed)
+ Only using tasklets from CS irq, direct submission of requests
+------------------------------------------------------------------------+
| + |
| + |
| + |
| + + |
| + + + |
| + + + + x x x |
| +++ + + + x x x x x x |
| +++ + ++ + + *x x x x x x |
| +++ + ++ + * *x x * x x x |
| + +++ + ++ * * +*xxx * x x xx |
| * +++ + ++++* *x+**xx+ * x x xxxx x |
| **x++++*++**+*x*x****x+ * +x xx xxxx x x |
|x* ******+***************++*+***xxxxxx* xx*x xxx + x+|
| |__________MA___________| |
| |______M__A________| |
+------------------------------------------------------------------------+
N Min Max Median Avg Stddev
x 118 91 186 124 125.28814 16.279137
+ 120 92 187 109 112.00833 13.458617
Difference at 95.0% confidence
-13.2798 +/- 3.79219
-10.5994% +/- 3.02677%
(Student's t, pooled s = 14.9237)
However the mean latency is adversely affected:
Mean latency in microseconds of a third party RT thread
(gem_syslatency -t 120 -f 1)
x Always using tasklets
+ Only using tasklets from CS irq, direct submission of requests
+------------------------------------------------------------------------+
| xxxxxx + ++ |
| xxxxxx + ++ |
| xxxxxx + +++ ++ |
| xxxxxxx +++++ ++ |
| xxxxxxx +++++ ++ |
| xxxxxxx +++++ +++ |
| xxxxxxx + ++++++++++ |
| xxxxxxxx ++ ++++++++++ |
| xxxxxxxx ++ ++++++++++ |
| xxxxxxxxxx +++++++++++++++ |
| xxxxxxxxxxx x +++++++++++++++ |
|x xxxxxxxxxxxxx x + + ++++++++++++++++++ +|
| |__A__| |
| |____A___| |
+------------------------------------------------------------------------+
N Min Max Median Avg Stddev
x 120 3.506 3.727 3.631 3.6321417 0.02773109
+ 120 3.834 4.149 4.039 4.0375167 0.041221676
Difference at 95.0% confidence
0.405375 +/- 0.00888913
11.1608% +/- 0.244735%
(Student's t, pooled s = 0.03513)
However, since the mean latency corresponds to the amount of irqsoff
processing we have to do for a CS interrupt, we only need to speed that
up to benefit not just system latency but our own throughput.
v2: Remember to defer submissions when under reset.
v4: Only use direct submission for new requests
v5: Be aware that with mixing direct tasklet evaluation and deferred
tasklets, we may end up idling before running the deferred tasklet.
v6: Remove the redudant likely() from tasklet_is_enabled(), restrict the
annotation to reset_in_progress().
Testcase: igt/gem_exec_latency/*rthog*
References: 27af5eea54d1 ("drm/i915: Move execlists irq handler to a bottom half")
Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_gem.h | 5 ++
drivers/gpu/drm/i915/intel_engine_cs.c | 1 +
drivers/gpu/drm/i915/intel_lrc.c | 102 ++++++++++++++++---------
3 files changed, 72 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
index 261da577829a..e46592956872 100644
--- a/drivers/gpu/drm/i915/i915_gem.h
+++ b/drivers/gpu/drm/i915/i915_gem.h
@@ -88,4 +88,9 @@ static inline void __tasklet_enable_sync_once(struct tasklet_struct *t)
tasklet_kill(t);
}
+static inline bool __tasklet_is_enabled(const struct tasklet_struct *t)
+{
+ return !atomic_read(&t->count);
+}
+
#endif /* __I915_GEM_H__ */
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index ace93958689e..9bbdee24e254 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1646,6 +1646,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
unlock:
write_sequnlock_irqrestore(&engine->stats.lock, flags);
tasklet_enable(&execlists->tasklet);
+ tasklet_hi_schedule(&execlists->tasklet);
return err;
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 7f8b29684d9d..ab89dabc2965 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -563,12 +563,14 @@ static void complete_preempt_context(struct intel_engine_execlists *execlists)
GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT));
execlists_cancel_port_requests(execlists);
- execlists_unwind_incomplete_requests(execlists);
+ __unwind_incomplete_requests(container_of(execlists,
+ struct intel_engine_cs,
+ execlists));
execlists_clear_active(execlists, EXECLISTS_ACTIVE_PREEMPT);
}
-static void __execlists_dequeue(struct intel_engine_cs *engine)
+static void execlists_dequeue(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
struct execlist_port *port = execlists->port;
@@ -578,9 +580,8 @@ static void __execlists_dequeue(struct intel_engine_cs *engine)
struct rb_node *rb;
bool submit = false;
- lockdep_assert_held(&engine->timeline.lock);
-
- /* Hardware submission is through 2 ports. Conceptually each port
+ /*
+ * Hardware submission is through 2 ports. Conceptually each port
* has a (RING_START, RING_HEAD, RING_TAIL) tuple. RING_START is
* static for a context, and unique to each, so we only execute
* requests belonging to a single context from each ring. RING_HEAD
@@ -770,15 +771,6 @@ static void __execlists_dequeue(struct intel_engine_cs *engine)
!port_isset(engine->execlists.port));
}
-static void execlists_dequeue(struct intel_engine_cs *engine)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&engine->timeline.lock, flags);
- __execlists_dequeue(engine);
- spin_unlock_irqrestore(&engine->timeline.lock, flags);
-}
-
void
execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
{
@@ -957,6 +949,12 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
spin_unlock_irqrestore(&engine->timeline.lock, flags);
}
+static inline bool
+reset_in_progress(const struct intel_engine_execlists *execlists)
+{
+ return unlikely(!__tasklet_is_enabled(&execlists->tasklet));
+}
+
static void process_csb(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
@@ -1108,18 +1106,9 @@ static void process_csb(struct intel_engine_cs *engine)
execlists->csb_head = head;
}
-/*
- * Check the unread Context Status Buffers and manage the submission of new
- * contexts to the ELSP accordingly.
- */
-static void execlists_submission_tasklet(unsigned long data)
+static void __execlists_submission_tasklet(struct intel_engine_cs *const engine)
{
- struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
-
- GEM_TRACE("%s awake?=%d, active=%x\n",
- engine->name,
- engine->i915->gt.awake,
- engine->execlists.active);
+ lockdep_assert_held(&engine->timeline.lock);
/*
* We can skip acquiring intel_runtime_pm_get() here as it was taken
@@ -1136,6 +1125,28 @@ static void execlists_submission_tasklet(unsigned long data)
execlists_dequeue(engine);
}
+/*
+ * Check the unread Context Status Buffers and manage the submission of new
+ * contexts to the ELSP accordingly.
+ */
+static void execlists_submission_tasklet(unsigned long data)
+{
+ struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
+ unsigned long flags;
+
+ GEM_TRACE("%s awake?=%d, active=%x\n",
+ engine->name,
+ engine->i915->gt.awake,
+ engine->execlists.active);
+
+ spin_lock_irqsave(&engine->timeline.lock, flags);
+
+ if (engine->i915->gt.awake) /* we may be delayed until after we idle! */
+ __execlists_submission_tasklet(engine);
+
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
+}
+
static void queue_request(struct intel_engine_cs *engine,
struct i915_sched_node *node,
int prio)
@@ -1144,16 +1155,30 @@ static void queue_request(struct intel_engine_cs *engine,
&lookup_priolist(engine, prio)->requests);
}
-static void __submit_queue(struct intel_engine_cs *engine, int prio)
+static void __update_queue(struct intel_engine_cs *engine, int prio)
{
engine->execlists.queue_priority = prio;
- tasklet_hi_schedule(&engine->execlists.tasklet);
+}
+
+static void __submit_queue_imm(struct intel_engine_cs *engine)
+{
+ struct intel_engine_execlists * const execlists = &engine->execlists;
+
+ if (reset_in_progress(execlists))
+ return; /* defer until we restart the engine following reset */
+
+ if (execlists->tasklet.func == execlists_submission_tasklet)
+ __execlists_submission_tasklet(engine);
+ else
+ tasklet_hi_schedule(&execlists->tasklet);
}
static void submit_queue(struct intel_engine_cs *engine, int prio)
{
- if (prio > engine->execlists.queue_priority)
- __submit_queue(engine, prio);
+ if (prio > engine->execlists.queue_priority) {
+ __update_queue(engine, prio);
+ __submit_queue_imm(engine);
+ }
}
static void execlists_submit_request(struct i915_request *request)
@@ -1165,11 +1190,12 @@ static void execlists_submit_request(struct i915_request *request)
spin_lock_irqsave(&engine->timeline.lock, flags);
queue_request(engine, &request->sched, rq_prio(request));
- submit_queue(engine, rq_prio(request));
GEM_BUG_ON(!engine->execlists.first);
GEM_BUG_ON(list_empty(&request->sched.link));
+ submit_queue(engine, rq_prio(request));
+
spin_unlock_irqrestore(&engine->timeline.lock, flags);
}
@@ -1296,8 +1322,11 @@ static void execlists_schedule(struct i915_request *request,
}
if (prio > engine->execlists.queue_priority &&
- i915_sw_fence_done(&sched_to_request(node)->submit))
- __submit_queue(engine, prio);
+ i915_sw_fence_done(&sched_to_request(node)->submit)) {
+ /* defer submission until after all of our updates */
+ __update_queue(engine, prio);
+ tasklet_hi_schedule(&engine->execlists.tasklet);
+ }
}
spin_unlock_irq(&engine->timeline.lock);
@@ -1878,6 +1907,7 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
struct i915_request *request, *active;
+ unsigned long flags;
GEM_TRACE("%s\n", engine->name);
@@ -1892,6 +1922,8 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
*/
__tasklet_disable_sync_once(&execlists->tasklet);
+ spin_lock_irqsave(&engine->timeline.lock, flags);
+
/*
* We want to flush the pending context switches, having disabled
* the tasklet above, we can assume exclusive access to the execlists.
@@ -1909,15 +1941,12 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
active = NULL;
request = port_request(execlists->port);
if (request) {
- unsigned long flags;
-
/*
* Prevent the breadcrumb from advancing before we decide
* which request is currently active.
*/
intel_engine_stop_cs(engine);
- spin_lock_irqsave(&engine->timeline.lock, flags);
list_for_each_entry_from_reverse(request,
&engine->timeline.requests,
link) {
@@ -1927,9 +1956,10 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
active = request;
}
- spin_unlock_irqrestore(&engine->timeline.lock, flags);
}
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
+
return active;
}
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v7] drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)
2018-06-28 12:33 ` [PATCH 9/9] drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd) Chris Wilson
@ 2018-06-28 13:11 ` Chris Wilson
2018-06-28 13:21 ` Tvrtko Ursulin
2018-06-28 13:33 ` Chris Wilson
1 sibling, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2018-06-28 13:11 UTC (permalink / raw)
To: intel-gfx
Back in commit 27af5eea54d1 ("drm/i915: Move execlists irq handler to a
bottom half"), we came to the conclusion that running our CSB processing
and ELSP submission from inside the irq handler was a bad idea. A really
bad idea as we could impose nearly 1s latency on other users of the
system, on average! Deferring our work to a tasklet allowed us to do the
processing with irqs enabled, reducing the impact to an average of about
50us.
We have since eradicated the use of forcewaked mmio from inside the CSB
processing and ELSP submission, bringing the impact down to around 5us
(on Kabylake); an order of magnitude better than our measurements 2
years ago on Broadwell and only about 2x worse on average than the
gem_syslatency on an unladen system.
In this iteration of the tasklet-vs-direct submission debate, we seek a
compromise where by we submit new requests immediately to the HW but
defer processing the CS interrupt onto a tasklet. We gain the advantage
of low-latency and ksoftirqd avoidance when waking up the HW, while
avoiding the system-wide starvation of our CS irq-storms.
Comparing the impact on the maximum latency observed (that is the time
stolen from an RT process) over a 120s interval, repeated several times
(using gem_syslatency, similar to RT's cyclictest) while the system is
fully laden with i915 nops, we see that direct submission an actually
improve the worse case.
Maximum latency in microseconds of a third party RT thread
(gem_syslatency -t 120 -f 2)
x Always using tasklets (a couple of >1000us outliers removed)
+ Only using tasklets from CS irq, direct submission of requests
+------------------------------------------------------------------------+
| + |
| + |
| + |
| + + |
| + + + |
| + + + + x x x |
| +++ + + + x x x x x x |
| +++ + ++ + + *x x x x x x |
| +++ + ++ + * *x x * x x x |
| + +++ + ++ * * +*xxx * x x xx |
| * +++ + ++++* *x+**xx+ * x x xxxx x |
| **x++++*++**+*x*x****x+ * +x xx xxxx x x |
|x* ******+***************++*+***xxxxxx* xx*x xxx + x+|
| |__________MA___________| |
| |______M__A________| |
+------------------------------------------------------------------------+
N Min Max Median Avg Stddev
x 118 91 186 124 125.28814 16.279137
+ 120 92 187 109 112.00833 13.458617
Difference at 95.0% confidence
-13.2798 +/- 3.79219
-10.5994% +/- 3.02677%
(Student's t, pooled s = 14.9237)
However the mean latency is adversely affected:
Mean latency in microseconds of a third party RT thread
(gem_syslatency -t 120 -f 1)
x Always using tasklets
+ Only using tasklets from CS irq, direct submission of requests
+------------------------------------------------------------------------+
| xxxxxx + ++ |
| xxxxxx + ++ |
| xxxxxx + +++ ++ |
| xxxxxxx +++++ ++ |
| xxxxxxx +++++ ++ |
| xxxxxxx +++++ +++ |
| xxxxxxx + ++++++++++ |
| xxxxxxxx ++ ++++++++++ |
| xxxxxxxx ++ ++++++++++ |
| xxxxxxxxxx +++++++++++++++ |
| xxxxxxxxxxx x +++++++++++++++ |
|x xxxxxxxxxxxxx x + + ++++++++++++++++++ +|
| |__A__| |
| |____A___| |
+------------------------------------------------------------------------+
N Min Max Median Avg Stddev
x 120 3.506 3.727 3.631 3.6321417 0.02773109
+ 120 3.834 4.149 4.039 4.0375167 0.041221676
Difference at 95.0% confidence
0.405375 +/- 0.00888913
11.1608% +/- 0.244735%
(Student's t, pooled s = 0.03513)
However, since the mean latency corresponds to the amount of irqsoff
processing we have to do for a CS interrupt, we only need to speed that
up to benefit not just system latency but our own throughput.
v2: Remember to defer submissions when under reset.
v4: Only use direct submission for new requests
v5: Be aware that with mixing direct tasklet evaluation and deferred
tasklets, we may end up idling before running the deferred tasklet.
v6: Remove the redudant likely() from tasklet_is_enabled(), restrict the
annotation to reset_in_progress().
v7: Take the full timeline.lock when enabling perf_pmu stats as the
tasklet is no longer a valid guard. A consequence is that the stats are
now only valid for engines also using the timeline.lock to process
state.
Testcase: igt/gem_exec_latency/*rthog*
References: 27af5eea54d1 ("drm/i915: Move execlists irq handler to a bottom half")
Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_gem.h | 5 ++
drivers/gpu/drm/i915/intel_engine_cs.c | 8 +-
drivers/gpu/drm/i915/intel_lrc.c | 102 ++++++++++++++++---------
3 files changed, 75 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
index 261da577829a..e46592956872 100644
--- a/drivers/gpu/drm/i915/i915_gem.h
+++ b/drivers/gpu/drm/i915/i915_gem.h
@@ -88,4 +88,9 @@ static inline void __tasklet_enable_sync_once(struct tasklet_struct *t)
tasklet_kill(t);
}
+static inline bool __tasklet_is_enabled(const struct tasklet_struct *t)
+{
+ return !atomic_read(&t->count);
+}
+
#endif /* __I915_GEM_H__ */
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index ace93958689e..7fc97bc5362d 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1619,8 +1619,8 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
if (!intel_engine_supports_stats(engine))
return -ENODEV;
- tasklet_disable(&execlists->tasklet);
- write_seqlock_irqsave(&engine->stats.lock, flags);
+ spin_lock_irqsave(&engine->timeline.lock, flags);
+ write_seqlock(&engine->stats.lock, flags);
if (unlikely(engine->stats.enabled == ~0)) {
err = -EBUSY;
@@ -1644,8 +1644,8 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
}
unlock:
- write_sequnlock_irqrestore(&engine->stats.lock, flags);
- tasklet_enable(&execlists->tasklet);
+ write_sequnlock(&engine->stats.lock);
+ spin_lock_irqrestore(&engine->timeline.lock, flags);
return err;
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 7f8b29684d9d..ab89dabc2965 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -563,12 +563,14 @@ static void complete_preempt_context(struct intel_engine_execlists *execlists)
GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT));
execlists_cancel_port_requests(execlists);
- execlists_unwind_incomplete_requests(execlists);
+ __unwind_incomplete_requests(container_of(execlists,
+ struct intel_engine_cs,
+ execlists));
execlists_clear_active(execlists, EXECLISTS_ACTIVE_PREEMPT);
}
-static void __execlists_dequeue(struct intel_engine_cs *engine)
+static void execlists_dequeue(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
struct execlist_port *port = execlists->port;
@@ -578,9 +580,8 @@ static void __execlists_dequeue(struct intel_engine_cs *engine)
struct rb_node *rb;
bool submit = false;
- lockdep_assert_held(&engine->timeline.lock);
-
- /* Hardware submission is through 2 ports. Conceptually each port
+ /*
+ * Hardware submission is through 2 ports. Conceptually each port
* has a (RING_START, RING_HEAD, RING_TAIL) tuple. RING_START is
* static for a context, and unique to each, so we only execute
* requests belonging to a single context from each ring. RING_HEAD
@@ -770,15 +771,6 @@ static void __execlists_dequeue(struct intel_engine_cs *engine)
!port_isset(engine->execlists.port));
}
-static void execlists_dequeue(struct intel_engine_cs *engine)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&engine->timeline.lock, flags);
- __execlists_dequeue(engine);
- spin_unlock_irqrestore(&engine->timeline.lock, flags);
-}
-
void
execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
{
@@ -957,6 +949,12 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
spin_unlock_irqrestore(&engine->timeline.lock, flags);
}
+static inline bool
+reset_in_progress(const struct intel_engine_execlists *execlists)
+{
+ return unlikely(!__tasklet_is_enabled(&execlists->tasklet));
+}
+
static void process_csb(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
@@ -1108,18 +1106,9 @@ static void process_csb(struct intel_engine_cs *engine)
execlists->csb_head = head;
}
-/*
- * Check the unread Context Status Buffers and manage the submission of new
- * contexts to the ELSP accordingly.
- */
-static void execlists_submission_tasklet(unsigned long data)
+static void __execlists_submission_tasklet(struct intel_engine_cs *const engine)
{
- struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
-
- GEM_TRACE("%s awake?=%d, active=%x\n",
- engine->name,
- engine->i915->gt.awake,
- engine->execlists.active);
+ lockdep_assert_held(&engine->timeline.lock);
/*
* We can skip acquiring intel_runtime_pm_get() here as it was taken
@@ -1136,6 +1125,28 @@ static void execlists_submission_tasklet(unsigned long data)
execlists_dequeue(engine);
}
+/*
+ * Check the unread Context Status Buffers and manage the submission of new
+ * contexts to the ELSP accordingly.
+ */
+static void execlists_submission_tasklet(unsigned long data)
+{
+ struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
+ unsigned long flags;
+
+ GEM_TRACE("%s awake?=%d, active=%x\n",
+ engine->name,
+ engine->i915->gt.awake,
+ engine->execlists.active);
+
+ spin_lock_irqsave(&engine->timeline.lock, flags);
+
+ if (engine->i915->gt.awake) /* we may be delayed until after we idle! */
+ __execlists_submission_tasklet(engine);
+
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
+}
+
static void queue_request(struct intel_engine_cs *engine,
struct i915_sched_node *node,
int prio)
@@ -1144,16 +1155,30 @@ static void queue_request(struct intel_engine_cs *engine,
&lookup_priolist(engine, prio)->requests);
}
-static void __submit_queue(struct intel_engine_cs *engine, int prio)
+static void __update_queue(struct intel_engine_cs *engine, int prio)
{
engine->execlists.queue_priority = prio;
- tasklet_hi_schedule(&engine->execlists.tasklet);
+}
+
+static void __submit_queue_imm(struct intel_engine_cs *engine)
+{
+ struct intel_engine_execlists * const execlists = &engine->execlists;
+
+ if (reset_in_progress(execlists))
+ return; /* defer until we restart the engine following reset */
+
+ if (execlists->tasklet.func == execlists_submission_tasklet)
+ __execlists_submission_tasklet(engine);
+ else
+ tasklet_hi_schedule(&execlists->tasklet);
}
static void submit_queue(struct intel_engine_cs *engine, int prio)
{
- if (prio > engine->execlists.queue_priority)
- __submit_queue(engine, prio);
+ if (prio > engine->execlists.queue_priority) {
+ __update_queue(engine, prio);
+ __submit_queue_imm(engine);
+ }
}
static void execlists_submit_request(struct i915_request *request)
@@ -1165,11 +1190,12 @@ static void execlists_submit_request(struct i915_request *request)
spin_lock_irqsave(&engine->timeline.lock, flags);
queue_request(engine, &request->sched, rq_prio(request));
- submit_queue(engine, rq_prio(request));
GEM_BUG_ON(!engine->execlists.first);
GEM_BUG_ON(list_empty(&request->sched.link));
+ submit_queue(engine, rq_prio(request));
+
spin_unlock_irqrestore(&engine->timeline.lock, flags);
}
@@ -1296,8 +1322,11 @@ static void execlists_schedule(struct i915_request *request,
}
if (prio > engine->execlists.queue_priority &&
- i915_sw_fence_done(&sched_to_request(node)->submit))
- __submit_queue(engine, prio);
+ i915_sw_fence_done(&sched_to_request(node)->submit)) {
+ /* defer submission until after all of our updates */
+ __update_queue(engine, prio);
+ tasklet_hi_schedule(&engine->execlists.tasklet);
+ }
}
spin_unlock_irq(&engine->timeline.lock);
@@ -1878,6 +1907,7 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
struct i915_request *request, *active;
+ unsigned long flags;
GEM_TRACE("%s\n", engine->name);
@@ -1892,6 +1922,8 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
*/
__tasklet_disable_sync_once(&execlists->tasklet);
+ spin_lock_irqsave(&engine->timeline.lock, flags);
+
/*
* We want to flush the pending context switches, having disabled
* the tasklet above, we can assume exclusive access to the execlists.
@@ -1909,15 +1941,12 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
active = NULL;
request = port_request(execlists->port);
if (request) {
- unsigned long flags;
-
/*
* Prevent the breadcrumb from advancing before we decide
* which request is currently active.
*/
intel_engine_stop_cs(engine);
- spin_lock_irqsave(&engine->timeline.lock, flags);
list_for_each_entry_from_reverse(request,
&engine->timeline.requests,
link) {
@@ -1927,9 +1956,10 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
active = request;
}
- spin_unlock_irqrestore(&engine->timeline.lock, flags);
}
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
+
return active;
}
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v7] drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)
2018-06-28 13:11 ` [PATCH v7] " Chris Wilson
@ 2018-06-28 13:21 ` Tvrtko Ursulin
2018-06-28 13:28 ` Chris Wilson
0 siblings, 1 reply; 24+ messages in thread
From: Tvrtko Ursulin @ 2018-06-28 13:21 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 28/06/2018 14:11, Chris Wilson wrote:
> Back in commit 27af5eea54d1 ("drm/i915: Move execlists irq handler to a
> bottom half"), we came to the conclusion that running our CSB processing
> and ELSP submission from inside the irq handler was a bad idea. A really
> bad idea as we could impose nearly 1s latency on other users of the
> system, on average! Deferring our work to a tasklet allowed us to do the
> processing with irqs enabled, reducing the impact to an average of about
> 50us.
>
> We have since eradicated the use of forcewaked mmio from inside the CSB
> processing and ELSP submission, bringing the impact down to around 5us
> (on Kabylake); an order of magnitude better than our measurements 2
> years ago on Broadwell and only about 2x worse on average than the
> gem_syslatency on an unladen system.
>
> In this iteration of the tasklet-vs-direct submission debate, we seek a
> compromise where by we submit new requests immediately to the HW but
> defer processing the CS interrupt onto a tasklet. We gain the advantage
> of low-latency and ksoftirqd avoidance when waking up the HW, while
> avoiding the system-wide starvation of our CS irq-storms.
>
> Comparing the impact on the maximum latency observed (that is the time
> stolen from an RT process) over a 120s interval, repeated several times
> (using gem_syslatency, similar to RT's cyclictest) while the system is
> fully laden with i915 nops, we see that direct submission an actually
> improve the worse case.
>
> Maximum latency in microseconds of a third party RT thread
> (gem_syslatency -t 120 -f 2)
> x Always using tasklets (a couple of >1000us outliers removed)
> + Only using tasklets from CS irq, direct submission of requests
> +------------------------------------------------------------------------+
> | + |
> | + |
> | + |
> | + + |
> | + + + |
> | + + + + x x x |
> | +++ + + + x x x x x x |
> | +++ + ++ + + *x x x x x x |
> | +++ + ++ + * *x x * x x x |
> | + +++ + ++ * * +*xxx * x x xx |
> | * +++ + ++++* *x+**xx+ * x x xxxx x |
> | **x++++*++**+*x*x****x+ * +x xx xxxx x x |
> |x* ******+***************++*+***xxxxxx* xx*x xxx + x+|
> | |__________MA___________| |
> | |______M__A________| |
> +------------------------------------------------------------------------+
> N Min Max Median Avg Stddev
> x 118 91 186 124 125.28814 16.279137
> + 120 92 187 109 112.00833 13.458617
> Difference at 95.0% confidence
> -13.2798 +/- 3.79219
> -10.5994% +/- 3.02677%
> (Student's t, pooled s = 14.9237)
>
> However the mean latency is adversely affected:
>
> Mean latency in microseconds of a third party RT thread
> (gem_syslatency -t 120 -f 1)
> x Always using tasklets
> + Only using tasklets from CS irq, direct submission of requests
> +------------------------------------------------------------------------+
> | xxxxxx + ++ |
> | xxxxxx + ++ |
> | xxxxxx + +++ ++ |
> | xxxxxxx +++++ ++ |
> | xxxxxxx +++++ ++ |
> | xxxxxxx +++++ +++ |
> | xxxxxxx + ++++++++++ |
> | xxxxxxxx ++ ++++++++++ |
> | xxxxxxxx ++ ++++++++++ |
> | xxxxxxxxxx +++++++++++++++ |
> | xxxxxxxxxxx x +++++++++++++++ |
> |x xxxxxxxxxxxxx x + + ++++++++++++++++++ +|
> | |__A__| |
> | |____A___| |
> +------------------------------------------------------------------------+
> N Min Max Median Avg Stddev
> x 120 3.506 3.727 3.631 3.6321417 0.02773109
> + 120 3.834 4.149 4.039 4.0375167 0.041221676
> Difference at 95.0% confidence
> 0.405375 +/- 0.00888913
> 11.1608% +/- 0.244735%
> (Student's t, pooled s = 0.03513)
>
> However, since the mean latency corresponds to the amount of irqsoff
> processing we have to do for a CS interrupt, we only need to speed that
> up to benefit not just system latency but our own throughput.
>
> v2: Remember to defer submissions when under reset.
> v4: Only use direct submission for new requests
> v5: Be aware that with mixing direct tasklet evaluation and deferred
> tasklets, we may end up idling before running the deferred tasklet.
> v6: Remove the redudant likely() from tasklet_is_enabled(), restrict the
> annotation to reset_in_progress().
> v7: Take the full timeline.lock when enabling perf_pmu stats as the
> tasklet is no longer a valid guard. A consequence is that the stats are
> now only valid for engines also using the timeline.lock to process
> state.
>
> Testcase: igt/gem_exec_latency/*rthog*
> References: 27af5eea54d1 ("drm/i915: Move execlists irq handler to a bottom half")
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.h | 5 ++
> drivers/gpu/drm/i915/intel_engine_cs.c | 8 +-
> drivers/gpu/drm/i915/intel_lrc.c | 102 ++++++++++++++++---------
> 3 files changed, 75 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> index 261da577829a..e46592956872 100644
> --- a/drivers/gpu/drm/i915/i915_gem.h
> +++ b/drivers/gpu/drm/i915/i915_gem.h
> @@ -88,4 +88,9 @@ static inline void __tasklet_enable_sync_once(struct tasklet_struct *t)
> tasklet_kill(t);
> }
>
> +static inline bool __tasklet_is_enabled(const struct tasklet_struct *t)
> +{
> + return !atomic_read(&t->count);
> +}
> +
> #endif /* __I915_GEM_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index ace93958689e..7fc97bc5362d 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1619,8 +1619,8 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
> if (!intel_engine_supports_stats(engine))
> return -ENODEV;
>
> - tasklet_disable(&execlists->tasklet);
> - write_seqlock_irqsave(&engine->stats.lock, flags);
> + spin_lock_irqsave(&engine->timeline.lock, flags);
> + write_seqlock(&engine->stats.lock, flags);
>
> if (unlikely(engine->stats.enabled == ~0)) {
> err = -EBUSY;
> @@ -1644,8 +1644,8 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
> }
>
> unlock:
> - write_sequnlock_irqrestore(&engine->stats.lock, flags);
> - tasklet_enable(&execlists->tasklet);
> + write_sequnlock(&engine->stats.lock);
> + spin_lock_irqrestore(&engine->timeline.lock, flags);
>
> return err;
> }
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 7f8b29684d9d..ab89dabc2965 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -563,12 +563,14 @@ static void complete_preempt_context(struct intel_engine_execlists *execlists)
> GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT));
>
> execlists_cancel_port_requests(execlists);
> - execlists_unwind_incomplete_requests(execlists);
> + __unwind_incomplete_requests(container_of(execlists,
> + struct intel_engine_cs,
> + execlists));
>
> execlists_clear_active(execlists, EXECLISTS_ACTIVE_PREEMPT);
> }
>
> -static void __execlists_dequeue(struct intel_engine_cs *engine)
> +static void execlists_dequeue(struct intel_engine_cs *engine)
> {
> struct intel_engine_execlists * const execlists = &engine->execlists;
> struct execlist_port *port = execlists->port;
> @@ -578,9 +580,8 @@ static void __execlists_dequeue(struct intel_engine_cs *engine)
> struct rb_node *rb;
> bool submit = false;
>
> - lockdep_assert_held(&engine->timeline.lock);
> -
> - /* Hardware submission is through 2 ports. Conceptually each port
> + /*
> + * Hardware submission is through 2 ports. Conceptually each port
> * has a (RING_START, RING_HEAD, RING_TAIL) tuple. RING_START is
> * static for a context, and unique to each, so we only execute
> * requests belonging to a single context from each ring. RING_HEAD
> @@ -770,15 +771,6 @@ static void __execlists_dequeue(struct intel_engine_cs *engine)
> !port_isset(engine->execlists.port));
> }
>
> -static void execlists_dequeue(struct intel_engine_cs *engine)
> -{
> - unsigned long flags;
> -
> - spin_lock_irqsave(&engine->timeline.lock, flags);
> - __execlists_dequeue(engine);
> - spin_unlock_irqrestore(&engine->timeline.lock, flags);
> -}
> -
> void
> execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
> {
> @@ -957,6 +949,12 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
> spin_unlock_irqrestore(&engine->timeline.lock, flags);
> }
>
> +static inline bool
> +reset_in_progress(const struct intel_engine_execlists *execlists)
> +{
> + return unlikely(!__tasklet_is_enabled(&execlists->tasklet));
> +}
> +
> static void process_csb(struct intel_engine_cs *engine)
> {
> struct intel_engine_execlists * const execlists = &engine->execlists;
> @@ -1108,18 +1106,9 @@ static void process_csb(struct intel_engine_cs *engine)
> execlists->csb_head = head;
> }
>
> -/*
> - * Check the unread Context Status Buffers and manage the submission of new
> - * contexts to the ELSP accordingly.
> - */
> -static void execlists_submission_tasklet(unsigned long data)
> +static void __execlists_submission_tasklet(struct intel_engine_cs *const engine)
> {
> - struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
> -
> - GEM_TRACE("%s awake?=%d, active=%x\n",
> - engine->name,
> - engine->i915->gt.awake,
> - engine->execlists.active);
> + lockdep_assert_held(&engine->timeline.lock);
>
> /*
> * We can skip acquiring intel_runtime_pm_get() here as it was taken
> @@ -1136,6 +1125,28 @@ static void execlists_submission_tasklet(unsigned long data)
> execlists_dequeue(engine);
> }
>
> +/*
> + * Check the unread Context Status Buffers and manage the submission of new
> + * contexts to the ELSP accordingly.
> + */
> +static void execlists_submission_tasklet(unsigned long data)
> +{
> + struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
> + unsigned long flags;
> +
> + GEM_TRACE("%s awake?=%d, active=%x\n",
> + engine->name,
> + engine->i915->gt.awake,
> + engine->execlists.active);
> +
> + spin_lock_irqsave(&engine->timeline.lock, flags);
> +
> + if (engine->i915->gt.awake) /* we may be delayed until after we idle! */
No races between the check and tasklet call? In other words the code
path which you were describing that can race is taking the timeline lock?
> + __execlists_submission_tasklet(engine);
> +
> + spin_unlock_irqrestore(&engine->timeline.lock, flags);
> +}
> +
> static void queue_request(struct intel_engine_cs *engine,
> struct i915_sched_node *node,
> int prio)
> @@ -1144,16 +1155,30 @@ static void queue_request(struct intel_engine_cs *engine,
> &lookup_priolist(engine, prio)->requests);
> }
>
> -static void __submit_queue(struct intel_engine_cs *engine, int prio)
> +static void __update_queue(struct intel_engine_cs *engine, int prio)
> {
> engine->execlists.queue_priority = prio;
> - tasklet_hi_schedule(&engine->execlists.tasklet);
> +}
> +
> +static void __submit_queue_imm(struct intel_engine_cs *engine)
> +{
> + struct intel_engine_execlists * const execlists = &engine->execlists;
> +
> + if (reset_in_progress(execlists))
> + return; /* defer until we restart the engine following reset */
We have a tasklet kick somewhere in that path?
> +
> + if (execlists->tasklet.func == execlists_submission_tasklet)
> + __execlists_submission_tasklet(engine);
> + else
> + tasklet_hi_schedule(&execlists->tasklet);
> }
>
> static void submit_queue(struct intel_engine_cs *engine, int prio)
> {
> - if (prio > engine->execlists.queue_priority)
> - __submit_queue(engine, prio);
> + if (prio > engine->execlists.queue_priority) {
> + __update_queue(engine, prio);
> + __submit_queue_imm(engine);
> + }
> }
>
> static void execlists_submit_request(struct i915_request *request)
> @@ -1165,11 +1190,12 @@ static void execlists_submit_request(struct i915_request *request)
> spin_lock_irqsave(&engine->timeline.lock, flags);
>
> queue_request(engine, &request->sched, rq_prio(request));
> - submit_queue(engine, rq_prio(request));
>
> GEM_BUG_ON(!engine->execlists.first);
> GEM_BUG_ON(list_empty(&request->sched.link));
>
> + submit_queue(engine, rq_prio(request));
> +
> spin_unlock_irqrestore(&engine->timeline.lock, flags);
> }
>
> @@ -1296,8 +1322,11 @@ static void execlists_schedule(struct i915_request *request,
> }
>
> if (prio > engine->execlists.queue_priority &&
> - i915_sw_fence_done(&sched_to_request(node)->submit))
> - __submit_queue(engine, prio);
> + i915_sw_fence_done(&sched_to_request(node)->submit)) {
> + /* defer submission until after all of our updates */
> + __update_queue(engine, prio);
> + tasklet_hi_schedule(&engine->execlists.tasklet);
_imm suffix on __submit_queue_imm sounds unused if there is no plain
__submit_queue, which could have been used here. But meh.
> + }
> }
>
> spin_unlock_irq(&engine->timeline.lock);
> @@ -1878,6 +1907,7 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
> {
> struct intel_engine_execlists * const execlists = &engine->execlists;
> struct i915_request *request, *active;
> + unsigned long flags;
>
> GEM_TRACE("%s\n", engine->name);
>
> @@ -1892,6 +1922,8 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
> */
> __tasklet_disable_sync_once(&execlists->tasklet);
>
> + spin_lock_irqsave(&engine->timeline.lock, flags);
> +
> /*
> * We want to flush the pending context switches, having disabled
> * the tasklet above, we can assume exclusive access to the execlists.
> @@ -1909,15 +1941,12 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
> active = NULL;
> request = port_request(execlists->port);
> if (request) {
> - unsigned long flags;
> -
> /*
> * Prevent the breadcrumb from advancing before we decide
> * which request is currently active.
> */
> intel_engine_stop_cs(engine);
>
> - spin_lock_irqsave(&engine->timeline.lock, flags);
> list_for_each_entry_from_reverse(request,
> &engine->timeline.requests,
> link) {
> @@ -1927,9 +1956,10 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
>
> active = request;
> }
> - spin_unlock_irqrestore(&engine->timeline.lock, flags);
> }
>
> + spin_unlock_irqrestore(&engine->timeline.lock, flags);
> +
> return active;
> }
>
>
Much easier to read and understand now that it is smaller, thanks!
Just a couple question above and no other complaints.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v7] drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)
2018-06-28 13:21 ` Tvrtko Ursulin
@ 2018-06-28 13:28 ` Chris Wilson
2018-06-28 13:59 ` Tvrtko Ursulin
0 siblings, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2018-06-28 13:28 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2018-06-28 14:21:06)
>
> On 28/06/2018 14:11, Chris Wilson wrote:
> > +/*
> > + * Check the unread Context Status Buffers and manage the submission of new
> > + * contexts to the ELSP accordingly.
> > + */
> > +static void execlists_submission_tasklet(unsigned long data)
> > +{
> > + struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
> > + unsigned long flags;
> > +
> > + GEM_TRACE("%s awake?=%d, active=%x\n",
> > + engine->name,
> > + engine->i915->gt.awake,
> > + engine->execlists.active);
> > +
> > + spin_lock_irqsave(&engine->timeline.lock, flags);
> > +
> > + if (engine->i915->gt.awake) /* we may be delayed until after we idle! */
>
> No races between the check and tasklet call? In other words the code
> path which you were describing that can race is taking the timeline lock?
intel_engine_is_idle() is unserialised.
> > + __execlists_submission_tasklet(engine);
> > +
> > + spin_unlock_irqrestore(&engine->timeline.lock, flags);
> > +}
> > +
> > static void queue_request(struct intel_engine_cs *engine,
> > struct i915_sched_node *node,
> > int prio)
> > @@ -1144,16 +1155,30 @@ static void queue_request(struct intel_engine_cs *engine,
> > &lookup_priolist(engine, prio)->requests);
> > }
> >
> > -static void __submit_queue(struct intel_engine_cs *engine, int prio)
> > +static void __update_queue(struct intel_engine_cs *engine, int prio)
> > {
> > engine->execlists.queue_priority = prio;
> > - tasklet_hi_schedule(&engine->execlists.tasklet);
> > +}
> > +
> > +static void __submit_queue_imm(struct intel_engine_cs *engine)
> > +{
> > + struct intel_engine_execlists * const execlists = &engine->execlists;
> > +
> > + if (reset_in_progress(execlists))
> > + return; /* defer until we restart the engine following reset */
>
> We have a tasklet kick somewhere in that path?
In execlists_reset_finish()
> > + if (execlists->tasklet.func == execlists_submission_tasklet)
> > + __execlists_submission_tasklet(engine);
> > + else
> > + tasklet_hi_schedule(&execlists->tasklet);
> > }
> >
> > static void submit_queue(struct intel_engine_cs *engine, int prio)
> > {
> > - if (prio > engine->execlists.queue_priority)
> > - __submit_queue(engine, prio);
> > + if (prio > engine->execlists.queue_priority) {
> > + __update_queue(engine, prio);
> > + __submit_queue_imm(engine);
> > + }
> > }
> >
> > static void execlists_submit_request(struct i915_request *request)
> > @@ -1165,11 +1190,12 @@ static void execlists_submit_request(struct i915_request *request)
> > spin_lock_irqsave(&engine->timeline.lock, flags);
> >
> > queue_request(engine, &request->sched, rq_prio(request));
> > - submit_queue(engine, rq_prio(request));
> >
> > GEM_BUG_ON(!engine->execlists.first);
> > GEM_BUG_ON(list_empty(&request->sched.link));
> >
> > + submit_queue(engine, rq_prio(request));
> > +
> > spin_unlock_irqrestore(&engine->timeline.lock, flags);
> > }
> >
> > @@ -1296,8 +1322,11 @@ static void execlists_schedule(struct i915_request *request,
> > }
> >
> > if (prio > engine->execlists.queue_priority &&
> > - i915_sw_fence_done(&sched_to_request(node)->submit))
> > - __submit_queue(engine, prio);
> > + i915_sw_fence_done(&sched_to_request(node)->submit)) {
> > + /* defer submission until after all of our updates */
> > + __update_queue(engine, prio);
> > + tasklet_hi_schedule(&engine->execlists.tasklet);
>
> _imm suffix on __submit_queue_imm sounds unused if there is no plain
> __submit_queue, which could have been used here. But meh.
I thought of trying to emphasis the immediate nature of this path. It's
not a huge deal, but I didn't particularly like calling it
direct_submit_queue() (or just direct_submit(), too many submits!)
__ prefix to indicate that it's an inner function to submit_queue,
_imm suffix to indicate what's special.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v7] drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)
2018-06-28 13:28 ` Chris Wilson
@ 2018-06-28 13:59 ` Tvrtko Ursulin
0 siblings, 0 replies; 24+ messages in thread
From: Tvrtko Ursulin @ 2018-06-28 13:59 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 28/06/2018 14:28, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-06-28 14:21:06)
>>
>> On 28/06/2018 14:11, Chris Wilson wrote:
>>> +/*
>>> + * Check the unread Context Status Buffers and manage the submission of new
>>> + * contexts to the ELSP accordingly.
>>> + */
>>> +static void execlists_submission_tasklet(unsigned long data)
>>> +{
>>> + struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
>>> + unsigned long flags;
>>> +
>>> + GEM_TRACE("%s awake?=%d, active=%x\n",
>>> + engine->name,
>>> + engine->i915->gt.awake,
>>> + engine->execlists.active);
>>> +
>>> + spin_lock_irqsave(&engine->timeline.lock, flags);
>>> +
>>> + if (engine->i915->gt.awake) /* we may be delayed until after we idle! */
>>
>> No races between the check and tasklet call? In other words the code
>> path which you were describing that can race is taking the timeline lock?
>
> intel_engine_is_idle() is unserialised.
Okay, think I understand. Ship it!
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
>>> + __execlists_submission_tasklet(engine);
>>> +
>>> + spin_unlock_irqrestore(&engine->timeline.lock, flags);
>>> +}
>>> +
>>> static void queue_request(struct intel_engine_cs *engine,
>>> struct i915_sched_node *node,
>>> int prio)
>>> @@ -1144,16 +1155,30 @@ static void queue_request(struct intel_engine_cs *engine,
>>> &lookup_priolist(engine, prio)->requests);
>>> }
>>>
>>> -static void __submit_queue(struct intel_engine_cs *engine, int prio)
>>> +static void __update_queue(struct intel_engine_cs *engine, int prio)
>>> {
>>> engine->execlists.queue_priority = prio;
>>> - tasklet_hi_schedule(&engine->execlists.tasklet);
>>> +}
>>> +
>>> +static void __submit_queue_imm(struct intel_engine_cs *engine)
>>> +{
>>> + struct intel_engine_execlists * const execlists = &engine->execlists;
>>> +
>>> + if (reset_in_progress(execlists))
>>> + return; /* defer until we restart the engine following reset */
>>
>> We have a tasklet kick somewhere in that path?
>
> In execlists_reset_finish()
>
>>> + if (execlists->tasklet.func == execlists_submission_tasklet)
>>> + __execlists_submission_tasklet(engine);
>>> + else
>>> + tasklet_hi_schedule(&execlists->tasklet);
>>> }
>>>
>>> static void submit_queue(struct intel_engine_cs *engine, int prio)
>>> {
>>> - if (prio > engine->execlists.queue_priority)
>>> - __submit_queue(engine, prio);
>>> + if (prio > engine->execlists.queue_priority) {
>>> + __update_queue(engine, prio);
>>> + __submit_queue_imm(engine);
>>> + }
>>> }
>>>
>>> static void execlists_submit_request(struct i915_request *request)
>>> @@ -1165,11 +1190,12 @@ static void execlists_submit_request(struct i915_request *request)
>>> spin_lock_irqsave(&engine->timeline.lock, flags);
>>>
>>> queue_request(engine, &request->sched, rq_prio(request));
>>> - submit_queue(engine, rq_prio(request));
>>>
>>> GEM_BUG_ON(!engine->execlists.first);
>>> GEM_BUG_ON(list_empty(&request->sched.link));
>>>
>>> + submit_queue(engine, rq_prio(request));
>>> +
>>> spin_unlock_irqrestore(&engine->timeline.lock, flags);
>>> }
>>>
>>> @@ -1296,8 +1322,11 @@ static void execlists_schedule(struct i915_request *request,
>>> }
>>>
>>> if (prio > engine->execlists.queue_priority &&
>>> - i915_sw_fence_done(&sched_to_request(node)->submit))
>>> - __submit_queue(engine, prio);
>>> + i915_sw_fence_done(&sched_to_request(node)->submit)) {
>>> + /* defer submission until after all of our updates */
>>> + __update_queue(engine, prio);
>>> + tasklet_hi_schedule(&engine->execlists.tasklet);
>>
>> _imm suffix on __submit_queue_imm sounds unused if there is no plain
>> __submit_queue, which could have been used here. But meh.
>
> I thought of trying to emphasis the immediate nature of this path. It's
> not a huge deal, but I didn't particularly like calling it
> direct_submit_queue() (or just direct_submit(), too many submits!)
> __ prefix to indicate that it's an inner function to submit_queue,
> _imm suffix to indicate what's special.
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v7] drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)
2018-06-28 12:33 ` [PATCH 9/9] drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd) Chris Wilson
2018-06-28 13:11 ` [PATCH v7] " Chris Wilson
@ 2018-06-28 13:33 ` Chris Wilson
1 sibling, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2018-06-28 13:33 UTC (permalink / raw)
To: intel-gfx
Back in commit 27af5eea54d1 ("drm/i915: Move execlists irq handler to a
bottom half"), we came to the conclusion that running our CSB processing
and ELSP submission from inside the irq handler was a bad idea. A really
bad idea as we could impose nearly 1s latency on other users of the
system, on average! Deferring our work to a tasklet allowed us to do the
processing with irqs enabled, reducing the impact to an average of about
50us.
We have since eradicated the use of forcewaked mmio from inside the CSB
processing and ELSP submission, bringing the impact down to around 5us
(on Kabylake); an order of magnitude better than our measurements 2
years ago on Broadwell and only about 2x worse on average than the
gem_syslatency on an unladen system.
In this iteration of the tasklet-vs-direct submission debate, we seek a
compromise where by we submit new requests immediately to the HW but
defer processing the CS interrupt onto a tasklet. We gain the advantage
of low-latency and ksoftirqd avoidance when waking up the HW, while
avoiding the system-wide starvation of our CS irq-storms.
Comparing the impact on the maximum latency observed (that is the time
stolen from an RT process) over a 120s interval, repeated several times
(using gem_syslatency, similar to RT's cyclictest) while the system is
fully laden with i915 nops, we see that direct submission an actually
improve the worse case.
Maximum latency in microseconds of a third party RT thread
(gem_syslatency -t 120 -f 2)
x Always using tasklets (a couple of >1000us outliers removed)
+ Only using tasklets from CS irq, direct submission of requests
+------------------------------------------------------------------------+
| + |
| + |
| + |
| + + |
| + + + |
| + + + + x x x |
| +++ + + + x x x x x x |
| +++ + ++ + + *x x x x x x |
| +++ + ++ + * *x x * x x x |
| + +++ + ++ * * +*xxx * x x xx |
| * +++ + ++++* *x+**xx+ * x x xxxx x |
| **x++++*++**+*x*x****x+ * +x xx xxxx x x |
|x* ******+***************++*+***xxxxxx* xx*x xxx + x+|
| |__________MA___________| |
| |______M__A________| |
+------------------------------------------------------------------------+
N Min Max Median Avg Stddev
x 118 91 186 124 125.28814 16.279137
+ 120 92 187 109 112.00833 13.458617
Difference at 95.0% confidence
-13.2798 +/- 3.79219
-10.5994% +/- 3.02677%
(Student's t, pooled s = 14.9237)
However the mean latency is adversely affected:
Mean latency in microseconds of a third party RT thread
(gem_syslatency -t 120 -f 1)
x Always using tasklets
+ Only using tasklets from CS irq, direct submission of requests
+------------------------------------------------------------------------+
| xxxxxx + ++ |
| xxxxxx + ++ |
| xxxxxx + +++ ++ |
| xxxxxxx +++++ ++ |
| xxxxxxx +++++ ++ |
| xxxxxxx +++++ +++ |
| xxxxxxx + ++++++++++ |
| xxxxxxxx ++ ++++++++++ |
| xxxxxxxx ++ ++++++++++ |
| xxxxxxxxxx +++++++++++++++ |
| xxxxxxxxxxx x +++++++++++++++ |
|x xxxxxxxxxxxxx x + + ++++++++++++++++++ +|
| |__A__| |
| |____A___| |
+------------------------------------------------------------------------+
N Min Max Median Avg Stddev
x 120 3.506 3.727 3.631 3.6321417 0.02773109
+ 120 3.834 4.149 4.039 4.0375167 0.041221676
Difference at 95.0% confidence
0.405375 +/- 0.00888913
11.1608% +/- 0.244735%
(Student's t, pooled s = 0.03513)
However, since the mean latency corresponds to the amount of irqsoff
processing we have to do for a CS interrupt, we only need to speed that
up to benefit not just system latency but our own throughput.
v2: Remember to defer submissions when under reset.
v4: Only use direct submission for new requests
v5: Be aware that with mixing direct tasklet evaluation and deferred
tasklets, we may end up idling before running the deferred tasklet.
v6: Remove the redudant likely() from tasklet_is_enabled(), restrict the
annotation to reset_in_progress().
v7: Take the full timeline.lock when enabling perf_pmu stats as the
tasklet is no longer a valid guard. A consequence is that the stats are
now only valid for engines also using the timeline.lock to process
state.
Testcase: igt/gem_exec_latency/*rthog*
References: 27af5eea54d1 ("drm/i915: Move execlists irq handler to a bottom half")
Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
Compile!
---
drivers/gpu/drm/i915/i915_gem.h | 5 ++
drivers/gpu/drm/i915/intel_engine_cs.c | 8 +-
drivers/gpu/drm/i915/intel_lrc.c | 102 ++++++++++++++++---------
3 files changed, 75 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
index 261da577829a..e46592956872 100644
--- a/drivers/gpu/drm/i915/i915_gem.h
+++ b/drivers/gpu/drm/i915/i915_gem.h
@@ -88,4 +88,9 @@ static inline void __tasklet_enable_sync_once(struct tasklet_struct *t)
tasklet_kill(t);
}
+static inline bool __tasklet_is_enabled(const struct tasklet_struct *t)
+{
+ return !atomic_read(&t->count);
+}
+
#endif /* __I915_GEM_H__ */
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index ace93958689e..01862884a436 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1619,8 +1619,8 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
if (!intel_engine_supports_stats(engine))
return -ENODEV;
- tasklet_disable(&execlists->tasklet);
- write_seqlock_irqsave(&engine->stats.lock, flags);
+ spin_lock_irqsave(&engine->timeline.lock, flags);
+ write_seqlock(&engine->stats.lock);
if (unlikely(engine->stats.enabled == ~0)) {
err = -EBUSY;
@@ -1644,8 +1644,8 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
}
unlock:
- write_sequnlock_irqrestore(&engine->stats.lock, flags);
- tasklet_enable(&execlists->tasklet);
+ write_sequnlock(&engine->stats.lock);
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
return err;
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 7f8b29684d9d..ab89dabc2965 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -563,12 +563,14 @@ static void complete_preempt_context(struct intel_engine_execlists *execlists)
GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT));
execlists_cancel_port_requests(execlists);
- execlists_unwind_incomplete_requests(execlists);
+ __unwind_incomplete_requests(container_of(execlists,
+ struct intel_engine_cs,
+ execlists));
execlists_clear_active(execlists, EXECLISTS_ACTIVE_PREEMPT);
}
-static void __execlists_dequeue(struct intel_engine_cs *engine)
+static void execlists_dequeue(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
struct execlist_port *port = execlists->port;
@@ -578,9 +580,8 @@ static void __execlists_dequeue(struct intel_engine_cs *engine)
struct rb_node *rb;
bool submit = false;
- lockdep_assert_held(&engine->timeline.lock);
-
- /* Hardware submission is through 2 ports. Conceptually each port
+ /*
+ * Hardware submission is through 2 ports. Conceptually each port
* has a (RING_START, RING_HEAD, RING_TAIL) tuple. RING_START is
* static for a context, and unique to each, so we only execute
* requests belonging to a single context from each ring. RING_HEAD
@@ -770,15 +771,6 @@ static void __execlists_dequeue(struct intel_engine_cs *engine)
!port_isset(engine->execlists.port));
}
-static void execlists_dequeue(struct intel_engine_cs *engine)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&engine->timeline.lock, flags);
- __execlists_dequeue(engine);
- spin_unlock_irqrestore(&engine->timeline.lock, flags);
-}
-
void
execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
{
@@ -957,6 +949,12 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
spin_unlock_irqrestore(&engine->timeline.lock, flags);
}
+static inline bool
+reset_in_progress(const struct intel_engine_execlists *execlists)
+{
+ return unlikely(!__tasklet_is_enabled(&execlists->tasklet));
+}
+
static void process_csb(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
@@ -1108,18 +1106,9 @@ static void process_csb(struct intel_engine_cs *engine)
execlists->csb_head = head;
}
-/*
- * Check the unread Context Status Buffers and manage the submission of new
- * contexts to the ELSP accordingly.
- */
-static void execlists_submission_tasklet(unsigned long data)
+static void __execlists_submission_tasklet(struct intel_engine_cs *const engine)
{
- struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
-
- GEM_TRACE("%s awake?=%d, active=%x\n",
- engine->name,
- engine->i915->gt.awake,
- engine->execlists.active);
+ lockdep_assert_held(&engine->timeline.lock);
/*
* We can skip acquiring intel_runtime_pm_get() here as it was taken
@@ -1136,6 +1125,28 @@ static void execlists_submission_tasklet(unsigned long data)
execlists_dequeue(engine);
}
+/*
+ * Check the unread Context Status Buffers and manage the submission of new
+ * contexts to the ELSP accordingly.
+ */
+static void execlists_submission_tasklet(unsigned long data)
+{
+ struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
+ unsigned long flags;
+
+ GEM_TRACE("%s awake?=%d, active=%x\n",
+ engine->name,
+ engine->i915->gt.awake,
+ engine->execlists.active);
+
+ spin_lock_irqsave(&engine->timeline.lock, flags);
+
+ if (engine->i915->gt.awake) /* we may be delayed until after we idle! */
+ __execlists_submission_tasklet(engine);
+
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
+}
+
static void queue_request(struct intel_engine_cs *engine,
struct i915_sched_node *node,
int prio)
@@ -1144,16 +1155,30 @@ static void queue_request(struct intel_engine_cs *engine,
&lookup_priolist(engine, prio)->requests);
}
-static void __submit_queue(struct intel_engine_cs *engine, int prio)
+static void __update_queue(struct intel_engine_cs *engine, int prio)
{
engine->execlists.queue_priority = prio;
- tasklet_hi_schedule(&engine->execlists.tasklet);
+}
+
+static void __submit_queue_imm(struct intel_engine_cs *engine)
+{
+ struct intel_engine_execlists * const execlists = &engine->execlists;
+
+ if (reset_in_progress(execlists))
+ return; /* defer until we restart the engine following reset */
+
+ if (execlists->tasklet.func == execlists_submission_tasklet)
+ __execlists_submission_tasklet(engine);
+ else
+ tasklet_hi_schedule(&execlists->tasklet);
}
static void submit_queue(struct intel_engine_cs *engine, int prio)
{
- if (prio > engine->execlists.queue_priority)
- __submit_queue(engine, prio);
+ if (prio > engine->execlists.queue_priority) {
+ __update_queue(engine, prio);
+ __submit_queue_imm(engine);
+ }
}
static void execlists_submit_request(struct i915_request *request)
@@ -1165,11 +1190,12 @@ static void execlists_submit_request(struct i915_request *request)
spin_lock_irqsave(&engine->timeline.lock, flags);
queue_request(engine, &request->sched, rq_prio(request));
- submit_queue(engine, rq_prio(request));
GEM_BUG_ON(!engine->execlists.first);
GEM_BUG_ON(list_empty(&request->sched.link));
+ submit_queue(engine, rq_prio(request));
+
spin_unlock_irqrestore(&engine->timeline.lock, flags);
}
@@ -1296,8 +1322,11 @@ static void execlists_schedule(struct i915_request *request,
}
if (prio > engine->execlists.queue_priority &&
- i915_sw_fence_done(&sched_to_request(node)->submit))
- __submit_queue(engine, prio);
+ i915_sw_fence_done(&sched_to_request(node)->submit)) {
+ /* defer submission until after all of our updates */
+ __update_queue(engine, prio);
+ tasklet_hi_schedule(&engine->execlists.tasklet);
+ }
}
spin_unlock_irq(&engine->timeline.lock);
@@ -1878,6 +1907,7 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
struct i915_request *request, *active;
+ unsigned long flags;
GEM_TRACE("%s\n", engine->name);
@@ -1892,6 +1922,8 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
*/
__tasklet_disable_sync_once(&execlists->tasklet);
+ spin_lock_irqsave(&engine->timeline.lock, flags);
+
/*
* We want to flush the pending context switches, having disabled
* the tasklet above, we can assume exclusive access to the execlists.
@@ -1909,15 +1941,12 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
active = NULL;
request = port_request(execlists->port);
if (request) {
- unsigned long flags;
-
/*
* Prevent the breadcrumb from advancing before we decide
* which request is currently active.
*/
intel_engine_stop_cs(engine);
- spin_lock_irqsave(&engine->timeline.lock, flags);
list_for_each_entry_from_reverse(request,
&engine->timeline.requests,
link) {
@@ -1927,9 +1956,10 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
active = request;
}
- spin_unlock_irqrestore(&engine->timeline.lock, flags);
}
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
+
return active;
}
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
` (7 preceding siblings ...)
2018-06-28 12:33 ` [PATCH 9/9] drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd) Chris Wilson
@ 2018-06-28 12:50 ` Patchwork
2018-06-28 13:05 ` ✓ Fi.CI.BAT: success " Patchwork
` (5 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2018-06-28 12:50 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts
URL : https://patchwork.freedesktop.org/series/45574/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
c25ca2aa7052 drm/i915: Drop posting reads to flush master interrupts
72c874d9629c drm/i915/execlists: Pull submit after dequeue under timeline lock
23551321d1f8 drm/i915/execlists: Pull CSB reset under the timeline.lock
339457d39e59 drm/i915/execlists: Process one CSB update at a time
-:68: WARNING:MEMORY_BARRIER: memory barrier without comment
#68: FILE: drivers/gpu/drm/i915/intel_lrc.c:966:
+ smp_mb__after_atomic();
-:114: WARNING:LONG_LINE: line over 100 characters
#114: FILE: drivers/gpu/drm/i915/intel_lrc.c:990:
+ head, GEN8_CSB_READ_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?",
-:115: WARNING:LONG_LINE: line over 100 characters
#115: FILE: drivers/gpu/drm/i915/intel_lrc.c:991:
+ tail, GEN8_CSB_WRITE_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?");
-:183: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#183: FILE: drivers/gpu/drm/i915/intel_lrc.c:1022:
+ status, buf[2*head + 1],
^
-:211: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#211: FILE: drivers/gpu/drm/i915/intel_lrc.c:1040:
+ buf[2*head + 1] == execlists->preempt_complete_status) {
^
total: 0 errors, 3 warnings, 2 checks, 316 lines checked
4f9269b804df drm/i915/execlists: Unify CSB access pointers
851d680bba4a drm/i915/execlists: Reset CSB write pointer after reset
258f82686b87 drm/i915/execlists: Stop storing the CSB read pointer in the mmio register
-:20: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#20:
References: cc53699b25b5 ("drm/i915: Use masked write for Context Status Buffer Pointer")
-:20: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit cc53699b25b5 ("drm/i915: Use masked write for Context Status Buffer Pointer")'
#20:
References: cc53699b25b5 ("drm/i915: Use masked write for Context Status Buffer Pointer")
total: 1 errors, 1 warnings, 0 checks, 8 lines checked
1b995c4ffd64 drm/i915/execlists: Trust the CSB
30fd3a0d176d drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)
-:104: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#104:
References: 27af5eea54d1 ("drm/i915: Move execlists irq handler to a bottom half")
-:104: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 27af5eea54d1 ("drm/i915: Move execlists irq handler to a bottom half")'
#104:
References: 27af5eea54d1 ("drm/i915: Move execlists irq handler to a bottom half")
total: 1 errors, 1 warnings, 0 checks, 219 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread* ✓ Fi.CI.BAT: success for series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
` (8 preceding siblings ...)
2018-06-28 12:50 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts Patchwork
@ 2018-06-28 13:05 ` Patchwork
2018-06-28 13:06 ` [PATCH 1/9] " Ville Syrjälä
` (4 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2018-06-28 13:05 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts
URL : https://patchwork.freedesktop.org/series/45574/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4397 -> Patchwork_9466 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/45574/revisions/1/mbox/
== Known issues ==
Here are the changes found in Patchwork_9466 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@debugfs_test@read_all_entries:
fi-snb-2520m: PASS -> INCOMPLETE (fdo#103713)
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
fi-bxt-dsi: PASS -> INCOMPLETE (fdo#103927)
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
== Participating hosts (44 -> 39) ==
Missing (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u
== Build changes ==
* Linux: CI_DRM_4397 -> Patchwork_9466
CI_DRM_4397: 7306233935b0e426454e8adcf09a8022faa03cbc @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9466: 30fd3a0d176db02ee1262d490acc80892eaa93e6 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
30fd3a0d176d drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)
1b995c4ffd64 drm/i915/execlists: Trust the CSB
258f82686b87 drm/i915/execlists: Stop storing the CSB read pointer in the mmio register
851d680bba4a drm/i915/execlists: Reset CSB write pointer after reset
4f9269b804df drm/i915/execlists: Unify CSB access pointers
339457d39e59 drm/i915/execlists: Process one CSB update at a time
23551321d1f8 drm/i915/execlists: Pull CSB reset under the timeline.lock
72c874d9629c drm/i915/execlists: Pull submit after dequeue under timeline lock
c25ca2aa7052 drm/i915: Drop posting reads to flush master interrupts
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9466/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
` (9 preceding siblings ...)
2018-06-28 13:05 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-06-28 13:06 ` Ville Syrjälä
2018-06-28 13:11 ` Chris Wilson
2018-06-28 13:28 ` ✗ Fi.CI.BAT: failure for series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts (rev2) Patchwork
` (3 subsequent siblings)
14 siblings, 1 reply; 24+ messages in thread
From: Ville Syrjälä @ 2018-06-28 13:06 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Thu, Jun 28, 2018 at 01:33:43PM +0100, Chris Wilson wrote:
> We do not need to do a posting read of our uncached mmio write to
> re-enable the master interrupt lines after handling an interrupt, so
> don't. This saves us a slow UC read before we can process the interrupt,
> most noticeable in execlists where any stalls imposes extra latency on
> GPU command execution.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 7a7c4a2bd778..e83fcedcbf1d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2168,7 +2168,6 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>
> I915_WRITE(VLV_IER, ier);
> I915_WRITE(VLV_MASTER_IER, MASTER_INTERRUPT_ENABLE);
> - POSTING_READ(VLV_MASTER_IER);
>
> if (gt_iir)
> snb_gt_irq_handler(dev_priv, gt_iir);
> @@ -2253,7 +2252,6 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>
> I915_WRITE(VLV_IER, ier);
> I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> - POSTING_READ(GEN8_MASTER_IRQ);
>
> gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
>
> @@ -2622,7 +2620,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> /* disable master interrupt before clearing iir */
> de_ier = I915_READ(DEIER);
> I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> - POSTING_READ(DEIER);
>
> /* Disable south interrupts. We'll only write to SDEIIR once, so further
> * interrupts will will be stored on its back queue, and then we'll be
> @@ -2632,7 +2629,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> if (!HAS_PCH_NOP(dev_priv)) {
> sde_ier = I915_READ(SDEIER);
> I915_WRITE(SDEIER, 0);
> - POSTING_READ(SDEIER);
> }
>
> /* Find, clear, then process each source of interrupt */
> @@ -2667,11 +2663,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> }
>
> I915_WRITE(DEIER, de_ier);
> - POSTING_READ(DEIER);
> - if (!HAS_PCH_NOP(dev_priv)) {
> + if (!HAS_PCH_NOP(dev_priv))
> I915_WRITE(SDEIER, sde_ier);
> - POSTING_READ(SDEIER);
> - }
Not 100% sure about the SDEIER thing. I believe it goes over the link
to the PCH so I suppose it might be posted. No idea how it would get
ordered wrt. subsequent accesses to CPU side registers. Hmm. Oh we
actually restore master first, then SDEIER. But I suppose the order
doesn't actually matter. As long as SDEIER is 0 at some point we should
get a new edge in the end if SDEIIR still has something asserted.
Yeah, seems fine by me:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> /* IRQs are synced during runtime_suspend, we don't require a wakeref */
> enable_rpm_wakeref_asserts(dev_priv);
> --
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts
2018-06-28 13:06 ` [PATCH 1/9] " Ville Syrjälä
@ 2018-06-28 13:11 ` Chris Wilson
0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2018-06-28 13:11 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Quoting Ville Syrjälä (2018-06-28 14:06:40)
> On Thu, Jun 28, 2018 at 01:33:43PM +0100, Chris Wilson wrote:
> > We do not need to do a posting read of our uncached mmio write to
> > re-enable the master interrupt lines after handling an interrupt, so
> > don't. This saves us a slow UC read before we can process the interrupt,
> > most noticeable in execlists where any stalls imposes extra latency on
> > GPU command execution.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 9 +--------
> > 1 file changed, 1 insertion(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 7a7c4a2bd778..e83fcedcbf1d 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2168,7 +2168,6 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> >
> > I915_WRITE(VLV_IER, ier);
> > I915_WRITE(VLV_MASTER_IER, MASTER_INTERRUPT_ENABLE);
> > - POSTING_READ(VLV_MASTER_IER);
> >
> > if (gt_iir)
> > snb_gt_irq_handler(dev_priv, gt_iir);
> > @@ -2253,7 +2252,6 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
> >
> > I915_WRITE(VLV_IER, ier);
> > I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> > - POSTING_READ(GEN8_MASTER_IRQ);
> >
> > gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
> >
> > @@ -2622,7 +2620,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> > /* disable master interrupt before clearing iir */
> > de_ier = I915_READ(DEIER);
> > I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> > - POSTING_READ(DEIER);
> >
> > /* Disable south interrupts. We'll only write to SDEIIR once, so further
> > * interrupts will will be stored on its back queue, and then we'll be
> > @@ -2632,7 +2629,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> > if (!HAS_PCH_NOP(dev_priv)) {
> > sde_ier = I915_READ(SDEIER);
> > I915_WRITE(SDEIER, 0);
> > - POSTING_READ(SDEIER);
> > }
> >
> > /* Find, clear, then process each source of interrupt */
> > @@ -2667,11 +2663,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> > }
> >
> > I915_WRITE(DEIER, de_ier);
> > - POSTING_READ(DEIER);
> > - if (!HAS_PCH_NOP(dev_priv)) {
> > + if (!HAS_PCH_NOP(dev_priv))
> > I915_WRITE(SDEIER, sde_ier);
> > - POSTING_READ(SDEIER);
> > - }
>
> Not 100% sure about the SDEIER thing. I believe it goes over the link
> to the PCH so I suppose it might be posted. No idea how it would get
> ordered wrt. subsequent accesses to CPU side registers. Hmm. Oh we
> actually restore master first, then SDEIER. But I suppose the order
> doesn't actually matter. As long as SDEIER is 0 at some point we should
> get a new edge in the end if SDEIIR still has something asserted.
I have vague memories of you saying "oh master is done before sdeier",
that's odd (or that might have been me)!
> Yeah, seems fine by me:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Ta,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts (rev2)
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
` (10 preceding siblings ...)
2018-06-28 13:06 ` [PATCH 1/9] " Ville Syrjälä
@ 2018-06-28 13:28 ` Patchwork
2018-06-28 14:12 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts (rev3) Patchwork
` (2 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2018-06-28 13:28 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts (rev2)
URL : https://patchwork.freedesktop.org/series/45574/
State : failure
== Summary ==
CALL scripts/checksyscalls.sh
DESCEND objtool
CHK include/generated/compile.h
CC [M] drivers/gpu/drm/i915/intel_engine_cs.o
drivers/gpu/drm/i915/intel_engine_cs.c: In function ‘intel_enable_engine_stats’:
drivers/gpu/drm/i915/intel_engine_cs.c:1623:2: error: too many arguments to function ‘write_seqlock’
write_seqlock(&engine->stats.lock, flags);
^~~~~~~~~~~~~
In file included from ./include/linux/dcache.h:10:0,
from ./include/linux/fs.h:8,
from ./include/linux/seq_file.h:11,
from ./include/drm/drm_print.h:31,
from drivers/gpu/drm/i915/intel_engine_cs.c:25:
./include/linux/seqlock.h:446:20: note: declared here
static inline void write_seqlock(seqlock_t *sl)
^~~~~~~~~~~~~
drivers/gpu/drm/i915/intel_engine_cs.c:1648:2: error: implicit declaration of function ‘spin_lock_irqrestore’; did you mean ‘spin_unlock_irqrestore’? [-Werror=implicit-function-declaration]
spin_lock_irqrestore(&engine->timeline.lock, flags);
^~~~~~~~~~~~~~~~~~~~
spin_unlock_irqrestore
cc1: all warnings being treated as errors
scripts/Makefile.build:317: recipe for target 'drivers/gpu/drm/i915/intel_engine_cs.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_engine_cs.o] Error 1
scripts/Makefile.build:558: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:558: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:558: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1034: recipe for target 'drivers' failed
make: *** [drivers] Error 2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts (rev3)
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
` (11 preceding siblings ...)
2018-06-28 13:28 ` ✗ Fi.CI.BAT: failure for series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts (rev2) Patchwork
@ 2018-06-28 14:12 ` Patchwork
2018-06-28 14:28 ` ✓ Fi.CI.BAT: success " Patchwork
2018-06-28 16:58 ` ✓ Fi.CI.IGT: " Patchwork
14 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2018-06-28 14:12 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts (rev3)
URL : https://patchwork.freedesktop.org/series/45574/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
491ac3bf222c drm/i915: Drop posting reads to flush master interrupts
73285bf1665a drm/i915/execlists: Pull submit after dequeue under timeline lock
77c034e96fb7 drm/i915/execlists: Pull CSB reset under the timeline.lock
03bb94e546bb drm/i915/execlists: Process one CSB update at a time
-:68: WARNING:MEMORY_BARRIER: memory barrier without comment
#68: FILE: drivers/gpu/drm/i915/intel_lrc.c:966:
+ smp_mb__after_atomic();
-:114: WARNING:LONG_LINE: line over 100 characters
#114: FILE: drivers/gpu/drm/i915/intel_lrc.c:990:
+ head, GEN8_CSB_READ_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?",
-:115: WARNING:LONG_LINE: line over 100 characters
#115: FILE: drivers/gpu/drm/i915/intel_lrc.c:991:
+ tail, GEN8_CSB_WRITE_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?");
-:183: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#183: FILE: drivers/gpu/drm/i915/intel_lrc.c:1022:
+ status, buf[2*head + 1],
^
-:211: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#211: FILE: drivers/gpu/drm/i915/intel_lrc.c:1040:
+ buf[2*head + 1] == execlists->preempt_complete_status) {
^
total: 0 errors, 3 warnings, 2 checks, 316 lines checked
cce3201c3696 drm/i915/execlists: Unify CSB access pointers
8df8ba80d786 drm/i915/execlists: Reset CSB write pointer after reset
6031de27159c drm/i915/execlists: Stop storing the CSB read pointer in the mmio register
-:20: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#20:
References: cc53699b25b5 ("drm/i915: Use masked write for Context Status Buffer Pointer")
-:20: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit cc53699b25b5 ("drm/i915: Use masked write for Context Status Buffer Pointer")'
#20:
References: cc53699b25b5 ("drm/i915: Use masked write for Context Status Buffer Pointer")
total: 1 errors, 1 warnings, 0 checks, 8 lines checked
6d20c065cb63 drm/i915/execlists: Trust the CSB
7eda28bdaa02 drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)
-:108: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#108:
References: 27af5eea54d1 ("drm/i915: Move execlists irq handler to a bottom half")
-:108: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 27af5eea54d1 ("drm/i915: Move execlists irq handler to a bottom half")'
#108:
References: 27af5eea54d1 ("drm/i915: Move execlists irq handler to a bottom half")
total: 1 errors, 1 warnings, 0 checks, 232 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread* ✓ Fi.CI.BAT: success for series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts (rev3)
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
` (12 preceding siblings ...)
2018-06-28 14:12 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts (rev3) Patchwork
@ 2018-06-28 14:28 ` Patchwork
2018-06-28 16:58 ` ✓ Fi.CI.IGT: " Patchwork
14 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2018-06-28 14:28 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts (rev3)
URL : https://patchwork.freedesktop.org/series/45574/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4397 -> Patchwork_9471 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/45574/revisions/3/mbox/
== Known issues ==
Here are the changes found in Patchwork_9471 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_exec_gttfill@basic:
fi-byt-n2820: PASS -> FAIL (fdo#106744)
fdo#106744 https://bugs.freedesktop.org/show_bug.cgi?id=106744
== Participating hosts (44 -> 39) ==
Missing (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u
== Build changes ==
* Linux: CI_DRM_4397 -> Patchwork_9471
CI_DRM_4397: 7306233935b0e426454e8adcf09a8022faa03cbc @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9471: 7eda28bdaa0276e29d1c664e9242e2f24a8aa643 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
7eda28bdaa02 drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)
6d20c065cb63 drm/i915/execlists: Trust the CSB
6031de27159c drm/i915/execlists: Stop storing the CSB read pointer in the mmio register
8df8ba80d786 drm/i915/execlists: Reset CSB write pointer after reset
cce3201c3696 drm/i915/execlists: Unify CSB access pointers
03bb94e546bb drm/i915/execlists: Process one CSB update at a time
77c034e96fb7 drm/i915/execlists: Pull CSB reset under the timeline.lock
73285bf1665a drm/i915/execlists: Pull submit after dequeue under timeline lock
491ac3bf222c drm/i915: Drop posting reads to flush master interrupts
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9471/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread* ✓ Fi.CI.IGT: success for series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts (rev3)
2018-06-28 12:33 [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts Chris Wilson
` (13 preceding siblings ...)
2018-06-28 14:28 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-06-28 16:58 ` Patchwork
14 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2018-06-28 16:58 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/9] drm/i915: Drop posting reads to flush master interrupts (rev3)
URL : https://patchwork.freedesktop.org/series/45574/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4397_full -> Patchwork_9471_full =
== Summary - WARNING ==
Minor unknown changes coming with Patchwork_9471_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_9471_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_9471_full:
=== IGT changes ===
==== Warnings ====
igt@gem_linear_blits@interruptible:
shard-apl: SKIP -> PASS
== Known issues ==
Here are the changes found in Patchwork_9471_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@drv_selftest@live_hangcheck:
shard-apl: PASS -> DMESG-FAIL (fdo#106947, fdo#106560)
igt@gem_exec_schedule@pi-ringfull-blt:
{shard-glk9}: NOTRUN -> FAIL (fdo#103158)
igt@gem_mmap_gtt@coherency:
shard-glk: NOTRUN -> FAIL (fdo#100587)
igt@gem_softpin@noreloc-s3:
shard-kbl: NOTRUN -> INCOMPLETE (fdo#103665)
igt@kms_flip@2x-plain-flip-fb-recreate:
shard-glk: PASS -> FAIL (fdo#100368)
igt@kms_flip_tiling@flip-y-tiled:
{shard-glk9}: NOTRUN -> FAIL (fdo#104724)
igt@kms_setmode@basic:
shard-kbl: PASS -> FAIL (fdo#99912)
igt@perf@blocking:
shard-hsw: PASS -> FAIL (fdo#102252)
igt@perf_pmu@busy-accuracy-98-vcs1:
shard-snb: NOTRUN -> INCOMPLETE (fdo#105411)
==== Possible fixes ====
igt@gem_exec_schedule@preemptive-hang-render:
shard-snb: INCOMPLETE (fdo#105411) -> SKIP
igt@kms_flip@2x-flip-vs-expired-vblank:
shard-glk: FAIL (fdo#105363) -> PASS
igt@kms_flip_tiling@flip-to-x-tiled:
shard-glk: FAIL (fdo#104724, fdo#103822) -> PASS
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#100587 https://bugs.freedesktop.org/show_bug.cgi?id=100587
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
== Participating hosts (6 -> 6) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_4397 -> Patchwork_9471
CI_DRM_4397: 7306233935b0e426454e8adcf09a8022faa03cbc @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9471: 7eda28bdaa0276e29d1c664e9242e2f24a8aa643 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9471/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread