* [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
@ 2017-10-31 15:27 Mika Kuoppala
2017-10-31 15:27 ` [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array Mika Kuoppala
` (3 more replies)
0 siblings, 4 replies; 25+ messages in thread
From: Mika Kuoppala @ 2017-10-31 15:27 UTC (permalink / raw)
To: intel-gfx
From: Mika Kuoppala <mika.kuoppala@intel.com>
Instead of trusting that first available port is at index 0,
use accessor to hide this. This is a preparation for a
following patches where head can be at arbitrary location
in the port array.
v2: improved commit message, elsp_ready readability (Chris)
v3: s/execlist_port_index/execlist_port (Chris)
v4: rebase to new naming
v5: fix port_next indexing
v6: adapt to preempt
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gpu_error.c | 6 ++--
drivers/gpu/drm/i915/i915_guc_submission.c | 50 ++++++++++++++++++------------
drivers/gpu/drm/i915/intel_engine_cs.c | 18 ++++++-----
drivers/gpu/drm/i915/intel_lrc.c | 49 ++++++++++++++++++-----------
drivers/gpu/drm/i915/intel_ringbuffer.h | 44 ++++++++++++++++++++++++--
5 files changed, 119 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 653fb69e7ecb..6d0bdb03b3f0 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1333,11 +1333,13 @@ static void engine_record_requests(struct intel_engine_cs *engine,
static void error_record_engine_execlists(struct intel_engine_cs *engine,
struct drm_i915_error_engine *ee)
{
- const struct intel_engine_execlists * const execlists = &engine->execlists;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
unsigned int n;
for (n = 0; n < execlists_num_ports(execlists); n++) {
- struct drm_i915_gem_request *rq = port_request(&execlists->port[n]);
+ struct drm_i915_gem_request *rq;
+
+ rq = port_request(execlists_port(execlists, n));
if (!rq)
break;
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 3049a0781b88..4600d0878c96 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -678,16 +678,18 @@ static void i915_guc_submit(struct intel_engine_cs *engine)
{
struct intel_guc *guc = &engine->i915->guc;
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port *port = execlists->port;
unsigned int n;
for (n = 0; n < execlists_num_ports(execlists); n++) {
+ struct execlist_port *port;
struct drm_i915_gem_request *rq;
unsigned int count;
- rq = port_unpack(&port[n], &count);
+ port = execlists_port(execlists, n);
+ rq = port_unpack(port, &count);
+
if (rq && count == 0) {
- port_set(&port[n], port_pack(rq, ++count));
+ port_set(port, port_pack(rq, ++count));
flush_ggtt_writes(rq->ring->vma);
@@ -710,10 +712,8 @@ static void port_assign(struct execlist_port *port,
static void i915_guc_dequeue(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port *port = execlists->port;
+ struct execlist_port *port, *last_port;
struct drm_i915_gem_request *last = NULL;
- const struct execlist_port * const last_port =
- &execlists->port[execlists->port_mask];
bool submit = false;
struct rb_node *rb;
@@ -724,6 +724,9 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
if (!rb)
goto unlock;
+ port = execlists_port_head(execlists);
+ last_port = execlists_port_tail(execlists);
+
if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) && port_isset(port)) {
struct guc_preempt_work *preempt_work =
&engine->i915->guc.preempt_work[engine->id];
@@ -739,7 +742,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
goto unlock;
}
- port++;
+ port = execlists_port_next(execlists, port);
}
do {
@@ -756,7 +759,8 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
if (submit)
port_assign(port, last);
- port++;
+
+ port = execlists_port_next(execlists, port);
}
INIT_LIST_HEAD(&rq->priotree.link);
@@ -784,24 +788,32 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
spin_unlock_irq(&engine->timeline->lock);
}
-static void i915_guc_irq_handler(unsigned long data)
+static void guc_complete_ready_ports(struct intel_engine_execlists * const execlists)
{
- struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
- struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port *port = execlists->port;
- struct drm_i915_gem_request *rq;
+ struct execlist_port *port = execlists_port_head(execlists);
+
+ while (port_isset(port)) {
+ struct drm_i915_gem_request *rq = port_request(port);
+
+ if (!i915_gem_request_completed(rq))
+ break;
- rq = port_request(&port[0]);
- while (rq && i915_gem_request_completed(rq)) {
trace_i915_gem_request_out(rq);
i915_gem_request_put(rq);
- execlists_port_complete(execlists, port);
+ port = execlists_head_complete(execlists, port);
+ };
- rq = port_request(&port[0]);
- }
- if (!rq)
+ if (!port_isset(port))
execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER);
+}
+
+static void i915_guc_irq_handler(unsigned long data)
+{
+ struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
+
+ guc_complete_ready_ports(execlists);
if (execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT) &&
intel_read_status_page(engine, I915_GEM_HWS_PREEMPT_INDEX) ==
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index f31f2d6384c3..1c08185a05ed 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1673,7 +1673,7 @@ static void print_request(struct drm_printer *m,
void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m)
{
struct intel_breadcrumbs * const b = &engine->breadcrumbs;
- const struct intel_engine_execlists * const execlists = &engine->execlists;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
struct i915_gpu_error * const error = &engine->i915->gpu_error;
struct drm_i915_private *dev_priv = engine->i915;
struct drm_i915_gem_request *rq;
@@ -1777,16 +1777,20 @@ void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m)
rcu_read_lock();
for (idx = 0; idx < execlists_num_ports(execlists); idx++) {
- unsigned int count;
+ struct execlist_port *port;
+ unsigned int count, idx_abs;
+
+ port = execlists_port(execlists, idx);
+ idx_abs = port_index(port, execlists);
- rq = port_unpack(&execlists->port[idx], &count);
+ rq = port_unpack(port, &count);
if (rq) {
- drm_printf(m, "\t\tELSP[%d] count=%d, ",
- idx, count);
+ drm_printf(m, "\t\tELSP[%d:%d] count=%d, ",
+ idx, idx_abs, count);
print_request(m, rq, "rq: ");
} else {
- drm_printf(m, "\t\tELSP[%d] idle\n",
- idx);
+ drm_printf(m, "\t\tELSP[%d:%d] idle\n",
+ idx, idx_abs);
}
}
drm_printf(m, "\t\tHW active? 0x%x\n", execlists->active);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6840ec8db037..62c3e06a110d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -448,24 +448,26 @@ static inline void elsp_write(u64 desc, u32 __iomem *elsp)
static void execlists_submit_ports(struct intel_engine_cs *engine)
{
- struct execlist_port *port = engine->execlists.port;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
u32 __iomem *elsp =
engine->i915->regs + i915_mmio_reg_offset(RING_ELSP(engine));
unsigned int n;
- for (n = execlists_num_ports(&engine->execlists); n--; ) {
+ for (n = execlists_num_ports(execlists); n--; ) {
+ struct execlist_port *port;
struct drm_i915_gem_request *rq;
unsigned int count;
u64 desc;
- rq = port_unpack(&port[n], &count);
+ port = execlists_port(execlists, n);
+ rq = port_unpack(port, &count);
if (rq) {
GEM_BUG_ON(count > !n);
if (!count++)
execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
- port_set(&port[n], port_pack(rq, count));
+ port_set(port, port_pack(rq, count));
desc = execlists_update_context(rq);
- GEM_DEBUG_EXEC(port[n].context_id = upper_32_bits(desc));
+ GEM_DEBUG_EXEC(port->context_id = upper_32_bits(desc));
} else {
GEM_BUG_ON(!n);
desc = 0;
@@ -529,10 +531,8 @@ static void inject_preempt_context(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;
- const struct execlist_port * const last_port =
- &execlists->port[execlists->port_mask];
- struct drm_i915_gem_request *last = port_request(port);
+ struct execlist_port *port, *last_port;
+ struct drm_i915_gem_request *last;
struct rb_node *rb;
bool submit = false;
@@ -563,6 +563,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
if (!rb)
goto unlock;
+ port = execlists_port_head(execlists);
+ last = port_request(port);
+
if (last) {
/*
* Don't resubmit or switch until all outstanding
@@ -570,7 +573,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
* know the next preemption status we see corresponds
* to this ELSP update.
*/
- if (port_count(&port[0]) > 1)
+ if (port_count(port) > 1)
goto unlock;
if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
@@ -605,7 +608,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
* the driver is unable to keep up the supply of new
* work).
*/
- if (port_count(&port[1]))
+ if (port_count(execlists_port_next(execlists, port)))
goto unlock;
/* WaIdleLiteRestore:bdw,skl
@@ -619,6 +622,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
}
}
+ last_port = execlists_port_tail(execlists);
+
do {
struct i915_priolist *p = rb_entry(rb, typeof(*p), node);
struct drm_i915_gem_request *rq, *rn;
@@ -665,7 +670,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
if (submit)
port_assign(port, last);
- port++;
+
+ port = execlists_port_next(execlists, port);
GEM_BUG_ON(port_isset(port));
}
@@ -699,8 +705,10 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
void
execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
{
- struct execlist_port *port = execlists->port;
unsigned int num_ports = execlists_num_ports(execlists);
+ struct execlist_port *port;
+
+ port = execlists_port_head(execlists);
while (num_ports-- && port_isset(port)) {
struct drm_i915_gem_request *rq = port_request(port);
@@ -709,9 +717,10 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED);
i915_gem_request_put(rq);
- memset(port, 0, sizeof(*port));
- port++;
+ port = execlists_head_complete(execlists, port);
}
+
+ GEM_BUG_ON(port_isset(execlists_port_head(execlists)));
}
static void execlists_cancel_requests(struct intel_engine_cs *engine)
@@ -778,7 +787,6 @@ static void intel_lrc_irq_handler(unsigned long data)
{
struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port * const port = execlists->port;
struct drm_i915_private *dev_priv = engine->i915;
/* We can skip acquiring intel_runtime_pm_get() here as it was taken
@@ -797,6 +805,8 @@ static void intel_lrc_irq_handler(unsigned long data)
* new request (outside of the context-switch interrupt).
*/
while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) {
+ struct execlist_port *port;
+
/* The HWSP contains a (cacheable) mirror of the CSB */
const u32 *buf =
&engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
@@ -833,6 +843,8 @@ static void intel_lrc_irq_handler(unsigned long data)
tail = READ_ONCE(buf[write_idx]);
}
+ port = execlists_port_head(execlists);
+
while (head != tail) {
struct drm_i915_gem_request *rq;
unsigned int status;
@@ -895,7 +907,7 @@ static void intel_lrc_irq_handler(unsigned long data)
trace_i915_gem_request_out(rq);
i915_gem_request_put(rq);
- execlists_port_complete(execlists, port);
+ port = execlists_head_complete(execlists, port);
} else {
port_set(port, port_pack(rq, count));
}
@@ -935,6 +947,7 @@ static void insert_request(struct intel_engine_cs *engine,
static void execlists_submit_request(struct drm_i915_gem_request *request)
{
struct intel_engine_cs *engine = request->engine;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
unsigned long flags;
/* Will be called from irq-context when using foreign fences. */
@@ -942,7 +955,7 @@ static void execlists_submit_request(struct drm_i915_gem_request *request)
insert_request(engine, &request->priotree, request->priotree.priority);
- GEM_BUG_ON(!engine->execlists.first);
+ GEM_BUG_ON(!execlists->first);
GEM_BUG_ON(list_empty(&request->priotree.link));
spin_unlock_irqrestore(&engine->timeline->lock, flags);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 69ad875fd011..387667fe50d3 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -259,6 +259,11 @@ struct intel_engine_execlists {
unsigned int port_mask;
/**
+ * @port_head: first used execlist port
+ */
+ unsigned int port_head;
+
+ /**
* @queue: queue of requests, in priority lists
*/
struct rb_root queue;
@@ -569,8 +574,41 @@ execlists_num_ports(const struct intel_engine_execlists * const execlists)
return execlists->port_mask + 1;
}
-static inline void
-execlists_port_complete(struct intel_engine_execlists * const execlists,
+#define __port_add(start, n, mask) (((start) + (n)) & (mask))
+#define port_head_add(e, n) __port_add((e)->port_head, n, (e)->port_mask)
+
+/* Index starting from port_head */
+static inline struct execlist_port *
+execlists_port(struct intel_engine_execlists * const execlists,
+ const unsigned int n)
+{
+ return &execlists->port[port_head_add(execlists, n)];
+}
+
+static inline struct execlist_port *
+execlists_port_head(struct intel_engine_execlists * const execlists)
+{
+ return execlists_port(execlists, 0);
+}
+
+static inline struct execlist_port *
+execlists_port_tail(struct intel_engine_execlists * const execlists)
+{
+ return execlists_port(execlists, -1);
+}
+
+static inline struct execlist_port *
+execlists_port_next(struct intel_engine_execlists * const execlists,
+ const struct execlist_port * const port)
+{
+ const unsigned int n = __port_add(port_index(port, execlists),
+ 1,
+ execlists->port_mask);
+ return &execlists->port[n];
+}
+
+static inline struct execlist_port *
+execlists_head_complete(struct intel_engine_execlists * const execlists,
struct execlist_port * const port)
{
const unsigned int m = execlists->port_mask;
@@ -580,6 +618,8 @@ execlists_port_complete(struct intel_engine_execlists * const execlists,
memmove(port, port + 1, m * sizeof(struct execlist_port));
memset(port + m, 0, sizeof(struct execlist_port));
+
+ return execlists_port_head(execlists);
}
static inline unsigned int
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array
2017-10-31 15:27 [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors Mika Kuoppala
@ 2017-10-31 15:27 ` Mika Kuoppala
2017-10-31 15:42 ` Chris Wilson
2017-11-02 15:07 ` Chris Wilson
2017-10-31 15:41 ` [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors Chris Wilson
` (2 subsequent siblings)
3 siblings, 2 replies; 25+ messages in thread
From: Mika Kuoppala @ 2017-10-31 15:27 UTC (permalink / raw)
To: intel-gfx
From: Mika Kuoppala <mika.kuoppala@intel.com>
As all our access to execlist ports are through head and tail
helpers, we can now move the head instead of memmoving the array.
v2: use memset (Chris)
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_ringbuffer.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 387667fe50d3..011c4b8f1339 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -611,13 +611,13 @@ static inline struct execlist_port *
execlists_head_complete(struct intel_engine_execlists * const execlists,
struct execlist_port * const port)
{
- const unsigned int m = execlists->port_mask;
-
- GEM_BUG_ON(port_index(port, execlists) != 0);
+ GEM_BUG_ON(port_index(port, execlists) != execlists->port_head);
+ GEM_BUG_ON(!port_isset(port));
GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_USER));
- memmove(port, port + 1, m * sizeof(struct execlist_port));
- memset(port + m, 0, sizeof(struct execlist_port));
+ memset(port, 0, sizeof(*port));
+
+ execlists->port_head = port_head_add(execlists, 1);
return execlists_port_head(execlists);
}
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-10-31 15:27 [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors Mika Kuoppala
2017-10-31 15:27 ` [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array Mika Kuoppala
@ 2017-10-31 15:41 ` Chris Wilson
2017-10-31 15:56 ` Chris Wilson
2017-11-02 10:38 ` Mika Kuoppala
2017-10-31 15:51 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] " Patchwork
2017-11-02 14:32 ` [PATCH 1/2] " Mika Kuoppala
3 siblings, 2 replies; 25+ messages in thread
From: Chris Wilson @ 2017-10-31 15:41 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx; +Cc: Mika
Quoting Mika Kuoppala (2017-10-31 15:27:33)
> +static inline struct execlist_port *
> +execlists_port_next(struct intel_engine_execlists * const execlists,
> + const struct execlist_port * const port)
> +{
> + const unsigned int n = __port_add(port_index(port, execlists),
> + 1,
> + execlists->port_mask);
How does this compare to
if (port++ == execlists->port + execlists->port_mask)
port = execlists->port;
return port;
?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array
2017-10-31 15:27 ` [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array Mika Kuoppala
@ 2017-10-31 15:42 ` Chris Wilson
2017-11-02 15:07 ` Chris Wilson
1 sibling, 0 replies; 25+ messages in thread
From: Chris Wilson @ 2017-10-31 15:42 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx; +Cc: Mika
Quoting Mika Kuoppala (2017-10-31 15:27:34)
> From: Mika Kuoppala <mika.kuoppala@intel.com>
>
> As all our access to execlist ports are through head and tail
> helpers, we can now move the head instead of memmoving the array.
>
> v2: use memset (Chris)
>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Still waiting for patch 3 to kill the memset ;)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Introduce execlist_port_* accessors
2017-10-31 15:27 [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors Mika Kuoppala
2017-10-31 15:27 ` [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array Mika Kuoppala
2017-10-31 15:41 ` [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors Chris Wilson
@ 2017-10-31 15:51 ` Patchwork
2017-11-02 14:32 ` [PATCH 1/2] " Mika Kuoppala
3 siblings, 0 replies; 25+ messages in thread
From: Patchwork @ 2017-10-31 15:51 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915: Introduce execlist_port_* accessors
URL : https://patchwork.freedesktop.org/series/32909/
State : failure
== Summary ==
Series 32909v1 series starting with [1/2] drm/i915: Introduce execlist_port_* accessors
https://patchwork.freedesktop.org/api/1.0/series/32909/revisions/1/mbox/
Test chamelium:
Subgroup dp-crc-fast:
fail -> PASS (fi-kbl-7500u) fdo#102514
Test kms_flip:
Subgroup basic-flip-vs-dpms:
pass -> INCOMPLETE (fi-cnl-y)
Test kms_frontbuffer_tracking:
Subgroup basic:
pass -> FAIL (fi-glk-dsi) fdo#103167
fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:438s
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:450s
fi-blb-e6850 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:376s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:517s
fi-bwr-2160 total:289 pass:183 dwarn:0 dfail:0 fail:0 skip:106 time:264s
fi-bxt-dsi total:289 pass:259 dwarn:0 dfail:0 fail:0 skip:30 time:495s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:497s
fi-byt-j1900 total:289 pass:253 dwarn:1 dfail:0 fail:0 skip:35 time:496s
fi-byt-n2820 total:289 pass:249 dwarn:1 dfail:0 fail:0 skip:39 time:472s
fi-cfl-s total:289 pass:253 dwarn:4 dfail:0 fail:0 skip:32 time:553s
fi-cnl-y total:217 pass:196 dwarn:0 dfail:0 fail:0 skip:20
fi-elk-e7500 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:423s
fi-gdg-551 total:289 pass:178 dwarn:1 dfail:0 fail:1 skip:109 time:247s
fi-glk-1 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:577s
fi-glk-dsi total:289 pass:258 dwarn:0 dfail:0 fail:1 skip:30 time:488s
fi-hsw-4770 total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:431s
fi-hsw-4770r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:427s
fi-ilk-650 total:289 pass:228 dwarn:0 dfail:0 fail:0 skip:61 time:418s
fi-ivb-3520m total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:496s
fi-ivb-3770 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:463s
fi-kbl-7500u total:289 pass:264 dwarn:1 dfail:0 fail:0 skip:24 time:491s
fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:568s
fi-kbl-7567u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:472s
fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:583s
fi-pnv-d510 total:289 pass:222 dwarn:1 dfail:0 fail:0 skip:66 time:551s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:454s
fi-skl-6600u total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:595s
fi-skl-6700hq total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:648s
fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:517s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:501s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:455s
fi-snb-2520m total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:554s
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:0 skip:40 time:421s
8bc65f660b9856c176d26032b1cc1230d984b1a3 drm-tip: 2017y-10m-31d-10h-44m-16s UTC integration manifest
0ed38f271ffc drm/i915: Move execlists port head instead of memmoving array
64100dcc99f0 drm/i915: Introduce execlist_port_* accessors
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6280/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-10-31 15:41 ` [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors Chris Wilson
@ 2017-10-31 15:56 ` Chris Wilson
2017-11-02 10:38 ` Mika Kuoppala
1 sibling, 0 replies; 25+ messages in thread
From: Chris Wilson @ 2017-10-31 15:56 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx
Quoting Chris Wilson (2017-10-31 15:41:52)
> Quoting Mika Kuoppala (2017-10-31 15:27:33)
> > +static inline struct execlist_port *
> > +execlists_port_next(struct intel_engine_execlists * const execlists,
> > + const struct execlist_port * const port)
> > +{
> > + const unsigned int n = __port_add(port_index(port, execlists),
> > + 1,
> > + execlists->port_mask);
>
> How does this compare to
>
> if (port++ == execlists->port + execlists->port_mask)
> port = execlists->port;
>
> return port;
> ?
Rough estimate from bloat-o-meter
patch:
intel_lrc_irq_handler 1734 1926 +192
i915_guc_irq_handler 1522 1620 +98
execlists_cancel_port_requests 81 129 +48
intel_engine_dump 2030 2077 +47
intel_engine_init_cmd_parser 1132 1136 +4
capture 5633 5620 -13
delta:
i915_guc_irq_handler 1620 1617 -3
intel_lrc_irq_handler 1926 1790 -136
overall:
i915_guc_irq_handler 1522 1617 +95
intel_lrc_irq_handler 1734 1790 +56
execlists_cancel_port_requests 81 129 +48
intel_engine_dump 2030 2077 +47
intel_engine_init_cmd_parser 1132 1136 +4
capture 5633 5620 -13
But still +56 in irq_handler, that's mostly dequeue I guess. Can we do
better?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-10-31 15:41 ` [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors Chris Wilson
2017-10-31 15:56 ` Chris Wilson
@ 2017-11-02 10:38 ` Mika Kuoppala
2017-11-02 10:57 ` Chris Wilson
1 sibling, 1 reply; 25+ messages in thread
From: Mika Kuoppala @ 2017-11-02 10:38 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Quoting Mika Kuoppala (2017-10-31 15:27:33)
>> +static inline struct execlist_port *
>> +execlists_port_next(struct intel_engine_execlists * const execlists,
>> + const struct execlist_port * const port)
>> +{
>> + const unsigned int n = __port_add(port_index(port, execlists),
>> + 1,
>> + execlists->port_mask);
>
> How does this compare to
>
> if (port++ == execlists->port + execlists->port_mask)
> port = execlists->port;
>
> return port;
> ?
add/remove: 0/0 grow/shrink: 1/1 up/down: 29/-29 (0)
function old new delta
i915_guc_irq_handler 2584 2613 +29
intel_lrc_irq_handler 2963 2934 -29
Total: Before=1123627, After=1123627, chg +0.00%
:)
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-11-02 10:38 ` Mika Kuoppala
@ 2017-11-02 10:57 ` Chris Wilson
2017-11-02 14:14 ` Mika Kuoppala
0 siblings, 1 reply; 25+ messages in thread
From: Chris Wilson @ 2017-11-02 10:57 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx
Quoting Mika Kuoppala (2017-11-02 10:38:30)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > Quoting Mika Kuoppala (2017-10-31 15:27:33)
> >> +static inline struct execlist_port *
> >> +execlists_port_next(struct intel_engine_execlists * const execlists,
> >> + const struct execlist_port * const port)
> >> +{
> >> + const unsigned int n = __port_add(port_index(port, execlists),
> >> + 1,
> >> + execlists->port_mask);
> >
> > How does this compare to
> >
> > if (port++ == execlists->port + execlists->port_mask)
> > port = execlists->port;
> >
> > return port;
> > ?
>
> add/remove: 0/0 grow/shrink: 1/1 up/down: 29/-29 (0)
> function old new delta
> i915_guc_irq_handler 2584 2613 +29
> intel_lrc_irq_handler 2963 2934 -29
> Total: Before=1123627, After=1123627, chg +0.00%
Hmm, your functions saw little difference and are twice as big as
mine... Weird.
I used gcc version 6.3.0 20170516 (Debian 6.3.0-18) with defconfig.
Yourself?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-11-02 10:57 ` Chris Wilson
@ 2017-11-02 14:14 ` Mika Kuoppala
2017-11-02 14:15 ` Mika Kuoppala
0 siblings, 1 reply; 25+ messages in thread
From: Mika Kuoppala @ 2017-11-02 14:14 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Quoting Mika Kuoppala (2017-11-02 10:38:30)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>>
>> > Quoting Mika Kuoppala (2017-10-31 15:27:33)
>> >> +static inline struct execlist_port *
>> >> +execlists_port_next(struct intel_engine_execlists * const execlists,
>> >> + const struct execlist_port * const port)
>> >> +{
>> >> + const unsigned int n = __port_add(port_index(port, execlists),
>> >> + 1,
>> >> + execlists->port_mask);
>> >
>> > How does this compare to
>> >
>> > if (port++ == execlists->port + execlists->port_mask)
>> > port = execlists->port;
>> >
>> > return port;
>> > ?
>>
>> add/remove: 0/0 grow/shrink: 1/1 up/down: 29/-29 (0)
>> function old new delta
>> i915_guc_irq_handler 2584 2613 +29
>> intel_lrc_irq_handler 2963 2934 -29
>> Total: Before=1123627, After=1123627, chg +0.00%
>
> Hmm, your functions saw little difference and are twice as big as
> mine... Weird.
>
> I used gcc version 6.3.0 20170516 (Debian 6.3.0-18) with defconfig.
> Yourself?
I had debugs on, sigh...
Now with the defconfig and gcc (Ubuntu 6.3.0-12ubuntu2) 6.3.0 20170406:
add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-139 (-139)
function old new delta
i915_guc_irq_handler 1620 1617 -3
intel_lrc_irq_handler 1926 1790 -136
So we have a clear winner.
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-11-02 14:14 ` Mika Kuoppala
@ 2017-11-02 14:15 ` Mika Kuoppala
0 siblings, 0 replies; 25+ messages in thread
From: Mika Kuoppala @ 2017-11-02 14:15 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Mika Kuoppala <mika.kuoppala@linux.intel.com> writes:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
>> Quoting Mika Kuoppala (2017-11-02 10:38:30)
>>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>>>
>>> > Quoting Mika Kuoppala (2017-10-31 15:27:33)
>>> >> +static inline struct execlist_port *
>>> >> +execlists_port_next(struct intel_engine_execlists * const execlists,
>>> >> + const struct execlist_port * const port)
>>> >> +{
>>> >> + const unsigned int n = __port_add(port_index(port, execlists),
>>> >> + 1,
>>> >> + execlists->port_mask);
>>> >
>>> > How does this compare to
>>> >
>>> > if (port++ == execlists->port + execlists->port_mask)
>>> > port = execlists->port;
>>> >
>>> > return port;
>>> > ?
>>>
>>> add/remove: 0/0 grow/shrink: 1/1 up/down: 29/-29 (0)
>>> function old new delta
>>> i915_guc_irq_handler 2584 2613 +29
>>> intel_lrc_irq_handler 2963 2934 -29
>>> Total: Before=1123627, After=1123627, chg +0.00%
>>
>> Hmm, your functions saw little difference and are twice as big as
>> mine... Weird.
>>
>> I used gcc version 6.3.0 20170516 (Debian 6.3.0-18) with defconfig.
>> Yourself?
>
> I had debugs on, sigh...
>
> Now with the defconfig and gcc (Ubuntu 6.3.0-12ubuntu2) 6.3.0 20170406:
>
> add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-139 (-139)
> function old new delta
> i915_guc_irq_handler 1620 1617 -3
> intel_lrc_irq_handler 1926 1790 -136
>
> So we have a clear winner.
>
And to be precise on what diff lead to above:
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 387667fe50d3..9131d66fb628 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -599,12 +599,12 @@ execlists_port_tail(struct intel_engine_execlists * const execlists)
static inline struct execlist_port *
execlists_port_next(struct intel_engine_execlists * const execlists,
- const struct execlist_port * const port)
+ struct execlist_port *port)
{
- const unsigned int n = __port_add(port_index(port, execlists),
- 1,
- execlists->port_mask);
- return &execlists->port[n];
+ if (port++ == execlists->port + execlists->port_mask)
+ port = execlists->port;
+
+ return port;
}
> -Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-10-31 15:27 [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors Mika Kuoppala
` (2 preceding siblings ...)
2017-10-31 15:51 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] " Patchwork
@ 2017-11-02 14:32 ` Mika Kuoppala
2017-11-02 15:03 ` Chris Wilson
3 siblings, 1 reply; 25+ messages in thread
From: Mika Kuoppala @ 2017-11-02 14:32 UTC (permalink / raw)
To: intel-gfx
From: Mika Kuoppala <mika.kuoppala@intel.com>
Instead of trusting that first available port is at index 0,
use accessor to hide this. This is a preparation for a
following patches where head can be at arbitrary location
in the port array.
v2: improved commit message, elsp_ready readability (Chris)
v3: s/execlist_port_index/execlist_port (Chris)
v4: rebase to new naming
v5: fix port_next indexing
v6: adapt to preempt
v7: improved _port_next (Chris)
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gpu_error.c | 6 ++--
drivers/gpu/drm/i915/i915_guc_submission.c | 50 ++++++++++++++++++------------
drivers/gpu/drm/i915/intel_engine_cs.c | 18 ++++++-----
drivers/gpu/drm/i915/intel_lrc.c | 49 ++++++++++++++++++-----------
drivers/gpu/drm/i915/intel_ringbuffer.h | 44 ++++++++++++++++++++++++--
5 files changed, 119 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 653fb69e7ecb..6d0bdb03b3f0 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1333,11 +1333,13 @@ static void engine_record_requests(struct intel_engine_cs *engine,
static void error_record_engine_execlists(struct intel_engine_cs *engine,
struct drm_i915_error_engine *ee)
{
- const struct intel_engine_execlists * const execlists = &engine->execlists;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
unsigned int n;
for (n = 0; n < execlists_num_ports(execlists); n++) {
- struct drm_i915_gem_request *rq = port_request(&execlists->port[n]);
+ struct drm_i915_gem_request *rq;
+
+ rq = port_request(execlists_port(execlists, n));
if (!rq)
break;
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index d14c1342f09d..458658e8e99b 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -693,16 +693,18 @@ static void i915_guc_submit(struct intel_engine_cs *engine)
{
struct intel_guc *guc = &engine->i915->guc;
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port *port = execlists->port;
unsigned int n;
for (n = 0; n < execlists_num_ports(execlists); n++) {
+ struct execlist_port *port;
struct drm_i915_gem_request *rq;
unsigned int count;
- rq = port_unpack(&port[n], &count);
+ port = execlists_port(execlists, n);
+ rq = port_unpack(port, &count);
+
if (rq && count == 0) {
- port_set(&port[n], port_pack(rq, ++count));
+ port_set(port, port_pack(rq, ++count));
flush_ggtt_writes(rq->ring->vma);
@@ -725,10 +727,8 @@ static void port_assign(struct execlist_port *port,
static void i915_guc_dequeue(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port *port = execlists->port;
+ struct execlist_port *port, *last_port;
struct drm_i915_gem_request *last = NULL;
- const struct execlist_port * const last_port =
- &execlists->port[execlists->port_mask];
bool submit = false;
struct rb_node *rb;
@@ -739,6 +739,9 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
if (!rb)
goto unlock;
+ port = execlists_port_head(execlists);
+ last_port = execlists_port_tail(execlists);
+
if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) && port_isset(port)) {
struct guc_preempt_work *preempt_work =
&engine->i915->guc.preempt_work[engine->id];
@@ -754,7 +757,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
goto unlock;
}
- port++;
+ port = execlists_port_next(execlists, port);
}
do {
@@ -771,7 +774,8 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
if (submit)
port_assign(port, last);
- port++;
+
+ port = execlists_port_next(execlists, port);
}
INIT_LIST_HEAD(&rq->priotree.link);
@@ -799,24 +803,32 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
spin_unlock_irq(&engine->timeline->lock);
}
-static void i915_guc_irq_handler(unsigned long data)
+static void guc_complete_ready_ports(struct intel_engine_execlists * const execlists)
{
- struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
- struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port *port = execlists->port;
- struct drm_i915_gem_request *rq;
+ struct execlist_port *port = execlists_port_head(execlists);
+
+ while (port_isset(port)) {
+ struct drm_i915_gem_request *rq = port_request(port);
+
+ if (!i915_gem_request_completed(rq))
+ break;
- rq = port_request(&port[0]);
- while (rq && i915_gem_request_completed(rq)) {
trace_i915_gem_request_out(rq);
i915_gem_request_put(rq);
- execlists_port_complete(execlists, port);
+ port = execlists_head_complete(execlists, port);
+ };
- rq = port_request(&port[0]);
- }
- if (!rq)
+ if (!port_isset(port))
execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER);
+}
+
+static void i915_guc_irq_handler(unsigned long data)
+{
+ struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
+
+ guc_complete_ready_ports(execlists);
if (execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT) &&
intel_read_status_page(engine, I915_GEM_HWS_PREEMPT_INDEX) ==
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index ddbe5c9bf45a..6dc47143f415 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1687,7 +1687,7 @@ static void print_request(struct drm_printer *m,
void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m)
{
struct intel_breadcrumbs * const b = &engine->breadcrumbs;
- const struct intel_engine_execlists * const execlists = &engine->execlists;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
struct i915_gpu_error * const error = &engine->i915->gpu_error;
struct drm_i915_private *dev_priv = engine->i915;
struct drm_i915_gem_request *rq;
@@ -1791,16 +1791,20 @@ void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m)
rcu_read_lock();
for (idx = 0; idx < execlists_num_ports(execlists); idx++) {
- unsigned int count;
+ struct execlist_port *port;
+ unsigned int count, idx_abs;
+
+ port = execlists_port(execlists, idx);
+ idx_abs = port_index(port, execlists);
- rq = port_unpack(&execlists->port[idx], &count);
+ rq = port_unpack(port, &count);
if (rq) {
- drm_printf(m, "\t\tELSP[%d] count=%d, ",
- idx, count);
+ drm_printf(m, "\t\tELSP[%d:%d] count=%d, ",
+ idx, idx_abs, count);
print_request(m, rq, "rq: ");
} else {
- drm_printf(m, "\t\tELSP[%d] idle\n",
- idx);
+ drm_printf(m, "\t\tELSP[%d:%d] idle\n",
+ idx, idx_abs);
}
}
drm_printf(m, "\t\tHW active? 0x%x\n", execlists->active);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6840ec8db037..62c3e06a110d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -448,24 +448,26 @@ static inline void elsp_write(u64 desc, u32 __iomem *elsp)
static void execlists_submit_ports(struct intel_engine_cs *engine)
{
- struct execlist_port *port = engine->execlists.port;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
u32 __iomem *elsp =
engine->i915->regs + i915_mmio_reg_offset(RING_ELSP(engine));
unsigned int n;
- for (n = execlists_num_ports(&engine->execlists); n--; ) {
+ for (n = execlists_num_ports(execlists); n--; ) {
+ struct execlist_port *port;
struct drm_i915_gem_request *rq;
unsigned int count;
u64 desc;
- rq = port_unpack(&port[n], &count);
+ port = execlists_port(execlists, n);
+ rq = port_unpack(port, &count);
if (rq) {
GEM_BUG_ON(count > !n);
if (!count++)
execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
- port_set(&port[n], port_pack(rq, count));
+ port_set(port, port_pack(rq, count));
desc = execlists_update_context(rq);
- GEM_DEBUG_EXEC(port[n].context_id = upper_32_bits(desc));
+ GEM_DEBUG_EXEC(port->context_id = upper_32_bits(desc));
} else {
GEM_BUG_ON(!n);
desc = 0;
@@ -529,10 +531,8 @@ static void inject_preempt_context(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;
- const struct execlist_port * const last_port =
- &execlists->port[execlists->port_mask];
- struct drm_i915_gem_request *last = port_request(port);
+ struct execlist_port *port, *last_port;
+ struct drm_i915_gem_request *last;
struct rb_node *rb;
bool submit = false;
@@ -563,6 +563,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
if (!rb)
goto unlock;
+ port = execlists_port_head(execlists);
+ last = port_request(port);
+
if (last) {
/*
* Don't resubmit or switch until all outstanding
@@ -570,7 +573,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
* know the next preemption status we see corresponds
* to this ELSP update.
*/
- if (port_count(&port[0]) > 1)
+ if (port_count(port) > 1)
goto unlock;
if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
@@ -605,7 +608,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
* the driver is unable to keep up the supply of new
* work).
*/
- if (port_count(&port[1]))
+ if (port_count(execlists_port_next(execlists, port)))
goto unlock;
/* WaIdleLiteRestore:bdw,skl
@@ -619,6 +622,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
}
}
+ last_port = execlists_port_tail(execlists);
+
do {
struct i915_priolist *p = rb_entry(rb, typeof(*p), node);
struct drm_i915_gem_request *rq, *rn;
@@ -665,7 +670,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
if (submit)
port_assign(port, last);
- port++;
+
+ port = execlists_port_next(execlists, port);
GEM_BUG_ON(port_isset(port));
}
@@ -699,8 +705,10 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
void
execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
{
- struct execlist_port *port = execlists->port;
unsigned int num_ports = execlists_num_ports(execlists);
+ struct execlist_port *port;
+
+ port = execlists_port_head(execlists);
while (num_ports-- && port_isset(port)) {
struct drm_i915_gem_request *rq = port_request(port);
@@ -709,9 +717,10 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED);
i915_gem_request_put(rq);
- memset(port, 0, sizeof(*port));
- port++;
+ port = execlists_head_complete(execlists, port);
}
+
+ GEM_BUG_ON(port_isset(execlists_port_head(execlists)));
}
static void execlists_cancel_requests(struct intel_engine_cs *engine)
@@ -778,7 +787,6 @@ static void intel_lrc_irq_handler(unsigned long data)
{
struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port * const port = execlists->port;
struct drm_i915_private *dev_priv = engine->i915;
/* We can skip acquiring intel_runtime_pm_get() here as it was taken
@@ -797,6 +805,8 @@ static void intel_lrc_irq_handler(unsigned long data)
* new request (outside of the context-switch interrupt).
*/
while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) {
+ struct execlist_port *port;
+
/* The HWSP contains a (cacheable) mirror of the CSB */
const u32 *buf =
&engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
@@ -833,6 +843,8 @@ static void intel_lrc_irq_handler(unsigned long data)
tail = READ_ONCE(buf[write_idx]);
}
+ port = execlists_port_head(execlists);
+
while (head != tail) {
struct drm_i915_gem_request *rq;
unsigned int status;
@@ -895,7 +907,7 @@ static void intel_lrc_irq_handler(unsigned long data)
trace_i915_gem_request_out(rq);
i915_gem_request_put(rq);
- execlists_port_complete(execlists, port);
+ port = execlists_head_complete(execlists, port);
} else {
port_set(port, port_pack(rq, count));
}
@@ -935,6 +947,7 @@ static void insert_request(struct intel_engine_cs *engine,
static void execlists_submit_request(struct drm_i915_gem_request *request)
{
struct intel_engine_cs *engine = request->engine;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
unsigned long flags;
/* Will be called from irq-context when using foreign fences. */
@@ -942,7 +955,7 @@ static void execlists_submit_request(struct drm_i915_gem_request *request)
insert_request(engine, &request->priotree, request->priotree.priority);
- GEM_BUG_ON(!engine->execlists.first);
+ GEM_BUG_ON(!execlists->first);
GEM_BUG_ON(list_empty(&request->priotree.link));
spin_unlock_irqrestore(&engine->timeline->lock, flags);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 69ad875fd011..9131d66fb628 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -259,6 +259,11 @@ struct intel_engine_execlists {
unsigned int port_mask;
/**
+ * @port_head: first used execlist port
+ */
+ unsigned int port_head;
+
+ /**
* @queue: queue of requests, in priority lists
*/
struct rb_root queue;
@@ -569,8 +574,41 @@ execlists_num_ports(const struct intel_engine_execlists * const execlists)
return execlists->port_mask + 1;
}
-static inline void
-execlists_port_complete(struct intel_engine_execlists * const execlists,
+#define __port_add(start, n, mask) (((start) + (n)) & (mask))
+#define port_head_add(e, n) __port_add((e)->port_head, n, (e)->port_mask)
+
+/* Index starting from port_head */
+static inline struct execlist_port *
+execlists_port(struct intel_engine_execlists * const execlists,
+ const unsigned int n)
+{
+ return &execlists->port[port_head_add(execlists, n)];
+}
+
+static inline struct execlist_port *
+execlists_port_head(struct intel_engine_execlists * const execlists)
+{
+ return execlists_port(execlists, 0);
+}
+
+static inline struct execlist_port *
+execlists_port_tail(struct intel_engine_execlists * const execlists)
+{
+ return execlists_port(execlists, -1);
+}
+
+static inline struct execlist_port *
+execlists_port_next(struct intel_engine_execlists * const execlists,
+ struct execlist_port *port)
+{
+ if (port++ == execlists->port + execlists->port_mask)
+ port = execlists->port;
+
+ return port;
+}
+
+static inline struct execlist_port *
+execlists_head_complete(struct intel_engine_execlists * const execlists,
struct execlist_port * const port)
{
const unsigned int m = execlists->port_mask;
@@ -580,6 +618,8 @@ execlists_port_complete(struct intel_engine_execlists * const execlists,
memmove(port, port + 1, m * sizeof(struct execlist_port));
memset(port + m, 0, sizeof(struct execlist_port));
+
+ return execlists_port_head(execlists);
}
static inline unsigned int
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-11-02 14:32 ` [PATCH 1/2] " Mika Kuoppala
@ 2017-11-02 15:03 ` Chris Wilson
0 siblings, 0 replies; 25+ messages in thread
From: Chris Wilson @ 2017-11-02 15:03 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx; +Cc: Mika
Quoting Mika Kuoppala (2017-11-02 14:32:38)
> From: Mika Kuoppala <mika.kuoppala@intel.com>
>
> Instead of trusting that first available port is at index 0,
> use accessor to hide this. This is a preparation for a
> following patches where head can be at arbitrary location
> in the port array.
>
> v2: improved commit message, elsp_ready readability (Chris)
> v3: s/execlist_port_index/execlist_port (Chris)
> v4: rebase to new naming
> v5: fix port_next indexing
> v6: adapt to preempt
> v7: improved _port_next (Chris)
>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gpu_error.c | 6 ++--
> drivers/gpu/drm/i915/i915_guc_submission.c | 50 ++++++++++++++++++------------
> drivers/gpu/drm/i915/intel_engine_cs.c | 18 ++++++-----
> drivers/gpu/drm/i915/intel_lrc.c | 49 ++++++++++++++++++-----------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 44 ++++++++++++++++++++++++--
> 5 files changed, 119 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 653fb69e7ecb..6d0bdb03b3f0 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1333,11 +1333,13 @@ static void engine_record_requests(struct intel_engine_cs *engine,
> static void error_record_engine_execlists(struct intel_engine_cs *engine,
> struct drm_i915_error_engine *ee)
> {
> - const struct intel_engine_execlists * const execlists = &engine->execlists;
> + struct intel_engine_execlists * const execlists = &engine->execlists;
> unsigned int n;
>
> for (n = 0; n < execlists_num_ports(execlists); n++) {
> - struct drm_i915_gem_request *rq = port_request(&execlists->port[n]);
> + struct drm_i915_gem_request *rq;
> +
> + rq = port_request(execlists_port(execlists, n));
>
This newline isn't as interesting as the others. No one will shed a tear
if it is removed.
> if (!rq)
> break;
> @@ -665,7 +670,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>
> if (submit)
> port_assign(port, last);
> - port++;
> +
> + port = execlists_port_next(execlists, port);
>
Spare us this newline as well. Let's have the advance and BUG() tightly
coupled.
> GEM_BUG_ON(port_isset(port));
> }
> @@ -699,8 +705,10 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> void
> execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
> {
> - struct execlist_port *port = execlists->port;
> unsigned int num_ports = execlists_num_ports(execlists);
> + struct execlist_port *port;
> +
> + port = execlists_port_head(execlists);
>
> while (num_ports-- && port_isset(port)) {
for (port = execlists_port_head(execlists);
num_ports-- && port_isset(port);
port = execlists_head_complete(execlists, port)) {
Might as well complete the transformation to more normal code ;)
> +static inline struct execlist_port *
> +execlists_head_complete(struct intel_engine_execlists * const execlists,
> struct execlist_port * const port)
> {
> const unsigned int m = execlists->port_mask;
> @@ -580,6 +618,8 @@ execlists_port_complete(struct intel_engine_execlists * const execlists,
>
> memmove(port, port + 1, m * sizeof(struct execlist_port));
> memset(port + m, 0, sizeof(struct execlist_port));
> +
> + return execlists_port_head(execlists);
Hang on a sec, isn't port->head itself meant to advance here? Oh,
that'll be the next patch and this is just prep.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array
2017-10-31 15:27 ` [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array Mika Kuoppala
2017-10-31 15:42 ` Chris Wilson
@ 2017-11-02 15:07 ` Chris Wilson
2017-11-22 13:52 ` Mika Kuoppala
1 sibling, 1 reply; 25+ messages in thread
From: Chris Wilson @ 2017-11-02 15:07 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx; +Cc: Mika
Quoting Mika Kuoppala (2017-10-31 15:27:34)
> From: Mika Kuoppala <mika.kuoppala@intel.com>
>
> As all our access to execlist ports are through head and tail
> helpers, we can now move the head instead of memmoving the array.
>
> v2: use memset (Chris)
>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_ringbuffer.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 387667fe50d3..011c4b8f1339 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -611,13 +611,13 @@ static inline struct execlist_port *
> execlists_head_complete(struct intel_engine_execlists * const execlists,
> struct execlist_port * const port)
> {
> - const unsigned int m = execlists->port_mask;
> -
> - GEM_BUG_ON(port_index(port, execlists) != 0);
> + GEM_BUG_ON(port_index(port, execlists) != execlists->port_head);
> + GEM_BUG_ON(!port_isset(port));
> GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_USER));
>
> - memmove(port, port + 1, m * sizeof(struct execlist_port));
> - memset(port + m, 0, sizeof(struct execlist_port));
> + memset(port, 0, sizeof(*port));
> +
> + execlists->port_head = port_head_add(execlists, 1);
Ok, I would have gone for
port = port_next(port);
execlists->port_head = port - execlists->port;
return port;
That to me looks more natural advance of port as we complete the
requests, and matches the loop in the irq handler.
Care to crunch the numbers and see which gcc favours?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array
2017-11-02 15:07 ` Chris Wilson
@ 2017-11-22 13:52 ` Mika Kuoppala
2017-11-22 13:57 ` Chris Wilson
0 siblings, 1 reply; 25+ messages in thread
From: Mika Kuoppala @ 2017-11-22 13:52 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Quoting Mika Kuoppala (2017-10-31 15:27:34)
>> From: Mika Kuoppala <mika.kuoppala@intel.com>
>>
>> As all our access to execlist ports are through head and tail
>> helpers, we can now move the head instead of memmoving the array.
>>
>> v2: use memset (Chris)
>>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_ringbuffer.h | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 387667fe50d3..011c4b8f1339 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -611,13 +611,13 @@ static inline struct execlist_port *
>> execlists_head_complete(struct intel_engine_execlists * const execlists,
>> struct execlist_port * const port)
>> {
>> - const unsigned int m = execlists->port_mask;
>> -
>> - GEM_BUG_ON(port_index(port, execlists) != 0);
>> + GEM_BUG_ON(port_index(port, execlists) != execlists->port_head);
>> + GEM_BUG_ON(!port_isset(port));
>> GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_USER));
>>
>> - memmove(port, port + 1, m * sizeof(struct execlist_port));
>> - memset(port + m, 0, sizeof(struct execlist_port));
>> + memset(port, 0, sizeof(*port));
>> +
>> + execlists->port_head = port_head_add(execlists, 1);
>
> Ok, I would have gone for
>
> port = port_next(port);
> execlists->port_head = port - execlists->port;
> return port;
>
> That to me looks more natural advance of port as we complete the
> requests, and matches the loop in the irq handler.
>
> Care to crunch the numbers and see which gcc favours?
gcc (Ubuntu 6.3.0-12ubuntu2) 6.3.0 20170406:
add/remove: 0/0 grow/shrink: 3/0 up/down: 219/0 (219)
function old new delta
execlists_submission_tasklet 2405 2525 +120
execlists_cancel_port_requests 315 376 +61
guc_submission_tasklet 1643 1681 +38
Total: Before=1168854, After=1169073, chg +0.02%
gcc (Ubuntu 7.2.0-8ubuntu3) 7.2.0:
add/remove: 0/0 grow/shrink: 3/0 up/down: 158/0 (158)
function old new delta
execlists_submission_tasklet 2383 2460 +77
execlists_cancel_port_requests 344 401 +57
guc_submission_tasklet 1684 1708 +24
Total: Before=1164662, After=1164820, chg +0.01%
where new:
execlists_head_complete(struct intel_engine_execlists * const execlists,
- struct execlist_port * const port)
+ struct execlist_port *port)
{
GEM_BUG_ON(port_index(port, execlists) != execlists->port_head);
GEM_BUG_ON(!port_isset(port));
@@ -677,9 +677,10 @@ execlists_head_complete(struct intel_engine_execlists * const execlists,
memset(port, 0, sizeof(*port));
- execlists->port_head = port_head_add(execlists, 1);
+ port = execlists_port_next(execlists, port);
+ execlists->port_head = port - execlists->port;
- return execlists_port_head(execlists);
+ return port;
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array
2017-11-22 13:52 ` Mika Kuoppala
@ 2017-11-22 13:57 ` Chris Wilson
0 siblings, 0 replies; 25+ messages in thread
From: Chris Wilson @ 2017-11-22 13:57 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx
Quoting Mika Kuoppala (2017-11-22 13:52:09)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > Quoting Mika Kuoppala (2017-10-31 15:27:34)
> >> From: Mika Kuoppala <mika.kuoppala@intel.com>
> >>
> >> As all our access to execlist ports are through head and tail
> >> helpers, we can now move the head instead of memmoving the array.
> >>
> >> v2: use memset (Chris)
> >>
> >> Cc: Michał Winiarski <michal.winiarski@intel.com>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_ringbuffer.h | 10 +++++-----
> >> 1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> index 387667fe50d3..011c4b8f1339 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> @@ -611,13 +611,13 @@ static inline struct execlist_port *
> >> execlists_head_complete(struct intel_engine_execlists * const execlists,
> >> struct execlist_port * const port)
> >> {
> >> - const unsigned int m = execlists->port_mask;
> >> -
> >> - GEM_BUG_ON(port_index(port, execlists) != 0);
> >> + GEM_BUG_ON(port_index(port, execlists) != execlists->port_head);
> >> + GEM_BUG_ON(!port_isset(port));
> >> GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_USER));
> >>
> >> - memmove(port, port + 1, m * sizeof(struct execlist_port));
> >> - memset(port + m, 0, sizeof(struct execlist_port));
> >> + memset(port, 0, sizeof(*port));
> >> +
> >> + execlists->port_head = port_head_add(execlists, 1);
> >
> > Ok, I would have gone for
> >
> > port = port_next(port);
> > execlists->port_head = port - execlists->port;
> > return port;
> >
> > That to me looks more natural advance of port as we complete the
> > requests, and matches the loop in the irq handler.
> >
> > Care to crunch the numbers and see which gcc favours?
>
> gcc (Ubuntu 6.3.0-12ubuntu2) 6.3.0 20170406:
>
> add/remove: 0/0 grow/shrink: 3/0 up/down: 219/0 (219)
> function old new delta
> execlists_submission_tasklet 2405 2525 +120
> execlists_cancel_port_requests 315 376 +61
> guc_submission_tasklet 1643 1681 +38
> Total: Before=1168854, After=1169073, chg +0.02%
>
> gcc (Ubuntu 7.2.0-8ubuntu3) 7.2.0:
>
> add/remove: 0/0 grow/shrink: 3/0 up/down: 158/0 (158)
> function old new delta
> execlists_submission_tasklet 2383 2460 +77
> execlists_cancel_port_requests 344 401 +57
> guc_submission_tasklet 1684 1708 +24
> Total: Before=1164662, After=1164820, chg +0.01%
Ok, have a
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-11-30 9:10 ` [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors Mika Kuoppala
@ 2017-11-30 10:21 ` Chris Wilson
0 siblings, 0 replies; 25+ messages in thread
From: Chris Wilson @ 2017-11-30 10:21 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx; +Cc: Mika
Quoting Mika Kuoppala (2017-11-30 09:10:27)
> From: Mika Kuoppala <mika.kuoppala@intel.com>
>
> Instead of trusting that first available port is at index 0,
> use accessor to hide this. This is a preparation for a
> following patches where head can be at arbitrary location
> in the port array.
>
> v2: improved commit message, elsp_ready readability (Chris)
> v3: s/execlist_port_index/execlist_port (Chris)
> v4: rebase to new naming
> v5: fix port_next indexing
> v6: adapt to preempt
> v7: improved _port_next (Chris)
> v8: whitespace, for loop (Chris),
> find_first_unset and GEM_BUG_ON after next_port in guc submission
>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
I get
add/remove: 0/0 grow/shrink: 4/2 up/down: 380/-2 (378)
function old new delta
guc_submission_tasklet 1488 1660 +172
execlists_submission_tasklet 2129 2245 +116
intel_engine_dump 2234 2281 +47
execlists_cancel_port_requests 254 299 +45
intel_engine_init_cmd_parser 1134 1133 -1
capture 5700 5699 -1
Not the end of the world, just something we may be able to tweak.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-11-30 9:10 [PATCH 0/2] execlist port handling improvements Mika Kuoppala
@ 2017-11-30 9:10 ` Mika Kuoppala
2017-11-30 10:21 ` Chris Wilson
0 siblings, 1 reply; 25+ messages in thread
From: Mika Kuoppala @ 2017-11-30 9:10 UTC (permalink / raw)
To: intel-gfx
From: Mika Kuoppala <mika.kuoppala@intel.com>
Instead of trusting that first available port is at index 0,
use accessor to hide this. This is a preparation for a
following patches where head can be at arbitrary location
in the port array.
v2: improved commit message, elsp_ready readability (Chris)
v3: s/execlist_port_index/execlist_port (Chris)
v4: rebase to new naming
v5: fix port_next indexing
v6: adapt to preempt
v7: improved _port_next (Chris)
v8: whitespace, for loop (Chris),
find_first_unset and GEM_BUG_ON after next_port in guc submission
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gpu_error.c | 5 +-
drivers/gpu/drm/i915/intel_engine_cs.c | 18 +++++---
drivers/gpu/drm/i915/intel_guc_submission.c | 71 ++++++++++++++++++++---------
drivers/gpu/drm/i915/intel_lrc.c | 55 +++++++++++++---------
drivers/gpu/drm/i915/intel_ringbuffer.h | 44 +++++++++++++++++-
5 files changed, 139 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 876be8f1d930..02bae7d697da 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1350,11 +1350,12 @@ static void engine_record_requests(struct intel_engine_cs *engine,
static void error_record_engine_execlists(struct intel_engine_cs *engine,
struct drm_i915_error_engine *ee)
{
- const struct intel_engine_execlists * const execlists = &engine->execlists;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
unsigned int n;
for (n = 0; n < execlists_num_ports(execlists); n++) {
- struct drm_i915_gem_request *rq = port_request(&execlists->port[n]);
+ struct drm_i915_gem_request *rq =
+ port_request(execlists_port(execlists, n));
if (!rq)
break;
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index cffd0c812b7e..e954a50d20cf 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1667,7 +1667,7 @@ static void print_request(struct drm_printer *m,
void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m)
{
struct intel_breadcrumbs * const b = &engine->breadcrumbs;
- const struct intel_engine_execlists * const execlists = &engine->execlists;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
struct i915_gpu_error * const error = &engine->i915->gpu_error;
struct drm_i915_private *dev_priv = engine->i915;
struct drm_i915_gem_request *rq;
@@ -1780,16 +1780,20 @@ void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m)
rcu_read_lock();
for (idx = 0; idx < execlists_num_ports(execlists); idx++) {
- unsigned int count;
+ struct execlist_port *port;
+ unsigned int count, idx_abs;
+
+ port = execlists_port(execlists, idx);
+ idx_abs = port_index(port, execlists);
- rq = port_unpack(&execlists->port[idx], &count);
+ rq = port_unpack(port, &count);
if (rq) {
- drm_printf(m, "\t\tELSP[%d] count=%d, ",
- idx, count);
+ drm_printf(m, "\t\tELSP[%d:%d] count=%d, ",
+ idx, idx_abs, count);
print_request(m, rq, "rq: ");
} else {
- drm_printf(m, "\t\tELSP[%d] idle\n",
- idx);
+ drm_printf(m, "\t\tELSP[%d:%d] idle\n",
+ idx, idx_abs);
}
}
drm_printf(m, "\t\tHW active? 0x%x\n", execlists->active);
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 912ff143d531..615782744093 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -697,16 +697,18 @@ static void guc_submit(struct intel_engine_cs *engine)
{
struct intel_guc *guc = &engine->i915->guc;
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port *port = execlists->port;
unsigned int n;
for (n = 0; n < execlists_num_ports(execlists); n++) {
+ struct execlist_port *port;
struct drm_i915_gem_request *rq;
unsigned int count;
- rq = port_unpack(&port[n], &count);
+ port = execlists_port(execlists, n);
+ rq = port_unpack(port, &count);
+
if (rq && count == 0) {
- port_set(&port[n], port_pack(rq, ++count));
+ port_set(port, port_pack(rq, ++count));
flush_ggtt_writes(rq->ring->vma);
@@ -715,6 +717,22 @@ static void guc_submit(struct intel_engine_cs *engine)
}
}
+static struct execlist_port *
+port_find_first_unset(struct intel_engine_execlists * const execlists,
+ struct execlist_port * const prev)
+{
+ struct execlist_port *port = execlists_port_next(execlists, prev);
+
+ while (port != prev) {
+ if (!port_isset(port))
+ return port;
+
+ port = execlists_port_next(execlists, port);
+ }
+
+ return NULL;
+}
+
static void port_assign(struct execlist_port *port,
struct drm_i915_gem_request *rq)
{
@@ -726,10 +744,8 @@ static void port_assign(struct execlist_port *port,
static void guc_dequeue(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port *port = execlists->port;
+ struct execlist_port *port, *last_port;
struct drm_i915_gem_request *last = NULL;
- const struct execlist_port * const last_port =
- &execlists->port[execlists->port_mask];
bool submit = false;
struct rb_node *rb;
@@ -740,6 +756,9 @@ static void guc_dequeue(struct intel_engine_cs *engine)
if (!rb)
goto unlock;
+ port = execlists_port_head(execlists);
+ last_port = execlists_port_tail(execlists);
+
if (port_isset(port)) {
if (HAS_LOGICAL_RING_PREEMPTION(engine->i915)) {
struct guc_preempt_work *preempt_work =
@@ -755,8 +774,8 @@ static void guc_dequeue(struct intel_engine_cs *engine)
}
}
- port++;
- if (port_isset(port))
+ port = port_find_first_unset(execlists, port);
+ if (!port)
goto unlock;
}
GEM_BUG_ON(port_isset(port));
@@ -775,7 +794,9 @@ static void guc_dequeue(struct intel_engine_cs *engine)
if (submit)
port_assign(port, last);
- port++;
+
+ port = execlists_port_next(execlists, port);
+ GEM_BUG_ON(port_isset(port));
}
INIT_LIST_HEAD(&rq->priotree.link);
@@ -804,29 +825,37 @@ static void guc_dequeue(struct intel_engine_cs *engine)
spin_unlock_irq(&engine->timeline->lock);
}
-static void guc_submission_tasklet(unsigned long data)
+static void guc_complete_ready_ports(struct intel_engine_execlists *execlists)
{
- struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
- struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port *port = execlists->port;
- struct drm_i915_gem_request *rq;
+ struct execlist_port *port = execlists_port_head(execlists);
+
+ while (port_isset(port)) {
+ struct drm_i915_gem_request *rq = port_request(port);
+
+ if (!i915_gem_request_completed(rq))
+ break;
- rq = port_request(&port[0]);
- while (rq && i915_gem_request_completed(rq)) {
trace_i915_gem_request_out(rq);
i915_gem_request_put(rq);
- execlists_port_complete(execlists, port);
+ port = execlists_head_complete(execlists, port);
+ };
- rq = port_request(&port[0]);
- }
- if (!rq)
+ if (!port_isset(port))
execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER);
+}
+
+static void guc_submission_tasklet(unsigned long data)
+{
+ struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
+
+ guc_complete_ready_ports(execlists);
if (execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT) &&
intel_read_status_page(engine, I915_GEM_HWS_PREEMPT_INDEX) ==
GUC_PREEMPT_FINISHED) {
- execlists_cancel_port_requests(&engine->execlists);
+ execlists_cancel_port_requests(execlists);
execlists_unwind_incomplete_requests(execlists);
wait_for_guc_preempt_report(engine);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2a8160f603ab..bc839729a78c 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -430,24 +430,27 @@ static inline void elsp_write(u64 desc, u32 __iomem *elsp)
static void execlists_submit_ports(struct intel_engine_cs *engine)
{
- struct execlist_port *port = engine->execlists.port;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
u32 __iomem *elsp =
engine->i915->regs + i915_mmio_reg_offset(RING_ELSP(engine));
unsigned int n;
- for (n = execlists_num_ports(&engine->execlists); n--; ) {
+ for (n = execlists_num_ports(execlists); n--; ) {
+ struct execlist_port *port;
struct drm_i915_gem_request *rq;
unsigned int count;
u64 desc;
- rq = port_unpack(&port[n], &count);
+ port = execlists_port(execlists, n);
+ rq = port_unpack(port, &count);
if (rq) {
GEM_BUG_ON(count > !n);
if (!count++)
execlists_context_schedule_in(rq);
- port_set(&port[n], port_pack(rq, count));
+
+ port_set(port, port_pack(rq, count));
desc = execlists_update_context(rq);
- GEM_DEBUG_EXEC(port[n].context_id = upper_32_bits(desc));
+ GEM_DEBUG_EXEC(port->context_id = upper_32_bits(desc));
GEM_TRACE("%s in[%d]: ctx=%d.%d, seqno=%x\n",
engine->name, n,
@@ -519,10 +522,8 @@ static void inject_preempt_context(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;
- const struct execlist_port * const last_port =
- &execlists->port[execlists->port_mask];
- struct drm_i915_gem_request *last = port_request(port);
+ struct execlist_port *port, *last_port;
+ struct drm_i915_gem_request *last;
struct rb_node *rb;
bool submit = false;
@@ -553,6 +554,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
if (!rb)
goto unlock;
+ port = execlists_port_head(execlists);
+ last = port_request(port);
+
if (last) {
/*
* Don't resubmit or switch until all outstanding
@@ -560,8 +564,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
* know the next preemption status we see corresponds
* to this ELSP update.
*/
- GEM_BUG_ON(!port_count(&port[0]));
- if (port_count(&port[0]) > 1)
+ GEM_BUG_ON(!port_count(port));
+ if (port_count(port) > 1)
goto unlock;
/*
@@ -606,7 +610,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
* the driver is unable to keep up the supply of new
* work).
*/
- if (port_count(&port[1]))
+ if (port_count(execlists_port_next(execlists, port)))
goto unlock;
/* WaIdleLiteRestore:bdw,skl
@@ -620,6 +624,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
}
}
+ last_port = execlists_port_tail(execlists);
+
do {
struct i915_priolist *p = rb_entry(rb, typeof(*p), node);
struct drm_i915_gem_request *rq, *rn;
@@ -666,8 +672,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
if (submit)
port_assign(port, last);
- port++;
+ port = execlists_port_next(execlists, port);
GEM_BUG_ON(port_isset(port));
}
@@ -700,20 +706,21 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
void
execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
{
- struct execlist_port *port = execlists->port;
unsigned int num_ports = execlists_num_ports(execlists);
+ struct execlist_port *port;
- while (num_ports-- && port_isset(port)) {
+ for (port = execlists_port_head(execlists);
+ num_ports-- && port_isset(port);
+ port = execlists_head_complete(execlists, port)) {
struct drm_i915_gem_request *rq = port_request(port);
GEM_BUG_ON(!execlists->active);
intel_engine_context_out(rq->engine);
execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED);
i915_gem_request_put(rq);
-
- memset(port, 0, sizeof(*port));
- port++;
}
+
+ GEM_BUG_ON(port_isset(execlists_port_head(execlists)));
}
static void execlists_cancel_requests(struct intel_engine_cs *engine)
@@ -780,7 +787,6 @@ static void execlists_submission_tasklet(unsigned long data)
{
struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port * const port = execlists->port;
struct drm_i915_private *dev_priv = engine->i915;
/* We can skip acquiring intel_runtime_pm_get() here as it was taken
@@ -799,6 +805,8 @@ static void execlists_submission_tasklet(unsigned long data)
* new request (outside of the context-switch interrupt).
*/
while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) {
+ struct execlist_port *port;
+
/* The HWSP contains a (cacheable) mirror of the CSB */
const u32 *buf =
&engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
@@ -839,6 +847,8 @@ static void execlists_submission_tasklet(unsigned long data)
head, GEN8_CSB_READ_PTR(readl(dev_priv->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))),
tail, GEN8_CSB_WRITE_PTR(readl(dev_priv->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))));
+ port = execlists_port_head(execlists);
+
while (head != tail) {
struct drm_i915_gem_request *rq;
unsigned int status;
@@ -914,14 +924,14 @@ static void execlists_submission_tasklet(unsigned long data)
GEM_BUG_ON(count == 0);
if (--count == 0) {
GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);
- GEM_BUG_ON(port_isset(&port[1]) &&
+ GEM_BUG_ON(port_isset(execlists_port(execlists, 1)) &&
!(status & GEN8_CTX_STATUS_ELEMENT_SWITCH));
GEM_BUG_ON(!i915_gem_request_completed(rq));
execlists_context_schedule_out(rq);
trace_i915_gem_request_out(rq);
i915_gem_request_put(rq);
- execlists_port_complete(execlists, port);
+ port = execlists_head_complete(execlists, port);
} else {
port_set(port, port_pack(rq, count));
}
@@ -961,6 +971,7 @@ static void insert_request(struct intel_engine_cs *engine,
static void execlists_submit_request(struct drm_i915_gem_request *request)
{
struct intel_engine_cs *engine = request->engine;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
unsigned long flags;
/* Will be called from irq-context when using foreign fences. */
@@ -968,7 +979,7 @@ static void execlists_submit_request(struct drm_i915_gem_request *request)
insert_request(engine, &request->priotree, request->priotree.priority);
- GEM_BUG_ON(!engine->execlists.first);
+ GEM_BUG_ON(!execlists->first);
GEM_BUG_ON(list_empty(&request->priotree.link));
spin_unlock_irqrestore(&engine->timeline->lock, flags);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index c68ab3ead83c..17f1fb4ded89 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -251,6 +251,11 @@ struct intel_engine_execlists {
unsigned int port_mask;
/**
+ * @port_head: first used execlist port
+ */
+ unsigned int port_head;
+
+ /**
* @queue: queue of requests, in priority lists
*/
struct rb_root queue;
@@ -643,8 +648,41 @@ execlists_num_ports(const struct intel_engine_execlists * const execlists)
return execlists->port_mask + 1;
}
-static inline void
-execlists_port_complete(struct intel_engine_execlists * const execlists,
+#define __port_add(start, n, mask) (((start) + (n)) & (mask))
+#define port_head_add(e, n) __port_add((e)->port_head, n, (e)->port_mask)
+
+/* Index starting from port_head */
+static inline struct execlist_port *
+execlists_port(struct intel_engine_execlists * const execlists,
+ const unsigned int n)
+{
+ return &execlists->port[port_head_add(execlists, n)];
+}
+
+static inline struct execlist_port *
+execlists_port_head(struct intel_engine_execlists * const execlists)
+{
+ return execlists_port(execlists, 0);
+}
+
+static inline struct execlist_port *
+execlists_port_tail(struct intel_engine_execlists * const execlists)
+{
+ return execlists_port(execlists, -1);
+}
+
+static inline struct execlist_port *
+execlists_port_next(struct intel_engine_execlists * const execlists,
+ struct execlist_port *port)
+{
+ if (port++ == execlists->port + execlists->port_mask)
+ port = execlists->port;
+
+ return port;
+}
+
+static inline struct execlist_port *
+execlists_head_complete(struct intel_engine_execlists * const execlists,
struct execlist_port * const port)
{
const unsigned int m = execlists->port_mask;
@@ -654,6 +692,8 @@ execlists_port_complete(struct intel_engine_execlists * const execlists,
memmove(port, port + 1, m * sizeof(struct execlist_port));
memset(port + m, 0, sizeof(struct execlist_port));
+
+ return execlists_port_head(execlists);
}
static inline unsigned int
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-10-20 11:12 ` Mika Kuoppala
2017-10-20 11:26 ` Chris Wilson
@ 2017-10-20 12:53 ` Mika Kuoppala
1 sibling, 0 replies; 25+ messages in thread
From: Mika Kuoppala @ 2017-10-20 12:53 UTC (permalink / raw)
To: Joonas Lahtinen, intel-gfx
Mika Kuoppala <mika.kuoppala@linux.intel.com> writes:
> Joonas Lahtinen <joonas.lahtinen@linux.intel.com> writes:
>
>> On Thu, 2017-10-19 at 17:39 +0300, Mika Kuoppala wrote:
>>> From: Mika Kuoppala <mika.kuoppala@intel.com>
>>>
>>> Instead of trusting that first available port is at index 0,
>>> use accessor to hide this. This is a preparation for a
>>> following patches where head can be at arbitrary location
>>> in the port array.
>>>
>>> v2: improved commit message, elsp_ready readability (Chris)
>>> v3: s/execlist_port_index/execlist_port (Chris)
>>> v4: rebase to new naming
>>> v5: fix port_next indexing
>>>
>>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>
>> <SNIP>
>>
>>> @@ -561,15 +563,20 @@ static void port_assign(struct execlist_port *port,
>>> static void i915_guc_dequeue(struct intel_engine_cs *engine)
>>> {
>>> struct intel_engine_execlists * const execlists = &engine->execlists;
>>> - struct execlist_port *port = execlists->port;
>>> + struct execlist_port *port;
>>> struct drm_i915_gem_request *last = NULL;
>>> - const struct execlist_port * const last_port =
>>> - &execlists->port[execlists->port_mask];
>>> bool submit = false;
>>> struct rb_node *rb;
>>>
>>> - if (port_isset(port))
>>> - port++;
>>> + port = execlists_port_head(execlists);
>>> +
>>> + /*
>>> + * We don't coalesce into last submitted port with guc.
>>> + * Find first free port, this is safe as we dont dequeue without
>>> + * atleast last port free.
>>
>> "at least" + "the"
>>
>> <SNIP>
>>
>>> @@ -557,6 +557,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>>> if (!rb)
>>> goto unlock;
>>>
>>> + port = execlists_port_head(execlists);
>>> + last = port_request(port);
>>> +
>>> if (last) {
>>> /*
>>> * Don't resubmit or switch until all outstanding
>>> @@ -564,7 +567,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>>> * know the next preemption status we see corresponds
>>> * to this ELSP update.
>>> */
>>> - if (port_count(&port[0]) > 1)
>>> + if (port_count(port) > 1)
>>> goto unlock;
>>>
>>> if (can_preempt(engine) &&
>>> @@ -598,7 +601,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>>> * the driver is unable to keep up the supply of new
>>> * work).
>>> */
>>> - if (port_count(&port[1]))
>>> + if (port_count(execlists_port_next(execlists, port)))
>>> goto unlock;
>>>
>>> /* WaIdleLiteRestore:bdw,skl
>>> @@ -634,7 +637,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>>> * combine this request with the last, then we
>>> * are done.
>>> */
>>> - if (port == last_port) {
>>> + if (port == execlists_port_tail(execlists)) {
>>> __list_del_many(&p->requests,
>>> &rq->priotree.link);
>>
>> Nothing to fix related to this patch, but I was sure next hunk was
>> going to escape my screen :) Maybe we need to cut the indents a bit.
>>
>
> I have noticed the same. But I didn't feel like attacking this loop
> until everything is in place and working.
>
>>> @@ -890,7 +902,7 @@ static void intel_lrc_irq_handler(unsigned long data)
>>> }
>>>
>>> /* After the final element, the hw should be idle */
>>> - GEM_BUG_ON(port_count(port) == 0 &&
>>> + GEM_BUG_ON(port_count(execlists_port_head(execlists)) == 0 &&
>>> !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
>>
>> Why did you stop trusting port variable here?
>>
>
> We did assing it pre loop before. Now we do it inside the loop. Also
> I thought I made a favour for reader (and for the bug triager
> as GEM_BUG_ON might soon show condition in dmesg)
> to note that it is always the first port count we assert
> for idleness.
My apologies. Now rereading this it is indeed that last port
count we need to check for hw idleness.
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-10-19 14:48 ` Chris Wilson
@ 2017-10-20 12:00 ` Mika Kuoppala
0 siblings, 0 replies; 25+ messages in thread
From: Mika Kuoppala @ 2017-10-20 12:00 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Quoting Mika Kuoppala (2017-10-19 15:39:41)
>> while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) {
>> + struct execlist_port *port;
>> +
>> /* The HWSP contains a (cacheable) mirror of the CSB */
>> const u32 *buf =
>> &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
>> @@ -855,7 +865,7 @@ static void intel_lrc_irq_handler(unsigned long data)
>>
>> if (status & GEN8_CTX_STATUS_ACTIVE_IDLE &&
>> buf[2*head + 1] == PREEMPT_ID) {
>> - execlist_cancel_port_requests(execlists);
>> + execlists_cancel_port_requests(execlists);
>>
>> spin_lock_irq(&engine->timeline->lock);
>> unwind_incomplete_requests(engine);
>> @@ -870,6 +880,8 @@ static void intel_lrc_irq_handler(unsigned long data)
>> execlists->preempt)
>> continue;
>>
>> + port = execlists_port_head(execlists);
>> +
>> /* Check the context/desc id for this event matches */
>> GEM_DEBUG_BUG_ON(buf[2 * head + 1] != port->context_id);
>>
>> @@ -890,7 +902,7 @@ static void intel_lrc_irq_handler(unsigned long data)
>> }
>>
>> /* After the final element, the hw should be idle */
>> - GEM_BUG_ON(port_count(port) == 0 &&
>> + GEM_BUG_ON(port_count(execlists_port_head(execlists)) == 0 &&
>> !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
>> }
>
> Can you try reworking this such that port is kept local without having
> to go back to the struct on every loop? And then compare code
> generation.
New one is with the last_port introduce and set outside loop.
add/remove: 0/0 grow/shrink: 1/1 up/down: 10/-7 (3)
function old new delta
intel_lrc_irq_handler 1847 1857 +10
i915_guc_irq_handler 1327 1320 -7
Without looking at the assembler, I would go with last_port
as a net win.
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-10-20 11:12 ` Mika Kuoppala
@ 2017-10-20 11:26 ` Chris Wilson
2017-10-20 12:53 ` Mika Kuoppala
1 sibling, 0 replies; 25+ messages in thread
From: Chris Wilson @ 2017-10-20 11:26 UTC (permalink / raw)
To: Mika Kuoppala, Joonas Lahtinen, intel-gfx
Quoting Mika Kuoppala (2017-10-20 12:12:02)
> Joonas Lahtinen <joonas.lahtinen@linux.intel.com> writes:
> >> @@ -890,7 +902,7 @@ static void intel_lrc_irq_handler(unsigned long data)
> >> }
> >>
> >> /* After the final element, the hw should be idle */
> >> - GEM_BUG_ON(port_count(port) == 0 &&
> >> + GEM_BUG_ON(port_count(execlists_port_head(execlists)) == 0 &&
> >> !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
> >
> > Why did you stop trusting port variable here?
> >
>
> We did assing it pre loop before. Now we do it inside the loop. Also
> I thought I made a favour for reader (and for the bug triager
> as GEM_BUG_ON might soon show condition in dmesg)
> to note that it is always the first port count we assert
> for idleness.
I would favour assert(port == el_port_head(el)); where appropriate.
The intent is that we comment upon the rationale of the GEM_BUG_ON() so
that we have an aide-memoire as to where to begin the hunt. For
triaging, we just need to be able to recognise the same BUG_ON as you
only need to dupe to the first one to catch up with the bug analysis.
We should err on making GEM_BUG_ON() fit the code better than a
potential bug report. We should be reading the code far more often...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-10-20 10:34 ` Joonas Lahtinen
@ 2017-10-20 11:12 ` Mika Kuoppala
2017-10-20 11:26 ` Chris Wilson
2017-10-20 12:53 ` Mika Kuoppala
0 siblings, 2 replies; 25+ messages in thread
From: Mika Kuoppala @ 2017-10-20 11:12 UTC (permalink / raw)
To: Joonas Lahtinen, intel-gfx
Joonas Lahtinen <joonas.lahtinen@linux.intel.com> writes:
> On Thu, 2017-10-19 at 17:39 +0300, Mika Kuoppala wrote:
>> From: Mika Kuoppala <mika.kuoppala@intel.com>
>>
>> Instead of trusting that first available port is at index 0,
>> use accessor to hide this. This is a preparation for a
>> following patches where head can be at arbitrary location
>> in the port array.
>>
>> v2: improved commit message, elsp_ready readability (Chris)
>> v3: s/execlist_port_index/execlist_port (Chris)
>> v4: rebase to new naming
>> v5: fix port_next indexing
>>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> <SNIP>
>
>> @@ -561,15 +563,20 @@ static void port_assign(struct execlist_port *port,
>> static void i915_guc_dequeue(struct intel_engine_cs *engine)
>> {
>> struct intel_engine_execlists * const execlists = &engine->execlists;
>> - struct execlist_port *port = execlists->port;
>> + struct execlist_port *port;
>> struct drm_i915_gem_request *last = NULL;
>> - const struct execlist_port * const last_port =
>> - &execlists->port[execlists->port_mask];
>> bool submit = false;
>> struct rb_node *rb;
>>
>> - if (port_isset(port))
>> - port++;
>> + port = execlists_port_head(execlists);
>> +
>> + /*
>> + * We don't coalesce into last submitted port with guc.
>> + * Find first free port, this is safe as we dont dequeue without
>> + * atleast last port free.
>
> "at least" + "the"
>
> <SNIP>
>
>> @@ -557,6 +557,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>> if (!rb)
>> goto unlock;
>>
>> + port = execlists_port_head(execlists);
>> + last = port_request(port);
>> +
>> if (last) {
>> /*
>> * Don't resubmit or switch until all outstanding
>> @@ -564,7 +567,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>> * know the next preemption status we see corresponds
>> * to this ELSP update.
>> */
>> - if (port_count(&port[0]) > 1)
>> + if (port_count(port) > 1)
>> goto unlock;
>>
>> if (can_preempt(engine) &&
>> @@ -598,7 +601,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>> * the driver is unable to keep up the supply of new
>> * work).
>> */
>> - if (port_count(&port[1]))
>> + if (port_count(execlists_port_next(execlists, port)))
>> goto unlock;
>>
>> /* WaIdleLiteRestore:bdw,skl
>> @@ -634,7 +637,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>> * combine this request with the last, then we
>> * are done.
>> */
>> - if (port == last_port) {
>> + if (port == execlists_port_tail(execlists)) {
>> __list_del_many(&p->requests,
>> &rq->priotree.link);
>
> Nothing to fix related to this patch, but I was sure next hunk was
> going to escape my screen :) Maybe we need to cut the indents a bit.
>
I have noticed the same. But I didn't feel like attacking this loop
until everything is in place and working.
>> @@ -890,7 +902,7 @@ static void intel_lrc_irq_handler(unsigned long data)
>> }
>>
>> /* After the final element, the hw should be idle */
>> - GEM_BUG_ON(port_count(port) == 0 &&
>> + GEM_BUG_ON(port_count(execlists_port_head(execlists)) == 0 &&
>> !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
>
> Why did you stop trusting port variable here?
>
We did assing it pre loop before. Now we do it inside the loop. Also
I thought I made a favour for reader (and for the bug triager
as GEM_BUG_ON might soon show condition in dmesg)
to note that it is always the first port count we assert
for idleness.
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-10-19 14:39 Mika Kuoppala
2017-10-19 14:48 ` Chris Wilson
2017-10-19 14:50 ` Chris Wilson
@ 2017-10-20 10:34 ` Joonas Lahtinen
2017-10-20 11:12 ` Mika Kuoppala
2 siblings, 1 reply; 25+ messages in thread
From: Joonas Lahtinen @ 2017-10-20 10:34 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx
On Thu, 2017-10-19 at 17:39 +0300, Mika Kuoppala wrote:
> From: Mika Kuoppala <mika.kuoppala@intel.com>
>
> Instead of trusting that first available port is at index 0,
> use accessor to hide this. This is a preparation for a
> following patches where head can be at arbitrary location
> in the port array.
>
> v2: improved commit message, elsp_ready readability (Chris)
> v3: s/execlist_port_index/execlist_port (Chris)
> v4: rebase to new naming
> v5: fix port_next indexing
>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
<SNIP>
> @@ -561,15 +563,20 @@ static void port_assign(struct execlist_port *port,
> static void i915_guc_dequeue(struct intel_engine_cs *engine)
> {
> struct intel_engine_execlists * const execlists = &engine->execlists;
> - struct execlist_port *port = execlists->port;
> + struct execlist_port *port;
> struct drm_i915_gem_request *last = NULL;
> - const struct execlist_port * const last_port =
> - &execlists->port[execlists->port_mask];
> bool submit = false;
> struct rb_node *rb;
>
> - if (port_isset(port))
> - port++;
> + port = execlists_port_head(execlists);
> +
> + /*
> + * We don't coalesce into last submitted port with guc.
> + * Find first free port, this is safe as we dont dequeue without
> + * atleast last port free.
"at least" + "the"
<SNIP>
> @@ -557,6 +557,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> if (!rb)
> goto unlock;
>
> + port = execlists_port_head(execlists);
> + last = port_request(port);
> +
> if (last) {
> /*
> * Don't resubmit or switch until all outstanding
> @@ -564,7 +567,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> * know the next preemption status we see corresponds
> * to this ELSP update.
> */
> - if (port_count(&port[0]) > 1)
> + if (port_count(port) > 1)
> goto unlock;
>
> if (can_preempt(engine) &&
> @@ -598,7 +601,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> * the driver is unable to keep up the supply of new
> * work).
> */
> - if (port_count(&port[1]))
> + if (port_count(execlists_port_next(execlists, port)))
> goto unlock;
>
> /* WaIdleLiteRestore:bdw,skl
> @@ -634,7 +637,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> * combine this request with the last, then we
> * are done.
> */
> - if (port == last_port) {
> + if (port == execlists_port_tail(execlists)) {
> __list_del_many(&p->requests,
> &rq->priotree.link);
Nothing to fix related to this patch, but I was sure next hunk was
going to escape my screen :) Maybe we need to cut the indents a bit.
> @@ -890,7 +902,7 @@ static void intel_lrc_irq_handler(unsigned long data)
> }
>
> /* After the final element, the hw should be idle */
> - GEM_BUG_ON(port_count(port) == 0 &&
> + GEM_BUG_ON(port_count(execlists_port_head(execlists)) == 0 &&
> !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
Why did you stop trusting port variable here?
Other than that, looks good to me.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-10-19 14:39 Mika Kuoppala
2017-10-19 14:48 ` Chris Wilson
@ 2017-10-19 14:50 ` Chris Wilson
2017-10-20 10:34 ` Joonas Lahtinen
2 siblings, 0 replies; 25+ messages in thread
From: Chris Wilson @ 2017-10-19 14:50 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx; +Cc: Mika
Quoting Mika Kuoppala (2017-10-19 15:39:41)
> +#define __port_add(start, n, mask) (((start) + (n)) & (mask))
> +#define port_head_add(e, n) __port_add((e)->port_head, n, (e)->port_mask)
> +
> +/* Index starting from port_head */
> +static inline struct execlist_port *
> +execlists_port(struct intel_engine_execlists * const execlists,
> + const unsigned int n)
> +{
> + return &execlists->port[port_head_add(execlists, n)];
nth_port cf nth_page?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
2017-10-19 14:39 Mika Kuoppala
@ 2017-10-19 14:48 ` Chris Wilson
2017-10-20 12:00 ` Mika Kuoppala
2017-10-19 14:50 ` Chris Wilson
2017-10-20 10:34 ` Joonas Lahtinen
2 siblings, 1 reply; 25+ messages in thread
From: Chris Wilson @ 2017-10-19 14:48 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx; +Cc: Mika
Quoting Mika Kuoppala (2017-10-19 15:39:41)
> while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) {
> + struct execlist_port *port;
> +
> /* The HWSP contains a (cacheable) mirror of the CSB */
> const u32 *buf =
> &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
> @@ -855,7 +865,7 @@ static void intel_lrc_irq_handler(unsigned long data)
>
> if (status & GEN8_CTX_STATUS_ACTIVE_IDLE &&
> buf[2*head + 1] == PREEMPT_ID) {
> - execlist_cancel_port_requests(execlists);
> + execlists_cancel_port_requests(execlists);
>
> spin_lock_irq(&engine->timeline->lock);
> unwind_incomplete_requests(engine);
> @@ -870,6 +880,8 @@ static void intel_lrc_irq_handler(unsigned long data)
> execlists->preempt)
> continue;
>
> + port = execlists_port_head(execlists);
> +
> /* Check the context/desc id for this event matches */
> GEM_DEBUG_BUG_ON(buf[2 * head + 1] != port->context_id);
>
> @@ -890,7 +902,7 @@ static void intel_lrc_irq_handler(unsigned long data)
> }
>
> /* After the final element, the hw should be idle */
> - GEM_BUG_ON(port_count(port) == 0 &&
> + GEM_BUG_ON(port_count(execlists_port_head(execlists)) == 0 &&
> !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
> }
Can you try reworking this such that port is kept local without having
to go back to the struct on every loop? And then compare code
generation.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors
@ 2017-10-19 14:39 Mika Kuoppala
2017-10-19 14:48 ` Chris Wilson
` (2 more replies)
0 siblings, 3 replies; 25+ messages in thread
From: Mika Kuoppala @ 2017-10-19 14:39 UTC (permalink / raw)
To: intel-gfx
From: Mika Kuoppala <mika.kuoppala@intel.com>
Instead of trusting that first available port is at index 0,
use accessor to hide this. This is a preparation for a
following patches where head can be at arbitrary location
in the port array.
v2: improved commit message, elsp_ready readability (Chris)
v3: s/execlist_port_index/execlist_port (Chris)
v4: rebase to new naming
v5: fix port_next indexing
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
drivers/gpu/drm/i915/i915_gpu_error.c | 6 ++-
drivers/gpu/drm/i915/i915_guc_submission.c | 53 +++++++++++++++----------
drivers/gpu/drm/i915/intel_engine_cs.c | 2 +-
drivers/gpu/drm/i915/intel_lrc.c | 63 ++++++++++++++++++------------
drivers/gpu/drm/i915/intel_ringbuffer.h | 38 ++++++++++++++++++
5 files changed, 114 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 653fb69e7ecb..6d0bdb03b3f0 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1333,11 +1333,13 @@ static void engine_record_requests(struct intel_engine_cs *engine,
static void error_record_engine_execlists(struct intel_engine_cs *engine,
struct drm_i915_error_engine *ee)
{
- const struct intel_engine_execlists * const execlists = &engine->execlists;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
unsigned int n;
for (n = 0; n < execlists_num_ports(execlists); n++) {
- struct drm_i915_gem_request *rq = port_request(&execlists->port[n]);
+ struct drm_i915_gem_request *rq;
+
+ rq = port_request(execlists_port(execlists, n));
if (!rq)
break;
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index a2e8114b739d..5222004db039 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -496,17 +496,19 @@ static void i915_guc_submit(struct intel_engine_cs *engine)
struct intel_guc *guc = &dev_priv->guc;
struct i915_guc_client *client = guc->execbuf_client;
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port *port = execlists->port;
const unsigned int engine_id = engine->id;
unsigned int n;
for (n = 0; n < execlists_num_ports(execlists); n++) {
+ struct execlist_port *port;
struct drm_i915_gem_request *rq;
unsigned int count;
- rq = port_unpack(&port[n], &count);
+ port = execlists_port(execlists, n);
+ rq = port_unpack(port, &count);
+
if (rq && count == 0) {
- port_set(&port[n], port_pack(rq, ++count));
+ port_set(port, port_pack(rq, ++count));
if (i915_vma_is_map_and_fenceable(rq->ring->vma))
POSTING_READ_FW(GUC_STATUS);
@@ -561,15 +563,20 @@ static void port_assign(struct execlist_port *port,
static void i915_guc_dequeue(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port *port = execlists->port;
+ struct execlist_port *port;
struct drm_i915_gem_request *last = NULL;
- const struct execlist_port * const last_port =
- &execlists->port[execlists->port_mask];
bool submit = false;
struct rb_node *rb;
- if (port_isset(port))
- port++;
+ port = execlists_port_head(execlists);
+
+ /*
+ * We don't coalesce into last submitted port with guc.
+ * Find first free port, this is safe as we dont dequeue without
+ * atleast last port free.
+ */
+ while (port_isset(port))
+ port = execlists_port_next(execlists, port);
spin_lock_irq(&engine->timeline->lock);
rb = execlists->first;
@@ -580,7 +587,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
list_for_each_entry_safe(rq, rn, &p->requests, priotree.link) {
if (last && rq->ctx != last->ctx) {
- if (port == last_port) {
+ if (port == execlists_port_tail(execlists)) {
__list_del_many(&p->requests,
&rq->priotree.link);
goto done;
@@ -588,7 +595,8 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
if (submit)
port_assign(port, last);
- port++;
+
+ port = execlists_port_next(execlists, port);
}
INIT_LIST_HEAD(&rq->priotree.link);
@@ -619,22 +627,27 @@ static void i915_guc_irq_handler(unsigned long data)
{
struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port *port = execlists->port;
- const struct execlist_port * const last_port =
- &execlists->port[execlists->port_mask];
- struct drm_i915_gem_request *rq;
- rq = port_request(&port[0]);
- while (rq && i915_gem_request_completed(rq)) {
+ do {
+ struct execlist_port *port;
+ struct drm_i915_gem_request *rq;
+
+ port = execlists_port_head(execlists);
+ rq = port_request(port);
+
+ if (!rq)
+ break;
+
+ if (!i915_gem_request_completed(rq))
+ break;
+
trace_i915_gem_request_out(rq);
i915_gem_request_put(rq);
execlists_port_complete(execlists, port);
+ } while (1);
- rq = port_request(&port[0]);
- }
-
- if (!port_isset(last_port))
+ if (!port_isset(execlists_port_tail(execlists)))
i915_guc_dequeue(engine);
}
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index a47a9c6bea52..8ba62d4c010e 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1549,7 +1549,7 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
return false;
/* Both ports drained, no more ELSP submission? */
- if (port_request(&engine->execlists.port[0]))
+ if (port_request(execlists_port_head(&engine->execlists)))
return false;
/* ELSP is empty, but there are ready requests? */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 7f45dd7dc3e5..2945aadc4b7e 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -437,24 +437,26 @@ static inline void elsp_write(u64 desc, u32 __iomem *elsp)
static void execlists_submit_ports(struct intel_engine_cs *engine)
{
- struct execlist_port *port = engine->execlists.port;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
u32 __iomem *elsp =
engine->i915->regs + i915_mmio_reg_offset(RING_ELSP(engine));
unsigned int n;
- for (n = execlists_num_ports(&engine->execlists); n--; ) {
+ for (n = execlists_num_ports(execlists); n--; ) {
+ struct execlist_port *port;
struct drm_i915_gem_request *rq;
unsigned int count;
u64 desc;
- rq = port_unpack(&port[n], &count);
+ port = execlists_port(execlists, n);
+ rq = port_unpack(port, &count);
if (rq) {
GEM_BUG_ON(count > !n);
if (!count++)
execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
- port_set(&port[n], port_pack(rq, count));
+ port_set(port, port_pack(rq, count));
desc = execlists_update_context(rq);
- GEM_DEBUG_EXEC(port[n].context_id = upper_32_bits(desc));
+ GEM_DEBUG_EXEC(port->context_id = upper_32_bits(desc));
} else {
GEM_BUG_ON(!n);
desc = 0;
@@ -523,10 +525,8 @@ static bool can_preempt(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;
- const struct execlist_port * const last_port =
- &execlists->port[execlists->port_mask];
- struct drm_i915_gem_request *last = port_request(port);
+ struct execlist_port *port;
+ struct drm_i915_gem_request *last;
struct rb_node *rb;
bool submit = false;
@@ -557,6 +557,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
if (!rb)
goto unlock;
+ port = execlists_port_head(execlists);
+ last = port_request(port);
+
if (last) {
/*
* Don't resubmit or switch until all outstanding
@@ -564,7 +567,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
* know the next preemption status we see corresponds
* to this ELSP update.
*/
- if (port_count(&port[0]) > 1)
+ if (port_count(port) > 1)
goto unlock;
if (can_preempt(engine) &&
@@ -598,7 +601,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
* the driver is unable to keep up the supply of new
* work).
*/
- if (port_count(&port[1]))
+ if (port_count(execlists_port_next(execlists, port)))
goto unlock;
/* WaIdleLiteRestore:bdw,skl
@@ -634,7 +637,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
* combine this request with the last, then we
* are done.
*/
- if (port == last_port) {
+ if (port == execlists_port_tail(execlists)) {
__list_del_many(&p->requests,
&rq->priotree.link);
goto done;
@@ -658,7 +661,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
if (submit)
port_assign(port, last);
- port++;
+
+ port = execlists_port_next(execlists, port);
GEM_BUG_ON(port_isset(port));
}
@@ -688,19 +692,24 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
}
static void
-execlist_cancel_port_requests(struct intel_engine_execlists *execlists)
+execlists_cancel_port_requests(struct intel_engine_execlists *execlists)
{
- struct execlist_port *port = execlists->port;
unsigned int num_ports = execlists_num_ports(execlists);
- while (num_ports-- && port_isset(port)) {
- struct drm_i915_gem_request *rq = port_request(port);
+ while (num_ports--) {
+ struct execlist_port *port;
+ struct drm_i915_gem_request *rq;
+
+ port = execlists_port_head(execlists);
+ if (!port_isset(port))
+ break;
+
+ rq = port_request(port);
execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED);
i915_gem_request_put(rq);
- memset(port, 0, sizeof(*port));
- port++;
+ execlists_port_complete(execlists, port);
}
}
@@ -714,7 +723,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
spin_lock_irqsave(&engine->timeline->lock, flags);
/* Cancel the requests on the HW and clear the ELSP tracker. */
- execlist_cancel_port_requests(execlists);
+ execlists_cancel_port_requests(execlists);
/* Mark all executing requests as skipped. */
list_for_each_entry(rq, &engine->timeline->requests, link) {
@@ -769,7 +778,6 @@ static void intel_lrc_irq_handler(unsigned long data)
{
struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
struct intel_engine_execlists * const execlists = &engine->execlists;
- struct execlist_port * const port = execlists->port;
struct drm_i915_private *dev_priv = engine->i915;
/* We can skip acquiring intel_runtime_pm_get() here as it was taken
@@ -788,6 +796,8 @@ static void intel_lrc_irq_handler(unsigned long data)
* new request (outside of the context-switch interrupt).
*/
while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) {
+ struct execlist_port *port;
+
/* The HWSP contains a (cacheable) mirror of the CSB */
const u32 *buf =
&engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
@@ -855,7 +865,7 @@ static void intel_lrc_irq_handler(unsigned long data)
if (status & GEN8_CTX_STATUS_ACTIVE_IDLE &&
buf[2*head + 1] == PREEMPT_ID) {
- execlist_cancel_port_requests(execlists);
+ execlists_cancel_port_requests(execlists);
spin_lock_irq(&engine->timeline->lock);
unwind_incomplete_requests(engine);
@@ -870,6 +880,8 @@ static void intel_lrc_irq_handler(unsigned long data)
execlists->preempt)
continue;
+ port = execlists_port_head(execlists);
+
/* Check the context/desc id for this event matches */
GEM_DEBUG_BUG_ON(buf[2 * head + 1] != port->context_id);
@@ -890,7 +902,7 @@ static void intel_lrc_irq_handler(unsigned long data)
}
/* After the final element, the hw should be idle */
- GEM_BUG_ON(port_count(port) == 0 &&
+ GEM_BUG_ON(port_count(execlists_port_head(execlists)) == 0 &&
!(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
}
@@ -921,6 +933,7 @@ static void insert_request(struct intel_engine_cs *engine,
static void execlists_submit_request(struct drm_i915_gem_request *request)
{
struct intel_engine_cs *engine = request->engine;
+ struct intel_engine_execlists * const execlists = &engine->execlists;
unsigned long flags;
/* Will be called from irq-context when using foreign fences. */
@@ -928,7 +941,7 @@ static void execlists_submit_request(struct drm_i915_gem_request *request)
insert_request(engine, &request->priotree, request->priotree.priority);
- GEM_BUG_ON(!engine->execlists.first);
+ GEM_BUG_ON(!execlists->first);
GEM_BUG_ON(list_empty(&request->priotree.link));
spin_unlock_irqrestore(&engine->timeline->lock, flags);
@@ -1520,7 +1533,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
* guessing the missed context-switch events by looking at what
* requests were completed.
*/
- execlist_cancel_port_requests(execlists);
+ execlists_cancel_port_requests(execlists);
/* Push back any incomplete requests for replay after the reset. */
unwind_incomplete_requests(engine);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 17186f067408..cfec73400d0f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -251,6 +251,11 @@ struct intel_engine_execlists {
unsigned int port_mask;
/**
+ * @port_head: first used execlist port
+ */
+ unsigned int port_head;
+
+ /**
* @queue: queue of requests, in priority lists
*/
struct rb_root queue;
@@ -531,6 +536,39 @@ execlists_num_ports(const struct intel_engine_execlists * const execlists)
return execlists->port_mask + 1;
}
+#define __port_add(start, n, mask) (((start) + (n)) & (mask))
+#define port_head_add(e, n) __port_add((e)->port_head, n, (e)->port_mask)
+
+/* Index starting from port_head */
+static inline struct execlist_port *
+execlists_port(struct intel_engine_execlists * const execlists,
+ const unsigned int n)
+{
+ return &execlists->port[port_head_add(execlists, n)];
+}
+
+static inline struct execlist_port *
+execlists_port_head(struct intel_engine_execlists * const execlists)
+{
+ return execlists_port(execlists, 0);
+}
+
+static inline struct execlist_port *
+execlists_port_tail(struct intel_engine_execlists * const execlists)
+{
+ return execlists_port(execlists, -1);
+}
+
+static inline struct execlist_port *
+execlists_port_next(struct intel_engine_execlists * const execlists,
+ const struct execlist_port * const port)
+{
+ const unsigned int n = __port_add(port_index(port, execlists),
+ 1,
+ execlists->port_mask);
+ return &execlists->port[n];
+}
+
static inline void
execlists_port_complete(struct intel_engine_execlists * const execlists,
struct execlist_port * const port)
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 25+ messages in thread
end of thread, other threads:[~2017-11-30 10:21 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-31 15:27 [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors Mika Kuoppala
2017-10-31 15:27 ` [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array Mika Kuoppala
2017-10-31 15:42 ` Chris Wilson
2017-11-02 15:07 ` Chris Wilson
2017-11-22 13:52 ` Mika Kuoppala
2017-11-22 13:57 ` Chris Wilson
2017-10-31 15:41 ` [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors Chris Wilson
2017-10-31 15:56 ` Chris Wilson
2017-11-02 10:38 ` Mika Kuoppala
2017-11-02 10:57 ` Chris Wilson
2017-11-02 14:14 ` Mika Kuoppala
2017-11-02 14:15 ` Mika Kuoppala
2017-10-31 15:51 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] " Patchwork
2017-11-02 14:32 ` [PATCH 1/2] " Mika Kuoppala
2017-11-02 15:03 ` Chris Wilson
-- strict thread matches above, loose matches on Subject: below --
2017-11-30 9:10 [PATCH 0/2] execlist port handling improvements Mika Kuoppala
2017-11-30 9:10 ` [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors Mika Kuoppala
2017-11-30 10:21 ` Chris Wilson
2017-10-19 14:39 Mika Kuoppala
2017-10-19 14:48 ` Chris Wilson
2017-10-20 12:00 ` Mika Kuoppala
2017-10-19 14:50 ` Chris Wilson
2017-10-20 10:34 ` Joonas Lahtinen
2017-10-20 11:12 ` Mika Kuoppala
2017-10-20 11:26 ` Chris Wilson
2017-10-20 12:53 ` Mika Kuoppala
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.