[PATCH v4 0/5] drm/i915/guc: use symbolic names for module parameter values

[PATCH v4 0/5] drm/i915/guc: use symbolic names for module parameter values

[Dave Gordon]

There are various literal constants used in the GuC module-parameter
processing code; this sequence of patches replaces them with symbolic
names for greater clarity.

And then it re-enables GuC submission by default :)

v3:
  Original patch broken into two (1/4 + 2/4)
  Name for GuC log level NONE added (3/4
  Re-enable GuC loading & submission (4/4)
  Added cover letter 

v4:
  Additional final patch (5/5) to change treatment of unrecognised
  option values (ignore them rather than force them into range).

Dave Gordon (5):
  drm/i915/guc: symbolic names for GuC submission preferences
  drm/i915/guc: symbolic names for GuC firmare loading preferences
  drm/i915/guc: symbolic name for GuC log-level none
  drm/i915/guc: use symbolic names in setting defaults for module
  drm/i915/guc: ignore unrecognised loading & submission options

 drivers/gpu/drm/i915/i915_guc_submission.c |  4 +--
 drivers/gpu/drm/i915/i915_params.c         | 10 +++---
 drivers/gpu/drm/i915/intel_guc.h           | 20 +++++++++++
 drivers/gpu/drm/i915/intel_guc_fwif.h      |  1 +
 drivers/gpu/drm/i915/intel_guc_loader.c    | 54 ++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_lrc.c           |  4 +--
 6 files changed, 70 insertions(+), 23 deletions(-)

-- 
1.9.1

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

[PATCH v4 1/5] drm/i915/guc: symbolic names for GuC submission preferences

[Dave Gordon]

The existing code that accesses the "enable_guc_submission"
parameter uses explicit numerical values for the various
possibilities, including in one case relying on boolean 0/1
mapping to specific values (which could be confusing for
maintainers).

So this patch just provides and uses names for the values
representing the DEFAULT, DISABLED, PREFERRED, and MANDATORY
submission options that the user can select (-1, 0, 1, 2
respectively).

This should produce identical code to the previous version!

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c |  2 +-
 drivers/gpu/drm/i915/intel_guc.h           |  6 ++++++
 drivers/gpu/drm/i915/intel_guc_loader.c    | 15 ++++++++-------
 drivers/gpu/drm/i915/intel_lrc.c           |  4 ++--
 4 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 03a5cef..3b9cfa5 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -969,7 +969,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS);
 	i915_guc_submission_disable(dev_priv);
 
-	if (!i915.enable_guc_submission)
+	if (i915.enable_guc_submission == GUC_SUBMISSION_DISABLED)
 		return 0; /* not enabled  */
 
 	if (guc->ctx_pool_obj)
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 623cf26..d15bac8 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -90,6 +90,12 @@ struct i915_guc_client {
 	uint64_t submissions[I915_NUM_ENGINES];
 };
 
+enum {
+	GUC_SUBMISSION_DEFAULT = -1,
+	GUC_SUBMISSION_DISABLED = 0,
+	GUC_SUBMISSION_PREFERRED,
+	GUC_SUBMISSION_MANDATORY
+};
 enum intel_guc_fw_status {
 	GUC_FIRMWARE_FAIL = -1,
 	GUC_FIRMWARE_NONE = 0,
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 3763e30..5128cbd 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -189,7 +189,7 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
 	}
 
 	/* If GuC submission is enabled, set up additional parameters here */
-	if (i915.enable_guc_submission) {
+	if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED) {
 		u32 pgs = i915_gem_obj_ggtt_offset(dev_priv->guc.ctx_pool_obj);
 		u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
 
@@ -495,7 +495,7 @@ int intel_guc_setup(struct drm_device *dev)
 		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
-	if (i915.enable_guc_submission) {
+	if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED) {
 		err = i915_guc_submission_enable(dev_priv);
 		if (err)
 			goto fail;
@@ -523,7 +523,7 @@ int intel_guc_setup(struct drm_device *dev)
 	 */
 	if (i915.enable_guc_loading > 1) {
 		ret = -EIO;
-	} else if (i915.enable_guc_submission > 1) {
+	} else if (i915.enable_guc_submission >= GUC_SUBMISSION_MANDATORY) {
 		ret = -EIO;
 	} else {
 		ret = 0;
@@ -538,7 +538,7 @@ int intel_guc_setup(struct drm_device *dev)
 	else
 		DRM_ERROR("GuC firmware load failed: %d\n", err);
 
-	if (i915.enable_guc_submission) {
+	if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED) {
 		if (fw_path == NULL)
 			DRM_INFO("GuC submission without firmware not supported\n");
 		if (ret == 0)
@@ -546,7 +546,7 @@ int intel_guc_setup(struct drm_device *dev)
 		else
 			DRM_ERROR("GuC init failed: %d\n", ret);
 	}
-	i915.enable_guc_submission = 0;
+	i915.enable_guc_submission = GUC_SUBMISSION_DISABLED;
 
 	return ret;
 }
@@ -690,8 +690,9 @@ void intel_guc_init(struct drm_device *dev)
 	/* 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 (i915.enable_guc_submission <= GUC_SUBMISSION_DEFAULT)
+		i915.enable_guc_submission = HAS_GUC_SCHED(dev) ?
+			GUC_SUBMISSION_PREFERRED : GUC_SUBMISSION_DISABLED;
 
 	if (!HAS_GUC_UCODE(dev)) {
 		fw_path = NULL;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 309c5d9..4196345 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -662,7 +662,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 
 	request->ring = ce->ring;
 
-	if (i915.enable_guc_submission) {
+	if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED) {
 		/*
 		 * Check that the GuC has space for the request before
 		 * going any further, as the i915_add_request() call
@@ -800,7 +800,7 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
 	ce->state->dirty = true;
 
 	/* Invalidate GuC TLB. */
-	if (i915.enable_guc_submission)
+	if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED)
 		I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
 
 	i915_gem_context_get(ctx);
-- 
1.9.1

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

[PATCH v4 2/5] drm/i915/guc: symbolic names for GuC firmare loading preferences

[Dave Gordon]

The existing code that accesses the "enable_guc_loading"
parameter uses explicit numerical values for the various
possibilities,  including in one case relying on boolean
0/1 mapping to specific values (which could be confusing
for maintainers).

So this patch just provides and uses names for the values
representing the DEFAULT, DISABLED, PREFERRED, and MANDATORY
options that the user can select (-1, 0, 1, 2 respectively).

This should produce identical code to the previous version!

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_guc.h        | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_guc_loader.c | 13 +++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index d15bac8..94db252 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -90,12 +90,26 @@ struct i915_guc_client {
 	uint64_t submissions[I915_NUM_ENGINES];
 };
 
+/*
+ * These signed ranges represent user-requested preferences.
+ * Out-of-range values from the user will be clipped towards
+ * zero: any negative value is treated as -1, anything over 2
+ * is just 2. ANY user-supplied value also taints the kernel.
+ */
 enum {
 	GUC_SUBMISSION_DEFAULT = -1,
 	GUC_SUBMISSION_DISABLED = 0,
 	GUC_SUBMISSION_PREFERRED,
 	GUC_SUBMISSION_MANDATORY
 };
+enum {
+	FIRMWARE_LOAD_DEFAULT = -1,
+	FIRMWARE_LOAD_DISABLED = 0,
+	FIRMWARE_LOAD_PREFERRED,
+	FIRMWARE_LOAD_MANDATORY
+};
+
+/* These represent the actual firmware status  */
 enum intel_guc_fw_status {
 	GUC_FIRMWARE_FAIL = -1,
 	GUC_FIRMWARE_NONE = 0,
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 5128cbd..33d9770 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -426,7 +426,7 @@ int intel_guc_setup(struct drm_device *dev)
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
 	/* Loading forbidden, or no firmware to load? */
-	if (!i915.enable_guc_loading) {
+	if (i915.enable_guc_loading == FIRMWARE_LOAD_DISABLED) {
 		err = 0;
 		goto fail;
 	} else if (fw_path == NULL) {
@@ -521,7 +521,7 @@ int intel_guc_setup(struct drm_device *dev)
 	 * nonfatal error (i.e. it doesn't prevent driver load, but
 	 * marks the GPU as wedged until reset).
 	 */
-	if (i915.enable_guc_loading > 1) {
+	if (i915.enable_guc_loading >= FIRMWARE_LOAD_MANDATORY) {
 		ret = -EIO;
 	} else if (i915.enable_guc_submission >= GUC_SUBMISSION_MANDATORY) {
 		ret = -EIO;
@@ -687,9 +687,10 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
-	/* A negative value means "use platform default" */
-	if (i915.enable_guc_loading < 0)
-		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
+	/* Any negative value means "use platform default" */
+	if (i915.enable_guc_loading <= FIRMWARE_LOAD_DEFAULT)
+		i915.enable_guc_loading = HAS_GUC_UCODE(dev) ?
+			FIRMWARE_LOAD_PREFERRED : FIRMWARE_LOAD_DISABLED;
 	if (i915.enable_guc_submission <= GUC_SUBMISSION_DEFAULT)
 		i915.enable_guc_submission = HAS_GUC_SCHED(dev) ?
 			GUC_SUBMISSION_PREFERRED : GUC_SUBMISSION_DISABLED;
@@ -718,7 +719,7 @@ void intel_guc_init(struct drm_device *dev)
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
 
 	/* Early (and silent) return if GuC loading is disabled */
-	if (!i915.enable_guc_loading)
+	if (i915.enable_guc_loading == FIRMWARE_LOAD_DISABLED)
 		return;
 	if (fw_path == NULL)
 		return;
-- 
1.9.1

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

[PATCH v4 3/5] drm/i915/guc: symbolic name for GuC log-level none

[Dave Gordon]

The existing code that accesses the "guc_log_level" parameter
uses an explicit numerical value for the "no logging" case,
whereas there are symbolic names for the other levels.

So this patch just provides and uses a name for the default log
level (NONE), with the same numeric value that is already used.

This should produce identical code to the previous version!

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 2 +-
 drivers/gpu/drm/i915/intel_guc_fwif.h      | 1 +
 drivers/gpu/drm/i915/intel_guc_loader.c    | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 3b9cfa5..a82f351 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -847,7 +847,7 @@ static void guc_create_log(struct intel_guc *guc)
 		obj = gem_allocate_guc_obj(dev_priv, size);
 		if (!obj) {
 			/* logging will be off */
-			i915.guc_log_level = -1;
+			i915.guc_log_level = GUC_LOG_VERBOSITY_NONE;
 			return;
 		}
 
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 944786d..bd89f4e 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -132,6 +132,7 @@
 #define   GUC_LOG_VERBOSITY_HIGH	(2 << GUC_LOG_VERBOSITY_SHIFT)
 #define   GUC_LOG_VERBOSITY_ULTRA	(3 << GUC_LOG_VERBOSITY_SHIFT)
 /* Verbosity range-check limits, without the shift */
+#define	  GUC_LOG_VERBOSITY_NONE	(-1)
 #define	  GUC_LOG_VERBOSITY_MIN		0
 #define	  GUC_LOG_VERBOSITY_MAX		3
 #define	  GUC_LOG_VERBOSITY_MASK	0x0000000f
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 33d9770..898cccb 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -175,7 +175,7 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
 	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
 			GUC_CTL_VCS2_ENABLED;
 
-	if (i915.guc_log_level >= 0) {
+	if (i915.guc_log_level > GUC_LOG_VERBOSITY_NONE) {
 		params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
 		params[GUC_CTL_DEBUG] =
 			i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
-- 
1.9.1

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

[PATCH v4 4/5] drm/i915/guc: use symbolic names in setting defaults for module parameters

[Dave Gordon]

Of course, this also re-enables GuC loading and submission
by default on suitable platforms, since it's Intel's Plan
of Record that GuC submission shall be used where available.

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

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b6e404c..6473011 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -54,9 +54,9 @@ struct i915_params i915 __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
-	.enable_guc_loading = 0,
-	.enable_guc_submission = 0,
-	.guc_log_level = -1,
+	.enable_guc_loading = FIRMWARE_LOAD_DEFAULT,
+	.enable_guc_submission = GUC_SUBMISSION_DEFAULT,
+	.guc_log_level = GUC_LOG_VERBOSITY_NONE,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
 	.enable_dpcd_backlight = false,
@@ -203,12 +203,12 @@ struct i915_params i915 __read_mostly = {
 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, 0=never [default], 1=if available, 2=required)");
+		"(-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)");
+		"(-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 v4 5/5] drm/i915/guc: ignore unrecognised loading & submission options

[Dave Gordon]

Previously the code allowed *any* values for the enable_guc_loading and
enable_guc_submission parameters, and forced them into range by clipping
at each extremum. This version instead ignores unknown values, treating
them as DEFAULT (which then gets converted to DISABLED or PREFERRED).

Of course we also have to report that we've ignored unknown values so
that the administrator or developer knows the kernel command line wasn't
sensible!

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_guc.h        |  8 ++++----
 drivers/gpu/drm/i915/intel_guc_loader.c | 34 ++++++++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 94db252..f8c44b1 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -91,10 +91,10 @@ struct i915_guc_client {
 };
 
 /*
- * These signed ranges represent user-requested preferences.
- * Out-of-range values from the user will be clipped towards
- * zero: any negative value is treated as -1, anything over 2
- * is just 2. ANY user-supplied value also taints the kernel.
+ * These values represent user-requested preferences; any other value will be
+ * treated as DEFAULT, so the driver will then choose an appropriate value.
+ *
+ * ANY user-supplied value (even DEFAULT) also taints the kernel.
  */
 enum {
 	GUC_SUBMISSION_DEFAULT = -1,
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 898cccb..8a81e80 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -521,9 +521,9 @@ int intel_guc_setup(struct drm_device *dev)
 	 * nonfatal error (i.e. it doesn't prevent driver load, but
 	 * marks the GPU as wedged until reset).
 	 */
-	if (i915.enable_guc_loading >= FIRMWARE_LOAD_MANDATORY) {
+	if (i915.enable_guc_loading == FIRMWARE_LOAD_MANDATORY) {
 		ret = -EIO;
-	} else if (i915.enable_guc_submission >= GUC_SUBMISSION_MANDATORY) {
+	} else if (i915.enable_guc_submission == GUC_SUBMISSION_MANDATORY) {
 		ret = -EIO;
 	} else {
 		ret = 0;
@@ -687,13 +687,37 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
-	/* Any negative value means "use platform default" */
-	if (i915.enable_guc_loading <= FIRMWARE_LOAD_DEFAULT)
+	/* Sanitise user-specified options */
+	switch (i915.enable_guc_loading) {
+	case FIRMWARE_LOAD_DISABLED:
+	case FIRMWARE_LOAD_PREFERRED:
+	case FIRMWARE_LOAD_MANDATORY:
+		break;
+
+	default:
+		DRM_INFO("ignoring unknown value %d for 'enable_guc_loading'\n",
+			 i915.enable_guc_loading);
+		/*FALLTHRU*/
+	case FIRMWARE_LOAD_DEFAULT:
 		i915.enable_guc_loading = HAS_GUC_UCODE(dev) ?
 			FIRMWARE_LOAD_PREFERRED : FIRMWARE_LOAD_DISABLED;
-	if (i915.enable_guc_submission <= GUC_SUBMISSION_DEFAULT)
+		break;
+	}
+
+	switch (i915.enable_guc_submission) {
+	case GUC_SUBMISSION_DISABLED:
+	case GUC_SUBMISSION_PREFERRED:
+	case GUC_SUBMISSION_MANDATORY:
+		break;
+
+	default:
+		DRM_INFO("ignoring unknown value %d for 'enable_guc_submission'\n",
+			 i915.enable_guc_submission);
+		/*FALLTHRU*/
+	case GUC_SUBMISSION_DEFAULT:
 		i915.enable_guc_submission = HAS_GUC_SCHED(dev) ?
 			GUC_SUBMISSION_PREFERRED : GUC_SUBMISSION_DISABLED;
+	}
 
 	if (!HAS_GUC_UCODE(dev)) {
 		fw_path = NULL;
-- 
1.9.1

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

✗ Ro.CI.BAT: warning for drm/i915/guc: use symbolic names for module parameter values (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: use symbolic names for module parameter values (rev2)
URL   : https://patchwork.freedesktop.org/series/10188/
State : warning

== Summary ==

Series 10188v2 drm/i915/guc: use symbolic names for module parameter values
http://patchwork.freedesktop.org/api/1.0/series/10188/revisions/2/mbox

Test drv_module_reload_basic:
                pass       -> DMESG-WARN (fi-hsw-i7-4770k)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)

fi-hsw-i7-4770k  total:107  pass:90   dwarn:1   dfail:0   fail:0   skip:15 
fi-kbl-qkkr      total:107  pass:83   dwarn:1   dfail:0   fail:0   skip:22 
fi-skl-i5-6260u  total:107  pass:98   dwarn:0   dfail:0   fail:0   skip:8  
fi-skl-i7-6700k  total:107  pass:84   dwarn:0   dfail:0   fail:0   skip:22 
fi-snb-i7-2600   total:107  pass:77   dwarn:0   dfail:0   fail:0   skip:29 
ro-bdw-i5-5250u  total:107  pass:97   dwarn:0   dfail:0   fail:0   skip:9  
ro-bdw-i7-5557U  total:107  pass:97   dwarn:0   dfail:0   fail:0   skip:9  
ro-bdw-i7-5600u  total:107  pass:79   dwarn:0   dfail:0   fail:0   skip:27 
ro-bsw-n3050     total:240  pass:194  dwarn:0   dfail:0   fail:4   skip:42 
ro-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40 
ro-hsw-i3-4010u  total:107  pass:87   dwarn:0   dfail:0   fail:0   skip:19 
ro-ilk1-i5-650   total:235  pass:173  dwarn:0   dfail:0   fail:2   skip:60 
ro-ivb-i7-3770   total:107  pass:80   dwarn:0   dfail:0   fail:0   skip:26 
ro-skl3-i5-6260u total:107  pass:97   dwarn:1   dfail:0   fail:0   skip:8  
ro-hsw-i7-4770r failed to connect after reboot
ro-ivb2-i7-3770 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1777/

e220d47 drm-intel-nightly: 2016y-08m-09d-09h-18m-12s UTC integration manifest
c47d6cf drm/i915/guc: ignore unrecognised loading & submission options
32f71df drm/i915/guc: use symbolic names in setting defaults for module parameters
449ee3e drm/i915/guc: symbolic name for GuC log-level none
ec2cf78 drm/i915/guc: symbolic names for GuC firmare loading preferences
1a3ec82 drm/i915/guc: symbolic names for GuC submission preferences

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

Re: ✗ Ro.CI.BAT: warning for drm/i915/guc: use symbolic names for module parameter values (rev2)

[Dave Gordon]

On 09/08/16 12:25, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915/guc: use symbolic names for module parameter values (rev2)
> URL   : https://patchwork.freedesktop.org/series/10188/
> State : warning
>
> == Summary ==
>
> Series 10188v2 drm/i915/guc: use symbolic names for module parameter values
> http://patchwork.freedesktop.org/api/1.0/series/10188/revisions/2/mbox
>
> Test drv_module_reload_basic:
>                 pass       -> DMESG-WARN (fi-hsw-i7-4770k)

The RCU-related recursive locking already mentioned by Ville, and under 
investigation elsewhere.

>                 pass       -> DMESG-WARN (ro-skl3-i5-6260u)

This machine is missing (the correct version of) the GuC firmware!

.Dave.

> fi-hsw-i7-4770k  total:107  pass:90   dwarn:1   dfail:0   fail:0   skip:15
> fi-kbl-qkkr      total:107  pass:83   dwarn:1   dfail:0   fail:0   skip:22
> fi-skl-i5-6260u  total:107  pass:98   dwarn:0   dfail:0   fail:0   skip:8
> fi-skl-i7-6700k  total:107  pass:84   dwarn:0   dfail:0   fail:0   skip:22
> fi-snb-i7-2600   total:107  pass:77   dwarn:0   dfail:0   fail:0   skip:29
> ro-bdw-i5-5250u  total:107  pass:97   dwarn:0   dfail:0   fail:0   skip:9
> ro-bdw-i7-5557U  total:107  pass:97   dwarn:0   dfail:0   fail:0   skip:9
> ro-bdw-i7-5600u  total:107  pass:79   dwarn:0   dfail:0   fail:0   skip:27
> ro-bsw-n3050     total:240  pass:194  dwarn:0   dfail:0   fail:4   skip:42
> ro-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40
> ro-hsw-i3-4010u  total:107  pass:87   dwarn:0   dfail:0   fail:0   skip:19
> ro-ilk1-i5-650   total:235  pass:173  dwarn:0   dfail:0   fail:2   skip:60
> ro-ivb-i7-3770   total:107  pass:80   dwarn:0   dfail:0   fail:0   skip:26
> ro-skl3-i5-6260u total:107  pass:97   dwarn:1   dfail:0   fail:0   skip:8
> ro-hsw-i7-4770r failed to connect after reboot
> ro-ivb2-i7-3770 failed to connect after reboot
>
> Results at /archive/results/CI_IGT_test/RO_Patchwork_1777/
>
> e220d47 drm-intel-nightly: 2016y-08m-09d-09h-18m-12s UTC integration manifest
> c47d6cf drm/i915/guc: ignore unrecognised loading & submission options
> 32f71df drm/i915/guc: use symbolic names in setting defaults for module parameters
> 449ee3e drm/i915/guc: symbolic name for GuC log-level none
> ec2cf78 drm/i915/guc: symbolic names for GuC firmare loading preferences
> 1a3ec82 drm/i915/guc: symbolic names for GuC submission preferences
>

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

✗ Ro.CI.BAT: failure for drm/i915/guc: use symbolic names for module parameter values (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: use symbolic names for module parameter values (rev2)
URL   : https://patchwork.freedesktop.org/series/10188/
State : failure

== Summary ==

Series 10188v2 drm/i915/guc: use symbolic names for module parameter values
http://patchwork.freedesktop.org/api/1.0/series/10188/revisions/2/mbox

Test drv_module_reload_basic:
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
                pass       -> SKIP       (ro-bdw-i5-5250u)
Test kms_cursor_legacy:
        Subgroup basic-flip-vs-cursor-legacy:
                pass       -> FAIL       (ro-byt-n2820)
                pass       -> FAIL       (fi-hsw-i7-4770k)
                pass       -> FAIL       (ro-bdw-i5-5250u)
        Subgroup basic-flip-vs-cursor-varying-size:
                pass       -> FAIL       (ro-skl3-i5-6260u)
                pass       -> FAIL       (ro-bdw-i5-5250u)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-c-frame-sequence:
                fail       -> PASS       (ro-ivb-i7-3770)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)

fi-hsw-i7-4770k  total:201  pass:179  dwarn:0   dfail:0   fail:1   skip:20 
fi-kbl-qkkr      total:244  pass:186  dwarn:29  dfail:0   fail:2   skip:27 
fi-skl-i5-6260u  total:244  pass:224  dwarn:4   dfail:0   fail:2   skip:14 
fi-skl-i7-6700k  total:107  pass:84   dwarn:0   dfail:0   fail:0   skip:22 
fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:217  dwarn:2   dfail:0   fail:2   skip:19 
ro-bdw-i7-5557U  total:240  pass:219  dwarn:1   dfail:0   fail:1   skip:19 
ro-bdw-i7-5600u  total:240  pass:207  dwarn:0   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:194  dwarn:0   dfail:0   fail:4   skip:42 
ro-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-ilk1-i5-650   total:235  pass:173  dwarn:0   dfail:0   fail:2   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:1   dfail:0   fail:3   skip:14 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1792/

d0e3a4b drm-intel-nightly: 2016y-08m-09d-20h-18m-38s UTC integration manifest
d74850c drm/i915/guc: ignore unrecognised loading & submission options
1d2e495 drm/i915/guc: use symbolic names in setting defaults for module parameters
c336a5b drm/i915/guc: symbolic name for GuC log-level none
305708f drm/i915/guc: symbolic names for GuC firmare loading preferences
6108dd2 drm/i915/guc: symbolic names for GuC submission preferences

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

[PATCH v4 2/5] drm/i915/guc: symbolic names for GuC firmare loading preferences

[Dave Gordon]

The existing code that accesses the "enable_guc_loading"
parameter uses explicit numerical values for the various
possibilities,  including in one case relying on boolean
0/1 mapping to specific values (which could be confusing
for maintainers).

So this patch just provides and uses names for the values
representing the DEFAULT, DISABLED, PREFERRED, and MANDATORY
options that the user can select (-1, 0, 1, 2 respectively).

This should produce identical code to the previous version!

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_guc.h        | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_guc_loader.c | 13 +++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 941d70e..83c2f295 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -89,12 +89,26 @@ struct i915_guc_client {
 	uint64_t submissions[I915_NUM_ENGINES];
 };
 
+/*
+ * These signed ranges represent user-requested preferences.
+ * Out-of-range values from the user will be clipped towards
+ * zero: any negative value is treated as -1, anything over 2
+ * is just 2. ANY user-supplied value also taints the kernel.
+ */
 enum {
 	GUC_SUBMISSION_DEFAULT = -1,
 	GUC_SUBMISSION_DISABLED = 0,
 	GUC_SUBMISSION_PREFERRED,
 	GUC_SUBMISSION_MANDATORY
 };
+enum {
+	FIRMWARE_LOAD_DEFAULT = -1,
+	FIRMWARE_LOAD_DISABLED = 0,
+	FIRMWARE_LOAD_PREFERRED,
+	FIRMWARE_LOAD_MANDATORY
+};
+
+/* These represent the actual firmware status  */
 enum intel_guc_fw_status {
 	GUC_FIRMWARE_FAIL = -1,
 	GUC_FIRMWARE_NONE = 0,
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 9782f23..ea7ebbd 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -438,7 +438,7 @@ int intel_guc_setup(struct drm_device *dev)
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
 	/* Loading forbidden, or no firmware to load? */
-	if (!i915.enable_guc_loading) {
+	if (i915.enable_guc_loading == FIRMWARE_LOAD_DISABLED) {
 		err = 0;
 		goto fail;
 	} else if (fw_path == NULL) {
@@ -533,7 +533,7 @@ int intel_guc_setup(struct drm_device *dev)
 	 * nonfatal error (i.e. it doesn't prevent driver load, but
 	 * marks the GPU as wedged until reset).
 	 */
-	if (i915.enable_guc_loading > 1) {
+	if (i915.enable_guc_loading >= FIRMWARE_LOAD_MANDATORY) {
 		ret = -EIO;
 	} else if (i915.enable_guc_submission >= GUC_SUBMISSION_MANDATORY) {
 		ret = -EIO;
@@ -700,9 +700,10 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
-	/* A negative value means "use platform default" */
-	if (i915.enable_guc_loading < 0)
-		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
+	/* Any negative value means "use platform default" */
+	if (i915.enable_guc_loading <= FIRMWARE_LOAD_DEFAULT)
+		i915.enable_guc_loading = HAS_GUC_UCODE(dev) ?
+			FIRMWARE_LOAD_PREFERRED : FIRMWARE_LOAD_DISABLED;
 	if (i915.enable_guc_submission <= GUC_SUBMISSION_DEFAULT)
 		i915.enable_guc_submission = HAS_GUC_SCHED(dev) ?
 			GUC_SUBMISSION_PREFERRED : GUC_SUBMISSION_DISABLED;
@@ -731,7 +732,7 @@ void intel_guc_init(struct drm_device *dev)
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
 
 	/* Early (and silent) return if GuC loading is disabled */
-	if (!i915.enable_guc_loading)
+	if (i915.enable_guc_loading == FIRMWARE_LOAD_DISABLED)
 		return;
 	if (fw_path == NULL)
 		return;
-- 
1.9.1

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

[PATCH v4 2/5] drm/i915/guc: symbolic names for GuC firmare loading preferences

[Dave Gordon]

The existing code that accesses the "enable_guc_loading"
parameter uses explicit numerical values for the various
possibilities,  including in one case relying on boolean
0/1 mapping to specific values (which could be confusing
for maintainers).

So this patch just provides and uses names for the values
representing the DEFAULT, DISABLED, PREFERRED, and MANDATORY
options that the user can select (-1, 0, 1, 2 respectively).

This should produce identical code to the previous version!

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_guc.h        | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_guc_loader.c | 13 +++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 941d70e..83c2f295 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -89,12 +89,26 @@ struct i915_guc_client {
 	uint64_t submissions[I915_NUM_ENGINES];
 };
 
+/*
+ * These signed ranges represent user-requested preferences.
+ * Out-of-range values from the user will be clipped towards
+ * zero: any negative value is treated as -1, anything over 2
+ * is just 2. ANY user-supplied value also taints the kernel.
+ */
 enum {
 	GUC_SUBMISSION_DEFAULT = -1,
 	GUC_SUBMISSION_DISABLED = 0,
 	GUC_SUBMISSION_PREFERRED,
 	GUC_SUBMISSION_MANDATORY
 };
+enum {
+	FIRMWARE_LOAD_DEFAULT = -1,
+	FIRMWARE_LOAD_DISABLED = 0,
+	FIRMWARE_LOAD_PREFERRED,
+	FIRMWARE_LOAD_MANDATORY
+};
+
+/* These represent the actual firmware status  */
 enum intel_guc_fw_status {
 	GUC_FIRMWARE_FAIL = -1,
 	GUC_FIRMWARE_NONE = 0,
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 49f846c..da047a8 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -438,7 +438,7 @@ int intel_guc_setup(struct drm_device *dev)
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
 	/* Loading forbidden, or no firmware to load? */
-	if (!i915.enable_guc_loading) {
+	if (i915.enable_guc_loading == FIRMWARE_LOAD_DISABLED) {
 		err = 0;
 		goto fail;
 	} else if (fw_path == NULL) {
@@ -533,7 +533,7 @@ int intel_guc_setup(struct drm_device *dev)
 	 * nonfatal error (i.e. it doesn't prevent driver load, but
 	 * marks the GPU as wedged until reset).
 	 */
-	if (i915.enable_guc_loading > 1) {
+	if (i915.enable_guc_loading >= FIRMWARE_LOAD_MANDATORY) {
 		ret = -EIO;
 	} else if (i915.enable_guc_submission >= GUC_SUBMISSION_MANDATORY) {
 		ret = -EIO;
@@ -699,9 +699,10 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
-	/* A negative value means "use platform default" */
-	if (i915.enable_guc_loading < 0)
-		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
+	/* Any negative value means "use platform default" */
+	if (i915.enable_guc_loading <= FIRMWARE_LOAD_DEFAULT)
+		i915.enable_guc_loading = HAS_GUC_UCODE(dev) ?
+			FIRMWARE_LOAD_PREFERRED : FIRMWARE_LOAD_DISABLED;
 	if (i915.enable_guc_submission <= GUC_SUBMISSION_DEFAULT)
 		i915.enable_guc_submission = HAS_GUC_SCHED(dev) ?
 			GUC_SUBMISSION_PREFERRED : GUC_SUBMISSION_DISABLED;
@@ -730,7 +731,7 @@ void intel_guc_init(struct drm_device *dev)
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
 
 	/* Early (and silent) return if GuC loading is disabled */
-	if (!i915.enable_guc_loading)
+	if (i915.enable_guc_loading == FIRMWARE_LOAD_DISABLED)
 		return;
 	if (fw_path == NULL)
 		return;
-- 
1.9.1

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