[PATCH 1/4] CI_ONLY: add enable_guc_loading parameter

[PATCH 1/4] CI_ONLY: 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,
but not yet to use it for submission. A later patch will change
to enable it if appropriate.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |  1 -
 drivers/gpu/drm/i915/i915_guc_submission.c |  4 +-
 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 ++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_uncore.c        |  2 +-
 6 files changed, 70 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 261a3ef..1ea4546 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4914,7 +4914,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_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index d40c13f..a542664 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -970,7 +970,7 @@ int intel_guc_suspend(struct drm_device *dev)
 	struct intel_context *ctx;
 	u32 data[3];
 
-	if (!i915.enable_guc_submission)
+	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
 	ctx = dev_priv->kernel_context;
@@ -996,7 +996,7 @@ int intel_guc_resume(struct drm_device *dev)
 	struct intel_context *ctx;
 	u32 data[3];
 
-	if (!i915.enable_guc_submission)
+	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
 	ctx = dev_priv->kernel_context;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 1779f02..79be9f8 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 = 0,
 	.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_loading,
+		"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, 0=never [default], 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 876e5da..2ec9cf1 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)
@@ -483,6 +471,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;
 
@@ -490,6 +479,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;
 }
 
@@ -631,8 +643,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;
@@ -641,26 +656,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);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 4f1dfe6..df698d7 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1758,7 +1758,7 @@ int intel_guc_reset(struct drm_i915_private *dev_priv)
 	int ret;
 	unsigned long irqflags;
 
-	if (!i915.enable_guc_submission)
+	if (!HAS_GUC_UCODE(dev_priv))
 		return -EINVAL;
 
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
-- 
1.9.1

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

[PATCH 2/4] CI_ONLY: change default to using GuC submission if possible

[Dave Gordon]

This patch simply changes the default value of "enable_guc_submission"
from 0 (never) to -1 (auto). This means that GuC submission will be
used if the platform has a GuC, the GuC supports the request submission
protocol, and any required GuC firmwware was successfully loaded. If any
of these conditions are not met, the driver will fall back to using
execlist mode.

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

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 79be9f8..0df8aa4 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -55,7 +55,7 @@ struct i915_params i915 __read_mostly = {
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
 	.enable_guc_loading = -1,
-	.enable_guc_submission = 0,
+	.enable_guc_submission = -1,
 	.guc_log_level = -1,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
@@ -206,7 +206,7 @@ struct i915_params i915 __read_mostly = {
 module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
 MODULE_PARM_DESC(enable_guc_submission,
 		"Enable GuC submission "
-		"(-1=auto, 0=never [default], 1=if available, 2=required)");
+		"(-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,
-- 
1.9.1

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

[PATCH 3/4] CI_ONLY: pass request (not client) to i915_guc_wq_check_space()

[Dave Gordon]

The knowledge of how to derive the relevant client from the request
should be localised within i915_guc_submission.c; the LRC code shouldn't
have to know about the internal details of the GuC submission process.

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index a542664..dc40a58 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -451,10 +451,11 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
 			     sizeof(desc) * client->ctx_index);
 }
 
-int i915_guc_wq_check_space(struct i915_guc_client *gc)
+int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
 {
+	const size_t size = sizeof(struct guc_wq_item);
+	struct i915_guc_client *gc = request->i915->guc.execbuf_client;
 	struct guc_process_desc *desc;
-	u32 size = sizeof(struct guc_wq_item);
 	int ret = -ETIMEDOUT, timeout_counter = 200;
 
 	if (!gc)
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 9d79c4c..a5cd4af 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -152,6 +152,6 @@ int i915_guc_submit(struct i915_guc_client *client,
 		    struct drm_i915_gem_request *rq);
 void i915_guc_submission_disable(struct drm_device *dev);
 void i915_guc_submission_fini(struct drm_device *dev);
-int i915_guc_wq_check_space(struct i915_guc_client *client);
+int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6179b59..cfb91f0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -708,9 +708,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 		 * going any further, as the i915_add_request() call
 		 * later on mustn't fail ...
 		 */
-		struct intel_guc *guc = &request->i915->guc;
-
-		ret = i915_guc_wq_check_space(guc->execbuf_client);
+		ret = i915_guc_wq_check_space(request);
 		if (ret)
 			return ret;
 	}
-- 
1.9.1

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

[PATCH 4/4] CI_ONLY: DO NOT MERGE: WARN on GuC WQ full

[Dave Gordon]

Let's try not waiting for the GuC WQ to be not-full, but instead
* count how many times this happens
* print a WARNING, at least sometimes
* return -EAGAIN
and see whether the GuC is ever a bottleneck for submission.

.Dave.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |  1 +
 drivers/gpu/drm/i915/i915_guc_submission.c | 37 ++++++++++++++----------------
 drivers/gpu/drm/i915/intel_guc.h           |  8 +++----
 3 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4950d05..d530fba 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2501,6 +2501,7 @@ static void i915_guc_client_info(struct seq_file *m,
 	seq_printf(m, "\tWQ size %d, offset: 0x%x, tail %d\n",
 		client->wq_size, client->wq_offset, client->wq_tail);
 
+	seq_printf(m, "\tWork queue full: %u\n", client->no_wq_space);
 	seq_printf(m, "\tFailed to queue: %u\n", client->q_fail);
 	seq_printf(m, "\tFailed doorbell: %u\n", client->b_fail);
 	seq_printf(m, "\tLast submission result: %d\n", client->retcode);
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index dc40a58..595b5e8 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -453,32 +453,29 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
 
 int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
 {
-	const size_t size = sizeof(struct guc_wq_item);
+	const size_t wqi_size = sizeof(struct guc_wq_item);
 	struct i915_guc_client *gc = request->i915->guc.execbuf_client;
 	struct guc_process_desc *desc;
-	int ret = -ETIMEDOUT, timeout_counter = 200;
 
 	if (!gc)
 		return 0;
 
 	desc = gc->client_base + gc->proc_desc_offset;
 
-	while (timeout_counter-- > 0) {
-		if (CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size) >= size) {
-			ret = 0;
-			break;
-		}
+	if (CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size) >= wqi_size)
+		return 0;
 
-		if (timeout_counter)
-			usleep_range(1000, 2000);
-	};
+	/* A one-in-a-million chance? */
+	WARN(gc->no_wq_space % 0x100000 == 0, "GuC WQ full!");
+	gc->no_wq_space += 1;
 
-	return ret;
+	return -EAGAIN;
 }
 
 static int guc_add_workqueue_item(struct i915_guc_client *gc,
 				  struct drm_i915_gem_request *rq)
 {
+	const size_t wqi_size = sizeof(struct guc_wq_item);
 	struct guc_process_desc *desc;
 	struct guc_wq_item *wqi;
 	void *base;
@@ -489,11 +486,6 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 	if (WARN_ON(space < sizeof(struct guc_wq_item)))
 		return -ENOSPC; /* shouldn't happen */
 
-	/* postincrement WQ tail for next time */
-	wq_off = gc->wq_tail;
-	gc->wq_tail += sizeof(struct guc_wq_item);
-	gc->wq_tail &= gc->wq_size - 1;
-
 	/* For now workqueue item is 4 DWs; workqueue buffer is 2 pages. So we
 	 * should not have the case where structure wqi is across page, neither
 	 * wrapped to the beginning. This simplifies the implementation below.
@@ -501,8 +493,13 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 	 * XXX: if not the case, we need save data to a temp wqi and copy it to
 	 * workqueue buffer dw by dw.
 	 */
-	WARN_ON(sizeof(struct guc_wq_item) != 16);
-	WARN_ON(wq_off & 3);
+	BUILD_BUG_ON(wqi_size != 16);
+
+	/* postincrement WQ tail for next time */
+	wq_off = gc->wq_tail;
+	WARN_ON(wq_off & (wqi_size - 1));
+	gc->wq_tail += wqi_size;
+	gc->wq_tail &= gc->wq_size - 1;
 
 	/* wq starts from the page after doorbell / process_desc */
 	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj,
@@ -510,8 +507,8 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 	wq_off &= PAGE_SIZE - 1;
 	wqi = (struct guc_wq_item *)((char *)base + wq_off);
 
-	/* len does not include the header */
-	wq_len = sizeof(struct guc_wq_item) / sizeof(u32) - 1;
+	/* len is in DWords, and does not include the one-word header */
+	wq_len = wqi_size/sizeof(u32) - 1;
 	wqi->header = WQ_TYPE_INORDER |
 			(wq_len << WQ_LEN_SHIFT) |
 			(rq->engine->guc_id << WQ_TARGET_SHIFT) |
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index a5cd4af..84e8593 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -73,10 +73,10 @@ struct i915_guc_client {
 
 	/* GuC submission statistics & status */
 	uint64_t submissions[GUC_MAX_ENGINES_NUM];
-	uint32_t q_fail;
-	uint32_t b_fail;
-	int retcode;
-	int spare;			/* pad to 32 DWords		*/
+	uint32_t no_wq_space;		/* Space pre-check failed	*/
+	uint32_t q_fail;		/* Failed to queue (MBZ)	*/
+	uint32_t b_fail;		/* Doorbell failure (MBZ)	*/
+	int retcode;			/* Result of last guc_submit()	*/
 };
 
 enum intel_guc_fw_status {
-- 
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 series starting with [1/4] CI_ONLY: add enable_guc_loading parameter

[Patchwork]

== Series Details ==

Series: series starting with [1/4] CI_ONLY: add enable_guc_loading parameter
URL   : https://patchwork.freedesktop.org/series/6268/
State : failure

== Summary ==

Series 6268v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/6268/revisions/1/mbox/

Test drv_hangman:
        Subgroup error-state-basic:
                incomplete -> PASS       (snb-dellxps)
Test drv_module_reload_basic:
                pass       -> FAIL       (snb-dellxps)
Test gem_ctx_switch:
        Subgroup basic-default:
                pass       -> DMESG-WARN (skl-nuci5)
                pass       -> DMESG-WARN (skl-i7k-2)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (bsw-nuc-2)
                pass       -> FAIL       (hsw-gt2)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b-frame-sequence:
                skip       -> PASS       (bdw-nuci7)

bdw-nuci7        total:200  pass:188  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:200  pass:175  dwarn:0   dfail:0   fail:0   skip:25 
bsw-nuc-2        total:199  pass:157  dwarn:0   dfail:0   fail:1   skip:41 
byt-nuc          total:199  pass:155  dwarn:0   dfail:0   fail:0   skip:44 
hsw-brixbox      total:200  pass:174  dwarn:0   dfail:0   fail:0   skip:26 
hsw-gt2          total:200  pass:178  dwarn:0   dfail:0   fail:1   skip:21 
ilk-hp8440p      total:200  pass:137  dwarn:0   dfail:0   fail:0   skip:63 
ivb-t430s        total:200  pass:166  dwarn:0   dfail:0   fail:0   skip:34 
skl-i7k-2        total:200  pass:172  dwarn:1   dfail:0   fail:0   skip:27 
skl-nuci5        total:200  pass:188  dwarn:1   dfail:0   fail:0   skip:11 
snb-dellxps      total:193  pass:154  dwarn:0   dfail:0   fail:1   skip:38 

Results at /archive/results/CI_IGT_test/Patchwork_2064/

f814551aa7232ed36d71244dd148b48660b53a78 drm-intel-nightly: 2016y-04m-25d-11h-36m-27s UTC integration manifest
5e8cac6 CI_ONLY: DO NOT MERGE: WARN on GuC WQ full
8ac46b1 CI_ONLY: pass request (not client) to i915_guc_wq_check_space()
1bcea52 CI_ONLY: change default to using GuC submission if possible
e9ee7d3 CI_ONLY: add enable_guc_loading parameter

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

Re: [PATCH 1/4] CI_ONLY: add enable_guc_loading parameter

[Daniel Vetter]

On Mon, Apr 25, 2016 at 03:37:11PM +0100, Dave Gordon wrote:
> 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,
> but not yet to use it for submission. A later patch will change
> to enable it if appropriate.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

intel-gfx-trybot@fd.o is your CI_only patchwork and CI instance:

https://patchwork.freedesktop.org/project/intel-gfx-trybot/series/?ordering=-last_updated

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem.c            |  1 -
>  drivers/gpu/drm/i915/i915_guc_submission.c |  4 +-
>  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 ++++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_uncore.c        |  2 +-
>  6 files changed, 70 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 261a3ef..1ea4546 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4914,7 +4914,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_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index d40c13f..a542664 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -970,7 +970,7 @@ int intel_guc_suspend(struct drm_device *dev)
>  	struct intel_context *ctx;
>  	u32 data[3];
>  
> -	if (!i915.enable_guc_submission)
> +	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>  		return 0;
>  
>  	ctx = dev_priv->kernel_context;
> @@ -996,7 +996,7 @@ int intel_guc_resume(struct drm_device *dev)
>  	struct intel_context *ctx;
>  	u32 data[3];
>  
> -	if (!i915.enable_guc_submission)
> +	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>  		return 0;
>  
>  	ctx = dev_priv->kernel_context;
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 1779f02..79be9f8 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 = 0,
>  	.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_loading,
> +		"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, 0=never [default], 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 876e5da..2ec9cf1 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)
> @@ -483,6 +471,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;
>  
> @@ -490,6 +479,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;
>  }
>  
> @@ -631,8 +643,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;
> @@ -641,26 +656,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);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 4f1dfe6..df698d7 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1758,7 +1758,7 @@ int intel_guc_reset(struct drm_i915_private *dev_priv)
>  	int ret;
>  	unsigned long irqflags;
>  
> -	if (!i915.enable_guc_submission)
> +	if (!HAS_GUC_UCODE(dev_priv))
>  		return -EINVAL;
>  
>  	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] CI_ONLY: add enable_guc_loading parameter

[Dave Gordon]

On 26/04/16 15:18, Daniel Vetter wrote:
> On Mon, Apr 25, 2016 at 03:37:11PM +0100, Dave Gordon wrote:
>> 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,
>> but not yet to use it for submission. A later patch will change
>> to enable it if appropriate.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>
> intel-gfx-trybot@fd.o is your CI_only patchwork and CI instance:
>
> https://patchwork.freedesktop.org/project/intel-gfx-trybot/series/?ordering=-last_updated
>
> Cheers, Daniel

Yes, I tried sending this to trybot several times before this, but it 
wasn't working well, with many tests giving spurious "INCOMPLETE" 
results, especially on the Romanian machines. So this was still the only 
was to get a good test run.

Hopefully trybot will be back in working order soon, but that link shows 
there has not been a single successful submission by anybody since 1st 
April!

.Dave.


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

Re: [PATCH 1/4] CI_ONLY: add enable_guc_loading parameter

[Daniel Vetter]

On Wed, Apr 27, 2016 at 10:43:59AM +0100, Dave Gordon wrote:
> On 26/04/16 15:18, Daniel Vetter wrote:
> >On Mon, Apr 25, 2016 at 03:37:11PM +0100, Dave Gordon wrote:
> >>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,
> >>but not yet to use it for submission. A later patch will change
> >>to enable it if appropriate.
> >>
> >>Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> >
> >intel-gfx-trybot@fd.o is your CI_only patchwork and CI instance:
> >
> >https://patchwork.freedesktop.org/project/intel-gfx-trybot/series/?ordering=-last_updated
> >
> >Cheers, Daniel
> 
> Yes, I tried sending this to trybot several times before this, but it wasn't
> working well, with many tests giving spurious "INCOMPLETE" results,
> especially on the Romanian machines. So this was still the only was to get a
> good test run.
> 
> Hopefully trybot will be back in working order soon, but that link shows
> there has not been a single successful submission by anybody since 1st
> April!

You need to escalate this all over the place. CI is meant as a service for
developers, if it doesn't work that's pretty critical (and imo much more
important than getting the next feature in).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx