[PATCH v3 1/6] drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED

[PATCH v3 1/6] drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED

[Dave Gordon]

For now, anything with a GuC requires uCode loading, and then supports
command submission once loaded. But these are logically distinct from
simply "having a GuC", so we need a separate macro for the latter. Then,
the test in intel_guc_reset() should depend only on whether the GuC
exists, not whether we're trying to use it for submission.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     | 10 ++++++++--
 drivers/gpu/drm/i915/intel_uncore.c |  2 +-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d5496ab..1142cb3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2704,8 +2704,14 @@ struct drm_i915_cmd_table {
 
 #define HAS_CSR(dev)	(IS_GEN9(dev))
 
-#define HAS_GUC_UCODE(dev)	(IS_GEN9(dev) && !IS_KABYLAKE(dev))
-#define HAS_GUC_SCHED(dev)	(IS_GEN9(dev) && !IS_KABYLAKE(dev))
+/*
+ * For now, anything with a GuC requires uCode loading, and then supports
+ * command submission once loaded. But these are logically independent
+ * properties, so we have separate macros to test them.
+ */
+#define HAS_GUC(dev)		(IS_GEN9(dev) && !IS_KABYLAKE(dev))
+#define HAS_GUC_UCODE(dev)	(HAS_GUC(dev))
+#define HAS_GUC_SCHED(dev)	(HAS_GUC(dev))
 
 #define HAS_RESOURCE_STREAMER(dev) (IS_HASWELL(dev) || \
 				    INTEL_INFO(dev)->gen >= 8)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 4f1dfe6..cdd6397 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(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 v3 2/6] drm/i915/guc: 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 ++++++++++++++++--------------
 5 files changed, 69 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a88e6c9..a1f5330 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4880,7 +4880,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 a304b0e..e6af672 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 383c076..6a5578c 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,
@@ -198,8 +199,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 65e73dd..1323261 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);
-- 
1.9.1

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

[PATCH v3 3/6] drm/i915/guc: pass request (not client) to i915_guc_{wq_check_space, submit}()

[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.
And all the information the GuC code needs should be encapsulated in (or
reachable from) the request.

v3:
    GEM_BUG_ON() for bad GuC client (Tvrtko Ursulin).
    Add/update kerneldoc explaining check_space/submit protocol

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index e6af672..9e30762 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -451,14 +451,30 @@ 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)
+/**
+ * i915_guc_wq_check_space() - check that the GuC can accept a request
+ * @request:	request associated with the commands
+ *
+ * Return:	0 if space is available
+ *		-EAGAIN if space is not currently available
+ *
+ * This function must be called (and must return 0) before a request
+ * is submitted to the GuC via i915_guc_submit() below. Once a result
+ * of 0 has been returned, it remains valid until (but only until)
+ * the next call to submit().
+ *
+ * This precheck allows the caller to determine in advance that space
+ * will be available for the next submission before committing resources
+ * to it, and helps avoid late failures with complicated recovery paths.
+ */
+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)
-		return 0;
+	GEM_BUG_ON(gc == NULL);
 
 	desc = gc->client_base + gc->proc_desc_offset;
 
@@ -532,16 +548,28 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 
 /**
  * i915_guc_submit() - Submit commands through GuC
- * @client:	the guc client where commands will go through
  * @rq:		request associated with the commands
  *
- * Return:	0 if succeed
+ * Return:	0 on success, otherwise an errno.
+ * 		(Note: nonzero really shouldn't happen!)
+ *
+ * The caller must have already called i915_guc_wq_check_space() above
+ * with a result of 0 (success) since the last request submission. This
+ * guarantees that there is space in the work queue for the new request,
+ * so enqueuing the item cannot fail.
+ *
+ * Bad Things Will Happen if the caller violates this protocol e.g. calls
+ * submit() when check() says there's no space, or calls submit() multiple
+ * times with no intervening check().
+ *
+ * The only error here arises if the doorbell hardware isn't functioning
+ * as expected, which really shouln't happen.
  */
-int i915_guc_submit(struct i915_guc_client *client,
-		    struct drm_i915_gem_request *rq)
+int i915_guc_submit(struct drm_i915_gem_request *rq)
 {
-	struct intel_guc *guc = client->guc;
 	unsigned int engine_id = rq->engine->guc_id;
+	struct intel_guc *guc = &rq->i915->guc;
+	struct i915_guc_client *client = guc->execbuf_client;
 	int q_ret, b_ret;
 
 	q_ret = guc_add_workqueue_item(client, rq);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 9d79c4c..b37c731 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -148,10 +148,9 @@ extern int intel_guc_resume(struct drm_device *dev);
 /* i915_guc_submission.c */
 int i915_guc_submission_init(struct drm_device *dev);
 int i915_guc_submission_enable(struct drm_device *dev);
-int i915_guc_submit(struct i915_guc_client *client,
-		    struct drm_i915_gem_request *rq);
+int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
+int i915_guc_submit(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);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d876352..79302e1 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -698,9 +698,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;
 	}
@@ -749,7 +747,6 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
 {
 	struct intel_ringbuffer *ringbuf = request->ringbuf;
-	struct drm_i915_private *dev_priv = request->i915;
 	struct intel_engine_cs *engine = request->engine;
 
 	intel_logical_ring_advance(ringbuf);
@@ -777,8 +774,8 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 	request->previous_context = engine->last_context;
 	engine->last_context = request->ctx;
 
-	if (dev_priv->guc.execbuf_client)
-		i915_guc_submit(dev_priv->guc.execbuf_client, request);
+	if (i915.enable_guc_submission)
+		i915_guc_submit(request);
 	else
 		execlists_context_queue(request);
 
-- 
1.9.1

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

[PATCH v3 4/6] drm/i915/guc: don't spinwait if the GuC's workqueue is full

[Dave Gordon]

Rather than wait to see whether more space becomes available in the GuC
submission workqueue, we can just return -EAGAIN and let the caller try
again in a little while. This gets rid of an uninterruptable sleep in
the polling code :)

We'll also add a counter to the GuC client statistics, to see how often
we find the WQ full.

v3:
    Flag the likely() code path (Tvtrko Ursulin).

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 | 18 +++++++-----------
 drivers/gpu/drm/i915/intel_guc.h           |  8 ++++----
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6ad008c..9e215e6 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2506,6 +2506,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 9e30762..21d3603 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -469,26 +469,22 @@ 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;
+	u32 freespace;
 
 	GEM_BUG_ON(gc == NULL);
 
 	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;
-		}
+	freespace = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
+	if (likely(freespace >= wqi_size))
+		return 0;
 
-		if (timeout_counter)
-			usleep_range(1000, 2000);
-	};
+	gc->no_wq_space += 1;
 
-	return ret;
+	return -EAGAIN;
 }
 
 static int guc_add_workqueue_item(struct i915_guc_client *gc,
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index b37c731..436f2d6 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

[PATCH v3 5/6] drm/i915/guc: rework guc_add_workqueue_item()

[Dave Gordon]

Mostly little optimisations and future-proofing against code breakage.
For instance, if the driver is correctly following the submission
protocol, the "out of space" condition is impossible, so the previous
runtime WARN_ON() is promoted to a GEM_BUG_ON() for a more dramatic
effect in development and less impact in end-user systems.

Similarly we can make alignment checking more stringent and replace
other WARN_ON() conditions that don't relate to the runtime hardware
state with either BUILD_BUG_ON() for compile-time-detectable issues, or
GEM_BUG_ON() for logical "can't happen" errors.

With those changes, we can convert it to void, as suggested by Chris
Wilson, and update the calling code appropriately.

v3:
  Note that we're now putting the request seqno in the "fence_id" field
  of each GuC-work-item, in case it turns up somewhere useful (Tvrtko).

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 21d3603..79897b0 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -487,23 +487,28 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
 	return -EAGAIN;
 }
 
-static int guc_add_workqueue_item(struct i915_guc_client *gc,
-				  struct drm_i915_gem_request *rq)
+static void guc_add_workqueue_item(struct i915_guc_client *gc,
+				   struct drm_i915_gem_request *rq)
 {
+	/* wqi_len is in DWords, and does not include the one-word header */
+	const size_t wqi_size = sizeof(struct guc_wq_item);
+	const u32 wqi_len = wqi_size/sizeof(u32) - 1;
 	struct guc_process_desc *desc;
 	struct guc_wq_item *wqi;
 	void *base;
-	u32 tail, wq_len, wq_off, space;
+	u32 freespace, tail, wq_off, wq_page;
 
 	desc = gc->client_base + gc->proc_desc_offset;
-	space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
-	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;
+	/* Free space is guaranteed, see i915_guc_wq_check_space() above */
+	freespace = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
+	GEM_BUG_ON(freespace < wqi_size);
+
+	/* The GuC firmware wants the tail index in QWords, not bytes */
+	tail = rq->tail;
+	GEM_BUG_ON(tail & 7);
+	tail >>= 3;
+	GEM_BUG_ON(tail > WQ_RING_TAIL_MAX);
 
 	/* 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
@@ -512,19 +517,23 @@ 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;
+	gc->wq_tail += wqi_size;
+	gc->wq_tail &= gc->wq_size - 1;
+	GEM_BUG_ON(wq_off & (wqi_size - 1));
 
-	/* wq starts from the page after doorbell / process_desc */
-	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj,
-			(wq_off + GUC_DB_SIZE) >> PAGE_SHIFT));
+	/* WQ starts from the page after doorbell / process_desc */
+	wq_page = (wq_off + GUC_DB_SIZE) >> PAGE_SHIFT;
 	wq_off &= PAGE_SIZE - 1;
+	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, wq_page));
 	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;
+	/* Now fill in the 4-word work queue item */
 	wqi->header = WQ_TYPE_INORDER |
-			(wq_len << WQ_LEN_SHIFT) |
+			(wqi_len << WQ_LEN_SHIFT) |
 			(rq->engine->guc_id << WQ_TARGET_SHIFT) |
 			WQ_NO_WCFLUSH_WAIT;
 
@@ -532,14 +541,10 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 	wqi->context_desc = (u32)intel_lr_context_descriptor(rq->ctx,
 							     rq->engine);
 
-	/* The GuC firmware wants the tail index in QWords, not bytes */
-	tail = rq->ringbuf->tail >> 3;
 	wqi->ring_tail = tail << WQ_RING_TAIL_SHIFT;
-	wqi->fence_id = 0; /*XXX: what fence to be here */
+	wqi->fence_id = rq->seqno;
 
 	kunmap_atomic(base);
-
-	return 0;
 }
 
 /**
@@ -566,26 +571,20 @@ int i915_guc_submit(struct drm_i915_gem_request *rq)
 	unsigned int engine_id = rq->engine->guc_id;
 	struct intel_guc *guc = &rq->i915->guc;
 	struct i915_guc_client *client = guc->execbuf_client;
-	int q_ret, b_ret;
+	int b_ret;
 
-	q_ret = guc_add_workqueue_item(client, rq);
-	if (q_ret == 0)
-		b_ret = guc_ring_doorbell(client);
+	guc_add_workqueue_item(client, rq);
+	b_ret = guc_ring_doorbell(client);
 
 	client->submissions[engine_id] += 1;
-	if (q_ret) {
-		client->q_fail += 1;
-		client->retcode = q_ret;
-	} else if (b_ret) {
+	client->retcode = b_ret;
+	if (b_ret)
 		client->b_fail += 1;
-		client->retcode = q_ret = b_ret;
-	} else {
-		client->retcode = 0;
-	}
+
 	guc->submissions[engine_id] += 1;
 	guc->last_seqno[engine_id] = rq->seqno;
 
-	return q_ret;
+	return b_ret;
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 436f2d6..10e1d5e 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -74,7 +74,7 @@ struct i915_guc_client {
 	/* GuC submission statistics & status */
 	uint64_t submissions[GUC_MAX_ENGINES_NUM];
 	uint32_t no_wq_space;		/* Space pre-check failed	*/
-	uint32_t q_fail;		/* Failed to queue (MBZ)	*/
+	uint32_t q_fail;		/* No longer used		*/
 	uint32_t b_fail;		/* Doorbell failure (MBZ)	*/
 	int retcode;			/* Result of last guc_submit()	*/
 };
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 2de57ff..944786d 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -71,7 +71,8 @@
 #define   WQ_WORKLOAD_TOUCH		(2 << WQ_WORKLOAD_SHIFT)
 
 #define WQ_RING_TAIL_SHIFT		20
-#define WQ_RING_TAIL_MASK		(0x7FF << WQ_RING_TAIL_SHIFT)
+#define WQ_RING_TAIL_MAX		0x7FF	/* 2^11 QWords */
+#define WQ_RING_TAIL_MASK		(WQ_RING_TAIL_MAX << WQ_RING_TAIL_SHIFT)
 
 #define GUC_DOORBELL_ENABLED		1
 #define GUC_DOORBELL_DISABLED		0
-- 
1.9.1

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

[PATCH v3 6/6] drm/i915/guc: 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 6a5578c..573e787 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,
@@ -207,7 +207,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

✗ Fi.CI.BAT: warning for series starting with [v3,1/6] drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/6] drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED
URL   : https://patchwork.freedesktop.org/series/6846/
State : warning

== Summary ==

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

Test drv_hangman:
        Subgroup error-state-basic:
                incomplete -> PASS       (snb-dellxps)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-b:
                skip       -> PASS       (skl-nuci5)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                pass       -> SKIP       (skl-nuci5)
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                pass       -> SKIP       (hsw-brixbox)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> SKIP       (hsw-brixbox)
        Subgroup suspend-read-crc-pipe-c:
                skip       -> PASS       (hsw-brixbox)

bdw-nuci7-2      total:219  pass:206  dwarn:0   dfail:0   fail:0   skip:13 
bdw-ultra        total:219  pass:193  dwarn:0   dfail:0   fail:0   skip:26 
bsw-nuc-2        total:218  pass:174  dwarn:0   dfail:0   fail:2   skip:42 
byt-nuc          total:218  pass:174  dwarn:0   dfail:0   fail:3   skip:41 
hsw-brixbox      total:219  pass:191  dwarn:0   dfail:0   fail:0   skip:28 
hsw-gt2          total:219  pass:197  dwarn:0   dfail:0   fail:1   skip:21 
ilk-hp8440p      total:219  pass:155  dwarn:0   dfail:0   fail:1   skip:63 
ivb-t430s        total:219  pass:188  dwarn:0   dfail:0   fail:0   skip:31 
skl-i7k-2        total:219  pass:191  dwarn:0   dfail:0   fail:0   skip:28 
skl-nuci5        total:219  pass:206  dwarn:0   dfail:0   fail:0   skip:13 
snb-dellxps      total:201  pass:164  dwarn:1   dfail:0   fail:0   skip:35 
snb-x220t        total:219  pass:176  dwarn:0   dfail:0   fail:1   skip:42 

Results at /archive/results/CI_IGT_test/Patchwork_2154/

bcc6a843e7e4a3f4794b90dbefb00174171365bd drm-intel-nightly: 2016y-05m-09d-12h-47m-46s UTC integration manifest
deeff5b drm/i915/guc: change default to using GuC submission if possible
e0ae188 drm/i915/guc: rework guc_add_workqueue_item()
60730a9 drm/i915/guc: don't spinwait if the GuC's workqueue is full
5497bcb drm/i915/guc: pass request (not client) to i915_guc_{wq_check_space, submit}()
e0056b2 drm/i915/guc: add enable_guc_loading parameter
45cf4ee drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED

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

Re: [PATCH v3 1/6] drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED

[Tvrtko Ursulin]

On 06/05/16 20:31, Dave Gordon wrote:
> For now, anything with a GuC requires uCode loading, and then supports
> command submission once loaded. But these are logically distinct from
> simply "having a GuC", so we need a separate macro for the latter. Then,
> the test in intel_guc_reset() should depend only on whether the GuC
> exists, not whether we're trying to use it for submission.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h     | 10 ++++++++--
>   drivers/gpu/drm/i915/intel_uncore.c |  2 +-
>   2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d5496ab..1142cb3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2704,8 +2704,14 @@ struct drm_i915_cmd_table {
>
>   #define HAS_CSR(dev)	(IS_GEN9(dev))
>
> -#define HAS_GUC_UCODE(dev)	(IS_GEN9(dev) && !IS_KABYLAKE(dev))
> -#define HAS_GUC_SCHED(dev)	(IS_GEN9(dev) && !IS_KABYLAKE(dev))
> +/*
> + * For now, anything with a GuC requires uCode loading, and then supports
> + * command submission once loaded. But these are logically independent
> + * properties, so we have separate macros to test them.
> + */
> +#define HAS_GUC(dev)		(IS_GEN9(dev) && !IS_KABYLAKE(dev))
> +#define HAS_GUC_UCODE(dev)	(HAS_GUC(dev))
> +#define HAS_GUC_SCHED(dev)	(HAS_GUC(dev))
>
>   #define HAS_RESOURCE_STREAMER(dev) (IS_HASWELL(dev) || \
>   				    INTEL_INFO(dev)->gen >= 8)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 4f1dfe6..cdd6397 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(dev_priv))
>   		return -EINVAL;
>
>   	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH v3 3/6] drm/i915/guc: pass request (not client) to i915_guc_{wq_check_space, submit}()

[Tvrtko Ursulin]

On 06/05/16 20:31, Dave Gordon wrote:
> 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.
> And all the information the GuC code needs should be encapsulated in (or
> reachable from) the request.
>
> v3:
>      GEM_BUG_ON() for bad GuC client (Tvrtko Ursulin).
>      Add/update kerneldoc explaining check_space/submit protocol
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 46 ++++++++++++++++++++++++------
>   drivers/gpu/drm/i915/intel_guc.h           |  5 ++--
>   drivers/gpu/drm/i915/intel_lrc.c           |  9 ++----
>   3 files changed, 42 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index e6af672..9e30762 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -451,14 +451,30 @@ 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)
> +/**
> + * i915_guc_wq_check_space() - check that the GuC can accept a request
> + * @request:	request associated with the commands
> + *
> + * Return:	0 if space is available
> + *		-EAGAIN if space is not currently available
> + *
> + * This function must be called (and must return 0) before a request
> + * is submitted to the GuC via i915_guc_submit() below. Once a result
> + * of 0 has been returned, it remains valid until (but only until)
> + * the next call to submit().
> + *
> + * This precheck allows the caller to determine in advance that space
> + * will be available for the next submission before committing resources
> + * to it, and helps avoid late failures with complicated recovery paths.
> + */
> +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)
> -		return 0;
> +	GEM_BUG_ON(gc == NULL);
>
>   	desc = gc->client_base + gc->proc_desc_offset;
>
> @@ -532,16 +548,28 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
>
>   /**
>    * i915_guc_submit() - Submit commands through GuC
> - * @client:	the guc client where commands will go through
>    * @rq:		request associated with the commands
>    *
> - * Return:	0 if succeed
> + * Return:	0 on success, otherwise an errno.
> + * 		(Note: nonzero really shouldn't happen!)
> + *
> + * The caller must have already called i915_guc_wq_check_space() above
> + * with a result of 0 (success) since the last request submission. This
> + * guarantees that there is space in the work queue for the new request,
> + * so enqueuing the item cannot fail.
> + *
> + * Bad Things Will Happen if the caller violates this protocol e.g. calls
> + * submit() when check() says there's no space, or calls submit() multiple
> + * times with no intervening check().
> + *
> + * The only error here arises if the doorbell hardware isn't functioning
> + * as expected, which really shouln't happen.
>    */
> -int i915_guc_submit(struct i915_guc_client *client,
> -		    struct drm_i915_gem_request *rq)
> +int i915_guc_submit(struct drm_i915_gem_request *rq)
>   {
> -	struct intel_guc *guc = client->guc;
>   	unsigned int engine_id = rq->engine->guc_id;
> +	struct intel_guc *guc = &rq->i915->guc;
> +	struct i915_guc_client *client = guc->execbuf_client;
>   	int q_ret, b_ret;
>
>   	q_ret = guc_add_workqueue_item(client, rq);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 9d79c4c..b37c731 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -148,10 +148,9 @@ extern int intel_guc_resume(struct drm_device *dev);
>   /* i915_guc_submission.c */
>   int i915_guc_submission_init(struct drm_device *dev);
>   int i915_guc_submission_enable(struct drm_device *dev);
> -int i915_guc_submit(struct i915_guc_client *client,
> -		    struct drm_i915_gem_request *rq);
> +int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
> +int i915_guc_submit(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);
>
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d876352..79302e1 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -698,9 +698,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;
>   	}
> @@ -749,7 +747,6 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>   intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
>   {
>   	struct intel_ringbuffer *ringbuf = request->ringbuf;
> -	struct drm_i915_private *dev_priv = request->i915;
>   	struct intel_engine_cs *engine = request->engine;
>
>   	intel_logical_ring_advance(ringbuf);
> @@ -777,8 +774,8 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>   	request->previous_context = engine->last_context;
>   	engine->last_context = request->ctx;
>
> -	if (dev_priv->guc.execbuf_client)
> -		i915_guc_submit(dev_priv->guc.execbuf_client, request);
> +	if (i915.enable_guc_submission)
> +		i915_guc_submit(request);
>   	else
>   		execlists_context_queue(request);
>
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH v3 5/6] drm/i915/guc: rework guc_add_workqueue_item()

[Tvrtko Ursulin]

On 06/05/16 20:31, Dave Gordon wrote:
> Mostly little optimisations and future-proofing against code breakage.
> For instance, if the driver is correctly following the submission
> protocol, the "out of space" condition is impossible, so the previous
> runtime WARN_ON() is promoted to a GEM_BUG_ON() for a more dramatic
> effect in development and less impact in end-user systems.
>
> Similarly we can make alignment checking more stringent and replace
> other WARN_ON() conditions that don't relate to the runtime hardware
> state with either BUILD_BUG_ON() for compile-time-detectable issues, or
> GEM_BUG_ON() for logical "can't happen" errors.
>
> With those changes, we can convert it to void, as suggested by Chris
> Wilson, and update the calling code appropriately.
>
> v3:
>    Note that we're now putting the request seqno in the "fence_id" field
>    of each GuC-work-item, in case it turns up somewhere useful (Tvrtko).
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 71 +++++++++++++++---------------
>   drivers/gpu/drm/i915/intel_guc.h           |  2 +-
>   drivers/gpu/drm/i915/intel_guc_fwif.h      |  3 +-
>   3 files changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 21d3603..79897b0 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -487,23 +487,28 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
>   	return -EAGAIN;
>   }
>
> -static int guc_add_workqueue_item(struct i915_guc_client *gc,
> -				  struct drm_i915_gem_request *rq)
> +static void guc_add_workqueue_item(struct i915_guc_client *gc,
> +				   struct drm_i915_gem_request *rq)
>   {
> +	/* wqi_len is in DWords, and does not include the one-word header */
> +	const size_t wqi_size = sizeof(struct guc_wq_item);
> +	const u32 wqi_len = wqi_size/sizeof(u32) - 1;
>   	struct guc_process_desc *desc;
>   	struct guc_wq_item *wqi;
>   	void *base;
> -	u32 tail, wq_len, wq_off, space;
> +	u32 freespace, tail, wq_off, wq_page;
>
>   	desc = gc->client_base + gc->proc_desc_offset;
> -	space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
> -	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;
> +	/* Free space is guaranteed, see i915_guc_wq_check_space() above */
> +	freespace = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
> +	GEM_BUG_ON(freespace < wqi_size);
> +
> +	/* The GuC firmware wants the tail index in QWords, not bytes */
> +	tail = rq->tail;
> +	GEM_BUG_ON(tail & 7);
> +	tail >>= 3;
> +	GEM_BUG_ON(tail > WQ_RING_TAIL_MAX);
>
>   	/* 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
> @@ -512,19 +517,23 @@ 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;
> +	gc->wq_tail += wqi_size;
> +	gc->wq_tail &= gc->wq_size - 1;
> +	GEM_BUG_ON(wq_off & (wqi_size - 1));
>
> -	/* wq starts from the page after doorbell / process_desc */
> -	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj,
> -			(wq_off + GUC_DB_SIZE) >> PAGE_SHIFT));
> +	/* WQ starts from the page after doorbell / process_desc */
> +	wq_page = (wq_off + GUC_DB_SIZE) >> PAGE_SHIFT;
>   	wq_off &= PAGE_SIZE - 1;
> +	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, wq_page));
>   	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;
> +	/* Now fill in the 4-word work queue item */
>   	wqi->header = WQ_TYPE_INORDER |
> -			(wq_len << WQ_LEN_SHIFT) |
> +			(wqi_len << WQ_LEN_SHIFT) |
>   			(rq->engine->guc_id << WQ_TARGET_SHIFT) |
>   			WQ_NO_WCFLUSH_WAIT;
>
> @@ -532,14 +541,10 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
>   	wqi->context_desc = (u32)intel_lr_context_descriptor(rq->ctx,
>   							     rq->engine);
>
> -	/* The GuC firmware wants the tail index in QWords, not bytes */
> -	tail = rq->ringbuf->tail >> 3;
>   	wqi->ring_tail = tail << WQ_RING_TAIL_SHIFT;
> -	wqi->fence_id = 0; /*XXX: what fence to be here */
> +	wqi->fence_id = rq->seqno;
>
>   	kunmap_atomic(base);
> -
> -	return 0;
>   }
>
>   /**
> @@ -566,26 +571,20 @@ int i915_guc_submit(struct drm_i915_gem_request *rq)
>   	unsigned int engine_id = rq->engine->guc_id;
>   	struct intel_guc *guc = &rq->i915->guc;
>   	struct i915_guc_client *client = guc->execbuf_client;
> -	int q_ret, b_ret;
> +	int b_ret;
>
> -	q_ret = guc_add_workqueue_item(client, rq);
> -	if (q_ret == 0)
> -		b_ret = guc_ring_doorbell(client);
> +	guc_add_workqueue_item(client, rq);
> +	b_ret = guc_ring_doorbell(client);
>
>   	client->submissions[engine_id] += 1;
> -	if (q_ret) {
> -		client->q_fail += 1;
> -		client->retcode = q_ret;
> -	} else if (b_ret) {
> +	client->retcode = b_ret;
> +	if (b_ret)
>   		client->b_fail += 1;
> -		client->retcode = q_ret = b_ret;
> -	} else {
> -		client->retcode = 0;
> -	}
> +
>   	guc->submissions[engine_id] += 1;
>   	guc->last_seqno[engine_id] = rq->seqno;
>
> -	return q_ret;
> +	return b_ret;
>   }
>
>   /*
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 436f2d6..10e1d5e 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -74,7 +74,7 @@ struct i915_guc_client {
>   	/* GuC submission statistics & status */
>   	uint64_t submissions[GUC_MAX_ENGINES_NUM];
>   	uint32_t no_wq_space;		/* Space pre-check failed	*/
> -	uint32_t q_fail;		/* Failed to queue (MBZ)	*/
> +	uint32_t q_fail;		/* No longer used		*/
>   	uint32_t b_fail;		/* Doorbell failure (MBZ)	*/
>   	int retcode;			/* Result of last guc_submit()	*/
>   };
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 2de57ff..944786d 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -71,7 +71,8 @@
>   #define   WQ_WORKLOAD_TOUCH		(2 << WQ_WORKLOAD_SHIFT)
>
>   #define WQ_RING_TAIL_SHIFT		20
> -#define WQ_RING_TAIL_MASK		(0x7FF << WQ_RING_TAIL_SHIFT)
> +#define WQ_RING_TAIL_MAX		0x7FF	/* 2^11 QWords */
> +#define WQ_RING_TAIL_MASK		(WQ_RING_TAIL_MAX << WQ_RING_TAIL_SHIFT)
>
>   #define GUC_DOORBELL_ENABLED		1
>   #define GUC_DOORBELL_DISABLED		0
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH v3 6/6] drm/i915/guc: change default to using GuC submission if possible

[Tvrtko Ursulin]

On 06/05/16 20:31, Dave Gordon wrote:
> 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 6a5578c..573e787 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,
> @@ -207,7 +207,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,
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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