[PATCH 1/2] drm/i915: Introduce execlist_port_* accessors

[PATCH 1/2] drm/i915: Introduce execlist_port_* accessors

[Mika Kuoppala]

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

[PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array

[Mika Kuoppala]

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.

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/intel_ringbuffer.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index cfec73400d0f..27f5c42ae7c8 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -573,12 +573,13 @@ static inline void
 execlists_port_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) != execlists->port_head);
+	GEM_BUG_ON(!port_isset(port));
 
-	GEM_BUG_ON(port_index(port, execlists) != 0);
+	port->request_count = NULL;
+	GEM_DEBUG_DECL(port->context_id = 0);
 
-	memmove(port, port + 1, m * sizeof(struct execlist_port));
-	memset(port + m, 0, sizeof(struct execlist_port));
+	execlists->port_head = port_head_add(execlists, 1);
 }
 
 static inline unsigned int
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors

[Chris Wilson]

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

Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors

[Chris Wilson]

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

✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Introduce execlist_port_* accessors

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Introduce execlist_port_* accessors
URL   : https://patchwork.freedesktop.org/series/32300/
State : warning

== Summary ==

Series 32300v1 series starting with [1/2] drm/i915: Introduce execlist_port_* accessors
https://patchwork.freedesktop.org/api/1.0/series/32300/revisions/1/mbox/

Test drv_module_reload:
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-skl-6770hq)

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:442s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:369s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:514s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:262s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:494s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:501s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:488s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:479s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:554s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:421s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:248s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:579s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:453s
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:433s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:491s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:462s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:482s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:575s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:474s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:588s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:546s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:448s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:645s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:516s
fi-skl-6770hq    total:289  pass:268  dwarn:1   dfail:0   fail:0   skip:20  time:506s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:460s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:556s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:420s
fi-bdw-gvtdvm failed to connect after reboot

93f001963c9915c52b5ad84500075d231e008ced drm-tip: 2017y-10m-19d-10h-52m-17s UTC integration manifest
7fb186a28a20 drm/i915: Move execlists port head instead of memmoving array
6b361fdad897 drm/i915: Introduce execlist_port_* accessors

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6104/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array

[Chris Wilson]

Quoting Mika Kuoppala (2017-10-19 15:39:42)
> 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.
> 
> 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/intel_ringbuffer.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index cfec73400d0f..27f5c42ae7c8 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -573,12 +573,13 @@ static inline void
>  execlists_port_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) != execlists->port_head);
> +       GEM_BUG_ON(!port_isset(port));
>  
> -       GEM_BUG_ON(port_index(port, execlists) != 0);
> +       port->request_count = NULL;
> +       GEM_DEBUG_DECL(port->context_id = 0);
memset(port, 0, sizeof(*port));

In my ring, we no longer needed the memset, but the tradeoff is tracking
the last_port.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors

[Joonas Lahtinen]

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

Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors

[Mika Kuoppala]

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

Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors

[Chris Wilson]

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

Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors

[Mika Kuoppala]

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

Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors

[Mika Kuoppala]

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

Re: [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array

[Chris Wilson]

Quoting Mika Kuoppala (2017-11-30 09:10:28)
> 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>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

This patch:
add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-58 (-58)
function                                     old     new   delta
execlists_cancel_port_requests               299     288     -11
guc_submission_tasklet                      1660    1643     -17
execlists_submission_tasklet                2245    2215     -30

Overall:
add/remove: 0/0 grow/shrink: 4/2 up/down: 322/-2 (320)
function                                     old     new   delta
guc_submission_tasklet                      1488    1643    +155
execlists_submission_tasklet                2129    2215     +86
intel_engine_dump                           2234    2281     +47
execlists_cancel_port_requests               254     288     +34
intel_engine_init_cmd_parser                1134    1133      -1
capture                                     5700    5699      -1

Better. Still room for improvement, 2215 bytes worth :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array

[Mika Kuoppala]

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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 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 17f1fb4ded89..6a3cb8e09ed6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -685,13 +685,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

Re: [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array

[Chris Wilson]

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

Re: [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array

[Mika Kuoppala]

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

Re: [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array

[Chris Wilson]

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

Re: [PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array

[Chris Wilson]

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

[PATCH 2/2] drm/i915: Move execlists port head instead of memmoving array

[Mika Kuoppala]

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