[RFC 1/4] drm/i915/scheduler: Remember request priority throughout its lifetime

[RFC 1/4] 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.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
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           | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 6193ad7..082f8ae 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -692,7 +692,6 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
 		rb = rb_next(rb);
 		rb_erase(&rq->priotree.node, &engine->execlist_queue);
 		RB_CLEAR_NODE(&rq->priotree.node);
-		rq->priotree.priority = INT_MAX;
 
 		i915_guc_submit(rq);
 		trace_i915_gem_request_in(rq, port - engine->execlist_port);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b0c3a02..301ae7c 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -488,7 +488,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		rb = rb_next(rb);
 		rb_erase(&cursor->priotree.node, &engine->execlist_queue);
 		RB_CLEAR_NODE(&cursor->priotree.node);
-		cursor->priotree.priority = INT_MAX;
 
 		__i915_gem_request_submit(cursor);
 		trace_i915_gem_request_in(cursor, port - engine->execlist_port);
-- 
2.9.3

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

[RFC 2/4] drm/i915/scheduler: Make insert_request resubmission aware

[Michał Winiarski]

Normally when we're inserting requests with equal priority, we're using
FIFO ordering. However, when we're resubmitting requests it's helpful
to be able to ensure that they're executed before their dependencies,
meaning that we need to use LIFO ordering instead.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
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 | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 301ae7c..107cf91 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -611,12 +611,15 @@ static void intel_lrc_irq_handler(unsigned long data)
 	intel_uncore_forcewake_put(dev_priv, engine->fw_domains);
 }
 
-static bool insert_request(struct i915_priotree *pt, struct rb_root *root)
+static bool insert_request(struct i915_priotree *pt, struct rb_root *root,
+			   bool reinsert)
 {
 	struct rb_node **p, *rb;
 	bool first = true;
 
-	/* most positive priority is scheduled first, equal priorities fifo */
+	/* most positive priority is scheduled first,
+	 * equal priorities - fifo, except when we're reinserting - lifo
+	 */
 	rb = NULL;
 	p = &root->rb_node;
 	while (*p) {
@@ -624,7 +627,8 @@ static bool insert_request(struct i915_priotree *pt, struct rb_root *root)
 
 		rb = *p;
 		pos = rb_entry(rb, typeof(*pos), node);
-		if (pt->priority > pos->priority) {
+		if ((pt->priority > pos->priority) ||
+		    ((pt->priority == pos->priority) && reinsert)) {
 			p = &rb->rb_left;
 		} else {
 			p = &rb->rb_right;
@@ -645,7 +649,8 @@ static void execlists_submit_request(struct drm_i915_gem_request *request)
 	/* Will be called from irq-context when using foreign fences. */
 	spin_lock_irqsave(&engine->timeline->lock, flags);
 
-	if (insert_request(&request->priotree, &engine->execlist_queue)) {
+	if (insert_request(&request->priotree,
+			   &engine->execlist_queue, false)) {
 		engine->execlist_first = &request->priotree.node;
 		if (execlists_elsp_ready(engine))
 			tasklet_hi_schedule(&engine->irq_tasklet);
@@ -742,7 +747,7 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio)
 
 		pt->priority = prio;
 		rb_erase(&pt->node, &engine->execlist_queue);
-		if (insert_request(pt, &engine->execlist_queue))
+		if (insert_request(pt, &engine->execlist_queue, false))
 			engine->execlist_first = &pt->node;
 	}
 
-- 
2.9.3

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

[RFC 3/4] 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.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
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            |   1 +
 drivers/gpu/drm/i915/i915_guc_submission.c |  12 ---
 drivers/gpu/drm/i915/intel_lrc.c           | 127 ++++++++++++++++-------------
 3 files changed, 71 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 84ea249..747ff37 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2924,6 +2924,7 @@ void i915_gem_reset_finish(struct drm_i915_private *dev_priv)
 	for_each_engine(engine, dev_priv, id) {
 		tasklet_enable(&engine->irq_tasklet);
 		kthread_unpark(engine->breadcrumbs.signaler);
+		tasklet_hi_schedule(&engine->irq_tasklet);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 082f8ae..9975244 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1245,24 +1245,12 @@ 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;
-
 		/* 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
 		 * in the tasklet.
 		 */
 		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);
-			__i915_guc_submit(rq);
-		}
-		spin_unlock_irq(&engine->timeline->lock);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 107cf91..ff34aba 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -377,6 +377,22 @@ static void execlists_submit_ports(struct intel_engine_cs *engine)
 	writel(lower_32_bits(desc[0]), elsp);
 }
 
+static void execlists_clear_ports(struct intel_engine_cs *engine)
+{
+	struct execlist_port *port = engine->execlist_port;
+	struct drm_i915_gem_request *rq;
+
+	rq = port->request;
+	while (rq) {
+		i915_gem_request_put(rq);
+		memset(port, 0, sizeof(*port));
+		if (port != engine->execlist_port)
+			break;
+		port++;
+		rq = port->request;
+	}
+}
+
 static bool ctx_single_port_submission(const struct i915_gem_context *ctx)
 {
 	return (IS_ENABLED(CONFIG_DRM_I915_GVT) &&
@@ -504,11 +520,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		execlists_submit_ports(engine);
 }
 
-static bool execlists_elsp_idle(struct intel_engine_cs *engine)
-{
-	return !engine->execlist_port[0].request;
-}
-
 static bool execlists_elsp_ready(const struct intel_engine_cs *engine)
 {
 	const struct execlist_port *port = engine->execlist_port;
@@ -895,6 +906,25 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request)
 	return ret;
 }
 
+static void intel_lr_resubmit_requests(struct intel_engine_cs *engine)
+{
+	struct drm_i915_gem_request *rq, *prev;
+
+	lockdep_assert_held(&engine->timeline->lock);
+
+	list_for_each_entry_safe_reverse(rq, prev,
+					 &engine->timeline->requests, link) {
+		if (!i915_gem_request_completed(rq)) {
+			__i915_gem_request_unsubmit(rq);
+			trace_i915_gem_request_out(rq);
+			if (insert_request(&rq->priotree,
+					   &engine->execlist_queue, true))
+				engine->execlist_first = &rq->priotree.node;
+		} else
+			break;
+	}
+}
+
 /*
  * 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
@@ -1160,11 +1190,6 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
 	return ret;
 }
 
-static u32 port_seqno(struct execlist_port *port)
-{
-	return port->request ? port->request->global_seqno : 0;
-}
-
 static int gen8_init_common_ring(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
@@ -1186,18 +1211,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);
-	if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) {
-		DRM_DEBUG_DRIVER("Restarting %s from requests [0x%x, 0x%x]\n",
-				 engine->name,
-				 port_seqno(&engine->execlist_port[0]),
-				 port_seqno(&engine->execlist_port[1]));
-		engine->execlist_port[0].count = 0;
-		engine->execlist_port[1].count = 0;
-		execlists_submit_ports(engine);
-	}
-
 	return 0;
 }
 
@@ -1237,10 +1250,10 @@ 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;
+	unsigned long flags;
 
-	/* 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
@@ -1250,42 +1263,42 @@ 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;
-
-	request->ring->head = request->postfix;
-	intel_ring_update_space(request->ring);
-
-	/* Catch up with any missed context-switch interrupts */
-	if (request->ctx != port[0].request->ctx) {
-		i915_gem_request_put(port[0].request);
-		port[0] = port[1];
-		memset(&port[1], 0, sizeof(port[1]));
+	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);
+
+
+		/* 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[0].request->ctx);
+	spin_lock_irqsave(&engine->timeline->lock, flags);
+	intel_lr_resubmit_requests(engine);
+	spin_unlock_irqrestore(&engine->timeline->lock, flags);
 
-	/* 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);
+	execlists_clear_ports(engine);
+	clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
 }
 
 static int intel_logical_ring_emit_pdps(struct drm_i915_gem_request *req)
-- 
2.9.3

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

[RFC 4/4] drm/i915/guc: Repurpose GuC wq reservation

[Michał Winiarski]

Now that we're only driving GuC submissions from the tasklet, we can
simply skip the submission if GuC wq is full. This allows us to
completely remove reservation from the request_alloc path, and only
use it to manage wq between tasklets belonging to different engines.

Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.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 | 54 ++++++++++++------------------
 drivers/gpu/drm/i915/intel_lrc.c           | 25 ++------------
 drivers/gpu/drm/i915/intel_uc.h            |  2 --
 3 files changed, 24 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 9975244..4a7ef70 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -407,11 +407,11 @@ static void guc_stage_desc_fini(struct intel_guc *guc,
 }
 
 /**
- * i915_guc_wq_reserve() - reserve space in the GuC's workqueue
- * @request:	request associated with the commands
+ * guc_wq_reserve() - reserve space in the GuC's workqueue
+ * @client:	GuC client used for submission
  *
  * Return:	0 if space is available
- *		-EAGAIN if space is not currently available
+ *		-ENOSPC 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
@@ -419,18 +419,17 @@ static void guc_stage_desc_fini(struct intel_guc *guc,
  * 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.
+ * will be available for the next submission.
  */
-int i915_guc_wq_reserve(struct drm_i915_gem_request *request)
+static int guc_wq_reserve(struct i915_guc_client *client)
 {
 	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);
+	spin_lock(&client->wq_lock);
+
 	freespace = CIRC_SPACE(client->wq_tail, desc->head, client->wq_size);
 	freespace -= client->wq_rsvd;
 	if (likely(freespace >= wqi_size)) {
@@ -438,29 +437,12 @@ int i915_guc_wq_reserve(struct drm_i915_gem_request *request)
 		ret = 0;
 	} else {
 		client->no_wq_space++;
-		ret = -EAGAIN;
+		ret = -ENOSPC;
 	}
-	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);
-}
+	spin_unlock(&client->wq_lock);
 
-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);
+	return ret;
 }
 
 /* Construct a Work Item and append it to the GuC's Work Queue */
@@ -475,7 +457,9 @@ 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 */
+	lockdep_assert_held(&client->wq_lock);
+
+	/* Free space is guaranteed, see guc_wq_reserve() above */
 	freespace = CIRC_SPACE(client->wq_tail, desc->head, client->wq_size);
 	GEM_BUG_ON(freespace < wqi_size);
 
@@ -526,6 +510,7 @@ static void guc_reset_wq(struct i915_guc_client *client)
 	desc->tail = 0;
 
 	client->wq_tail = 0;
+	client->wq_rsvd = 0;
 }
 
 static int guc_ring_doorbell(struct i915_guc_client *client)
@@ -585,7 +570,7 @@ 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
+ * The caller must have already called 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.
  *
@@ -603,14 +588,13 @@ 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);
@@ -623,7 +607,7 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq)
 	guc->submissions[engine_id] += 1;
 	guc->last_seqno[engine_id] = rq->global_seqno;
 
-	spin_unlock_irqrestore(&client->wq_lock, flags);
+	spin_unlock(&client->wq_lock);
 }
 
 static void i915_guc_submit(struct drm_i915_gem_request *rq)
@@ -659,6 +643,7 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
 {
 	struct execlist_port *port = engine->execlist_port;
 	struct drm_i915_gem_request *last = port[0].request;
+	struct i915_guc_client *client = engine->i915->guc.execbuf_client;
 	struct rb_node *rb;
 	bool submit = false;
 
@@ -680,6 +665,9 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
 		struct drm_i915_gem_request *rq =
 			rb_entry(rb, typeof(*rq), priotree.node);
 
+		if (guc_wq_reserve(client) != 0)
+			break;
+
 		if (last && rq->ctx != last->ctx) {
 			if (port != engine->execlist_port)
 				break;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ff34aba..4372a52 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -864,27 +864,14 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request)
 	GEM_BUG_ON(!ce->ring);
 	request->ring = ce->ring;
 
-	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;
 	}
@@ -898,12 +885,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;
 }
 
 static void intel_lr_resubmit_requests(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 087192d..092583c 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -215,8 +215,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.3

_______________________________________________
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 [RFC,1/4] drm/i915/scheduler: Remember request priority throughout its lifetime

[Patchwork]

== Series Details ==

Series: series starting with [RFC,1/4] drm/i915/scheduler: Remember request priority throughout its lifetime
URL   : https://patchwork.freedesktop.org/series/22030/
State : failure

== Summary ==

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

Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> INCOMPLETE (fi-ivb-3520m)
                pass       -> INCOMPLETE (fi-hsw-4770)
                pass       -> INCOMPLETE (fi-hsw-4770r)
                pass       -> INCOMPLETE (fi-snb-2600)
                pass       -> INCOMPLETE (fi-ivb-3770)
                pass       -> INCOMPLETE (fi-byt-j1900)
                pass       -> INCOMPLETE (fi-snb-2520m)
                pass       -> INCOMPLETE (fi-byt-n2820)
                pass       -> INCOMPLETE (fi-ilk-650)
Test gem_exec_suspend:
        Subgroup basic-s3:
                incomplete -> PASS       (fi-skl-6260u)
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#100428

fdo#100428 https://bugs.freedesktop.org/show_bug.cgi?id=100428

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 459s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 456s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 579s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 544s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 595s
fi-byt-j1900     total:5    pass:4    dwarn:0   dfail:0   fail:0   skip:0   time: 0s
fi-byt-n2820     total:5    pass:4    dwarn:0   dfail:0   fail:0   skip:0   time: 0s
fi-hsw-4770      total:5    pass:4    dwarn:0   dfail:0   fail:0   skip:0   time: 0s
fi-hsw-4770r     total:5    pass:4    dwarn:0   dfail:0   fail:0   skip:0   time: 0s
fi-ilk-650       total:5    pass:4    dwarn:0   dfail:0   fail:0   skip:0   time: 0s
fi-ivb-3520m     total:5    pass:4    dwarn:0   dfail:0   fail:0   skip:0   time: 0s
fi-ivb-3770      total:5    pass:4    dwarn:0   dfail:0   fail:0   skip:0   time: 0s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 483s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 594s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 479s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 606s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 493s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 530s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 460s
fi-snb-2520m     total:5    pass:4    dwarn:0   dfail:0   fail:0   skip:0   time: 0s
fi-snb-2600      total:5    pass:4    dwarn:0   dfail:0   fail:0   skip:0   time: 0s

dabd992961047cf26698036f563aa86a083284ac drm-tip: 2017y-03m-28d-15h-25m-54s UTC integration manifest
74cf11b drm/i915/guc: Repurpose GuC wq reservation
db83b51 drm/i915/scheduler: Use priorities when resubmitting after reset
f16f880 drm/i915/scheduler: Make insert_request resubmission aware
045035d drm/i915/scheduler: Remember request priority throughout its lifetime

== Logs ==

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

Re: [RFC 1/4] drm/i915/scheduler: Remember request priority throughout its lifetime

[Chris Wilson]

On Tue, Mar 28, 2017 at 08:00:26PM +0200, Michał Winiarski wrote:
> 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.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> 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           | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 6193ad7..082f8ae 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -692,7 +692,6 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
>  		rb = rb_next(rb);
>  		rb_erase(&rq->priotree.node, &engine->execlist_queue);
>  		RB_CLEAR_NODE(&rq->priotree.node);
> -		rq->priotree.priority = INT_MAX;

This still breaks execlists_schedule().
-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: [RFC 2/4] drm/i915/scheduler: Make insert_request resubmission aware

[Chris Wilson]

On Tue, Mar 28, 2017 at 08:00:27PM +0200, Michał Winiarski wrote:
> Normally when we're inserting requests with equal priority, we're using
> FIFO ordering. However, when we're resubmitting requests it's helpful
> to be able to ensure that they're executed before their dependencies,
> meaning that we need to use LIFO ordering instead.

You shouldn't be requeueing before you've resubmitted its dependencies.
FIFO should still be holding. This doesn't make sense without seeing the
other side, and I suspect that other side isn't right :-p
-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: [RFC 3/4] drm/i915/scheduler: Use priorities when resubmitting after reset

[Chris Wilson]

On Tue, Mar 28, 2017 at 08:00:28PM +0200, Michał Winiarski wrote:
> 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.

Let's not overcomplicate this.
-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: [RFC 4/4] drm/i915/guc: Repurpose GuC wq reservation

[Chris Wilson]

On Tue, Mar 28, 2017 at 08:00:29PM +0200, Michał Winiarski wrote:
> Now that we're only driving GuC submissions from the tasklet, we can
> simply skip the submission if GuC wq is full. This allows us to
> completely remove reservation from the request_alloc path, and only
> use it to manage wq between tasklets belonging to different engines.

Hmm, this sounds like a very good idea.

> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.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 | 54 ++++++++++++------------------
>  drivers/gpu/drm/i915/intel_lrc.c           | 25 ++------------
>  drivers/gpu/drm/i915/intel_uc.h            |  2 --
>  3 files changed, 24 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 9975244..4a7ef70 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -407,11 +407,11 @@ static void guc_stage_desc_fini(struct intel_guc *guc,
>  }
>  
>  /**
> - * i915_guc_wq_reserve() - reserve space in the GuC's workqueue
> - * @request:	request associated with the commands
> + * guc_wq_reserve() - reserve space in the GuC's workqueue
> + * @client:	GuC client used for submission
>   *
>   * Return:	0 if space is available
> - *		-EAGAIN if space is not currently available
> + *		-ENOSPC 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
> @@ -419,18 +419,17 @@ static void guc_stage_desc_fini(struct intel_guc *guc,
>   * 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.
> + * will be available for the next submission.
>   */
> -int i915_guc_wq_reserve(struct drm_i915_gem_request *request)
> +static int guc_wq_reserve(struct i915_guc_client *client)
>  {
>  	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);
> +	spin_lock(&client->wq_lock);
> +
>  	freespace = CIRC_SPACE(client->wq_tail, desc->head, client->wq_size);
>  	freespace -= client->wq_rsvd;
>  	if (likely(freespace >= wqi_size)) {
> @@ -438,29 +437,12 @@ int i915_guc_wq_reserve(struct drm_i915_gem_request *request)
>  		ret = 0;
>  	} else {
>  		client->no_wq_space++;
> -		ret = -EAGAIN;
> +		ret = -ENOSPC;
>  	}
> -	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);
> -}
> +	spin_unlock(&client->wq_lock);
>  
> -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);
> +	return ret;
>  }
>  
>  /* Construct a Work Item and append it to the GuC's Work Queue */
> @@ -475,7 +457,9 @@ 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 */
> +	lockdep_assert_held(&client->wq_lock);
> +
> +	/* Free space is guaranteed, see guc_wq_reserve() above */
>  	freespace = CIRC_SPACE(client->wq_tail, desc->head, client->wq_size);
>  	GEM_BUG_ON(freespace < wqi_size);
>  
> @@ -526,6 +510,7 @@ static void guc_reset_wq(struct i915_guc_client *client)
>  	desc->tail = 0;
>  
>  	client->wq_tail = 0;
> +	client->wq_rsvd = 0;
>  }
>  
>  static int guc_ring_doorbell(struct i915_guc_client *client)
> @@ -585,7 +570,7 @@ 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
> + * The caller must have already called 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.
>   *
> @@ -603,14 +588,13 @@ 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);
> @@ -623,7 +607,7 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq)
>  	guc->submissions[engine_id] += 1;
>  	guc->last_seqno[engine_id] = rq->global_seqno;
>  
> -	spin_unlock_irqrestore(&client->wq_lock, flags);
> +	spin_unlock(&client->wq_lock);
>  }
>  
>  static void i915_guc_submit(struct drm_i915_gem_request *rq)
> @@ -659,6 +643,7 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
>  {
>  	struct execlist_port *port = engine->execlist_port;
>  	struct drm_i915_gem_request *last = port[0].request;
> +	struct i915_guc_client *client = engine->i915->guc.execbuf_client;
>  	struct rb_node *rb;
>  	bool submit = false;
>  
> @@ -680,6 +665,9 @@ static bool i915_guc_dequeue(struct intel_engine_cs *engine)
>  		struct drm_i915_gem_request *rq =
>  			rb_entry(rb, typeof(*rq), priotree.node);
>  
> +		if (guc_wq_reserve(client) != 0)
> +			break;

We shouldn't have to reserve for every request in this case, just for
the batch of requests for the same context as they will share the tail
update.

I don't see a reason why this would depend upon the earlier patches, so
can we spin this off and focus on getting this improvement in?
-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: [RFC 2/4] drm/i915/scheduler: Make insert_request resubmission aware

[Chris Wilson]

On Tue, Mar 28, 2017 at 08:00:27PM +0200, Michał Winiarski wrote:
> Normally when we're inserting requests with equal priority, we're using
> FIFO ordering. However, when we're resubmitting requests it's helpful
> to be able to ensure that they're executed before their dependencies,
> meaning that we need to use LIFO ordering instead.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> 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 | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 301ae7c..107cf91 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -611,12 +611,15 @@ static void intel_lrc_irq_handler(unsigned long data)
>  	intel_uncore_forcewake_put(dev_priv, engine->fw_domains);
>  }
>  
> -static bool insert_request(struct i915_priotree *pt, struct rb_root *root)
> +static bool insert_request(struct i915_priotree *pt, struct rb_root *root,
> +			   bool reinsert)

I would just use a different insertion routine for unsubmit. The most
likely insertion point is execlist_first and so we can optimise the
reinsert to begin it's search there (we'll only have to search if there
were higher (than it) priority requests added that didn't trigger a
preemption).

I am not keen on making insert_request more complicated, semantically at
least. execlists_schedule() is built around the idea that the dependency
graphs are always have an order to their priority (no dependent request
can have a greater priority than their dependencies). insert_request()
is tightly coupled into that picture.

I need more coaxing, I have this nagging feeling that there is a simpler
way.
-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