[PATCH v2 0/6] drm/i915/guc: updates to GuC doorbell handling

[PATCH v2 0/6] drm/i915/guc: updates to GuC doorbell handling

[Dave Gordon]

Various GuC (doorbell) related patches, some trivial. The bulk of the
changes are in [patch 5/6], but it's mostly just reorganisation of
existing code. The new functionality is implemented in a single new
function in [patch 6/6].

Background, from v1 of this patchset:

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 (6):
  drm/i915/guc: add doorbell map to debugfs/i915_guc_info
  drm/i915/guc: move guc_ring_doorbell() nearer to callsite
  drm/i915/guc: prefer __set/clear_bit() to bitmap_set/clear()
  drm/i915/guc: remove writes to GEN8_DRBREG registers
  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        |   4 +
 drivers/gpu/drm/i915/i915_guc_submission.c | 249 +++++++++++++++++------------
 2 files changed, 153 insertions(+), 100 deletions(-)

-- 
1.9.1

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

[PATCH v2 1/6] 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.

v2: use kernel bitmap-printing format (%pb) rather than %x.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e4f2c55..6efc8b1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2574,6 +2574,10 @@ static int i915_guc_info(struct seq_file *m, void *data)
 
 	mutex_unlock(&dev->struct_mutex);
 
+	seq_printf(m, "Doorbell map:\n");
+	seq_printf(m, "\t%*pb\n", GUC_MAX_DOORBELLS, guc.doorbell_bitmap);
+	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 v2 2/6] 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>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@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 v2 3/6] drm/i915/guc: prefer __set/clear_bit() to bitmap_set/clear()

[Dave Gordon]

Bitmap operators are overkill when touching only one bit.

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 7510841..e198599 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -251,7 +251,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);
@@ -261,7 +261,7 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
 
 static void release_doorbell(struct intel_guc *guc, uint16_t id)
 {
-	bitmap_clear(guc->doorbell_bitmap, id, 1);
+	__clear_bit(id, guc->doorbell_bitmap);
 }
 
 /*
-- 
1.9.1

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

[PATCH v2 4/6] drm/i915/guc: remove writes to GEN8_DRBREG registers

[Dave Gordon]

These registers are not actually writable by the CPU; only the GuC can
actually program them. So let's not do writes that have no effect.

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index e198599..45b33f8 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -197,14 +197,9 @@ static void guc_disable_doorbell(struct intel_guc *guc,
 
 	doorbell->db_status = GUC_DOORBELL_DISABLED;
 
-	I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
-
 	value = I915_READ(drbreg);
 	WARN_ON((value & GEN8_DRB_VALID) != 0);
 
-	I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
-	I915_WRITE(drbreg, 0);
-
 	/* XXX: wait for any interrupts */
 	/* XXX: wait for workqueue to drain */
 }
-- 
1.9.1

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

[PATCH v2 5/6] drm/i915/guc: refactor doorbell management code

[Dave Gordon]

This patch refactors the driver's handling and tracking of doorbells, in
preparation for a later one which will resolve a suspend-resume issue.

There are three resources to be managed:
1. Cachelines: a single line within the client-object's page 0
   is snooped by doorbell hardware for writes from the host.
2. Doorbell registers: each defines one cacheline to be snooped.
3. Bitmap: tracks which doorbell 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 phase 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 that we don't need to be able to do them separately they're
now collected into the setup phase of the common function.

The only new code (and new capability) is the block tagged
    /* 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.

v2: Trivial independent fixes pushed ahead as separate patches.
    MUCH longer commit message :) [Tvrtko Ursulin]

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 45b33f8..1833bfd 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -174,31 +174,59 @@ 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 intel_guc *guc,
+				  struct i915_guc_client *client,
+				  u16 new_id)
 {
+	struct sg_table *sg = guc->ctx_pool_obj->pages;
+	void *doorbell_bitmap = guc->doorbell_bitmap;
 	struct guc_doorbell_info *doorbell;
+	struct guc_context_desc desc;
+	size_t len;
 
 	doorbell = client->client_base + client->doorbell_offset;
 
-	doorbell->db_status = GUC_DOORBELL_ENABLED;
+	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(guc, client);
+		__clear_bit(client->doorbell_id, doorbell_bitmap);
+	}
+
+	/* 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(new_id, doorbell_bitmap);
 	doorbell->cookie = 0;
+	doorbell->db_status = GUC_DOORBELL_ENABLED;
+	return host2guc_allocate_doorbell(guc, client);
+}
+
+static int guc_init_doorbell(struct intel_guc *guc,
+			      struct i915_guc_client *client,
+			      uint16_t db_id)
+{
+	return guc_update_doorbell_id(guc, client, db_id);
 }
 
 static void guc_disable_doorbell(struct intel_guc *guc,
 				 struct i915_guc_client *client)
 {
-	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;
-
-	value = I915_READ(drbreg);
-	WARN_ON((value & GEN8_DRB_VALID) != 0);
+	(void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
 
 	/* XXX: wait for any interrupts */
 	/* XXX: wait for workqueue to drain */
@@ -254,11 +282,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)
-{
-	__clear_bit(id, guc->doorbell_bitmap);
-}
-
 /*
  * Initialise the process descriptor shared with the GuC firmware.
  */
@@ -652,21 +675,11 @@ 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
-		 * memory. So first disable the doorbell, then tell the
-		 * GuC that we've finished with it, finally deallocate
-		 * it in our bitmap
+		 * If we got as far as setting up a doorbell, make sure we
+		 * shut it down before unmapping & deallocating the memory.
 		 */
-		if (db_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);
-		}
+		guc_disable_doorbell(guc, client);
 
 		kunmap(kmap_to_page(client->client_base));
 	}
@@ -701,6 +714,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)
@@ -741,22 +755,20 @@ 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);
-
-	/* XXX: Any cache flushes needed? General domain mgmt calls? */
-
-	if (host2guc_allocate_doorbell(guc, client))
+	if (guc_init_doorbell(guc, client, db_id))
 		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 v2 6/6] 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). 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/or the newly-loaded GuC firmware.

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 | 44 +++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 1833bfd..120d2e8 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -694,6 +694,48 @@ 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;
+	uint16_t db_id, i;
+	int err;
+
+	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);
+
+		err = guc_update_doorbell_id(guc, client, i);
+
+		/* Report update failure or unexpectedly active doorbell */
+		if (err || (i != db_id && (value & GUC_DOORBELL_ENABLED)))
+			DRM_DEBUG_DRIVER("Doorbell %d (reg 0x%x) was 0x%x, err %d\n",
+					  i, drbreg.reg, value, err);
+	}
+
+	/* Restore to original value */
+	err = guc_update_doorbell_id(guc, client, db_id);
+	if (err)
+		DRM_ERROR("Failed to restore doorbell to %d, err %d\n",
+			db_id, err);
+
+	for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
+		i915_reg_t drbreg = GEN8_DRBREGL(i);
+		u32 value = I915_READ(drbreg);
+
+		if (i != db_id && (value & GUC_DOORBELL_ENABLED))
+			DRM_DEBUG_DRIVER("Doorbell %d (reg 0x%x) finally 0x%x\n",
+					  i, drbreg.reg, value);
+
+	}
+}
+
 /**
  * guc_client_alloc() - Allocate an i915_guc_client
  * @dev:	drm device
@@ -959,8 +1001,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/guc: updates to GuC doorbell handling

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                dmesg-warn -> PASS       (ro-hsw-i3-4010u)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                skip       -> PASS       (fi-skl-i5-6260u)
        Subgroup suspend-read-crc-pipe-c:
                skip       -> PASS       (fi-skl-i5-6260u)

fi-bdw-i7-5557u  total:213  pass:201  dwarn:0   dfail:0   fail:0   skip:12 
fi-skl-i5-6260u  total:213  pass:202  dwarn:0   dfail:0   fail:0   skip:11 
fi-skl-i7-6700k  total:213  pass:188  dwarn:0   dfail:0   fail:0   skip:25 
fi-snb-i7-2600   total:213  pass:174  dwarn:0   dfail:0   fail:0   skip:39 
ro-bdw-i7-5600u  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
ro-bsw-n3050     total:213  pass:172  dwarn:0   dfail:0   fail:2   skip:39 
ro-byt-n2820     total:213  pass:173  dwarn:0   dfail:0   fail:3   skip:37 
ro-hsw-i3-4010u  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
ro-ilk-i7-620lm  total:213  pass:150  dwarn:0   dfail:0   fail:1   skip:62 
ro-ilk1-i5-650   total:208  pass:150  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:213  pass:181  dwarn:0   dfail:0   fail:0   skip:32 
ro-ivb2-i7-3770  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
ro-skl3-i5-6260u total:213  pass:201  dwarn:1   dfail:0   fail:0   skip:11 
ro-snb-i7-2620M  total:213  pass:174  dwarn:0   dfail:0   fail:1   skip:38 
fi-hsw-i7-4770k failed to connect after reboot
ro-bdw-i5-5250u failed to connect after reboot
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1161/

94fd582 drm-intel-nightly: 2016y-06m-10d-16h-42m-36s UTC integration manifest
fd29e26 drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission
3c41a24 drm/i915/guc: refactor doorbell management code
a691e53 drm/i915/guc: remove writes to GEN8_DRBREG registers
bcb2128 drm/i915/guc: prefer __set/clear_bit() to bitmap_set/clear()
893dfd7 drm/i915/guc: move guc_ring_doorbell() nearer to callsite
11b028f 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 v2 1/6] drm/i915/guc: add doorbell map to debugfs/i915_guc_info

[Tvrtko Ursulin]

On 10/06/16 17:50, 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.
>
> v2: use kernel bitmap-printing format (%pb) rather than %x.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e4f2c55..6efc8b1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2574,6 +2574,10 @@ static int i915_guc_info(struct seq_file *m, void *data)
>
>   	mutex_unlock(&dev->struct_mutex);
>
> +	seq_printf(m, "Doorbell map:\n");
> +	seq_printf(m, "\t%*pb\n", GUC_MAX_DOORBELLS, guc.doorbell_bitmap);
> +	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);
>

Didn't know about this one, cool.

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 v2 3/6] drm/i915/guc: prefer __set/clear_bit() to bitmap_set/clear()

[Tvrtko Ursulin]

On 10/06/16 17:50, Dave Gordon wrote:
> Bitmap operators are overkill when touching only one bit.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 7510841..e198599 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -251,7 +251,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);
> @@ -261,7 +261,7 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
>
>   static void release_doorbell(struct intel_guc *guc, uint16_t id)
>   {
> -	bitmap_clear(guc->doorbell_bitmap, id, 1);
> +	__clear_bit(id, guc->doorbell_bitmap);
>   }
>
>   /*
>

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 v2 4/6] drm/i915/guc: remove writes to GEN8_DRBREG registers

[Tvrtko Ursulin]

On 10/06/16 17:50, Dave Gordon wrote:
> These registers are not actually writable by the CPU; only the GuC can
> actually program them. So let's not do writes that have no effect.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 5 -----
>   1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index e198599..45b33f8 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -197,14 +197,9 @@ static void guc_disable_doorbell(struct intel_guc *guc,
>
>   	doorbell->db_status = GUC_DOORBELL_DISABLED;
>
> -	I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
> -
>   	value = I915_READ(drbreg);
>   	WARN_ON((value & GEN8_DRB_VALID) != 0);
>
> -	I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
> -	I915_WRITE(drbreg, 0);
> -
>   	/* XXX: wait for any interrupts */
>   	/* XXX: wait for workqueue to drain */
>   }
>

I have to trust you on this one. :)

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 v2 5/6] drm/i915/guc: refactor doorbell management code

[Tvrtko Ursulin]

On 10/06/16 17:51, Dave Gordon wrote:
> This patch refactors the driver's handling and tracking of doorbells, in
> preparation for a later one which will resolve a suspend-resume issue.
>
> There are three resources to be managed:
> 1. Cachelines: a single line within the client-object's page 0
>     is snooped by doorbell hardware for writes from the host.
> 2. Doorbell registers: each defines one cacheline to be snooped.
> 3. Bitmap: tracks which doorbell 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 phase 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 that we don't need to be able to do them separately they're
> now collected into the setup phase of the common function.
>
> The only new code (and new capability) is the block tagged
>      /* 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.
>
> v2: Trivial independent fixes pushed ahead as separate patches.
>      MUCH longer commit message :) [Tvrtko Ursulin]
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 94 +++++++++++++++++-------------
>   1 file changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 45b33f8..1833bfd 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -174,31 +174,59 @@ 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 intel_guc *guc,
> +				  struct i915_guc_client *client,
> +				  u16 new_id)
>   {
> +	struct sg_table *sg = guc->ctx_pool_obj->pages;
> +	void *doorbell_bitmap = guc->doorbell_bitmap;
>   	struct guc_doorbell_info *doorbell;
> +	struct guc_context_desc desc;
> +	size_t len;
>
>   	doorbell = client->client_base + client->doorbell_offset;
>
> -	doorbell->db_status = GUC_DOORBELL_ENABLED;
> +	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(guc, client);
> +		__clear_bit(client->doorbell_id, doorbell_bitmap);
> +	}
> +
> +	/* 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(new_id, doorbell_bitmap);

Is this the same bit as in assign_doorbell so redundant?

>   	doorbell->cookie = 0;
> +	doorbell->db_status = GUC_DOORBELL_ENABLED;
> +	return host2guc_allocate_doorbell(guc, client);
> +}
> +
> +static int guc_init_doorbell(struct intel_guc *guc,
> +			      struct i915_guc_client *client,
> +			      uint16_t db_id)
> +{
> +	return guc_update_doorbell_id(guc, client, db_id);
>   }
>
>   static void guc_disable_doorbell(struct intel_guc *guc,
>   				 struct i915_guc_client *client)
>   {
> -	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;
> -
> -	value = I915_READ(drbreg);
> -	WARN_ON((value & GEN8_DRB_VALID) != 0);
> +	(void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
>
>   	/* XXX: wait for any interrupts */
>   	/* XXX: wait for workqueue to drain */
> @@ -254,11 +282,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)
> -{
> -	__clear_bit(id, guc->doorbell_bitmap);
> -}
> -
>   /*
>    * Initialise the process descriptor shared with the GuC firmware.
>    */
> @@ -652,21 +675,11 @@ 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
> -		 * memory. So first disable the doorbell, then tell the
> -		 * GuC that we've finished with it, finally deallocate
> -		 * it in our bitmap
> +		 * If we got as far as setting up a doorbell, make sure we
> +		 * shut it down before unmapping & deallocating the memory.
>   		 */
> -		if (db_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);
> -		}
> +		guc_disable_doorbell(guc, client);
>
>   		kunmap(kmap_to_page(client->client_base));
>   	}
> @@ -701,6 +714,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)
> @@ -741,22 +755,20 @@ 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);
> -
> -	/* XXX: Any cache flushes needed? General domain mgmt calls? */
> -
> -	if (host2guc_allocate_doorbell(guc, client))
> +	if (guc_init_doorbell(guc, client, db_id))
>   		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;
>
>

Otherwise looks good to me, much more easier to understand what is 
happening now.

Regards,

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

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

[Tvrtko Ursulin]

On 10/06/16 17:51, Dave Gordon wrote:
> 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). 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/or the newly-loaded GuC firmware.
>
> 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 | 44 +++++++++++++++++++++++++++++-
>   1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 1833bfd..120d2e8 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -694,6 +694,48 @@ 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;
> +	uint16_t db_id, i;
> +	int err;
> +
> +	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);
> +
> +		err = guc_update_doorbell_id(guc, client, i);
> +
> +		/* Report update failure or unexpectedly active doorbell */
> +		if (err || (i != db_id && (value & GUC_DOORBELL_ENABLED)))
> +			DRM_DEBUG_DRIVER("Doorbell %d (reg 0x%x) was 0x%x, err %d\n",
> +					  i, drbreg.reg, value, err);
> +	}
> +
> +	/* Restore to original value */
> +	err = guc_update_doorbell_id(guc, client, db_id);
> +	if (err)
> +		DRM_ERROR("Failed to restore doorbell to %d, err %d\n",
> +			db_id, err);
> +
> +	for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
> +		i915_reg_t drbreg = GEN8_DRBREGL(i);
> +		u32 value = I915_READ(drbreg);
> +
> +		if (i != db_id && (value & GUC_DOORBELL_ENABLED))
> +			DRM_DEBUG_DRIVER("Doorbell %d (reg 0x%x) finally 0x%x\n",
> +					  i, drbreg.reg, value);
> +
> +	}
> +}
> +
>   /**
>    * guc_client_alloc() - Allocate an i915_guc_client
>    * @dev:	drm device
> @@ -959,8 +1001,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;
>   }
>

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 v2 5/6] drm/i915/guc: refactor doorbell management code

[Dave Gordon]

On 13/06/16 10:48, Tvrtko Ursulin wrote:
>
> On 10/06/16 17:51, Dave Gordon wrote:
>> This patch refactors the driver's handling and tracking of doorbells, in
>> preparation for a later one which will resolve a suspend-resume issue.
>>
>> There are three resources to be managed:
>> 1. Cachelines: a single line within the client-object's page 0
>>     is snooped by doorbell hardware for writes from the host.
>> 2. Doorbell registers: each defines one cacheline to be snooped.
>> 3. Bitmap: tracks which doorbell 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 phase 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 that we don't need to be able to do them separately they're
>> now collected into the setup phase of the common function.
>>
>> The only new code (and new capability) is the block tagged
>>      /* 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.
>>
>> v2: Trivial independent fixes pushed ahead as separate patches.
>>      MUCH longer commit message :) [Tvrtko Ursulin]
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 94
>> +++++++++++++++++-------------
>>   1 file changed, 53 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 45b33f8..1833bfd 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -174,31 +174,59 @@ 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 intel_guc *guc,
>> +                  struct i915_guc_client *client,
>> +                  u16 new_id)
>>   {
>> +    struct sg_table *sg = guc->ctx_pool_obj->pages;
>> +    void *doorbell_bitmap = guc->doorbell_bitmap;
>>       struct guc_doorbell_info *doorbell;
>> +    struct guc_context_desc desc;
>> +    size_t len;
>>
>>       doorbell = client->client_base + client->doorbell_offset;
>>
>> -    doorbell->db_status = GUC_DOORBELL_ENABLED;
>> +    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(guc, client);
>> +        __clear_bit(client->doorbell_id, doorbell_bitmap);
>> +    }
>> +
>> +    /* 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(new_id, doorbell_bitmap);
>
> Is this the same bit as in assign_doorbell so redundant?

It is the same bit, and yes, it will be redundant during the initial 
setup, but when we come to *re*assign the association between a client 
and a doorbell (in the next patch) then it won't be.

We could also choose to have assign_doorbell() NOT update the map, so 
then this would be the only place where the bitmap gets updated. I'd 
have to rename it though, as it would no longer be assigning anything!

.Dave.

>>       doorbell->cookie = 0;
>> +    doorbell->db_status = GUC_DOORBELL_ENABLED;
>> +    return host2guc_allocate_doorbell(guc, client);
>> +}
>> +
>> +static int guc_init_doorbell(struct intel_guc *guc,
>> +                  struct i915_guc_client *client,
>> +                  uint16_t db_id)
>> +{
>> +    return guc_update_doorbell_id(guc, client, db_id);
>>   }
>>
>>   static void guc_disable_doorbell(struct intel_guc *guc,
>>                    struct i915_guc_client *client)
>>   {
>> -    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;
>> -
>> -    value = I915_READ(drbreg);
>> -    WARN_ON((value & GEN8_DRB_VALID) != 0);
>> +    (void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
>>
>>       /* XXX: wait for any interrupts */
>>       /* XXX: wait for workqueue to drain */
>> @@ -254,11 +282,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)
>> -{
>> -    __clear_bit(id, guc->doorbell_bitmap);
>> -}
>> -
>>   /*
>>    * Initialise the process descriptor shared with the GuC firmware.
>>    */
>> @@ -652,21 +675,11 @@ 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
>> -         * memory. So first disable the doorbell, then tell the
>> -         * GuC that we've finished with it, finally deallocate
>> -         * it in our bitmap
>> +         * If we got as far as setting up a doorbell, make sure we
>> +         * shut it down before unmapping & deallocating the memory.
>>            */
>> -        if (db_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);
>> -        }
>> +        guc_disable_doorbell(guc, client);
>>
>>           kunmap(kmap_to_page(client->client_base));
>>       }
>> @@ -701,6 +714,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)
>> @@ -741,22 +755,20 @@ 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);
>> -
>> -    /* XXX: Any cache flushes needed? General domain mgmt calls? */
>> -
>> -    if (host2guc_allocate_doorbell(guc, client))
>> +    if (guc_init_doorbell(guc, client, db_id))
>>           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;
>>
>>
>
> Otherwise looks good to me, much more easier to understand what is
> happening now.
>
> Regards,
>
> Tvrtko

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

Re: [PATCH v2 5/6] drm/i915/guc: refactor doorbell management code

[Tvrtko Ursulin]

On 13/06/16 11:25, Dave Gordon wrote:
> On 13/06/16 10:48, Tvrtko Ursulin wrote:
>>
>> On 10/06/16 17:51, Dave Gordon wrote:
>>> This patch refactors the driver's handling and tracking of doorbells, in
>>> preparation for a later one which will resolve a suspend-resume issue.
>>>
>>> There are three resources to be managed:
>>> 1. Cachelines: a single line within the client-object's page 0
>>>     is snooped by doorbell hardware for writes from the host.
>>> 2. Doorbell registers: each defines one cacheline to be snooped.
>>> 3. Bitmap: tracks which doorbell 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 phase 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 that we don't need to be able to do them separately they're
>>> now collected into the setup phase of the common function.
>>>
>>> The only new code (and new capability) is the block tagged
>>>      /* 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.
>>>
>>> v2: Trivial independent fixes pushed ahead as separate patches.
>>>      MUCH longer commit message :) [Tvrtko Ursulin]
>>>
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 94
>>> +++++++++++++++++-------------
>>>   1 file changed, 53 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index 45b33f8..1833bfd 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -174,31 +174,59 @@ 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 intel_guc *guc,
>>> +                  struct i915_guc_client *client,
>>> +                  u16 new_id)
>>>   {
>>> +    struct sg_table *sg = guc->ctx_pool_obj->pages;
>>> +    void *doorbell_bitmap = guc->doorbell_bitmap;
>>>       struct guc_doorbell_info *doorbell;
>>> +    struct guc_context_desc desc;
>>> +    size_t len;
>>>
>>>       doorbell = client->client_base + client->doorbell_offset;
>>>
>>> -    doorbell->db_status = GUC_DOORBELL_ENABLED;
>>> +    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(guc, client);
>>> +        __clear_bit(client->doorbell_id, doorbell_bitmap);
>>> +    }
>>> +
>>> +    /* 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(new_id, doorbell_bitmap);
>>
>> Is this the same bit as in assign_doorbell so redundant?
>
> It is the same bit, and yes, it will be redundant during the initial
> setup, but when we come to *re*assign the association between a client
> and a doorbell (in the next patch) then it won't be.
>
> We could also choose to have assign_doorbell() NOT update the map, so
> then this would be the only place where the bitmap gets updated. I'd
> have to rename it though, as it would no longer be assigning anything!

Maybe pick_doorbell or find_free_doorbell? It would be a little bit 
clearer and safer (early returns above could leave the bitmap set) with 
a single bitmap update location so I think it would be worth it.

Regards,

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

Re: [PATCH v2 5/6] drm/i915/guc: refactor doorbell management code

[Dave Gordon]

On 13/06/16 11:53, Tvrtko Ursulin wrote:
>
> On 13/06/16 11:25, Dave Gordon wrote:
>> On 13/06/16 10:48, Tvrtko Ursulin wrote:
>>>
>>> On 10/06/16 17:51, Dave Gordon wrote:
>>>> This patch refactors the driver's handling and tracking of
>>>> doorbells, in
>>>> preparation for a later one which will resolve a suspend-resume issue.
>>>>
>>>> There are three resources to be managed:
>>>> 1. Cachelines: a single line within the client-object's page 0
>>>>     is snooped by doorbell hardware for writes from the host.
>>>> 2. Doorbell registers: each defines one cacheline to be snooped.
>>>> 3. Bitmap: tracks which doorbell 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 phase 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 that we don't need to be able to do them separately they're
>>>> now collected into the setup phase of the common function.
>>>>
>>>> The only new code (and new capability) is the block tagged
>>>>      /* 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.
>>>>
>>>> v2: Trivial independent fixes pushed ahead as separate patches.
>>>>      MUCH longer commit message :) [Tvrtko Ursulin]
>>>>
>>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 94
>>>> +++++++++++++++++-------------
>>>>   1 file changed, 53 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> index 45b33f8..1833bfd 100644
>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> @@ -174,31 +174,59 @@ 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 intel_guc *guc,
>>>> +                  struct i915_guc_client *client,
>>>> +                  u16 new_id)
>>>>   {
>>>> +    struct sg_table *sg = guc->ctx_pool_obj->pages;
>>>> +    void *doorbell_bitmap = guc->doorbell_bitmap;
>>>>       struct guc_doorbell_info *doorbell;
>>>> +    struct guc_context_desc desc;
>>>> +    size_t len;
>>>>
>>>>       doorbell = client->client_base + client->doorbell_offset;
>>>>
>>>> -    doorbell->db_status = GUC_DOORBELL_ENABLED;
>>>> +    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(guc, client);
>>>> +        __clear_bit(client->doorbell_id, doorbell_bitmap);
>>>> +    }
>>>> +
>>>> +    /* 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(new_id, doorbell_bitmap);
>>>
>>> Is this the same bit as in assign_doorbell so redundant?
>>
>> It is the same bit, and yes, it will be redundant during the initial
>> setup, but when we come to *re*assign the association between a client
>> and a doorbell (in the next patch) then it won't be.
>>
>> We could also choose to have assign_doorbell() NOT update the map, so
>> then this would be the only place where the bitmap gets updated. I'd
>> have to rename it though, as it would no longer be assigning anything!
>
> Maybe pick_doorbell or find_free_doorbell? It would be a little bit
> clearer and safer (early returns above could leave the bitmap set) with
> a single bitmap update location so I think it would be worth it.
>
> Regards,
> Tvrtko

Roger wilco, but it looks simpler if I make it a followup [7/6]?
Otherwise (if I mix it into this rework) git scrambles the diff
around so its less easy to see how the code was reorganised.

Update patch will follow soon ...

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

Re: [PATCH v2 5/6] drm/i915/guc: refactor doorbell management code

[Tvrtko Ursulin]

On 13/06/16 16:59, Dave Gordon wrote:
> On 13/06/16 11:53, Tvrtko Ursulin wrote:
>>
>> On 13/06/16 11:25, Dave Gordon wrote:
>>> On 13/06/16 10:48, Tvrtko Ursulin wrote:
>>>>
>>>> On 10/06/16 17:51, Dave Gordon wrote:
>>>>> This patch refactors the driver's handling and tracking of
>>>>> doorbells, in
>>>>> preparation for a later one which will resolve a suspend-resume issue.
>>>>>
>>>>> There are three resources to be managed:
>>>>> 1. Cachelines: a single line within the client-object's page 0
>>>>>     is snooped by doorbell hardware for writes from the host.
>>>>> 2. Doorbell registers: each defines one cacheline to be snooped.
>>>>> 3. Bitmap: tracks which doorbell 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 phase 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 that we don't need to be able to do them separately they're
>>>>> now collected into the setup phase of the common function.
>>>>>
>>>>> The only new code (and new capability) is the block tagged
>>>>>      /* 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.
>>>>>
>>>>> v2: Trivial independent fixes pushed ahead as separate patches.
>>>>>      MUCH longer commit message :) [Tvrtko Ursulin]
>>>>>
>>>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 94
>>>>> +++++++++++++++++-------------
>>>>>   1 file changed, 53 insertions(+), 41 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>>> index 45b33f8..1833bfd 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>>> @@ -174,31 +174,59 @@ 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 intel_guc *guc,
>>>>> +                  struct i915_guc_client *client,
>>>>> +                  u16 new_id)
>>>>>   {
>>>>> +    struct sg_table *sg = guc->ctx_pool_obj->pages;
>>>>> +    void *doorbell_bitmap = guc->doorbell_bitmap;
>>>>>       struct guc_doorbell_info *doorbell;
>>>>> +    struct guc_context_desc desc;
>>>>> +    size_t len;
>>>>>
>>>>>       doorbell = client->client_base + client->doorbell_offset;
>>>>>
>>>>> -    doorbell->db_status = GUC_DOORBELL_ENABLED;
>>>>> +    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(guc, client);
>>>>> +        __clear_bit(client->doorbell_id, doorbell_bitmap);
>>>>> +    }
>>>>> +
>>>>> +    /* 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(new_id, doorbell_bitmap);
>>>>
>>>> Is this the same bit as in assign_doorbell so redundant?
>>>
>>> It is the same bit, and yes, it will be redundant during the initial
>>> setup, but when we come to *re*assign the association between a client
>>> and a doorbell (in the next patch) then it won't be.
>>>
>>> We could also choose to have assign_doorbell() NOT update the map, so
>>> then this would be the only place where the bitmap gets updated. I'd
>>> have to rename it though, as it would no longer be assigning anything!
>>
>> Maybe pick_doorbell or find_free_doorbell? It would be a little bit
>> clearer and safer (early returns above could leave the bitmap set) with
>> a single bitmap update location so I think it would be worth it.
>>
>> Regards,
>> Tvrtko
>
> Roger wilco, but it looks simpler if I make it a followup [7/6]?
> Otherwise (if I mix it into this rework) git scrambles the diff
> around so its less easy to see how the code was reorganised.
>
> Update patch will follow soon ...

Yes fine, have it outside this series for simplicity. So for this patch:

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

[PATCH v2 7/6] drm/i915/guc: replace assign_doorbell() with select_doorbell_register()

[Dave Gordon]

This version doesn't update the doorbell bitmap, as that will
be done when the selected doorbell is associated with a client.

Also it's called a little earlier, just on the general principle
that potentially-failing operations should be done before those
that can't fail, to simplify error handling.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 62 +++++++++++++++---------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 120d2e8..83c236c 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -232,6 +232,32 @@ static void guc_disable_doorbell(struct intel_guc *guc,
 	/* XXX: wait for workqueue to drain */
 }
 
+static uint16_t
+select_doorbell_register(struct intel_guc *guc, uint32_t priority)
+{
+	/*
+	 * The bitmap tracks which doorbell registers are currently in use.
+	 * It is split into two halves; the first half is used for normal
+	 * priority contexts, the second half for high-priority ones.
+	 * Note that logically higher priorities are numerically less than
+	 * normal ones, so the test below means "is it high-priority?"
+	 */
+	const bool hi_pri = (priority <= GUC_CTX_PRIORITY_HIGH);
+	const uint16_t half = GUC_MAX_DOORBELLS / 2;
+	const uint16_t start = hi_pri ? half : 0;
+	const uint16_t end = start + half;
+	uint16_t id;
+
+	id = find_next_zero_bit(guc->doorbell_bitmap, end, start);
+	if (id == end)
+		id = GUC_INVALID_DOORBELL_ID;
+
+	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
+			hi_pri ? "high" : "normal", id);
+
+	return id;
+}
+
 /*
  * Select, assign and relase doorbell cachelines
  *
@@ -256,32 +282,6 @@ static uint32_t select_doorbell_cacheline(struct intel_guc *guc)
 	return offset;
 }
 
-static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
-{
-	/*
-	 * The bitmap is split into two halves; the first half is used for
-	 * normal priority contexts, the second half for high-priority ones.
-	 * Note that logically higher priorities are numerically less than
-	 * normal ones, so the test below means "is it high-priority?"
-	 */
-	const bool hi_pri = (priority <= GUC_CTX_PRIORITY_HIGH);
-	const uint16_t half = GUC_MAX_DOORBELLS / 2;
-	const uint16_t start = hi_pri ? half : 0;
-	const uint16_t end = start + half;
-	uint16_t id;
-
-	id = find_next_zero_bit(guc->doorbell_bitmap, end, start);
-	if (id == end)
-		id = GUC_INVALID_DOORBELL_ID;
-	else
-		__set_bit(id, guc->doorbell_bitmap);
-
-	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
-			hi_pri ? "high" : "normal", id);
-
-	return id;
-}
-
 /*
  * Initialise the process descriptor shared with the GuC firmware.
  */
@@ -785,6 +785,11 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 	client->wq_offset = GUC_DB_SIZE;
 	client->wq_size = GUC_WQ_SIZE;
 
+	db_id = select_doorbell_register(guc, client->priority);
+	if (db_id == GUC_INVALID_DOORBELL_ID)
+		/* XXX: evict a doorbell instead? */
+		goto err;
+
 	client->doorbell_offset = select_doorbell_cacheline(guc);
 
 	/*
@@ -797,11 +802,6 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 	else
 		client->proc_desc_offset = (GUC_DB_SIZE / 2);
 
-	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);
 	if (guc_init_doorbell(guc, client, db_id))
-- 
1.9.1

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

Re: [PATCH v2 4/6] drm/i915/guc: remove writes to GEN8_DRBREG registers

[Dave Gordon]

On 13/06/16 10:35, Tvrtko Ursulin wrote:
>
> On 10/06/16 17:50, Dave Gordon wrote:
>> These registers are not actually writable by the CPU; only the GuC can
>> actually program them. So let's not do writes that have no effect.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index e198599..45b33f8 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -197,14 +197,9 @@ static void guc_disable_doorbell(struct intel_guc
>> *guc,
>>
>>       doorbell->db_status = GUC_DOORBELL_DISABLED;
>>
>> -    I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
>> -
>>       value = I915_READ(drbreg);
>>       WARN_ON((value & GEN8_DRB_VALID) != 0);
>>
>> -    I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
>> -    I915_WRITE(drbreg, 0);
>> -
>>       /* XXX: wait for any interrupts */
>>       /* XXX: wait for workqueue to drain */
>>   }
>>
>
> I have to trust you on this one. :)
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Regards,
> Tvrtko

I found an example of what can occasionally happen during 
resume-from-disk with the old code:

[  407.811985] [drm:intel_guc_setup] GuC fw status: path 
i915/skl_guc_ver6_1.bin, fetch SUCCESS, load SUCCESS
[  407.811988] [drm:intel_guc_setup] GuC fw status: fetch SUCCESS, load 
PENDING
[  407.811989] ------------[ cut here ]------------
[  407.812006] WARNING: CPU: 2 PID: 1501 at 
/usr2/dsgordon/Source/drm-intel-nightly/drm-intel/drivers/gpu/drm/i915/i915_guc_submission.c:258 
guc_client_free+0x190/0x1a0 [i915]
[  407.812007] WARN_ON((value & (1<<0)) != 0)

In other words, we have just "written" this register with the VALID bit 
clear, but when we read it back it's NOT clear, showing that CPU writes 
really don't have any effect.

[  407.812017] Modules linked in: nls_iso8859_1 nls_cp437 vfat fat 
snd_hda_codec_hdmi snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core 
x86_pkg_temp_thermal coretemp snd_pcm input_leds led_class snd_seq 
snd_seq_device snd_timer snd soundcore serio_raw acpi_pad tpm_tis 
autofs4 hid_generic usbhid hid i915 e1000e i2c_algo_bit drm_kms_helper 
ptp syscopyarea sysfillrect sysimgblt fb_sys_fops psmouse pps_core drm video
[  407.812018] CPU: 2 PID: 1501 Comm: kworker/u16:38 Tainted: G     U 
        4.7.0-rc2-dsg-00656-g41795e2-dsg-work-080 #1501
[  407.812019] Hardware name: Intel Corporation Skylake Client 
platform/Skylake AIO DDR3L RVP10, BIOS SKLSE2R1.R00.X100.B01.1509220551 
09/22/2015
[  407.812022] Workqueue: events_unbound async_run_entry_fn
[  407.812023]  0000000000000000 ffff880165c1bae8 ffffffff81331caf 
ffff880165c1bb38
[  407.812024]  0000000000000000 ffff880165c1bb28 ffffffff8105aed1 
0000010200000006
[  407.812025]  ffff88016675f200 ffff8801667c0000 0000000000001000 
0000000000000000
[  407.812025] Call Trace:
[  407.812028]  [<ffffffff81331caf>] dump_stack+0x4d/0x6e
[  407.812030]  [<ffffffff8105aed1>] __warn+0xd1/0xf0
[  407.812031]  [<ffffffff8105af3f>] warn_slowpath_fmt+0x4f/0x60
[  407.812044]  [<ffffffffa01c6e20>] ? chv_write16+0x390/0x390 [i915]
[  407.812056]  [<ffffffffa01cad60>] guc_client_free+0x190/0x1a0 [i915]
[  407.812067]  [<ffffffffa01c6e20>] ? chv_write16+0x390/0x390 [i915]
[  407.812078]  [<ffffffffa01cb6eb>] 
i915_guc_submission_disable+0x4b/0x60 [i915]
[  407.812088]  [<ffffffffa01cb734>] i915_guc_submission_init+0x34/0x3a0 
[i915]
[  407.812098]  [<ffffffffa01c98fd>] intel_guc_setup+0x9d/0x820 [i915]
[  407.812108]  [<ffffffffa01c6e20>] ? chv_write16+0x390/0x390 [i915]
[  407.812118]  [<ffffffffa01b8abb>] ? 
intel_mocs_init_l3cc_table+0xbb/0x110 [i915]
[  407.812128]  [<ffffffffa01a761c>] i915_gem_init_hw+0x10c/0x2a0 [i915]
[  407.812130]  [<ffffffff81375fb0>] ? pci_pm_suspend_noirq+0x190/0x190
[  407.812136]  [<ffffffffa0168086>] i915_drm_resume+0x86/0x180 [i915]
[  407.812143]  [<ffffffffa01681a7>] i915_pm_resume+0x27/0x30 [i915]
[  407.812149]  [<ffffffffa01681be>] i915_pm_restore+0xe/0x10 [i915]
[  407.812150]  [<ffffffff81376029>] pci_pm_restore+0x79/0xb0
[  407.812152]  [<ffffffff81450e7e>] dpm_run_callback+0x4e/0x130
[  407.812153]  [<ffffffff81451403>] device_resume+0xd3/0x1f0
[  407.812154]  [<ffffffff8145153d>] async_resume+0x1d/0x50
[  407.812154]  [<ffffffff81079bf8>] async_run_entry_fn+0x48/0x150
[  407.812156]  [<ffffffff81071d28>] process_one_work+0x148/0x3f0
[  407.812157]  [<ffffffff810720fb>] worker_thread+0x12b/0x490
[  407.812159]  [<ffffffff81071fd0>] ? process_one_work+0x3f0/0x3f0
[  407.812159]  [<ffffffff81077159>] kthread+0xc9/0xe0
[  407.812162]  [<ffffffff816c46cf>] ret_from_fork+0x1f/0x40
[  407.812162]  [<ffffffff81077090>] ? kthread_park+0x60/0x60
[  407.812163] ---[ end trace 7dc15bdf4dd23983 ]---
[  407.815449] [drm:guc_ucode_xfer_dma] DMA status 0x10, GuC status 
0x8002f0ec
[  407.815449] [drm:guc_ucode_xfer_dma] returning 0
[  407.815450] [drm:intel_guc_setup] GuC fw status: fetch SUCCESS, load 
SUCCESS

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