[v3,3/5] drm/i915/guc: Simplify GuC doorbell logic

Commit Message

Michał Winiarski Sept. 14, 2017, 8:32 a.m. UTC

All we're really doing is incrementing a simple counter in a
doorbell_info struct. We can do without extra variables and a separate
counter kept in guc_client. Since it's gone, we're also removing its
debugfs.
The only functional change here, is that we're no longer treating 0 as a
special value. GuC doesn't seem to care, why should we?

v2: Restore desc->tail update.
v3: Drop the retry loop, assert that doorbell cookie doesn't change
behind our back.

Cc: Chris Wilson <[e-mail]>
Cc: Daniele Ceraolo Spurio <[e-mail]>
Cc: Michal Wajdeczko <[e-mail]>
Cc: Oscar Mateo <[e-mail]>
Suggested-by: Chris Wilson <[e-mail]>
Signed-off-by: Michał Winiarski <[e-mail]>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |  4 +-
 drivers/gpu/drm/i915/i915_guc_submission.c | 61 +++++++-----------------------
 drivers/gpu/drm/i915/intel_uc.h            |  1 -
 3 files changed, 16 insertions(+), 50 deletions(-)

Comments

Chris Wilson Sept. 14, 2017, 10:37 a.m. UTC | #1

Quoting Michał Winiarski (2017-09-14 09:32:14)
> +       /* we're not expecting the doorbell cookie to change behind our back */
> +       cookie = READ_ONCE(db->cookie);
> +       if (cmpxchg(&db->cookie, cookie, cookie + 1) != cookie)
> +               BUG();

Or just
WARN_ON_ONCE(xchg(&db->cookie, cookie + 1) != cookie);
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a4c2d0718a70..46ac6091772e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2445,8 +2445,8 @@  static void i915_guc_client_info(struct seq_file *m,
 
 	seq_printf(m, "\tPriority %d, GuC stage index: %u, PD offset 0x%x\n",
 		client->priority, client->stage_id, client->proc_desc_offset);
-	seq_printf(m, "\tDoorbell id %d, offset: 0x%lx, cookie 0x%x\n",
-		client->doorbell_id, client->doorbell_offset, client->doorbell_cookie);
+	seq_printf(m, "\tDoorbell id %d, offset: 0x%lx\n",
+		client->doorbell_id, client->doorbell_offset);
 	seq_printf(m, "\tWQ size %d, offset: 0x%x, tail %d\n",
 		client->wq_size, client->wq_offset, client->wq_tail);
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 84beedae3708..360be80613ba 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -192,13 +192,12 @@  static int __create_doorbell(struct i915_guc_client *client)
 
 	doorbell = __get_doorbell(client);
 	doorbell->db_status = GUC_DOORBELL_ENABLED;
-	doorbell->cookie = client->doorbell_cookie;
+	doorbell->cookie = 0;
 
 	err = __guc_allocate_doorbell(client->guc, client->stage_id);
-	if (err) {
+	if (err)
 		doorbell->db_status = GUC_DOORBELL_DISABLED;
-		doorbell->cookie = 0;
-	}
+
 	return err;
 }
 
@@ -466,57 +465,25 @@  static void guc_reset_wq(struct i915_guc_client *client)
 	client->wq_tail = 0;
 }
 
-static int guc_ring_doorbell(struct i915_guc_client *client)
+static void guc_ring_doorbell(struct i915_guc_client *client)
 {
 	struct guc_process_desc *desc = __get_process_desc(client);
-	union guc_doorbell_qw db_cmp, db_exc, db_ret;
-	union guc_doorbell_qw *db;
-	int attempt = 2, ret = -EAGAIN;
+	struct guc_doorbell_info *db;
+	u32 cookie;
 
 	/* Update the tail so it is visible to GuC */
 	desc->tail = client->wq_tail;
 
-	/* current cookie */
-	db_cmp.db_status = GUC_DOORBELL_ENABLED;
-	db_cmp.cookie = client->doorbell_cookie;
-
-	/* cookie to be updated */
-	db_exc.db_status = GUC_DOORBELL_ENABLED;
-	db_exc.cookie = client->doorbell_cookie + 1;
-	if (db_exc.cookie == 0)
-		db_exc.cookie = 1;
-
 	/* pointer of current doorbell cacheline */
-	db = (union guc_doorbell_qw *)__get_doorbell(client);
-
-	while (attempt--) {
-		/* lets ring the doorbell */
-		db_ret.value_qw = atomic64_cmpxchg((atomic64_t *)db,
-			db_cmp.value_qw, db_exc.value_qw);
-
-		/* if the exchange was successfully executed */
-		if (db_ret.value_qw == db_cmp.value_qw) {
-			/* db was successfully rung */
-			client->doorbell_cookie = db_exc.cookie;
-			ret = 0;
-			break;
-		}
-
-		/* XXX: doorbell was lost and need to acquire it again */
-		if (db_ret.db_status == GUC_DOORBELL_DISABLED)
-			break;
+	db = __get_doorbell(client);
 
-		DRM_WARN("Cookie mismatch. Expected %d, found %d\n",
-			 db_cmp.cookie, db_ret.cookie);
+	/* we're not expecting the doorbell cookie to change behind our back */
+	cookie = READ_ONCE(db->cookie);
+	if (cmpxchg(&db->cookie, cookie, cookie + 1) != cookie)
+		BUG();
 
-		/* update the cookie to newly read cookie from GuC */
-		db_cmp.cookie = db_ret.cookie;
-		db_exc.cookie = db_ret.cookie + 1;
-		if (db_exc.cookie == 0)
-			db_exc.cookie = 1;
-	}
-
-	return ret;
+	/* XXX: doorbell was lost and need to acquire it again */
+	GEM_BUG_ON(db->db_status != GUC_DOORBELL_ENABLED);
 }
 
 /**
@@ -550,7 +517,7 @@  static void i915_guc_submit(struct intel_engine_cs *engine)
 			spin_lock_irqsave(&client->wq_lock, flags);
 
 			guc_wq_item_append(client, rq);
-			WARN_ON(guc_ring_doorbell(client));
+			guc_ring_doorbell(client);
 
 			client->submissions[engine_id] += 1;
 
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index d41051688221..f505dcccd613 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -66,7 +66,6 @@  struct i915_guc_client {
 
 	u16 doorbell_id;
 	unsigned long doorbell_offset;
-	u32 doorbell_cookie;
 
 	spinlock_t wq_lock;
 	uint32_t wq_offset;