All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oscar Mateo <oscar.mateo@intel.com>
To: intel-gfx@lists.freedesktop.org
Subject: [01/11 v3] drm/i915/guc: Sanitize GuC client initialization
Date: Fri, 10 Mar 2017 08:28:48 -0800	[thread overview]
Message-ID: <1489163337-36080-2-git-send-email-oscar.mateo@intel.com> (raw)
In-Reply-To: <1489163337-36080-1-git-send-email-oscar.mateo@intel.com>

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

  reply	other threads:[~2017-03-11  0:28 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-10 16:28 [00/11 v2] Various improvements around the GuC topic Oscar Mateo
2017-03-10 16:28 ` Oscar Mateo [this message]
2017-03-13 20:19   ` [01/11 v3] drm/i915/guc: Sanitize GuC client initialization Daniele Ceraolo Spurio
2017-03-10 16:28 ` [02/11 v3] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access Oscar Mateo
2017-03-10 16:28 ` [03/11 v3] drm/i915/guc: Add onion teardown to the GuC setup Oscar Mateo
2017-03-13 13:30   ` Arkadiusz Hiler
2017-03-16 13:04   ` Joonas Lahtinen
2017-03-10 16:28 ` [04/11 v2] drm/i915/guc: s/ads_vma/addon Oscar Mateo
2017-03-10 16:28 ` [05/11 v2] drm/i915/guc: Break out the GuC log extras into their own "runtime" struct Oscar Mateo
2017-03-16 13:20   ` Joonas Lahtinen
2017-03-10 16:28 ` [06/11 v3] drm/i915/guc: Make intel_guc_send a function pointer Oscar Mateo
2017-03-13 21:00   ` Daniele Ceraolo Spurio
2017-03-10 16:28 ` [07/11] drm/i915/guc: Improve the GuC documentation & comments about proxy submissions Oscar Mateo
2017-03-13 21:37   ` Daniele Ceraolo Spurio
2017-03-10 16:28 ` [08/11] drm/i915/guc: Wait for doorbell to be inactive before deallocating Oscar Mateo
2017-03-13 11:46   ` Chris Wilson
2017-03-13  8:56     ` Oscar Mateo
2017-03-15  0:55       ` Daniele Ceraolo Spurio
2017-03-16 13:26   ` Joonas Lahtinen
2017-03-10 16:28 ` [09/11] drm/i915/guc: A little bit more of doorbell sanitization Oscar Mateo
2017-03-20 13:14   ` Joonas Lahtinen
2017-03-10 16:28 ` [10/11] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor" Oscar Mateo
2017-03-13 11:49   ` Chris Wilson
2017-03-13  8:59     ` Oscar Mateo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1489163337-36080-2-git-send-email-oscar.mateo@intel.com \
    --to=oscar.mateo@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.