[PATCH] drm/i915/guc: Remove preemption support for current fw

* [PATCH] drm/i915/guc: Remove preemption support for current fw
@ 2019-02-15 22:48 Chris Wilson
  2019-02-16  0:02 ` ✗ Fi.CI.SPARSE: warning for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Chris Wilson @ 2019-02-15 22:48 UTC (permalink / raw)
  To: intel-gfx

Preemption via GuC submission quickly degrades into confused firmware
and misordered requests. It is not being supported with no intention of
being fixed in its current incarnation, so remove the unstable code. By
removing the workqueue from the GuC submission path, we can now do
direct engine-resets and direct submission from atomic context.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108622
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Ewins <jon.ewins@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c           |   5 -
 drivers/gpu/drm/i915/intel_guc.c              |  31 ---
 drivers/gpu/drm/i915/intel_guc.h              |   9 -
 drivers/gpu/drm/i915/intel_guc_submission.c   | 203 +-----------------
 drivers/gpu/drm/i915/intel_ringbuffer.h       |   2 -
 drivers/gpu/drm/i915/selftests/intel_guc.c    |  17 +-
 .../gpu/drm/i915/selftests/intel_hangcheck.c  |   3 -
 7 files changed, 5 insertions(+), 265 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2aeea977283f..636e28367980 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2233,11 +2233,6 @@ static int i915_guc_info(struct seq_file *m, void *data)
 
 	seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client);
 	i915_guc_client_info(m, dev_priv, guc->execbuf_client);
-	if (guc->preempt_client) {
-		seq_printf(m, "\nGuC preempt client @ %p:\n",
-			   guc->preempt_client);
-		i915_guc_client_info(m, dev_priv, guc->preempt_client);
-	}
 
 	/* Add more as required ... */
 
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 8660af3fd755..8fa2de84d82e 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -76,8 +76,6 @@ void intel_guc_init_early(struct intel_guc *guc)
 
 static int guc_init_wq(struct intel_guc *guc)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
 	/*
 	 * GuC log buffer flush work item has to do register access to
 	 * send the ack to GuC and this work item, if not synced before
@@ -97,31 +95,6 @@ static int guc_init_wq(struct intel_guc *guc)
 		return -ENOMEM;
 	}
 
-	/*
-	 * Even though both sending GuC action, and adding a new workitem to
-	 * GuC workqueue are serialized (each with its own locking), since
-	 * we're using mutliple engines, it's possible that we're going to
-	 * issue a preempt request with two (or more - each for different
-	 * engine) workitems in GuC queue. In this situation, GuC may submit
-	 * all of them, which will make us very confused.
-	 * Our preemption contexts may even already be complete - before we
-	 * even had the chance to sent the preempt action to GuC!. Rather
-	 * than introducing yet another lock, we can just use ordered workqueue
-	 * to make sure we're always sending a single preemption request with a
-	 * single workitem.
-	 */
-	if (HAS_LOGICAL_RING_PREEMPTION(dev_priv) &&
-	    USES_GUC_SUBMISSION(dev_priv)) {
-		guc->preempt_wq = alloc_ordered_workqueue("i915-guc_preempt",
-							  WQ_HIGHPRI);
-		if (!guc->preempt_wq) {
-			destroy_workqueue(guc->log.relay.flush_wq);
-			DRM_ERROR("Couldn't allocate workqueue for GuC "
-				  "preemption\n");
-			return -ENOMEM;
-		}
-	}
-
 	return 0;
 }
 
@@ -129,10 +102,6 @@ static void guc_fini_wq(struct intel_guc *guc)
 {
 	struct workqueue_struct *wq;
 
-	wq = fetch_and_zero(&guc->preempt_wq);
-	if (wq)
-		destroy_workqueue(wq);
-
 	wq = fetch_and_zero(&guc->log.relay.flush_wq);
 	if (wq)
 		destroy_workqueue(wq);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 744220296653..b8c4615715c0 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -34,11 +34,6 @@
 #include "intel_uc_fw.h"
 #include "i915_vma.h"
 
-struct guc_preempt_work {
-	struct work_struct work;
-	struct intel_engine_cs *engine;
-};
-
 /*
  * Top level structure of GuC. It handles firmware loading and manages client
  * pool and doorbells. intel_guc owns a intel_guc_client to replace the legacy
@@ -65,10 +60,6 @@ struct intel_guc {
 	void *shared_data_vaddr;
 
 	struct intel_guc_client *execbuf_client;
-	struct intel_guc_client *preempt_client;
-
-	struct guc_preempt_work preempt_work[I915_NUM_ENGINES];
-	struct workqueue_struct *preempt_wq;
 
 	DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
 	/* Cyclic counter mod pagesize	*/
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 8bc8aa54aa35..d5677dd8cc15 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -81,12 +81,6 @@
  *
  */
 
-static inline u32 intel_hws_preempt_done_address(struct intel_engine_cs *engine)
-{
-	return (i915_ggtt_offset(engine->status_page.vma) +
-		I915_GEM_HWS_PREEMPT_ADDR);
-}
-
 static inline struct i915_priolist *to_priolist(struct rb_node *rb)
 {
 	return rb_entry(rb, struct i915_priolist, node);
@@ -558,125 +552,6 @@ static void flush_ggtt_writes(struct i915_vma *vma)
 		POSTING_READ_FW(GUC_STATUS);
 }
 
-static void inject_preempt_context(struct work_struct *work)
-{
-	struct guc_preempt_work *preempt_work =
-		container_of(work, typeof(*preempt_work), work);
-	struct intel_engine_cs *engine = preempt_work->engine;
-	struct intel_guc *guc = container_of(preempt_work, typeof(*guc),
-					     preempt_work[engine->id]);
-	struct intel_guc_client *client = guc->preempt_client;
-	struct guc_stage_desc *stage_desc = __get_stage_desc(client);
-	struct intel_context *ce = to_intel_context(client->owner, engine);
-	u32 data[7];
-
-	if (!ce->ring->emit) { /* recreate upon load/resume */
-		u32 addr = intel_hws_preempt_done_address(engine);
-		u32 *cs;
-
-		cs = ce->ring->vaddr;
-		if (engine->id == RCS) {
-			cs = gen8_emit_ggtt_write_rcs(cs,
-						      GUC_PREEMPT_FINISHED,
-						      addr,
-						      PIPE_CONTROL_CS_STALL);
-		} else {
-			cs = gen8_emit_ggtt_write(cs,
-						  GUC_PREEMPT_FINISHED,
-						  addr);
-			*cs++ = MI_NOOP;
-			*cs++ = MI_NOOP;
-		}
-		*cs++ = MI_USER_INTERRUPT;
-		*cs++ = MI_NOOP;
-
-		ce->ring->emit = GUC_PREEMPT_BREADCRUMB_BYTES;
-		GEM_BUG_ON((void *)cs - ce->ring->vaddr != ce->ring->emit);
-
-		flush_ggtt_writes(ce->ring->vma);
-	}
-
-	spin_lock_irq(&client->wq_lock);
-	guc_wq_item_append(client, engine->guc_id, lower_32_bits(ce->lrc_desc),
-			   GUC_PREEMPT_BREADCRUMB_BYTES / sizeof(u64), 0);
-	spin_unlock_irq(&client->wq_lock);
-
-	/*
-	 * If GuC firmware performs an engine reset while that engine had
-	 * a preemption pending, it will set the terminated attribute bit
-	 * on our preemption stage descriptor. GuC firmware retains all
-	 * pending work items for a high-priority GuC client, unlike the
-	 * normal-priority GuC client where work items are dropped. It
-	 * wants to make sure the preempt-to-idle work doesn't run when
-	 * scheduling resumes, and uses this bit to inform its scheduler
-	 * and presumably us as well. Our job is to clear it for the next
-	 * preemption after reset, otherwise that and future preemptions
-	 * will never complete. We'll just clear it every time.
-	 */
-	stage_desc->attribute &= ~GUC_STAGE_DESC_ATTR_TERMINATED;
-
-	data[0] = INTEL_GUC_ACTION_REQUEST_PREEMPTION;
-	data[1] = client->stage_id;
-	data[2] = INTEL_GUC_PREEMPT_OPTION_DROP_WORK_Q |
-		  INTEL_GUC_PREEMPT_OPTION_DROP_SUBMIT_Q;
-	data[3] = engine->guc_id;
-	data[4] = guc->execbuf_client->priority;
-	data[5] = guc->execbuf_client->stage_id;
-	data[6] = intel_guc_ggtt_offset(guc, guc->shared_data);
-
-	if (WARN_ON(intel_guc_send(guc, data, ARRAY_SIZE(data)))) {
-		execlists_clear_active(&engine->execlists,
-				       EXECLISTS_ACTIVE_PREEMPT);
-		tasklet_schedule(&engine->execlists.tasklet);
-	}
-
-	(void)I915_SELFTEST_ONLY(engine->execlists.preempt_hang.count++);
-}
-
-/*
- * We're using user interrupt and HWSP value to mark that preemption has
- * finished and GPU is idle. Normally, we could unwind and continue similar to
- * execlists submission path. Unfortunately, with GuC we also need to wait for
- * it to finish its own postprocessing, before attempting to submit. Otherwise
- * GuC may silently ignore our submissions, and thus we risk losing request at
- * best, executing out-of-order and causing kernel panic at worst.
- */
-#define GUC_PREEMPT_POSTPROCESS_DELAY_MS 10
-static void wait_for_guc_preempt_report(struct intel_engine_cs *engine)
-{
-	struct intel_guc *guc = &engine->i915->guc;
-	struct guc_shared_ctx_data *data = guc->shared_data_vaddr;
-	struct guc_ctx_report *report =
-		&data->preempt_ctx_report[engine->guc_id];
-
-	WARN_ON(wait_for_atomic(report->report_return_status ==
-				INTEL_GUC_REPORT_STATUS_COMPLETE,
-				GUC_PREEMPT_POSTPROCESS_DELAY_MS));
-	/*
-	 * GuC is expecting that we're also going to clear the affected context
-	 * counter, let's also reset the return status to not depend on GuC
-	 * resetting it after recieving another preempt action
-	 */
-	report->affected_count = 0;
-	report->report_return_status = INTEL_GUC_REPORT_STATUS_UNKNOWN;
-}
-
-static void complete_preempt_context(struct intel_engine_cs *engine)
-{
-	struct intel_engine_execlists *execlists = &engine->execlists;
-
-	GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT));
-
-	if (inject_preempt_hang(execlists))
-		return;
-
-	execlists_cancel_port_requests(execlists);
-	execlists_unwind_incomplete_requests(execlists);
-
-	wait_for_guc_preempt_report(engine);
-	intel_write_status_page(engine, I915_GEM_HWS_PREEMPT, 0);
-}
-
 /**
  * guc_submit() - Submit commands through GuC
  * @engine: engine associated with the commands
@@ -736,20 +611,6 @@ static bool __guc_dequeue(struct intel_engine_cs *engine)
 	lockdep_assert_held(&engine->timeline.lock);
 
 	if (port_isset(port)) {
-		if (intel_engine_has_preemption(engine)) {
-			struct guc_preempt_work *preempt_work =
-				&engine->i915->guc.preempt_work[engine->id];
-			int prio = execlists->queue_priority_hint;
-
-			if (__execlists_need_preempt(prio, port_prio(port))) {
-				execlists_set_active(execlists,
-						     EXECLISTS_ACTIVE_PREEMPT);
-				queue_work(engine->i915->guc.preempt_wq,
-					   &preempt_work->work);
-				return false;
-			}
-		}
-
 		port++;
 		if (port_isset(port))
 			return false;
@@ -832,13 +693,7 @@ static void guc_submission_tasklet(unsigned long data)
 		}
 	}
 
-	if (execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT) &&
-	    intel_read_status_page(engine, I915_GEM_HWS_PREEMPT) ==
-	    GUC_PREEMPT_FINISHED)
-		complete_preempt_context(engine);
-
-	if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT))
-		guc_dequeue(engine);
+	guc_dequeue(engine);
 
 	spin_unlock_irqrestore(&engine->timeline.lock, flags);
 }
@@ -859,16 +714,6 @@ static void guc_reset_prepare(struct intel_engine_cs *engine)
 	 * prevents the race.
 	 */
 	__tasklet_disable_sync_once(&execlists->tasklet);
-
-	/*
-	 * We're using worker to queue preemption requests from the tasklet in
-	 * GuC submission mode.
-	 * Even though tasklet was disabled, we may still have a worker queued.
-	 * Let's make sure that all workers scheduled before disabling the
-	 * tasklet are completed before continuing with the reset.
-	 */
-	if (engine->i915->guc.preempt_wq)
-		flush_workqueue(engine->i915->guc.preempt_wq);
 }
 
 /*
@@ -1028,7 +873,6 @@ static int guc_clients_create(struct intel_guc *guc)
 	struct intel_guc_client *client;
 
 	GEM_BUG_ON(guc->execbuf_client);
-	GEM_BUG_ON(guc->preempt_client);
 
 	client = guc_client_alloc(dev_priv,
 				  INTEL_INFO(dev_priv)->ring_mask,
@@ -1040,20 +884,6 @@ static int guc_clients_create(struct intel_guc *guc)
 	}
 	guc->execbuf_client = client;
 
-	if (dev_priv->preempt_context) {
-		client = guc_client_alloc(dev_priv,
-					  INTEL_INFO(dev_priv)->ring_mask,
-					  GUC_CLIENT_PRIORITY_KMD_HIGH,
-					  dev_priv->preempt_context);
-		if (IS_ERR(client)) {
-			DRM_ERROR("Failed to create GuC client for preemption!\n");
-			guc_client_free(guc->execbuf_client);
-			guc->execbuf_client = NULL;
-			return PTR_ERR(client);
-		}
-		guc->preempt_client = client;
-	}
-
 	return 0;
 }
 
@@ -1061,10 +891,6 @@ static void guc_clients_destroy(struct intel_guc *guc)
 {
 	struct intel_guc_client *client;
 
-	client = fetch_and_zero(&guc->preempt_client);
-	if (client)
-		guc_client_free(client);
-
 	client = fetch_and_zero(&guc->execbuf_client);
 	if (client)
 		guc_client_free(client);
@@ -1113,22 +939,11 @@ static int guc_clients_enable(struct intel_guc *guc)
 	if (ret)
 		return ret;
 
-	if (guc->preempt_client) {
-		ret = __guc_client_enable(guc->preempt_client);
-		if (ret) {
-			__guc_client_disable(guc->execbuf_client);
-			return ret;
-		}
-	}
-
 	return 0;
 }
 
 static void guc_clients_disable(struct intel_guc *guc)
 {
-	if (guc->preempt_client)
-		__guc_client_disable(guc->preempt_client);
-
 	if (guc->execbuf_client)
 		__guc_client_disable(guc->execbuf_client);
 }
@@ -1139,9 +954,6 @@ static void guc_clients_disable(struct intel_guc *guc)
  */
 int intel_guc_submission_init(struct intel_guc *guc)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	struct intel_engine_cs *engine;
-	enum intel_engine_id id;
 	int ret;
 
 	if (guc->stage_desc_pool)
@@ -1161,11 +973,6 @@ int intel_guc_submission_init(struct intel_guc *guc)
 	if (ret)
 		goto err_pool;
 
-	for_each_engine(engine, dev_priv, id) {
-		guc->preempt_work[id].engine = engine;
-		INIT_WORK(&guc->preempt_work[id].work, inject_preempt_context);
-	}
-
 	return 0;
 
 err_pool:
@@ -1175,13 +982,6 @@ int intel_guc_submission_init(struct intel_guc *guc)
 
 void intel_guc_submission_fini(struct intel_guc *guc)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	struct intel_engine_cs *engine;
-	enum intel_engine_id id;
-
-	for_each_engine(engine, dev_priv, id)
-		cancel_work_sync(&guc->preempt_work[id].work);
-
 	guc_clients_destroy(guc);
 	WARN_ON(!guc_verify_doorbells(guc));
 
@@ -1292,6 +1092,7 @@ static void guc_set_default_submission(struct intel_engine_cs *engine)
 	engine->reset.prepare = guc_reset_prepare;
 
 	engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
+	engine->flags &= ~I915_ENGINE_HAS_PREEMPTION;
 }
 
 int intel_guc_submission_enable(struct intel_guc *guc)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 710ffb221775..9bd3f8aad6ac 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -724,8 +724,6 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
  */
 #define I915_GEM_HWS_INDEX		0x30
 #define I915_GEM_HWS_INDEX_ADDR		(I915_GEM_HWS_INDEX * sizeof(u32))
-#define I915_GEM_HWS_PREEMPT		0x32
-#define I915_GEM_HWS_PREEMPT_ADDR	(I915_GEM_HWS_PREEMPT * sizeof(u32))
 #define I915_GEM_HWS_SEQNO		0x40
 #define I915_GEM_HWS_SEQNO_ADDR		(I915_GEM_HWS_SEQNO * sizeof(u32))
 #define I915_GEM_HWS_SCRATCH		0x80
diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c
index c5e0a0e98fcb..28ce5ce01043 100644
--- a/drivers/gpu/drm/i915/selftests/intel_guc.c
+++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
@@ -162,7 +162,7 @@ static int igt_guc_clients(void *args)
 	 */
 	guc_clients_disable(guc);
 	guc_clients_destroy(guc);
-	if (guc->execbuf_client || guc->preempt_client) {
+	if (guc->execbuf_client) {
 		pr_err("guc_clients_destroy lied!\n");
 		err = -EINVAL;
 		goto unlock;
@@ -182,18 +182,8 @@ static int igt_guc_clients(void *args)
 		goto out;
 	}
 
-	if (guc->preempt_client) {
-		err = validate_client(guc->preempt_client,
-				      GUC_CLIENT_PRIORITY_KMD_HIGH, true);
-		if (err) {
-			pr_err("preempt client validation failed\n");
-			goto out;
-		}
-	}
-
 	/* each client should now have reserved a doorbell */
-	if (!has_doorbell(guc->execbuf_client) ||
-	    (guc->preempt_client && !has_doorbell(guc->preempt_client))) {
+	if (!has_doorbell(guc->execbuf_client)) {
 		pr_err("guc_clients_create didn't reserve doorbells\n");
 		err = -EINVAL;
 		goto out;
@@ -203,8 +193,7 @@ static int igt_guc_clients(void *args)
 	guc_clients_enable(guc);
 
 	/* each client should now have received a doorbell */
-	if (!client_doorbell_in_sync(guc->execbuf_client) ||
-	    !client_doorbell_in_sync(guc->preempt_client)) {
+	if (!client_doorbell_in_sync(guc->execbuf_client)) {
 		pr_err("failed to initialize the doorbells\n");
 		err = -EINVAL;
 		goto out;
diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
index 74e743b101d9..28af146f28f3 100644
--- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
+++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
@@ -1580,9 +1580,6 @@ static int igt_atomic_reset(void *arg)
 
 	/* Check that the resets are usable from atomic context */
 
-	if (USES_GUC_SUBMISSION(i915))
-		return 0; /* guc is dead; long live the guc */
-
 	igt_global_reset_lock(i915);
 	mutex_lock(&i915->drm.struct_mutex);
 	wakeref = intel_runtime_pm_get(i915);
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread