[PATCH v2 1/7] drm/i915/huc: Move firmware selection to init_early

[PATCH v2 1/7] drm/i915/huc: Move firmware selection to init_early

[Michal Wajdeczko]

Doing HuC firmware path selection from sanitize_options function
is not perfect, while there is no problem with doing so during
early init stage as we already have all needed data.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  3 ++
 drivers/gpu/drm/i915/intel_huc.c | 60 +++++++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_huc.h |  2 +-
 drivers/gpu/drm/i915/intel_uc.c  |  4 +--
 4 files changed, 42 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bddd658..4c3616b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3232,6 +3232,9 @@ intel_info(const struct drm_i915_private *dev_priv)
 #define HAS_GUC_CT(dev_priv)	((dev_priv)->info.has_guc_ct)
 #define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
 #define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
+
+/* For now, anything with a GuC has also HuC */
+#define HAS_HUC(dev_priv)	(HAS_GUC(dev_priv))
 #define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
 
 #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 98d1725..6d0e050 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -77,43 +77,57 @@ MODULE_FIRMWARE(I915_KBL_HUC_UCODE);
 #define I915_GLK_HUC_UCODE HUC_FW_PATH(glk, GLK_HUC_FW_MAJOR, \
 	GLK_HUC_FW_MINOR, GLK_BLD_NUM)
 
-/**
- * intel_huc_select_fw() - selects HuC firmware for loading
- * @huc:	intel_huc struct
- */
-void intel_huc_select_fw(struct intel_huc *huc)
+static void huc_fw_select(struct intel_uc_fw *huc_fw)
 {
+	struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw);
 	struct drm_i915_private *dev_priv = huc_to_i915(huc);
 
-	intel_uc_fw_init(&huc->fw, INTEL_UC_FW_TYPE_HUC);
+	GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
+
+	if (!HAS_HUC(dev_priv))
+		return;
 
 	if (i915_modparams.huc_firmware_path) {
-		huc->fw.path = i915_modparams.huc_firmware_path;
-		huc->fw.major_ver_wanted = 0;
-		huc->fw.minor_ver_wanted = 0;
+		huc_fw->path = i915_modparams.huc_firmware_path;
+		huc_fw->major_ver_wanted = 0;
+		huc_fw->minor_ver_wanted = 0;
 	} else if (IS_SKYLAKE(dev_priv)) {
-		huc->fw.path = I915_SKL_HUC_UCODE;
-		huc->fw.major_ver_wanted = SKL_HUC_FW_MAJOR;
-		huc->fw.minor_ver_wanted = SKL_HUC_FW_MINOR;
+		huc_fw->path = I915_SKL_HUC_UCODE;
+		huc_fw->major_ver_wanted = SKL_HUC_FW_MAJOR;
+		huc_fw->minor_ver_wanted = SKL_HUC_FW_MINOR;
 	} else if (IS_BROXTON(dev_priv)) {
-		huc->fw.path = I915_BXT_HUC_UCODE;
-		huc->fw.major_ver_wanted = BXT_HUC_FW_MAJOR;
-		huc->fw.minor_ver_wanted = BXT_HUC_FW_MINOR;
+		huc_fw->path = I915_BXT_HUC_UCODE;
+		huc_fw->major_ver_wanted = BXT_HUC_FW_MAJOR;
+		huc_fw->minor_ver_wanted = BXT_HUC_FW_MINOR;
 	} else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
-		huc->fw.path = I915_KBL_HUC_UCODE;
-		huc->fw.major_ver_wanted = KBL_HUC_FW_MAJOR;
-		huc->fw.minor_ver_wanted = KBL_HUC_FW_MINOR;
+		huc_fw->path = I915_KBL_HUC_UCODE;
+		huc_fw->major_ver_wanted = KBL_HUC_FW_MAJOR;
+		huc_fw->minor_ver_wanted = KBL_HUC_FW_MINOR;
 	} else if (IS_GEMINILAKE(dev_priv)) {
-		huc->fw.path = I915_GLK_HUC_UCODE;
-		huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
-		huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
+		huc_fw->path = I915_GLK_HUC_UCODE;
+		huc_fw->major_ver_wanted = GLK_HUC_FW_MAJOR;
+		huc_fw->minor_ver_wanted = GLK_HUC_FW_MINOR;
 	} else {
-		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
-		return;
+		DRM_WARN("%s: No firmware known for this platform!\n",
+			 intel_uc_fw_type_repr(huc_fw->type));
 	}
 }
 
 /**
+ * intel_huc_init_early() - initializes HuC struct
+ * @huc: intel_huc struct
+ *
+ * On platforms with HuC selects firmware for uploading
+ */
+void intel_huc_init_early(struct intel_huc *huc)
+{
+	struct intel_uc_fw *huc_fw = &huc->fw;
+
+	intel_uc_fw_init(huc_fw, INTEL_UC_FW_TYPE_HUC);
+	huc_fw_select(huc_fw);
+}
+
+/**
  * huc_ucode_xfer() - DMA's the firmware
  * @dev_priv: the drm_i915_private device
  *
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
index aaa38b9..3d757bc 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -34,7 +34,7 @@ struct intel_huc {
 	/* HuC-specific additions */
 };
 
-void intel_huc_select_fw(struct intel_huc *huc);
+void intel_huc_init_early(struct intel_huc *huc);
 void intel_huc_init_hw(struct intel_huc *huc);
 void intel_huc_auth(struct intel_huc *huc);
 
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 1e2a30a..95b524c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -65,9 +65,6 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 
 	/* Verify firmware version */
 	if (i915_modparams.enable_guc_loading) {
-		if (HAS_HUC_UCODE(dev_priv))
-			intel_huc_select_fw(&dev_priv->huc);
-
 		if (intel_guc_fw_select(&dev_priv->guc))
 			i915_modparams.enable_guc_loading = 0;
 	}
@@ -84,6 +81,7 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
 	intel_guc_init_early(&dev_priv->guc);
+	intel_huc_init_early(&dev_priv->huc);
 }
 
 void intel_uc_init_fw(struct drm_i915_private *dev_priv)
-- 
2.7.4

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

[PATCH v2 2/7] drm/i915/guc: Move firmware selection to init_early

[Michal Wajdeczko]

Doing GuC firmware path selection from sanitize_options function
is not perfect, while there is no problem with doing so during
early init stage as we already have all needed data.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c    |  1 +
 drivers/gpu/drm/i915/intel_guc_fw.c | 63 +++++++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_guc_fw.h |  2 +-
 drivers/gpu/drm/i915/intel_uc.c     |  2 +-
 drivers/gpu/drm/i915/intel_uc_fw.h  |  5 +++
 5 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 823d0c2..8136478 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -61,6 +61,7 @@ void intel_guc_init_send_regs(struct intel_guc *guc)
 
 void intel_guc_init_early(struct intel_guc *guc)
 {
+	intel_guc_fw_init_early(guc);
 	intel_guc_ct_init_early(&guc->ct);
 
 	mutex_init(&guc->send_mutex);
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index 89862fa..cbc51c9 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -56,45 +56,54 @@ MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
 
 #define I915_GLK_GUC_UCODE GUC_FW_PATH(glk, GLK_FW_MAJOR, GLK_FW_MINOR)
 
-/**
- * intel_guc_fw_select() - selects GuC firmware for uploading
- *
- * @guc:	intel_guc struct
- *
- * Return: zero when we know firmware, non-zero in other case
- */
-int intel_guc_fw_select(struct intel_guc *guc)
+static void guc_fw_select(struct intel_uc_fw *guc_fw)
 {
+	struct intel_guc *guc = container_of(guc_fw, struct intel_guc, fw);
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
-	intel_uc_fw_init(&guc->fw, INTEL_UC_FW_TYPE_GUC);
+	GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
+
+	if (!HAS_GUC(dev_priv))
+		return;
 
 	if (i915_modparams.guc_firmware_path) {
-		guc->fw.path = i915_modparams.guc_firmware_path;
-		guc->fw.major_ver_wanted = 0;
-		guc->fw.minor_ver_wanted = 0;
+		guc_fw->path = i915_modparams.guc_firmware_path;
+		guc_fw->major_ver_wanted = 0;
+		guc_fw->minor_ver_wanted = 0;
 	} else if (IS_SKYLAKE(dev_priv)) {
-		guc->fw.path = I915_SKL_GUC_UCODE;
-		guc->fw.major_ver_wanted = SKL_FW_MAJOR;
-		guc->fw.minor_ver_wanted = SKL_FW_MINOR;
+		guc_fw->path = I915_SKL_GUC_UCODE;
+		guc_fw->major_ver_wanted = SKL_FW_MAJOR;
+		guc_fw->minor_ver_wanted = SKL_FW_MINOR;
 	} else if (IS_BROXTON(dev_priv)) {
-		guc->fw.path = I915_BXT_GUC_UCODE;
-		guc->fw.major_ver_wanted = BXT_FW_MAJOR;
-		guc->fw.minor_ver_wanted = BXT_FW_MINOR;
+		guc_fw->path = I915_BXT_GUC_UCODE;
+		guc_fw->major_ver_wanted = BXT_FW_MAJOR;
+		guc_fw->minor_ver_wanted = BXT_FW_MINOR;
 	} else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
-		guc->fw.path = I915_KBL_GUC_UCODE;
-		guc->fw.major_ver_wanted = KBL_FW_MAJOR;
-		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
+		guc_fw->path = I915_KBL_GUC_UCODE;
+		guc_fw->major_ver_wanted = KBL_FW_MAJOR;
+		guc_fw->minor_ver_wanted = KBL_FW_MINOR;
 	} else if (IS_GEMINILAKE(dev_priv)) {
-		guc->fw.path = I915_GLK_GUC_UCODE;
-		guc->fw.major_ver_wanted = GLK_FW_MAJOR;
-		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
+		guc_fw->path = I915_GLK_GUC_UCODE;
+		guc_fw->major_ver_wanted = GLK_FW_MAJOR;
+		guc_fw->minor_ver_wanted = GLK_FW_MINOR;
 	} else {
-		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
-		return -ENOENT;
+		DRM_WARN("%s: No firmware known for this platform!\n",
+			 intel_uc_fw_type_repr(guc_fw->type));
 	}
+}
 
-	return 0;
+/**
+ * intel_guc_fw_init_early() - initializes GuC firmware struct
+ * @guc: intel_guc struct
+ *
+ * On platforms with GuC selects firmware for uploading
+ */
+void intel_guc_fw_init_early(struct intel_guc *guc)
+{
+	struct intel_uc_fw *guc_fw = &guc->fw;
+
+	intel_uc_fw_init(guc_fw, INTEL_UC_FW_TYPE_GUC);
+	guc_fw_select(guc_fw);
 }
 
 static void guc_prepare_xfer(struct intel_guc *guc)
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h b/drivers/gpu/drm/i915/intel_guc_fw.h
index 023f5ba..4ec5d3d 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.h
+++ b/drivers/gpu/drm/i915/intel_guc_fw.h
@@ -27,7 +27,7 @@
 
 struct intel_guc;
 
-int intel_guc_fw_select(struct intel_guc *guc);
+void intel_guc_fw_init_early(struct intel_guc *guc);
 int intel_guc_fw_upload(struct intel_guc *guc);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 95b524c..4b7f2a7 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -65,7 +65,7 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 
 	/* Verify firmware version */
 	if (i915_modparams.enable_guc_loading) {
-		if (intel_guc_fw_select(&dev_priv->guc))
+		if (!intel_uc_fw_is_selected(&dev_priv->guc.fw))
 			i915_modparams.enable_guc_loading = 0;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
index 5394d9d..d5fd460 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -110,6 +110,11 @@ void intel_uc_fw_init(struct intel_uc_fw *uc_fw, enum intel_uc_fw_type type)
 	uc_fw->type = type;
 }
 
+static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw)
+{
+	return uc_fw->path != NULL;
+}
+
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 		       struct intel_uc_fw *uc_fw);
 int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
-- 
2.7.4

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

[PATCH v2 3/7] drm/i915/guc: Introduce USES_GUC_xxx helper macros

[Michal Wajdeczko]

In the upcoming patch we will change the way how to recognize
when GuC is in use. Using helper macros will minimize scope
of that changes. While here, update dev_info message.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  4 ++++
 drivers/gpu/drm/i915/i915_gem_context.c    |  4 ++--
 drivers/gpu/drm/i915/i915_gem_gtt.c        |  2 +-
 drivers/gpu/drm/i915/i915_irq.c            |  2 +-
 drivers/gpu/drm/i915/intel_guc.c           |  2 +-
 drivers/gpu/drm/i915/intel_guc_log.c       |  6 +++---
 drivers/gpu/drm/i915/intel_gvt.c           |  2 +-
 drivers/gpu/drm/i915/intel_uc.c            | 23 +++++++++++------------
 drivers/gpu/drm/i915/selftests/intel_guc.c |  2 +-
 9 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4c3616b..2c386c7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3237,6 +3237,10 @@ intel_info(const struct drm_i915_private *dev_priv)
 #define HAS_HUC(dev_priv)	(HAS_GUC(dev_priv))
 #define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
 
+/* Having a GuC is not the same as using a GuC */
+#define USES_GUC(dev_priv)		(i915_modparams.enable_guc_loading)
+#define USES_GUC_SUBMISSION(dev_priv)	(i915_modparams.enable_guc_submission)
+
 #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
 
 #define HAS_POOLED_EU(dev_priv)	((dev_priv)->info.has_pooled_eu)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index ce3139e..21ce374 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -316,7 +316,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
 	 * present or not in use we still need a small bias as ring wraparound
 	 * at offset 0 sometimes hangs. No idea why.
 	 */
-	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
+	if (USES_GUC(dev_priv))
 		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
 	else
 		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
@@ -409,7 +409,7 @@ i915_gem_context_create_gvt(struct drm_device *dev)
 	i915_gem_context_set_closed(ctx); /* not user accessible */
 	i915_gem_context_clear_bannable(ctx);
 	i915_gem_context_set_force_single_submission(ctx);
-	if (!i915_modparams.enable_guc_submission)
+	if (!USES_GUC_SUBMISSION(to_i915(dev)))
 		ctx->ring_size = 512 * PAGE_SIZE; /* Max ring buffer size */
 
 	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index f3c35e8..209bb11 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3503,7 +3503,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv)
 	 * currently don't have any bits spare to pass in this upper
 	 * restriction!
 	 */
-	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) {
+	if (USES_GUC(dev_priv)) {
 		ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
 		ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
 	}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7cac07d..3517c65 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1400,7 +1400,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
 
 	if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
 		notify_ring(engine);
-		tasklet |= i915_modparams.enable_guc_submission;
+		tasklet |= USES_GUC_SUBMISSION(engine->i915);
 	}
 
 	if (tasklet)
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 8136478..39919e9 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -129,7 +129,7 @@ void intel_guc_init_params(struct intel_guc *guc)
 	}
 
 	/* If GuC submission is enabled, set up additional parameters here */
-	if (i915_modparams.enable_guc_submission) {
+	if (USES_GUC_SUBMISSION(dev_priv)) {
 		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
 		u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
 		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 76d3eb1..1a2c5ee 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -505,7 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
-	if (!i915_modparams.enable_guc_submission ||
+	if (!USES_GUC_SUBMISSION(dev_priv) ||
 	    (i915_modparams.guc_log_level < 0))
 		return;
 
@@ -646,7 +646,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 
 void i915_guc_log_register(struct drm_i915_private *dev_priv)
 {
-	if (!i915_modparams.enable_guc_submission ||
+	if (!USES_GUC_SUBMISSION(dev_priv) ||
 	    (i915_modparams.guc_log_level < 0))
 		return;
 
@@ -657,7 +657,7 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
 
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 {
-	if (!i915_modparams.enable_guc_submission)
+	if (!USES_GUC_SUBMISSION(dev_priv))
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c
index 126f7c7..a2fe7c8 100644
--- a/drivers/gpu/drm/i915/intel_gvt.c
+++ b/drivers/gpu/drm/i915/intel_gvt.c
@@ -95,7 +95,7 @@ int intel_gvt_init(struct drm_i915_private *dev_priv)
 		return 0;
 	}
 
-	if (i915_modparams.enable_guc_submission) {
+	if (USES_GUC_SUBMISSION(dev_priv)) {
 		DRM_ERROR("i915 GVT-g loading failed due to Graphics virtualization is not yet supported with GuC submission\n");
 		return -EIO;
 	}
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 4b7f2a7..ed2dd76 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -152,7 +152,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 	int ret, attempts;
 
-	if (!i915_modparams.enable_guc_loading)
+	if (!USES_GUC(dev_priv))
 		return 0;
 
 	guc_disable_communication(guc);
@@ -161,7 +161,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	/* We need to notify the guc whenever we change the GGTT */
 	i915_ggtt_enable_guc(dev_priv);
 
-	if (i915_modparams.enable_guc_submission) {
+	if (USES_GUC_SUBMISSION(dev_priv)) {
 		/*
 		 * This is stuff we need to have available at fw load time
 		 * if we are planning to enable submission later
@@ -211,7 +211,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		goto err_log_capture;
 
 	intel_huc_auth(&dev_priv->huc);
-	if (i915_modparams.enable_guc_submission) {
+	if (USES_GUC_SUBMISSION(dev_priv)) {
 		if (i915_modparams.guc_log_level >= 0)
 			gen9_enable_guc_interrupts(dev_priv);
 
@@ -220,11 +220,10 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 			goto err_interrupts;
 	}
 
-	dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version %u.%u])\n",
-		 i915_modparams.enable_guc_submission ? "submission enabled" :
-							"loaded",
-		 guc->fw.path,
+	dev_info(dev_priv->drm.dev, "GuC firmware version %u.%u\n",
 		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
+	dev_info(dev_priv->drm.dev, "GuC submission %s\n",
+		 enableddisabled(USES_GUC_SUBMISSION(dev_priv)));
 
 	return 0;
 
@@ -243,7 +242,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 err_log_capture:
 	guc_capture_load_err_log(guc);
 err_submission:
-	if (i915_modparams.enable_guc_submission)
+	if (USES_GUC_SUBMISSION(dev_priv))
 		intel_guc_submission_fini(guc);
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
@@ -257,7 +256,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		ret = 0;
 	}
 
-	if (i915_modparams.enable_guc_submission) {
+	if (USES_GUC_SUBMISSION(dev_priv)) {
 		i915_modparams.enable_guc_submission = 0;
 		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
 	}
@@ -273,15 +272,15 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 
 	guc_free_load_err_log(guc);
 
-	if (!i915_modparams.enable_guc_loading)
+	if (!USES_GUC(dev_priv))
 		return;
 
-	if (i915_modparams.enable_guc_submission)
+	if (USES_GUC_SUBMISSION(dev_priv))
 		intel_guc_submission_disable(guc);
 
 	guc_disable_communication(guc);
 
-	if (i915_modparams.enable_guc_submission) {
+	if (USES_GUC_SUBMISSION(dev_priv)) {
 		gen9_disable_guc_interrupts(dev_priv);
 		intel_guc_submission_fini(guc);
 	}
diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c
index 7b23597..68d6a69 100644
--- a/drivers/gpu/drm/i915/selftests/intel_guc.c
+++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
@@ -362,7 +362,7 @@ int intel_guc_live_selftest(struct drm_i915_private *dev_priv)
 		SUBTEST(igt_guc_doorbells),
 	};
 
-	if (!i915_modparams.enable_guc_submission)
+	if (!USES_GUC_SUBMISSION(dev_priv))
 		return 0;
 
 	return i915_subtests(tests, dev_priv);
-- 
2.7.4

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

[PATCH v2 4/7] drm/i915/uc: Don't use -EIO to report missing firmware

[Michal Wajdeczko]

-EIO has special meaning and is used when we want to allow
engine initialization to fail and mark GPU as wedged.
Missing firmware does not fit into this scenario as this is
permanent error not related to GPU condition.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_uc_fw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index b376dd3..784eff9 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -214,7 +214,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
 
 	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
-		return -EIO;
+		return -ENOEXEC;
 
 	uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
 	DRM_DEBUG_DRIVER("%s fw load %s\n",
-- 
2.7.4

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

[PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams

[Michal Wajdeczko]

We currently have two module parameters that control GuC:
"enable_guc_loading" and "enable_guc_submission". Whenever
we need submission=1, we also need loading=1. We also need
loading=1 when we want to want to load and verify the HuC.

Lets combine above module parameters into one "enable_guc"
modparam. New supported bit values are:

 0=disable GuC (no GuC submission, no HuC)
 1=enable GuC submission
 2=enable HuC load

Special value "-1" can be used to let driver decide what
option should be enabled for given platform based on
hardware/firmware availability or preference.

Explicit enabling any of the GuC features makes GuC load
a required step, fallback to non-GuC mode will not be
supported.

v2: Don't use -EIO

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |   5 +-
 drivers/gpu/drm/i915/i915_params.c |  11 ++--
 drivers/gpu/drm/i915/i915_params.h |   3 +-
 drivers/gpu/drm/i915/intel_uc.c    | 100 +++++++++++++++++++++----------------
 4 files changed, 65 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2c386c7..9162a60 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3238,8 +3238,9 @@ intel_info(const struct drm_i915_private *dev_priv)
 #define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
 
 /* Having a GuC is not the same as using a GuC */
-#define USES_GUC(dev_priv)		(i915_modparams.enable_guc_loading)
-#define USES_GUC_SUBMISSION(dev_priv)	(i915_modparams.enable_guc_submission)
+#define USES_GUC(dev_priv)		(!!(i915_modparams.enable_guc > 0))
+#define USES_GUC_SUBMISSION(dev_priv)	(!!(i915_modparams.enable_guc & 1))
+#define USES_HUC(dev_priv)		(!!(i915_modparams.enable_guc & 2))
 
 #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
 
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 3328147..8654e45 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -154,13 +154,10 @@ i915_param_named_unsafe(edp_vswing, int, 0400,
 	"(0=use value from vbt [default], 1=low power swing(200mV),"
 	"2=default swing(400mV))");
 
-i915_param_named_unsafe(enable_guc_loading, int, 0400,
-	"Enable GuC firmware loading "
-	"(-1=auto, 0=never [default], 1=if available, 2=required)");
-
-i915_param_named_unsafe(enable_guc_submission, int, 0400,
-	"Enable GuC submission "
-	"(-1=auto, 0=never [default], 1=if available, 2=required)");
+i915_param_named_unsafe(enable_guc, int, 0400,
+	"Enable GuC load for GuC submission and/or HuC load. "
+	"Required functionality can be selected using bitmask values. "
+	"(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
 
 i915_param_named(guc_log_level, int, 0400,
 	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 8321bd8..10e2f74 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -42,8 +42,7 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc_loading, 0) \
-	param(int, enable_guc_submission, 0) \
+	param(int, enable_guc, 0) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index ed2dd76..8a761af 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -47,35 +47,59 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
+static int __get_default_enable_guc(struct drm_i915_private *dev_priv)
 {
-	if (!HAS_GUC(dev_priv)) {
-		if (i915_modparams.enable_guc_loading > 0 ||
-		    i915_modparams.enable_guc_submission > 0)
-			DRM_INFO("Ignoring GuC options, no hardware\n");
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
+	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
+	/* Default is to enable GuC/HuC if we know their firmwares */
+	int enable_guc = (intel_uc_fw_is_selected(guc_fw) ? 1 : 0) |
+			 (intel_uc_fw_is_selected(huc_fw) ? 2 : 0);
 
-		i915_modparams.enable_guc_loading = 0;
-		i915_modparams.enable_guc_submission = 0;
-		return;
-	}
+	/* Any platform specific fine-tuning can be done here */
 
-	/* A negative value means "use platform default" */
-	if (i915_modparams.enable_guc_loading < 0)
-		i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
+	/* Make sure that our "platform default" is well-defined */
+	GEM_BUG_ON(enable_guc < 0);
+	GEM_BUG_ON((enable_guc > 0) && !intel_uc_fw_is_selected(guc_fw));
+	GEM_BUG_ON((enable_guc & 2) && !intel_uc_fw_is_selected(huc_fw));
 
-	/* Verify firmware version */
-	if (i915_modparams.enable_guc_loading) {
-		if (!intel_uc_fw_is_selected(&dev_priv->guc.fw))
-			i915_modparams.enable_guc_loading = 0;
-	}
+	return enable_guc;
+}
 
-	/* Can't enable guc submission without guc loaded */
-	if (!i915_modparams.enable_guc_loading)
-		i915_modparams.enable_guc_submission = 0;
+/**
+ * intel_uc_sanitize_options - sanitize uC related modparam options
+ * @dev_priv: device private
+ *
+ * In case of "enable_guc" option this function will attempt to modify
+ * it only if it was initially set to "auto(-1)". Default value for this
+ * modparam varies between platforms and it is hardcoded in driver code.
+ * Any other modparam value is only monitored against availability of the
+ * related hardware or firmware definitions.
+ */
+void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
+{
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
+	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
+	int enable_guc = __get_default_enable_guc(dev_priv);
 
 	/* A negative value means "use platform default" */
-	if (i915_modparams.enable_guc_submission < 0)
-		i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
+	if (i915_modparams.enable_guc < 0)
+		i915_modparams.enable_guc = enable_guc;
+
+	/* Verify GuC firmware availability */
+	if (USES_GUC(dev_priv) && !intel_uc_fw_is_selected(guc_fw)) {
+		DRM_WARN("Incompatible option enable_guc=%d - %s\n",
+			 i915_modparams.enable_guc,
+			 !HAS_GUC(dev_priv) ? "no GuC hardware" :
+					      "no GuC firmware");
+	}
+
+	/* Verify HuC firmware availability */
+	if (USES_GUC_SUBMISSION(dev_priv) && !intel_uc_fw_is_selected(huc_fw)) {
+		DRM_WARN("Incompatible option enable_guc=%d - %s\n",
+			 i915_modparams.enable_guc,
+			 !HAS_HUC(dev_priv) ? "no HuC hardware" :
+					      "no HuC firmware");
+	}
 }
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
@@ -155,6 +179,10 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (!USES_GUC(dev_priv))
 		return 0;
 
+	/* Late trap for incompatible modparam option */
+	if (!HAS_GUC(dev_priv))
+		return -ENODEV;
+
 	guc_disable_communication(guc);
 	gen9_reset_guc_interrupts(dev_priv);
 
@@ -229,12 +257,6 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 
 	/*
 	 * 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).
 	 */
 err_interrupts:
 	guc_disable_communication(guc);
@@ -247,22 +269,14 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
 
-	if (i915_modparams.enable_guc_loading > 1 ||
-	    i915_modparams.enable_guc_submission > 1) {
-		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
-		ret = -EIO;
-	} else {
-		DRM_NOTE("GuC init failed. Firmware loading disabled.\n");
-		ret = 0;
-	}
-
-	if (USES_GUC_SUBMISSION(dev_priv)) {
-		i915_modparams.enable_guc_submission = 0;
-		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
-	}
-
-	i915_modparams.enable_guc_loading = 0;
+	dev_err(dev_priv->drm.dev, "GuC initialization failed.\n");
 
+	/*
+	 * There is no fallback as either user explicitly asked for the GuC
+	 * or driver default was to run with GuC enabled.
+	 */
+	if (GEM_WARN_ON(ret == -EIO))
+		return -EINVAL;
 	return ret;
 }
 
-- 
2.7.4

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

[PATCH v2 6/7] drm/i915/huc: Load HuC only if requested

[Michal Wajdeczko]

Our new "enable_guc" modparam allows to control whenever HuC
should be loaded. However existing code will try load and
authenticate HuC always when we use the GuC. This patch is
trying to enforce modparam selection.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_huc.c | 21 +++++++++++----------
 drivers/gpu/drm/i915/intel_huc.h |  4 ++--
 drivers/gpu/drm/i915/intel_uc.c  | 17 +++++++++++++++--
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 6d0e050..abe301f 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -181,17 +181,17 @@ static int huc_ucode_xfer(struct intel_uc_fw *huc_fw, struct i915_vma *vma)
  * intel_huc_init_hw() - load HuC uCode to device
  * @huc: intel_huc structure
  *
- * Called from guc_setup() during driver loading and also after a GPU reset.
- * Be note that HuC loading must be done before GuC loading.
+ * Called from intel_uc_init_hw() during driver loading and also after a GPU
+ * reset. Be note that HuC loading must be done before GuC loading.
  *
  * The firmware image should have already been fetched into memory by the
- * earlier call to intel_huc_init(), so here we need only check that
+ * earlier call to intel_uc_init_fw(), so here we need only check that
  * is succeeded, and then transfer the image to the h/w.
  *
  */
-void intel_huc_init_hw(struct intel_huc *huc)
+int intel_huc_init_hw(struct intel_huc *huc)
 {
-	intel_uc_fw_upload(&huc->fw, huc_ucode_xfer);
+	return intel_uc_fw_upload(&huc->fw, huc_ucode_xfer);
 }
 
 /**
@@ -205,7 +205,7 @@ void intel_huc_init_hw(struct intel_huc *huc)
  * signature through intel_guc_auth_huc(). It then waits for 50ms for
  * firmware verification ACK and unpins the object.
  */
-void intel_huc_auth(struct intel_huc *huc)
+int intel_huc_auth(struct intel_huc *huc)
 {
 	struct drm_i915_private *i915 = huc_to_i915(huc);
 	struct intel_guc *guc = &i915->guc;
@@ -213,14 +213,14 @@ void intel_huc_auth(struct intel_huc *huc)
 	int ret;
 
 	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
-		return;
+		return -ENOEXEC;
 
 	vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
 				PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
 	if (IS_ERR(vma)) {
-		DRM_ERROR("failed to pin huc fw object %d\n",
-				(int)PTR_ERR(vma));
-		return;
+		ret = (int)PTR_ERR(vma);
+		DRM_ERROR("HuC: Failed to pin huc fw object %d\n", ret);
+		return ret;
 	}
 
 	ret = intel_guc_auth_huc(guc,
@@ -243,4 +243,5 @@ void intel_huc_auth(struct intel_huc *huc)
 
 out:
 	i915_vma_unpin(vma);
+	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
index 3d757bc..40039db 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -35,7 +35,7 @@ struct intel_huc {
 };
 
 void intel_huc_init_early(struct intel_huc *huc);
-void intel_huc_init_hw(struct intel_huc *huc);
-void intel_huc_auth(struct intel_huc *huc);
+int intel_huc_init_hw(struct intel_huc *huc);
+int intel_huc_auth(struct intel_huc *huc);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 8a761af..6c30a4d 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -174,6 +174,7 @@ static void guc_disable_communication(struct intel_guc *guc)
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
+	struct intel_huc *huc = &dev_priv->huc;
 	int ret, attempts;
 
 	if (!USES_GUC(dev_priv))
@@ -220,7 +221,12 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		if (ret)
 			goto err_submission;
 
-		intel_huc_init_hw(&dev_priv->huc);
+		if (USES_HUC(dev_priv)) {
+			ret = intel_huc_init_hw(huc);
+			if (ret)
+				break;
+		}
+
 		intel_guc_init_params(guc);
 		ret = intel_guc_fw_upload(guc);
 		if (ret == 0 || ret != -EAGAIN)
@@ -238,7 +244,12 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_log_capture;
 
-	intel_huc_auth(&dev_priv->huc);
+	if (USES_HUC(dev_priv)) {
+		ret = intel_huc_auth(huc);
+		if (ret)
+			goto err_interrupts;
+	}
+
 	if (USES_GUC_SUBMISSION(dev_priv)) {
 		if (i915_modparams.guc_log_level >= 0)
 			gen9_enable_guc_interrupts(dev_priv);
@@ -252,6 +263,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
 	dev_info(dev_priv->drm.dev, "GuC submission %s\n",
 		 enableddisabled(USES_GUC_SUBMISSION(dev_priv)));
+	dev_info(dev_priv->drm.dev, "HuC %s\n",
+		 enableddisabled(USES_HUC(dev_priv)));
 
 	return 0;
 
-- 
2.7.4

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

[PATCH v2 7/7] HAX enable GuC/HuC load

[Michal Wajdeczko]

Also revert ("drm/i915/guc: Assert that we switch between
known ggtt->invalidate functions")

v2: don't enable GuC on GLK

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++------
 drivers/gpu/drm/i915/i915_params.h  | 2 +-
 drivers/gpu/drm/i915/intel_uc.c     | 2 ++
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 209bb11..a5e75a3 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3590,17 +3590,13 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
 
 void i915_ggtt_enable_guc(struct drm_i915_private *i915)
 {
-	GEM_BUG_ON(i915->ggtt.invalidate != gen6_ggtt_invalidate);
-
 	i915->ggtt.invalidate = guc_ggtt_invalidate;
 }
 
 void i915_ggtt_disable_guc(struct drm_i915_private *i915)
 {
-	/* We should only be called after i915_ggtt_enable_guc() */
-	GEM_BUG_ON(i915->ggtt.invalidate != guc_ggtt_invalidate);
-
-	i915->ggtt.invalidate = gen6_ggtt_invalidate;
+	if (i915->ggtt.invalidate == guc_ggtt_invalidate)
+		i915->ggtt.invalidate = gen6_ggtt_invalidate;
 }
 
 void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 10e2f74..fd5e598 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -42,7 +42,7 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc, 0) \
+	param(int, enable_guc, -1) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 6c30a4d..76d4658 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -56,6 +56,8 @@ static int __get_default_enable_guc(struct drm_i915_private *dev_priv)
 			 (intel_uc_fw_is_selected(huc_fw) ? 2 : 0);
 
 	/* Any platform specific fine-tuning can be done here */
+	if (IS_GEMINILAKE(dev_priv))
+		enable_guc = 0; /* no firmware on CI machines */
 
 	/* Make sure that our "platform default" is well-defined */
 	GEM_BUG_ON(enable_guc < 0);
-- 
2.7.4

_______________________________________________
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 [v2,1/7] drm/i915/huc: Move firmware selection to init_early

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/7] drm/i915/huc: Move firmware selection to init_early
URL   : https://patchwork.freedesktop.org/series/34738/
State : warning

== Summary ==

Series 34738v1 series starting with [v2,1/7] drm/i915/huc: Move firmware selection to init_early
https://patchwork.freedesktop.org/api/1.0/series/34738/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-skl-6600u)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-skl-gvtdvm)
                pass       -> DMESG-WARN (fi-bxt-dsi)
                pass       -> DMESG-WARN (fi-bxt-j4205)
                pass       -> DMESG-WARN (fi-kbl-7500u) fdo#103285
                pass       -> DMESG-WARN (fi-kbl-7560u)
                pass       -> DMESG-WARN (fi-kbl-7567u)
                pass       -> DMESG-WARN (fi-kbl-r)
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713

fdo#103285 https://bugs.freedesktop.org/show_bug.cgi?id=103285
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:443s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:443s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:384s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:516s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:283s
fi-bxt-dsi       total:288  pass:257  dwarn:1   dfail:0   fail:0   skip:30  time:486s
fi-bxt-j4205     total:288  pass:258  dwarn:1   dfail:0   fail:0   skip:29  time:481s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:485s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:480s
fi-elk-e7500     total:224  pass:162  dwarn:16  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:1   dfail:0   fail:0   skip:108 time:266s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:542s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:358s
fi-hsw-4770r     total:288  pass:224  dwarn:0   dfail:0   fail:0   skip:64  time:263s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:391s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:487s
fi-ivb-3770      total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:443s
fi-kbl-7500u     total:288  pass:262  dwarn:2   dfail:0   fail:0   skip:24  time:474s
fi-kbl-7560u     total:288  pass:268  dwarn:1   dfail:0   fail:0   skip:19  time:513s
fi-kbl-7567u     total:288  pass:267  dwarn:1   dfail:0   fail:0   skip:20  time:450s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:518s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:594s
fi-skl-6260u     total:288  pass:267  dwarn:1   dfail:0   fail:0   skip:20  time:429s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:530s
fi-skl-6700hq    total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:557s
fi-skl-6700k     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:500s
fi-skl-6770hq    total:288  pass:267  dwarn:1   dfail:0   fail:0   skip:20  time:476s
fi-skl-gvtdvm    total:288  pass:264  dwarn:1   dfail:0   fail:0   skip:23  time:435s
fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:419s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:593s
fi-glk-dsi       total:162  pass:77   dwarn:0   dfail:1   fail:0   skip:83 

9e7a016a79ab9948d08e1afccb14805a848fb7df drm-tip: 2017y-12m-01d-09h-20m-42s UTC integration manifest
4f8097244eec HAX enable GuC/HuC load
17b7a62b25c9 drm/i915/huc: Load HuC only if requested
8d8ee68dbe22 drm/i915/guc: Combine enable_guc_loading|submission modparams
90d41677bb95 drm/i915/uc: Don't use -EIO to report missing firmware
1eed9c449e57 drm/i915/guc: Introduce USES_GUC_xxx helper macros
03e04733c93c drm/i915/guc: Move firmware selection to init_early
ed8b3e1c5832 drm/i915/huc: Move firmware selection to init_early

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7389/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/7] drm/i915/huc: Move firmware selection to init_early

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-01 10:33:11)
> Doing HuC firmware path selection from sanitize_options function
> is not perfect, while there is no problem with doing so during
> early init stage as we already have all needed data.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/7] drm/i915/guc: Move firmware selection to init_early

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-01 10:33:12)
> Doing GuC firmware path selection from sanitize_options function
> is not perfect, while there is no problem with doing so during
> early init stage as we already have all needed data.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/7] drm/i915/guc: Introduce USES_GUC_xxx helper macros

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-01 10:33:13)
> In the upcoming patch we will change the way how to recognize
> when GuC is in use. Using helper macros will minimize scope
> of that changes. While here, update dev_info message.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  4 ++++
>  drivers/gpu/drm/i915/i915_gem_context.c    |  4 ++--
>  drivers/gpu/drm/i915/i915_gem_gtt.c        |  2 +-
>  drivers/gpu/drm/i915/i915_irq.c            |  2 +-
>  drivers/gpu/drm/i915/intel_guc.c           |  2 +-
>  drivers/gpu/drm/i915/intel_guc_log.c       |  6 +++---
>  drivers/gpu/drm/i915/intel_gvt.c           |  2 +-
>  drivers/gpu/drm/i915/intel_uc.c            | 23 +++++++++++------------
>  drivers/gpu/drm/i915/selftests/intel_guc.c |  2 +-
>  9 files changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4c3616b..2c386c7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3237,6 +3237,10 @@ intel_info(const struct drm_i915_private *dev_priv)
>  #define HAS_HUC(dev_priv)      (HAS_GUC(dev_priv))
>  #define HAS_HUC_UCODE(dev_priv)        (HAS_GUC(dev_priv))
>  
> +/* Having a GuC is not the same as using a GuC */
> +#define USES_GUC(dev_priv)             (i915_modparams.enable_guc_loading)
> +#define USES_GUC_SUBMISSION(dev_priv)  (i915_modparams.enable_guc_submission)

I was thinking that maybe we should keep the HAS_GUC here until the end,
but afaict the modparam is always sanitized by the point we inspect it
for USES_GUC.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/7] drm/i915/uc: Don't use -EIO to report missing firmware

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-01 10:33:14)
> -EIO has special meaning and is used when we want to allow
> engine initialization to fail and mark GPU as wedged.
> Missing firmware does not fit into this scenario as this is
> permanent error not related to GPU condition.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>

Ok, keeping -EIO to mean something special is a good idea. So if upload
now fails, we abort loading of the driver with ENOEXEC.

Is that sensible? Let's say due to fs corruption, or other error, we
aren't able to upload a fw, what should we do? If we abort the driver
load at this point, the user is likely left with a blank screen. If we
use -EIO, at least KMS is still functional and the user can still
interact with the system. (If we just fell back to execlists, then the
system remains very usable.)

What is the plan for HW initialisation failure?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-01 10:33:15)
> We currently have two module parameters that control GuC:
> "enable_guc_loading" and "enable_guc_submission". Whenever
> we need submission=1, we also need loading=1. We also need
> loading=1 when we want to want to load and verify the HuC.
> 
> Lets combine above module parameters into one "enable_guc"
> modparam. New supported bit values are:
> 
>  0=disable GuC (no GuC submission, no HuC)
>  1=enable GuC submission
>  2=enable HuC load
> 
> Special value "-1" can be used to let driver decide what
> option should be enabled for given platform based on
> hardware/firmware availability or preference.
> 
> Explicit enabling any of the GuC features makes GuC load
> a required step, fallback to non-GuC mode will not be
> supported.
> 
> v2: Don't use -EIO
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |   5 +-
>  drivers/gpu/drm/i915/i915_params.c |  11 ++--
>  drivers/gpu/drm/i915/i915_params.h |   3 +-
>  drivers/gpu/drm/i915/intel_uc.c    | 100 +++++++++++++++++++++----------------
>  4 files changed, 65 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2c386c7..9162a60 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3238,8 +3238,9 @@ intel_info(const struct drm_i915_private *dev_priv)
>  #define HAS_HUC_UCODE(dev_priv)        (HAS_GUC(dev_priv))
>  
>  /* Having a GuC is not the same as using a GuC */
> -#define USES_GUC(dev_priv)             (i915_modparams.enable_guc_loading)
> -#define USES_GUC_SUBMISSION(dev_priv)  (i915_modparams.enable_guc_submission)
> +#define USES_GUC(dev_priv)             (!!(i915_modparams.enable_guc > 0))
> +#define USES_GUC_SUBMISSION(dev_priv)  (!!(i915_modparams.enable_guc & 1))
> +#define USES_HUC(dev_priv)             (!!(i915_modparams.enable_guc & 2))

* channelling Joonas

Please use a magic name for each bit and so

#define USE_GUC_SUBMISSION_BIT 0
#define USES_GUC_SUBMISSION (!!(i915_modparams.enable_guc & BIT(USE_GUC_SUBMISSION_BIT)))

Gah, that's so ugly.

or

static inline bool intel_use_guc_submission(void)
{
	return i915_modparams.enable_guc & BIT(USE_GUC_SUBMISSION_BIT);
}

which at least stops being so shouty.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 6/7] drm/i915/huc: Load HuC only if requested

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-01 10:33:16)
> Our new "enable_guc" modparam allows to control whenever HuC
> should be loaded. However existing code will try load and
> authenticate HuC always when we use the GuC. This patch is
> trying to enforce modparam selection.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
> -void intel_huc_auth(struct intel_huc *huc)
> +int intel_huc_auth(struct intel_huc *huc)
>  {
>         struct drm_i915_private *i915 = huc_to_i915(huc);
>         struct intel_guc *guc = &i915->guc;
> @@ -213,14 +213,14 @@ void intel_huc_auth(struct intel_huc *huc)
>         int ret;
>  
>         if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> -               return;
> +               return -ENOEXEC;
>  
>         vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
>                                 PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
>         if (IS_ERR(vma)) {
> -               DRM_ERROR("failed to pin huc fw object %d\n",
> -                               (int)PTR_ERR(vma));
> -               return;
> +               ret = (int)PTR_ERR(vma);

The advantage here is that (int) is now implicit.

Looks ok. I'd recommend we add the various expected combinations to
drv_module_reload.
"enable_guc=0"
"enable_guc=0x1"
"enable_guc=0x2"
"enable_guc=0x3"
etc?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams

[Michal Wajdeczko]

On Fri, 01 Dec 2017 13:36:06 +0100, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-12-01 10:33:15)
>> We currently have two module parameters that control GuC:
>> "enable_guc_loading" and "enable_guc_submission". Whenever
>> we need submission=1, we also need loading=1. We also need
>> loading=1 when we want to want to load and verify the HuC.
>>
>> Lets combine above module parameters into one "enable_guc"
>> modparam. New supported bit values are:
>>
>>  0=disable GuC (no GuC submission, no HuC)
>>  1=enable GuC submission
>>  2=enable HuC load
>>
>> Special value "-1" can be used to let driver decide what
>> option should be enabled for given platform based on
>> hardware/firmware availability or preference.
>>
>> Explicit enabling any of the GuC features makes GuC load
>> a required step, fallback to non-GuC mode will not be
>> supported.
>>
>> v2: Don't use -EIO
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h    |   5 +-
>>  drivers/gpu/drm/i915/i915_params.c |  11 ++--
>>  drivers/gpu/drm/i915/i915_params.h |   3 +-
>>  drivers/gpu/drm/i915/intel_uc.c    | 100  
>> +++++++++++++++++++++----------------
>>  4 files changed, 65 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 2c386c7..9162a60 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3238,8 +3238,9 @@ intel_info(const struct drm_i915_private  
>> *dev_priv)
>>  #define HAS_HUC_UCODE(dev_priv)        (HAS_GUC(dev_priv))
>>
>>  /* Having a GuC is not the same as using a GuC */
>> -#define USES_GUC(dev_priv)              
>> (i915_modparams.enable_guc_loading)
>> -#define USES_GUC_SUBMISSION(dev_priv)   
>> (i915_modparams.enable_guc_submission)
>> +#define USES_GUC(dev_priv)             (!!(i915_modparams.enable_guc >  
>> 0))
>> +#define USES_GUC_SUBMISSION(dev_priv)  (!!(i915_modparams.enable_guc &  
>> 1))
>> +#define USES_HUC(dev_priv)             (!!(i915_modparams.enable_guc &  
>> 2))
>
> * channelling Joonas
>
> Please use a magic name for each bit and so
>
> #define USE_GUC_SUBMISSION_BIT 0

I was considering that, but then I decided to follow existing code
(see USES_PPGTT* and enable_ppgtt where we use plain numbers only)

But if we want to start using magic values, then these values should
be defined in i915_params.h and rather in this way:

#define ENABLE_GUC_SUBMISSION   BIT(0)
#define ENABLE_GUC_HUC_LOAD     BIT(1)
         ^^^^^^^^^^
         this part matches modparam name

> #define USES_GUC_SUBMISSION (!!(i915_modparams.enable_guc &  
> BIT(USE_GUC_SUBMISSION_BIT)))
>
> Gah, that's so ugly.
>
> or
>
> static inline bool intel_use_guc_submission(void)

"intel_" ? maybe correct prefix should be "i915_" ?

> {
> 	return i915_modparams.enable_guc & BIT(USE_GUC_SUBMISSION_BIT);
> }

I assume that above will still be wrapped inside macro

#define USES_GUC_SUBMISSION(dev_priv) intel_uses_guc_submission()

>
> which at least stops being so shouty.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-01 14:09:31)
> On Fri, 01 Dec 2017 13:36:06 +0100, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2017-12-01 10:33:15)
> >> We currently have two module parameters that control GuC:
> >> "enable_guc_loading" and "enable_guc_submission". Whenever
> >> we need submission=1, we also need loading=1. We also need
> >> loading=1 when we want to want to load and verify the HuC.
> >>
> >> Lets combine above module parameters into one "enable_guc"
> >> modparam. New supported bit values are:
> >>
> >>  0=disable GuC (no GuC submission, no HuC)
> >>  1=enable GuC submission
> >>  2=enable HuC load
> >>
> >> Special value "-1" can be used to let driver decide what
> >> option should be enabled for given platform based on
> >> hardware/firmware availability or preference.
> >>
> >> Explicit enabling any of the GuC features makes GuC load
> >> a required step, fallback to non-GuC mode will not be
> >> supported.
> >>
> >> v2: Don't use -EIO
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> >> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h    |   5 +-
> >>  drivers/gpu/drm/i915/i915_params.c |  11 ++--
> >>  drivers/gpu/drm/i915/i915_params.h |   3 +-
> >>  drivers/gpu/drm/i915/intel_uc.c    | 100  
> >> +++++++++++++++++++++----------------
> >>  4 files changed, 65 insertions(+), 54 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
> >> b/drivers/gpu/drm/i915/i915_drv.h
> >> index 2c386c7..9162a60 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -3238,8 +3238,9 @@ intel_info(const struct drm_i915_private  
> >> *dev_priv)
> >>  #define HAS_HUC_UCODE(dev_priv)        (HAS_GUC(dev_priv))
> >>
> >>  /* Having a GuC is not the same as using a GuC */
> >> -#define USES_GUC(dev_priv)              
> >> (i915_modparams.enable_guc_loading)
> >> -#define USES_GUC_SUBMISSION(dev_priv)   
> >> (i915_modparams.enable_guc_submission)
> >> +#define USES_GUC(dev_priv)             (!!(i915_modparams.enable_guc >  
> >> 0))
> >> +#define USES_GUC_SUBMISSION(dev_priv)  (!!(i915_modparams.enable_guc &  
> >> 1))
> >> +#define USES_HUC(dev_priv)             (!!(i915_modparams.enable_guc &  
> >> 2))
> >
> > * channelling Joonas
> >
> > Please use a magic name for each bit and so
> >
> > #define USE_GUC_SUBMISSION_BIT 0
> 
> I was considering that, but then I decided to follow existing code
> (see USES_PPGTT* and enable_ppgtt where we use plain numbers only)

enable_ppgtt is on my list to kill. If vgpu didn't conspire against us,
it would be simpler!

> But if we want to start using magic values, then these values should
> be defined in i915_params.h and rather in this way:
> 
> #define ENABLE_GUC_SUBMISSION   BIT(0)
> #define ENABLE_GUC_HUC_LOAD     BIT(1)
>          ^^^^^^^^^^
>          this part matches modparam name

Reasonable.

> > #define USES_GUC_SUBMISSION (!!(i915_modparams.enable_guc &  
> > BIT(USE_GUC_SUBMISSION_BIT)))
> >
> > Gah, that's so ugly.
> >
> > or
> >
> > static inline bool intel_use_guc_submission(void)
> 
> "intel_" ? maybe correct prefix should be "i915_" ?
> 
> > {
> >       return i915_modparams.enable_guc & BIT(USE_GUC_SUBMISSION_BIT);
> > }
> 
> I assume that above will still be wrapped inside macro
> 
> #define USES_GUC_SUBMISSION(dev_priv) intel_uses_guc_submission()

Why? The compiler will dce functions just as well as macros; the age of
the macro is long past, so the question is just a matter of how much is
it worth continuing to cargo-cult a pattern that is long past requirement.

Even its placement in i915_drv.h is up for debate. Maintaining the
status quo is a valid argument, we just need a good reason to switch
patterns (splitting up X-thousand lines of code into manageable chunks
with consistent forward-facing interfaces is one such :).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams

[Sagar Arun Kamble]

On 12/1/2017 4:03 PM, Michal Wajdeczko wrote:
> We currently have two module parameters that control GuC:
> "enable_guc_loading" and "enable_guc_submission". Whenever
> we need submission=1, we also need loading=1. We also need
> loading=1 when we want to want to load and verify the HuC.
>
> Lets combine above module parameters into one "enable_guc"
> modparam. New supported bit values are:
>
>   0=disable GuC (no GuC submission, no HuC)
>   1=enable GuC submission
>   2=enable HuC load
>
> Special value "-1" can be used to let driver decide what
> option should be enabled for given platform based on
> hardware/firmware availability or preference.
>
> Explicit enabling any of the GuC features makes GuC load
> a required step, fallback to non-GuC mode will not be
> supported.
>
> v2: Don't use -EIO
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
<snip>
> +void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> +	int enable_guc = __get_default_enable_guc(dev_priv);
>   
>   	/* A negative value means "use platform default" */
> -	if (i915_modparams.enable_guc_submission < 0)
> -		i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> +	if (i915_modparams.enable_guc < 0)
> +		i915_modparams.enable_guc = enable_guc;
> +
> +	/* Verify GuC firmware availability */
> +	if (USES_GUC(dev_priv) && !intel_uc_fw_is_selected(guc_fw)) {
> +		DRM_WARN("Incompatible option enable_guc=%d - %s\n",
> +			 i915_modparams.enable_guc,
> +			 !HAS_GUC(dev_priv) ? "no GuC hardware" :
> +					      "no GuC firmware");
> +	}
> +
> +	/* Verify HuC firmware availability */
> +	if (USES_GUC_SUBMISSION(dev_priv) && !intel_uc_fw_is_selected(huc_fw)) {
should be USES_HUC here
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/7] drm/i915/uc: Don't use -EIO to report missing firmware

[Sagar Arun Kamble]

On 12/1/2017 6:01 PM, Chris Wilson wrote:
> Quoting Michal Wajdeczko (2017-12-01 10:33:14)
>> -EIO has special meaning and is used when we want to allow
>> engine initialization to fail and mark GPU as wedged.
>> Missing firmware does not fit into this scenario as this is
>> permanent error not related to GPU condition.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Ok, keeping -EIO to mean something special is a good idea. So if upload
> now fails, we abort loading of the driver with ENOEXEC.
>
> Is that sensible? Let's say due to fs corruption, or other error, we
> aren't able to upload a fw, what should we do? If we abort the driver
> load at this point, the user is likely left with a blank screen. If we
> use -EIO, at least KMS is still functional and the user can still
> interact with the system. (If we just fell back to execlists, then the
> system remains very usable.)
>
> What is the plan for HW initialisation failure?
Earlier we were returning -EIO from intel_uc_init_hw when GuC 
load/submission was "required" (enable_guc_loading/submission=2).
Keeping the same behavior I feel we should return -EIO if (enable_guc & 
1) (need to know that user requested "required")
I feel on auto mode (need to know user requested "auto") falling back to 
execlists makes sense as user dint enforce, so we should be returning 0 
then.
> -Chris

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

Re: [PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams

[Michal Wajdeczko]

On Fri, 01 Dec 2017 16:47:40 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

>
>
> On 12/1/2017 4:03 PM, Michal Wajdeczko wrote:
>> We currently have two module parameters that control GuC:
>> "enable_guc_loading" and "enable_guc_submission". Whenever
>> we need submission=1, we also need loading=1. We also need
>> loading=1 when we want to want to load and verify the HuC.
>>
>> Lets combine above module parameters into one "enable_guc"
>> modparam. New supported bit values are:
>>
>>   0=disable GuC (no GuC submission, no HuC)
>>   1=enable GuC submission
>>   2=enable HuC load
>>
>> Special value "-1" can be used to let driver decide what
>> option should be enabled for given platform based on
>> hardware/firmware availability or preference.
>>
>> Explicit enabling any of the GuC features makes GuC load
>> a required step, fallback to non-GuC mode will not be
>> supported.
>>
>> v2: Don't use -EIO
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> <snip>
>> +void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>> +{
>> +	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>> +	int enable_guc = __get_default_enable_guc(dev_priv);
>>     	/* A negative value means "use platform default" */
>> -	if (i915_modparams.enable_guc_submission < 0)
>> -		i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
>> +	if (i915_modparams.enable_guc < 0)
>> +		i915_modparams.enable_guc = enable_guc;
>> +
>> +	/* Verify GuC firmware availability */
>> +	if (USES_GUC(dev_priv) && !intel_uc_fw_is_selected(guc_fw)) {
>> +		DRM_WARN("Incompatible option enable_guc=%d - %s\n",
>> +			 i915_modparams.enable_guc,
>> +			 !HAS_GUC(dev_priv) ? "no GuC hardware" :
>> +					      "no GuC firmware");
>> +	}
>> +
>> +	/* Verify HuC firmware availability */
>> +	if (USES_GUC_SUBMISSION(dev_priv) &&  
>> !intel_uc_fw_is_selected(huc_fw)) {
> should be USES_HUC here

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

Re: [PATCH v2 4/7] drm/i915/uc: Don't use -EIO to report missing firmware

[Michal Wajdeczko]

On Fri, 01 Dec 2017 16:55:41 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

>
>
> On 12/1/2017 6:01 PM, Chris Wilson wrote:
>> Quoting Michal Wajdeczko (2017-12-01 10:33:14)
>>> -EIO has special meaning and is used when we want to allow
>>> engine initialization to fail and mark GPU as wedged.
>>> Missing firmware does not fit into this scenario as this is
>>> permanent error not related to GPU condition.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Ok, keeping -EIO to mean something special is a good idea. So if upload
>> now fails, we abort loading of the driver with ENOEXEC.
>>
>> Is that sensible? Let's say due to fs corruption, or other error, we
>> aren't able to upload a fw, what should we do? If we abort the driver
>> load at this point, the user is likely left with a blank screen. If we
>> use -EIO, at least KMS is still functional and the user can still
>> interact with the system. (If we just fell back to execlists, then the
>> system remains very usable.)
>>
>> What is the plan for HW initialisation failure?
> Earlier we were returning -EIO from intel_uc_init_hw when GuC  
> load/submission was "required" (enable_guc_loading/submission=2).
> Keeping the same behavior I feel we should return -EIO if (enable_guc &  
> 1) (need to know that user requested "required")
> I feel on auto mode (need to know user requested "auto") falling back to  
> execlists makes sense as user dint enforce, so we should be returning 0  
> then.

if we return 0 and use execlist then we will have mismatched state
in flags as USES_GUC will be still returning true (as the same time
we don't want to do late changes to enable_guc modparam on failure
as part of the driver was already initiated with old value)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 6/7] drm/i915/huc: Load HuC only if requested

[Sagar Arun Kamble]

On 12/1/2017 4:03 PM, Michal Wajdeczko wrote:
> Our new "enable_guc" modparam allows to control whenever HuC
> should be loaded. However existing code will try load and
> authenticate HuC always when we use the GuC. This patch is
> trying to enforce modparam selection.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
<snip>
>   int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   {
>   	struct intel_guc *guc = &dev_priv->guc;
> +	struct intel_huc *huc = &dev_priv->huc;
>   	int ret, attempts;
>   
>   	if (!USES_GUC(dev_priv))
> @@ -220,7 +221,12 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   		if (ret)
>   			goto err_submission;
>   
> -		intel_huc_init_hw(&dev_priv->huc);
> +		if (USES_HUC(dev_priv)) {
> +			ret = intel_huc_init_hw(huc);
> +			if (ret)
> +				break;
this break should be "goto err_submission" as GuC is still not ready.
looks like user has to be very careful with param now that HuC failure 
can block GuC tasks too.
> +		}
> +
>   		intel_guc_init_params(guc);
>   		ret = intel_guc_fw_upload(guc);
>   		if (ret == 0 || ret != -EAGAIN)
> @@ -238,7 +244,12 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	if (ret)
>   		goto err_log_capture;
>   
> -	intel_huc_auth(&dev_priv->huc);
> +	if (USES_HUC(dev_priv)) {
> +		ret = intel_huc_auth(huc);
> +		if (ret)
> +			goto err_interrupts;
> +	}
> +
>   	if (USES_GUC_SUBMISSION(dev_priv)) {
>   		if (i915_modparams.guc_log_level >= 0)
>   			gen9_enable_guc_interrupts(dev_priv);
> @@ -252,6 +263,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
>   	dev_info(dev_priv->drm.dev, "GuC submission %s\n",
>   		 enableddisabled(USES_GUC_SUBMISSION(dev_priv)));
> +	dev_info(dev_priv->drm.dev, "HuC %s\n",
> +		 enableddisabled(USES_HUC(dev_priv)));
>   
>   	return 0;
>   

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

Re: [PATCH v2 6/7] drm/i915/huc: Load HuC only if requested

[Michal Wajdeczko]

On Fri, 01 Dec 2017 17:39:38 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

>
>
> On 12/1/2017 4:03 PM, Michal Wajdeczko wrote:
>> Our new "enable_guc" modparam allows to control whenever HuC
>> should be loaded. However existing code will try load and
>> authenticate HuC always when we use the GuC. This patch is
>> trying to enforce modparam selection.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> <snip>
>>   int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>>   {
>>   	struct intel_guc *guc = &dev_priv->guc;
>> +	struct intel_huc *huc = &dev_priv->huc;
>>   	int ret, attempts;
>>     	if (!USES_GUC(dev_priv))
>> @@ -220,7 +221,12 @@ int intel_uc_init_hw(struct drm_i915_private  
>> *dev_priv)
>>   		if (ret)
>>   			goto err_submission;
>>   -		intel_huc_init_hw(&dev_priv->huc);
>> +		if (USES_HUC(dev_priv)) {
>> +			ret = intel_huc_init_hw(huc);
>> +			if (ret)
>> +				break;
> this break should be "goto err_submission" as GuC is still not ready.

but GuC may also be not ready due to guc fw upload failure ...

> looks like user has to be very careful with param now that HuC failure  
> can block GuC tasks too.

yep, this is lowlight of this patch, as all selected options are now  
'required'

>> +		}
>> +
>>   		intel_guc_init_params(guc);
>>   		ret = intel_guc_fw_upload(guc);
>>   		if (ret == 0 || ret != -EAGAIN)
>> @@ -238,7 +244,12 @@ int intel_uc_init_hw(struct drm_i915_private  
>> *dev_priv)
>>   	if (ret)
>>   		goto err_log_capture;
>>   -	intel_huc_auth(&dev_priv->huc);
>> +	if (USES_HUC(dev_priv)) {
>> +		ret = intel_huc_auth(huc);
>> +		if (ret)
>> +			goto err_interrupts;
>> +	}
>> +
>>   	if (USES_GUC_SUBMISSION(dev_priv)) {
>>   		if (i915_modparams.guc_log_level >= 0)
>>   			gen9_enable_guc_interrupts(dev_priv);
>> @@ -252,6 +263,8 @@ int intel_uc_init_hw(struct drm_i915_private  
>> *dev_priv)
>>   		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
>>   	dev_info(dev_priv->drm.dev, "GuC submission %s\n",
>>   		 enableddisabled(USES_GUC_SUBMISSION(dev_priv)));
>> +	dev_info(dev_priv->drm.dev, "HuC %s\n",
>> +		 enableddisabled(USES_HUC(dev_priv)));
>>     	return 0;
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx