[PATCH v4 0/6] Fixes and workarounds for GuC/doorbell setup

[PATCH v4 0/6] Fixes and workarounds for GuC/doorbell setup

[Dave Gordon]

Several issues have been found and recently fixed around the general
area of resetting and reloading the GuC, but the driver can still
leave the doorbell hardware in an inconsistent state after a reset
and reload, such as during a hibernate-and-resume cycle.

This set of patches provides solutions (or in some cases workarounds)
for the various issues identified in this area.

Cc: Alex Dai <yu.dai@intel.com>
Cc: Tom O'Rourke <tom.orourke@intel.com>
Cc: Arun Siluvery <arun.siluvery@linux.intel.com>

Dave Gordon (5+1):
  drm/i915/guc: add doorbell map to debugfs/i915_guc_info
  drm/i915/guc: move guc_ring_doorbell() nearer to callsite
  drm/i915/guc: refactor doorbell management code
  drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission
  drm/i915/guc: disable GuC submission earlier during GuC (re)load
 [DO NOT MERGE: add enable_guc_loading parameter]

 drivers/gpu/drm/i915/i915_debugfs.c        |   8 +
 drivers/gpu/drm/i915/i915_guc_submission.c | 264 ++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_guc_loader.c    |   3 -
 6 files changed, 242 insertions(+), 149 deletions(-)

-- 
1.9.1

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

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

[Dave Gordon]

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

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

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

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

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

[Yu Dai]

LGTM.

Reviewed-by: Alex Dai <yu.dai@intel.com>

On 04/07/2016 10:21 AM, Dave Gordon wrote:
> To properly verify the driver->doorbell->GuC functionality, validation
> needs to know how the driver has assigned the doorbell cache lines and
> registers, so make them visible through debugfs.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index be4bcdc..87a9f3e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2488,6 +2488,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
>   	struct i915_guc_client client = {};
>   	struct intel_engine_cs *engine;
>   	u64 total = 0;
> +	int i;
>   
>   	if (!HAS_GUC_SCHED(dev_priv))
>   		return 0;
> @@ -2502,6 +2503,13 @@ static int i915_guc_info(struct seq_file *m, void *data)
>   
>   	mutex_unlock(&dev->struct_mutex);
>   
> +	seq_printf(m, "Doorbell map:\n");
> +	for (i = 0; i < BITS_TO_LONGS(GUC_MAX_DOORBELLS) - 3; i += 4)
> +		seq_printf(m, "\t%016lx %016lx %016lx %016lx\n",
> +			guc.doorbell_bitmap[i], guc.doorbell_bitmap[i+1],
> +			guc.doorbell_bitmap[i+2], guc.doorbell_bitmap[i+3]);
> +	seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", guc.db_cacheline);
> +
>   	seq_printf(m, "GuC total action count: %llu\n", guc.action_count);
>   	seq_printf(m, "GuC action failure count: %u\n", guc.action_fail);
>   	seq_printf(m, "GuC last action command: 0x%x\n", guc.action_cmd);

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

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

[Dave Gordon]

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

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index da86bdb..2171759 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -190,67 +190,6 @@ static void guc_init_doorbell(struct intel_guc *guc,
 	kunmap_atomic(base);
 }
 
-static int guc_ring_doorbell(struct i915_guc_client *gc)
-{
-	struct guc_process_desc *desc;
-	union guc_doorbell_qw db_cmp, db_exc, db_ret;
-	union guc_doorbell_qw *db;
-	void *base;
-	int attempt = 2, ret = -EAGAIN;
-
-	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
-	desc = base + gc->proc_desc_offset;
-
-	/* Update the tail so it is visible to GuC */
-	desc->tail = gc->wq_tail;
-
-	/* current cookie */
-	db_cmp.db_status = GUC_DOORBELL_ENABLED;
-	db_cmp.cookie = gc->cookie;
-
-	/* cookie to be updated */
-	db_exc.db_status = GUC_DOORBELL_ENABLED;
-	db_exc.cookie = gc->cookie + 1;
-	if (db_exc.cookie == 0)
-		db_exc.cookie = 1;
-
-	/* pointer of current doorbell cacheline */
-	db = base + gc->doorbell_offset;
-
-	while (attempt--) {
-		/* lets ring the doorbell */
-		db_ret.value_qw = atomic64_cmpxchg((atomic64_t *)db,
-			db_cmp.value_qw, db_exc.value_qw);
-
-		/* if the exchange was successfully executed */
-		if (db_ret.value_qw == db_cmp.value_qw) {
-			/* db was successfully rung */
-			gc->cookie = db_exc.cookie;
-			ret = 0;
-			break;
-		}
-
-		/* XXX: doorbell was lost and need to acquire it again */
-		if (db_ret.db_status == GUC_DOORBELL_DISABLED)
-			break;
-
-		DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n",
-			  db_cmp.cookie, db_ret.cookie);
-
-		/* update the cookie to newly read cookie from GuC */
-		db_cmp.cookie = db_ret.cookie;
-		db_exc.cookie = db_ret.cookie + 1;
-		if (db_exc.cookie == 0)
-			db_exc.cookie = 1;
-	}
-
-	/* Finally, update the cached copy of the GuC's WQ head */
-	gc->wq_head = desc->head;
-
-	kunmap_atomic(base);
-	return ret;
-}
-
 static void guc_disable_doorbell(struct intel_guc *guc,
 				 struct i915_guc_client *client)
 {
@@ -471,6 +410,12 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
 			     sizeof(desc) * client->ctx_index);
 }
 
+/*
+ * Everything above here is concerned with setup & teardown, and is
+ * therefore not part of the somewhat time-critical batch-submission
+ * path of i915_guc_submit() below.
+ */
+
 int i915_guc_wq_check_space(struct i915_guc_client *gc)
 {
 	struct guc_process_desc *desc;
@@ -559,6 +504,67 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 	return 0;
 }
 
+static int guc_ring_doorbell(struct i915_guc_client *gc)
+{
+	struct guc_process_desc *desc;
+	union guc_doorbell_qw db_cmp, db_exc, db_ret;
+	union guc_doorbell_qw *db;
+	void *base;
+	int attempt = 2, ret = -EAGAIN;
+
+	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
+	desc = base + gc->proc_desc_offset;
+
+	/* Update the tail so it is visible to GuC */
+	desc->tail = gc->wq_tail;
+
+	/* current cookie */
+	db_cmp.db_status = GUC_DOORBELL_ENABLED;
+	db_cmp.cookie = gc->cookie;
+
+	/* cookie to be updated */
+	db_exc.db_status = GUC_DOORBELL_ENABLED;
+	db_exc.cookie = gc->cookie + 1;
+	if (db_exc.cookie == 0)
+		db_exc.cookie = 1;
+
+	/* pointer of current doorbell cacheline */
+	db = base + gc->doorbell_offset;
+
+	while (attempt--) {
+		/* lets ring the doorbell */
+		db_ret.value_qw = atomic64_cmpxchg((atomic64_t *)db,
+			db_cmp.value_qw, db_exc.value_qw);
+
+		/* if the exchange was successfully executed */
+		if (db_ret.value_qw == db_cmp.value_qw) {
+			/* db was successfully rung */
+			gc->cookie = db_exc.cookie;
+			ret = 0;
+			break;
+		}
+
+		/* XXX: doorbell was lost and need to acquire it again */
+		if (db_ret.db_status == GUC_DOORBELL_DISABLED)
+			break;
+
+		DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n",
+			  db_cmp.cookie, db_ret.cookie);
+
+		/* update the cookie to newly read cookie from GuC */
+		db_cmp.cookie = db_ret.cookie;
+		db_exc.cookie = db_ret.cookie + 1;
+		if (db_exc.cookie == 0)
+			db_exc.cookie = 1;
+	}
+
+	/* Finally, update the cached copy of the GuC's WQ head */
+	gc->wq_head = desc->head;
+
+	kunmap_atomic(base);
+	return ret;
+}
+
 /**
  * i915_guc_submit() - Submit commands through GuC
  * @client:	the guc client where commands will go through
-- 
1.9.1

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

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

[Yu Dai]

LGTM.

Reviewed-by: Alex Dai <yu.dai@intel.com>

On 04/07/2016 10:21 AM, Dave Gordon wrote:
> Just code movement, no actual change to the function. This is in
> preparation for the next patch, which will reorganise all the other
> doorbell code, but doesn't change this function. So let's shuffle it
> down near its caller rather than leaving it mixed in with the setup
> code. Unlike the doorbell management code, this function is somewhat
> time-critical, so putting it near its caller may even yield a tiny
> performance improvement.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 128 +++++++++++++++--------------
>   1 file changed, 67 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index da86bdb..2171759 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -190,67 +190,6 @@ static void guc_init_doorbell(struct intel_guc *guc,
>   	kunmap_atomic(base);
>   }
>   
> -static int guc_ring_doorbell(struct i915_guc_client *gc)
> -{
> -	struct guc_process_desc *desc;
> -	union guc_doorbell_qw db_cmp, db_exc, db_ret;
> -	union guc_doorbell_qw *db;
> -	void *base;
> -	int attempt = 2, ret = -EAGAIN;
> -
> -	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> -	desc = base + gc->proc_desc_offset;
> -
> -	/* Update the tail so it is visible to GuC */
> -	desc->tail = gc->wq_tail;
> -
> -	/* current cookie */
> -	db_cmp.db_status = GUC_DOORBELL_ENABLED;
> -	db_cmp.cookie = gc->cookie;
> -
> -	/* cookie to be updated */
> -	db_exc.db_status = GUC_DOORBELL_ENABLED;
> -	db_exc.cookie = gc->cookie + 1;
> -	if (db_exc.cookie == 0)
> -		db_exc.cookie = 1;
> -
> -	/* pointer of current doorbell cacheline */
> -	db = base + gc->doorbell_offset;
> -
> -	while (attempt--) {
> -		/* lets ring the doorbell */
> -		db_ret.value_qw = atomic64_cmpxchg((atomic64_t *)db,
> -			db_cmp.value_qw, db_exc.value_qw);
> -
> -		/* if the exchange was successfully executed */
> -		if (db_ret.value_qw == db_cmp.value_qw) {
> -			/* db was successfully rung */
> -			gc->cookie = db_exc.cookie;
> -			ret = 0;
> -			break;
> -		}
> -
> -		/* XXX: doorbell was lost and need to acquire it again */
> -		if (db_ret.db_status == GUC_DOORBELL_DISABLED)
> -			break;
> -
> -		DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n",
> -			  db_cmp.cookie, db_ret.cookie);
> -
> -		/* update the cookie to newly read cookie from GuC */
> -		db_cmp.cookie = db_ret.cookie;
> -		db_exc.cookie = db_ret.cookie + 1;
> -		if (db_exc.cookie == 0)
> -			db_exc.cookie = 1;
> -	}
> -
> -	/* Finally, update the cached copy of the GuC's WQ head */
> -	gc->wq_head = desc->head;
> -
> -	kunmap_atomic(base);
> -	return ret;
> -}
> -
>   static void guc_disable_doorbell(struct intel_guc *guc,
>   				 struct i915_guc_client *client)
>   {
> @@ -471,6 +410,12 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
>   			     sizeof(desc) * client->ctx_index);
>   }
>   
> +/*
> + * Everything above here is concerned with setup & teardown, and is
> + * therefore not part of the somewhat time-critical batch-submission
> + * path of i915_guc_submit() below.
> + */
> +
>   int i915_guc_wq_check_space(struct i915_guc_client *gc)
>   {
>   	struct guc_process_desc *desc;
> @@ -559,6 +504,67 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
>   	return 0;
>   }
>   
> +static int guc_ring_doorbell(struct i915_guc_client *gc)
> +{
> +	struct guc_process_desc *desc;
> +	union guc_doorbell_qw db_cmp, db_exc, db_ret;
> +	union guc_doorbell_qw *db;
> +	void *base;
> +	int attempt = 2, ret = -EAGAIN;
> +
> +	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> +	desc = base + gc->proc_desc_offset;
> +
> +	/* Update the tail so it is visible to GuC */
> +	desc->tail = gc->wq_tail;
> +
> +	/* current cookie */
> +	db_cmp.db_status = GUC_DOORBELL_ENABLED;
> +	db_cmp.cookie = gc->cookie;
> +
> +	/* cookie to be updated */
> +	db_exc.db_status = GUC_DOORBELL_ENABLED;
> +	db_exc.cookie = gc->cookie + 1;
> +	if (db_exc.cookie == 0)
> +		db_exc.cookie = 1;
> +
> +	/* pointer of current doorbell cacheline */
> +	db = base + gc->doorbell_offset;
> +
> +	while (attempt--) {
> +		/* lets ring the doorbell */
> +		db_ret.value_qw = atomic64_cmpxchg((atomic64_t *)db,
> +			db_cmp.value_qw, db_exc.value_qw);
> +
> +		/* if the exchange was successfully executed */
> +		if (db_ret.value_qw == db_cmp.value_qw) {
> +			/* db was successfully rung */
> +			gc->cookie = db_exc.cookie;
> +			ret = 0;
> +			break;
> +		}
> +
> +		/* XXX: doorbell was lost and need to acquire it again */
> +		if (db_ret.db_status == GUC_DOORBELL_DISABLED)
> +			break;
> +
> +		DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n",
> +			  db_cmp.cookie, db_ret.cookie);
> +
> +		/* update the cookie to newly read cookie from GuC */
> +		db_cmp.cookie = db_ret.cookie;
> +		db_exc.cookie = db_ret.cookie + 1;
> +		if (db_exc.cookie == 0)
> +			db_exc.cookie = 1;
> +	}
> +
> +	/* Finally, update the cached copy of the GuC's WQ head */
> +	gc->wq_head = desc->head;
> +
> +	kunmap_atomic(base);
> +	return ret;
> +}
> +
>   /**
>    * i915_guc_submit() - Submit commands through GuC
>    * @client:	the guc client where commands will go through

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

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

[Dave Gordon]

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

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2171759..2fc69f1 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -175,8 +175,48 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
  * client object which contains the page being used for the doorbell
  */
 
+static int guc_update_doorbell_id(struct i915_guc_client *client,
+				  struct guc_doorbell_info *doorbell,
+				  u16 new_id)
+{
+	struct sg_table *sg = client->guc->ctx_pool_obj->pages;
+	void *doorbell_bitmap = client->guc->doorbell_bitmap;
+	struct guc_context_desc desc;
+	size_t len;
+
+	if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
+	    test_bit(client->doorbell_id, doorbell_bitmap)) {
+		/* Deactivate the old doorbell */
+		doorbell->db_status = GUC_DOORBELL_DISABLED;
+		(void)host2guc_release_doorbell(client->guc, client);
+		clear_bit(client->doorbell_id, doorbell_bitmap);
+	}
+
+	/* Update the GuC's idea of the doorbell ID */
+	len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
+			     sizeof(desc) * client->ctx_index);
+	if (len != sizeof(desc))
+		return -EFAULT;
+	desc.db_id = new_id;
+	len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
+			     sizeof(desc) * client->ctx_index);
+	if (len != sizeof(desc))
+		return -EFAULT;
+
+	client->doorbell_id = new_id;
+	if (new_id == GUC_INVALID_DOORBELL_ID)
+		return 0;
+
+	/* Activate the new doorbell */
+	set_bit(client->doorbell_id, doorbell_bitmap);
+	doorbell->db_status = GUC_DOORBELL_ENABLED;
+	doorbell->cookie = 0;
+	return host2guc_allocate_doorbell(client->guc, client);
+}
+
 static void guc_init_doorbell(struct intel_guc *guc,
-			      struct i915_guc_client *client)
+			      struct i915_guc_client *client,
+			      uint16_t db_id)
 {
 	struct guc_doorbell_info *doorbell;
 	void *base;
@@ -184,8 +224,7 @@ static void guc_init_doorbell(struct intel_guc *guc,
 	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
 	doorbell = base + client->doorbell_offset;
 
-	doorbell->db_status = 1;
-	doorbell->cookie = 0;
+	guc_update_doorbell_id(client, doorbell, db_id);
 
 	kunmap_atomic(base);
 }
@@ -193,27 +232,16 @@ static void guc_init_doorbell(struct intel_guc *guc,
 static void guc_disable_doorbell(struct intel_guc *guc,
 				 struct i915_guc_client *client)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct guc_doorbell_info *doorbell;
 	void *base;
-	i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
-	int value;
 
 	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
 	doorbell = base + client->doorbell_offset;
 
-	doorbell->db_status = 0;
+	guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID);
 
 	kunmap_atomic(base);
 
-	I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
-
-	value = I915_READ(drbreg);
-	WARN_ON((value & GEN8_DRB_VALID) != 0);
-
-	I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
-	I915_WRITE(drbreg, 0);
-
 	/* XXX: wait for any interrupts */
 	/* XXX: wait for workqueue to drain */
 }
@@ -260,7 +288,7 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
 	if (id == end)
 		id = GUC_INVALID_DOORBELL_ID;
 	else
-		bitmap_set(guc->doorbell_bitmap, id, 1);
+		set_bit(id, guc->doorbell_bitmap);
 
 	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
 			hi_pri ? "high" : "normal", id);
@@ -268,11 +296,6 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
 	return id;
 }
 
-static void release_doorbell(struct intel_guc *guc, uint16_t id)
-{
-	bitmap_clear(guc->doorbell_bitmap, id, 1);
-}
-
 /*
  * Initialise the process descriptor shared with the GuC firmware.
  */
@@ -667,15 +690,7 @@ static void guc_client_free(struct drm_device *dev,
 	if (!client)
 		return;
 
-	if (client->doorbell_id != GUC_INVALID_DOORBELL_ID) {
-		/*
-		 * First disable the doorbell, then tell the GuC we've
-		 * finished with it, finally deallocate it in our bitmap
-		 */
-		guc_disable_doorbell(guc, client);
-		host2guc_release_doorbell(guc, client);
-		release_doorbell(guc, client->doorbell_id);
-	}
+	guc_disable_doorbell(guc, client);
 
 	/*
 	 * XXX: wait for any outstanding submissions before freeing memory.
@@ -712,6 +727,7 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc *guc = &dev_priv->guc;
 	struct drm_i915_gem_object *obj;
+	uint16_t db_id;
 
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client)
@@ -750,22 +766,24 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 	else
 		client->proc_desc_offset = (GUC_DB_SIZE / 2);
 
-	client->doorbell_id = assign_doorbell(guc, client->priority);
-	if (client->doorbell_id == GUC_INVALID_DOORBELL_ID)
+	db_id = assign_doorbell(guc, client->priority);
+	if (db_id == GUC_INVALID_DOORBELL_ID)
 		/* XXX: evict a doorbell instead */
 		goto err;
 
 	guc_init_proc_desc(guc, client);
 	guc_init_ctx_desc(guc, client);
-	guc_init_doorbell(guc, client);
+	guc_init_doorbell(guc, client, db_id);
 
 	/* XXX: Any cache flushes needed? General domain mgmt calls? */
 
 	if (host2guc_allocate_doorbell(guc, client))
 		goto err;
 
-	DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id %u\n",
-		priority, client, client->ctx_index, client->doorbell_id);
+	DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u\n",
+		priority, client, client->ctx_index);
+	DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n",
+		client->doorbell_id, client->doorbell_offset);
 
 	return client;
 
-- 
1.9.1

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

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

[Yu Dai]

On 04/07/2016 10:21 AM, Dave Gordon wrote:
> During a hibernate/resume cycle, the driver, the GuC, and the doorbell
> hardware can end up in inconsistent states. This patch refactors the
> driver's handling and tracking of doorbells, in preparation for a later
> one which will resolve the issue.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 88 ++++++++++++++++++------------
>   1 file changed, 53 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 2171759..2fc69f1 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -175,8 +175,48 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
>    * client object which contains the page being used for the doorbell
>    */
>   
> +static int guc_update_doorbell_id(struct i915_guc_client *client,
> +				  struct guc_doorbell_info *doorbell,
> +				  u16 new_id)
> +{
> +	struct sg_table *sg = client->guc->ctx_pool_obj->pages;
> +	void *doorbell_bitmap = client->guc->doorbell_bitmap;
> +	struct guc_context_desc desc;
> +	size_t len;
> +
> +	if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
> +	    test_bit(client->doorbell_id, doorbell_bitmap)) {
> +		/* Deactivate the old doorbell */
> +		doorbell->db_status = GUC_DOORBELL_DISABLED;
> +		(void)host2guc_release_doorbell(client->guc, client);
> +		clear_bit(client->doorbell_id, doorbell_bitmap);
> +	}
> +
> +	/* Update the GuC's idea of the doorbell ID */
> +	len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
> +			     sizeof(desc) * client->ctx_index);
> +	if (len != sizeof(desc))
> +		return -EFAULT;
> +	desc.db_id = new_id;
> +	len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
> +			     sizeof(desc) * client->ctx_index);
> +	if (len != sizeof(desc))
> +		return -EFAULT;
> +

We may cache the vmap of context pool for its life cycle to avoid these 
copies. That is why a generic vmap helper function is really nice to have.

Alex
> +	client->doorbell_id = new_id;
> +	if (new_id == GUC_INVALID_DOORBELL_ID)
> +		return 0;
> +
> +	/* Activate the new doorbell */
> +	set_bit(client->doorbell_id, doorbell_bitmap);
> +	doorbell->db_status = GUC_DOORBELL_ENABLED;
> +	doorbell->cookie = 0;
> +	return host2guc_allocate_doorbell(client->guc, client);
> +}
> +
>   static void guc_init_doorbell(struct intel_guc *guc,
> -			      struct i915_guc_client *client)
> +			      struct i915_guc_client *client,
> +			      uint16_t db_id)
>   {
>   	struct guc_doorbell_info *doorbell;
>   	void *base;
> @@ -184,8 +224,7 @@ static void guc_init_doorbell(struct intel_guc *guc,
>   	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
>   	doorbell = base + client->doorbell_offset;
>   
> -	doorbell->db_status = 1;
> -	doorbell->cookie = 0;
> +	guc_update_doorbell_id(client, doorbell, db_id);
>   
>   	kunmap_atomic(base);
>   }
> @@ -193,27 +232,16 @@ static void guc_init_doorbell(struct intel_guc *guc,
>   static void guc_disable_doorbell(struct intel_guc *guc,
>   				 struct i915_guc_client *client)
>   {
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   	struct guc_doorbell_info *doorbell;
>   	void *base;
> -	i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
> -	int value;
>   
>   	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
>   	doorbell = base + client->doorbell_offset;
>   
> -	doorbell->db_status = 0;
> +	guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID);
>   
>   	kunmap_atomic(base);
>   
> -	I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
> -
> -	value = I915_READ(drbreg);
> -	WARN_ON((value & GEN8_DRB_VALID) != 0);
> -
> -	I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
> -	I915_WRITE(drbreg, 0);
> -
>   	/* XXX: wait for any interrupts */
>   	/* XXX: wait for workqueue to drain */
>   }
> @@ -260,7 +288,7 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
>   	if (id == end)
>   		id = GUC_INVALID_DOORBELL_ID;
>   	else
> -		bitmap_set(guc->doorbell_bitmap, id, 1);
> +		set_bit(id, guc->doorbell_bitmap);
>   
>   	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
>   			hi_pri ? "high" : "normal", id);
> @@ -268,11 +296,6 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
>   	return id;
>   }
>   
> -static void release_doorbell(struct intel_guc *guc, uint16_t id)
> -{
> -	bitmap_clear(guc->doorbell_bitmap, id, 1);
> -}
> -
>   /*
>    * Initialise the process descriptor shared with the GuC firmware.
>    */
> @@ -667,15 +690,7 @@ static void guc_client_free(struct drm_device *dev,
>   	if (!client)
>   		return;
>   
> -	if (client->doorbell_id != GUC_INVALID_DOORBELL_ID) {
> -		/*
> -		 * First disable the doorbell, then tell the GuC we've
> -		 * finished with it, finally deallocate it in our bitmap
> -		 */
> -		guc_disable_doorbell(guc, client);
> -		host2guc_release_doorbell(guc, client);
> -		release_doorbell(guc, client->doorbell_id);
> -	}
> +	guc_disable_doorbell(guc, client);
>   
>   	/*
>   	 * XXX: wait for any outstanding submissions before freeing memory.
> @@ -712,6 +727,7 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc *guc = &dev_priv->guc;
>   	struct drm_i915_gem_object *obj;
> +	uint16_t db_id;
>   
>   	client = kzalloc(sizeof(*client), GFP_KERNEL);
>   	if (!client)
> @@ -750,22 +766,24 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>   	else
>   		client->proc_desc_offset = (GUC_DB_SIZE / 2);
>   
> -	client->doorbell_id = assign_doorbell(guc, client->priority);
> -	if (client->doorbell_id == GUC_INVALID_DOORBELL_ID)
> +	db_id = assign_doorbell(guc, client->priority);
> +	if (db_id == GUC_INVALID_DOORBELL_ID)
>   		/* XXX: evict a doorbell instead */
>   		goto err;
>   
>   	guc_init_proc_desc(guc, client);
>   	guc_init_ctx_desc(guc, client);
> -	guc_init_doorbell(guc, client);
> +	guc_init_doorbell(guc, client, db_id);
>   
>   	/* XXX: Any cache flushes needed? General domain mgmt calls? */
>   
>   	if (host2guc_allocate_doorbell(guc, client))
>   		goto err;
>   
> -	DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id %u\n",
> -		priority, client, client->ctx_index, client->doorbell_id);
> +	DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u\n",
> +		priority, client, client->ctx_index);
> +	DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n",
> +		client->doorbell_id, client->doorbell_offset);
>   
>   	return client;
>   

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

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

[Dave Gordon]

On 07/04/16 22:26, Yu Dai wrote:
>
> On 04/07/2016 10:21 AM, Dave Gordon wrote:
>> During a hibernate/resume cycle, the driver, the GuC, and the doorbell
>> hardware can end up in inconsistent states. This patch refactors the
>> driver's handling and tracking of doorbells, in preparation for a later
>> one which will resolve the issue.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 88
>> ++++++++++++++++++------------
>>   1 file changed, 53 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 2171759..2fc69f1 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -175,8 +175,48 @@ static int host2guc_sample_forcewake(struct
>> intel_guc *guc,
>>    * client object which contains the page being used for the doorbell
>>    */
>> +static int guc_update_doorbell_id(struct i915_guc_client *client,
>> +                  struct guc_doorbell_info *doorbell,
>> +                  u16 new_id)
>> +{
>> +    struct sg_table *sg = client->guc->ctx_pool_obj->pages;
>> +    void *doorbell_bitmap = client->guc->doorbell_bitmap;
>> +    struct guc_context_desc desc;
>> +    size_t len;
>> +
>> +    if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
>> +        test_bit(client->doorbell_id, doorbell_bitmap)) {
>> +        /* Deactivate the old doorbell */
>> +        doorbell->db_status = GUC_DOORBELL_DISABLED;
>> +        (void)host2guc_release_doorbell(client->guc, client);
>> +        clear_bit(client->doorbell_id, doorbell_bitmap);
>> +    }
>> +
>> +    /* Update the GuC's idea of the doorbell ID */
>> +    len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
>> +                 sizeof(desc) * client->ctx_index);
>> +    if (len != sizeof(desc))
>> +        return -EFAULT;
>> +    desc.db_id = new_id;
>> +    len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
>> +                 sizeof(desc) * client->ctx_index);
>> +    if (len != sizeof(desc))
>> +        return -EFAULT;
>> +
>
> We may cache the vmap of context pool for its life cycle to avoid these
> copies. That is why a generic vmap helper function is really nice to have.
>
> Alex

Yes, I'm hoping we'll make some progress with that now that Chris has 
merged some new vmap code.

Meanwhile can you please review all of the patches in this series?
Thanks,
.Dave.

>> +    client->doorbell_id = new_id;
>> +    if (new_id == GUC_INVALID_DOORBELL_ID)
>> +        return 0;
>> +
>> +    /* Activate the new doorbell */
>> +    set_bit(client->doorbell_id, doorbell_bitmap);
>> +    doorbell->db_status = GUC_DOORBELL_ENABLED;
>> +    doorbell->cookie = 0;
>> +    return host2guc_allocate_doorbell(client->guc, client);
>> +}
>> +
>>   static void guc_init_doorbell(struct intel_guc *guc,
>> -                  struct i915_guc_client *client)
>> +                  struct i915_guc_client *client,
>> +                  uint16_t db_id)
>>   {
>>       struct guc_doorbell_info *doorbell;
>>       void *base;
>> @@ -184,8 +224,7 @@ static void guc_init_doorbell(struct intel_guc *guc,
>>       base = kmap_atomic(i915_gem_object_get_page(client->client_obj,
>> 0));
>>       doorbell = base + client->doorbell_offset;
>> -    doorbell->db_status = 1;
>> -    doorbell->cookie = 0;
>> +    guc_update_doorbell_id(client, doorbell, db_id);
>>       kunmap_atomic(base);
>>   }
>> @@ -193,27 +232,16 @@ static void guc_init_doorbell(struct intel_guc
>> *guc,
>>   static void guc_disable_doorbell(struct intel_guc *guc,
>>                    struct i915_guc_client *client)
>>   {
>> -    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>       struct guc_doorbell_info *doorbell;
>>       void *base;
>> -    i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
>> -    int value;
>>       base = kmap_atomic(i915_gem_object_get_page(client->client_obj,
>> 0));
>>       doorbell = base + client->doorbell_offset;
>> -    doorbell->db_status = 0;
>> +    guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID);
>>       kunmap_atomic(base);
>> -    I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
>> -
>> -    value = I915_READ(drbreg);
>> -    WARN_ON((value & GEN8_DRB_VALID) != 0);
>> -
>> -    I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
>> -    I915_WRITE(drbreg, 0);
>> -
>>       /* XXX: wait for any interrupts */
>>       /* XXX: wait for workqueue to drain */
>>   }
>> @@ -260,7 +288,7 @@ static uint16_t assign_doorbell(struct intel_guc
>> *guc, uint32_t priority)
>>       if (id == end)
>>           id = GUC_INVALID_DOORBELL_ID;
>>       else
>> -        bitmap_set(guc->doorbell_bitmap, id, 1);
>> +        set_bit(id, guc->doorbell_bitmap);
>>       DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
>>               hi_pri ? "high" : "normal", id);
>> @@ -268,11 +296,6 @@ static uint16_t assign_doorbell(struct intel_guc
>> *guc, uint32_t priority)
>>       return id;
>>   }
>> -static void release_doorbell(struct intel_guc *guc, uint16_t id)
>> -{
>> -    bitmap_clear(guc->doorbell_bitmap, id, 1);
>> -}
>> -
>>   /*
>>    * Initialise the process descriptor shared with the GuC firmware.
>>    */
>> @@ -667,15 +690,7 @@ static void guc_client_free(struct drm_device *dev,
>>       if (!client)
>>           return;
>> -    if (client->doorbell_id != GUC_INVALID_DOORBELL_ID) {
>> -        /*
>> -         * First disable the doorbell, then tell the GuC we've
>> -         * finished with it, finally deallocate it in our bitmap
>> -         */
>> -        guc_disable_doorbell(guc, client);
>> -        host2guc_release_doorbell(guc, client);
>> -        release_doorbell(guc, client->doorbell_id);
>> -    }
>> +    guc_disable_doorbell(guc, client);
>>       /*
>>        * XXX: wait for any outstanding submissions before freeing memory.
>> @@ -712,6 +727,7 @@ static struct i915_guc_client
>> *guc_client_alloc(struct drm_device *dev,
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       struct intel_guc *guc = &dev_priv->guc;
>>       struct drm_i915_gem_object *obj;
>> +    uint16_t db_id;
>>       client = kzalloc(sizeof(*client), GFP_KERNEL);
>>       if (!client)
>> @@ -750,22 +766,24 @@ static struct i915_guc_client
>> *guc_client_alloc(struct drm_device *dev,
>>       else
>>           client->proc_desc_offset = (GUC_DB_SIZE / 2);
>> -    client->doorbell_id = assign_doorbell(guc, client->priority);
>> -    if (client->doorbell_id == GUC_INVALID_DOORBELL_ID)
>> +    db_id = assign_doorbell(guc, client->priority);
>> +    if (db_id == GUC_INVALID_DOORBELL_ID)
>>           /* XXX: evict a doorbell instead */
>>           goto err;
>>       guc_init_proc_desc(guc, client);
>>       guc_init_ctx_desc(guc, client);
>> -    guc_init_doorbell(guc, client);
>> +    guc_init_doorbell(guc, client, db_id);
>>       /* XXX: Any cache flushes needed? General domain mgmt calls? */
>>       if (host2guc_allocate_doorbell(guc, client))
>>           goto err;
>> -    DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id
>> %u\n",
>> -        priority, client, client->ctx_index, client->doorbell_id);
>> +    DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u\n",
>> +        priority, client, client->ctx_index);
>> +    DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n",
>> +        client->doorbell_id, client->doorbell_offset);
>>       return client;
>

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

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

[Dave Gordon]

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

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

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

This patch can be removed if/when the GuC firmware is updated so that it
(re)initialises the doorbell hardware after every firmware (re)load.

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

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

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

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

[Yu Dai]

On 04/07/2016 10:21 AM, Dave Gordon wrote:
> During a hibernate/resume cycle, the whole system is reset, including
> the GuC and the doorbell hardware. Then the system is booted up, drivers
> are loaded, etc -- the GuC firmware may be loaded and set running at this
> point. But then, the booted kernel is replaced by the hibernated image,
> and this resumed kernel will also try to reload the GuC firmware (which
> will fail). To recover, we reset the GuC and try again (which should
> work). But this GuC reset doesn't also reset the doorbell hardware, so
> it can be left in a state inconsistent with that assumed by the driver
> and the GuC.
>
> It would be better if the GuC reset also cleared all doorbell state,
> but that's not how the hardware currently works; also, the driver cannot
> directly reprogram the doorbell hardware (only the GuC can do that).
>
> So this patch cycles through all doorbells, assigning and releasing each
> in turn, so that all the doorbell hardware is left in a consistent state,
> no matter how it was programmed by the previously-running kernel and/or
> GuC firmware.
>
> This patch can be removed if/when the GuC firmware is updated so that it
> (re)initialises the doorbell hardware after every firmware (re)load.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 46 +++++++++++++++++++++++++++++-
>   1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 2fc69f1..f466eab 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -707,6 +707,50 @@ static void guc_client_free(struct drm_device *dev,
>   	kfree(client);
>   }
>   
> +/*
> + * Borrow the first client to set up & tear down every doorbell
> + * in turn, to ensure that all doorbell h/w is (re)initialised.
> + */
> +static void guc_init_doorbell_hw(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct i915_guc_client *client = guc->execbuf_client;
> +	struct guc_doorbell_info *doorbell;
> +	uint16_t db_id, i;
> +	void *base;
> +	int ret;
> +
> +	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
> +	doorbell = base + client->doorbell_offset;
> +	db_id = client->doorbell_id;
> +
> +	for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
> +		i915_reg_t drbreg = GEN8_DRBREGL(i);
> +		u32 value = I915_READ(drbreg);
> +
> +		ret = guc_update_doorbell_id(client, doorbell, i);
> +
> +		if (((value & GUC_DOORBELL_ENABLED) && (i != db_id)) || ret)
> +			DRM_DEBUG_DRIVER("Doorbell reg 0x%x was 0x%x, ret %d\n",
> +				drbreg.reg, value, ret);
> +	}
> +
> +	/* Restore to original value */
> +	guc_update_doorbell_id(client, doorbell, db_id);
> +
> +	for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
> +		i915_reg_t drbreg = GEN8_DRBREGL(i);
> +		u32 value = I915_READ(drbreg);
> +
> +		if ((value & GUC_DOORBELL_ENABLED) && (i != db_id))
> +			DRM_DEBUG_DRIVER("Doorbell reg 0x%x finally 0x%x\n",
> +						drbreg.reg, value);
> +
> +	}
> +

The for loop above is not needed. It can be merged into previous loop by 
print out new drbreg value (read it again after update_doorbell_id).

At this point, only need to check if db_id is correctly enabled or not 
by print out I915_READ(GEN8_DRBREGL(db_id)).

Alex

> +	kunmap_atomic(base);
> +}
> +
>   /**
>    * guc_client_alloc() - Allocate an i915_guc_client
>    * @dev:	drm device
> @@ -971,8 +1015,8 @@ int i915_guc_submission_enable(struct drm_device *dev)
>   	}
>   
>   	guc->execbuf_client = client;
> -
>   	host2guc_sample_forcewake(guc, client);
> +	guc_init_doorbell_hw(guc);
>   
>   	return 0;
>   }

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

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

[Dave Gordon]

On 13/04/16 18:50, Yu Dai wrote:
>
>
> On 04/07/2016 10:21 AM, Dave Gordon wrote:
>> During a hibernate/resume cycle, the whole system is reset, including
>> the GuC and the doorbell hardware. Then the system is booted up, drivers
>> are loaded, etc -- the GuC firmware may be loaded and set running at this
>> point. But then, the booted kernel is replaced by the hibernated image,
>> and this resumed kernel will also try to reload the GuC firmware (which
>> will fail). To recover, we reset the GuC and try again (which should
>> work). But this GuC reset doesn't also reset the doorbell hardware, so
>> it can be left in a state inconsistent with that assumed by the driver
>> and the GuC.
>>
>> It would be better if the GuC reset also cleared all doorbell state,
>> but that's not how the hardware currently works; also, the driver cannot
>> directly reprogram the doorbell hardware (only the GuC can do that).
>>
>> So this patch cycles through all doorbells, assigning and releasing each
>> in turn, so that all the doorbell hardware is left in a consistent state,
>> no matter how it was programmed by the previously-running kernel and/or
>> GuC firmware.
>>
>> This patch can be removed if/when the GuC firmware is updated so that it
>> (re)initialises the doorbell hardware after every firmware (re)load.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 46
>> +++++++++++++++++++++++++++++-
>>   1 file changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 2fc69f1..f466eab 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -707,6 +707,50 @@ static void guc_client_free(struct drm_device *dev,
>>       kfree(client);
>>   }
>> +/*
>> + * Borrow the first client to set up & tear down every doorbell
>> + * in turn, to ensure that all doorbell h/w is (re)initialised.
>> + */
>> +static void guc_init_doorbell_hw(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +    struct i915_guc_client *client = guc->execbuf_client;
>> +    struct guc_doorbell_info *doorbell;
>> +    uint16_t db_id, i;
>> +    void *base;
>> +    int ret;
>> +
>> +    base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
>> +    doorbell = base + client->doorbell_offset;
>> +    db_id = client->doorbell_id;
>> +
>> +    for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
>> +        i915_reg_t drbreg = GEN8_DRBREGL(i);
>> +        u32 value = I915_READ(drbreg);
>> +
>> +        ret = guc_update_doorbell_id(client, doorbell, i);
>> +
>> +        if (((value & GUC_DOORBELL_ENABLED) && (i != db_id)) || ret)
>> +            DRM_DEBUG_DRIVER("Doorbell reg 0x%x was 0x%x, ret %d\n",
>> +                drbreg.reg, value, ret);
>> +    }
>> +
>> +    /* Restore to original value */
>> +    guc_update_doorbell_id(client, doorbell, db_id);
>> +
>> +    for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
>> +        i915_reg_t drbreg = GEN8_DRBREGL(i);
>> +        u32 value = I915_READ(drbreg);
>> +
>> +        if ((value & GUC_DOORBELL_ENABLED) && (i != db_id))
>> +            DRM_DEBUG_DRIVER("Doorbell reg 0x%x finally 0x%x\n",
>> +                        drbreg.reg, value);
>> +
>> +    }
>> +
>
> The for loop above is not needed. It can be merged into previous loop by
> print out new drbreg value (read it again after update_doorbell_id).
>
> At this point, only need to check if db_id is correctly enabled or not
> by print out I915_READ(GEN8_DRBREGL(db_id)).
>
> Alex

No, the idea is not to check that the GuC call has *enabled* each 
selected doorbell, but to check that after the end of the first loop, 
and the subsequent restore, all *other* doorbells have been *disabled*. 
We're only *selecting* each doorbell so that we can then *deselect* it 
as a side effect of selecting the next one!

Hence separate loop required ...

.Dave.

>> +    kunmap_atomic(base);
>> +}
>> +
>>   /**
>>    * guc_client_alloc() - Allocate an i915_guc_client
>>    * @dev:    drm device
>> @@ -971,8 +1015,8 @@ int i915_guc_submission_enable(struct drm_device
>> *dev)
>>       }
>>       guc->execbuf_client = client;
>> -
>>       host2guc_sample_forcewake(guc, client);
>> +    guc_init_doorbell_hw(guc);
>>       return 0;
>>   }
>

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

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

[Yu Dai]

On 04/13/2016 12:46 PM, Dave Gordon wrote:
> On 13/04/16 18:50, Yu Dai wrote:
> >
> >
> > On 04/07/2016 10:21 AM, Dave Gordon wrote:
> >> During a hibernate/resume cycle, the whole system is reset, including
> >> the GuC and the doorbell hardware. Then the system is booted up, drivers
> >> are loaded, etc -- the GuC firmware may be loaded and set running at this
> >> point. But then, the booted kernel is replaced by the hibernated image,
> >> and this resumed kernel will also try to reload the GuC firmware (which
> >> will fail). To recover, we reset the GuC and try again (which should
> >> work). But this GuC reset doesn't also reset the doorbell hardware, so
> >> it can be left in a state inconsistent with that assumed by the driver
> >> and the GuC.
> >>
> >> It would be better if the GuC reset also cleared all doorbell state,
> >> but that's not how the hardware currently works; also, the driver cannot
> >> directly reprogram the doorbell hardware (only the GuC can do that).
> >>
> >> So this patch cycles through all doorbells, assigning and releasing each
> >> in turn, so that all the doorbell hardware is left in a consistent state,
> >> no matter how it was programmed by the previously-running kernel and/or
> >> GuC firmware.
> >>
> >> This patch can be removed if/when the GuC firmware is updated so that it
> >> (re)initialises the doorbell hardware after every firmware (re)load.
> >>
> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_guc_submission.c | 46
> >> +++++++++++++++++++++++++++++-
> >>   1 file changed, 45 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> >> b/drivers/gpu/drm/i915/i915_guc_submission.c
> >> index 2fc69f1..f466eab 100644
> >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> >> @@ -707,6 +707,50 @@ static void guc_client_free(struct drm_device *dev,
> >>       kfree(client);
> >>   }
> >> +/*
> >> + * Borrow the first client to set up & tear down every doorbell
> >> + * in turn, to ensure that all doorbell h/w is (re)initialised.
> >> + */
> >> +static void guc_init_doorbell_hw(struct intel_guc *guc)
> >> +{
> >> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
> >> +    struct i915_guc_client *client = guc->execbuf_client;
> >> +    struct guc_doorbell_info *doorbell;
> >> +    uint16_t db_id, i;
> >> +    void *base;
> >> +    int ret;
> >> +
> >> +    base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
> >> +    doorbell = base + client->doorbell_offset;
> >> +    db_id = client->doorbell_id;
> >> +
> >> +    for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
> >> +        i915_reg_t drbreg = GEN8_DRBREGL(i);
> >> +        u32 value = I915_READ(drbreg);
> >> +
> >> +        ret = guc_update_doorbell_id(client, doorbell, i);
> >> +
> >> +        if (((value & GUC_DOORBELL_ENABLED) && (i != db_id)) || ret)
> >> +            DRM_DEBUG_DRIVER("Doorbell reg 0x%x was 0x%x, ret %d\n",
> >> +                drbreg.reg, value, ret);
> >> +    }
> >> +
> >> +    /* Restore to original value */
> >> +    guc_update_doorbell_id(client, doorbell, db_id);
> >> +
> >> +    for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
> >> +        i915_reg_t drbreg = GEN8_DRBREGL(i);
> >> +        u32 value = I915_READ(drbreg);
> >> +
> >> +        if ((value & GUC_DOORBELL_ENABLED) && (i != db_id))
> >> +            DRM_DEBUG_DRIVER("Doorbell reg 0x%x finally 0x%x\n",
> >> +                        drbreg.reg, value);
> >> +
> >> +    }
> >> +
> >
> > The for loop above is not needed. It can be merged into previous loop by
> > print out new drbreg value (read it again after update_doorbell_id).
> >
> > At this point, only need to check if db_id is correctly enabled or not
> > by print out I915_READ(GEN8_DRBREGL(db_id)).
> >
> > Alex
>
> No, the idea is not to check that the GuC call has *enabled* each
> selected doorbell, but to check that after the end of the first loop,
> and the subsequent restore, all *other* doorbells have been *disabled*.
> We're only *selecting* each doorbell so that we can then *deselect* it
> as a side effect of selecting the next one!
>
> Hence separate loop required ...
>
> .Dave.

This still can be done by backup of previous client->doorbell_id. If it 
is not same as the desired db_id, then make sure it is *disabled* after 
the update.

The real problem here, at least not for now, is that it assumes there is 
only one guc_client. In future, if there is user created guc_client, the 
code doesn't restore doorbell for it.

Alex

> >> +    kunmap_atomic(base);
> >> +}
> >> +
> >>   /**
> >>    * guc_client_alloc() - Allocate an i915_guc_client
> >>    * @dev:    drm device
> >> @@ -971,8 +1015,8 @@ int i915_guc_submission_enable(struct drm_device
> >> *dev)
> >>       }
> >>       guc->execbuf_client = client;
> >> -
> >>       host2guc_sample_forcewake(guc, client);
> >> +    guc_init_doorbell_hw(guc);
> >>       return 0;
> >>   }
> >
>

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

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

[Dave Gordon]

On 13/04/16 21:13, Yu Dai wrote:
> On 04/13/2016 12:46 PM, Dave Gordon wrote:
>> On 13/04/16 18:50, Yu Dai wrote:
>> >
>> > On 04/07/2016 10:21 AM, Dave Gordon wrote:
>> >> During a hibernate/resume cycle, the whole system is reset, including
>> >> the GuC and the doorbell hardware. Then the system is booted up,
>> drivers
>> >> are loaded, etc -- the GuC firmware may be loaded and set running
>> at this
>> >> point. But then, the booted kernel is replaced by the hibernated
>> image,
>> >> and this resumed kernel will also try to reload the GuC firmware
>> (which
>> >> will fail). To recover, we reset the GuC and try again (which should
>> >> work). But this GuC reset doesn't also reset the doorbell hardware, so
>> >> it can be left in a state inconsistent with that assumed by the driver
>> >> and the GuC.
>> >>
>> >> It would be better if the GuC reset also cleared all doorbell state,
>> >> but that's not how the hardware currently works; also, the driver
>> cannot
>> >> directly reprogram the doorbell hardware (only the GuC can do that).
>> >>
>> >> So this patch cycles through all doorbells, assigning and releasing
>> each
>> >> in turn, so that all the doorbell hardware is left in a consistent
>> state,
>> >> no matter how it was programmed by the previously-running kernel
>> and/or
>> >> GuC firmware.
>> >>
>> >> This patch can be removed if/when the GuC firmware is updated so
>> that it
>> >> (re)initialises the doorbell hardware after every firmware (re)load.
>> >>
>> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> >> ---
>> >>   drivers/gpu/drm/i915/i915_guc_submission.c | 46
>> >> +++++++++++++++++++++++++++++-
>> >>   1 file changed, 45 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> >> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> >> index 2fc69f1..f466eab 100644
>> >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> >> @@ -707,6 +707,50 @@ static void guc_client_free(struct drm_device
>> *dev,
>> >>       kfree(client);
>> >>   }
>> >> +/*
>> >> + * Borrow the first client to set up & tear down every doorbell
>> >> + * in turn, to ensure that all doorbell h/w is (re)initialised.
>> >> + */
>> >> +static void guc_init_doorbell_hw(struct intel_guc *guc)
>> >> +{
>> >> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> >> +    struct i915_guc_client *client = guc->execbuf_client;
>> >> +    struct guc_doorbell_info *doorbell;
>> >> +    uint16_t db_id, i;
>> >> +    void *base;
>> >> +    int ret;
>> >> +
>> >> +    base =
>> kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
>> >> +    doorbell = base + client->doorbell_offset;
>> >> +    db_id = client->doorbell_id;
>> >> +
>> >> +    for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
>> >> +        i915_reg_t drbreg = GEN8_DRBREGL(i);
>> >> +        u32 value = I915_READ(drbreg);
>> >> +
>> >> +        ret = guc_update_doorbell_id(client, doorbell, i);
>> >> +
>> >> +        if (((value & GUC_DOORBELL_ENABLED) && (i != db_id)) || ret)
>> >> +            DRM_DEBUG_DRIVER("Doorbell reg 0x%x was 0x%x, ret %d\n",
>> >> +                drbreg.reg, value, ret);
>> >> +    }
>> >> +
>> >> +    /* Restore to original value */
>> >> +    guc_update_doorbell_id(client, doorbell, db_id);
>> >> +
>> >> +    for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
>> >> +        i915_reg_t drbreg = GEN8_DRBREGL(i);
>> >> +        u32 value = I915_READ(drbreg);
>> >> +
>> >> +        if ((value & GUC_DOORBELL_ENABLED) && (i != db_id))
>> >> +            DRM_DEBUG_DRIVER("Doorbell reg 0x%x finally 0x%x\n",
>> >> +                        drbreg.reg, value);
>> >> +
>> >> +    }
>> >> +
>> >
>> > The for loop above is not needed. It can be merged into previous
>> loop by
>> > print out new drbreg value (read it again after update_doorbell_id).
>> >
>> > At this point, only need to check if db_id is correctly enabled or not
>> > by print out I915_READ(GEN8_DRBREGL(db_id)).
>> >
>> > Alex
>>
>> No, the idea is not to check that the GuC call has *enabled* each
>> selected doorbell, but to check that after the end of the first loop,
>> and the subsequent restore, all *other* doorbells have been *disabled*.
>> We're only *selecting* each doorbell so that we can then *deselect* it
>> as a side effect of selecting the next one!
>>
>> Hence separate loop required ...
>>
>> .Dave.
>
> This still can be done by backup of previous client->doorbell_id. If it
> is not same as the desired db_id, then make sure it is *disabled* after
> the update.

No, I also want to check that there's no crosstalk, so that after having 
swept the entire space, there's exactly ONE doorbell enabled, and it's 
the one we most recently associated with the client.

> The real problem here, at least not for now, is that it assumes there is
> only one guc_client. In future, if there is user created guc_client, the
> code doesn't restore doorbell for it.
>
> Alex

By definition there is only one (newly-created) client at the point 
where this is called. I'd have done this first, but we need a client for 
talking to the GuC! Any additional clients (e.g. for preemption) will be 
created afterwards.

This entire function wouldn't be necessary if the GuC would guarantee to 
clear all doorbells at startup, or if the hardware provide a RESET 
signal for the doorbell block :(

BTW the persistent-kmap patch should be merged today, so I'll have to 
update the above to get rid of the kmap_atomic() above.

.Dave.

>> >> +    kunmap_atomic(base);
>> >> +}
>> >> +
>> >>   /**
>> >>    * guc_client_alloc() - Allocate an i915_guc_client
>> >>    * @dev:    drm device
>> >> @@ -971,8 +1015,8 @@ int i915_guc_submission_enable(struct drm_device
>> >> *dev)
>> >>       }
>> >>       guc->execbuf_client = client;
>> >> -
>> >>       host2guc_sample_forcewake(guc, client);
>> >> +    guc_init_doorbell_hw(guc);
>> >>       return 0;
>> >>   }

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

[PATCH v4 5/6] drm/i915/guc: disable GuC submission earlier during GuC (re)load

[Dave Gordon]

When resetting and reloading the GuC, the GuC submission management code
also needs to destroy and recreate the GuC client(s). Currently this is
done by a separate call from the GuC loader, but really, it's just an
internal detail of the submission code. So here we remove the call from
the loader (which is too late, really, because the GuC has already been
reloaded at this point) and put it into guc_submission_init() instead.
This means that any preexisting client is destroyed *before* the GuC
(re)load and then recreated after, iff the firmware was successfully
loaded. If the GuC reload fails, we don't recreate the client, so fallback
to execlists mode (if active) won't leak the client object (previously,
the now-unusable client would have been left allocated, and leaked if
the driver were unloaded).

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index f466eab..a8717f7 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -981,6 +981,10 @@ int i915_guc_submission_init(struct drm_device *dev)
 	const size_t gemsize = round_up(poolsize, PAGE_SIZE);
 	struct intel_guc *guc = &dev_priv->guc;
 
+	/* Wipe bitmap & delete client in case of reinitialisation */
+	bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS);
+	i915_guc_submission_disable(dev);
+
 	if (!i915.enable_guc_submission)
 		return 0; /* not enabled  */
 
@@ -992,9 +996,7 @@ int i915_guc_submission_init(struct drm_device *dev)
 		return -ENOMEM;
 
 	ida_init(&guc->ctx_ids);
-
 	guc_create_log(guc);
-
 	guc_create_ads(guc);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 876e5da..3e14a9a 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -470,9 +470,6 @@ int intel_guc_ucode_load(struct drm_device *dev)
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
 	if (i915.enable_guc_submission) {
-		/* The execbuf_client will be recreated. Release it first. */
-		i915_guc_submission_disable(dev);
-
 		err = i915_guc_submission_enable(dev);
 		if (err)
 			goto fail;
-- 
1.9.1

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

Re: [PATCH v4 5/6] drm/i915/guc: disable GuC submission earlier during GuC (re)load

[Yu Dai]

LGTM.

Reviewed-by: Alex Dai <yu.dai@intel.com>

On 04/07/2016 10:21 AM, Dave Gordon wrote:
> When resetting and reloading the GuC, the GuC submission management code
> also needs to destroy and recreate the GuC client(s). Currently this is
> done by a separate call from the GuC loader, but really, it's just an
> internal detail of the submission code. So here we remove the call from
> the loader (which is too late, really, because the GuC has already been
> reloaded at this point) and put it into guc_submission_init() instead.
> This means that any preexisting client is destroyed *before* the GuC
> (re)load and then recreated after, iff the firmware was successfully
> loaded. If the GuC reload fails, we don't recreate the client, so fallback
> to execlists mode (if active) won't leak the client object (previously,
> the now-unusable client would have been left allocated, and leaked if
> the driver were unloaded).
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++++--
>   drivers/gpu/drm/i915/intel_guc_loader.c    | 3 ---
>   2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index f466eab..a8717f7 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -981,6 +981,10 @@ int i915_guc_submission_init(struct drm_device *dev)
>   	const size_t gemsize = round_up(poolsize, PAGE_SIZE);
>   	struct intel_guc *guc = &dev_priv->guc;
>   
> +	/* Wipe bitmap & delete client in case of reinitialisation */
> +	bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS);
> +	i915_guc_submission_disable(dev);
> +
>   	if (!i915.enable_guc_submission)
>   		return 0; /* not enabled  */
>   
> @@ -992,9 +996,7 @@ int i915_guc_submission_init(struct drm_device *dev)
>   		return -ENOMEM;
>   
>   	ida_init(&guc->ctx_ids);
> -
>   	guc_create_log(guc);
> -
>   	guc_create_ads(guc);
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 876e5da..3e14a9a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -470,9 +470,6 @@ int intel_guc_ucode_load(struct drm_device *dev)
>   		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>   
>   	if (i915.enable_guc_submission) {
> -		/* The execbuf_client will be recreated. Release it first. */
> -		i915_guc_submission_disable(dev);
> -
>   		err = i915_guc_submission_enable(dev);
>   		if (err)
>   			goto fail;

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

[PATCH v4 6/6] DO NOT MERGE: add enable_guc_loading parameter

[Dave Gordon]

Split the function of "enable_guc_submission" into two separate options.
The new one "enable_guc_loading" controls only the *fetching and loading*
of the GuC firmware image. The existing one is redefined to control only
the *use* of the GuC for batch submission once the firmware is loaded.

In addition, the degree of control has been refined from a simple bool
to an integer key, allowing several options:
 -1  (default)    whatever the platform default is
  0  DISABLE      don't load/use the GuC
  1  BEST EFFORT  try to load/use the GuC, fallback if not available
  2  REQUIRE      must load/use the GuC, else leave the GPU wedged

The new platform default (as coded here) will be to attempt to load
the GuC iff the device has a GuC that requires firmware, to attempt to
use it iff the device has a GuC that supports the submission protocol
(with or without firmware), and to fall back to execlist mode if any
required firmware cannot be found or fails to load.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |  1 -
 drivers/gpu/drm/i915/i915_params.c      | 14 ++++-
 drivers/gpu/drm/i915/i915_params.h      |  3 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 98 ++++++++++++++++++---------------
 4 files changed, 67 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b342f67..7a5fdfb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4887,7 +4887,6 @@ int i915_gem_init_engines(struct drm_device *dev)
 		ret = intel_guc_ucode_load(dev);
 		if (ret) {
 			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
-			ret = -EIO;
 			goto out;
 		}
 	}
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 1779f02..21f325b 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -54,7 +54,8 @@ struct i915_params i915 __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
-	.enable_guc_submission = false,
+	.enable_guc_loading = -1,
+	.enable_guc_submission = -1,
 	.guc_log_level = -1,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
@@ -197,8 +198,15 @@ struct i915_params i915 __read_mostly = {
 		 "(0=use value from vbt [default], 1=low power swing(200mV),"
 		 "2=default swing(400mV))");
 
-module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400);
-MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)");
+module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
+MODULE_PARM_DESC(enable_guc_submission,
+		"Enable GuC firmware loading "
+		"(-1=auto [default], 0=never, 1=if available, 2=required)");
+
+module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
+MODULE_PARM_DESC(enable_guc_submission,
+		"Enable GuC submission "
+		"(-1=auto [default], 0=never, 1=if available, 2=required)");
 
 module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
 MODULE_PARM_DESC(guc_log_level,
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 02bc278..9f1d17b 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -45,6 +45,8 @@ struct i915_params {
 	int enable_ips;
 	int invert_brightness;
 	int enable_cmd_parser;
+	int enable_guc_loading;
+	int enable_guc_submission;
 	int guc_log_level;
 	int use_mmio_flip;
 	int mmio_debug;
@@ -57,7 +59,6 @@ struct i915_params {
 	bool load_detect_test;
 	bool reset;
 	bool disable_display;
-	bool enable_guc_submission;
 	bool verbose_state_checks;
 	bool nuclear_pageflip;
 	bool enable_dp_mst;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 3e14a9a..7f54278 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -387,49 +387,37 @@ int intel_guc_ucode_load(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
+	const char *fw_path = guc_fw->guc_fw_path;
 	int retries, err = 0;
 
-	if (!i915.enable_guc_submission)
-		return 0;
-
-	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
+	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
+		fw_path,
 		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
-	direct_interrupts_to_host(dev_priv);
+	/* Loading forbidden, or no firmware to load? */
+	if (!i915.enable_guc_loading)
+		goto fail;
+	if (fw_path == NULL)
+		goto fail;
+	if (*fw_path == '\0') {
+		DRM_ERROR("No GuC firmware known for this platform\n");
+		goto fail;
+	}
 
-	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE)
-		return 0;
+	/* Fetch failed, or already fetched but failed to load? */
+	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS)
+		goto fail;
+	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
+		goto fail;
 
-	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS &&
-	    guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
-		return -ENOEXEC;
+	direct_interrupts_to_host(dev_priv);
 
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
 
-	DRM_DEBUG_DRIVER("GuC fw fetch status %s\n",
-		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
-
-	switch (guc_fw->guc_fw_fetch_status) {
-	case GUC_FIRMWARE_FAIL:
-		/* something went wrong :( */
-		err = -EIO;
-		goto fail;
-
-	case GUC_FIRMWARE_NONE:
-	case GUC_FIRMWARE_PENDING:
-	default:
-		/* "can't happen" */
-		WARN_ONCE(1, "GuC fw %s invalid guc_fw_fetch_status %s [%d]\n",
-			guc_fw->guc_fw_path,
-			intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
-			guc_fw->guc_fw_fetch_status);
-		err = -ENXIO;
-		goto fail;
-
-	case GUC_FIRMWARE_SUCCESS:
-		break;
-	}
+	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
+		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
+		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
 	err = i915_guc_submission_init(dev);
 	if (err)
@@ -480,6 +468,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
 
 fail:
 	DRM_ERROR("GuC firmware load failed, err %d\n", err);
+
 	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
 		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
 
@@ -487,6 +476,29 @@ int intel_guc_ucode_load(struct drm_device *dev)
 	i915_guc_submission_disable(dev);
 	i915_guc_submission_fini(dev);
 
+	/*
+	 * We've failed to load the firmware :(
+	 *
+	 * Decide whether to disable GuC submission and fall back to
+	 * execlist mode, and whether to hide the error by returning
+	 * zero or to return -EIO, which the caller will treat as a
+	 * nonfatal error (i.e. it doesn't prevent driver load, but
+	 * marks the GPU as wedged until reset).
+	 */
+	if (i915.enable_guc_loading > 1) {
+		err = -EIO;
+	} else if (HAS_GUC_SCHED(dev) && !HAS_GUC_UCODE(dev)) {
+		return 0;
+	} else if (i915.enable_guc_submission > 1) {
+		err = -EIO;
+	} else {
+		err = 0;
+	}
+
+	i915.enable_guc_submission = 0;
+
+	DRM_DEBUG_DRIVER("falling back to execlist mode, err %d\n", err);
+
 	return err;
 }
 
@@ -628,8 +640,11 @@ void intel_guc_ucode_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
-	if (!HAS_GUC_SCHED(dev))
-		i915.enable_guc_submission = false;
+	/* A negative value means "use platform default" */
+	if (i915.enable_guc_loading < 0)
+		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
+	if (i915.enable_guc_submission < 0)
+		i915.enable_guc_submission = HAS_GUC_SCHED(dev);
 
 	if (!HAS_GUC_UCODE(dev)) {
 		fw_path = NULL;
@@ -638,26 +653,21 @@ void intel_guc_ucode_init(struct drm_device *dev)
 		guc_fw->guc_fw_major_wanted = 6;
 		guc_fw->guc_fw_minor_wanted = 1;
 	} else {
-		i915.enable_guc_submission = false;
 		fw_path = "";	/* unknown device */
 	}
 
-	if (!i915.enable_guc_submission)
-		return;
-
 	guc_fw->guc_dev = dev;
 	guc_fw->guc_fw_path = fw_path;
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
 
+	/* Early (and silent) return if GuC loading is disabled */
+	if (!i915.enable_guc_loading)
+		return;
 	if (fw_path == NULL)
 		return;
-
-	if (*fw_path == '\0') {
-		DRM_ERROR("No GuC firmware known for this platform\n");
-		guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
+	if (*fw_path == '\0')
 		return;
-	}
 
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
 	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
-- 
1.9.1

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

✗ Fi.CI.BAT: failure for Fixes and workarounds for GuC/doorbell setup

[Patchwork]

== Series Details ==

Series: Fixes and workarounds for GuC/doorbell setup
URL   : https://patchwork.freedesktop.org/series/5426/
State : failure

== Summary ==

Series 5426v1 Fixes and workarounds for GuC/doorbell setup
http://patchwork.freedesktop.org/api/1.0/series/5426/revisions/1/mbox/

Test drv_getparams_basic:
        Subgroup basic-subslice-total:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test drv_module_reload_basic:
                incomplete -> PASS       (bsw-nuc-2)
Test gem_basic:
        Subgroup bad-close:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup create-close:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup create-fd-close:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_ctx_param_basic:
        Subgroup basic:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup invalid-ctx-set:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup invalid-param-get:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup invalid-param-set:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup root-set:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_ctx_switch:
        Subgroup basic-default:
                skip       -> PASS       (bsw-nuc-2)
                pass       -> DMESG-WARN (skl-i7k-2)
Test gem_exec_basic:
        Subgroup basic-blt:
                skip       -> PASS       (bsw-nuc-2)
        Subgroup gtt-bsd:
                skip       -> PASS       (bsw-nuc-2)
        Subgroup readonly-bsd:
                skip       -> PASS       (bsw-nuc-2)
        Subgroup readonly-render:
                skip       -> PASS       (bsw-nuc-2)
        Subgroup readonly-vebox:
                skip       -> PASS       (bsw-nuc-2)
Test gem_exec_store:
        Subgroup basic-bsd:
                skip       -> PASS       (bsw-nuc-2)
        Subgroup basic-default:
                skip       -> PASS       (bsw-nuc-2)
        Subgroup basic-vebox:
                skip       -> PASS       (bsw-nuc-2)
Test gem_exec_whisper:
        Subgroup basic:
                skip       -> PASS       (bsw-nuc-2)
Test gem_mmap:
        Subgroup basic-small-bo:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_mmap_gtt:
        Subgroup basic-write:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup basic-write-gtt:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_ringfill:
        Subgroup basic-default-hang:
                skip       -> PASS       (bsw-nuc-2)
Test gem_sync:
        Subgroup basic-blt:
                skip       -> PASS       (bsw-nuc-2)
        Subgroup basic-vebox:
                skip       -> PASS       (bsw-nuc-2)
Test gem_tiled_pread_basic:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test kms_addfb_basic:
        Subgroup addfb25-modifier-no-flag:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup addfb25-y-tiled:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup bad-pitch-65536:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup basic-x-tiled:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup framebuffer-vs-set-tiling:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup too-wide:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                dmesg-fail -> PASS       (bsw-nuc-2)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-c-frame-sequence:
                dmesg-fail -> PASS       (bsw-nuc-2)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test prime_self_import:
        Subgroup basic-with_fd_dup:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup basic-with_one_bo_two_files:
                dmesg-warn -> PASS       (bsw-nuc-2)

bdw-nuci7        total:196  pass:184  dwarn:0   dfail:0   fail:0   skip:12 
bsw-nuc-2        total:196  pass:159  dwarn:0   dfail:0   fail:0   skip:37 
byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:196  pass:179  dwarn:0   dfail:0   fail:0   skip:17 
skl-i7k-2        total:196  pass:172  dwarn:1   dfail:0   fail:0   skip:23 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
BOOT FAILED for bdw-ultra
BOOT FAILED for ilk-hp8440p
BOOT FAILED for ivb-t430s

Results at /archive/results/CI_IGT_test/Patchwork_1837/

851708c7e97537ed618fadbe5d342eaf8fa5146d drm-intel-nightly: 2016y-04m-07d-13h-56m-00s UTC integration manifest
a9619071522d956fc6ff3aa5b5208a692f258842 DO NOT MERGE: add enable_guc_loading parameter
64bea5d51362b9a78a1686291e680c86ef957678 drm/i915/guc: disable GuC submission earlier during GuC (re)load
ee186c856e0469d5f0ec3993e2bd58c2ce961e16 drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission
79449d6f59a9d725e868b5dee7527d8e4555a7af drm/i915/guc: refactor doorbell management code
f4279359c18d037494c7f9e41bbe7f4c2af647b5 drm/i915/guc: move guc_ring_doorbell() nearer to callsite
6d14828ccf541c8593072e25e718573c6f5095b1 drm/i915/guc: add doorbell map to debugfs/i915_guc_info

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

Re: ✗ Fi.CI.BAT: failure for Fixes and workarounds for GuC/doorbell setup

[Dave Gordon]

On 08/04/16 08:59, Patchwork wrote:
> gem_ctx_switch:
>          Subgroup basic-default:
>                  skip       -> PASS       (bsw-nuc-2)
>                  pass       -> DMESG-WARN (skl-i7k-2)

Known bug:
https://bugs.freedesktop.org/show_bug.cgi?id=93847
"GuC is calling a sleeping function in atomic context"
Fix is in progress, should be merged today.

Everything else looks good :)

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