[PATCH v2 1/8] drm/i915: Remove misleading comment in request_alloc

[PATCH v2 1/8] drm/i915: Remove misleading comment in request_alloc

[Michał Winiarski]

Passing NULL ctx to request_alloc would lead to null-ptr-deref.

v2: Let's not replace the comment with a BUG_ON

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_request.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 1ccf252..0d1e0d8 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -538,9 +538,6 @@ submit_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state)
  *
  * @engine: engine that we wish to issue the request on.
  * @ctx: context that the request will be associated with.
- *       This can be NULL if the request is not directly related to
- *       any specific user context, in which case this function will
- *       choose an appropriate context to use.
  *
  * Returns a pointer to the allocated request if successful,
  * or an error code if not.
-- 
2.9.4

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

[PATCH 2/8] drm/i915/guc: Skip port assign on first iteration of GuC dequeue

[Michał Winiarski]

If port[0] is occupied and we're trying to dequeue request from
different context, we will inevitably hit BUG_ON in port_assign.
Let's skip it - similar to what we're doing in execlists counterpart.

Fixes: 77f0d0e925e8a0 ("drm/i915/execlists: Pack the count into the low bits of the port.request")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Wajdeczko <michal.wajdeczko@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 777f48e..e6e0c6e 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -681,7 +681,8 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
 					goto done;
 				}
 
-				port_assign(port, last);
+				if (submit)
+					port_assign(port, last);
 				port++;
 			}
 
-- 
2.9.4

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

[PATCH v3 3/8] drm/i915/guc: Submit GuC workitems containing coalesced requests

[Michał Winiarski]

To create an upper bound on number of GuC workitems, we need to change
the way that requests are being submitted. Rather than submitting each
request as an individual workitem, we can do coalescing in a similar way
we're handlig execlist submission ports. We also need to stop pretending
that we're doing "lite-restore" in GuC submission (we would create a
workitem each time we hit this condition).

v2: Also coalesce when replaying on reset (Daniele)
v3: Consistent wq_resv - per-request (Daniele)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 72 +++++++++++++++---------------
 1 file changed, 37 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index e6e0c6e..2a0c3161 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -491,14 +491,12 @@ static void guc_wq_item_append(struct i915_guc_client *client,
 	 * workqueue buffer dw by dw.
 	 */
 	BUILD_BUG_ON(wqi_size != 16);
-	GEM_BUG_ON(client->wq_rsvd < wqi_size);
 
 	/* postincrement WQ tail for next time */
 	wq_off = client->wq_tail;
 	GEM_BUG_ON(wq_off & (wqi_size - 1));
 	client->wq_tail += wqi_size;
 	client->wq_tail &= client->wq_size - 1;
-	client->wq_rsvd -= wqi_size;
 
 	/* WQ starts from the page after doorbell / process_desc */
 	wqi = client->vaddr + wq_off + GUC_DB_SIZE;
@@ -580,7 +578,7 @@ static int guc_ring_doorbell(struct i915_guc_client *client)
 }
 
 /**
- * __i915_guc_submit() - Submit commands through GuC
+ * i915_guc_submit() - Submit commands through GuC
  * @rq:		request associated with the commands
  *
  * The caller must have already called i915_guc_wq_reserve() above with
@@ -594,7 +592,7 @@ static int guc_ring_doorbell(struct i915_guc_client *client)
  * The only error here arises if the doorbell hardware isn't functioning
  * as expected, which really shouln't happen.
  */
-static void __i915_guc_submit(struct drm_i915_gem_request *rq)
+static void i915_guc_submit(struct drm_i915_gem_request *rq)
 {
 	struct drm_i915_private *dev_priv = rq->i915;
 	struct intel_engine_cs *engine = rq->engine;
@@ -618,12 +616,6 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq)
 	spin_unlock_irqrestore(&client->wq_lock, flags);
 }
 
-static void i915_guc_submit(struct drm_i915_gem_request *rq)
-{
-	__i915_gem_request_submit(rq);
-	__i915_guc_submit(rq);
-}
-
 static void nested_enable_signaling(struct drm_i915_gem_request *rq)
 {
 	/* If we use dma_fence_enable_sw_signaling() directly, lockdep
@@ -659,12 +651,15 @@ static void port_assign(struct execlist_port *port,
 	nested_enable_signaling(rq);
 }
 
-static bool i915_guc_dequeue(struct intel_engine_cs *engine)
+static void i915_guc_dequeue(struct intel_engine_cs *engine)
 {
 	struct execlist_port *port = engine->execlist_port;
-	struct drm_i915_gem_request *last = port_request(port);
-	struct rb_node *rb;
+	struct drm_i915_gem_request *last = NULL;
 	bool submit = false;
+	struct rb_node *rb;
+
+	if (port_request(port))
+		port++;
 
 	spin_lock_irq(&engine->timeline->lock);
 	rb = engine->execlist_first;
@@ -681,18 +676,22 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
 					goto done;
 				}
 
-				if (submit)
+				if (submit) {
 					port_assign(port, last);
+					i915_guc_submit(last);
+					submit = false;
+				}
 				port++;
 			}
 
 			INIT_LIST_HEAD(&rq->priotree.link);
 			rq->priotree.priority = INT_MAX;
 
-			i915_guc_submit(rq);
+			__i915_gem_request_submit(rq);
 			trace_i915_gem_request_in(rq, port_index(port, engine));
 			last = rq;
 			submit = true;
+			i915_guc_wq_unreserve(rq);
 		}
 
 		rb = rb_next(rb);
@@ -703,11 +702,11 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
 	}
 done:
 	engine->execlist_first = rb;
-	if (submit)
+	if (submit) {
 		port_assign(port, last);
+		i915_guc_submit(last);
+	}
 	spin_unlock_irq(&engine->timeline->lock);
-
-	return submit;
 }
 
 static void i915_guc_irq_handler(unsigned long data)
@@ -715,24 +714,20 @@ static void i915_guc_irq_handler(unsigned long data)
 	struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
 	struct execlist_port *port = engine->execlist_port;
 	struct drm_i915_gem_request *rq;
-	bool submit;
 
-	do {
-		rq = port_request(&port[0]);
-		while (rq && i915_gem_request_completed(rq)) {
-			trace_i915_gem_request_out(rq);
-			i915_gem_request_put(rq);
+	rq = port_request(&port[0]);
+	while (rq && i915_gem_request_completed(rq)) {
+		trace_i915_gem_request_out(rq);
+		i915_gem_request_put(rq);
 
-			port[0] = port[1];
-			memset(&port[1], 0, sizeof(port[1]));
+		port[0] = port[1];
+		memset(&port[1], 0, sizeof(port[1]));
 
-			rq = port_request(&port[0]);
-		}
+		rq = port_request(&port[0]);
+	}
 
-		submit = false;
-		if (!port_count(&port[1]))
-			submit = i915_guc_dequeue(engine);
-	} while (submit);
+	if (!port_isset(&port[1]))
+		i915_guc_dequeue(engine);
 }
 
 /*
@@ -1250,6 +1245,8 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 	for_each_engine(engine, dev_priv, id) {
 		const int wqi_size = sizeof(struct guc_wq_item);
 		struct drm_i915_gem_request *rq;
+		struct execlist_port *port = engine->execlist_port;
+		int n;
 
 		/* The tasklet was initialised by execlists, and may be in
 		 * a state of flux (across a reset) and so we just want to
@@ -1261,11 +1258,16 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 
 		/* Replay the current set of previously submitted requests */
 		spin_lock_irq(&engine->timeline->lock);
-		list_for_each_entry(rq, &engine->timeline->requests, link) {
+		list_for_each_entry(rq, &engine->timeline->requests, link)
 			guc_client_update_wq_rsvd(client, wqi_size);
-			__i915_guc_submit(rq);
-		}
 		spin_unlock_irq(&engine->timeline->lock);
+
+		for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) {
+			if (!port_isset(&port[n]))
+				break;
+
+			i915_guc_submit(port_request(&port[n]));
+		}
 	}
 
 	return 0;
-- 
2.9.4

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

[PATCH v3 4/8] drm/i915/guc: Remove GuC wq reservation

[Michał Winiarski]

Now that we have an upper bound on the number of work items being sent
to GuC, we can remove the reservation.

v2: Multiply by number of engines in compile time assert (Daniele)
v3: Comment on reasoning behind the compile time assert (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |  2 -
 drivers/gpu/drm/i915/i915_guc_submission.c | 89 +++++-------------------------
 drivers/gpu/drm/i915/intel_lrc.c           | 25 +--------
 drivers/gpu/drm/i915/intel_uc.h            |  8 ---
 4 files changed, 17 insertions(+), 107 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0969b8d..83366a4 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2481,8 +2481,6 @@ static void i915_guc_client_info(struct seq_file *m,
 	seq_printf(m, "\tWQ size %d, offset: 0x%x, tail %d\n",
 		client->wq_size, client->wq_offset, client->wq_tail);
 
-	seq_printf(m, "\tWork queue full: %u\n", client->no_wq_space);
-
 	for_each_engine(engine, dev_priv, id) {
 		u64 submissions = client->submissions[id];
 		tot += submissions;
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2a0c3161..66d8eaf 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -406,63 +406,6 @@ static void guc_stage_desc_fini(struct intel_guc *guc,
 	memset(desc, 0, sizeof(*desc));
 }
 
-/**
- * i915_guc_wq_reserve() - reserve space in the GuC's workqueue
- * @request:	request associated with the commands
- *
- * Return:	0 if space is available
- *		-EAGAIN if space is not currently available
- *
- * This function must be called (and must return 0) before a request
- * is submitted to the GuC via i915_guc_submit() below. Once a result
- * of 0 has been returned, it must be balanced by a corresponding
- * call to submit().
- *
- * Reservation allows the caller to determine in advance that space
- * will be available for the next submission before committing resources
- * to it, and helps avoid late failures with complicated recovery paths.
- */
-int i915_guc_wq_reserve(struct drm_i915_gem_request *request)
-{
-	const size_t wqi_size = sizeof(struct guc_wq_item);
-	struct i915_guc_client *client = request->i915->guc.execbuf_client;
-	struct guc_process_desc *desc = __get_process_desc(client);
-	u32 freespace;
-	int ret;
-
-	spin_lock_irq(&client->wq_lock);
-	freespace = CIRC_SPACE(client->wq_tail, desc->head, client->wq_size);
-	freespace -= client->wq_rsvd;
-	if (likely(freespace >= wqi_size)) {
-		client->wq_rsvd += wqi_size;
-		ret = 0;
-	} else {
-		client->no_wq_space++;
-		ret = -EAGAIN;
-	}
-	spin_unlock_irq(&client->wq_lock);
-
-	return ret;
-}
-
-static void guc_client_update_wq_rsvd(struct i915_guc_client *client, int size)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&client->wq_lock, flags);
-	client->wq_rsvd += size;
-	spin_unlock_irqrestore(&client->wq_lock, flags);
-}
-
-void i915_guc_wq_unreserve(struct drm_i915_gem_request *request)
-{
-	const int wqi_size = sizeof(struct guc_wq_item);
-	struct i915_guc_client *client = request->i915->guc.execbuf_client;
-
-	GEM_BUG_ON(READ_ONCE(client->wq_rsvd) < wqi_size);
-	guc_client_update_wq_rsvd(client, -wqi_size);
-}
-
 /* Construct a Work Item and append it to the GuC's Work Queue */
 static void guc_wq_item_append(struct i915_guc_client *client,
 			       struct drm_i915_gem_request *rq)
@@ -475,7 +418,7 @@ static void guc_wq_item_append(struct i915_guc_client *client,
 	struct guc_wq_item *wqi;
 	u32 freespace, tail, wq_off;
 
-	/* Free space is guaranteed, see i915_guc_wq_reserve() above */
+	/* Free space is guaranteed */
 	freespace = CIRC_SPACE(client->wq_tail, desc->head, client->wq_size);
 	GEM_BUG_ON(freespace < wqi_size);
 
@@ -581,14 +524,6 @@ static int guc_ring_doorbell(struct i915_guc_client *client)
  * i915_guc_submit() - Submit commands through GuC
  * @rq:		request associated with the commands
  *
- * The caller must have already called i915_guc_wq_reserve() above with
- * a result of 0 (success), guaranteeing that there is space in the work
- * queue for the new request, so enqueuing the item cannot fail.
- *
- * Bad Things Will Happen if the caller violates this protocol e.g. calls
- * submit() when _reserve() says there's no space, or calls _submit()
- * a different number of times from (successful) calls to _reserve().
- *
  * The only error here arises if the doorbell hardware isn't functioning
  * as expected, which really shouln't happen.
  */
@@ -691,7 +626,6 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
 			trace_i915_gem_request_in(rq, port_index(port, engine));
 			last = rq;
 			submit = true;
-			i915_guc_wq_unreserve(rq);
 		}
 
 		rb = rb_next(rb);
@@ -1216,6 +1150,19 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 	enum intel_engine_id id;
 	int err;
 
+	/*
+	 * We're using GuC work items for submitting work through GuC. Since
+	 * we're coalescing multiple requests from a single context into a
+	 * single work item prior to assigning it to execlist_port, we can
+	 * never have more work items than the total number of ports (for all
+	 * engines). The GuC firmware is controlling the HEAD of work queue,
+	 * and it is guaranteed that it will remove the work item from the
+	 * queue before our request is completed.
+	 */
+	BUILD_BUG_ON(ARRAY_SIZE(engine->execlist_port) *
+		     sizeof(struct guc_wq_item) *
+		     I915_NUM_ENGINES > GUC_WQ_SIZE);
+
 	if (!client) {
 		client = guc_client_alloc(dev_priv,
 					  INTEL_INFO(dev_priv)->ring_mask,
@@ -1243,8 +1190,6 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 	guc_interrupts_capture(dev_priv);
 
 	for_each_engine(engine, dev_priv, id) {
-		const int wqi_size = sizeof(struct guc_wq_item);
-		struct drm_i915_gem_request *rq;
 		struct execlist_port *port = engine->execlist_port;
 		int n;
 
@@ -1256,12 +1201,6 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 		engine->irq_tasklet.func = i915_guc_irq_handler;
 		clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
 
-		/* Replay the current set of previously submitted requests */
-		spin_lock_irq(&engine->timeline->lock);
-		list_for_each_entry(rq, &engine->timeline->requests, link)
-			guc_client_update_wq_rsvd(client, wqi_size);
-		spin_unlock_irq(&engine->timeline->lock);
-
 		for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) {
 			if (!port_isset(&port[n]))
 				break;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 014b30a..5d4f23c 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -913,27 +913,14 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request)
 	 */
 	request->reserved_space += EXECLISTS_REQUEST_SIZE;
 
-	if (i915.enable_guc_submission) {
-		/*
-		 * Check that the GuC has space for the request before
-		 * going any further, as the i915_add_request() call
-		 * later on mustn't fail ...
-		 */
-		ret = i915_guc_wq_reserve(request);
-		if (ret)
-			goto err;
-	}
-
 	cs = intel_ring_begin(request, 0);
-	if (IS_ERR(cs)) {
-		ret = PTR_ERR(cs);
-		goto err_unreserve;
-	}
+	if (IS_ERR(cs))
+		return PTR_ERR(cs);
 
 	if (!ce->initialised) {
 		ret = engine->init_context(request);
 		if (ret)
-			goto err_unreserve;
+			return ret;
 
 		ce->initialised = true;
 	}
@@ -947,12 +934,6 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request)
 
 	request->reserved_space -= EXECLISTS_REQUEST_SIZE;
 	return 0;
-
-err_unreserve:
-	if (i915.enable_guc_submission)
-		i915_guc_wq_unreserve(request);
-err:
-	return ret;
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 930f2e1..b86f866 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -55,10 +55,6 @@ struct drm_i915_gem_request;
  *
  * We also keep a few statistics on failures. Ideally, these should all
  * be zero!
- *   no_wq_space: times that the submission pre-check found no space was
- *                available in the work queue (note, the queue is shared,
- *                not per-engine). It is OK for this to be nonzero, but
- *                it should not be huge!
  */
 struct i915_guc_client {
 	struct i915_vma *vma;
@@ -79,8 +75,6 @@ struct i915_guc_client {
 	uint32_t wq_offset;
 	uint32_t wq_size;
 	uint32_t wq_tail;
-	uint32_t wq_rsvd;
-	uint32_t no_wq_space;
 
 	/* Per-engine counts of GuC submissions */
 	uint64_t submissions[I915_NUM_ENGINES];
@@ -242,8 +236,6 @@ u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 /* i915_guc_submission.c */
 int i915_guc_submission_init(struct drm_i915_private *dev_priv);
 int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
-int i915_guc_wq_reserve(struct drm_i915_gem_request *rq);
-void i915_guc_wq_unreserve(struct drm_i915_gem_request *request);
 void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
 void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
-- 
2.9.4

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

[PATCH v2 5/8] drm/i915/scheduler: Remember request priority throughout its lifetime

[Michał Winiarski]

Since request can be unsubmitted, we need to avoid overriding its priority
during submission. Otherwise we won't be able to resubmit it with
correct priority.

v2: Limit DFS by excluding completed requests (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c |  1 -
 drivers/gpu/drm/i915/intel_lrc.c           | 13 ++++++++++---
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 66d8eaf..f89718c 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -620,7 +620,6 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
 			}
 
 			INIT_LIST_HEAD(&rq->priotree.link);
-			rq->priotree.priority = INT_MAX;
 
 			__i915_gem_request_submit(rq);
 			trace_i915_gem_request_in(rq, port_index(port, engine));
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 5d4f23c..1255724 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -487,7 +487,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 			}
 
 			INIT_LIST_HEAD(&rq->priotree.link);
-			rq->priotree.priority = INT_MAX;
 
 			__i915_gem_request_submit(rq);
 			trace_i915_gem_request_in(rq, port_index(port, engine));
@@ -771,9 +770,17 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio)
 		 * engines.
 		 */
 		list_for_each_entry(p, &pt->signalers_list, signal_link) {
+			struct drm_i915_gem_request *s =
+				container_of(p->signaler, typeof(*s), priotree);
+
 			GEM_BUG_ON(p->signaler->priority < pt->priority);
-			if (prio > READ_ONCE(p->signaler->priority))
-				list_move_tail(&p->dfs_link, &dfs);
+			if (prio <= READ_ONCE(p->signaler->priority))
+				continue;
+
+			if (i915_gem_request_completed(s))
+				continue;
+
+			list_move_tail(&p->dfs_link, &dfs);
 		}
 
 		list_safe_reset_next(dep, p, dfs_link);
-- 
2.9.4

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

[PATCH v2 6/8] drm/i915/scheduler: Split insert_request

[Michał Winiarski]

We'd like to reuse the priolist lookup in request resubmission path,
let's split insert_request to make that happen.

v2: Handle allocation error in lookup rather than in caller (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 41 ++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 1255724..8fc852c 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -627,20 +627,16 @@ static void intel_lrc_irq_handler(unsigned long data)
 	intel_uncore_forcewake_put(dev_priv, engine->fw_domains);
 }
 
-static bool
-insert_request(struct intel_engine_cs *engine,
-	       struct i915_priotree *pt,
-	       int prio)
+static struct i915_priolist *
+priolist_lookup(struct intel_engine_cs *engine, int prio, bool *first)
 {
 	struct i915_priolist *p;
 	struct rb_node **parent, *rb;
-	bool first = true;
 
+find_priolist:
 	if (unlikely(engine->no_priolist))
 		prio = I915_PRIORITY_NORMAL;
-
-find_priolist:
-	/* most positive priority is scheduled first, equal priorities fifo */
+	*first = true;
 	rb = NULL;
 	parent = &engine->execlist_queue.rb_node;
 	while (*parent) {
@@ -650,10 +646,10 @@ insert_request(struct intel_engine_cs *engine,
 			parent = &rb->rb_left;
 		} else if (prio < p->priority) {
 			parent = &rb->rb_right;
-			first = false;
+			*first = false;
 		} else {
-			list_add_tail(&pt->link, &p->requests);
-			return false;
+			*first = false;
+			return p;
 		}
 	}
 
@@ -661,10 +657,8 @@ insert_request(struct intel_engine_cs *engine,
 		p = &engine->default_priolist;
 	} else {
 		p = kmem_cache_alloc(engine->i915->priorities, GFP_ATOMIC);
-		/* Convert an allocation failure to a priority bump */
-		if (unlikely(!p)) {
-			prio = I915_PRIORITY_NORMAL; /* recurses just once */
 
+		if (unlikely(!p)) {
 			/* To maintain ordering with all rendering, after an
 			 * allocation failure we have to disable all scheduling.
 			 * Requests will then be executed in fifo, and schedule
@@ -683,11 +677,26 @@ insert_request(struct intel_engine_cs *engine,
 	rb_insert_color(&p->node, &engine->execlist_queue);
 
 	INIT_LIST_HEAD(&p->requests);
-	list_add_tail(&pt->link, &p->requests);
 
-	if (first)
+	if (*first)
 		engine->execlist_first = &p->node;
 
+	return p;
+}
+
+static bool
+insert_request(struct intel_engine_cs *engine,
+	       struct i915_priotree *pt,
+	       int prio)
+{
+	struct i915_priolist *p;
+	bool first = false;
+
+	p = priolist_lookup(engine, prio, &first);
+
+	/* most positive priority is scheduled first, equal priorities fifo */
+	list_add_tail(&pt->link, &p->requests);
+
 	return first;
 }
 
-- 
2.9.4

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

[PATCH v3 7/8] drm/i915/scheduler: Use priorities when resubmitting after reset

[Michał Winiarski]

Now that we're able to unsubmit requests, we can take advantage of it
during reset. Rather than resubmitting the previous workload directly to
GuC/ELSP, we can simply move the requests back to priority queue,
submitting from the tasklet instead.

v2: Move the tasklet schedule out for legacy ringbuffer submission
v3: Handle allocation error in lookup rather than in caller (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |   6 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |  15 +--
 drivers/gpu/drm/i915/intel_lrc.c           | 141 +++++++++++++++++------------
 drivers/gpu/drm/i915/intel_lrc.h           |   1 +
 4 files changed, 89 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a637cc0..28e21fd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3019,15 +3019,11 @@ static void engine_set_wedged(struct intel_engine_cs *engine)
 	 */
 
 	if (i915.enable_execlists) {
-		struct execlist_port *port = engine->execlist_port;
 		unsigned long flags;
-		unsigned int n;
 
 		spin_lock_irqsave(&engine->timeline->lock, flags);
 
-		for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++)
-			i915_gem_request_put(port_request(&port[n]));
-		memset(engine->execlist_port, 0, sizeof(engine->execlist_port));
+		intel_lr_clear_execlist_ports(engine);
 		engine->execlist_queue = RB_ROOT;
 		engine->execlist_first = NULL;
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index f89718c..f6a1f6e 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -534,21 +534,20 @@ static void i915_guc_submit(struct drm_i915_gem_request *rq)
 	unsigned int engine_id = engine->id;
 	struct intel_guc *guc = &rq->i915->guc;
 	struct i915_guc_client *client = guc->execbuf_client;
-	unsigned long flags;
 	int b_ret;
 
 	/* WA to flush out the pending GMADR writes to ring buffer. */
 	if (i915_vma_is_map_and_fenceable(rq->ring->vma))
 		POSTING_READ_FW(GUC_STATUS);
 
-	spin_lock_irqsave(&client->wq_lock, flags);
+	spin_lock(&client->wq_lock);
 
 	guc_wq_item_append(client, rq);
 	b_ret = guc_ring_doorbell(client);
 
 	client->submissions[engine_id] += 1;
 
-	spin_unlock_irqrestore(&client->wq_lock, flags);
+	spin_unlock(&client->wq_lock);
 }
 
 static void nested_enable_signaling(struct drm_i915_gem_request *rq)
@@ -1189,9 +1188,6 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 	guc_interrupts_capture(dev_priv);
 
 	for_each_engine(engine, dev_priv, id) {
-		struct execlist_port *port = engine->execlist_port;
-		int n;
-
 		/* The tasklet was initialised by execlists, and may be in
 		 * a state of flux (across a reset) and so we just want to
 		 * take over the callback without changing any other state
@@ -1199,13 +1195,6 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 		 */
 		engine->irq_tasklet.func = i915_guc_irq_handler;
 		clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
-
-		for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) {
-			if (!port_isset(&port[n]))
-				break;
-
-			i915_guc_submit(port_request(&port[n]));
-		}
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 8fc852c..356a6d2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -291,6 +291,26 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx,
 	return ctx->engine[engine->id].lrc_desc;
 }
 
+static inline struct execlist_port *
+execlists_last_port(struct intel_engine_cs *engine)
+{
+	return &engine->execlist_port[ARRAY_SIZE(engine->execlist_port) - 1];
+}
+
+void intel_lr_clear_execlist_ports(struct intel_engine_cs *engine)
+{
+	struct execlist_port *port = engine->execlist_port;
+	struct drm_i915_gem_request *rq;
+
+	while ((rq = port_request(port))) {
+		i915_gem_request_put(rq);
+		if (port == execlists_last_port(engine))
+			break;
+		port++;
+	}
+	memset(engine->execlist_port, 0, sizeof(engine->execlist_port));
+}
+
 static inline void
 execlists_context_status_change(struct drm_i915_gem_request *rq,
 				unsigned long status)
@@ -952,6 +972,36 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request)
 	return 0;
 }
 
+static void intel_lr_resubmit_requests(struct intel_engine_cs *engine)
+{
+	struct i915_priolist *p = &engine->default_priolist;
+	struct drm_i915_gem_request *rq, *rq_prev;
+	struct i915_priotree *pt;
+	bool first;
+	int last_prio;
+
+	lockdep_assert_held(&engine->timeline->lock);
+
+	last_prio = INT_MIN;
+
+	list_for_each_entry_safe_reverse(rq, rq_prev,
+					 &engine->timeline->requests, link) {
+		if (i915_gem_request_completed(rq))
+			break;
+
+		pt = &rq->priotree;
+		if (pt->priority != last_prio)
+			p = priolist_lookup(engine, pt->priority,
+					    &first);
+		__i915_gem_request_unsubmit(rq);
+		trace_i915_gem_request_out(rq);
+
+		/* lifo, since we're traversing timeline in reverse */
+		list_add(&pt->link, &p->requests);
+		last_prio = pt->priority;
+	}
+}
+
 /*
  * In this WA we need to set GEN8_L3SQCREG4[21:21] and reset it after
  * PIPE_CONTROL instruction. This is required for the flush to happen correctly
@@ -1220,9 +1270,6 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
 static int gen8_init_common_ring(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
-	struct execlist_port *port = engine->execlist_port;
-	unsigned int n;
-	bool submit;
 	int ret;
 
 	ret = intel_mocs_init_engine(engine);
@@ -1241,26 +1288,6 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
 
 	DRM_DEBUG_DRIVER("Execlists enabled for %s\n", engine->name);
 
-	/* After a GPU reset, we may have requests to replay */
-	clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
-
-	submit = false;
-	for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) {
-		if (!port_isset(&port[n]))
-			break;
-
-		DRM_DEBUG_DRIVER("Restarting %s:%d from 0x%x\n",
-				 engine->name, n,
-				 port_request(&port[n])->global_seqno);
-
-		/* Discard the current inflight count */
-		port_set(&port[n], port_request(&port[n]));
-		submit = true;
-	}
-
-	if (submit && !i915.enable_guc_submission)
-		execlists_submit_ports(engine);
-
 	return 0;
 }
 
@@ -1300,10 +1327,9 @@ static int gen9_init_render_ring(struct intel_engine_cs *engine)
 static void reset_common_ring(struct intel_engine_cs *engine,
 			      struct drm_i915_gem_request *request)
 {
-	struct execlist_port *port = engine->execlist_port;
 	struct intel_context *ce;
 
-	/* If the request was innocent, we leave the request in the ELSP
+	/* If the request was innocent, we leave the request intact
 	 * and will try to replay it on restarting. The context image may
 	 * have been corrupted by the reset, in which case we may have
 	 * to service a new GPU hang, but more likely we can continue on
@@ -1313,42 +1339,45 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 	 * and have to at least restore the RING register in the context
 	 * image back to the expected values to skip over the guilty request.
 	 */
-	if (!request || request->fence.error != -EIO)
-		return;
-
-	/* We want a simple context + ring to execute the breadcrumb update.
-	 * We cannot rely on the context being intact across the GPU hang,
-	 * so clear it and rebuild just what we need for the breadcrumb.
-	 * All pending requests for this context will be zapped, and any
-	 * future request will be after userspace has had the opportunity
-	 * to recreate its own state.
-	 */
-	ce = &request->ctx->engine[engine->id];
-	execlists_init_reg_state(ce->lrc_reg_state,
-				 request->ctx, engine, ce->ring);
-
-	/* Move the RING_HEAD onto the breadcrumb, past the hanging batch */
-	ce->lrc_reg_state[CTX_RING_BUFFER_START+1] =
-		i915_ggtt_offset(ce->ring->vma);
-	ce->lrc_reg_state[CTX_RING_HEAD+1] = request->postfix;
+	if (request && request->fence.error == -EIO) {
+		/* We want a simple context + ring to execute the breadcrumb
+		 * update. We cannot rely on the context being intact across
+		 * the GPU hang, so clear it and rebuild just what we need for
+		 * the breadcrumb. All pending requests for this context will
+		 * be zapped, and any future request will be after userspace
+		 * has had the opportunity to recreate its own state.
+		 */
+		ce = &request->ctx->engine[engine->id];
+		execlists_init_reg_state(ce->lrc_reg_state,
+					 request->ctx, engine, ce->ring);
 
-	request->ring->head = request->postfix;
-	intel_ring_update_space(request->ring);
 
-	/* Catch up with any missed context-switch interrupts */
-	if (request->ctx != port_request(port)->ctx) {
-		i915_gem_request_put(port_request(port));
-		port[0] = port[1];
-		memset(&port[1], 0, sizeof(port[1]));
+		/* Move the RING_HEAD onto the breadcrumb,
+		 * past the hanging batch
+		 */
+		ce->lrc_reg_state[CTX_RING_BUFFER_START+1] =
+			i915_ggtt_offset(ce->ring->vma);
+		ce->lrc_reg_state[CTX_RING_HEAD+1] = request->postfix;
+
+		request->ring->head = request->postfix;
+		intel_ring_update_space(request->ring);
+
+		/* Reset WaIdleLiteRestore:bdw,skl as well */
+		request->tail =
+			intel_ring_wrap(request->ring,
+					request->wa_tail -
+					WA_TAIL_DWORDS * sizeof(u32));
+		assert_ring_tail_valid(request->ring, request->tail);
 	}
 
-	GEM_BUG_ON(request->ctx != port_request(port)->ctx);
+	spin_lock_irq(&engine->timeline->lock);
+	intel_lr_resubmit_requests(engine);
+	spin_unlock_irq(&engine->timeline->lock);
 
-	/* Reset WaIdleLiteRestore:bdw,skl as well */
-	request->tail =
-		intel_ring_wrap(request->ring,
-				request->wa_tail - WA_TAIL_DWORDS*sizeof(u32));
-	assert_ring_tail_valid(request->ring, request->tail);
+	intel_lr_clear_execlist_ports(engine);
+	clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
+
+	tasklet_hi_schedule(&engine->irq_tasklet);
 }
 
 static int intel_logical_ring_emit_pdps(struct drm_i915_gem_request *req)
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 52b3a1f..8e1ef4d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -83,6 +83,7 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx,
 				     struct intel_engine_cs *engine);
 
 /* Execlists */
