[00/11 v2] Various improvements around the GuC topic

[00/11 v2] Various improvements around the GuC topic

[Oscar Mateo]

They have been discussed in various threads, but I am putting them together
as a series for clarity. They now include Joonas' "Sanitize GuC client
initialization" with a small tweak, plus an extra doorbell refactoring patch
at the end, so they are self-contained and... ready to merge?.

Oscar Mateo (11):
  drm/i915/guc: Sanitize GuC client initialization
  drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
  drm/i915/guc: Add onion teardown to the GuC setup
  drm/i915/guc: s/ads_vma/addon
  drm/i915/guc: Break out the GuC log extras into their own "runtime"
    struct
  drm/i915/guc: Make intel_guc_send a function pointer
  drm/i915/guc: Improve the GuC documentation & comments about proxy
    submissions
  drm/i915/guc: Wait for doorbell to be inactive before deallocating
  drm/i915/guc: A little bit more of doorbell sanitization
  drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC
    stage descriptor"
  drm/i915/guc: Enable GuC loading & submission

 drivers/gpu/drm/i915/i915_debugfs.c        |   8 +-
 drivers/gpu/drm/i915/i915_drv.c            |   6 +-
 drivers/gpu/drm/i915/i915_guc_submission.c | 733 +++++++++++++++++------------
 drivers/gpu/drm/i915/i915_irq.c            |   4 +-
 drivers/gpu/drm/i915/i915_params.c         |   8 +-
 drivers/gpu/drm/i915/intel_guc_fwif.h      |  46 +-
 drivers/gpu/drm/i915/intel_guc_loader.c    |  54 ++-
 drivers/gpu/drm/i915/intel_guc_log.c       | 384 ++++++++-------
 drivers/gpu/drm/i915/intel_uc.c            |  17 +-
 drivers/gpu/drm/i915/intel_uc.h            |  48 +-
 10 files changed, 727 insertions(+), 581 deletions(-)

-- 
1.9.1

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

[01/11 v3] drm/i915/guc: Sanitize GuC client initialization

[Oscar Mateo]

Started adding proper teardown to guc_client_alloc, ended up removing
quite a few dead ends where errors communicating with the GuC were
silently ignored. There also seemed to be quite a few erronous
teardown actions performed in case of an error (ordering wrong).

v2:
  - Increase function symmetry/proximity (Michal/Daniele)
  - Fix __reserve_doorbell accounting for high priority (Daniele)
  - Call __update_doorbell_desc! (Daniele)
  - Isolate __guc_{,de}allocate_doorbell (Michal/Daniele)

v3:
  - "Select" a cacheline is a more accurate verb than "reserve" (Daniele).
  - We cannot update & create the doorbell without reserving it first, so
    move the whole doorbell creation for execbuf_client to the submission
    enable (Oscar).i
  - Add a fixme for ignoring possible doorbell destroy errors.

Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |   4 +-
 drivers/gpu/drm/i915/i915_guc_submission.c | 391 ++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_guc_fwif.h      |   4 +-
 drivers/gpu/drm/i915/intel_uc.h            |  11 +-
 4 files changed, 229 insertions(+), 181 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 56674df..c395ccf 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2469,7 +2469,7 @@ static void i915_guc_client_info(struct seq_file *m,
 
 	seq_printf(m, "\tPriority %d, GuC ctx index: %u, PD offset 0x%x\n",
 		client->priority, client->ctx_index, client->proc_desc_offset);
-	seq_printf(m, "\tDoorbell id %d, offset: 0x%x, cookie 0x%x\n",
+	seq_printf(m, "\tDoorbell id %d, offset: 0x%lx, cookie 0x%x\n",
 		client->doorbell_id, client->doorbell_offset, client->doorbell_cookie);
 	seq_printf(m, "\tWQ size %d, offset: 0x%x, tail %d\n",
 		client->wq_size, client->wq_offset, client->wq_tail);
@@ -2504,7 +2504,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
 	}
 
 	seq_printf(m, "Doorbell map:\n");
-	seq_printf(m, "\t%*pb\n", GUC_MAX_DOORBELLS, guc->doorbell_bitmap);
+	seq_printf(m, "\t%*pb\n", GUC_NUM_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);
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 41f2dd8..ceeb1fa 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -62,27 +62,73 @@
  *
  */
 
+static inline bool is_high_priority(struct i915_guc_client* client)
+{
+	return client->priority <= GUC_CTX_PRIORITY_HIGH;
+}
+
+static int __reserve_doorbell(struct i915_guc_client *client)
+{
+	unsigned long offset;
+	unsigned long end;
+	u16 id;
+
+	GEM_BUG_ON(client->doorbell_id != GUC_DOORBELL_INVALID);
+
+	/*
+	 * 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?"
+	 */
+
+	offset = 0;
+	end = GUC_NUM_DOORBELLS/2;
+	if (is_high_priority(client)) {
+		offset = end;
+		end += offset;
+        }
+
+	id = find_next_zero_bit(client->guc->doorbell_bitmap, offset, end);
+	if (id == end)
+		return -ENOSPC;
+
+	__set_bit(id, client->guc->doorbell_bitmap);
+	client->doorbell_id = id;
+	DRM_DEBUG_DRIVER("client %u (high prio=%s) reserved doorbell %d: \n",
+			 client->ctx_index, yesno(is_high_priority(client)),
+			 id);
+	return 0;
+}
+
+static void __unreserve_doorbell(struct i915_guc_client *client)
+{
+	GEM_BUG_ON(client->doorbell_id == GUC_DOORBELL_INVALID);
+
+	__clear_bit(client->doorbell_id, client->guc->doorbell_bitmap);
+	client->doorbell_id = GUC_DOORBELL_INVALID;
+}
+
 /*
  * Tell the GuC to allocate or deallocate a specific doorbell
  */
 
-static int guc_allocate_doorbell(struct intel_guc *guc,
-				 struct i915_guc_client *client)
+static int __guc_allocate_doorbell(struct intel_guc *guc, u32 ctx_index)
 {
 	u32 action[] = {
 		INTEL_GUC_ACTION_ALLOCATE_DOORBELL,
-		client->ctx_index
+		ctx_index
 	};
 
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
-static int guc_release_doorbell(struct intel_guc *guc,
-				struct i915_guc_client *client)
+static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
 {
 	u32 action[] = {
 		INTEL_GUC_ACTION_DEALLOCATE_DOORBELL,
-		client->ctx_index
+		ctx_index
 	};
 
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
@@ -95,104 +141,100 @@ static int guc_release_doorbell(struct intel_guc *guc,
  * client object which contains the page being used for the doorbell
  */
 
-static int guc_update_doorbell_id(struct intel_guc *guc,
-				  struct i915_guc_client *client,
-				  u16 new_id)
+static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
 {
-	struct sg_table *sg = guc->ctx_pool_vma->pages;
-	void *doorbell_bitmap = guc->doorbell_bitmap;
-	struct guc_doorbell_info *doorbell;
+	struct sg_table *sg = client->guc->ctx_pool_vma->pages;
 	struct guc_context_desc desc;
 	size_t len;
 
-	doorbell = client->vaddr + client->doorbell_offset;
-
-	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)guc_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);
+				 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);
+				   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;
+	return 0;
+}
 
-	/* Activate the new doorbell */
-	__set_bit(new_id, doorbell_bitmap);
+static struct guc_doorbell_info *__get_doorbell(struct i915_guc_client *client)
+{
+	return client->vaddr + client->doorbell_offset;
+}
+
+static bool has_doorbell(struct i915_guc_client *client)
+{
+	if (client->doorbell_id == GUC_DOORBELL_INVALID)
+		return false;
+
+	return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
+}
+
+static int __create_doorbell(struct i915_guc_client *client)
+{
+	struct guc_doorbell_info *doorbell;
+	int err;
+
+	doorbell = __get_doorbell(client);
 	doorbell->db_status = GUC_DOORBELL_ENABLED;
 	doorbell->cookie = client->doorbell_cookie;
-	return guc_allocate_doorbell(guc, client);
+
+	err = __guc_allocate_doorbell(client->guc, client->ctx_index);
+	if (err) {
+		doorbell->db_status = GUC_DOORBELL_DISABLED;
+		doorbell->cookie = 0;
+	}
+	return err;
 }
 
-static void guc_disable_doorbell(struct intel_guc *guc,
-				 struct i915_guc_client *client)
+static int __destroy_doorbell(struct i915_guc_client *client)
 {
-	(void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
+	struct guc_doorbell_info *doorbell;
 
-	/* XXX: wait for any interrupts */
-	/* XXX: wait for workqueue to drain */
+	doorbell = __get_doorbell(client);
+	doorbell->db_status = GUC_DOORBELL_DISABLED;
+	doorbell->cookie = 0;
+
+	return __guc_deallocate_doorbell(client->guc, client->ctx_index);
 }
 
-static uint16_t
-select_doorbell_register(struct intel_guc *guc, uint32_t priority)
+static int destroy_doorbell(struct i915_guc_client *client)
 {
-	/*
-	 * 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;
+	int err;
 
-	id = find_next_zero_bit(guc->doorbell_bitmap, end, start);
-	if (id == end)
-		id = GUC_INVALID_DOORBELL_ID;
+	GEM_BUG_ON(!has_doorbell(client));
 
-	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
-			hi_pri ? "high" : "normal", id);
+	/* XXX: wait for any interrupts */
+	/* XXX: wait for workqueue to drain */
 
-	return id;
-}
+	err = __destroy_doorbell(client);
+	if (err)
+		return err;
 
-/*
- * Select, assign and relase doorbell cachelines
- *
- * These functions track which doorbell cachelines are in use.
- * The data they manipulate is protected by the intel_guc_send lock.
- */
+	__update_doorbell_desc(client, GUC_DOORBELL_INVALID);
+
+	__unreserve_doorbell(client);
 
-static uint32_t select_doorbell_cacheline(struct intel_guc *guc)
+	return 0;
+}
+
+static unsigned long __select_cacheline(struct intel_guc* guc)
 {
-	const uint32_t cacheline_size = cache_line_size();
-	uint32_t offset;
+	unsigned long offset;
 
 	/* Doorbell uses a single cache line within a page */
 	offset = offset_in_page(guc->db_cacheline);
 
 	/* Moving to next cache line to reduce contention */
-	guc->db_cacheline += cacheline_size;
-
-	DRM_DEBUG_DRIVER("selected doorbell cacheline 0x%x, next 0x%x, linesize %u\n",
-			offset, guc->db_cacheline, cacheline_size);
+	guc->db_cacheline += cache_line_size();
 
+	DRM_DEBUG_DRIVER("reserved cacheline 0x%lx, next 0x%x, linesize %u\n",
+			offset, guc->db_cacheline, cache_line_size());
 	return offset;
 }
 
@@ -594,93 +636,96 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
 	return vma;
 }
 
-static void
-guc_client_free(struct drm_i915_private *dev_priv,
-		struct i915_guc_client *client)
+static void guc_client_free(struct i915_guc_client *client)
 {
-	struct intel_guc *guc = &dev_priv->guc;
-
-	if (!client)
-		return;
-
 	/*
 	 * XXX: wait for any outstanding submissions before freeing memory.
 	 * Be sure to drop any locks
 	 */
-
-	if (client->vaddr) {
-		/*
-		 * If we got as far as setting up a doorbell, make sure we
-		 * shut it down before unmapping & deallocating the memory.
-		 */
-		guc_disable_doorbell(guc, client);
-
-		i915_gem_object_unpin_map(client->vma->obj);
-	}
-
+	guc_ctx_desc_fini(client->guc, client);
+	i915_gem_object_unpin_map(client->vma->obj);
 	i915_vma_unpin_and_release(&client->vma);
-
-	if (client->ctx_index != GUC_INVALID_CTX_ID) {
-		guc_ctx_desc_fini(guc, client);
-		ida_simple_remove(&guc->ctx_ids, client->ctx_index);
-	}
-
+	ida_simple_remove(&client->guc->ctx_ids, client->ctx_index);
 	kfree(client);
 }
 
 /* Check that a doorbell register is in the expected state */
-static bool guc_doorbell_check(struct intel_guc *guc, uint16_t db_id)
+static bool doorbell_ok(struct intel_guc *guc, u16 db_id)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	i915_reg_t drbreg = GEN8_DRBREGL(db_id);
-	uint32_t value = I915_READ(drbreg);
-	bool enabled = (value & GUC_DOORBELL_ENABLED) != 0;
-	bool expected = test_bit(db_id, guc->doorbell_bitmap);
+	u32 drbregl;
+	bool valid;
 
-	if (enabled == expected)
+	GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID);
+
+	drbregl = I915_READ(GEN8_DRBREGL(db_id));
+	valid = drbregl & GEN8_DRB_VALID;
+
+	if (test_bit(db_id, guc->doorbell_bitmap) == valid)
 		return true;
 
-	DRM_DEBUG_DRIVER("Doorbell %d (reg 0x%x) 0x%x, should be %s\n",
-			 db_id, drbreg.reg, value,
-			 expected ? "active" : "inactive");
+	DRM_DEBUG_DRIVER("Doorbell %d has unexpected state (0x%x): valid=%s\n",
+			 db_id, drbregl, yesno(valid));
 
 	return false;
 }
 
+static int __reset_doorbell(struct i915_guc_client* client, u16 db_id)
+{
+	int err;
+
+	err = __update_doorbell_desc(client, db_id);
+	if (!err)
+		err = __create_doorbell(client);
+	if (!err)
+		err = __destroy_doorbell(client);
+
+	return err;
+}
+
 /*
  * Borrow the first client to set up & tear down each unused doorbell
  * in turn, to ensure that all doorbell h/w is (re)initialised.
  */
-static void guc_init_doorbell_hw(struct intel_guc *guc)
+static int guc_init_doorbell_hw(struct intel_guc *guc)
 {
 	struct i915_guc_client *client = guc->execbuf_client;
-	uint16_t db_id;
-	int i, err;
+	int err;
+	int i;
 
-	guc_disable_doorbell(guc, client);
+	if (has_doorbell(client))
+		destroy_doorbell(client);
 
-	for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
-		/* Skip if doorbell is OK */
-		if (guc_doorbell_check(guc, i))
+	for (i = 0; i < GUC_NUM_DOORBELLS; ++i) {
+		if (doorbell_ok(guc, i))
 			continue;
 
-		err = guc_update_doorbell_id(guc, client, i);
-		if (err)
-			DRM_DEBUG_DRIVER("Doorbell %d update failed, err %d\n",
-					i, err);
+		err = __reset_doorbell(client, i);
+		WARN(err, "Doorbell %d reset failed, err %d\n", i, err);
 	}
 
-	db_id = select_doorbell_register(guc, client->priority);
-	WARN_ON(db_id == GUC_INVALID_DOORBELL_ID);
+	/* Read back & verify all doorbell registers */
+	for (i = 0; i < GUC_NUM_DOORBELLS; ++i)
+		WARN_ON(!doorbell_ok(guc, i));
 
-	err = guc_update_doorbell_id(guc, client, db_id);
+	err = __reserve_doorbell(client);
 	if (err)
-		DRM_WARN("Failed to restore doorbell to %d, err %d\n",
-			 db_id, err);
+		return err;
 
-	/* Read back & verify all doorbell registers */
-	for (i = 0; i < GUC_MAX_DOORBELLS; ++i)
-		(void)guc_doorbell_check(guc, i);
+	err = __update_doorbell_desc(client, client->doorbell_id);
+	if (err)
+		goto err_reserve;
+
+	err = __create_doorbell(client);
+	if (err)
+		goto err_update;
+
+	return 0;
+err_reserve:
+	__unreserve_doorbell(client);
+err_update:
+	__update_doorbell_desc(client, GUC_DOORBELL_INVALID);
+	return err;
 }
 
 /**
@@ -706,49 +751,46 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
 	struct intel_guc *guc = &dev_priv->guc;
 	struct i915_vma *vma;
 	void *vaddr;
-	uint16_t db_id;
+	int ret;
 
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
-	client->owner = ctx;
 	client->guc = guc;
+	client->owner = ctx;
 	client->engines = engines;
 	client->priority = priority;
-	client->doorbell_id = GUC_INVALID_DOORBELL_ID;
+	client->doorbell_id = GUC_DOORBELL_INVALID;
+	client->wq_offset = GUC_DB_SIZE;
+	client->wq_size = GUC_WQ_SIZE;
+	spin_lock_init(&client->wq_lock);
 
-	client->ctx_index = (uint32_t)ida_simple_get(&guc->ctx_ids, 0,
-			GUC_MAX_GPU_CONTEXTS, GFP_KERNEL);
-	if (client->ctx_index >= GUC_MAX_GPU_CONTEXTS) {
-		client->ctx_index = GUC_INVALID_CTX_ID;
-		goto err;
-	}
+	ret = ida_simple_get(&guc->ctx_ids, 0, GUC_MAX_GPU_CONTEXTS,
+				GFP_KERNEL);
+	if (ret < 0)
+		goto err_client;
+
+	client->ctx_index = ret;
 
 	/* The first page is doorbell/proc_desc. Two followed pages are wq. */
 	vma = intel_guc_allocate_vma(guc, GUC_DB_SIZE + GUC_WQ_SIZE);
-	if (IS_ERR(vma))
-		goto err;
+	if (IS_ERR(vma)) {
+		ret = PTR_ERR(vma);
+		goto err_id;
+	}
 
 	/* We'll keep just the first (doorbell/proc) page permanently kmap'd. */
 	client->vma = vma;
 
 	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
-	if (IS_ERR(vaddr))
-		goto err;
-
+	if (IS_ERR(vaddr)) {
+		ret = PTR_ERR(vaddr);
+		goto err_vma;
+	}
 	client->vaddr = vaddr;
 
-	spin_lock_init(&client->wq_lock);
-	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);
+	client->doorbell_offset = __select_cacheline(guc);
 
 	/*
 	 * Since the doorbell only requires a single cacheline, we can save
@@ -763,27 +805,28 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
 	guc_proc_desc_init(guc, client);
 	guc_ctx_desc_init(guc, client);
 
-	/* For runtime client allocation we need to enable the doorbell. Not
-	 * required yet for the static execbuf_client as this special kernel
-	 * client is enabled from i915_guc_submission_enable().
-	 *
-	 * guc_update_doorbell_id(guc, client, db_id);
-	 */
+	/* FIXME: Runtime client allocation (which currently we don't do) will
+	 * require that the doorbell gets created now. The static execbuf_client
+	 * is now getting its doorbell later (on submission enable) but maybe we
+	 * also want to reorder things in the future so that we don't have to
+	 * special case the doorbell creation */
 
 	DRM_DEBUG_DRIVER("new priority %u client %p for engine(s) 0x%x: ctx_index %u\n",
-		priority, client, client->engines, client->ctx_index);
-	DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n",
-		client->doorbell_id, client->doorbell_offset);
+			 priority, client, client->engines, client->ctx_index);
+	DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%lx\n",
+			 client->doorbell_id, client->doorbell_offset);
 
 	return client;
+err_vma:
+	i915_vma_unpin_and_release(&client->vma);
+err_id:
+	ida_simple_remove(&guc->ctx_ids, client->ctx_index);
+err_client:
+	kfree(client);
 
-err:
-	guc_client_free(dev_priv, client);
-	return NULL;
+	return ERR_PTR(ret);
 }
 
-
-
 static void guc_policies_init(struct guc_policies *policies)
 {
 	struct guc_policy *policy;
@@ -891,7 +934,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 		return 0;
 
 	/* Wipe bitmap & delete client in case of reinitialisation */
-	bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS);
+	bitmap_clear(guc->doorbell_bitmap, 0, GUC_NUM_DOORBELLS);
 	i915_guc_submission_disable(dev_priv);
 
 	if (!i915.enable_guc_submission)
@@ -913,7 +956,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 					       INTEL_INFO(dev_priv)->ring_mask,
 					       GUC_CTX_PRIORITY_KMD_NORMAL,
 					       dev_priv->kernel_context);
-	if (!guc->execbuf_client) {
+	if (IS_ERR(guc->execbuf_client)) {
 		DRM_ERROR("Failed to create GuC client for execbuf!\n");
 		goto err;
 	}
@@ -962,14 +1005,19 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 	struct i915_guc_client *client = guc->execbuf_client;
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
+	int err;
 
 	if (!client)
 		return -ENODEV;
 
-	intel_guc_sample_forcewake(guc);
+	err = intel_guc_sample_forcewake(guc);
+	if (err)
+		return err;
 
 	guc_reset_wq(client);
-	guc_init_doorbell_hw(guc);
+	err = guc_init_doorbell_hw(guc);
+	if (err)
+		return err;
 
 	/* Take over from manual control of ELSP (execlists) */
 	for_each_engine(engine, dev_priv, id) {
@@ -1002,6 +1050,11 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
 	if (!guc->execbuf_client)
 		return;
 
+	/* FIXME: in many cases, by the time we get here the GuC has been
+	 * reset, so we cannot destroy the doorbell properly. Ignore the
+	 * error message for now */
+	destroy_doorbell(guc->execbuf_client);
+
 	/* Revert back to manual ELSP submission */
 	intel_execlists_enable_submission(dev_priv);
 }
@@ -1012,10 +1065,8 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	struct i915_guc_client *client;
 
 	client = fetch_and_zero(&guc->execbuf_client);
-	if (!client)
-		return;
-
-	guc_client_free(dev_priv, client);
+	if (client && !IS_ERR(client))
+		guc_client_free(client);
 
 	i915_vma_unpin_and_release(&guc->ads_vma);
 	i915_vma_unpin_and_release(&guc->log.vma);
@@ -1077,5 +1128,3 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 
 	return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
-
-
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 25691f0..3ae8cef 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -241,8 +241,8 @@ struct guc_doorbell_info {
 	u64 value_qw;
 } __packed;
 
-#define GUC_MAX_DOORBELLS		256
-#define GUC_INVALID_DOORBELL_ID		(GUC_MAX_DOORBELLS)
+#define GUC_NUM_DOORBELLS	256
+#define GUC_DOORBELL_INVALID	(GUC_NUM_DOORBELLS)
 
 #define GUC_DB_SIZE			(PAGE_SIZE)
 #define GUC_WQ_SIZE			(PAGE_SIZE * 2)
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index d74f4d3..604e71e 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -72,13 +72,12 @@ struct i915_guc_client {
 
 	uint32_t engines;		/* bitmap of (host) engine ids	*/
 	uint32_t priority;
-	uint32_t ctx_index;
+	u32 ctx_index;
 	uint32_t proc_desc_offset;
 
-	uint32_t doorbell_offset;
-	uint32_t doorbell_cookie;
-	uint16_t doorbell_id;
-	uint16_t padding[3];		/* Maintain alignment		*/
+	u16 doorbell_id;
+	unsigned long doorbell_offset;
+	u32 doorbell_cookie;
 
 	spinlock_t wq_lock;
 	uint32_t wq_offset;
@@ -159,7 +158,7 @@ struct intel_guc {
 
 	struct i915_guc_client *execbuf_client;
 
-	DECLARE_BITMAP(doorbell_bitmap, GUC_MAX_DOORBELLS);
+	DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
 	uint32_t db_cacheline;		/* Cyclic counter mod pagesize	*/
 
 	/* Action status & statistics */
-- 
1.9.1

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

[02/11 v3] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access

[Oscar Mateo]

The GuC descriptor is big in size. If we use a local definition of
guc_desc we have a chance to overflow stack, so avoid it.

Also, Chris abhors scatterlists :)

v2: Rebased, helper function to retrieve the context descriptor,
s/ctx_pool_vma/ctx_pool/

v3: Zero out guc_context_desc before initialization

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 95 +++++++++++++++---------------
 drivers/gpu/drm/i915/intel_guc_loader.c    |  2 +-
 drivers/gpu/drm/i915/intel_uc.h            |  3 +-
 3 files changed, 49 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index ceeb1fa..843c73a 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -134,6 +134,12 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
+static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)
+{
+	return client->guc->ctx_pool_vaddr +
+		sizeof(struct guc_context_desc) * client->ctx_index;
+}
+
 /*
  * Initialise, update, or clear doorbell data shared with the GuC
  *
@@ -143,21 +149,11 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
 
 static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
 {
-	struct sg_table *sg = client->guc->ctx_pool_vma->pages;
-	struct guc_context_desc desc;
-	size_t len;
+	struct guc_context_desc *desc;
 
 	/* 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;
+	desc = __get_context_desc(client);
+	desc->db_id = new_id;
 
 	return 0;
 }
@@ -272,29 +268,28 @@ static void guc_proc_desc_init(struct intel_guc *guc,
  * data structures relating to this client (doorbell, process descriptor,
  * write queue, etc).
  */
-
 static void guc_ctx_desc_init(struct intel_guc *guc,
 			      struct i915_guc_client *client)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct intel_engine_cs *engine;
 	struct i915_gem_context *ctx = client->owner;
-	struct guc_context_desc desc;
-	struct sg_table *sg;
+	struct guc_context_desc *desc;
 	unsigned int tmp;
 	u32 gfx_addr;
 
-	memset(&desc, 0, sizeof(desc));
+	desc = __get_context_desc(client);
+	memset(desc, 0, sizeof(*desc));
 
-	desc.attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
-	desc.context_id = client->ctx_index;
-	desc.priority = client->priority;
-	desc.db_id = client->doorbell_id;
+	desc->attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
+	desc->context_id = client->ctx_index;
+	desc->priority = client->priority;
+	desc->db_id = client->doorbell_id;
 
 	for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
 		struct intel_context *ce = &ctx->engine[engine->id];
 		uint32_t guc_engine_id = engine->guc_id;
-		struct guc_execlist_context *lrc = &desc.lrc[guc_engine_id];
+		struct guc_execlist_context *lrc = &desc->lrc[guc_engine_id];
 
 		/* TODO: We have a design issue to be solved here. Only when we
 		 * receive the first batch, we know which engine is used by the
@@ -319,50 +314,41 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
 		lrc->ring_next_free_location = lrc->ring_begin;
 		lrc->ring_current_tail_pointer_value = 0;
 
-		desc.engines_used |= (1 << guc_engine_id);
+		desc->engines_used |= (1 << guc_engine_id);
 	}
 
 	DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
-			client->engines, desc.engines_used);
-	WARN_ON(desc.engines_used == 0);
+			client->engines, desc->engines_used);
+	WARN_ON(desc->engines_used == 0);
 
 	/*
 	 * The doorbell, process descriptor, and workqueue are all parts
 	 * of the client object, which the GuC will reference via the GGTT
 	 */
 	gfx_addr = guc_ggtt_offset(client->vma);
-	desc.db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
+	desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
 				client->doorbell_offset;
-	desc.db_trigger_cpu =
-		(uintptr_t)client->vaddr + client->doorbell_offset;
-	desc.db_trigger_uk = gfx_addr + client->doorbell_offset;
-	desc.process_desc = gfx_addr + client->proc_desc_offset;
-	desc.wq_addr = gfx_addr + client->wq_offset;
-	desc.wq_size = client->wq_size;
+	desc->db_trigger_cpu = (uintptr_t)client->vaddr +
+				client->doorbell_offset;
+	desc->db_trigger_uk = gfx_addr + client->doorbell_offset;
+	desc->process_desc = gfx_addr + client->proc_desc_offset;
+	desc->wq_addr = gfx_addr + client->wq_offset;
+	desc->wq_size = client->wq_size;
 
 	/*
 	 * XXX: Take LRCs from an existing context if this is not an
 	 * IsKMDCreatedContext client
 	 */
-	desc.desc_private = (uintptr_t)client;
-
-	/* Pool context is pinned already */
-	sg = guc->ctx_pool_vma->pages;
-	sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
-			     sizeof(desc) * client->ctx_index);
+	desc->desc_private = (uintptr_t)client;
 }
 
 static void guc_ctx_desc_fini(struct intel_guc *guc,
 			      struct i915_guc_client *client)
 {
-	struct guc_context_desc desc;
-	struct sg_table *sg;
+	struct guc_context_desc *desc;
 
-	memset(&desc, 0, sizeof(desc));
-
-	sg = guc->ctx_pool_vma->pages;
-	sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
-			     sizeof(desc) * client->ctx_index);
+	desc = __get_context_desc(client);
+	memset(desc, 0, sizeof(*desc));
 }
 
 /**
@@ -929,6 +915,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	const size_t gemsize = round_up(poolsize, PAGE_SIZE);
 	struct intel_guc *guc = &dev_priv->guc;
 	struct i915_vma *vma;
+	void *vaddr;
 
 	if (!HAS_GUC_SCHED(dev_priv))
 		return 0;
@@ -940,14 +927,21 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	if (!i915.enable_guc_submission)
 		return 0; /* not enabled  */
 
-	if (guc->ctx_pool_vma)
+	if (guc->ctx_pool)
 		return 0; /* already allocated */
 
 	vma = intel_guc_allocate_vma(guc, gemsize);
 	if (IS_ERR(vma))
 		return PTR_ERR(vma);
 
-	guc->ctx_pool_vma = vma;
+	guc->ctx_pool = vma;
+
+	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
+	if (IS_ERR(vaddr))
+		goto err;
+
+	guc->ctx_pool_vaddr = vaddr;
+
 	ida_init(&guc->ctx_ids);
 	intel_guc_log_create(guc);
 	guc_addon_create(guc);
@@ -1071,9 +1065,12 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	i915_vma_unpin_and_release(&guc->ads_vma);
 	i915_vma_unpin_and_release(&guc->log.vma);
 
-	if (guc->ctx_pool_vma)
+	if (guc->ctx_pool_vaddr) {
 		ida_destroy(&guc->ctx_ids);
-	i915_vma_unpin_and_release(&guc->ctx_pool_vma);
+		i915_gem_object_unpin_map(guc->ctx_pool->obj);
+	}
+
+	i915_vma_unpin_and_release(&guc->ctx_pool);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 2e24712..b1ebfce 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -175,7 +175,7 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
 
 	/* If GuC submission is enabled, set up additional parameters here */
 	if (i915.enable_guc_submission) {
-		u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool_vma);
+		u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool);
 		u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
 
 		pgs >>= PAGE_SHIFT;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 604e71e..0d12c8e 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -153,7 +153,8 @@ struct intel_guc {
 	bool interrupts_enabled;
 
 	struct i915_vma *ads_vma;
-	struct i915_vma *ctx_pool_vma;
+	struct i915_vma *ctx_pool;
+	void *ctx_pool_vaddr;
 	struct ida ctx_ids;
 
 	struct i915_guc_client *execbuf_client;
-- 
1.9.1

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

[03/11 v3] drm/i915/guc: Add onion teardown to the GuC setup

[Oscar Mateo]

Starting with intel_guc_loader, down to intel_guc_submission
and finally to intel_guc_log.

v2:
  - Null execbuf client outside guc_client_free (Daniele)
  - Assert if things try to get allocated twice (Daniele/Joonas)
  - Null guc->log.buf_addr when destroyed (Daniele)
  - Newline between returning success and error labels (Joonas)
  - Remove some unnecessary comments (Joonas)
  - Keep guc_log_create_extras naming convention (Joonas)
  - Helper function guc_log_has_extras (Joonas)
  - No need for separate relay_channel create/destroy. It's just another extra.
  - No need to nullify guc->log.flush_wq when destroyed (Joonas)
  - Hoist the check for has_extras out of guc_log_create_extras (Joonas)
  - Try to do i915_guc_log_register/unregister calls (kind of) symmetric (Daniele)
  - Make sure initel_guc_fini is not called before init is ever called (Daniele)

v3:
  - Remove unnecessary parenthesis (Joonas)
  - Check for logs enabled on debugfs registration
  - Rebase on top of Tvrtko's "Fix request re-submission after reset"

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            |   7 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |  94 ++++----
 drivers/gpu/drm/i915/intel_guc_loader.c    |  32 +--
 drivers/gpu/drm/i915/intel_guc_log.c       | 362 ++++++++++++++---------------
 drivers/gpu/drm/i915/intel_uc.h            |   8 +-
 5 files changed, 261 insertions(+), 242 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b1e9027..3329507 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -549,6 +549,8 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
 static void i915_gem_fini(struct drm_i915_private *dev_priv)
 {
 	mutex_lock(&dev_priv->drm.struct_mutex);
+	if (i915.enable_guc_submission)
+		intel_guc_cleanup(dev_priv);
 	i915_gem_cleanup_engines(dev_priv);
 	i915_gem_context_fini(dev_priv);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
@@ -612,7 +614,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	ret = i915_gem_init(dev_priv);
 	if (ret)
-		goto cleanup_irq;
+		goto cleanup_uc;
 
 	intel_modeset_gem_init(dev);
 
@@ -634,9 +636,10 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (i915_gem_suspend(dev_priv))
 		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
 	i915_gem_fini(dev_priv);
-cleanup_irq:
+cleanup_uc:
 	intel_guc_fini(dev_priv);
 	intel_huc_fini(dev_priv);
+cleanup_irq:
 	drm_irq_uninstall(dev);
 	intel_teardown_gmbus(dev_priv);
 cleanup_csr:
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 843c73a..f33c659 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -809,7 +809,6 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 	ida_simple_remove(&guc->ctx_ids, client->ctx_index);
 err_client:
 	kfree(client);
-
 	return ERR_PTR(ret);
 }
 
@@ -835,7 +834,7 @@ static void guc_policies_init(struct guc_policies *policies)
 	policies->is_valid = 1;
 }
 
-static void guc_addon_create(struct intel_guc *guc)
+static int guc_addon_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct i915_vma *vma;
@@ -847,19 +846,18 @@ static void guc_addon_create(struct intel_guc *guc)
 	struct page *page;
 	u32 size;
 
+	GEM_BUG_ON(guc->ads_vma);
+
 	/* The ads obj includes the struct itself and buffers passed to GuC */
 	size = sizeof(struct guc_ads) + sizeof(struct guc_policies) +
 			sizeof(struct guc_mmio_reg_state) +
 			GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
 
-	vma = guc->ads_vma;
-	if (!vma) {
-		vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
-		if (IS_ERR(vma))
-			return;
+	vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
 
-		guc->ads_vma = vma;
-	}
+	guc->ads_vma = vma;
 
 	page = i915_vma_first_page(vma);
 	ads = kmap(page);
@@ -902,6 +900,13 @@ static void guc_addon_create(struct intel_guc *guc)
 			sizeof(struct guc_mmio_reg_state);
 
 	kunmap(page);
+
+	return 0;
+}
+
+static void guc_addon_destroy(struct intel_guc *guc)
+{
+	i915_vma_unpin_and_release(&guc->ads_vma);
 }
 
 /*
@@ -916,6 +921,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 	struct i915_vma *vma;
 	void *vaddr;
+	int ret;
 
 	if (!HAS_GUC_SCHED(dev_priv))
 		return 0;
@@ -925,10 +931,10 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	i915_guc_submission_disable(dev_priv);
 
 	if (!i915.enable_guc_submission)
-		return 0; /* not enabled  */
+		return 0;
 
 	if (guc->ctx_pool)
-		return 0; /* already allocated */
+		return 0;
 
 	vma = intel_guc_allocate_vma(guc, gemsize);
 	if (IS_ERR(vma))
@@ -936,15 +942,23 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 
 	guc->ctx_pool = vma;
 
-	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
-	if (IS_ERR(vaddr))
-		goto err;
+	vaddr = i915_gem_object_pin_map(guc->ctx_pool->obj, I915_MAP_WB);
+	if (IS_ERR(vaddr)) {
+		ret = PTR_ERR(vaddr);
+		goto err_vma;
+	}
 
 	guc->ctx_pool_vaddr = vaddr;
 
+	ret = intel_guc_log_create(guc);
+	if (ret < 0)
+		goto err_vaddr;
+
+	ret = guc_addon_create(guc);
+	if (ret < 0)
+		goto err_log;
+
 	ida_init(&guc->ctx_ids);
-	intel_guc_log_create(guc);
-	guc_addon_create(guc);
 
 	guc->execbuf_client = guc_client_alloc(dev_priv,
 					       INTEL_INFO(dev_priv)->ring_mask,
@@ -952,14 +966,34 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 					       dev_priv->kernel_context);
 	if (IS_ERR(guc->execbuf_client)) {
 		DRM_ERROR("Failed to create GuC client for execbuf!\n");
-		goto err;
+		ret = PTR_ERR(guc->execbuf_client);
+		goto err_ads;
 	}
 
 	return 0;
 
-err:
-	i915_guc_submission_fini(dev_priv);
-	return -ENOMEM;
+err_ads:
+	guc_addon_destroy(guc);
+err_log:
+	intel_guc_log_destroy(guc);
+err_vaddr:
+	i915_gem_object_unpin_map(guc->ctx_pool->obj);
+err_vma:
+	i915_vma_unpin_and_release(&guc->ctx_pool);
+	return ret;
+}
+
+void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
+{
+	struct intel_guc *guc = &dev_priv->guc;
+
+	guc_client_free(guc->execbuf_client);
+	guc->execbuf_client = NULL;
+	ida_destroy(&guc->ctx_ids);
+	guc_addon_destroy(guc);
+	intel_guc_log_destroy(guc);
+	i915_gem_object_unpin_map(guc->ctx_pool->obj);
+	i915_vma_unpin_and_release(&guc->ctx_pool);
 }
 
 static void guc_reset_wq(struct i915_guc_client *client)
@@ -1053,26 +1087,6 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
 	intel_execlists_enable_submission(dev_priv);
 }
 
-void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
-{
-	struct intel_guc *guc = &dev_priv->guc;
-	struct i915_guc_client *client;
-
-	client = fetch_and_zero(&guc->execbuf_client);
-	if (client && !IS_ERR(client))
-		guc_client_free(client);
-
-	i915_vma_unpin_and_release(&guc->ads_vma);
-	i915_vma_unpin_and_release(&guc->log.vma);
-
-	if (guc->ctx_pool_vaddr) {
-		ida_destroy(&guc->ctx_ids);
-		i915_gem_object_unpin_map(guc->ctx_pool->obj);
-	}
-
-	i915_vma_unpin_and_release(&guc->ctx_pool);
-}
-
 /**
  * intel_guc_suspend() - notify GuC entering suspend state
  * @dev_priv:	i915 device private
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index b1ebfce..f9e183a 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -443,7 +443,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 
 	err = i915_guc_submission_init(dev_priv);
 	if (err)
-		goto fail;
+		goto err_guc;
 
 	/*
 	 * WaEnableuKernelHeaderValidFix:skl,bxt
@@ -458,7 +458,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 		 */
 		err = guc_hw_reset(dev_priv);
 		if (err)
-			goto fail;
+			goto err_submission;
 
 		intel_huc_load(dev_priv);
 		err = guc_ucode_xfer(dev_priv);
@@ -466,7 +466,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 			break;
 
 		if (--retries == 0)
-			goto fail;
+			goto err_submission;
 
 		DRM_INFO("GuC fw load failed: %d; will reset and "
 			 "retry %d more time(s)\n", err, retries);
@@ -482,7 +482,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 
 		err = i915_guc_submission_enable(dev_priv);
 		if (err)
-			goto fail;
+			goto err_interrupts;
 	}
 
 	DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
@@ -492,15 +492,16 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 
 	return 0;
 
+err_interrupts:
+	gen9_disable_guc_interrupts(dev_priv);
+err_submission:
+	i915_guc_submission_fini(dev_priv);
+err_guc:
+	i915_ggtt_disable_guc(dev_priv);
 fail:
 	if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
 		guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
 
-	guc_interrupts_release(dev_priv);
-	i915_guc_submission_disable(dev_priv);
-	i915_guc_submission_fini(dev_priv);
-	i915_ggtt_disable_guc(dev_priv);
-
 	/*
 	 * We've failed to load the firmware :(
 	 *
@@ -540,6 +541,13 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
+void intel_guc_cleanup(struct drm_i915_private *dev_priv)
+{
+	gen9_disable_guc_interrupts(dev_priv);
+	i915_guc_submission_fini(dev_priv);
+	i915_ggtt_disable_guc(dev_priv);
+}
+
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 			 struct intel_uc_fw *uc_fw)
 {
@@ -745,12 +753,6 @@ void intel_guc_fini(struct drm_i915_private *dev_priv)
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 	struct drm_i915_gem_object *obj;
 
-	mutex_lock(&dev_priv->drm.struct_mutex);
-	guc_interrupts_release(dev_priv);
-	i915_guc_submission_disable(dev_priv);
-	i915_guc_submission_fini(dev_priv);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-
 	obj = fetch_and_zero(&guc_fw->obj);
 	if (obj)
 		i915_gem_object_put(obj);
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 5c0f9a4..3b1644b 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -66,7 +66,6 @@ static int guc_log_control(struct intel_guc *guc, u32 control_val)
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
-
 /*
  * Sub buffer switch callback. Called whenever relay has to switch to a new
  * sub buffer, relay stays on the same sub buffer if 0 is returned.
@@ -139,45 +138,15 @@ static int remove_buf_file_callback(struct dentry *dentry)
 	.remove_buf_file = remove_buf_file_callback,
 };
 
-static void guc_log_remove_relay_file(struct intel_guc *guc)
-{
-	relay_close(guc->log.relay_chan);
-}
-
-static int guc_log_create_relay_channel(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	struct rchan *guc_log_relay_chan;
-	size_t n_subbufs, subbuf_size;
-
-	/* Keep the size of sub buffers same as shared log buffer */
-	subbuf_size = guc->log.vma->obj->base.size;
-
-	/* Store up to 8 snapshots, which is large enough to buffer sufficient
-	 * boot time logs and provides enough leeway to User, in terms of
-	 * latency, for consuming the logs from relay. Also doesn't take
-	 * up too much memory.
-	 */
-	n_subbufs = 8;
-
-	guc_log_relay_chan = relay_open(NULL, NULL, subbuf_size,
-					n_subbufs, &relay_callbacks, dev_priv);
-	if (!guc_log_relay_chan) {
-		DRM_ERROR("Couldn't create relay chan for GuC logging\n");
-		return -ENOMEM;
-	}
-
-	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
-	guc->log.relay_chan = guc_log_relay_chan;
-	return 0;
-}
-
-static int guc_log_create_relay_file(struct intel_guc *guc)
+static int guc_log_relay_file_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct dentry *log_dir;
 	int ret;
 
+	if (i915.guc_log_level < 0)
+		return 0;
+
 	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
 	log_dir = dev_priv->drm.primary->debugfs_root;
 
@@ -198,7 +167,7 @@ static int guc_log_create_relay_file(struct intel_guc *guc)
 	}
 
 	ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
-	if (ret) {
+	if (ret < 0 && ret != -EEXIST) {
 		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
 		return ret;
 	}
@@ -371,31 +340,6 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 	}
 }
 
-static void guc_log_cleanup(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
-	lockdep_assert_held(&dev_priv->drm.struct_mutex);
-
-	/* First disable the flush interrupt */
-	gen9_disable_guc_interrupts(dev_priv);
-
-	if (guc->log.flush_wq)
-		destroy_workqueue(guc->log.flush_wq);
-
-	guc->log.flush_wq = NULL;
-
-	if (guc->log.relay_chan)
-		guc_log_remove_relay_file(guc);
-
-	guc->log.relay_chan = NULL;
-
-	if (guc->log.buf_addr)
-		i915_gem_object_unpin_map(guc->log.vma->obj);
-
-	guc->log.buf_addr = NULL;
-}
-
 static void capture_logs_work(struct work_struct *work)
 {
 	struct intel_guc *guc =
@@ -404,120 +348,105 @@ static void capture_logs_work(struct work_struct *work)
 	guc_log_capture_logs(guc);
 }
 
+static bool guc_log_has_extras(struct intel_guc *guc)
+{
+	return guc->log.buf_addr != NULL;
+}
+
 static int guc_log_create_extras(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	void *vaddr;
-	int ret;
+	struct rchan *guc_log_relay_chan;
+	size_t n_subbufs, subbuf_size;
+	int ret = 0;
 
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
 
-	/* Nothing to do */
-	if (i915.guc_log_level < 0)
-		return 0;
-
-	if (!guc->log.buf_addr) {
-		/* Create a WC (Uncached for read) vmalloc mapping of log
-		 * buffer pages, so that we can directly get the data
-		 * (up-to-date) from memory.
-		 */
-		vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
-		if (IS_ERR(vaddr)) {
-			ret = PTR_ERR(vaddr);
-			DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
-			return ret;
-		}
-
-		guc->log.buf_addr = vaddr;
-	}
+	GEM_BUG_ON(guc_log_has_extras(guc));
 
-	if (!guc->log.relay_chan) {
-		/* Create a relay channel, so that we have buffers for storing
-		 * the GuC firmware logs, the channel will be linked with a file
-		 * later on when debugfs is registered.
-		 */
-		ret = guc_log_create_relay_channel(guc);
-		if (ret)
-			return ret;
+	/* Create a WC (Uncached for read) vmalloc mapping of log
+	 * buffer pages, so that we can directly get the data
+	 * (up-to-date) from memory.
+	 */
+	vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
+	if (IS_ERR(vaddr)) {
+		DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
+		return PTR_ERR(vaddr);
 	}
 
-	if (!guc->log.flush_wq) {
-		INIT_WORK(&guc->log.flush_work, capture_logs_work);
-
-		 /*
-		 * GuC log buffer flush work item has to do register access to
-		 * send the ack to GuC and this work item, if not synced before
-		 * suspend, can potentially get executed after the GFX device is
-		 * suspended.
-		 * By marking the WQ as freezable, we don't have to bother about
-		 * flushing of this work item from the suspend hooks, the pending
-		 * work item if any will be either executed before the suspend
-		 * or scheduled later on resume. This way the handling of work
-		 * item can be kept same between system suspend & rpm suspend.
-		 */
-		guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
-							    WQ_HIGHPRI | WQ_FREEZABLE);
-		if (guc->log.flush_wq == NULL) {
-			DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
-			return -ENOMEM;
-		}
-	}
+	guc->log.buf_addr = vaddr;
 
-	return 0;
-}
+	 /* Keep the size of sub buffers same as shared log buffer */
+	subbuf_size = guc->log.vma->obj->base.size;
 
-void intel_guc_log_create(struct intel_guc *guc)
-{
-	struct i915_vma *vma;
-	unsigned long offset;
-	uint32_t size, flags;
+	/* Store up to 8 snapshots, which is large enough to buffer sufficient
+	 * boot time logs and provides enough leeway to User, in terms of
+	 * latency, for consuming the logs from relay. Also doesn't take
+	 * up too much memory.
+	 */
+	n_subbufs = 8;
 
-	if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
-		i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
+	/* Create a relay channel, so that we have buffers for storing
+	 * the GuC firmware logs, the channel will be linked with a file
+	 * later on when debugfs is registered.
+	 */
+	guc_log_relay_chan = relay_open(NULL, NULL, subbuf_size,
+					n_subbufs, &relay_callbacks, dev_priv);
+	if (!guc_log_relay_chan) {
+		DRM_ERROR("Couldn't create relay chan for GuC logging\n");
 
-	/* The first page is to save log buffer state. Allocate one
-	 * extra page for others in case for overlap */
-	size = (1 + GUC_LOG_DPC_PAGES + 1 +
-		GUC_LOG_ISR_PAGES + 1 +
-		GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
+		ret = -ENOMEM;
+		goto err_vaddr;
+	}
 
-	vma = guc->log.vma;
-	if (!vma) {
-		/* We require SSE 4.1 for fast reads from the GuC log buffer and
-		 * it should be present on the chipsets supporting GuC based
-		 * submisssions.
-		 */
-		if (WARN_ON(!i915_has_memcpy_from_wc())) {
-			/* logging will not be enabled */
-			i915.guc_log_level = -1;
-			return;
-		}
+	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
+	guc->log.relay_chan = guc_log_relay_chan;
 
-		vma = intel_guc_allocate_vma(guc, size);
-		if (IS_ERR(vma)) {
-			/* logging will be off */
-			i915.guc_log_level = -1;
-			return;
-		}
+	INIT_WORK(&guc->log.flush_work, capture_logs_work);
+
+	/*
+	 * GuC log buffer flush work item has to do register access to
+	 * send the ack to GuC and this work item, if not synced before
+	 * suspend, can potentially get executed after the GFX device is
+	 * suspended.
+	 * By marking the WQ as freezable, we don't have to bother about
+	 * flushing of this work item from the suspend hooks, the pending
+	 * work item if any will be either executed before the suspend
+	 * or scheduled later on resume. This way the handling of work
+	 * item can be kept same between system suspend & rpm suspend.
+	 */
+	guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
+						    WQ_HIGHPRI | WQ_FREEZABLE);
+	if (!guc->log.flush_wq) {
+		DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
+		ret = -ENOMEM;
+		goto err_relaychan;
+	}
 
-		guc->log.vma = vma;
+	return 0;
 
-		if (guc_log_create_extras(guc)) {
-			guc_log_cleanup(guc);
-			i915_vma_unpin_and_release(&guc->log.vma);
-			i915.guc_log_level = -1;
-			return;
-		}
-	}
+err_relaychan:
+	relay_close(guc->log.relay_chan);
+err_vaddr:
+	i915_gem_object_unpin_map(guc->log.vma->obj);
+	guc->log.buf_addr = NULL;
+	return ret;
+}
 
-	/* each allocated unit is a page */
-	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
-		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
-		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
-		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
+static void guc_log_destroy_extras(struct intel_guc *guc)
+{
+	/*
+	 * It's possible that extras were never allocated because guc_log_level
+	 * was < 0 at the time
+	 **/
+	if (!guc_log_has_extras(guc))
+		return;
 
-	offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
-	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
+	destroy_workqueue(guc->log.flush_wq);
+	relay_close(guc->log.relay_chan);
+	i915_gem_object_unpin_map(guc->log.vma->obj);
+	guc->log.buf_addr = NULL;
 }
 
 static int guc_log_late_setup(struct intel_guc *guc)
@@ -527,24 +456,25 @@ static int guc_log_late_setup(struct intel_guc *guc)
 
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
 
-	if (i915.guc_log_level < 0)
-		return -EINVAL;
+	if (!guc_log_has_extras(guc)) {
+		/* If log_level was set as -1 at boot time, then setup needed to
+		 * handle log buffer flush interrupts would not have been done yet,
+		 * so do that now.
+		 */
+		ret = guc_log_create_extras(guc);
+		if (ret)
+			goto err;
+	}
 
-	/* If log_level was set as -1 at boot time, then setup needed to
-	 * handle log buffer flush interrupts would not have been done yet,
-	 * so do that now.
-	 */
-	ret = guc_log_create_extras(guc);
+	ret = guc_log_relay_file_create(guc);
 	if (ret)
-		goto err;
-
-	ret = guc_log_create_relay_file(guc);
-	if (ret)
-		goto err;
+		goto err_extras;
 
 	return 0;
+
+err_extras:
+	guc_log_destroy_extras(guc);
 err:
-	guc_log_cleanup(guc);
 	/* logging will remain off */
 	i915.guc_log_level = -1;
 	return ret;
@@ -586,6 +516,72 @@ static void guc_flush_logs(struct intel_guc *guc)
 	guc_log_capture_logs(guc);
 }
 
+int intel_guc_log_create(struct intel_guc *guc)
+{
+	struct i915_vma *vma;
+	unsigned long offset;
+	uint32_t size, flags;
+	int ret;
+
+	GEM_BUG_ON(guc->log.vma);
+
+	if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
+		i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
+
+	/* The first page is to save log buffer state. Allocate one
+	 * extra page for others in case for overlap */
+	size = (1 + GUC_LOG_DPC_PAGES + 1 +
+		GUC_LOG_ISR_PAGES + 1 +
+		GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
+
+	/* We require SSE 4.1 for fast reads from the GuC log buffer and
+	 * it should be present on the chipsets supporting GuC based
+	 * submisssions.
+	 */
+	if (WARN_ON(!i915_has_memcpy_from_wc())) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	vma = intel_guc_allocate_vma(guc, size);
+	if (IS_ERR(vma)) {
+		ret = PTR_ERR(vma);
+		goto err;
+	}
+
+	guc->log.vma = vma;
+
+	if (i915.guc_log_level >= 0) {
+		ret = guc_log_create_extras(guc);
+		if (ret < 0)
+			goto err_vma;
+	}
+
+	/* each allocated unit is a page */
+	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
+		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
+		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
+		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
+
+	offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
+	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
+
+	return 0;
+
+err_vma:
+	i915_vma_unpin_and_release(&guc->log.vma);
+err:
+	/* logging will be off */
+	i915.guc_log_level = -1;
+	return ret;
+}
+
+void intel_guc_log_destroy(struct intel_guc *guc)
+{
+	guc_log_destroy_extras(guc);
+	i915_vma_unpin_and_release(&guc->log.vma);
+}
+
 int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 {
 	struct intel_guc *guc = &dev_priv->guc;
@@ -609,17 +605,21 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 		return ret;
 	}
 
-	i915.guc_log_level = log_param.verbosity;
+	if (log_param.logging_enabled) {
+		i915.guc_log_level = log_param.verbosity;
 
-	/* If log_level was set as -1 at boot time, then the relay channel file
-	 * wouldn't have been created by now and interrupts also would not have
-	 * been enabled.
-	 */
-	if (!dev_priv->guc.log.relay_chan) {
+		/* If log_level was set as -1 at boot time, then the relay channel file
+		 * wouldn't have been created by now and interrupts also would not have
+		 * been enabled. Try again now, just in case.
+		 */
 		ret = guc_log_late_setup(guc);
-		if (!ret)
-			gen9_enable_guc_interrupts(dev_priv);
-	} else if (!log_param.logging_enabled) {
+		if (ret < 0) {
+			DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
+			return ret;
+		}
+
+		gen9_enable_guc_interrupts(dev_priv);
+	} else {
 		/* Once logging is disabled, GuC won't generate logs & send an
 		 * interrupt. But there could be some data in the log buffer
 		 * which is yet to be captured. So request GuC to update the log
@@ -629,9 +629,6 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 
 		/* As logging is disabled, update log level to reflect that */
 		i915.guc_log_level = -1;
-	} else {
-		/* In case interrupts were disabled, enable them now */
-		gen9_enable_guc_interrupts(dev_priv);
 	}
 
 	return ret;
@@ -639,7 +636,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 
 void i915_guc_log_register(struct drm_i915_private *dev_priv)
 {
-	if (!i915.enable_guc_submission)
+	if (!i915.enable_guc_submission || i915.guc_log_level < 0)
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
@@ -653,6 +650,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	guc_log_cleanup(&dev_priv->guc);
+	gen9_disable_guc_interrupts(dev_priv);
+	guc_log_destroy_extras(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 0d12c8e..1a75ef2 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -190,8 +190,9 @@ struct intel_huc {
 
 /* intel_guc_loader.c */
 extern void intel_guc_init(struct drm_i915_private *dev_priv);
-extern int intel_guc_setup(struct drm_i915_private *dev_priv);
 extern void intel_guc_fini(struct drm_i915_private *dev_priv);
+extern int intel_guc_setup(struct drm_i915_private *dev_priv);
+extern void intel_guc_cleanup(struct drm_i915_private *dev_priv);
 extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
 extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
 extern int intel_guc_resume(struct drm_i915_private *dev_priv);
@@ -209,10 +210,11 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
 
 /* intel_guc_log.c */
-void intel_guc_log_create(struct intel_guc *guc);
+int intel_guc_log_create(struct intel_guc *guc);
+void intel_guc_log_destroy(struct intel_guc *guc);
+int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
 void i915_guc_log_register(struct drm_i915_private *dev_priv);
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
-int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
 
 static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 {
-- 
1.9.1

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

[04/11 v2] drm/i915/guc: s/ads_vma/addon

[Oscar Mateo]

This vma contains much more than just the additional data struct (ads)
and since we were already using the word "addon" as an object in
guc_addon_create, make it the preffered one. No need for the vma suffix
either, as that dependency is given by the type.

While at it, add an explanation of what things go inside the addon object.

v2: Move the explanation to the header of the file (Joonas).

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 14 ++++++++++----
 drivers/gpu/drm/i915/intel_guc_loader.c    |  4 ++--
 drivers/gpu/drm/i915/intel_uc.h            |  2 +-
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index f33c659..996e9f3 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -60,6 +60,13 @@
  * ELSP context descriptor dword into Work Item.
  * See guc_wq_item_append()
  *
+ * Addon object:
+ * The additional data struct (ADS) has pointers for different buffers used by
+ * the GuC. One single gem object, the "addon" object, contains the ADS itself\
+ * (guc_ads), the scheduling policies (guc_policies), a structure describing
+ * a collection of register sets (guc_mmio_reg_state) and some extra
+ * pages for the GuC to save its internal state for sleep.
+ *
  */
 
 static inline bool is_high_priority(struct i915_guc_client* client)
@@ -846,9 +853,8 @@ static int guc_addon_create(struct intel_guc *guc)
 	struct page *page;
 	u32 size;
 
-	GEM_BUG_ON(guc->ads_vma);
+	GEM_BUG_ON(guc->addon);
 
-	/* The ads obj includes the struct itself and buffers passed to GuC */
 	size = sizeof(struct guc_ads) + sizeof(struct guc_policies) +
 			sizeof(struct guc_mmio_reg_state) +
 			GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
@@ -857,7 +863,7 @@ static int guc_addon_create(struct intel_guc *guc)
 	if (IS_ERR(vma))
 		return PTR_ERR(vma);
 
-	guc->ads_vma = vma;
+	guc->addon = vma;
 
 	page = i915_vma_first_page(vma);
 	ads = kmap(page);
@@ -906,7 +912,7 @@ static int guc_addon_create(struct intel_guc *guc)
 
 static void guc_addon_destroy(struct intel_guc *guc)
 {
-	i915_vma_unpin_and_release(&guc->ads_vma);
+	i915_vma_unpin_and_release(&guc->addon);
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index f9e183a..3c59528 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -167,8 +167,8 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
 	} else
 		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
 
-	if (guc->ads_vma) {
-		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
+	if (guc->addon) {
+		u32 ads = guc_ggtt_offset(guc->addon) >> PAGE_SHIFT;
 		params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
 		params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
 	}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 1a75ef2..6c4d189 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -152,7 +152,7 @@ struct intel_guc {
 	/* intel_guc_recv interrupt related state */
 	bool interrupts_enabled;
 
-	struct i915_vma *ads_vma;
+	struct i915_vma *addon;
 	struct i915_vma *ctx_pool;
 	void *ctx_pool_vaddr;
 	struct ida ctx_ids;
-- 
1.9.1

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

[05/11 v2] drm/i915/guc: Break out the GuC log extras into their own "runtime" struct

[Oscar Mateo]

When initializing the GuC log struct, there is an object we need to
allocate always, since the GuC needs its address at fw load time.
The rest is only needed during runtime, in the sense that we only
create if we actually enable GuC logging. Make that distinction
explicit by subdividing further the intel_guc_log struct.

v2: Call the new struct "runtime", instead of "extras" (Joonas).

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  4 +--
 drivers/gpu/drm/i915/intel_guc_log.c | 70 ++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_uc.h      | 12 ++++---
 3 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ce445bc..e5461f1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1740,8 +1740,8 @@ static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
 			I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
 
 			/* Handle flush interrupt in bottom half */
-			queue_work(dev_priv->guc.log.flush_wq,
-				   &dev_priv->guc.log.flush_work);
+			queue_work(dev_priv->guc.log.runtime.flush_wq,
+				   &dev_priv->guc.log.runtime.flush_work);
 
 			dev_priv->guc.log.flush_interrupt_count++;
 		} else {
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 3b1644b..1224803 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -166,7 +166,7 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
 		return -ENODEV;
 	}
 
-	ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
+	ret = relay_late_setup_files(guc->log.runtime.relay_chan, "guc_log", log_dir);
 	if (ret < 0 && ret != -EEXIST) {
 		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
 		return ret;
@@ -183,15 +183,15 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
 	smp_wmb();
 
 	/* All data has been written, so now move the offset of sub buffer. */
-	relay_reserve(guc->log.relay_chan, guc->log.vma->obj->base.size);
+	relay_reserve(guc->log.runtime.relay_chan, guc->log.vma->obj->base.size);
 
 	/* Switch to the next sub buffer */
-	relay_flush(guc->log.relay_chan);
+	relay_flush(guc->log.runtime.relay_chan);
 }
 
 static void *guc_get_write_buffer(struct intel_guc *guc)
 {
-	if (!guc->log.relay_chan)
+	if (!guc->log.runtime.relay_chan)
 		return NULL;
 
 	/* Just get the base address of a new sub buffer and copy data into it
@@ -202,7 +202,7 @@ static void *guc_get_write_buffer(struct intel_guc *guc)
 	 * done without using relay_reserve() along with relay_write(). So its
 	 * better to use relay_reserve() alone.
 	 */
-	return relay_reserve(guc->log.relay_chan, 0);
+	return relay_reserve(guc->log.runtime.relay_chan, 0);
 }
 
 static bool guc_check_log_buf_overflow(struct intel_guc *guc,
@@ -253,11 +253,11 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 	void *src_data, *dst_data;
 	bool new_overflow;
 
-	if (WARN_ON(!guc->log.buf_addr))
+	if (WARN_ON(!guc->log.runtime.buf_addr))
 		return;
 
 	/* Get the pointer to shared GuC log buffer */
-	log_buf_state = src_data = guc->log.buf_addr;
+	log_buf_state = src_data = guc->log.runtime.buf_addr;
 
 	/* Get the pointer to local buffer to store the logs */
 	log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
@@ -343,17 +343,17 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 static void capture_logs_work(struct work_struct *work)
 {
 	struct intel_guc *guc =
-		container_of(work, struct intel_guc, log.flush_work);
+		container_of(work, struct intel_guc, log.runtime.flush_work);
 
 	guc_log_capture_logs(guc);
 }
 
-static bool guc_log_has_extras(struct intel_guc *guc)
+static bool guc_log_has_runtime(struct intel_guc *guc)
 {
-	return guc->log.buf_addr != NULL;
+	return guc->log.runtime.buf_addr != NULL;
 }
 
-static int guc_log_create_extras(struct intel_guc *guc)
+static int guc_log_runtime_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	void *vaddr;
@@ -363,7 +363,7 @@ static int guc_log_create_extras(struct intel_guc *guc)
 
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
 
-	GEM_BUG_ON(guc_log_has_extras(guc));
+	GEM_BUG_ON(guc_log_has_runtime(guc));
 
 	/* Create a WC (Uncached for read) vmalloc mapping of log
 	 * buffer pages, so that we can directly get the data
@@ -375,7 +375,7 @@ static int guc_log_create_extras(struct intel_guc *guc)
 		return PTR_ERR(vaddr);
 	}
 
-	guc->log.buf_addr = vaddr;
+	guc->log.runtime.buf_addr = vaddr;
 
 	 /* Keep the size of sub buffers same as shared log buffer */
 	subbuf_size = guc->log.vma->obj->base.size;
@@ -401,9 +401,9 @@ static int guc_log_create_extras(struct intel_guc *guc)
 	}
 
 	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
-	guc->log.relay_chan = guc_log_relay_chan;
+	guc->log.runtime.relay_chan = guc_log_relay_chan;
 
-	INIT_WORK(&guc->log.flush_work, capture_logs_work);
+	INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
 
 	/*
 	 * GuC log buffer flush work item has to do register access to
@@ -416,9 +416,9 @@ static int guc_log_create_extras(struct intel_guc *guc)
 	 * or scheduled later on resume. This way the handling of work
 	 * item can be kept same between system suspend & rpm suspend.
 	 */
-	guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
+	guc->log.runtime.flush_wq = alloc_ordered_workqueue("i915-guc_log",
 						    WQ_HIGHPRI | WQ_FREEZABLE);
-	if (!guc->log.flush_wq) {
+	if (!guc->log.runtime.flush_wq) {
 		DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
 		ret = -ENOMEM;
 		goto err_relaychan;
@@ -427,26 +427,26 @@ static int guc_log_create_extras(struct intel_guc *guc)
 	return 0;
 
 err_relaychan:
-	relay_close(guc->log.relay_chan);
+	relay_close(guc->log.runtime.relay_chan);
 err_vaddr:
 	i915_gem_object_unpin_map(guc->log.vma->obj);
-	guc->log.buf_addr = NULL;
+	guc->log.runtime.buf_addr = NULL;
 	return ret;
 }
 
-static void guc_log_destroy_extras(struct intel_guc *guc)
+static void guc_log_runtime_destroy(struct intel_guc *guc)
 {
 	/*
-	 * It's possible that extras were never allocated because guc_log_level
-	 * was < 0 at the time
+	 * It's possible that the runtime stuff was never allocated because
+	 * guc_log_level was < 0 at the time
 	 **/
-	if (!guc_log_has_extras(guc))
+	if (!guc_log_has_runtime(guc))
 		return;
 
-	destroy_workqueue(guc->log.flush_wq);
-	relay_close(guc->log.relay_chan);
+	destroy_workqueue(guc->log.runtime.flush_wq);
+	relay_close(guc->log.runtime.relay_chan);
 	i915_gem_object_unpin_map(guc->log.vma->obj);
-	guc->log.buf_addr = NULL;
+	guc->log.runtime.buf_addr = NULL;
 }
 
 static int guc_log_late_setup(struct intel_guc *guc)
@@ -456,24 +456,24 @@ static int guc_log_late_setup(struct intel_guc *guc)
 
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
 
-	if (!guc_log_has_extras(guc)) {
+	if (!guc_log_has_runtime(guc)) {
 		/* If log_level was set as -1 at boot time, then setup needed to
 		 * handle log buffer flush interrupts would not have been done yet,
 		 * so do that now.
 		 */
-		ret = guc_log_create_extras(guc);
+		ret = guc_log_runtime_create(guc);
 		if (ret)
 			goto err;
 	}
 
 	ret = guc_log_relay_file_create(guc);
 	if (ret)
-		goto err_extras;
+		goto err_runtime;
 
 	return 0;
 
-err_extras:
-	guc_log_destroy_extras(guc);
+err_runtime:
+	guc_log_runtime_destroy(guc);
 err:
 	/* logging will remain off */
 	i915.guc_log_level = -1;
@@ -507,7 +507,7 @@ static void guc_flush_logs(struct intel_guc *guc)
 	/* Before initiating the forceful flush, wait for any pending/ongoing
 	 * flush to complete otherwise forceful flush may not actually happen.
 	 */
-	flush_work(&guc->log.flush_work);
+	flush_work(&guc->log.runtime.flush_work);
 
 	/* Ask GuC to update the log buffer state */
 	guc_log_flush(guc);
@@ -552,7 +552,7 @@ int intel_guc_log_create(struct intel_guc *guc)
 	guc->log.vma = vma;
 
 	if (i915.guc_log_level >= 0) {
-		ret = guc_log_create_extras(guc);
+		ret = guc_log_runtime_create(guc);
 		if (ret < 0)
 			goto err_vma;
 	}
@@ -578,7 +578,7 @@ int intel_guc_log_create(struct intel_guc *guc)
 
 void intel_guc_log_destroy(struct intel_guc *guc)
 {
-	guc_log_destroy_extras(guc);
+	guc_log_runtime_destroy(guc);
 	i915_vma_unpin_and_release(&guc->log.vma);
 }
 
@@ -651,6 +651,6 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	gen9_disable_guc_interrupts(dev_priv);
-	guc_log_destroy_extras(&dev_priv->guc);
+	guc_log_runtime_destroy(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 6c4d189..1567c3e3 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -132,11 +132,13 @@ struct intel_uc_fw {
 struct intel_guc_log {
 	uint32_t flags;
 	struct i915_vma *vma;
-	void *buf_addr;
-	struct workqueue_struct *flush_wq;
-	struct work_struct flush_work;
-	struct rchan *relay_chan;
-
+	/* The runtime stuff gets created only when GuC logging gets enabled */
+	struct {
+		void *buf_addr;
+		struct workqueue_struct *flush_wq;
+		struct work_struct flush_work;
+		struct rchan *relay_chan;
+	} runtime;
 	/* logging related stats */
 	u32 capture_miss_count;
 	u32 flush_interrupt_count;
-- 
1.9.1

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

[06/11 v3] drm/i915/guc: Make intel_guc_send a function pointer

[Oscar Mateo]

Prepare for an alternate GuC communication interface.

v2: Make a few functions static and name them correctly while we are at it (Oscar), but
leave an intel_guc_send_mmio interface for users that require old-style communication.

v3: Send intel_uc_init_early back to the top (Michal).

Signed-off-by: Michel Thierry <michel.thierry@intel.com>
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 17 +++++++++++------
 drivers/gpu/drm/i915/intel_uc.h | 10 +++++++++-
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index c46bc85..ade7194 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -27,14 +27,17 @@
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
-	mutex_init(&dev_priv->guc.send_mutex);
+	struct intel_guc *guc = &dev_priv->guc;
+
+	mutex_init(&guc->send_mutex);
+	guc->send = intel_guc_send_mmio;
 }
 
 /*
  * Read GuC command/status register (SOFT_SCRATCH_0)
  * Return true if it contains a response rather than a command
  */
-static bool intel_guc_recv(struct intel_guc *guc, u32 *status)
+static bool guc_recv(struct intel_guc *guc, u32 *status)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
@@ -43,7 +46,10 @@ static bool intel_guc_recv(struct intel_guc *guc, u32 *status)
 	return INTEL_GUC_RECV_IS_RESPONSE(val);
 }
 
-int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
+/*
+ * This function implements the MMIO based host to GuC interface.
+ */
+int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	u32 status;
@@ -71,9 +77,9 @@ int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
 	 * up to that length of time, then switch to a slower sleep-wait loop.
 	 * No inte_guc_send command should ever take longer than 10ms.
 	 */
-	ret = wait_for_us(intel_guc_recv(guc, &status), 10);
+	ret = wait_for_us(guc_recv(guc, &status), 10);
 	if (ret)
-		ret = wait_for(intel_guc_recv(guc, &status), 10);
+		ret = wait_for(guc_recv(guc, &status), 10);
 	if (status != INTEL_GUC_STATUS_SUCCESS) {
 		/*
 		 * Either the GuC explicitly returned an error (which
@@ -113,4 +119,3 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
 
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
-
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 1567c3e3..011a9e0 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -176,6 +176,9 @@ struct intel_guc {
 
 	/* To serialize the intel_guc_send actions */
 	struct mutex send_mutex;
+
+	/* GuC's FW specific send function */
+	int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
 };
 
 struct intel_huc {
@@ -187,9 +190,14 @@ struct intel_huc {
 
 /* intel_uc.c */
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
-int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 
+int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
+static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
+{
+	return guc->send(guc, action, len);
+}
+
 /* intel_guc_loader.c */
 extern void intel_guc_init(struct drm_i915_private *dev_priv);
 extern void intel_guc_fini(struct drm_i915_private *dev_priv);
-- 
1.9.1

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

[07/11] drm/i915/guc: Improve the GuC documentation & comments about proxy submissions

[Oscar Mateo]

While at it, fix a typo (s/ring_lcra/ring_lrca) and improve the naming of one
firware interface field (s/ring_tail/submit_element_info, since it can contain
more than just the ring tail).

No change in functionality.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 42 +++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_guc_fwif.h      |  4 +--
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 996e9f3..4e82be6 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -28,16 +28,25 @@
 /**
  * DOC: GuC-based command submission
  *
- * i915_guc_client:
- * We use the term client to avoid confusion with contexts. A i915_guc_client is
- * equivalent to GuC object guc_context_desc. This context descriptor is
- * allocated from a pool of 1024 entries. Kernel driver will allocate doorbell
- * and workqueue for it. Also the process descriptor (guc_process_desc), which
- * is mapped to client space. So the client can write Work Item then ring the
- * doorbell.
+ * GuC client:
+ * A i915_guc_client refers to a unique user of the GuC. Currently, there is
+ * only one of these (the execbuf_client) and this one is charged with all
+ * submissions to the GuC. This struct is the owner of a doorbell, a process
+ * descriptor and a workqueue (all of them inside a single gem object that
+ * contains all required pages for these elements).
  *
- * To simplify the implementation, we allocate one gem object that contains all
- * pages for doorbell, process descriptor and workqueue.
+ * GuC context descriptor:
+ * During initialization, the driver allocates a static pool of 1024 such
+ * descriptors, and shares them with the GuC.
+ * Currently, there exists a 1:1 mapping between a i915_guc_client and a
+ * guc_context_desc (via the client's context_index), so effectively only
+ * one guc_context_desc gets used. This context descriptor lets the GuC know
+ * about the doorbell, workqueue and process descriptor. Theoretically, it also
+ * lets the GuC know about our HW contexts (Context ID, etc...), but we actually
+ * employ a kind of submission where the GuC uses the LRCA sent via the work
+ * item instead (the single guc_context_desc associated to execbuf client
+ * contains information about the default kernel context only, but this is
+ * essentially unused). This is called a "proxy" submission.
  *
  * The Scratch registers:
  * There are 16 MMIO-based registers start from 0xC180. The kernel driver writes
@@ -308,10 +317,17 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
 		if (!ce->state)
 			break;	/* XXX: continue? */
 
+		/*
+		 * XXX: When this is a GUC_CTX_DESC_ATTR_KERNEL client (proxy
+		 * submission or, in other words, not using a direct submission
+		 * model) the KMD's LRCA is not used for any work submission.
+		 * Instead, the GuC uses the LRCA of the user mode context (see
+		 * guc_wq_item_append below).
+		 */
 		lrc->context_desc = lower_32_bits(ce->lrc_desc);
 
 		/* The state page is after PPHWSP */
-		lrc->ring_lcra =
+		lrc->ring_lrca =
 			guc_ggtt_offset(ce->state) + LRC_STATE_PN * PAGE_SIZE;
 		lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
 				(guc_engine_id << GUC_ELC_ENGINE_OFFSET);
@@ -342,10 +358,6 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
 	desc->wq_addr = gfx_addr + client->wq_offset;
 	desc->wq_size = client->wq_size;
 
-	/*
-	 * XXX: Take LRCs from an existing context if this is not an
-	 * IsKMDCreatedContext client
-	 */
 	desc->desc_private = (uintptr_t)client;
 }
 
@@ -469,7 +481,7 @@ static void guc_wq_item_append(struct i915_guc_client *client,
 	/* The GuC wants only the low-order word of the context descriptor */
 	wqi->context_desc = (u32)intel_lr_context_descriptor(rq->ctx, engine);
 
-	wqi->ring_tail = tail << WQ_RING_TAIL_SHIFT;
+	wqi->submit_element_info = tail << WQ_RING_TAIL_SHIFT;
 	wqi->fence_id = rq->global_seqno;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 3ae8cef..4edf40f 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -251,7 +251,7 @@ struct guc_doorbell_info {
 struct guc_wq_item {
 	u32 header;
 	u32 context_desc;
-	u32 ring_tail;
+	u32 submit_element_info;
 	u32 fence_id;
 } __packed;
 
@@ -278,7 +278,7 @@ struct guc_execlist_context {
 	u32 context_desc;
 	u32 context_id;
 	u32 ring_status;
-	u32 ring_lcra;
+	u32 ring_lrca;
 	u32 ring_begin;
 	u32 ring_end;
 	u32 ring_next_free_location;
-- 
1.9.1

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

[08/11] drm/i915/guc: Wait for doorbell to be inactive before deallocating

[Oscar Mateo]

Doorbell release flow requires that we wait for GEN8_DRB_VALID bit to go
to zero after updating db_status before we call the GuC to release the
doorbell.

Kudos to Daniele for finding this out.

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 4e82be6..eb677d5 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -206,12 +206,22 @@ static int __create_doorbell(struct i915_guc_client *client)
 
 static int __destroy_doorbell(struct i915_guc_client *client)
 {
+	struct drm_i915_private *dev_priv = guc_to_i915(client->guc);
 	struct guc_doorbell_info *doorbell;
+	u16 db_id = client->doorbell_id;
+
+	GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID);
 
 	doorbell = __get_doorbell(client);
 	doorbell->db_status = GUC_DOORBELL_DISABLED;
 	doorbell->cookie = 0;
 
+	/* Doorbell release flow requires that we wait for GEN8_DRB_VALID bit
+	 * to go to zero after updating db_status before we call the GuC to
+	 * release the doorbell */
+	if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID), 10))
+		DRM_ERROR("Doorbell never became invalid after disable\n");
+
 	return __guc_deallocate_doorbell(client->guc, client->ctx_index);
 }
 
-- 
1.9.1

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

[09/11] drm/i915/guc: A little bit more of doorbell sanitization

[Oscar Mateo]

Some recent refactoring patches have left the doorbell creation outside
the GuC client allocation, which does not make a lot of sense (a client
without a doorbell is something useless). Move it back there, and
refactor the init_doorbell_hw consequently.

Thanks to this, we can do some other improvements, like hoisting the
check for GuC submission enabled out of the enable function.

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            |   3 +-
 drivers/gpu/drm/i915/i915_guc_submission.c | 228 ++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_guc_loader.c    |  22 ++-
 3 files changed, 143 insertions(+), 110 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3329507..8f0e569 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -549,8 +549,7 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
 static void i915_gem_fini(struct drm_i915_private *dev_priv)
 {
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	if (i915.enable_guc_submission)
-		intel_guc_cleanup(dev_priv);
+	intel_guc_cleanup(dev_priv);
 	i915_gem_cleanup_engines(dev_priv);
 	i915_gem_context_fini(dev_priv);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index eb677d5..a912ec4 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -163,15 +163,13 @@ static struct guc_context_desc *__get_context_desc(struct i915_guc_client *clien
  * client object which contains the page being used for the doorbell
  */
 
-static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
+static void __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
 {
 	struct guc_context_desc *desc;
 
 	/* Update the GuC's idea of the doorbell ID */
 	desc = __get_context_desc(client);
 	desc->db_id = new_id;
-
-	return 0;
 }
 
 static struct guc_doorbell_info *__get_doorbell(struct i915_guc_client *client)
@@ -225,6 +223,28 @@ static int __destroy_doorbell(struct i915_guc_client *client)
 	return __guc_deallocate_doorbell(client->guc, client->ctx_index);
 }
 
+static int create_doorbell(struct i915_guc_client *client)
+{
+	int ret;
+
+	ret = __reserve_doorbell(client);
+	if (ret)
+		return ret;
+
+	__update_doorbell_desc(client, client->doorbell_id);
+
+	ret = __create_doorbell(client);
+	if (ret)
+		goto err;
+
+	return 0;
+
+err:
+	__update_doorbell_desc(client, GUC_DOORBELL_INVALID);
+	__unreserve_doorbell(client);
+	return ret;
+}
+
 static int destroy_doorbell(struct i915_guc_client *client)
 {
 	int err;
@@ -495,6 +515,17 @@ static void guc_wq_item_append(struct i915_guc_client *client,
 	wqi->fence_id = rq->global_seqno;
 }
 
+static void guc_reset_wq(struct i915_guc_client *client)
+{
+	struct guc_process_desc *desc = client->vaddr +
+					client->proc_desc_offset;
+
+	desc->head = 0;
+	desc->tail = 0;
+
+	client->wq_tail = 0;
+}
+
 static int guc_ring_doorbell(struct i915_guc_client *client)
 {
 	struct guc_process_desc *desc;
@@ -651,19 +682,6 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
 	return vma;
 }
 
-static void guc_client_free(struct i915_guc_client *client)
-{
-	/*
-	 * XXX: wait for any outstanding submissions before freeing memory.
-	 * Be sure to drop any locks
-	 */
-	guc_ctx_desc_fini(client->guc, client);
-	i915_gem_object_unpin_map(client->vma->obj);
-	i915_vma_unpin_and_release(&client->vma);
-	ida_simple_remove(&client->guc->ctx_ids, client->ctx_index);
-	kfree(client);
-}
-
 /* Check that a doorbell register is in the expected state */
 static bool doorbell_ok(struct intel_guc *guc, u16 db_id)
 {
@@ -689,9 +707,8 @@ static int __reset_doorbell(struct i915_guc_client* client, u16 db_id)
 {
 	int err;
 
-	err = __update_doorbell_desc(client, db_id);
-	if (!err)
-		err = __create_doorbell(client);
+	__update_doorbell_desc(client, db_id);
+	err = __create_doorbell(client);
 	if (!err)
 		err = __destroy_doorbell(client);
 
@@ -699,48 +716,61 @@ static int __reset_doorbell(struct i915_guc_client* client, u16 db_id)
 }
 
 /*
- * Borrow the first client to set up & tear down each unused doorbell
- * in turn, to ensure that all doorbell h/w is (re)initialised.
+ * Set up & tear down each unused doorbell in turn, to ensure that all doorbell
+ * HW is (re)initialised. For that end, we might have to borrow the first
+ * client. Also, tell GuC about all the doorbells in use by all clients.
+ * We do this because the KMD, the GuC and the doorbell HW can easily go out of
+ * sync (e.g. we can reset the GuC, but not the doorbel HW).
  */
 static int guc_init_doorbell_hw(struct intel_guc *guc)
 {
 	struct i915_guc_client *client = guc->execbuf_client;
-	int err;
-	int i;
-
-	if (has_doorbell(client))
-		destroy_doorbell(client);
+	bool recreate_first_client = false;
+	u16 db_id;
+	int ret;
 
-	for (i = 0; i < GUC_NUM_DOORBELLS; ++i) {
-		if (doorbell_ok(guc, i))
+	/* For unused doorbells, make sure they are disabled */
+	for_each_clear_bit(db_id, guc->doorbell_bitmap, GUC_NUM_DOORBELLS) {
+		if (doorbell_ok(guc, db_id))
 			continue;
 
-		err = __reset_doorbell(client, i);
-		WARN(err, "Doorbell %d reset failed, err %d\n", i, err);
+		if (has_doorbell(client)) {
+			/* Borrow execbuf_client (we will recreate it later) */
+			destroy_doorbell(client);
+			recreate_first_client = true;
+		}
+
+		ret = __reset_doorbell(client, db_id);
+		WARN(ret, "Doorbell %u reset failed, err %d\n", db_id, ret);
 	}
 
-	/* Read back & verify all doorbell registers */
-	for (i = 0; i < GUC_NUM_DOORBELLS; ++i)
-		WARN_ON(!doorbell_ok(guc, i));
+	if (recreate_first_client) {
+		ret = __reserve_doorbell(client);
+		if (unlikely(ret)) {
+			DRM_ERROR("Couldn't re-reserve first client db: %d\n", ret);
+			return ret;
+		}
 
-	err = __reserve_doorbell(client);
-	if (err)
-		return err;
+		__update_doorbell_desc(client, client->doorbell_id);
+	}
 
-	err = __update_doorbell_desc(client, client->doorbell_id);
-	if (err)
-		goto err_reserve;
+	/* Now for every client (and not only execbuf_client) make sure their
+	 * doorbells are known by the GuC */
+	//for (client = client_list; client != NULL; client = client->next)
+	{
+		ret = __create_doorbell(client);
+		if (ret) {
+			DRM_ERROR("Couldn't recreate client %u doorbell: %d\n",
+				client->ctx_index, ret);
+			return ret;
+		}
+	}
 
-	err = __create_doorbell(client);
-	if (err)
-		goto err_update;
+	/* Read back & verify all (used & unused) doorbell registers */
+	for (db_id = 0; db_id < GUC_NUM_DOORBELLS; ++db_id)
+		WARN_ON(!doorbell_ok(guc, db_id));
 
 	return 0;
-err_reserve:
-	__unreserve_doorbell(client);
-err_update:
-	__update_doorbell_desc(client, GUC_DOORBELL_INVALID);
-	return err;
 }
 
 /**
@@ -820,11 +850,9 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 	guc_proc_desc_init(guc, client);
 	guc_ctx_desc_init(guc, client);
 
-	/* FIXME: Runtime client allocation (which currently we don't do) will
-	 * require that the doorbell gets created now. The static execbuf_client
-	 * is now getting its doorbell later (on submission enable) but maybe we
-	 * also want to reorder things in the future so that we don't have to
-	 * special case the doorbell creation */
+	ret = create_doorbell(client);
+	if (ret)
+		goto err_vaddr;
 
 	DRM_DEBUG_DRIVER("new priority %u client %p for engine(s) 0x%x: ctx_index %u\n",
 			 priority, client, client->engines, client->ctx_index);
@@ -832,6 +860,9 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 			 client->doorbell_id, client->doorbell_offset);
 
 	return client;
+
+err_vaddr:
+	i915_gem_object_unpin_map(client->vma->obj);
 err_vma:
 	i915_vma_unpin_and_release(&client->vma);
 err_id:
@@ -841,6 +872,24 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 	return ERR_PTR(ret);
 }
 
+static void guc_client_free(struct i915_guc_client *client)
+{
+	/*
+	 * XXX: wait for any outstanding submissions before freeing memory.
+	 * Be sure to drop any locks
+	 */
+
+	/* FIXME: in many cases, by the time we get here the GuC has been
+	 * reset, so we cannot destroy the doorbell properly. Ignore the
+	 * error message for now */
+	destroy_doorbell(client);
+	guc_ctx_desc_fini(client->guc, client);
+	i915_gem_object_unpin_map(client->vma->obj);
+	i915_vma_unpin_and_release(&client->vma);
+	ida_simple_remove(&client->guc->ctx_ids, client->ctx_index);
+	kfree(client);
+}
+
 static void guc_policies_init(struct guc_policies *policies)
 {
 	struct guc_policy *policy;
@@ -938,8 +987,8 @@ static void guc_addon_destroy(struct intel_guc *guc)
 }
 
 /*
- * Set up the memory resources to be shared with the GuC.  At this point,
- * we require just one object that can be mapped through the GGTT.
+ * Set up the memory resources to be shared with the GuC (via the GGTT)
+ * at firmware loading time.
  */
 int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 {
@@ -951,16 +1000,6 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	void *vaddr;
 	int ret;
 
-	if (!HAS_GUC_SCHED(dev_priv))
-		return 0;
-
-	/* Wipe bitmap & delete client in case of reinitialisation */
-	bitmap_clear(guc->doorbell_bitmap, 0, GUC_NUM_DOORBELLS);
-	i915_guc_submission_disable(dev_priv);
-
-	if (!i915.enable_guc_submission)
-		return 0;
-
 	if (guc->ctx_pool)
 		return 0;
 
@@ -988,20 +1027,8 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 
 	ida_init(&guc->ctx_ids);
 
-	guc->execbuf_client = guc_client_alloc(dev_priv,
-					       INTEL_INFO(dev_priv)->ring_mask,
-					       GUC_CTX_PRIORITY_KMD_NORMAL,
-					       dev_priv->kernel_context);
-	if (IS_ERR(guc->execbuf_client)) {
-		DRM_ERROR("Failed to create GuC client for execbuf!\n");
-		ret = PTR_ERR(guc->execbuf_client);
-		goto err_ads;
-	}
-
 	return 0;
 
-err_ads:
-	guc_addon_destroy(guc);
 err_log:
 	intel_guc_log_destroy(guc);
 err_vaddr:
@@ -1015,8 +1042,6 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
 
-	guc_client_free(guc->execbuf_client);
-	guc->execbuf_client = NULL;
 	ida_destroy(&guc->ctx_ids);
 	guc_addon_destroy(guc);
 	intel_guc_log_destroy(guc);
@@ -1024,17 +1049,6 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	i915_vma_unpin_and_release(&guc->ctx_pool);
 }
 
-static void guc_reset_wq(struct i915_guc_client *client)
-{
-	struct guc_process_desc *desc = client->vaddr +
-					client->proc_desc_offset;
-
-	desc->head = 0;
-	desc->tail = 0;
-
-	client->wq_tail = 0;
-}
-
 static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
@@ -1063,17 +1077,28 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 	enum intel_engine_id id;
 	int err;
 
-	if (!client)
-		return -ENODEV;
+	if (!client) {
+		client = guc_client_alloc(dev_priv,
+					  INTEL_INFO(dev_priv)->ring_mask,
+					  GUC_CTX_PRIORITY_KMD_NORMAL,
+					  dev_priv->kernel_context);
+		if (IS_ERR(client)) {
+			DRM_ERROR("Failed to create GuC client for execbuf!\n");
+			return PTR_ERR(client);
+		}
+
+		guc->execbuf_client = client;
+	}
 
 	err = intel_guc_sample_forcewake(guc);
 	if (err)
-		return err;
+		goto err_execbuf_client;
 
 	guc_reset_wq(client);
+
 	err = guc_init_doorbell_hw(guc);
 	if (err)
-		return err;
+		goto err_execbuf_client;
 
 	/* Take over from manual control of ELSP (execlists) */
 	for_each_engine(engine, dev_priv, id) {
@@ -1097,22 +1122,22 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 	}
 
 	return 0;
+
+err_execbuf_client:
+	guc_client_free(guc->execbuf_client);
+	guc->execbuf_client = NULL;
+	return err;
 }
 
 void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
 
-	if (!guc->execbuf_client)
-		return;
-
-	/* FIXME: in many cases, by the time we get here the GuC has been
-	 * reset, so we cannot destroy the doorbell properly. Ignore the
-	 * error message for now */
-	destroy_doorbell(guc->execbuf_client);
-
 	/* Revert back to manual ELSP submission */
 	intel_execlists_enable_submission(dev_priv);
+
+	guc_client_free(guc->execbuf_client);
+	guc->execbuf_client = NULL;
 }
 
 /**
@@ -1141,7 +1166,6 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
 
-
 /**
  * intel_guc_resume() - notify GuC resuming from suspend state
  * @dev_priv:	i915 device private
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 3c59528..649c4db 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -441,9 +441,13 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 		intel_uc_fw_status_repr(guc_fw->fetch_status),
 		intel_uc_fw_status_repr(guc_fw->load_status));
 
-	err = i915_guc_submission_init(dev_priv);
-	if (err)
-		goto err_guc;
+	if (i915.enable_guc_submission) {
+		/* This is stuff we need to have available at fw load time
+		 * if we are planning to enable submission later */
+		err = i915_guc_submission_init(dev_priv);
+		if (err)
+			goto err_guc;
+	}
 
 	/*
 	 * WaEnableuKernelHeaderValidFix:skl,bxt
@@ -495,7 +499,8 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 err_interrupts:
 	gen9_disable_guc_interrupts(dev_priv);
 err_submission:
-	i915_guc_submission_fini(dev_priv);
+	if (i915.enable_guc_submission)
+		i915_guc_submission_fini(dev_priv);
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
 fail:
@@ -543,8 +548,13 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 
 void intel_guc_cleanup(struct drm_i915_private *dev_priv)
 {
-	gen9_disable_guc_interrupts(dev_priv);
-	i915_guc_submission_fini(dev_priv);
+	if (i915.enable_guc_submission) {
+		guc_interrupts_release(dev_priv);
+		i915_guc_submission_disable(dev_priv);
+		gen9_disable_guc_interrupts(dev_priv);
+		i915_guc_submission_fini(dev_priv);
+	}
+
 	i915_ggtt_disable_guc(dev_priv);
 }
 
-- 
1.9.1

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

[10/11] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor"

[Oscar Mateo]

A GuC context and a HW context are in no way related, so the name "GuC context descriptor"
is very unfortunate, because a new reader of the code gets overwhelmed very quickly with
a lot of things called "context" that refer to different things. We can improve legibility
a lot by simply renaming a few objects in the GuC code.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |   4 +-
 drivers/gpu/drm/i915/i915_guc_submission.c | 103 +++++++++++++++--------------
 drivers/gpu/drm/i915/intel_guc_fwif.h      |  38 +++++------
 drivers/gpu/drm/i915/intel_guc_loader.c    |   2 +-
 drivers/gpu/drm/i915/intel_uc.h            |   4 +-
 5 files changed, 78 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c395ccf..76a5d39 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2467,8 +2467,8 @@ static void i915_guc_client_info(struct seq_file *m,
 	enum intel_engine_id id;
 	uint64_t tot = 0;
 
-	seq_printf(m, "\tPriority %d, GuC ctx index: %u, PD offset 0x%x\n",
-		client->priority, client->ctx_index, client->proc_desc_offset);
+	seq_printf(m, "\tPriority %d, GuC stage index: %u, PD offset 0x%x\n",
+		client->priority, client->stage_id, client->proc_desc_offset);
 	seq_printf(m, "\tDoorbell id %d, offset: 0x%lx, cookie 0x%x\n",
 		client->doorbell_id, client->doorbell_offset, client->doorbell_cookie);
 	seq_printf(m, "\tWQ size %d, offset: 0x%x, tail %d\n",
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index a912ec4..e320008 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -35,16 +35,18 @@
  * descriptor and a workqueue (all of them inside a single gem object that
  * contains all required pages for these elements).
  *
- * GuC context descriptor:
- * During initialization, the driver allocates a static pool of 1024 such
- * descriptors, and shares them with the GuC.
+ * GuC stage descriptor:
+ * Technically, this is known as a "GuC Context" descriptor in the specs, but
+ * we use the term "stage" to avoid confusion with all the other things already
+ * named "context" in the driver. During initialization, the driver allocates
+ * a static pool of 1024 such descriptors, and shares them with the GuC.
  * Currently, there exists a 1:1 mapping between a i915_guc_client and a
- * guc_context_desc (via the client's context_index), so effectively only
- * one guc_context_desc gets used. This context descriptor lets the GuC know
- * about the doorbell, workqueue and process descriptor. Theoretically, it also
- * lets the GuC know about our HW contexts (Context ID, etc...), but we actually
+ * guc_stage_desc (via the client's stage_id), so effectively only one
+ * gets used. This stage descriptor lets the GuC know about the doorbell,
+ * workqueue and process descriptor. Theoretically, it also lets the GuC
+ * know about our HW contexts (context ID, etc...), but we actually
  * employ a kind of submission where the GuC uses the LRCA sent via the work
- * item instead (the single guc_context_desc associated to execbuf client
+ * item instead (the single guc_stage_desc associated to execbuf client
  * contains information about the default kernel context only, but this is
  * essentially unused). This is called a "proxy" submission.
  *
@@ -80,7 +82,7 @@
 
 static inline bool is_high_priority(struct i915_guc_client* client)
 {
-	return client->priority <= GUC_CTX_PRIORITY_HIGH;
+	return client->priority <= GUC_CLIENT_PRIORITY_HIGH;
 }
 
 static int __reserve_doorbell(struct i915_guc_client *client)
@@ -113,7 +115,7 @@ static int __reserve_doorbell(struct i915_guc_client *client)
 	__set_bit(id, client->guc->doorbell_bitmap);
 	client->doorbell_id = id;
 	DRM_DEBUG_DRIVER("client %u (high prio=%s) reserved doorbell %d: \n",
-			 client->ctx_index, yesno(is_high_priority(client)),
+			 client->stage_id, yesno(is_high_priority(client)),
 			 id);
 	return 0;
 }
@@ -130,30 +132,30 @@ static void __unreserve_doorbell(struct i915_guc_client *client)
  * Tell the GuC to allocate or deallocate a specific doorbell
  */
 
-static int __guc_allocate_doorbell(struct intel_guc *guc, u32 ctx_index)
+static int __guc_allocate_doorbell(struct intel_guc *guc, u32 stage_id)
 {
 	u32 action[] = {
 		INTEL_GUC_ACTION_ALLOCATE_DOORBELL,
-		ctx_index
+		stage_id
 	};
 
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
-static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
+static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 stage_id)
 {
 	u32 action[] = {
 		INTEL_GUC_ACTION_DEALLOCATE_DOORBELL,
-		ctx_index
+		stage_id
 	};
 
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
-static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)
+static struct guc_stage_desc *__get_stage_desc(struct i915_guc_client *client)
 {
 	return client->guc->ctx_pool_vaddr +
-		sizeof(struct guc_context_desc) * client->ctx_index;
+		sizeof(struct guc_stage_desc) * client->stage_id;
 }
 
 /*
@@ -165,10 +167,10 @@ static struct guc_context_desc *__get_context_desc(struct i915_guc_client *clien
 
 static void __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
 {
-	struct guc_context_desc *desc;
+	struct guc_stage_desc *desc;
 
 	/* Update the GuC's idea of the doorbell ID */
-	desc = __get_context_desc(client);
+	desc = __get_stage_desc(client);
 	desc->db_id = new_id;
 }
 
@@ -194,7 +196,7 @@ static int __create_doorbell(struct i915_guc_client *client)
 	doorbell->db_status = GUC_DOORBELL_ENABLED;
 	doorbell->cookie = client->doorbell_cookie;
 
-	err = __guc_allocate_doorbell(client->guc, client->ctx_index);
+	err = __guc_allocate_doorbell(client->guc, client->stage_id);
 	if (err) {
 		doorbell->db_status = GUC_DOORBELL_DISABLED;
 		doorbell->cookie = 0;
@@ -220,7 +222,7 @@ static int __destroy_doorbell(struct i915_guc_client *client)
 	if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID), 10))
 		DRM_ERROR("Doorbell never became invalid after disable\n");
 
-	return __guc_deallocate_doorbell(client->guc, client->ctx_index);
+	return __guc_deallocate_doorbell(client->guc, client->stage_id);
 }
 
 static int create_doorbell(struct i915_guc_client *client)
@@ -301,34 +303,34 @@ static void guc_proc_desc_init(struct intel_guc *guc,
 	desc->wq_base_addr = 0;
 	desc->db_base_addr = 0;
 
-	desc->context_id = client->ctx_index;
+	desc->stage_id = client->stage_id;
 	desc->wq_size_bytes = client->wq_size;
 	desc->wq_status = WQ_STATUS_ACTIVE;
 	desc->priority = client->priority;
 }
 
 /*
- * Initialise/clear the context descriptor shared with the GuC firmware.
+ * Initialise/clear the stage descriptor shared with the GuC firmware.
  *
  * This descriptor tells the GuC where (in GGTT space) to find the important
  * data structures relating to this client (doorbell, process descriptor,
  * write queue, etc).
  */
-static void guc_ctx_desc_init(struct intel_guc *guc,
-			      struct i915_guc_client *client)
+static void guc_stage_desc_init(struct intel_guc *guc,
+				struct i915_guc_client *client)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct intel_engine_cs *engine;
 	struct i915_gem_context *ctx = client->owner;
-	struct guc_context_desc *desc;
+	struct guc_stage_desc *desc;
 	unsigned int tmp;
 	u32 gfx_addr;
 
-	desc = __get_context_desc(client);
+	desc = __get_stage_desc(client);
 	memset(desc, 0, sizeof(*desc));
 
-	desc->attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
-	desc->context_id = client->ctx_index;
+	desc->attribute = GUC_STAGE_DESC_ATTR_ACTIVE | GUC_STAGE_DESC_ATTR_KERNEL;
+	desc->stage_id = client->stage_id;
 	desc->priority = client->priority;
 	desc->db_id = client->doorbell_id;
 
@@ -348,7 +350,7 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
 			break;	/* XXX: continue? */
 
 		/*
-		 * XXX: When this is a GUC_CTX_DESC_ATTR_KERNEL client (proxy
+		 * XXX: When this is a GUC_STAGE_DESC_ATTR_KERNEL client (proxy
 		 * submission or, in other words, not using a direct submission
 		 * model) the KMD's LRCA is not used for any work submission.
 		 * Instead, the GuC uses the LRCA of the user mode context (see
@@ -359,7 +361,10 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
 		/* The state page is after PPHWSP */
 		lrc->ring_lrca =
 			guc_ggtt_offset(ce->state) + LRC_STATE_PN * PAGE_SIZE;
-		lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
+
+		/* XXX: In direct submission, the GuC wants the HW context id
+		 * here. In proxy submission, it wants the stage id */
+		lrc->context_id = (client->stage_id << GUC_ELC_CTXID_OFFSET) |
 				(guc_engine_id << GUC_ELC_ENGINE_OFFSET);
 
 		lrc->ring_begin = guc_ggtt_offset(ce->ring->vma);
@@ -391,12 +396,12 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
 	desc->desc_private = (uintptr_t)client;
 }
 
-static void guc_ctx_desc_fini(struct intel_guc *guc,
-			      struct i915_guc_client *client)
+static void guc_stage_desc_fini(struct intel_guc *guc,
+				struct i915_guc_client *client)
 {
-	struct guc_context_desc *desc;
+	struct guc_stage_desc *desc;
 
-	desc = __get_context_desc(client);
+	desc = __get_stage_desc(client);
 	memset(desc, 0, sizeof(*desc));
 }
 
@@ -761,7 +766,7 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 		ret = __create_doorbell(client);
 		if (ret) {
 			DRM_ERROR("Couldn't recreate client %u doorbell: %d\n",
-				client->ctx_index, ret);
+				client->stage_id, ret);
 			return ret;
 		}
 	}
@@ -811,12 +816,12 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 	client->wq_size = GUC_WQ_SIZE;
 	spin_lock_init(&client->wq_lock);
 
-	ret = ida_simple_get(&guc->ctx_ids, 0, GUC_MAX_GPU_CONTEXTS,
+	ret = ida_simple_get(&guc->stage_ids, 0, GUC_MAX_STAGE_DESCRIPTORS,
 				GFP_KERNEL);
 	if (ret < 0)
 		goto err_client;
 
-	client->ctx_index = ret;
+	client->stage_id = ret;
 
 	/* The first page is doorbell/proc_desc. Two followed pages are wq. */
 	vma = intel_guc_allocate_vma(guc, GUC_DB_SIZE + GUC_WQ_SIZE);
@@ -848,14 +853,14 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 		client->proc_desc_offset = (GUC_DB_SIZE / 2);
 
 	guc_proc_desc_init(guc, client);
-	guc_ctx_desc_init(guc, client);
+	guc_stage_desc_init(guc, client);
 
 	ret = create_doorbell(client);
 	if (ret)
 		goto err_vaddr;
 
-	DRM_DEBUG_DRIVER("new priority %u client %p for engine(s) 0x%x: ctx_index %u\n",
-			 priority, client, client->engines, client->ctx_index);
+	DRM_DEBUG_DRIVER("new priority %u client %p for engine(s) 0x%x: stage_id %u\n",
+			 priority, client, client->engines, client->stage_id);
 	DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%lx\n",
 			 client->doorbell_id, client->doorbell_offset);
 
@@ -866,7 +871,7 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 err_vma:
 	i915_vma_unpin_and_release(&client->vma);
 err_id:
-	ida_simple_remove(&guc->ctx_ids, client->ctx_index);
+	ida_simple_remove(&guc->stage_ids, client->stage_id);
 err_client:
 	kfree(client);
 	return ERR_PTR(ret);
@@ -883,10 +888,10 @@ static void guc_client_free(struct i915_guc_client *client)
 	 * reset, so we cannot destroy the doorbell properly. Ignore the
 	 * error message for now */
 	destroy_doorbell(client);
-	guc_ctx_desc_fini(client->guc, client);
+	guc_stage_desc_fini(client->guc, client);
 	i915_gem_object_unpin_map(client->vma->obj);
 	i915_vma_unpin_and_release(&client->vma);
-	ida_simple_remove(&client->guc->ctx_ids, client->ctx_index);
+	ida_simple_remove(&client->guc->stage_ids, client->stage_id);
 	kfree(client);
 }
 
@@ -898,7 +903,7 @@ static void guc_policies_init(struct guc_policies *policies)
 	policies->dpc_promote_time = 500000;
 	policies->max_num_work_items = POLICY_MAX_NUM_WI;
 
-	for (p = 0; p < GUC_CTX_PRIORITY_NUM; p++) {
+	for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
 		for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
 			policy = &policies->policy[p][i];
 
@@ -992,8 +997,8 @@ static void guc_addon_destroy(struct intel_guc *guc)
  */
 int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 {
-	const size_t ctxsize = sizeof(struct guc_context_desc);
-	const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize;
+	const size_t ctxsize = sizeof(struct guc_stage_desc);
+	const size_t poolsize = GUC_MAX_STAGE_DESCRIPTORS * ctxsize;
 	const size_t gemsize = round_up(poolsize, PAGE_SIZE);
 	struct intel_guc *guc = &dev_priv->guc;
 	struct i915_vma *vma;
@@ -1025,7 +1030,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	if (ret < 0)
 		goto err_log;
 
-	ida_init(&guc->ctx_ids);
+	ida_init(&guc->stage_ids);
 
 	return 0;
 
@@ -1042,7 +1047,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
 
-	ida_destroy(&guc->ctx_ids);
+	ida_destroy(&guc->stage_ids);
 	guc_addon_destroy(guc);
 	intel_guc_log_destroy(guc);
 	i915_gem_object_unpin_map(guc->ctx_pool->obj);
@@ -1080,7 +1085,7 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 	if (!client) {
 		client = guc_client_alloc(dev_priv,
 					  INTEL_INFO(dev_priv)->ring_mask,
-					  GUC_CTX_PRIORITY_KMD_NORMAL,
+					  GUC_CLIENT_PRIORITY_KMD_NORMAL,
 					  dev_priv->kernel_context);
 		if (IS_ERR(client)) {
 			DRM_ERROR("Failed to create GuC client for execbuf!\n");
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 4edf40f..d5955cf 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -26,14 +26,14 @@
 #define GFXCORE_FAMILY_GEN9		12
 #define GFXCORE_FAMILY_UNKNOWN		0x7fffffff
 
-#define GUC_CTX_PRIORITY_KMD_HIGH	0
-#define GUC_CTX_PRIORITY_HIGH		1
-#define GUC_CTX_PRIORITY_KMD_NORMAL	2
-#define GUC_CTX_PRIORITY_NORMAL		3
-#define GUC_CTX_PRIORITY_NUM		4
+#define GUC_CLIENT_PRIORITY_KMD_HIGH	0
+#define GUC_CLIENT_PRIORITY_HIGH	1
+#define GUC_CLIENT_PRIORITY_KMD_NORMAL	2
+#define GUC_CLIENT_PRIORITY_NORMAL	3
+#define GUC_CLIENT_PRIORITY_NUM		4
 
-#define GUC_MAX_GPU_CONTEXTS		1024
-#define	GUC_INVALID_CTX_ID		GUC_MAX_GPU_CONTEXTS
+#define GUC_MAX_STAGE_DESCRIPTORS	1024
+#define	GUC_INVALID_STAGE_ID		GUC_MAX_STAGE_DESCRIPTORS
 
 #define GUC_RENDER_ENGINE		0
 #define GUC_VIDEO_ENGINE		1
@@ -68,14 +68,14 @@
 #define GUC_DOORBELL_ENABLED		1
 #define GUC_DOORBELL_DISABLED		0
 
-#define GUC_CTX_DESC_ATTR_ACTIVE	(1 << 0)
-#define GUC_CTX_DESC_ATTR_PENDING_DB	(1 << 1)
-#define GUC_CTX_DESC_ATTR_KERNEL	(1 << 2)
-#define GUC_CTX_DESC_ATTR_PREEMPT	(1 << 3)
-#define GUC_CTX_DESC_ATTR_RESET		(1 << 4)
-#define GUC_CTX_DESC_ATTR_WQLOCKED	(1 << 5)
-#define GUC_CTX_DESC_ATTR_PCH		(1 << 6)
-#define GUC_CTX_DESC_ATTR_TERMINATED	(1 << 7)
+#define GUC_STAGE_DESC_ATTR_ACTIVE	(1 << 0)
+#define GUC_STAGE_DESC_ATTR_PENDING_DB	(1 << 1)
+#define GUC_STAGE_DESC_ATTR_KERNEL	(1 << 2)
+#define GUC_STAGE_DESC_ATTR_PREEMPT	(1 << 3)
+#define GUC_STAGE_DESC_ATTR_RESET	(1 << 4)
+#define GUC_STAGE_DESC_ATTR_WQLOCKED	(1 << 5)
+#define GUC_STAGE_DESC_ATTR_PCH		(1 << 6)
+#define GUC_STAGE_DESC_ATTR_TERMINATED	(1 << 7)
 
 /* The guc control data is 10 DWORDs */
 #define GUC_CTL_CTXINFO			0
@@ -256,7 +256,7 @@ struct guc_wq_item {
 } __packed;
 
 struct guc_process_desc {
-	u32 context_id;
+	u32 stage_id;
 	u64 db_base_addr;
 	u32 head;
 	u32 tail;
@@ -290,9 +290,9 @@ struct guc_execlist_context {
 } __packed;
 
 /*Context descriptor for communicating between uKernel and Driver*/
-struct guc_context_desc {
+struct guc_stage_desc {
 	u32 sched_common_area;
-	u32 context_id;
+	u32 stage_id;
 	u32 pas_id;
 	u8 engines_used;
 	u64 db_trigger_cpu;
@@ -359,7 +359,7 @@ struct guc_policy {
 } __packed;
 
 struct guc_policies {
-	struct guc_policy policy[GUC_CTX_PRIORITY_NUM][GUC_MAX_ENGINES_NUM];
+	struct guc_policy policy[GUC_CLIENT_PRIORITY_NUM][GUC_MAX_ENGINES_NUM];
 
 	/* In micro seconds. How much time to allow before DPC processing is
 	 * called back via interrupt (to prevent DPC queue drain starving).
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 649c4db..4042d5d 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -176,7 +176,7 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
 	/* If GuC submission is enabled, set up additional parameters here */
 	if (i915.enable_guc_submission) {
 		u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool);
-		u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
+		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
 
 		pgs >>= PAGE_SHIFT;
 		params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 011a9e0..2e9f0e0 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -72,7 +72,7 @@ struct i915_guc_client {
 
 	uint32_t engines;		/* bitmap of (host) engine ids	*/
 	uint32_t priority;
-	u32 ctx_index;
+	u32 stage_id;
 	uint32_t proc_desc_offset;
 
 	u16 doorbell_id;
@@ -157,7 +157,7 @@ struct intel_guc {
 	struct i915_vma *addon;
 	struct i915_vma *ctx_pool;
 	void *ctx_pool_vaddr;
-	struct ida ctx_ids;
+	struct ida stage_ids;
 
 	struct i915_guc_client *execbuf_client;
 
-- 
1.9.1

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

Re: [08/11] drm/i915/guc: Wait for doorbell to be inactive before deallocating

[Oscar Mateo]

On 03/13/2017 04:46 AM, Chris Wilson wrote:
> On Fri, Mar 10, 2017 at 08:28:55AM -0800, Oscar Mateo wrote:
>> Doorbell release flow requires that we wait for GEN8_DRB_VALID bit to go
>> to zero after updating db_status before we call the GuC to release the
>> doorbell.
> Does this fix some of the '110' errors?
> -Chris

No, AFAICT this doesn't fix anything (but it's required nonetheless).

The 110 errors (ENTER_S_STATE and DEALLOCATE_DOORBELL failed calls to 
GuC) are cause by us resetting the GuC right before we ask it to do 
something. E.g.: in the suspend path, we do 
i915_gem_suspend()->i915_gem_sanitize()->intel_gpu_reset() and then 
intel_guc_suspend() inmediately after. After the sanitize, the GuC is in 
reset state and without a firmware loaded, so asking it to suspend is 
pretty useless. Same thing when trying to orderly shutdown the doorbells 
right after reset (the GuC has no idea of what doorbell we are talking 
about).

For now, the simple fix would be to keep doing the reset and not ask the 
GuC to do anything after. But sooner or later, we won't be able to get 
away with this. E.g.: if we enable direct submission, the GuC is the 
only one that will know about all the requests in-flight, and therefore 
the only one that can decide when to suspend. Also, surely loading the 
GuC firmware every time is slowing down the resume path by a lot?

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

Re: [10/11] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor"

[Oscar Mateo]

On 03/13/2017 04:49 AM, Chris Wilson wrote:
> On Fri, Mar 10, 2017 at 08:28:57AM -0800, Oscar Mateo wrote:
>> A GuC context and a HW context are in no way related, so the name "GuC context descriptor"
>> is very unfortunate, because a new reader of the code gets overwhelmed very quickly with
>> a lot of things called "context" that refer to different things. We can improve legibility
>> a lot by simply renaming a few objects in the GuC code.
>>
>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
>> ---
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index a912ec4..e320008 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -35,16 +35,18 @@
>>    * descriptor and a workqueue (all of them inside a single gem object that
>>    * contains all required pages for these elements).
>>    *
>> - * GuC context descriptor:
>> - * During initialization, the driver allocates a static pool of 1024 such
>> - * descriptors, and shares them with the GuC.
>> + * GuC stage descriptor:
>> + * Technically, this is known as a "GuC Context" descriptor in the specs, but
>> + * we use the term "stage" to avoid confusion with all the other things already
>> + * named "context" in the driver. During initialization, the driver allocates
>> + * a static pool of 1024 such descriptors, and shares them with the GuC.
>>    * Currently, there exists a 1:1 mapping between a i915_guc_client and a
>> - * guc_context_desc (via the client's context_index), so effectively only
>> - * one guc_context_desc gets used. This context descriptor lets the GuC know
>> - * about the doorbell, workqueue and process descriptor. Theoretically, it also
>> - * lets the GuC know about our HW contexts (Context ID, etc...), but we actually
>> + * guc_stage_desc (via the client's stage_id), so effectively only one
>> + * gets used. This stage descriptor lets the GuC know about the doorbell,
>> + * workqueue and process descriptor. Theoretically, it also lets the GuC
>> + * know about our HW contexts (context ID, etc...), but we actually
>>    * employ a kind of submission where the GuC uses the LRCA sent via the work
>> - * item instead (the single guc_context_desc associated to execbuf client
>> + * item instead (the single guc_stage_desc associated to execbuf client
>>    * contains information about the default kernel context only, but this is
>>    * essentially unused). This is called a "proxy" submission.
> Some of the above (esp. "this is known as a "GuC Context" descriptor in
> the specs") would be good here. Some might say this is where we expect
> the kerneldoc for structs to be...
>
>>   /*Context descriptor for communicating between uKernel and Driver*/
>> -struct guc_context_desc {
>> +struct guc_stage_desc {
>>   	u32 sched_common_area;
>> -	u32 context_id;
>> +	u32 stage_id;
>>   	u32 pas_id;
>>   	u8 engines_used;
>>   	u64 db_trigger_cpu;
>> @@ -359,7 +359,7 @@ struct guc_policy {
>>   } __packed;
>>
Fair enough, I'll move it. Thanks,
-- Oscar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [08/11] drm/i915/guc: Wait for doorbell to be inactive before deallocating

[Chris Wilson]

On Fri, Mar 10, 2017 at 08:28:55AM -0800, Oscar Mateo wrote:
> Doorbell release flow requires that we wait for GEN8_DRB_VALID bit to go
> to zero after updating db_status before we call the GuC to release the
> doorbell.

Does this fix some of the '110' errors?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [10/11] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor"

[Chris Wilson]

On Fri, Mar 10, 2017 at 08:28:57AM -0800, Oscar Mateo wrote:
> A GuC context and a HW context are in no way related, so the name "GuC context descriptor"
> is very unfortunate, because a new reader of the code gets overwhelmed very quickly with
> a lot of things called "context" that refer to different things. We can improve legibility
> a lot by simply renaming a few objects in the GuC code.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index a912ec4..e320008 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -35,16 +35,18 @@
>   * descriptor and a workqueue (all of them inside a single gem object that
>   * contains all required pages for these elements).
>   *
> - * GuC context descriptor:
> - * During initialization, the driver allocates a static pool of 1024 such
> - * descriptors, and shares them with the GuC.
> + * GuC stage descriptor:
> + * Technically, this is known as a "GuC Context" descriptor in the specs, but
> + * we use the term "stage" to avoid confusion with all the other things already
> + * named "context" in the driver. During initialization, the driver allocates
> + * a static pool of 1024 such descriptors, and shares them with the GuC.
>   * Currently, there exists a 1:1 mapping between a i915_guc_client and a
> - * guc_context_desc (via the client's context_index), so effectively only
> - * one guc_context_desc gets used. This context descriptor lets the GuC know
> - * about the doorbell, workqueue and process descriptor. Theoretically, it also
> - * lets the GuC know about our HW contexts (Context ID, etc...), but we actually
> + * guc_stage_desc (via the client's stage_id), so effectively only one
> + * gets used. This stage descriptor lets the GuC know about the doorbell,
> + * workqueue and process descriptor. Theoretically, it also lets the GuC
> + * know about our HW contexts (context ID, etc...), but we actually
>   * employ a kind of submission where the GuC uses the LRCA sent via the work
> - * item instead (the single guc_context_desc associated to execbuf client
> + * item instead (the single guc_stage_desc associated to execbuf client
>   * contains information about the default kernel context only, but this is
>   * essentially unused). This is called a "proxy" submission.

Some of the above (esp. "this is known as a "GuC Context" descriptor in
the specs") would be good here. Some might say this is where we expect
the kerneldoc for structs to be...

>  /*Context descriptor for communicating between uKernel and Driver*/
> -struct guc_context_desc {
> +struct guc_stage_desc {
>  	u32 sched_common_area;
> -	u32 context_id;
> +	u32 stage_id;
>  	u32 pas_id;
>  	u8 engines_used;
>  	u64 db_trigger_cpu;
> @@ -359,7 +359,7 @@ struct guc_policy {
>  } __packed;

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [03/11 v3] drm/i915/guc: Add onion teardown to the GuC setup

[Arkadiusz Hiler]

On Fri, Mar 10, 2017 at 08:28:50AM -0800, Oscar Mateo wrote:
> Starting with intel_guc_loader, down to intel_guc_submission
> and finally to intel_guc_log.
> 
> v2:
>   - Null execbuf client outside guc_client_free (Daniele)
>   - Assert if things try to get allocated twice (Daniele/Joonas)
>   - Null guc->log.buf_addr when destroyed (Daniele)
>   - Newline between returning success and error labels (Joonas)
>   - Remove some unnecessary comments (Joonas)
>   - Keep guc_log_create_extras naming convention (Joonas)
>   - Helper function guc_log_has_extras (Joonas)
>   - No need for separate relay_channel create/destroy. It's just another extra.
>   - No need to nullify guc->log.flush_wq when destroyed (Joonas)
>   - Hoist the check for has_extras out of guc_log_create_extras (Joonas)
>   - Try to do i915_guc_log_register/unregister calls (kind of) symmetric (Daniele)
>   - Make sure initel_guc_fini is not called before init is ever called (Daniele)
> 
> v3:
>   - Remove unnecessary parenthesis (Joonas)
>   - Check for logs enabled on debugfs registration
>   - Rebase on top of Tvrtko's "Fix request re-submission after reset"
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c            |   7 +-
>  drivers/gpu/drm/i915/i915_guc_submission.c |  94 ++++----
>  drivers/gpu/drm/i915/intel_guc_loader.c    |  32 +--
>  drivers/gpu/drm/i915/intel_guc_log.c       | 362 ++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_uc.h            |   8 +-
>  5 files changed, 261 insertions(+), 242 deletions(-)
> 

<SNIP>

> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index b1ebfce..f9e183a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -443,7 +443,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  
>  	err = i915_guc_submission_init(dev_priv);
>  	if (err)
> -		goto fail;
> +		goto err_guc;
>  
>  	/*
>  	 * WaEnableuKernelHeaderValidFix:skl,bxt
> @@ -458,7 +458,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  		 */
>  		err = guc_hw_reset(dev_priv);
>  		if (err)
> -			goto fail;
> +			goto err_submission;
>  
>  		intel_huc_load(dev_priv);
>  		err = guc_ucode_xfer(dev_priv);
> @@ -466,7 +466,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  			break;
>  
>  		if (--retries == 0)
> -			goto fail;
> +			goto err_submission;
>  
>  		DRM_INFO("GuC fw load failed: %d; will reset and "
>  			 "retry %d more time(s)\n", err, retries);
> @@ -482,7 +482,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  
>  		err = i915_guc_submission_enable(dev_priv);
>  		if (err)
> -			goto fail;
> +			goto err_interrupts;
>  	}
>  
>  	DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
> @@ -492,15 +492,16 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  
>  	return 0;
>  
> +err_interrupts:
> +	gen9_disable_guc_interrupts(dev_priv);

It's no longer called from this place as of:

commit 7762ebb9a455db18eef5c366da5496fb38429d56
Author: Sagar Arun Kamble <sagar.a.kamble@intel.com>

drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable

Also, I did similar thing in my GuC series (for guc_loader.c only). It
should be easy to rebase one on top of the other depending which will
get there first.

Othere changes here LGTM

-- 
Cheers,
Arek


> +err_submission:
> +	i915_guc_submission_fini(dev_priv);
> +err_guc:
> +	i915_ggtt_disable_guc(dev_priv);
>  fail:
>  	if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
>  		guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
>  
> -	guc_interrupts_release(dev_priv);
> -	i915_guc_submission_disable(dev_priv);
> -	i915_guc_submission_fini(dev_priv);
> -	i915_ggtt_disable_guc(dev_priv);
> -
>  	/*
>  	 * We've failed to load the firmware :(
>  	 *
> @@ -540,6 +541,13 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  

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

Re: [01/11 v3] drm/i915/guc: Sanitize GuC client initialization

[Daniele Ceraolo Spurio]

On 10/03/17 08:28, Oscar Mateo wrote:
> Started adding proper teardown to guc_client_alloc, ended up removing
> quite a few dead ends where errors communicating with the GuC were
> silently ignored. There also seemed to be quite a few erronous
> teardown actions performed in case of an error (ordering wrong).
>
> v2:
>   - Increase function symmetry/proximity (Michal/Daniele)
>   - Fix __reserve_doorbell accounting for high priority (Daniele)
>   - Call __update_doorbell_desc! (Daniele)
>   - Isolate __guc_{,de}allocate_doorbell (Michal/Daniele)
>
> v3:
>   - "Select" a cacheline is a more accurate verb than "reserve" (Daniele).
>   - We cannot update & create the doorbell without reserving it first, so
>     move the whole doorbell creation for execbuf_client to the submission
>     enable (Oscar).i
>   - Add a fixme for ignoring possible doorbell destroy errors.
>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        |   4 +-
>  drivers/gpu/drm/i915/i915_guc_submission.c | 391 ++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_guc_fwif.h      |   4 +-
>  drivers/gpu/drm/i915/intel_uc.h            |  11 +-
>  4 files changed, 229 insertions(+), 181 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 56674df..c395ccf 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2469,7 +2469,7 @@ static void i915_guc_client_info(struct seq_file *m,
>
>  	seq_printf(m, "\tPriority %d, GuC ctx index: %u, PD offset 0x%x\n",
>  		client->priority, client->ctx_index, client->proc_desc_offset);
> -	seq_printf(m, "\tDoorbell id %d, offset: 0x%x, cookie 0x%x\n",
> +	seq_printf(m, "\tDoorbell id %d, offset: 0x%lx, cookie 0x%x\n",
>  		client->doorbell_id, client->doorbell_offset, client->doorbell_cookie);
>  	seq_printf(m, "\tWQ size %d, offset: 0x%x, tail %d\n",
>  		client->wq_size, client->wq_offset, client->wq_tail);
> @@ -2504,7 +2504,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
>  	}
>
>  	seq_printf(m, "Doorbell map:\n");
> -	seq_printf(m, "\t%*pb\n", GUC_MAX_DOORBELLS, guc->doorbell_bitmap);
> +	seq_printf(m, "\t%*pb\n", GUC_NUM_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);
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 41f2dd8..ceeb1fa 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -62,27 +62,73 @@
>   *
>   */
>
> +static inline bool is_high_priority(struct i915_guc_client* client)
> +{
> +	return client->priority <= GUC_CTX_PRIORITY_HIGH;
> +}
> +
> +static int __reserve_doorbell(struct i915_guc_client *client)
> +{
> +	unsigned long offset;
> +	unsigned long end;
> +	u16 id;
> +
> +	GEM_BUG_ON(client->doorbell_id != GUC_DOORBELL_INVALID);
> +
> +	/*
> +	 * 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?"

The comment about the test doesn't apply anymore so we can either remove 
it or move it in the is_high_priority() function.

> +	 */
> +
> +	offset = 0;
> +	end = GUC_NUM_DOORBELLS/2;
> +	if (is_high_priority(client)) {
> +		offset = end;
> +		end += offset;
> +        }
> +
> +	id = find_next_zero_bit(client->guc->doorbell_bitmap, offset, end);
> +	if (id == end)
> +		return -ENOSPC;
> +
> +	__set_bit(id, client->guc->doorbell_bitmap);
> +	client->doorbell_id = id;
> +	DRM_DEBUG_DRIVER("client %u (high prio=%s) reserved doorbell %d: \n",

"%d:" -> ": %d" ?

> +			 client->ctx_index, yesno(is_high_priority(client)),
> +			 id);
> +	return 0;
> +}
> +
> +static void __unreserve_doorbell(struct i915_guc_client *client)
> +{
> +	GEM_BUG_ON(client->doorbell_id == GUC_DOORBELL_INVALID);
> +
> +	__clear_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> +	client->doorbell_id = GUC_DOORBELL_INVALID;
> +}
> +
>  /*
>   * Tell the GuC to allocate or deallocate a specific doorbell
>   */
>
> -static int guc_allocate_doorbell(struct intel_guc *guc,
> -				 struct i915_guc_client *client)
> +static int __guc_allocate_doorbell(struct intel_guc *guc, u32 ctx_index)
>  {
>  	u32 action[] = {
>  		INTEL_GUC_ACTION_ALLOCATE_DOORBELL,
> -		client->ctx_index
> +		ctx_index
>  	};
>
>  	return intel_guc_send(guc, action, ARRAY_SIZE(action));
>  }
>
> -static int guc_release_doorbell(struct intel_guc *guc,
> -				struct i915_guc_client *client)
> +static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
>  {
>  	u32 action[] = {
>  		INTEL_GUC_ACTION_DEALLOCATE_DOORBELL,
> -		client->ctx_index
> +		ctx_index
>  	};
>
>  	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> @@ -95,104 +141,100 @@ static int guc_release_doorbell(struct intel_guc *guc,
>   * client object which contains the page being used for the doorbell
>   */
>
> -static int guc_update_doorbell_id(struct intel_guc *guc,
> -				  struct i915_guc_client *client,
> -				  u16 new_id)
> +static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
>  {
> -	struct sg_table *sg = guc->ctx_pool_vma->pages;
> -	void *doorbell_bitmap = guc->doorbell_bitmap;
> -	struct guc_doorbell_info *doorbell;
> +	struct sg_table *sg = client->guc->ctx_pool_vma->pages;
>  	struct guc_context_desc desc;
>  	size_t len;
>
> -	doorbell = client->vaddr + client->doorbell_offset;
> -
> -	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)guc_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);
> +				 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);
> +				   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;
> +	return 0;
> +}
>
> -	/* Activate the new doorbell */
> -	__set_bit(new_id, doorbell_bitmap);
> +static struct guc_doorbell_info *__get_doorbell(struct i915_guc_client *client)

from what I can see this could also be used from guc_ring_doorbell and 
possibly from guc_ctx_desc_init

> +{
> +	return client->vaddr + client->doorbell_offset;

Not introduced by this patch, but arithmetic on void pointers doesn't 
really look clean IMO. Can we cast the pointer to uintptr_t or char* or 
whatever else is appropriate before incrementing it? I'm happy for it to 
be done in a follow up.

> +}
> +
> +static bool has_doorbell(struct i915_guc_client *client)
> +{
> +	if (client->doorbell_id == GUC_DOORBELL_INVALID)
> +		return false;
> +
> +	return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> +}
> +
> +static int __create_doorbell(struct i915_guc_client *client)
> +{
> +	struct guc_doorbell_info *doorbell;
> +	int err;
> +
> +	doorbell = __get_doorbell(client);
>  	doorbell->db_status = GUC_DOORBELL_ENABLED;
>  	doorbell->cookie = client->doorbell_cookie;
> -	return guc_allocate_doorbell(guc, client);
> +
> +	err = __guc_allocate_doorbell(client->guc, client->ctx_index);
> +	if (err) {
> +		doorbell->db_status = GUC_DOORBELL_DISABLED;
> +		doorbell->cookie = 0;
> +	}
> +	return err;
>  }
>
> -static void guc_disable_doorbell(struct intel_guc *guc,
> -				 struct i915_guc_client *client)
> +static int __destroy_doorbell(struct i915_guc_client *client)
>  {
> -	(void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
> +	struct guc_doorbell_info *doorbell;
>
> -	/* XXX: wait for any interrupts */
> -	/* XXX: wait for workqueue to drain */
> +	doorbell = __get_doorbell(client);
> +	doorbell->db_status = GUC_DOORBELL_DISABLED;
> +	doorbell->cookie = 0;
> +
> +	return __guc_deallocate_doorbell(client->guc, client->ctx_index);
>  }
>
> -static uint16_t
> -select_doorbell_register(struct intel_guc *guc, uint32_t priority)
> +static int destroy_doorbell(struct i915_guc_client *client)
>  {
> -	/*
> -	 * 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;
> +	int err;
>
> -	id = find_next_zero_bit(guc->doorbell_bitmap, end, start);
> -	if (id == end)
> -		id = GUC_INVALID_DOORBELL_ID;
> +	GEM_BUG_ON(!has_doorbell(client));
>
> -	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
> -			hi_pri ? "high" : "normal", id);
> +	/* XXX: wait for any interrupts */
> +	/* XXX: wait for workqueue to drain */
>
> -	return id;
> -}
> +	err = __destroy_doorbell(client);
> +	if (err)
> +		return err;
>
> -/*
> - * Select, assign and relase doorbell cachelines
> - *
> - * These functions track which doorbell cachelines are in use.
> - * The data they manipulate is protected by the intel_guc_send lock.
> - */
> +	__update_doorbell_desc(client, GUC_DOORBELL_INVALID);
> +
> +	__unreserve_doorbell(client);
>
> -static uint32_t select_doorbell_cacheline(struct intel_guc *guc)
> +	return 0;
> +}
> +
> +static unsigned long __select_cacheline(struct intel_guc* guc)
>  {
> -	const uint32_t cacheline_size = cache_line_size();
> -	uint32_t offset;
> +	unsigned long offset;
>
>  	/* Doorbell uses a single cache line within a page */
>  	offset = offset_in_page(guc->db_cacheline);
>
>  	/* Moving to next cache line to reduce contention */
> -	guc->db_cacheline += cacheline_size;
> -
> -	DRM_DEBUG_DRIVER("selected doorbell cacheline 0x%x, next 0x%x, linesize %u\n",
> -			offset, guc->db_cacheline, cacheline_size);
> +	guc->db_cacheline += cache_line_size();
>
> +	DRM_DEBUG_DRIVER("reserved cacheline 0x%lx, next 0x%x, linesize %u\n",
> +			offset, guc->db_cacheline, cache_line_size());
>  	return offset;
>  }
>
> @@ -594,93 +636,96 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>  	return vma;
>  }
>
> -static void
> -guc_client_free(struct drm_i915_private *dev_priv,
> -		struct i915_guc_client *client)
> +static void guc_client_free(struct i915_guc_client *client)
>  {
> -	struct intel_guc *guc = &dev_priv->guc;
> -
> -	if (!client)
> -		return;
> -
>  	/*
>  	 * XXX: wait for any outstanding submissions before freeing memory.
>  	 * Be sure to drop any locks
>  	 */
> -
> -	if (client->vaddr) {
> -		/*
> -		 * If we got as far as setting up a doorbell, make sure we
> -		 * shut it down before unmapping & deallocating the memory.
> -		 */
> -		guc_disable_doorbell(guc, client);
> -
> -		i915_gem_object_unpin_map(client->vma->obj);
> -	}
> -
> +	guc_ctx_desc_fini(client->guc, client);
> +	i915_gem_object_unpin_map(client->vma->obj);
>  	i915_vma_unpin_and_release(&client->vma);
> -
> -	if (client->ctx_index != GUC_INVALID_CTX_ID) {
> -		guc_ctx_desc_fini(guc, client);
> -		ida_simple_remove(&guc->ctx_ids, client->ctx_index);
> -	}
> -
> +	ida_simple_remove(&client->guc->ctx_ids, client->ctx_index);
>  	kfree(client);
>  }
>
>  /* Check that a doorbell register is in the expected state */
> -static bool guc_doorbell_check(struct intel_guc *guc, uint16_t db_id)
> +static bool doorbell_ok(struct intel_guc *guc, u16 db_id)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	i915_reg_t drbreg = GEN8_DRBREGL(db_id);
> -	uint32_t value = I915_READ(drbreg);
> -	bool enabled = (value & GUC_DOORBELL_ENABLED) != 0;
> -	bool expected = test_bit(db_id, guc->doorbell_bitmap);
> +	u32 drbregl;
> +	bool valid;
>
> -	if (enabled == expected)
> +	GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID);
> +
> +	drbregl = I915_READ(GEN8_DRBREGL(db_id));
> +	valid = drbregl & GEN8_DRB_VALID;
> +
> +	if (test_bit(db_id, guc->doorbell_bitmap) == valid)
>  		return true;
>
> -	DRM_DEBUG_DRIVER("Doorbell %d (reg 0x%x) 0x%x, should be %s\n",
> -			 db_id, drbreg.reg, value,
> -			 expected ? "active" : "inactive");
> +	DRM_DEBUG_DRIVER("Doorbell %d has unexpected state (0x%x): valid=%s\n",
> +			 db_id, drbregl, yesno(valid));
>
>  	return false;
>  }
>
> +static int __reset_doorbell(struct i915_guc_client* client, u16 db_id)

This reset only works if the GuC FW thinks that the doorbell is 
unassigned, otherwise the ALLOCATE_DOORBELL action will fail. We 
currently only use it to reset the doorbell HW status after resetting 
GuC and reloading the FW so it is ok, but we could use a comment to 
explain the limitation of this reset.

The patch looks functionally ok, so with the minor comments addressed:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

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

Re: [06/11 v3] drm/i915/guc: Make intel_guc_send a function pointer

[Daniele Ceraolo Spurio]

On 10/03/17 08:28, Oscar Mateo wrote:
> Prepare for an alternate GuC communication interface.
>
> v2: Make a few functions static and name them correctly while we are at it (Oscar), but
> leave an intel_guc_send_mmio interface for users that require old-style communication.
>
> v3: Send intel_uc_init_early back to the top (Michal).
>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

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

Re: [07/11] drm/i915/guc: Improve the GuC documentation & comments about proxy submissions

[Daniele Ceraolo Spurio]

On 10/03/17 08:28, Oscar Mateo wrote:
> While at it, fix a typo (s/ring_lcra/ring_lrca) and improve the naming of one
> firware interface field (s/ring_tail/submit_element_info, since it can contain
> more than just the ring tail).
>
> No change in functionality.
>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 42 +++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_guc_fwif.h      |  4 +--
>  2 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 996e9f3..4e82be6 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -28,16 +28,25 @@
>  /**
>   * DOC: GuC-based command submission
>   *
> - * i915_guc_client:
> - * We use the term client to avoid confusion with contexts. A i915_guc_client is
> - * equivalent to GuC object guc_context_desc. This context descriptor is
> - * allocated from a pool of 1024 entries. Kernel driver will allocate doorbell
> - * and workqueue for it. Also the process descriptor (guc_process_desc), which
> - * is mapped to client space. So the client can write Work Item then ring the
> - * doorbell.
> + * GuC client:
> + * A i915_guc_client refers to a unique user of the GuC. Currently, there is

After re-reading this I think that matching a client with a "unique 
user" is slightly misleading because i915 can have multiple 
i915_guc_client objects but it still counts as a single user. Maybe we 
could define the client as a "submission path" through GuC (a set of 
work-queue, doorbell, priority) or something similar.

It might be also worth keeping the statement about the term "client" 
being introduced by us to reduce confusion.

Thanks,
Daniele

> + * only one of these (the execbuf_client) and this one is charged with all
> + * submissions to the GuC. This struct is the owner of a doorbell, a process
> + * descriptor and a workqueue (all of them inside a single gem object that
> + * contains all required pages for these elements).
>   *
> - * To simplify the implementation, we allocate one gem object that contains all
> - * pages for doorbell, process descriptor and workqueue.
> + * GuC context descriptor:
> + * During initialization, the driver allocates a static pool of 1024 such
> + * descriptors, and shares them with the GuC.
> + * Currently, there exists a 1:1 mapping between a i915_guc_client and a
> + * guc_context_desc (via the client's context_index), so effectively only
> + * one guc_context_desc gets used. This context descriptor lets the GuC know
> + * about the doorbell, workqueue and process descriptor. Theoretically, it also
> + * lets the GuC know about our HW contexts (Context ID, etc...), but we actually
> + * employ a kind of submission where the GuC uses the LRCA sent via the work
> + * item instead (the single guc_context_desc associated to execbuf client
> + * contains information about the default kernel context only, but this is
> + * essentially unused). This is called a "proxy" submission.
>   *
>   * The Scratch registers:
>   * There are 16 MMIO-based registers start from 0xC180. The kernel driver writes
> @@ -308,10 +317,17 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
>  		if (!ce->state)
>  			break;	/* XXX: continue? */
>
> +		/*
> +		 * XXX: When this is a GUC_CTX_DESC_ATTR_KERNEL client (proxy
> +		 * submission or, in other words, not using a direct submission
> +		 * model) the KMD's LRCA is not used for any work submission.
> +		 * Instead, the GuC uses the LRCA of the user mode context (see
> +		 * guc_wq_item_append below).
> +		 */
>  		lrc->context_desc = lower_32_bits(ce->lrc_desc);
>
>  		/* The state page is after PPHWSP */
> -		lrc->ring_lcra =
> +		lrc->ring_lrca =
>  			guc_ggtt_offset(ce->state) + LRC_STATE_PN * PAGE_SIZE;
>  		lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
>  				(guc_engine_id << GUC_ELC_ENGINE_OFFSET);
> @@ -342,10 +358,6 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
>  	desc->wq_addr = gfx_addr + client->wq_offset;
>  	desc->wq_size = client->wq_size;
>
> -	/*
> -	 * XXX: Take LRCs from an existing context if this is not an
> -	 * IsKMDCreatedContext client
> -	 */
>  	desc->desc_private = (uintptr_t)client;
>  }
>
> @@ -469,7 +481,7 @@ static void guc_wq_item_append(struct i915_guc_client *client,
>  	/* The GuC wants only the low-order word of the context descriptor */
>  	wqi->context_desc = (u32)intel_lr_context_descriptor(rq->ctx, engine);
>
> -	wqi->ring_tail = tail << WQ_RING_TAIL_SHIFT;
> +	wqi->submit_element_info = tail << WQ_RING_TAIL_SHIFT;
>  	wqi->fence_id = rq->global_seqno;
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 3ae8cef..4edf40f 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -251,7 +251,7 @@ struct guc_doorbell_info {
>  struct guc_wq_item {
>  	u32 header;
>  	u32 context_desc;
> -	u32 ring_tail;
> +	u32 submit_element_info;
>  	u32 fence_id;
>  } __packed;
>
> @@ -278,7 +278,7 @@ struct guc_execlist_context {
>  	u32 context_desc;
>  	u32 context_id;
>  	u32 ring_status;
> -	u32 ring_lcra;
> +	u32 ring_lrca;
>  	u32 ring_begin;
>  	u32 ring_end;
>  	u32 ring_next_free_location;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [08/11] drm/i915/guc: Wait for doorbell to be inactive before deallocating

[Daniele Ceraolo Spurio]

On 13/03/17 01:56, Oscar Mateo wrote:
>
>
> On 03/13/2017 04:46 AM, Chris Wilson wrote:
>> On Fri, Mar 10, 2017 at 08:28:55AM -0800, Oscar Mateo wrote:
>>> Doorbell release flow requires that we wait for GEN8_DRB_VALID bit to go
>>> to zero after updating db_status before we call the GuC to release the
>>> doorbell.
>> Does this fix some of the '110' errors?
>> -Chris
>
> No, AFAICT this doesn't fix anything (but it's required nonetheless).
>
> The 110 errors (ENTER_S_STATE and DEALLOCATE_DOORBELL failed calls to
> GuC) are cause by us resetting the GuC right before we ask it to do
> something. E.g.: in the suspend path, we do
> i915_gem_suspend()->i915_gem_sanitize()->intel_gpu_reset() and then
> intel_guc_suspend() inmediately after. After the sanitize, the GuC is in
> reset state and without a firmware loaded, so asking it to suspend is
> pretty useless. Same thing when trying to orderly shutdown the doorbells
> right after reset (the GuC has no idea of what doorbell we are talking
> about).
>
> For now, the simple fix would be to keep doing the reset and not ask the
> GuC to do anything after. But sooner or later, we won't be able to get
> away with this. E.g.: if we enable direct submission, the GuC is the
> only one that will know about all the requests in-flight, and therefore
> the only one that can decide when to suspend. Also, surely loading the
> GuC firmware every time is slowing down the resume path by a lot?
>
> -- Oscar

To add a bit of info, the wait is required because if there are pending 
rings on the doorbell the HW may require a slightly longer time to 
disable it after we update the status. If we call into GuC before the 
disabling is completed GuC will return an error because the doorbell 
will still be active from its point of view, even if it is in the 
process of shutting down. I think that with our current flow it should 
be impossible to hit this situation so it as Oscar said it doesn't fix 
anything, but it is still worth adding it to avoid issues in the future.

In regards to the reset, I agree that we should either reorder stuff to 
make sure that the H2G messages are sent before the reset or drop the 
messages entirely.
In the particular case of suspend, it is not guaranteed that the GuC 
will survive a suspend/resume cycle (it depends on the depth of the 
sleep state). That is why the ENTER_S_STATE message exists: this message 
tells GuC to save all the information it needs in the buffer we 
provided; we can then tell GuC to restore this state with the 
EXIT_S_STATE message, which can be used on a freshly loaded FW as well.
With our current flow the GuC queues should be empty when we go to 
suspend so it shouldn't matter, but direct submission will need this to 
work.

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

Re: [03/11 v3] drm/i915/guc: Add onion teardown to the GuC setup

[Joonas Lahtinen]

On pe, 2017-03-10 at 08:28 -0800, Oscar Mateo wrote:
> Starting with intel_guc_loader, down to intel_guc_submission
> and finally to intel_guc_log.
> 
> v2:
>   - Null execbuf client outside guc_client_free (Daniele)
>   - Assert if things try to get allocated twice (Daniele/Joonas)
>   - Null guc->log.buf_addr when destroyed (Daniele)
>   - Newline between returning success and error labels (Joonas)
>   - Remove some unnecessary comments (Joonas)
>   - Keep guc_log_create_extras naming convention (Joonas)
>   - Helper function guc_log_has_extras (Joonas)
>   - No need for separate relay_channel create/destroy. It's just another extra.
>   - No need to nullify guc->log.flush_wq when destroyed (Joonas)
>   - Hoist the check for has_extras out of guc_log_create_extras (Joonas)
>   - Try to do i915_guc_log_register/unregister calls (kind of) symmetric (Daniele)
>   - Make sure initel_guc_fini is not called before init is ever called (Daniele)
> 
> v3:
>   - Remove unnecessary parenthesis (Joonas)
>   - Check for logs enabled on debugfs registration
>   - Rebase on top of Tvrtko's "Fix request re-submission after reset"
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

Drop comments around the interrupt enabling/disabling that log is
currently the only interrupt user.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [05/11 v2] drm/i915/guc: Break out the GuC log extras into their own "runtime" struct

[Joonas Lahtinen]

On pe, 2017-03-10 at 08:28 -0800, Oscar Mateo wrote:
> When initializing the GuC log struct, there is an object we need to
> allocate always, since the GuC needs its address at fw load time.
> The rest is only needed during runtime, in the sense that we only
> create if we actually enable GuC logging. Make that distinction
> explicit by subdividing further the intel_guc_log struct.
> 
> v2: Call the new struct "runtime", instead of "extras" (Joonas).
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

<SNIP>

> @@ -416,9 +416,9 @@ static int guc_log_create_extras(struct intel_guc *guc)
>  	 * or scheduled later on resume. This way the handling of work
>  	 * item can be kept same between system suspend & rpm suspend.
>  	 */
> -	guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
> +	guc->log.runtime.flush_wq = alloc_ordered_workqueue("i915-guc_log",
>  						    WQ_HIGHPRI | WQ_FREEZABLE);

Check indent here.

I see this is as a major improvement in readability;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

In the future, we might even refactor into guc_log_enable /
guc_log_disable :)

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [08/11] drm/i915/guc: Wait for doorbell to be inactive before deallocating

[Joonas Lahtinen]

On pe, 2017-03-10 at 08:28 -0800, Oscar Mateo wrote:
> Doorbell release flow requires that we wait for GEN8_DRB_VALID bit to go
> to zero after updating db_status before we call the GuC to release the
> doorbell.
> 
> Kudos to Daniele for finding this out.
> 
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

<SNIP>
 
>  static int __destroy_doorbell(struct i915_guc_client *client)
>  {
> +	struct drm_i915_private *dev_priv = guc_to_i915(client->guc);
>  	struct guc_doorbell_info *doorbell;
> +	u16 db_id = client->doorbell_id;
> +
> +	GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID);
>  
>  	doorbell = __get_doorbell(client);
>  	doorbell->db_status = GUC_DOORBELL_DISABLED;
>  	doorbell->cookie = 0;
>  
> +	/* Doorbell release flow requires that we wait for GEN8_DRB_VALID bit
> +	 * to go to zero after updating db_status before we call the GuC to
> +	 * release the doorbell */
> +	if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID), 10))
> +		DRM_ERROR("Doorbell never became invalid after disable\n");

Would the appropriate action be to return -ETIMEOUT and not proceed
with deallocation? I would at least WARN here. If GuC submission gets
enabled, we should yell if it's not working. So only the enable stage
should bail out with DRM_ERRORs and without WARN.

> +
>  	return __guc_deallocate_doorbell(client->guc, client->ctx_index);
>  }

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [09/11] drm/i915/guc: A little bit more of doorbell sanitization

[Joonas Lahtinen]

On pe, 2017-03-10 at 08:28 -0800, Oscar Mateo wrote:
> Some recent refactoring patches have left the doorbell creation outside
> the GuC client allocation, which does not make a lot of sense (a client
> without a doorbell is something useless). Move it back there, and
> refactor the init_doorbell_hw consequently.
> 
> Thanks to this, we can do some other improvements, like hoisting the
> check for GuC submission enabled out of the enable function.
> 
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

Looks good.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx