[PATCH 0/4] drm/i915: updates to GuC doorbell handling

[PATCH 0/4] drm/i915: updates to GuC doorbell handling

[Dave Gordon]

The Linux hibernate/resume sequence involves booting one kernel, and
then replacing(!) its in-memory image with that of the previously
hibernated system.  This can lead to inconsistencies in the state of
the hardware, in particular where a driver does not or cannot reset
it to a well-defined initial state during resume.

For i915, the issue is that the doorbell hardware is not reset when
the GuC is reset; also, the driver *cannot* directly reprogram it:
only the GuC can do that. So this set of patches first reorganises
the doorbell handling, and then (in the last patch of the set)
ensures that the doorbell hardware is fully (re-)initialised when
the GuC is (re-)loaded.

Dave Gordon (4):
  drm/i915/guc: add doorbell map to debugfs/i915_guc_info
  drm/i915/guc: move guc_ring_doorbell() nearer to callsite
  drm/i915/guc: refactor doorbell management code
  drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission

 drivers/gpu/drm/i915/i915_debugfs.c        |   9 ++
 drivers/gpu/drm/i915/i915_guc_submission.c | 232 ++++++++++++++++++-----------
 2 files changed, 154 insertions(+), 87 deletions(-)

-- 
1.9.1

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

[PATCH 1/4] drm/i915/guc: add doorbell map to debugfs/i915_guc_info

[Dave Gordon]

To properly verify the driver->doorbell->GuC functionality, validation
needs to know how the driver has assigned the doorbell cache lines and
registers, so make them visible through debugfs.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e4f2c55..fbb4b16 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2560,6 +2560,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
 	struct i915_guc_client client = {};
 	struct intel_engine_cs *engine;
 	u64 total = 0;
+	int i;
 
 	if (!HAS_GUC_SCHED(dev_priv))
 		return 0;
@@ -2574,6 +2575,14 @@ static int i915_guc_info(struct seq_file *m, void *data)
 
 	mutex_unlock(&dev->struct_mutex);
 
+	seq_printf(m, "Doorbell map:\n");
+	BUILD_BUG_ON(ARRAY_SIZE(guc.doorbell_bitmap) % 4);
+	for (i = 0; i < ARRAY_SIZE(guc.doorbell_bitmap) - 3; i += 4)
+		seq_printf(m, "\t%016lx %016lx %016lx %016lx\n",
+			guc.doorbell_bitmap[i], guc.doorbell_bitmap[i+1],
+			guc.doorbell_bitmap[i+2], guc.doorbell_bitmap[i+3]);
+	seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", guc.db_cacheline);
+
 	seq_printf(m, "GuC total action count: %llu\n", guc.action_count);
 	seq_printf(m, "GuC action failure count: %u\n", guc.action_fail);
 	seq_printf(m, "GuC last action command: 0x%x\n", guc.action_cmd);
-- 
1.9.1

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

[PATCH 2/4] drm/i915/guc: move guc_ring_doorbell() nearer to callsite

[Dave Gordon]

Just code movement, no actual change to the function. This is in
preparation for the next patch, which will reorganise all the other
doorbell code, but doesn't change this function. So let's shuffle it
down near its caller rather than leaving it mixed in with the setup
code. Unlike the doorbell management code, this function is somewhat
time-critical, so putting it near its caller may even yield a tiny
performance improvement.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 110 ++++++++++++++---------------
 1 file changed, 55 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2db1182..7510841 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -185,61 +185,6 @@ static void guc_init_doorbell(struct intel_guc *guc,
 	doorbell->cookie = 0;
 }
 
-static int guc_ring_doorbell(struct i915_guc_client *gc)
-{
-	struct guc_process_desc *desc;
-	union guc_doorbell_qw db_cmp, db_exc, db_ret;
-	union guc_doorbell_qw *db;
-	int attempt = 2, ret = -EAGAIN;
-
-	desc = gc->client_base + gc->proc_desc_offset;
-
-	/* Update the tail so it is visible to GuC */
-	desc->tail = gc->wq_tail;
-
-	/* current cookie */
-	db_cmp.db_status = GUC_DOORBELL_ENABLED;
-	db_cmp.cookie = gc->cookie;
-
-	/* cookie to be updated */
-	db_exc.db_status = GUC_DOORBELL_ENABLED;
-	db_exc.cookie = gc->cookie + 1;
-	if (db_exc.cookie == 0)
-		db_exc.cookie = 1;
-
-	/* pointer of current doorbell cacheline */
-	db = gc->client_base + gc->doorbell_offset;
-
-	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 */
-			gc->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;
-
-		DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n",
-			  db_cmp.cookie, db_ret.cookie);
-
-		/* 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;
-}
-
 static void guc_disable_doorbell(struct intel_guc *guc,
 				 struct i915_guc_client *client)
 {
@@ -543,6 +488,61 @@ static void guc_add_workqueue_item(struct i915_guc_client *gc,
 	kunmap_atomic(base);
 }
 
+static int guc_ring_doorbell(struct i915_guc_client *gc)
+{
+	struct guc_process_desc *desc;
+	union guc_doorbell_qw db_cmp, db_exc, db_ret;
+	union guc_doorbell_qw *db;
+	int attempt = 2, ret = -EAGAIN;
+
+	desc = gc->client_base + gc->proc_desc_offset;
+
+	/* Update the tail so it is visible to GuC */
+	desc->tail = gc->wq_tail;
+
+	/* current cookie */
+	db_cmp.db_status = GUC_DOORBELL_ENABLED;
+	db_cmp.cookie = gc->cookie;
+
+	/* cookie to be updated */
+	db_exc.db_status = GUC_DOORBELL_ENABLED;
+	db_exc.cookie = gc->cookie + 1;
+	if (db_exc.cookie == 0)
+		db_exc.cookie = 1;
+
+	/* pointer of current doorbell cacheline */
+	db = gc->client_base + gc->doorbell_offset;
+
+	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 */
+			gc->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;
+
+		DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n",
+			  db_cmp.cookie, db_ret.cookie);
+
+		/* 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;
+}
+
 /**
  * i915_guc_submit() - Submit commands through GuC
  * @rq:		request associated with the commands
-- 
1.9.1

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

[PATCH 3/4] drm/i915/guc: refactor doorbell management code

[Dave Gordon]

During a hibernate/resume cycle, the driver, the GuC, and the doorbell
hardware can end up in inconsistent states. This patch refactors the
driver's handling and tracking of doorbells, in preparation for a later
one which will resolve the issue.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 87 ++++++++++++++++++------------
 1 file changed, 53 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 7510841..eef67c8 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -174,36 +174,63 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
  * client object which contains the page being used for the doorbell
  */
 
-static void guc_init_doorbell(struct intel_guc *guc,
-			      struct i915_guc_client *client)
+static int guc_update_doorbell_id(struct i915_guc_client *client,
+				  struct guc_doorbell_info *doorbell,
+				  u16 new_id)
 {
-	struct guc_doorbell_info *doorbell;
+	struct sg_table *sg = client->guc->ctx_pool_obj->pages;
+	void *doorbell_bitmap = client->guc->doorbell_bitmap;
+	struct guc_context_desc desc;
+	size_t len;
+
+	if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
+	    test_bit(client->doorbell_id, doorbell_bitmap)) {
+		/* Deactivate the old doorbell */
+		doorbell->db_status = GUC_DOORBELL_DISABLED;
+		(void)host2guc_release_doorbell(client->guc, client);
+		clear_bit(client->doorbell_id, doorbell_bitmap);
+	}
 
-	doorbell = client->client_base + client->doorbell_offset;
+	/* Update the GuC's idea of the doorbell ID */
+	len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
+			     sizeof(desc) * client->ctx_index);
+	if (len != sizeof(desc))
+		return -EFAULT;
+	desc.db_id = new_id;
+	len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
+			     sizeof(desc) * client->ctx_index);
+	if (len != sizeof(desc))
+		return -EFAULT;
+
+	client->doorbell_id = new_id;
+	if (new_id == GUC_INVALID_DOORBELL_ID)
+		return 0;
 
+	/* Activate the new doorbell */
+	set_bit(client->doorbell_id, doorbell_bitmap);
 	doorbell->db_status = GUC_DOORBELL_ENABLED;
 	doorbell->cookie = 0;
+	return host2guc_allocate_doorbell(client->guc, client);
 }
 
-static void guc_disable_doorbell(struct intel_guc *guc,
-				 struct i915_guc_client *client)
+static void guc_init_doorbell(struct intel_guc *guc,
+			      struct i915_guc_client *client,
+			      uint16_t db_id)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct guc_doorbell_info *doorbell;
-	i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
-	int value;
 
 	doorbell = client->client_base + client->doorbell_offset;
 
-	doorbell->db_status = GUC_DOORBELL_DISABLED;
-
-	I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
+	guc_update_doorbell_id(client, doorbell, db_id);
+}
 
-	value = I915_READ(drbreg);
-	WARN_ON((value & GEN8_DRB_VALID) != 0);
+static void guc_disable_doorbell(struct intel_guc *guc,
+				 struct i915_guc_client *client)
+{
+	struct guc_doorbell_info *doorbell;
 
-	I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
-	I915_WRITE(drbreg, 0);
+	doorbell = client->client_base + client->doorbell_offset;
+	guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID);
 
 	/* XXX: wait for any interrupts */
 	/* XXX: wait for workqueue to drain */
@@ -251,7 +278,7 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
 	if (id == end)
 		id = GUC_INVALID_DOORBELL_ID;
 	else
-		bitmap_set(guc->doorbell_bitmap, id, 1);
+		set_bit(id, guc->doorbell_bitmap);
 
 	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
 			hi_pri ? "high" : "normal", id);
@@ -259,11 +286,6 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
 	return id;
 }
 
-static void release_doorbell(struct intel_guc *guc, uint16_t id)
-{
-	bitmap_clear(guc->doorbell_bitmap, id, 1);
-}
-
 /*
  * Initialise the process descriptor shared with the GuC firmware.
  */
@@ -657,8 +679,6 @@ static void guc_client_free(struct drm_device *dev,
 	 */
 
 	if (client->client_base) {
-		uint16_t db_id = client->doorbell_id;
-
 		/*
 		 * If we got as far as setting up a doorbell, make sure
 		 * we shut it down before unmapping & deallocating the
@@ -666,12 +686,8 @@ static void guc_client_free(struct drm_device *dev,
 		 * GuC that we've finished with it, finally deallocate
 		 * it in our bitmap
 		 */
-		if (db_id != GUC_INVALID_DOORBELL_ID) {
+		if (client->doorbell_id != GUC_INVALID_DOORBELL_ID)
 			guc_disable_doorbell(guc, client);
-			if (test_bit(db_id, guc->doorbell_bitmap))
-				host2guc_release_doorbell(guc, client);
-			release_doorbell(guc, db_id);
-		}
 
 		kunmap(kmap_to_page(client->client_base));
 	}
@@ -706,6 +722,7 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc *guc = &dev_priv->guc;
 	struct drm_i915_gem_object *obj;
+	uint16_t db_id;
 
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client)
@@ -746,22 +763,24 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 	else
 		client->proc_desc_offset = (GUC_DB_SIZE / 2);
 
-	client->doorbell_id = assign_doorbell(guc, client->priority);
-	if (client->doorbell_id == GUC_INVALID_DOORBELL_ID)
+	db_id = assign_doorbell(guc, client->priority);
+	if (db_id == GUC_INVALID_DOORBELL_ID)
 		/* XXX: evict a doorbell instead */
 		goto err;
 
 	guc_init_proc_desc(guc, client);
 	guc_init_ctx_desc(guc, client);
-	guc_init_doorbell(guc, client);
+	guc_init_doorbell(guc, client, db_id);
 
 	/* XXX: Any cache flushes needed? General domain mgmt calls? */
 
 	if (host2guc_allocate_doorbell(guc, client))
 		goto err;
 
-	DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id %u\n",
-		priority, client, client->ctx_index, client->doorbell_id);
+	DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u\n",
+		priority, client, client->ctx_index);
+	DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n",
+		client->doorbell_id, client->doorbell_offset);
 
 	return client;
 
-- 
1.9.1

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

[PATCH 4/4] drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission

[Dave Gordon]

During a hibernate/resume cycle, the whole system is reset, including
the GuC and the doorbell hardware. Then the system is booted up, drivers
are loaded, etc -- the GuC firmware may be loaded and set running at this
point. But then, the booted kernel is replaced by the hibernated image,
and this resumed kernel will also try to reload the GuC firmware (which
will fail, because the GuC firmware area is write-once). To recover, we
reset the GuC and try again (which should work). But this GuC reset
*doesn't* also reset the doorbell hardware, so it can be left in a state
inconsistent with that assumed by the driver and the GuC.

It would be better if the GuC reset also cleared all doorbell state,
but that's not how the hardware currently works; also, the driver cannot
directly reprogram the doorbell hardware (only the GuC can do that).

So this patch cycles through all doorbells, assigning and releasing each
in turn, so that all the doorbell hardware is left in a consistent state,
no matter how it was programmed by the previously-running kernel and/or
GuC firmware.

v2: don't use kmap_atomic() now that client page 0 is kept mapped.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 43 ++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index eef67c8..7aba25b 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -220,7 +220,6 @@ static void guc_init_doorbell(struct intel_guc *guc,
 	struct guc_doorbell_info *doorbell;
 
 	doorbell = client->client_base + client->doorbell_offset;
-
 	guc_update_doorbell_id(client, doorbell, db_id);
 }
 
@@ -702,6 +701,46 @@ static void guc_client_free(struct drm_device *dev,
 	kfree(client);
 }
 
+/*
+ * Borrow the first client to set up & tear down every doorbell
+ * in turn, to ensure that all doorbell h/w is (re)initialised.
+ */
+static void guc_init_doorbell_hw(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct i915_guc_client *client = guc->execbuf_client;
+	struct guc_doorbell_info *doorbell;
+	uint16_t db_id, i;
+	int ret;
+
+	doorbell = client->client_base + client->doorbell_offset;
+	db_id = client->doorbell_id;
+
+	for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
+		i915_reg_t drbreg = GEN8_DRBREGL(i);
+		u32 value = I915_READ(drbreg);
+
+		ret = guc_update_doorbell_id(client, doorbell, i);
+
+		if (((value & GUC_DOORBELL_ENABLED) && (i != db_id)) || ret)
+			DRM_DEBUG_DRIVER("Doorbell reg 0x%x was 0x%x, ret %d\n",
+				drbreg.reg, value, ret);
+	}
+
+	/* Restore to original value */
+	guc_update_doorbell_id(client, doorbell, db_id);
+
+	for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
+		i915_reg_t drbreg = GEN8_DRBREGL(i);
+		u32 value = I915_READ(drbreg);
+
+		if ((value & GUC_DOORBELL_ENABLED) && (i != db_id))
+			DRM_DEBUG_DRIVER("Doorbell reg 0x%x finally 0x%x\n",
+						drbreg.reg, value);
+
+	}
+}
+
 /**
  * guc_client_alloc() - Allocate an i915_guc_client
  * @dev:	drm device
@@ -971,8 +1010,8 @@ int i915_guc_submission_enable(struct drm_device *dev)
 	}
 
 	guc->execbuf_client = client;
-
 	host2guc_sample_forcewake(guc, client);
+	guc_init_doorbell_hw(guc);
 
 	return 0;
 }
-- 
1.9.1

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

✓ Ro.CI.BAT: success for drm/i915: updates to GuC doorbell handling

[Patchwork]

== Series Details ==

Series: drm/i915: updates to GuC doorbell handling
URL   : https://patchwork.freedesktop.org/series/8441/
State : success

== Summary ==

Series 8441v1 drm/i915: updates to GuC doorbell handling
http://patchwork.freedesktop.org/api/1.0/series/8441/revisions/1/mbox


fi-bdw-i7-5557u  total:102  pass:93   dwarn:0   dfail:0   fail:0   skip:8  
fi-skl-i5-6260u  total:209  pass:198  dwarn:0   dfail:0   fail:0   skip:11 
fi-skl-i7-6700k  total:209  pass:184  dwarn:0   dfail:0   fail:0   skip:25 
fi-snb-i7-2600   total:209  pass:170  dwarn:0   dfail:0   fail:0   skip:39 
ro-bdw-i5-5250u  total:183  pass:172  dwarn:0   dfail:0   fail:0   skip:10 
ro-bsw-n3050     total:208  pass:167  dwarn:0   dfail:0   fail:2   skip:39 
ro-byt-n2820     total:208  pass:168  dwarn:0   dfail:0   fail:3   skip:37 
ro-hsw-i3-4010u  total:208  pass:185  dwarn:0   dfail:0   fail:0   skip:23 
ro-ilk1-i5-650   total:204  pass:146  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:183  pass:154  dwarn:0   dfail:0   fail:0   skip:28 
ro-ivb2-i7-3770  total:183  pass:158  dwarn:0   dfail:0   fail:0   skip:24 
ro-snb-i7-2620M  total:183  pass:151  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-i7-4770k failed to connect after reboot
ro-bdw-i7-5557U failed to connect after reboot
ro-bdw-i7-5600u failed to connect after reboot
ro-hsw-i7-4770r failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1137/

131a54b drm-intel-nightly: 2016y-06m-07d-19h-46m-33s UTC integration manifest
d22be7d4 drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission
b7d2be1 drm/i915/guc: refactor doorbell management code
e566931 drm/i915/guc: move guc_ring_doorbell() nearer to callsite
cd5f059 drm/i915/guc: add doorbell map to debugfs/i915_guc_info

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

Re: [PATCH 1/4] drm/i915/guc: add doorbell map to debugfs/i915_guc_info

[Tvrtko Ursulin]

On 08/06/16 11:55, Dave Gordon wrote:
> To properly verify the driver->doorbell->GuC functionality, validation
> needs to know how the driver has assigned the doorbell cache lines and
> registers, so make them visible through debugfs.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e4f2c55..fbb4b16 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2560,6 +2560,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
>   	struct i915_guc_client client = {};
>   	struct intel_engine_cs *engine;
>   	u64 total = 0;
> +	int i;
>
>   	if (!HAS_GUC_SCHED(dev_priv))
>   		return 0;
> @@ -2574,6 +2575,14 @@ static int i915_guc_info(struct seq_file *m, void *data)
>
>   	mutex_unlock(&dev->struct_mutex);
>
> +	seq_printf(m, "Doorbell map:\n");
> +	BUILD_BUG_ON(ARRAY_SIZE(guc.doorbell_bitmap) % 4);
> +	for (i = 0; i < ARRAY_SIZE(guc.doorbell_bitmap) - 3; i += 4)
> +		seq_printf(m, "\t%016lx %016lx %016lx %016lx\n",

Bitmap is unsigned long so 32-bit or 64-bit depending on the kernel 
build. Which makes the format wrong for 32-bit. And the ARRAY_SIZE will 
be different as well. So I think output should be made consistent 
between the two. Probably either pick u32 or u64 and dump it like that.

#define DECLARE_BITMAP(name,bits) \
         unsigned long name[BITS_TO_LONGS(bits)]

  DECLARE_BITMAP(doorbell_bitmap, GUC_MAX_DOORBELLS);

> +			guc.doorbell_bitmap[i], guc.doorbell_bitmap[i+1],
> +			guc.doorbell_bitmap[i+2], guc.doorbell_bitmap[i+3]);
> +	seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", guc.db_cacheline);
> +
>   	seq_printf(m, "GuC total action count: %llu\n", guc.action_count);
>   	seq_printf(m, "GuC action failure count: %u\n", guc.action_fail);
>   	seq_printf(m, "GuC last action command: 0x%x\n", guc.action_cmd);
>

Regards,

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

Re: [PATCH 2/4] drm/i915/guc: move guc_ring_doorbell() nearer to callsite

[Tvrtko Ursulin]

On 08/06/16 11:55, Dave Gordon wrote:
> Just code movement, no actual change to the function. This is in
> preparation for the next patch, which will reorganise all the other
> doorbell code, but doesn't change this function. So let's shuffle it
> down near its caller rather than leaving it mixed in with the setup
> code. Unlike the doorbell management code, this function is somewhat
> time-critical, so putting it near its caller may even yield a tiny
> performance improvement.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 110 ++++++++++++++---------------
>   1 file changed, 55 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 2db1182..7510841 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -185,61 +185,6 @@ static void guc_init_doorbell(struct intel_guc *guc,
>   	doorbell->cookie = 0;
>   }
>
> -static int guc_ring_doorbell(struct i915_guc_client *gc)
> -{
> -	struct guc_process_desc *desc;
> -	union guc_doorbell_qw db_cmp, db_exc, db_ret;
> -	union guc_doorbell_qw *db;
> -	int attempt = 2, ret = -EAGAIN;
> -
> -	desc = gc->client_base + gc->proc_desc_offset;
> -
> -	/* Update the tail so it is visible to GuC */
> -	desc->tail = gc->wq_tail;
> -
> -	/* current cookie */
> -	db_cmp.db_status = GUC_DOORBELL_ENABLED;
> -	db_cmp.cookie = gc->cookie;
> -
> -	/* cookie to be updated */
> -	db_exc.db_status = GUC_DOORBELL_ENABLED;
> -	db_exc.cookie = gc->cookie + 1;
> -	if (db_exc.cookie == 0)
> -		db_exc.cookie = 1;
> -
> -	/* pointer of current doorbell cacheline */
> -	db = gc->client_base + gc->doorbell_offset;
> -
> -	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 */
> -			gc->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;
> -
> -		DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n",
> -			  db_cmp.cookie, db_ret.cookie);
> -
> -		/* 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;
> -}
> -
>   static void guc_disable_doorbell(struct intel_guc *guc,
>   				 struct i915_guc_client *client)
>   {
> @@ -543,6 +488,61 @@ static void guc_add_workqueue_item(struct i915_guc_client *gc,
>   	kunmap_atomic(base);
>   }
>
> +static int guc_ring_doorbell(struct i915_guc_client *gc)
> +{
> +	struct guc_process_desc *desc;
> +	union guc_doorbell_qw db_cmp, db_exc, db_ret;
> +	union guc_doorbell_qw *db;
> +	int attempt = 2, ret = -EAGAIN;
> +
> +	desc = gc->client_base + gc->proc_desc_offset;
> +
> +	/* Update the tail so it is visible to GuC */
> +	desc->tail = gc->wq_tail;
> +
> +	/* current cookie */
> +	db_cmp.db_status = GUC_DOORBELL_ENABLED;
> +	db_cmp.cookie = gc->cookie;
> +
> +	/* cookie to be updated */
> +	db_exc.db_status = GUC_DOORBELL_ENABLED;
> +	db_exc.cookie = gc->cookie + 1;
> +	if (db_exc.cookie == 0)
> +		db_exc.cookie = 1;
> +
> +	/* pointer of current doorbell cacheline */
> +	db = gc->client_base + gc->doorbell_offset;
> +
> +	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 */
> +			gc->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;
> +
> +		DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n",
> +			  db_cmp.cookie, db_ret.cookie);
> +
> +		/* 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;
> +}
> +
>   /**
>    * i915_guc_submit() - Submit commands through GuC
>    * @rq:		request associated with the commands
>

Diffstat is correct, did not read the individual lines. :)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 3/4] drm/i915/guc: refactor doorbell management code

[Tvrtko Ursulin]

On 08/06/16 11:55, Dave Gordon wrote:
> During a hibernate/resume cycle, the driver, the GuC, and the doorbell
> hardware can end up in inconsistent states. This patch refactors the
> driver's handling and tracking of doorbells, in preparation for a later
> one which will resolve the issue.

Perhaps a few word on the goal, method and result of refactoring. Would 
be good for documentation and easier review.

> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 87 ++++++++++++++++++------------
>   1 file changed, 53 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 7510841..eef67c8 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -174,36 +174,63 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
>    * client object which contains the page being used for the doorbell
>    */
>
> -static void guc_init_doorbell(struct intel_guc *guc,
> -			      struct i915_guc_client *client)
> +static int guc_update_doorbell_id(struct i915_guc_client *client,
> +				  struct guc_doorbell_info *doorbell,
> +				  u16 new_id)
>   {
> -	struct guc_doorbell_info *doorbell;
> +	struct sg_table *sg = client->guc->ctx_pool_obj->pages;
> +	void *doorbell_bitmap = client->guc->doorbell_bitmap;
> +	struct guc_context_desc desc;
> +	size_t len;
> +
> +	if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
> +	    test_bit(client->doorbell_id, doorbell_bitmap)) {
> +		/* Deactivate the old doorbell */
> +		doorbell->db_status = GUC_DOORBELL_DISABLED;
> +		(void)host2guc_release_doorbell(client->guc, client);
> +		clear_bit(client->doorbell_id, doorbell_bitmap);
> +	}
>
> -	doorbell = client->client_base + client->doorbell_offset;
> +	/* Update the GuC's idea of the doorbell ID */
> +	len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
> +			     sizeof(desc) * client->ctx_index);
> +	if (len != sizeof(desc))
> +		return -EFAULT;
> +	desc.db_id = new_id;
> +	len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
> +			     sizeof(desc) * client->ctx_index);
> +	if (len != sizeof(desc))
> +		return -EFAULT;
> +
> +	client->doorbell_id = new_id;
> +	if (new_id == GUC_INVALID_DOORBELL_ID)
> +		return 0;
>
> +	/* Activate the new doorbell */
> +	set_bit(client->doorbell_id, doorbell_bitmap);
>   	doorbell->db_status = GUC_DOORBELL_ENABLED;
>   	doorbell->cookie = 0;
> +	return host2guc_allocate_doorbell(client->guc, client);

A bunch of new code here which wasn't done on doorbell init before. 
Populating the ctx_pool_obj and hust2guc call. I think the commit needs 
to explain what is happening here.

Maybe it is mostly a matter of copying over some text from the cover 
letter, but ideally it should explain what it is doing more precisely.

>   }
>
> -static void guc_disable_doorbell(struct intel_guc *guc,
> -				 struct i915_guc_client *client)
> +static void guc_init_doorbell(struct intel_guc *guc,
> +			      struct i915_guc_client *client,
> +			      uint16_t db_id)
>   {
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   	struct guc_doorbell_info *doorbell;
> -	i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
> -	int value;
>
>   	doorbell = client->client_base + client->doorbell_offset;
>
> -	doorbell->db_status = GUC_DOORBELL_DISABLED;
> -
> -	I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
> +	guc_update_doorbell_id(client, doorbell, db_id);
> +}

This function does not end up doing anything now, just a pass-through to 
guc_update_doorbell_id.

What happens to the write to drbreg ? It is completely gone and also 
from the init doorbell path together with the write to another register.

>
> -	value = I915_READ(drbreg);
> -	WARN_ON((value & GEN8_DRB_VALID) != 0);
> +static void guc_disable_doorbell(struct intel_guc *guc,
> +				 struct i915_guc_client *client)
> +{
> +	struct guc_doorbell_info *doorbell;
>
> -	I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
> -	I915_WRITE(drbreg, 0);
> +	doorbell = client->client_base + client->doorbell_offset;
> +	guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID);
>
>   	/* XXX: wait for any interrupts */
>   	/* XXX: wait for workqueue to drain */
> @@ -251,7 +278,7 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
>   	if (id == end)
>   		id = GUC_INVALID_DOORBELL_ID;
>   	else
> -		bitmap_set(guc->doorbell_bitmap, id, 1);
> +		set_bit(id, guc->doorbell_bitmap);

set_bit is atomic - is there a reason it is required here?

Actually the same question for the clear_bit above, where it was 
bitmap_clear before.

>
>   	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
>   			hi_pri ? "high" : "normal", id);
> @@ -259,11 +286,6 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
>   	return id;
>   }
>
> -static void release_doorbell(struct intel_guc *guc, uint16_t id)
> -{
> -	bitmap_clear(guc->doorbell_bitmap, id, 1);
> -}
> -
>   /*
>    * Initialise the process descriptor shared with the GuC firmware.
>    */
> @@ -657,8 +679,6 @@ static void guc_client_free(struct drm_device *dev,
>   	 */
>
>   	if (client->client_base) {
> -		uint16_t db_id = client->doorbell_id;
> -
>   		/*
>   		 * If we got as far as setting up a doorbell, make sure
>   		 * we shut it down before unmapping & deallocating the
> @@ -666,12 +686,8 @@ static void guc_client_free(struct drm_device *dev,
>   		 * GuC that we've finished with it, finally deallocate
>   		 * it in our bitmap
>   		 */
> -		if (db_id != GUC_INVALID_DOORBELL_ID) {
> +		if (client->doorbell_id != GUC_INVALID_DOORBELL_ID)
>   			guc_disable_doorbell(guc, client);
> -			if (test_bit(db_id, guc->doorbell_bitmap))
> -				host2guc_release_doorbell(guc, client);
> -			release_doorbell(guc, db_id);
> -		}
>
>   		kunmap(kmap_to_page(client->client_base));
>   	}
> @@ -706,6 +722,7 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc *guc = &dev_priv->guc;
>   	struct drm_i915_gem_object *obj;
> +	uint16_t db_id;
>
>   	client = kzalloc(sizeof(*client), GFP_KERNEL);
>   	if (!client)
> @@ -746,22 +763,24 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>   	else
>   		client->proc_desc_offset = (GUC_DB_SIZE / 2);
>
> -	client->doorbell_id = assign_doorbell(guc, client->priority);
> -	if (client->doorbell_id == GUC_INVALID_DOORBELL_ID)
> +	db_id = assign_doorbell(guc, client->priority);
> +	if (db_id == GUC_INVALID_DOORBELL_ID)
>   		/* XXX: evict a doorbell instead */
>   		goto err;
>
>   	guc_init_proc_desc(guc, client);
>   	guc_init_ctx_desc(guc, client);
> -	guc_init_doorbell(guc, client);
> +	guc_init_doorbell(guc, client, db_id);
>
>   	/* XXX: Any cache flushes needed? General domain mgmt calls? */
>
>   	if (host2guc_allocate_doorbell(guc, client))
>   		goto err;
>
> -	DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id %u\n",
> -		priority, client, client->ctx_index, client->doorbell_id);
> +	DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u\n",
> +		priority, client, client->ctx_index);
> +	DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n",
> +		client->doorbell_id, client->doorbell_offset);
>
>   	return client;
>
>

Regards,

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

Re: [PATCH 3/4] drm/i915/guc: refactor doorbell management code

[Dave Gordon]

On 08/06/16 14:11, Tvrtko Ursulin wrote:
>
> On 08/06/16 11:55, Dave Gordon wrote:
>> During a hibernate/resume cycle, the driver, the GuC, and the doorbell
>> hardware can end up in inconsistent states. This patch refactors the
>> driver's handling and tracking of doorbells, in preparation for a later
>> one which will resolve the issue.
>
> Perhaps a few word on the goal, method and result of refactoring. Would
> be good for documentation and easier review.

The refactoring is exactly that; there's very little new code.
All we're doing is centralising all doorbell management into
just one function and turning the (outermost) old functions
into wrappers for the single function.

There are three resources to be managed:
1. Cachelines: a single line within the client-object's page 0
    is snooped for writes by the host.
2. Doorbell registers: each defines a cacheline to be snooped.
3. Bitmap: tracks which registers are in use.

The doorbell setup/teardown protocol starts with:
1. Pick a cacheline: select_doorbell_cacheline()
2. Find an available doorbell (register): assign_doorbell()
(These values are passed to the GuC via the shared context
descriptor; this part of the sequence remains unchanged).

3. Update the bitmap to reflect registers-in-use
4. Prepare the cacheline for use by setting its status to ENABLED
5. Ask the GuC to program the doorbell to snoop the cacheline

and of course teardown is very similar:
6. Set the cacheline to DISABLED
7. Ask the GuC to reprogram the doorbell to stop snooping
8. Record that the doorbell is not in use.

Operations 6-8 (guc_disable_doorbell(), host2guc_release_doorbell(), and 
release_doorbell()) were called in sequence from guc_client_free(), but 
are now moved into the teardown part of the common function.

Steps 4-5 (guc_init_doorbell() and host2guc_allocate_doorbell()) were 
similarly done as sequential steps in guc_client_alloc(), but since it 
turns out we don't need to do them separately they're now collected into 
the setup part of the common function.

>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 87
>> ++++++++++++++++++------------
>>   1 file changed, 53 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 7510841..eef67c8 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -174,36 +174,63 @@ static int host2guc_sample_forcewake(struct
>> intel_guc *guc,
>>    * client object which contains the page being used for the doorbell
>>    */
>>
>> -static void guc_init_doorbell(struct intel_guc *guc,
>> -                  struct i915_guc_client *client)
>> +static int guc_update_doorbell_id(struct i915_guc_client *client,
>> +                  struct guc_doorbell_info *doorbell,
>> +                  u16 new_id)
>>   {
>> -    struct guc_doorbell_info *doorbell;
>> +    struct sg_table *sg = client->guc->ctx_pool_obj->pages;
>> +    void *doorbell_bitmap = client->guc->doorbell_bitmap;
>> +    struct guc_context_desc desc;
>> +    size_t len;
>> +
>> +    if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
>> +        test_bit(client->doorbell_id, doorbell_bitmap)) {
>> +        /* Deactivate the old doorbell */
>> +        doorbell->db_status = GUC_DOORBELL_DISABLED;
>> +        (void)host2guc_release_doorbell(client->guc, client);
>> +        clear_bit(client->doorbell_id, doorbell_bitmap);
>> +    }
>>
>> -    doorbell = client->client_base + client->doorbell_offset;
>> +    /* Update the GuC's idea of the doorbell ID */
>> +    len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
>> +                 sizeof(desc) * client->ctx_index);
>> +    if (len != sizeof(desc))
>> +        return -EFAULT;
>> +    desc.db_id = new_id;
>> +    len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
>> +                 sizeof(desc) * client->ctx_index);
>> +    if (len != sizeof(desc))
>> +        return -EFAULT;
>> +
>> +    client->doorbell_id = new_id;
>> +    if (new_id == GUC_INVALID_DOORBELL_ID)
>> +        return 0;
>>
>> +    /* Activate the new doorbell */
>> +    set_bit(client->doorbell_id, doorbell_bitmap);
>>       doorbell->db_status = GUC_DOORBELL_ENABLED;
>>       doorbell->cookie = 0;
>> +    return host2guc_allocate_doorbell(client->guc, client);
>
> A bunch of new code here which wasn't done on doorbell init before.
> Populating the ctx_pool_obj and hust2guc call. I think the commit needs
> to explain what is happening here.

The only new code (and new capability) is the block
     /* Update the GuC's idea of the doorbell ID */
i.e. we can now *change* the doorbell (register) used by an
existing client, whereas previously it was set once for the
entire lifetime of the client. We will use this new feature
in the next patch.

The host2guc call is not new, but it was previously called
separately after the return from guc_init_doorbell().

> Maybe it is mostly a matter of copying over some text from the cover
> letter, but ideally it should explain what it is doing more precisely.

I'll put some of this reply into the commit message

>>   }
>>
>> -static void guc_disable_doorbell(struct intel_guc *guc,
>> -                 struct i915_guc_client *client)
>> +static void guc_init_doorbell(struct intel_guc *guc,
>> +                  struct i915_guc_client *client,
>> +                  uint16_t db_id)
>>   {
>> -    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>       struct guc_doorbell_info *doorbell;
>> -    i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
>> -    int value;
>>
>>       doorbell = client->client_base + client->doorbell_offset;
>>
>> -    doorbell->db_status = GUC_DOORBELL_DISABLED;
>> -
>> -    I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
>> +    guc_update_doorbell_id(client, doorbell, db_id);
>> +}
>
> This function does not end up doing anything now, just a pass-through to
> guc_update_doorbell_id.

Correct. It's retained to make it clear that here we're initialising a 
newly-created client with a doorbell assignment for the first time, as 
opposed to updating an existing doorbell assignment. Similarly we retain 
guc_disable_doorbell() as an alias for changing the doorbell ID (back) 
to INVALID.

> What happens to the write to drbreg ? It is completely gone and also
> from the init doorbell path together with the write to another register.

Those writes were useless. Turns out those registers are not writeable 
by the CPU; they can *only* be updated by the GuC!

>> -    value = I915_READ(drbreg);
>> -    WARN_ON((value & GEN8_DRB_VALID) != 0);
>> +static void guc_disable_doorbell(struct intel_guc *guc,
>> +                 struct i915_guc_client *client)
>> +{
>> +    struct guc_doorbell_info *doorbell;
>>
>> -    I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
>> -    I915_WRITE(drbreg, 0);
>> +    doorbell = client->client_base + client->doorbell_offset;
>> +    guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID);
>>
>>       /* XXX: wait for any interrupts */
>>       /* XXX: wait for workqueue to drain */
>> @@ -251,7 +278,7 @@ static uint16_t assign_doorbell(struct intel_guc
>> *guc, uint32_t priority)
>>       if (id == end)
>>           id = GUC_INVALID_DOORBELL_ID;
>>       else
>> -        bitmap_set(guc->doorbell_bitmap, id, 1);
>> +        set_bit(id, guc->doorbell_bitmap);
>
> set_bit is atomic - is there a reason it is required here?
>
> Actually the same question for the clear_bit above, where it was
> bitmap_clear before.

The bitmap_set/clear functions don't have a comparable TEST
operator, so we have to use test_bit(). The others were just
changed for symmetry. We don't care about atomic or not, and
this is rarely executed (setup/teardown) code so we don't worry
about performance either.

Actually, though, I'd expect the one-bit functions set_bit()
and clear_bit() to be MORE efficient than the general multibit
operations bitmap_set/clear. They should get inlined too :)

We could use __set/clear_bit() if we really don't want the LOCK.

New version later ...

.Dave.

>>       DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
>>               hi_pri ? "high" : "normal", id);
>> @@ -259,11 +286,6 @@ static uint16_t assign_doorbell(struct intel_guc
>> *guc, uint32_t priority)
>>       return id;
>>   }
>>
>> -static void release_doorbell(struct intel_guc *guc, uint16_t id)
>> -{
>> -    bitmap_clear(guc->doorbell_bitmap, id, 1);
>> -}
>> -
>>   /*
>>    * Initialise the process descriptor shared with the GuC firmware.
>>    */
>> @@ -657,8 +679,6 @@ static void guc_client_free(struct drm_device *dev,
>>        */
>>
>>       if (client->client_base) {
>> -        uint16_t db_id = client->doorbell_id;
>> -
>>           /*
>>            * If we got as far as setting up a doorbell, make sure
>>            * we shut it down before unmapping & deallocating the
>> @@ -666,12 +686,8 @@ static void guc_client_free(struct drm_device *dev,
>>            * GuC that we've finished with it, finally deallocate
>>            * it in our bitmap
>>            */
>> -        if (db_id != GUC_INVALID_DOORBELL_ID) {
>> +        if (client->doorbell_id != GUC_INVALID_DOORBELL_ID)
>>               guc_disable_doorbell(guc, client);
>> -            if (test_bit(db_id, guc->doorbell_bitmap))
>> -                host2guc_release_doorbell(guc, client);
>> -            release_doorbell(guc, db_id);
>> -        }
>>
>>           kunmap(kmap_to_page(client->client_base));
>>       }
>> @@ -706,6 +722,7 @@ static struct i915_guc_client
>> *guc_client_alloc(struct drm_device *dev,
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       struct intel_guc *guc = &dev_priv->guc;
>>       struct drm_i915_gem_object *obj;
>> +    uint16_t db_id;
>>
>>       client = kzalloc(sizeof(*client), GFP_KERNEL);
>>       if (!client)
>> @@ -746,22 +763,24 @@ static struct i915_guc_client
>> *guc_client_alloc(struct drm_device *dev,
>>       else
>>           client->proc_desc_offset = (GUC_DB_SIZE / 2);
>>
>> -    client->doorbell_id = assign_doorbell(guc, client->priority);
>> -    if (client->doorbell_id == GUC_INVALID_DOORBELL_ID)
>> +    db_id = assign_doorbell(guc, client->priority);
>> +    if (db_id == GUC_INVALID_DOORBELL_ID)
>>           /* XXX: evict a doorbell instead */
>>           goto err;
>>
>>       guc_init_proc_desc(guc, client);
>>       guc_init_ctx_desc(guc, client);
>> -    guc_init_doorbell(guc, client);
>> +    guc_init_doorbell(guc, client, db_id);
>>
>>       /* XXX: Any cache flushes needed? General domain mgmt calls? */
>>
>>       if (host2guc_allocate_doorbell(guc, client))
>>           goto err;
>>
>> -    DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id
>> %u\n",
>> -        priority, client, client->ctx_index, client->doorbell_id);
>> +    DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u\n",
>> +        priority, client, client->ctx_index);
>> +    DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n",
>> +        client->doorbell_id, client->doorbell_offset);
>>
>>       return client;
>>
>>
>
> Regards,
> Tvrtko

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