+void intel_lr_clear_execlist_ports(struct intel_engine_cs *engine);
 int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv,
 				    int enable_execlists);
 
-- 
2.9.4

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

[PATCH 8/8] HAX: Enable GuC submission for CI

[Michał Winiarski]

---
 drivers/gpu/drm/i915/i915_params.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b6a7e36..9dcc8a0 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -56,8 +56,8 @@ struct i915_params i915 __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
-	.enable_guc_loading = 0,
-	.enable_guc_submission = 0,
+	.enable_guc_loading = 1,
+	.enable_guc_submission = 1,
 	.guc_log_level = -1,
 	.guc_firmware_path = NULL,
 	.huc_firmware_path = NULL,
-- 
2.9.4

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

✗ Fi.CI.BAT: failure for series starting with [v2,1/8] drm/i915: Remove misleading comment in request_alloc

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/8] drm/i915: Remove misleading comment in request_alloc
URL   : https://patchwork.freedesktop.org/series/24801/
State : failure

== Summary ==

Series 24801v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/24801/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test gem_sync:
        Subgroup basic-store-each:
                pass       -> FAIL       (fi-kbl-7560u)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101144
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (fi-skl-6770hq) fdo#99739
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> INCOMPLETE (fi-bdw-5557u)
                pass       -> INCOMPLETE (fi-bdw-gvtdvm)
        Subgroup hang-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-bsw-n3050)

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fdo#99739 https://bugs.freedesktop.org/show_bug.cgi?id=99739

fi-bdw-5557u     total:220  pass:211  dwarn:0   dfail:0   fail:0   skip:8  
fi-bdw-gvtdvm    total:220  pass:208  dwarn:2   dfail:0   fail:0   skip:9  
fi-bsw-n3050     total:222  pass:199  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:521s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:492s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:491s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:416s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:415s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:414s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:492s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:469s
fi-kbl-7560u     total:278  pass:262  dwarn:5   dfail:0   fail:1   skip:10  time:575s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:456s
fi-skl-6700hq    total:278  pass:260  dwarn:1   dfail:0   fail:0   skip:17  time:574s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:472s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:502s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:432s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:541s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:404s

5c5ce2ab8183e4bd85a603169e260cc938837e2c drm-tip: 2017y-05m-22d-13h-42m-03s UTC integration manifest
e7be59c HAX: Enable GuC submission for CI
9486c76 drm/i915/scheduler: Use priorities when resubmitting after reset
8fa10e2 drm/i915/scheduler: Split insert_request
7314c03 drm/i915/scheduler: Remember request priority throughout its lifetime
b9887c1 drm/i915/guc: Remove GuC wq reservation
933c881 drm/i915/guc: Submit GuC workitems containing coalesced requests
0be4978 drm/i915/guc: Skip port assign on first iteration of GuC dequeue
f711323 drm/i915: Remove misleading comment in request_alloc

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4776/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915/guc: Submit GuC workitems containing coalesced requests

[Daniele Ceraolo Spurio]

On 22/05/17 15:07, Michał Winiarski wrote:
> To create an upper bound on number of GuC workitems, we need to change
> the way that requests are being submitted. Rather than submitting each
> request as an individual workitem, we can do coalescing in a similar way
> we're handlig execlist submission ports. We also need to stop pretending
> that we're doing "lite-restore" in GuC submission (we would create a
> workitem each time we hit this condition).
>
> v2: Also coalesce when replaying on reset (Daniele)
> v3: Consistent wq_resv - per-request (Daniele)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 72 +++++++++++++++---------------
>  1 file changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index e6e0c6e..2a0c3161 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -491,14 +491,12 @@ static void guc_wq_item_append(struct i915_guc_client *client,
>  	 * workqueue buffer dw by dw.
>  	 */
>  	BUILD_BUG_ON(wqi_size != 16);
> -	GEM_BUG_ON(client->wq_rsvd < wqi_size);
>
>  	/* postincrement WQ tail for next time */
>  	wq_off = client->wq_tail;
>  	GEM_BUG_ON(wq_off & (wqi_size - 1));
>  	client->wq_tail += wqi_size;
>  	client->wq_tail &= client->wq_size - 1;
> -	client->wq_rsvd -= wqi_size;
>
>  	/* WQ starts from the page after doorbell / process_desc */
>  	wqi = client->vaddr + wq_off + GUC_DB_SIZE;
> @@ -580,7 +578,7 @@ static int guc_ring_doorbell(struct i915_guc_client *client)
>  }
>
>  /**
> - * __i915_guc_submit() - Submit commands through GuC
> + * i915_guc_submit() - Submit commands through GuC
>   * @rq:		request associated with the commands
>   *
>   * The caller must have already called i915_guc_wq_reserve() above with
> @@ -594,7 +592,7 @@ static int guc_ring_doorbell(struct i915_guc_client *client)
>   * The only error here arises if the doorbell hardware isn't functioning
>   * as expected, which really shouln't happen.
>   */
> -static void __i915_guc_submit(struct drm_i915_gem_request *rq)
> +static void i915_guc_submit(struct drm_i915_gem_request *rq)
>  {
>  	struct drm_i915_private *dev_priv = rq->i915;
>  	struct intel_engine_cs *engine = rq->engine;
> @@ -618,12 +616,6 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq)
>  	spin_unlock_irqrestore(&client->wq_lock, flags);
>  }
>
> -static void i915_guc_submit(struct drm_i915_gem_request *rq)
> -{
> -	__i915_gem_request_submit(rq);
> -	__i915_guc_submit(rq);
> -}
> -
>  static void nested_enable_signaling(struct drm_i915_gem_request *rq)
>  {
>  	/* If we use dma_fence_enable_sw_signaling() directly, lockdep
> @@ -659,12 +651,15 @@ static void port_assign(struct execlist_port *port,
>  	nested_enable_signaling(rq);
>  }
>
> -static bool i915_guc_dequeue(struct intel_engine_cs *engine)
> +static void i915_guc_dequeue(struct intel_engine_cs *engine)
>  {
>  	struct execlist_port *port = engine->execlist_port;
> -	struct drm_i915_gem_request *last = port_request(port);
> -	struct rb_node *rb;
> +	struct drm_i915_gem_request *last = NULL;
>  	bool submit = false;
> +	struct rb_node *rb;
> +
> +	if (port_request(port))
> +		port++;
>
>  	spin_lock_irq(&engine->timeline->lock);
>  	rb = engine->execlist_first;
> @@ -681,18 +676,22 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
>  					goto done;
>  				}
>
> -				if (submit)
> +				if (submit) {
>  					port_assign(port, last);
> +					i915_guc_submit(last);
> +					submit = false;
> +				}
>  				port++;
>  			}
>
>  			INIT_LIST_HEAD(&rq->priotree.link);
>  			rq->priotree.priority = INT_MAX;
>
> -			i915_guc_submit(rq);
> +			__i915_gem_request_submit(rq);
>  			trace_i915_gem_request_in(rq, port_index(port, engine));
>  			last = rq;
>  			submit = true;
> +			i915_guc_wq_unreserve(rq);

Bikeshed: doesn't moving the removal of the reservation to before the 
actual WQ tail update actually theoretically introduce the possibility 
of racing over-reserving? It can't hurt anyway because of the 
coalescing, but it might be worth squashing the next patch into this one 
to make things cleaner.

>  		}
>
>  		rb = rb_next(rb);
> @@ -703,11 +702,11 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
>  	}
>  done:
>  	engine->execlist_first = rb;
> -	if (submit)
> +	if (submit) {
>  		port_assign(port, last);
> +		i915_guc_submit(last);
> +	}
>  	spin_unlock_irq(&engine->timeline->lock);
> -
> -	return submit;
>  }
>
>  static void i915_guc_irq_handler(unsigned long data)
> @@ -715,24 +714,20 @@ static void i915_guc_irq_handler(unsigned long data)
>  	struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
>  	struct execlist_port *port = engine->execlist_port;
>  	struct drm_i915_gem_request *rq;
> -	bool submit;
>
> -	do {
> -		rq = port_request(&port[0]);
> -		while (rq && i915_gem_request_completed(rq)) {
> -			trace_i915_gem_request_out(rq);
> -			i915_gem_request_put(rq);
> +	rq = port_request(&port[0]);
> +	while (rq && i915_gem_request_completed(rq)) {
> +		trace_i915_gem_request_out(rq);
> +		i915_gem_request_put(rq);
>
> -			port[0] = port[1];
> -			memset(&port[1], 0, sizeof(port[1]));
> +		port[0] = port[1];
> +		memset(&port[1], 0, sizeof(port[1]));
>
> -			rq = port_request(&port[0]);
> -		}
> +		rq = port_request(&port[0]);
> +	}
>
> -		submit = false;
> -		if (!port_count(&port[1]))
> -			submit = i915_guc_dequeue(engine);
> -	} while (submit);
> +	if (!port_isset(&port[1]))
> +		i915_guc_dequeue(engine);
>  }
>
>  /*
> @@ -1250,6 +1245,8 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>  	for_each_engine(engine, dev_priv, id) {
>  		const int wqi_size = sizeof(struct guc_wq_item);
>  		struct drm_i915_gem_request *rq;
> +		struct execlist_port *port = engine->execlist_port;
> +		int n;
>
>  		/* The tasklet was initialised by execlists, and may be in
>  		 * a state of flux (across a reset) and so we just want to
> @@ -1261,11 +1258,16 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>
>  		/* Replay the current set of previously submitted requests */
>  		spin_lock_irq(&engine->timeline->lock);
> -		list_for_each_entry(rq, &engine->timeline->requests, link) {
> +		list_for_each_entry(rq, &engine->timeline->requests, link)
>  			guc_client_update_wq_rsvd(client, wqi_size);

Can we just drop this entirely due to the new changes in this version of 
the patch? We don't require wq_rsvd to be > 0 in i915_guc_submit anymore 
and we're guaranteed to have space (the wq is empty and we're only going 
to submit up to 2 items). Otherwise, aren't we going to leak this rsvd 
space since we don't clean it in i915_guc_submit anymore?

Thanks,
Daniele

> -			__i915_guc_submit(rq);
> -		}
>  		spin_unlock_irq(&engine->timeline->lock);
> +
> +		for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) {
> +			if (!port_isset(&port[n]))
> +				break;
> +
> +			i915_guc_submit(port_request(&port[n]));
> +		}
>  	}
>
>  	return 0;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915/guc: Submit GuC workitems containing coalesced requests

[Chris Wilson]

On Tue, May 23, 2017 at 12:07:50AM +0200, Michał Winiarski wrote:
> To create an upper bound on number of GuC workitems, we need to change
> the way that requests are being submitted. Rather than submitting each
> request as an individual workitem, we can do coalescing in a similar way
> we're handlig execlist submission ports. We also need to stop pretending
> that we're doing "lite-restore" in GuC submission (we would create a
> workitem each time we hit this condition).
> 
> v2: Also coalesce when replaying on reset (Daniele)
> v3: Consistent wq_resv - per-request (Daniele)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 72 +++++++++++++++---------------
>  1 file changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index e6e0c6e..2a0c3161 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -491,14 +491,12 @@ static void guc_wq_item_append(struct i915_guc_client *client,
>  	 * workqueue buffer dw by dw.
>  	 */
>  	BUILD_BUG_ON(wqi_size != 16);
> -	GEM_BUG_ON(client->wq_rsvd < wqi_size);
>  
>  	/* postincrement WQ tail for next time */
>  	wq_off = client->wq_tail;
>  	GEM_BUG_ON(wq_off & (wqi_size - 1));
>  	client->wq_tail += wqi_size;
>  	client->wq_tail &= client->wq_size - 1;
> -	client->wq_rsvd -= wqi_size;

Stray chunk for another patch.

>  
>  	/* WQ starts from the page after doorbell / process_desc */
>  	wqi = client->vaddr + wq_off + GUC_DB_SIZE;
> @@ -580,7 +578,7 @@ static int guc_ring_doorbell(struct i915_guc_client *client)
>  }
>  
>  /**
> - * __i915_guc_submit() - Submit commands through GuC
> + * i915_guc_submit() - Submit commands through GuC
>   * @rq:		request associated with the commands
>   *
>   * The caller must have already called i915_guc_wq_reserve() above with
> @@ -594,7 +592,7 @@ static int guc_ring_doorbell(struct i915_guc_client *client)
>   * The only error here arises if the doorbell hardware isn't functioning
>   * as expected, which really shouln't happen.
>   */
> -static void __i915_guc_submit(struct drm_i915_gem_request *rq)
> +static void i915_guc_submit(struct drm_i915_gem_request *rq)
>  {
>  	struct drm_i915_private *dev_priv = rq->i915;
>  	struct intel_engine_cs *engine = rq->engine;
> @@ -618,12 +616,6 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq)
>  	spin_unlock_irqrestore(&client->wq_lock, flags);
>  }
>  
> -static void i915_guc_submit(struct drm_i915_gem_request *rq)
> -{
> -	__i915_gem_request_submit(rq);
> -	__i915_guc_submit(rq);
> -}
> -
>  static void nested_enable_signaling(struct drm_i915_gem_request *rq)
>  {
>  	/* If we use dma_fence_enable_sw_signaling() directly, lockdep
> @@ -659,12 +651,15 @@ static void port_assign(struct execlist_port *port,
>  	nested_enable_signaling(rq);
>  }
>  
> -static bool i915_guc_dequeue(struct intel_engine_cs *engine)
> +static void i915_guc_dequeue(struct intel_engine_cs *engine)
>  {
>  	struct execlist_port *port = engine->execlist_port;
> -	struct drm_i915_gem_request *last = port_request(port);
> -	struct rb_node *rb;
> +	struct drm_i915_gem_request *last = NULL;
>  	bool submit = false;
> +	struct rb_node *rb;
> +
> +	if (port_request(port))

port_isset()

> +		port++;
>  
>  	spin_lock_irq(&engine->timeline->lock);
>  	rb = engine->execlist_first;
> @@ -681,18 +676,22 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
>  					goto done;
>  				}
>  
> -				if (submit)
> +				if (submit) {
>  					port_assign(port, last);
> +					i915_guc_submit(last);
> +					submit = false;

The way I did it was to make it look more like execlists again by
passing engine to i915_guc_submit() and have it iterate over the ports,
so that is only called at the end and can be done outside of the
spinlock. (Which is an issue as this spinlock is highly contended on
guc.) I then used port_count to avoid resubmitting requests.

> +				}
>  				port++;
>  			}
>  
>  			INIT_LIST_HEAD(&rq->priotree.link);
>  			rq->priotree.priority = INT_MAX;
>  
> -			i915_guc_submit(rq);
> +			__i915_gem_request_submit(rq);
>  			trace_i915_gem_request_in(rq, port_index(port, engine));
>  			last = rq;
>  			submit = true;
> +			i915_guc_wq_unreserve(rq);
>  		}
>  
>  		rb = rb_next(rb);
> @@ -703,11 +702,11 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
>  	}
>  done:
>  	engine->execlist_first = rb;
> -	if (submit)
> +	if (submit) {
>  		port_assign(port, last);
> +		i915_guc_submit(last);
> +	}
>  	spin_unlock_irq(&engine->timeline->lock);
> -
> -	return submit;
>  }
>  
>  static void i915_guc_irq_handler(unsigned long data)
> @@ -715,24 +714,20 @@ static void i915_guc_irq_handler(unsigned long data)
>  	struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
>  	struct execlist_port *port = engine->execlist_port;
>  	struct drm_i915_gem_request *rq;
> -	bool submit;
>  
> -	do {
> -		rq = port_request(&port[0]);
> -		while (rq && i915_gem_request_completed(rq)) {
> -			trace_i915_gem_request_out(rq);
> -			i915_gem_request_put(rq);
> +	rq = port_request(&port[0]);
> +	while (rq && i915_gem_request_completed(rq)) {
> +		trace_i915_gem_request_out(rq);
> +		i915_gem_request_put(rq);
>  
> -			port[0] = port[1];
> -			memset(&port[1], 0, sizeof(port[1]));
> +		port[0] = port[1];
> +		memset(&port[1], 0, sizeof(port[1]));
>  
> -			rq = port_request(&port[0]);
> -		}
> +		rq = port_request(&port[0]);
> +	}
>  
> -		submit = false;
> -		if (!port_count(&port[1]))
> -			submit = i915_guc_dequeue(engine);
> -	} while (submit);
> +	if (!port_isset(&port[1]))
> +		i915_guc_dequeue(engine);

This is a change not recorded in the changelog, so separate patch for
justification (e.g. it is nothing to do with lite-restore).

>  }
>  
>  /*
> @@ -1250,6 +1245,8 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>  	for_each_engine(engine, dev_priv, id) {
>  		const int wqi_size = sizeof(struct guc_wq_item);
>  		struct drm_i915_gem_request *rq;
> +		struct execlist_port *port = engine->execlist_port;
> +		int n;
>  
>  		/* The tasklet was initialised by execlists, and may be in
>  		 * a state of flux (across a reset) and so we just want to
> @@ -1261,11 +1258,16 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>  
>  		/* Replay the current set of previously submitted requests */
>  		spin_lock_irq(&engine->timeline->lock);
> -		list_for_each_entry(rq, &engine->timeline->requests, link) {
> +		list_for_each_entry(rq, &engine->timeline->requests, link)
>  			guc_client_update_wq_rsvd(client, wqi_size);
> -			__i915_guc_submit(rq);
> -		}
>  		spin_unlock_irq(&engine->timeline->lock);
> +
> +		for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) {
> +			if (!port_isset(&port[n]))
> +				break;
> +
> +			i915_guc_submit(port_request(&port[n]));
> +		}

... which just falls out if you passed engine to i915_guc_submit().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915/guc: Skip port assign on first iteration of GuC dequeue

[Chris Wilson]

On Tue, May 23, 2017 at 12:07:49AM +0200, Michał Winiarski wrote:
> If port[0] is occupied and we're trying to dequeue request from
> different context, we will inevitably hit BUG_ON in port_assign.
> Let's skip it - similar to what we're doing in execlists counterpart.
> 
> Fixes: 77f0d0e925e8a0 ("drm/i915/execlists: Pack the count into the low bits of the port.request")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Can you split out the first two and resend with a CI prefix + a guc
enabling patch? Let's get this fixed first.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx