[RFC/CI] drm/i915: Sanitize GuC client initialization

[RFC/CI] drm/i915: Sanitize GuC client initialization

[Joonas Lahtinen]

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).

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: Oscar Mateo <oscar.mateo@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |   4 +-
 drivers/gpu/drm/i915/i915_guc_submission.c | 384 +++++++++++++++++------------
 drivers/gpu/drm/i915/intel_guc_fwif.h      |   4 +-
 drivers/gpu/drm/i915/intel_uc.h            |   9 +-
 4 files changed, 230 insertions(+), 171 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1ccc297..86c6f831 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2480,7 +2480,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);
@@ -2515,7 +2515,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 8ced9e2..d0c7004 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -62,30 +62,79 @@
  *
  */
 
+static inline bool is_high_priority(struct i915_guc_client* client)
+{
+	return client->priority <= GUC_CTX_PRIORITY_HIGH;
+}
+
+static bool __test_doorbell(struct i915_guc_client *client)
+{
+	return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
+}
+
+static void __release_doorbell(struct i915_guc_client *client)
+{
+	GEM_BUG_ON(client->doorbell_id == GUC_DOORBELL_INVALID);
+
+	__clear_bit(client->doorbell_id, client->guc->doorbell_bitmap);
+}
+
+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;
+	if (is_high_priority(client))
+		offset = GUC_NUM_DOORBELLS/2;
+
+	end = GUC_NUM_DOORBELLS - 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;
+}
+
 /*
  * 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 __create_doorbell_hw(struct i915_guc_client *client)
 {
 	u32 action[] = {
 		INTEL_GUC_ACTION_ALLOCATE_DOORBELL,
 		client->ctx_index
 	};
 
-	return intel_guc_send(guc, action, ARRAY_SIZE(action));
+	return intel_guc_send(client->guc, action, ARRAY_SIZE(action));
 }
 
-static int guc_release_doorbell(struct intel_guc *guc,
-				struct i915_guc_client *client)
+static int __destroy_doorbell_hw(struct i915_guc_client *client)
 {
 	u32 action[] = {
 		INTEL_GUC_ACTION_DEALLOCATE_DOORBELL,
 		client->ctx_index
 	};
 
-	return intel_guc_send(guc, action, ARRAY_SIZE(action));
+	return intel_guc_send(client->guc, action, ARRAY_SIZE(action));
 }
 
 /*
@@ -95,104 +144,114 @@ 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(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;
+}
+
+static bool has_doorbell(struct i915_guc_client *client)
+{
+	if (client->doorbell_id == GUC_DOORBELL_INVALID)
+		return false;
+
+	return __test_doorbell(client);
+}
 
-	/* 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 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 = __create_doorbell_hw(client);
+	if (err) {
+		doorbell->db_status = GUC_DOORBELL_DISABLED;
+		doorbell->cookie = 0;
+		__release_doorbell(client);
+	}
+	return err;
 }
 
-static void guc_disable_doorbell(struct intel_guc *guc,
-				 struct i915_guc_client *client)
+/*
+static int create_doorbell(struct i915_guc_client *client)
 {
-	(void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
+	int err;
 
-	/* XXX: wait for any interrupts */
-	/* XXX: wait for workqueue to drain */
+	GEM_BUG_ON(has_doorbell(client));
+
+	err = __reserve_doorbell(client);
+	if (err)
+		return err;
+
+	return __create_doorbell(client);
 }
+*/
 
-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;
-
-	id = find_next_zero_bit(guc->doorbell_bitmap, end, start);
-	if (id == end)
-		id = GUC_INVALID_DOORBELL_ID;
+	struct guc_doorbell_info *doorbell;
 
-	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
-			hi_pri ? "high" : "normal", id);
+	doorbell = __get_doorbell(client);
+	doorbell->db_status = GUC_DOORBELL_DISABLED;
+	doorbell->cookie = 0;
 
-	return id;
+	return __destroy_doorbell_hw(client);
 }
 
-/*
- * 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.
- */
+static int destroy_doorbell(struct i915_guc_client *client)
+{
+	int err;
+
+	GEM_BUG_ON(!has_doorbell(client));
 
-static uint32_t select_doorbell_cacheline(struct intel_guc *guc)
+	/* XXX: wait for any interrupts */
+	/* XXX: wait for workqueue to drain */
+
+	err = __destroy_doorbell(client);
+	if (err)
+		return err;
+
+	__release_doorbell(client);
+
+	return 0;
+}
+
+static unsigned long __reserve_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;
 }
 
@@ -584,93 +643,81 @@ 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);
-	}
-
+	WARN_ON(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);
-
-	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, u8 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);
 
-	if (enabled == expected)
+	u32 drbregl = I915_READ(GEN8_DRBREGL(db_id));
+
+	bool 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(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);
-
-	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);
+	for (i = 0; i < GUC_NUM_DOORBELLS; ++i)
+		WARN_ON(!doorbell_ok(guc, i));
+
+	return 0;
 }
 
 /**
@@ -696,49 +743,52 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
 	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)
+	ret = __reserve_doorbell(client);
+	if (ret)
 		/* XXX: evict a doorbell instead? */
-		goto err;
+		goto err_vaddr;
 
-	client->doorbell_offset = select_doorbell_cacheline(guc);
+	client->doorbell_offset = __reserve_cacheline(guc);
 
 	/*
 	 * Since the doorbell only requires a single cacheline, we can save
@@ -753,26 +803,31 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
 	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);
-	 */
+	/* For runtime client allocation we need to enable the doorbell. */
+	ret = __create_doorbell(client);
+	if (ret)
+		goto err_db;
 
 	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:
-	guc_client_free(dev_priv, client);
-	return NULL;
-}
-
+err_db:
+	__release_doorbell(client);
+err_vaddr:
+	i915_gem_object_unpin_map(client->vma->obj);
+err_vma:
+	i915_vma_unpin_and_release(&client->vma);
+err_id:
+	ida_simple_remove(&guc->ctx_ids, client->ctx_index);
+err_client:
+	kfree(client);
 
+	return ERR_PTR(ret);
+}
 
 static void guc_policies_init(struct guc_policies *policies)
 {
@@ -881,7 +936,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)
@@ -932,14 +987,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) {
@@ -978,7 +1038,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	if (!client)
 		return;
 
-	guc_client_free(dev_priv, client);
+	guc_client_free(client);
 
 	i915_vma_unpin_and_release(&guc->ads_vma);
 	i915_vma_unpin_and_release(&guc->log.vma);
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 @@ union guc_doorbell_qw {
 	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..3cbf853 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -75,10 +75,9 @@ struct i915_guc_client {
 	uint32_t 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 */
-- 
2.7.4

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

✓ Fi.CI.BAT: success for drm/i915: Sanitize GuC client initialization

[Patchwork]

== Series Details ==

Series: drm/i915: Sanitize GuC client initialization
URL   : https://patchwork.freedesktop.org/series/19452/
State : success

== Summary ==

Series 19452v1 drm/i915: Sanitize GuC client initialization
https://patchwork.freedesktop.org/api/1.0/series/19452/revisions/1/mbox/

Test pm_rpm:
        Subgroup basic-pci-d3-state:
                fail       -> PASS       (fi-kbl-7500u)
        Subgroup basic-rte:
                fail       -> PASS       (fi-kbl-7500u)

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

c46c80c1b1cdc18ed3d44ce289032946b48454c6 drm-tip: 2017y-02m-10d-09h-45m-22s UTC integration manifest
0232192 drm/i915: Sanitize GuC client initialization

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3765/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC/CI] drm/i915: Sanitize GuC client initialization

[Chris Wilson]

On Fri, Feb 10, 2017 at 03:30:10PM +0200, Joonas Lahtinen wrote:
> +static unsigned long __reserve_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);

But unsigned long? I was trying to work out why we might need unsigned
long doorbell_offset, for when the doorbell object is of known size.

That's the only thing that stuck out like a sore thumb.
-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: [RFC/CI] drm/i915: Sanitize GuC client initialization

[Michal Wajdeczko]

On Fri, Feb 10, 2017 at 03:30:10PM +0200, Joonas Lahtinen 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).
> 
> 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: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        |   4 +-
>  drivers/gpu/drm/i915/i915_guc_submission.c | 384 +++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_guc_fwif.h      |   4 +-
>  drivers/gpu/drm/i915/intel_uc.h            |   9 +-
>  4 files changed, 230 insertions(+), 171 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1ccc297..86c6f831 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2480,7 +2480,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);
> @@ -2515,7 +2515,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 8ced9e2..d0c7004 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -62,30 +62,79 @@
>   *
>   */
>  
> +static inline bool is_high_priority(struct i915_guc_client* client)
> +{
> +	return client->priority <= GUC_CTX_PRIORITY_HIGH;
> +}
> +
> +static bool __test_doorbell(struct i915_guc_client *client)

explicit inline ? or you want to let gcc decide ?

> +{
> +	return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> +}
> +
> +static void __release_doorbell(struct i915_guc_client *client)
> +{
> +	GEM_BUG_ON(client->doorbell_id == GUC_DOORBELL_INVALID);
> +
> +	__clear_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> +}
> +
> +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;
> +	if (is_high_priority(client))
> +		offset = GUC_NUM_DOORBELLS/2;
> +
> +	end = GUC_NUM_DOORBELLS - 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;
> +}
> +
>  /*
>   * 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 __create_doorbell_hw(struct i915_guc_client *client)

I would rather prefer to only change signature of this function into

	static int guc_allocate_doorbell(struct intel_guc *guc, u32 index)

as a clean wrap around GUC_ACTION_ALLOCATE_DOORBELL. This way we also preserve
consistency between function name and the guc action name used inside.

Based on the above we can still add 

	static int __create_doorbell_hw(struct i915_guc_client *client)
	{
		return guc_allocate_doorbell(client->guc, client->ctx_index);
	}

Note that location of the ctx_index member may change in the future, and this
approach will minimize impact of these future changes.


>  {
>  	u32 action[] = {
>  		INTEL_GUC_ACTION_ALLOCATE_DOORBELL,
>  		client->ctx_index
>  	};
>  
> -	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> +	return intel_guc_send(client->guc, action, ARRAY_SIZE(action));
>  }
>  
> -static int guc_release_doorbell(struct intel_guc *guc,
> -				struct i915_guc_client *client)
> +static int __destroy_doorbell_hw(struct i915_guc_client *client)

Same comment here.

>  {
>  	u32 action[] = {
>  		INTEL_GUC_ACTION_DEALLOCATE_DOORBELL,
>  		client->ctx_index
>  	};
>  
> -	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> +	return intel_guc_send(client->guc, action, ARRAY_SIZE(action));
>  }
>  
>  /*
> @@ -95,104 +144,114 @@ 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(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;
> +}
> +
> +static bool has_doorbell(struct i915_guc_client *client)
> +{
> +	if (client->doorbell_id == GUC_DOORBELL_INVALID)
> +		return false;
> +
> +	return __test_doorbell(client);
> +}

Can we keep related inline helpers together ?


>  
> -	/* 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 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 = __create_doorbell_hw(client);
> +	if (err) {
> +		doorbell->db_status = GUC_DOORBELL_DISABLED;
> +		doorbell->cookie = 0;
> +		__release_doorbell(client);
> +	}
> +	return err;
>  }
>  
> -static void guc_disable_doorbell(struct intel_guc *guc,
> -				 struct i915_guc_client *client)
> +/*
> +static int create_doorbell(struct i915_guc_client *client)
>  {
> -	(void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
> +	int err;
>  
> -	/* XXX: wait for any interrupts */
> -	/* XXX: wait for workqueue to drain */
> +	GEM_BUG_ON(has_doorbell(client));
> +
> +	err = __reserve_doorbell(client);
> +	if (err)
> +		return err;
> +
> +	return __create_doorbell(client);
>  }
> +*/

Wrong commit ?


>  
> -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;
> -
> -	id = find_next_zero_bit(guc->doorbell_bitmap, end, start);
> -	if (id == end)
> -		id = GUC_INVALID_DOORBELL_ID;
> +	struct guc_doorbell_info *doorbell;
>  
> -	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
> -			hi_pri ? "high" : "normal", id);
> +	doorbell = __get_doorbell(client);
> +	doorbell->db_status = GUC_DOORBELL_DISABLED;
> +	doorbell->cookie = 0;
>  
> -	return id;
> +	return __destroy_doorbell_hw(client);
>  }
>  
> -/*
> - * 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.
> - */
> +static int destroy_doorbell(struct i915_guc_client *client)
> +{
> +	int err;
> +
> +	GEM_BUG_ON(!has_doorbell(client));
>  
> -static uint32_t select_doorbell_cacheline(struct intel_guc *guc)
> +	/* XXX: wait for any interrupts */
> +	/* XXX: wait for workqueue to drain */
> +
> +	err = __destroy_doorbell(client);
> +	if (err)
> +		return err;
> +
> +	__release_doorbell(client);
> +
> +	return 0;
> +}
> +
> +static unsigned long __reserve_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;
>  }
>  
> @@ -584,93 +643,81 @@ 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;

We use guc few times, so maybe we can leave it as

	guc = client->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);
> -	}
> -
> +	WARN_ON(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);
> -
> -	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);

What about adding small helper function and use it here instead of
directly touching guc internal member:

	guc_release_client_index(guc, 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, u8 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);
>  
> -	if (enabled == expected)
> +	u32 drbregl = I915_READ(GEN8_DRBREGL(db_id));
> +
> +	bool 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)

Hmm, in previous function db_id was declared as u8.

> +{
> +	int err;
> +
> +	err = __update_doorbell(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);
> -
> -	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);
> +	for (i = 0; i < GUC_NUM_DOORBELLS; ++i)
> +		WARN_ON(!doorbell_ok(guc, i));
> +
> +	return 0;
>  }
>  
>  /**
> @@ -696,49 +743,52 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
>  	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)
> +	ret = __reserve_doorbell(client);
> +	if (ret)
>  		/* XXX: evict a doorbell instead? */
> -		goto err;
> +		goto err_vaddr;
>  
> -	client->doorbell_offset = select_doorbell_cacheline(guc);
> +	client->doorbell_offset = __reserve_cacheline(guc);
>  
>  	/*
>  	 * Since the doorbell only requires a single cacheline, we can save
> @@ -753,26 +803,31 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
>  	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);
> -	 */
> +	/* For runtime client allocation we need to enable the doorbell. */
> +	ret = __create_doorbell(client);
> +	if (ret)
> +		goto err_db;
>  
>  	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:
> -	guc_client_free(dev_priv, client);
> -	return NULL;
> -}
> -
> +err_db:
> +	__release_doorbell(client);
> +err_vaddr:
> +	i915_gem_object_unpin_map(client->vma->obj);
> +err_vma:
> +	i915_vma_unpin_and_release(&client->vma);
> +err_id:
> +	ida_simple_remove(&guc->ctx_ids, client->ctx_index);
> +err_client:
> +	kfree(client);
>  
> +	return ERR_PTR(ret);
> +}
>  
>  static void guc_policies_init(struct guc_policies *policies)
>  {
> @@ -881,7 +936,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)
> @@ -932,14 +987,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) {
> @@ -978,7 +1038,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>  	if (!client)
>  		return;

Shouldn't we fix it now in this patch as well?


>  
> -	guc_client_free(dev_priv, client);
> +	guc_client_free(client);
>  
>  	i915_vma_unpin_and_release(&guc->ads_vma);
>  	i915_vma_unpin_and_release(&guc->log.vma);
> 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 @@ union guc_doorbell_qw {
>  	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..3cbf853 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -75,10 +75,9 @@ struct i915_guc_client {
>  	uint32_t 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 */
> -- 
> 2.7.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC/CI] drm/i915: Sanitize GuC client initialization

[Daniele Ceraolo Spurio]

On 10/02/17 05:30, Joonas Lahtinen 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).
>
> 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: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        |   4 +-
>  drivers/gpu/drm/i915/i915_guc_submission.c | 384 +++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_guc_fwif.h      |   4 +-
>  drivers/gpu/drm/i915/intel_uc.h            |   9 +-
>  4 files changed, 230 insertions(+), 171 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1ccc297..86c6f831 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2480,7 +2480,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);
> @@ -2515,7 +2515,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 8ced9e2..d0c7004 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -62,30 +62,79 @@
>   *
>   */
>
> +static inline bool is_high_priority(struct i915_guc_client* client)
> +{
> +	return client->priority <= GUC_CTX_PRIORITY_HIGH;
> +}
> +
> +static bool __test_doorbell(struct i915_guc_client *client)
> +{
> +	return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> +}

bikeshed: this helper is only used in one place inside a very small 
function, so I'd prefer to drop it.

> +
> +static void __release_doorbell(struct i915_guc_client *client)

bikeshed: in other places we use "unreserve" instead of "release" for 
the symmetrical function of "reserve". That would be clearer here as 
well IMHO.

> +{
> +	GEM_BUG_ON(client->doorbell_id == GUC_DOORBELL_INVALID);
> +
> +	__clear_bit(client->doorbell_id, client->guc->doorbell_bitmap);

I would set client->doorbell_id = GUC_DOORBELL_INVALID here.

> +}
> +
> +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;
> +	if (is_high_priority(client))
> +		offset = GUC_NUM_DOORBELLS/2;
> +
> +	end = GUC_NUM_DOORBELLS - offset;

Should this be

	end = offset + GUC_NUM_DOORBELLS/2;

?

Otherwise if offset=0 you'll have end=GUC_NUM_DOORBELLS


> +
> +	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;
> +}
> +
>  /*
>   * 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 __create_doorbell_hw(struct i915_guc_client *client)
>  {
>  	u32 action[] = {
>  		INTEL_GUC_ACTION_ALLOCATE_DOORBELL,
>  		client->ctx_index
>  	};
>
> -	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> +	return intel_guc_send(client->guc, action, ARRAY_SIZE(action));
>  }
>
> -static int guc_release_doorbell(struct intel_guc *guc,
> -				struct i915_guc_client *client)
> +static int __destroy_doorbell_hw(struct i915_guc_client *client)
>  {
>  	u32 action[] = {
>  		INTEL_GUC_ACTION_DEALLOCATE_DOORBELL,
>  		client->ctx_index
>  	};
>
> -	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> +	return intel_guc_send(client->guc, action, ARRAY_SIZE(action));
>  }
>
>  /*
> @@ -95,104 +144,114 @@ 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(struct i915_guc_client *client, u16 new_id)

I'd prefer this to be called __update_doorbell_id or 
__update_doorbell_desc, to make it clear that it is just changing the id 
in the descriptor and not doing any re-allocation of the doorbell.


>  {
> -	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;
> +}
> +
> +static bool has_doorbell(struct i915_guc_client *client)
> +{
> +	if (client->doorbell_id == GUC_DOORBELL_INVALID)
> +		return false;
> +
> +	return __test_doorbell(client);
> +}
>
> -	/* 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 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 = __create_doorbell_hw(client);
> +	if (err) {
> +		doorbell->db_status = GUC_DOORBELL_DISABLED;
> +		doorbell->cookie = 0;
> +		__release_doorbell(client);
> +	}
> +	return err;
>  }
>
> -static void guc_disable_doorbell(struct intel_guc *guc,
> -				 struct i915_guc_client *client)
> +/*
> +static int create_doorbell(struct i915_guc_client *client)
>  {
> -	(void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
> +	int err;
>
> -	/* XXX: wait for any interrupts */
> -	/* XXX: wait for workqueue to drain */
> +	GEM_BUG_ON(has_doorbell(client));
> +
> +	err = __reserve_doorbell(client);
> +	if (err)
> +		return err;
> +
> +	return __create_doorbell(client);
>  }
> +*/
>
> -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;
> -
> -	id = find_next_zero_bit(guc->doorbell_bitmap, end, start);
> -	if (id == end)
> -		id = GUC_INVALID_DOORBELL_ID;
> +	struct guc_doorbell_info *doorbell;
>
> -	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
> -			hi_pri ? "high" : "normal", id);
> +	doorbell = __get_doorbell(client);
> +	doorbell->db_status = GUC_DOORBELL_DISABLED;
> +	doorbell->cookie = 0;
>
> -	return id;
> +	return __destroy_doorbell_hw(client);
>  }
>
> -/*
> - * 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.
> - */
> +static int destroy_doorbell(struct i915_guc_client *client)
> +{
> +	int err;
> +
> +	GEM_BUG_ON(!has_doorbell(client));
>
> -static uint32_t select_doorbell_cacheline(struct intel_guc *guc)
> +	/* XXX: wait for any interrupts */
> +	/* XXX: wait for workqueue to drain */
> +
> +	err = __destroy_doorbell(client);
> +	if (err)
> +		return err;
> +
> +	__release_doorbell(client);
> +
> +	return 0;
> +}
> +
> +static unsigned long __reserve_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;
>  }
>
> @@ -584,93 +643,81 @@ 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);
> -	}
> -
> +	WARN_ON(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);
> -
> -	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, u8 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);
>
> -	if (enabled == expected)
> +	u32 drbregl = I915_READ(GEN8_DRBREGL(db_id));
> +
> +	bool 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(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);

Not sure about the logic here. If the doorbell is active when it 
shouldn't have been, __reset_doorbell will try to acquire the doorbell 
before releasing it, which will fail if the doorbell is active (thus 
also skipping the release_doorbell step). If the doorbell is not active 
when it should have been, __reset_doorbell will still leave it 
deactivated. If we care about both cases maybe we should specialize the 
handling for the 2 possible scenarios, while if we care only about the 
first one we should release the doorbell first (not sure if we still 
want to do a setup & tear-down cycle afterwards).

The original logic looks also incorrect because guc_update_doorbell_id 
will always enable the doorbell at the end.

> +		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);
> -
> -	err = guc_update_doorbell_id(guc, client, db_id);
> +	err = __reserve_doorbell(client);

Shouldn't this be create_doorbell() instead of reserve?

Thanks,
Daniele

>  	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);
> +	for (i = 0; i < GUC_NUM_DOORBELLS; ++i)
> +		WARN_ON(!doorbell_ok(guc, i));
> +
> +	return 0;
>  }
>
>  /**
> @@ -696,49 +743,52 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
>  	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)
> +	ret = __reserve_doorbell(client);
> +	if (ret)
>  		/* XXX: evict a doorbell instead? */
> -		goto err;
> +		goto err_vaddr;
>
> -	client->doorbell_offset = select_doorbell_cacheline(guc);
> +	client->doorbell_offset = __reserve_cacheline(guc);
>
>  	/*
>  	 * Since the doorbell only requires a single cacheline, we can save
> @@ -753,26 +803,31 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
>  	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);
> -	 */
> +	/* For runtime client allocation we need to enable the doorbell. */
> +	ret = __create_doorbell(client);
> +	if (ret)
> +		goto err_db;
>
>  	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:
> -	guc_client_free(dev_priv, client);
> -	return NULL;
> -}
> -
> +err_db:
> +	__release_doorbell(client);
> +err_vaddr:
> +	i915_gem_object_unpin_map(client->vma->obj);
> +err_vma:
> +	i915_vma_unpin_and_release(&client->vma);
> +err_id:
> +	ida_simple_remove(&guc->ctx_ids, client->ctx_index);
> +err_client:
> +	kfree(client);
>
> +	return ERR_PTR(ret);
> +}
>
>  static void guc_policies_init(struct guc_policies *policies)
>  {
> @@ -881,7 +936,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)
> @@ -932,14 +987,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) {
> @@ -978,7 +1038,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>  	if (!client)
>  		return;
>
> -	guc_client_free(dev_priv, client);
> +	guc_client_free(client);
>
>  	i915_vma_unpin_and_release(&guc->ads_vma);
>  	i915_vma_unpin_and_release(&guc->log.vma);
> 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 @@ union guc_doorbell_qw {
>  	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..3cbf853 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -75,10 +75,9 @@ struct i915_guc_client {
>  	uint32_t 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 */
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC/CI] drm/i915: Sanitize GuC client initialization

[Daniele Ceraolo Spurio]

<snip>
>> +
>>  /*
>>   * 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 __create_doorbell_hw(struct i915_guc_client *client)
>
> I would rather prefer to only change signature of this function into
>
> 	static int guc_allocate_doorbell(struct intel_guc *guc, u32 index)
>
> as a clean wrap around GUC_ACTION_ALLOCATE_DOORBELL. This way we also preserve
> consistency between function name and the guc action name used inside.
>
> Based on the above we can still add
>
> 	static int __create_doorbell_hw(struct i915_guc_client *client)
> 	{
> 		return guc_allocate_doorbell(client->guc, client->ctx_index);
> 	}
>
> Note that location of the ctx_index member may change in the future, and this
> approach will minimize impact of these future changes.
>

+1.
The client is a SW abstraction that we use to track our registrations 
with GuC, but it only works with our current mode of operation. If we 
plumb it too deep into the low-level functions it'll be more difficult 
to do any reworks in the future.

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

Re: [RFC/CI] drm/i915: Sanitize GuC client initialization

[Joonas Lahtinen]

On pe, 2017-02-10 at 11:55 -0800, Daniele Ceraolo Spurio wrote:
> 
> On 10/02/17 05:30, Joonas Lahtinen wrote:

<SNIP>

> > +static bool __test_doorbell(struct i915_guc_client *client)
> > +{
> > +	return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> > +}
> 
> bikeshed: this helper is only used in one place inside a very small 
> function, so I'd prefer to drop it.

Good catch, the need disappeared during refactoring.

> 
> > 
> > +
> > +static void __release_doorbell(struct i915_guc_client *client)
> 
> bikeshed: in other places we use "unreserve" instead of "release" for 
> the symmetrical function of "reserve". That would be clearer here as 
> well IMHO.

That's true, fixed.

> 
> > 
> > +{
> > +	GEM_BUG_ON(client->doorbell_id == GUC_DOORBELL_INVALID);
> > +
> > +	__clear_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> 
> I would set client->doorbell_id = GUC_DOORBELL_INVALID here.

Yeah, it's better for symmetry, fixed that.

> > +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;
> > +	if (is_high_priority(client))
> > +		offset = GUC_NUM_DOORBELLS/2;
> > +
> > +	end = GUC_NUM_DOORBELLS - offset;
> 
> Should this be
> 
> 	end = offset + GUC_NUM_DOORBELLS/2;
> 
> ?
> 
> Otherwise if offset=0 you'll have end=GUC_NUM_DOORBELLS

Whoops, fixed :)

> > @@ -95,104 +144,114 @@ 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(struct i915_guc_client *client, u16 new_id)
> 
> I'd prefer this to be called __update_doorbell_id or 
> __update_doorbell_desc, to make it clear that it is just changing the id 
> in the descriptor and not doing any re-allocation of the doorbell.

__update_doorbell_desc it is (I'd like to split doorbell vs. client to
their own structs, but lets leave that for later).

This only gets called during

> > -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);
> 
> Not sure about the logic here. If the doorbell is active when it 
> shouldn't have been, __reset_doorbell will try to acquire the doorbell 
> before releasing it, which will fail if the doorbell is active (thus 
> also skipping the release_doorbell step). If the doorbell is not active 
> when it should have been, __reset_doorbell will still leave it 
> deactivated. If we care about both cases maybe we should specialize the 
> handling for the 2 possible scenarios, while if we care only about the 
> first one we should release the doorbell first (not sure if we still 
> want to do a setup & tear-down cycle afterwards).
> 
> The original logic looks also incorrect because guc_update_doorbell_id 
> will always enable the doorbell at the end.

I was assuming we want to deactivate all doorbells from the hardware if
they are active (because they would be stale entries). If there are no
other users than the KMD, then that'd be the appropriate action, and
rename doorbell_ok check into doorbell_active, and just nuke everything
active once loading?

> > +		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);
> > -
> > -	err = guc_update_doorbell_id(guc, client, db_id);
> > +	err = __reserve_doorbell(client);
> 
> 
> Shouldn't this be create_doorbell() instead of reserve?

That would end up calling the __update_doorbell, which we can't as the
desc is for some strange reason not mapped yet. I think you sent a
patch to move intel_guc_allocate_vma into the init, so let me give it a
look.

Which brings us to the point that I forgot to call __update_doorbell
completely!

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: [RFC/CI] drm/i915: Sanitize GuC client initialization

[Joonas Lahtinen]

On pe, 2017-02-10 at 12:03 -0800, Daniele Ceraolo Spurio wrote:
> <snip>
> > 
> > > 
> > > +
> > >  /*
> > >   * 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 __create_doorbell_hw(struct i915_guc_client *client)
> > 
> > I would rather prefer to only change signature of this function into
> > 
> > 	static int guc_allocate_doorbell(struct intel_guc *guc, u32 index)
> > 
> > as a clean wrap around GUC_ACTION_ALLOCATE_DOORBELL. This way we also preserve
> > consistency between function name and the guc action name used inside.
> > 
> > Based on the above we can still add
> > 
> > 	static int __create_doorbell_hw(struct i915_guc_client *client)
> > 	{
> > 		return guc_allocate_doorbell(client->guc, client->ctx_index);
> > 	}
> > 
> > Note that location of the ctx_index member may change in the future, and this
> > approach will minimize impact of these future changes.
> > 
> 
> +1.
> The client is a SW abstraction that we use to track our registrations 
> with GuC, but it only works with our current mode of operation. If we 
> plumb it too deep into the low-level functions it'll be more difficult 
> to do any reworks in the future.

Indeed, the ctx_index should move. I'd like to keep it __ prefixed
function, because the client needs the desc updated for the function to
be meaningful.

So I'll make it __guc_{de}allocate_doorbell for now.

Might be good to have i915_guc_doorbell, i915_guc_context,
i915_guc_client in the future to reduce code entangelment.

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: [RFC/CI] drm/i915: Sanitize GuC client initialization

[Joonas Lahtinen]

On pe, 2017-02-10 at 16:11 +0100, Michal Wajdeczko wrote:
> On Fri, Feb 10, 2017 at 03:30:10PM +0200, Joonas Lahtinen 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).
> > 
> > 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: Oscar Mateo <oscar.mateo@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> explicit inline ? or you want to let gcc decide ?

GCC should do it, these are not hot path functions.

 
> > -static int guc_allocate_doorbell(struct intel_guc *guc,
> > -				 struct i915_guc_client *client)
> > +static int __create_doorbell_hw(struct i915_guc_client *client)
> 
> I would rather prefer to only change signature of this function into
> 
> 	static int guc_allocate_doorbell(struct intel_guc *guc, u32 index)
> 
> as a clean wrap around GUC_ACTION_ALLOCATE_DOORBELL. This way we also preserve
> consistency between function name and the guc action name used inside.
> 
> Based on the above we can still add 
> 
> 	static int __create_doorbell_hw(struct i915_guc_client *client)
> 	{
> > 		return guc_allocate_doorbell(client->guc, client->ctx_index);
> 	}
> 
> Note that location of the ctx_index member may change in the future, and this
> approach will minimize impact of these future changes.

That's viable, I made it;

__guc_allocate_doorbell(guc, client->ctx_index)

So it can bemoved out of the submission code in future.

> > +static bool has_doorbell(struct i915_guc_client *client)
> > +{
> > +	if (client->doorbell_id == GUC_DOORBELL_INVALID)
> > +		return false;
> > +
> > +	return __test_doorbell(client);
> > +}
> 
> Can we keep related inline helpers together ?

Moved.
 
> > -static void guc_disable_doorbell(struct intel_guc *guc,
> > -				 struct i915_guc_client *client)
> > +/*
> > +static int create_doorbell(struct i915_guc_client *client)
> >  {
> > -	(void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
> > +	int err;
> >  
> > -	/* XXX: wait for any interrupts */
> > -	/* XXX: wait for workqueue to drain */
> > +	GEM_BUG_ON(has_doorbell(client));
> > +
> > +	err = __reserve_doorbell(client);
> > +	if (err)
> > +		return err;
> > +
> > +	return __create_doorbell(client);
> >  }
> > +*/
> 
> Wrong commit ?

Nuked.

> > -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;
> 
> We use guc few times, so maybe we can leave it as
> 
> 	guc = client->guc;

Wanted to make it explicit that there are behind the scenes contracts
(allocated doorbells) between the client and guc.

> > +	WARN_ON(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);
> > -
> > -	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);
> 
> What about adding small helper function and use it here instead of
> directly touching guc internal member:
> 
> 	guc_release_client_index(guc, client->ctx_index);

I'll add a follow-up task for that.
 
> >  /* 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, u8 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);
> >  
> > -	if (enabled == expected)
> > +	u32 drbregl = I915_READ(GEN8_DRBREGL(db_id));
> > +
> > +	bool 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)
> 
> Hmm, in previous function db_id was declared as u8.

Yeah, would be kinda mean to check the status of GUC_DOORBELL_INVALID.
But as it just happens to the current situation, I changed the
doorbell_ok signature and added GEM_BUG_ON check for future proofing.

> > @@ -978,7 +1038,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> >  	if (!client)
> > 		return;
> 
> Shouldn't we fix it now in this patch as well?
> 

I'd wait for Daniele's patches to for persitent desc mapping
(allocation is currently phased rather strangely), and didn't want to
duplicate work.

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: [RFC/CI] drm/i915: Sanitize GuC client initialization

[Joonas Lahtinen]

On pe, 2017-02-10 at 14:36 +0000, Chris Wilson wrote:
> On Fri, Feb 10, 2017 at 03:30:10PM +0200, Joonas Lahtinen wrote:
> > 
> > +static unsigned long __reserve_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);
> 
> But unsigned long? I was trying to work out why we might need unsigned
> long doorbell_offset, for when the doorbell object is of known size.
> 
> That's the only thing that stuck out like a sore thumb.

Fixing that type took me for a too long journey, so I aborted mission.
Added a follow-up task.

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