[PATCH v3 1/8] drm/i915/huc: Move firmware selection to init_early

[PATCH v3 1/8] 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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 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 594fd14..bd4eea5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3234,6 +3234,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 v3 2/8] 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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 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 d08e760..df86907 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 v3 3/8] 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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 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 bd4eea5..937fa02 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3239,6 +3239,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 df86907..177ee69 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 v3 4/8] drm/i915/uc: Don't fetch GuC firmware if no plan to use GuC

[Michal Wajdeczko]

If we don't plan to use GuC then we should not try to fetch GuC and
HuC firmwares. We can save memory and avoid possible dmesg noise.

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.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index ed2dd76..c3981aa 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -86,12 +86,18 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 
 void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 {
+	if (!USES_GUC(dev_priv))
+		return;
+
 	intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
 	intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
 }
 
 void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
 {
+	if (!USES_GUC(dev_priv))
+		return;
+
 	intel_uc_fw_fini(&dev_priv->guc.fw);
 	intel_uc_fw_fini(&dev_priv->huc.fw);
 }
-- 
2.7.4

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

[PATCH v3 5/8] 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.

However here at this function we should return error code
that corresponds to upload status only, as any decision how
to handle missing firmware should be done higher level function
(silent fallback to non-GuC mode, fail into wedged mode, or
abort driver load with fatal error).

v2: commit message update (Michal)

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 v3 6/8] 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
v3: define modparam bits (Chris)

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 |   7 ++-
 drivers/gpu/drm/i915/intel_uc.c    | 109 ++++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_uc.h    |  19 +++++++
 5 files changed, 96 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 937fa02..02551c7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3240,8 +3240,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)		intel_uc_is_using_guc()
+#define USES_GUC_SUBMISSION(dev_priv)	intel_uc_is_using_guc_submission()
+#define USES_HUC(dev_priv)		intel_uc_is_using_huc()
 
 #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 7bc5386..8dfea03 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -147,13 +147,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 c48c88b..792ce26 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -25,8 +25,12 @@
 #ifndef _I915_PARAMS_H_
 #define _I915_PARAMS_H_
 
+#include <linux/bitops.h>
 #include <linux/cache.h> /* for __read_mostly */
 
+#define ENABLE_GUC_SUBMISSION		BIT(0)
+#define ENABLE_GUC_LOAD_HUC		BIT(1)
+
 #define I915_PARAMS_FOR_EACH(param) \
 	param(char *, vbt_firmware, NULL) \
 	param(int, modeset, -1) \
@@ -41,8 +45,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 c3981aa..7dfc7e0 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -47,35 +47,65 @@ 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_platform_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;
+	int enable_guc = 0;
 
-		i915_modparams.enable_guc_loading = 0;
-		i915_modparams.enable_guc_submission = 0;
-		return;
-	}
+	/* Default is to enable GuC/HuC if we know their firmwares */
+	if (intel_uc_fw_is_selected(guc_fw))
+		enable_guc |= ENABLE_GUC_SUBMISSION;
+	if (intel_uc_fw_is_selected(huc_fw))
+		enable_guc |= ENABLE_GUC_LOAD_HUC;
 
-	/* A negative value means "use platform default" */
-	if (i915_modparams.enable_guc_loading < 0)
-		i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
+	/* Any platform specific fine-tuning can be done here */
 
-	/* 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;
 
 	/* 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 = __get_platform_enable_guc(dev_priv);
+
+	DRM_DEBUG_DRIVER("enable_guc=%d (submission:%s huc:%s)\n",
+			 i915_modparams.enable_guc,
+			 yesno(intel_uc_is_using_guc_submission()),
+			 yesno(intel_uc_is_using_huc()));
+
+	/* Verify GuC firmware availability */
+	if (intel_uc_is_using_guc() && !intel_uc_fw_is_selected(guc_fw)) {
+		DRM_WARN("Incompatible option detected: enable_guc=%d, %s!\n",
+			 i915_modparams.enable_guc,
+			 !HAS_GUC(dev_priv) ? "no GuC hardware" :
+					      "no GuC firmware");
+	}
+
+	/* Verify HuC firmware availability */
+	if (intel_uc_is_using_huc() && !intel_uc_fw_is_selected(huc_fw)) {
+		DRM_WARN("Incompatible option detected: enable_guc=%d, %s!\n",
+			 i915_modparams.enable_guc,
+			 !HAS_HUC(dev_priv) ? "no HuC hardware" :
+					      "no HuC firmware");
+	}
+
+	/* Make sure that sanitization was done */
+	GEM_BUG_ON(i915_modparams.enable_guc < 0);
 }
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
@@ -161,6 +191,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (!USES_GUC(dev_priv))
 		return 0;
 
+	if (!HAS_GUC(dev_priv)) {
+		ret = -ENODEV;
+		goto err_out;
+	}
+
 	guc_disable_communication(guc);
 	gen9_reset_guc_interrupts(dev_priv);
 
@@ -235,12 +270,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);
@@ -252,23 +281,15 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		intel_guc_submission_fini(guc);
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
+err_out:
+	/*
+	 * Note that there is no fallback as either user explicitly asked for
+	 * the GuC or driver default option was to run with the GuC enabled.
+	 */
+	if (GEM_WARN_ON(ret == -EIO))
+		ret = -EINVAL;
 
-	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 %d\n", ret);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index e18d3bb..a995d60 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -26,6 +26,7 @@
 
 #include "intel_guc.h"
 #include "intel_huc.h"
+#include "i915_params.h"
 
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
@@ -35,4 +36,22 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
 int intel_uc_init_hw(struct drm_i915_private *dev_priv);
 void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
 
+static inline bool intel_uc_is_using_guc(void)
+{
+	GEM_BUG_ON(i915_modparams.enable_guc < 0);
+	return i915_modparams.enable_guc > 0;
+}
+
+static inline bool intel_uc_is_using_guc_submission(void)
+{
+	GEM_BUG_ON(i915_modparams.enable_guc < 0);
+	return !!(i915_modparams.enable_guc & ENABLE_GUC_SUBMISSION);
+}
+
+static inline bool intel_uc_is_using_huc(void)
+{
+	GEM_BUG_ON(i915_modparams.enable_guc < 0);
+	return !!(i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC);
+}
+
 #endif
-- 
2.7.4

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

[PATCH v3 7/8] 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.

v2: no need to cast PTR_ERR (Chris)
    fetch/fini only if required (Michal)
    fix wrong break (Sagar)

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  | 25 +++++++++++++++++++++----
 3 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 6d0e050..974be3d 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 = 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 7dfc7e0..49bccc9 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -119,7 +119,9 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 	if (!USES_GUC(dev_priv))
 		return;
 
-	intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
+	if (USES_HUC(dev_priv))
+		intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
+
 	intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
 }
 
@@ -129,7 +131,9 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
 		return;
 
 	intel_uc_fw_fini(&dev_priv->guc.fw);
-	intel_uc_fw_fini(&dev_priv->huc.fw);
+
+	if (USES_HUC(dev_priv))
+		intel_uc_fw_fini(&dev_priv->huc.fw);
 }
 
 /**
@@ -186,6 +190,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))
@@ -233,7 +238,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)
+				goto err_submission;
+		}
+
 		intel_guc_init_params(guc);
 		ret = intel_guc_fw_upload(guc);
 		if (ret == 0 || ret != -EAGAIN)
@@ -251,7 +261,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);
@@ -265,6 +280,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 v3 8/8] 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 792ce26..9725c5a 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -45,7 +45,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 49bccc9..22b0afe 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -60,6 +60,8 @@ static int __get_platform_enable_guc(struct drm_i915_private *dev_priv)
 		enable_guc |= ENABLE_GUC_LOAD_HUC;
 
 	/* Any platform specific fine-tuning can be done here */
+	if (IS_GEMINILAKE(dev_priv))
+		enable_guc = 0; /* no firmware on CI machines */
 
 	return enable_guc;
 }
-- 
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 [v3,1/8] drm/i915/huc: Move firmware selection to init_early

[Patchwork]

== Series Details ==

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

== Summary ==

Series 34919v1 series starting with [v3,1/8] drm/i915/huc: Move firmware selection to init_early
https://patchwork.freedesktop.org/api/1.0/series/34919/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-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 kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-hsw-4770) fdo#103375

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

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:439s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:381s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:515s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:281s
fi-bxt-j4205     total:288  pass:258  dwarn:1   dfail:0   fail:0   skip:29  time:484s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:489s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:467s
fi-elk-e7500     total:224  pass:163  dwarn:15  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:178  dwarn:1   dfail:0   fail:1   skip:108 time:272s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-hsw-4770      total:246  pass:222  dwarn:0   dfail:0   fail:0   skip:23 
fi-hsw-4770r     total:288  pass:224  dwarn:0   dfail:0   fail:0   skip:64  time:258s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:398s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-ivb-3770      total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:444s
fi-kbl-7500u     total:288  pass:262  dwarn:2   dfail:0   fail:0   skip:24  time:469s
fi-kbl-7560u     total:288  pass:268  dwarn:1   dfail:0   fail:0   skip:19  time:514s
fi-kbl-7567u     total:288  pass:267  dwarn:1   dfail:0   fail:0   skip:20  time:462s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:522s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:590s
fi-skl-6260u     total:288  pass:267  dwarn:1   dfail:0   fail:0   skip:20  time:432s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:526s
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:499s
fi-skl-6770hq    total:288  pass:267  dwarn:1   dfail:0   fail:0   skip:20  time:487s
fi-skl-gvtdvm    total:288  pass:264  dwarn:1   dfail:0   fail:0   skip:23  time:442s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:417s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:601s
fi-cnl-y         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:640s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:488s
fi-bdw-gvtdvm failed to collect. IGT log at Patchwork_7413/fi-bdw-gvtdvm/igt.log
fi-bxt-dsi failed to collect. IGT log at Patchwork_7413/fi-bxt-dsi/igt.log
fi-snb-2520m failed to collect. IGT log at Patchwork_7413/fi-snb-2520m/igt.log

0d0fe916f52ad8f05dddab384ae7c90bb62ebac4 drm-tip: 2017y-12m-05d-14h-52m-17s UTC integration manifest
e3794fa68059 HAX enable GuC/HuC load
e1cba63a03bc drm/i915/huc: Load HuC only if requested
8f18d9dfba70 drm/i915/guc: Combine enable_guc_loading|submission modparams
1c7710c959f8 drm/i915/uc: Don't use -EIO to report missing firmware
ee26419440d7 drm/i915/uc: Don't fetch GuC firmware if no plan to use GuC
a9cbe0b82523 drm/i915/guc: Introduce USES_GUC_xxx helper macros
ea46ab5331e9 drm/i915/guc: Move firmware selection to init_early
02c22700f17b drm/i915/huc: Move firmware selection to init_early

== Logs ==

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

Re: [PATCH v3 8/8] HAX enable GuC/HuC load

[Rodrigo Vivi]

Is this a real attempt or enabling GuC by default or is it only
for CI validating the series?

If it is the second option I'd like to see CI testing
this series without this patch here as well to make sure
these changes are not breaking any of the current default flow.

One case or the other I believe we should have more info here
on the commit message.

Thanks,
Rodrigo.

On Tue, Dec 05, 2017 at 04:38:44PM +0000, Michal Wajdeczko wrote:
> 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 792ce26..9725c5a 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -45,7 +45,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 49bccc9..22b0afe 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -60,6 +60,8 @@ static int __get_platform_enable_guc(struct drm_i915_private *dev_priv)
>  		enable_guc |= ENABLE_GUC_LOAD_HUC;
>  
>  	/* Any platform specific fine-tuning can be done here */
> +	if (IS_GEMINILAKE(dev_priv))
> +		enable_guc = 0; /* no firmware on CI machines */
>  
>  	return enable_guc;
>  }
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 4/8] drm/i915/uc: Don't fetch GuC firmware if no plan to use GuC

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-05 16:38:40)
> If we don't plan to use GuC then we should not try to fetch GuC and
> HuC firmwares. We can save memory and avoid possible dmesg noise.
> 
> 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 v3 3/8] drm/i915/guc: Introduce USES_GUC_xxx helper macros

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-05 16:38:39)
> 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>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

... but can we please stop it with dev_priv :) We don't use it now or
later, we are just making it more complicated for no imminent benefit,
right?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 6/8] drm/i915/guc: Combine enable_guc_loading|submission modparams

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-05 16:38:42)
> -void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
> +static int __get_platform_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;
> +       int enable_guc = 0;
>  
> -               i915_modparams.enable_guc_loading = 0;
> -               i915_modparams.enable_guc_submission = 0;
> -               return;
> -       }
> +       /* Default is to enable GuC/HuC if we know their firmwares */
> +       if (intel_uc_fw_is_selected(guc_fw))
> +               enable_guc |= ENABLE_GUC_SUBMISSION;
> +       if (intel_uc_fw_is_selected(huc_fw))
> +               enable_guc |= ENABLE_GUC_LOAD_HUC;
>  
> -       /* A negative value means "use platform default" */
> -       if (i915_modparams.enable_guc_loading < 0)
> -               i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> +       /* Any platform specific fine-tuning can be done here */
>  
> -       /* 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;
>  
>         /* 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 = __get_platform_enable_guc(dev_priv);
> +
> +       DRM_DEBUG_DRIVER("enable_guc=%d (submission:%s huc:%s)\n",
> +                        i915_modparams.enable_guc,
> +                        yesno(intel_uc_is_using_guc_submission()),
> +                        yesno(intel_uc_is_using_huc()));
> +
> +       /* Verify GuC firmware availability */
> +       if (intel_uc_is_using_guc() && !intel_uc_fw_is_selected(guc_fw)) {
> +               DRM_WARN("Incompatible option detected: enable_guc=%d, %s!\n",
> +                        i915_modparams.enable_guc,
> +                        !HAS_GUC(dev_priv) ? "no GuC hardware" :
> +                                             "no GuC firmware");
> +       }
> +
> +       /* Verify HuC firmware availability */
> +       if (intel_uc_is_using_huc() && !intel_uc_fw_is_selected(huc_fw)) {
> +               DRM_WARN("Incompatible option detected: enable_guc=%d, %s!\n",
> +                        i915_modparams.enable_guc,
> +                        !HAS_HUC(dev_priv) ? "no HuC hardware" :
> +                                             "no HuC firmware");
> +       }

With the intent that after the warning, the uc load will fail and the
module load will then abort? Just to be clear.

> +
> +       /* Make sure that sanitization was done */
> +       GEM_BUG_ON(i915_modparams.enable_guc < 0);
>  }

> +static inline bool intel_uc_is_using_guc_submission(void)
> +{
> +       GEM_BUG_ON(i915_modparams.enable_guc < 0);
> +       return !!(i915_modparams.enable_guc & ENABLE_GUC_SUBMISSION);

Equivalent to a plain return i915_modparms.enable_guc &
ENABLE_GUC_SUBMISSION thanks to the implicit (bool).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 7/8] drm/i915/huc: Load HuC only if requested

[Sagar Arun Kamble]

On 12/5/2017 10:08 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.
>
> v2: no need to cast PTR_ERR (Chris)
>      fetch/fini only if required (Michal)
>      fix wrong break (Sagar)
>
> 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>

>   /**
> @@ -186,6 +190,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))
> @@ -233,7 +238,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)
> +				goto err_submission;
> +		}
> +
>   		intel_guc_init_params(guc);
>   		ret = intel_guc_fw_upload(guc);
>   		if (ret == 0 || ret != -EAGAIN)
> @@ -251,7 +261,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;

I think we need to create new label "err_communication" and jump there since interrupts are not yet enabled.
Planning to move interrupts enabling before enabling communication in the other series - https://patchwork.freedesktop.org/patch/183349/

With that updated patch looks good to me.
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>

> +	}
> +
>   	if (USES_GUC_SUBMISSION(dev_priv)) {
>   		if (i915_modparams.guc_log_level >= 0)
>   			gen9_enable_guc_interrupts(dev_priv);
> @@ -265,6 +280,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 v3 3/8] drm/i915/guc: Introduce USES_GUC_xxx helper macros

[Michal Wajdeczko]

On Tue, 05 Dec 2017 23:43:19 +0100, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-12-05 16:38:39)
>> 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>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> ... but can we please stop it with dev_priv :) We don't use it now or
> later, we are just making it more complicated for no imminent benefit,
> right?

Note that there are few benefits for keeping dev_priv:

a) it nicely matches all other existing HAS_|USES_ macros
    (even if dev_priv is not used/required: see USES_PPGTT or HAS_AUX_IRQ)
    (yes I know that you want to kill former and later is not widely used ;)
b) as "uses" suggests that it reflects state of the driver, I hope one day
    we will stop relying on modparam and read actual state of driver from
    dev_priv (or struct guc or struct uc or something else that can be
    reached out from this dev_priv)

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

Re: [PATCH v3 3/8] drm/i915/guc: Introduce USES_GUC_xxx helper macros

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-06 11:24:33)
> On Tue, 05 Dec 2017 23:43:19 +0100, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2017-12-05 16:38:39)
> >> 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>
> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > ... but can we please stop it with dev_priv :) We don't use it now or
> > later, we are just making it more complicated for no imminent benefit,
> > right?
> 
> Note that there are few benefits for keeping dev_priv:
> 
> a) it nicely matches all other existing HAS_|USES_ macros
>     (even if dev_priv is not used/required: see USES_PPGTT or HAS_AUX_IRQ)
>     (yes I know that you want to kill former and later is not widely used ;)
> b) as "uses" suggests that it reflects state of the driver, I hope one day
>     we will stop relying on modparam and read actual state of driver from
>     dev_priv (or struct guc or struct uc or something else that can be
>     reached out from this dev_priv)

But we don't and we are adding it to places where we didn't need, if I
remember the patches correctly. My concern is that we are creating work
and not saving it, as we^W I don't have a design for the future derived
state checks.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 6/8] drm/i915/guc: Combine enable_guc_loading|submission modparams

[Michal Wajdeczko]

On Tue, 05 Dec 2017 23:47:21 +0100, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-12-05 16:38:42)
>> -void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>> +static int __get_platform_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;
>> +       int enable_guc = 0;
>>
>> -               i915_modparams.enable_guc_loading = 0;
>> -               i915_modparams.enable_guc_submission = 0;
>> -               return;
>> -       }
>> +       /* Default is to enable GuC/HuC if we know their firmwares */
>> +       if (intel_uc_fw_is_selected(guc_fw))
>> +               enable_guc |= ENABLE_GUC_SUBMISSION;
>> +       if (intel_uc_fw_is_selected(huc_fw))
>> +               enable_guc |= ENABLE_GUC_LOAD_HUC;
>>
>> -       /* A negative value means "use platform default" */
>> -       if (i915_modparams.enable_guc_loading < 0)
>> -               i915_modparams.enable_guc_loading =  
>> HAS_GUC_UCODE(dev_priv);
>> +       /* Any platform specific fine-tuning can be done here */
>>
>> -       /* 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;
>>
>>         /* 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 =  
>> __get_platform_enable_guc(dev_priv);
>> +
>> +       DRM_DEBUG_DRIVER("enable_guc=%d (submission:%s huc:%s)\n",
>> +                        i915_modparams.enable_guc,
>> +                        yesno(intel_uc_is_using_guc_submission()),
>> +                        yesno(intel_uc_is_using_huc()));
>> +
>> +       /* Verify GuC firmware availability */
>> +       if (intel_uc_is_using_guc() &&  
>> !intel_uc_fw_is_selected(guc_fw)) {
>> +               DRM_WARN("Incompatible option detected: enable_guc=%d,  
>> %s!\n",
>> +                        i915_modparams.enable_guc,
>> +                        !HAS_GUC(dev_priv) ? "no GuC hardware" :
>> +                                             "no GuC firmware");
>> +       }
>> +
>> +       /* Verify HuC firmware availability */
>> +       if (intel_uc_is_using_huc() &&  
>> !intel_uc_fw_is_selected(huc_fw)) {
>> +               DRM_WARN("Incompatible option detected: enable_guc=%d,  
>> %s!\n",
>> +                        i915_modparams.enable_guc,
>> +                        !HAS_HUC(dev_priv) ? "no HuC hardware" :
>> +                                             "no HuC firmware");
>> +       }
>
> With the intent that after the warning, the uc load will fail and the
> module load will then abort? Just to be clear.
>

Yes. As I'm not sure that we should ignore explicit user options and
pretend that nothing really happen and continue with some fallback.

But I must admit that with this patch we can upset users when module is
loaded with enable_guc=-1(auto) and driver has corresponding GuC/HuC fw
definitions but firmware blobs are not present on the target machine.

Then we can wrongly read these options as explicit and abort module load
due to fw upload failure.

But impact of this issue can be minimized if we try to fetch GuC/HuC
firmwares much earlier (init_early). Then we can quickly update our
"preferred" auto options into "available". However, if we fail on any
later stage, after we decided to enable GuC/Huc, there still will be
no fallback and we will abort driver load.

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

Re: [PATCH v3 6/8] drm/i915/guc: Combine enable_guc_loading|submission modparams

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-06 12:10:15)
> On Tue, 05 Dec 2017 23:47:21 +0100, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2017-12-05 16:38:42)
> >> -void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
> >> +static int __get_platform_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;
> >> +       int enable_guc = 0;
> >>
> >> -               i915_modparams.enable_guc_loading = 0;
> >> -               i915_modparams.enable_guc_submission = 0;
> >> -               return;
> >> -       }
> >> +       /* Default is to enable GuC/HuC if we know their firmwares */
> >> +       if (intel_uc_fw_is_selected(guc_fw))
> >> +               enable_guc |= ENABLE_GUC_SUBMISSION;
> >> +       if (intel_uc_fw_is_selected(huc_fw))
> >> +               enable_guc |= ENABLE_GUC_LOAD_HUC;
> >>
> >> -       /* A negative value means "use platform default" */
> >> -       if (i915_modparams.enable_guc_loading < 0)
> >> -               i915_modparams.enable_guc_loading =  
> >> HAS_GUC_UCODE(dev_priv);
> >> +       /* Any platform specific fine-tuning can be done here */
> >>
> >> -       /* 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;
> >>
> >>         /* 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 =  
> >> __get_platform_enable_guc(dev_priv);
> >> +
> >> +       DRM_DEBUG_DRIVER("enable_guc=%d (submission:%s huc:%s)\n",
> >> +                        i915_modparams.enable_guc,
> >> +                        yesno(intel_uc_is_using_guc_submission()),
> >> +                        yesno(intel_uc_is_using_huc()));
> >> +
> >> +       /* Verify GuC firmware availability */
> >> +       if (intel_uc_is_using_guc() &&  
> >> !intel_uc_fw_is_selected(guc_fw)) {
> >> +               DRM_WARN("Incompatible option detected: enable_guc=%d,  
> >> %s!\n",
> >> +                        i915_modparams.enable_guc,
> >> +                        !HAS_GUC(dev_priv) ? "no GuC hardware" :
> >> +                                             "no GuC firmware");
> >> +       }
> >> +
> >> +       /* Verify HuC firmware availability */
> >> +       if (intel_uc_is_using_huc() &&  
> >> !intel_uc_fw_is_selected(huc_fw)) {
> >> +               DRM_WARN("Incompatible option detected: enable_guc=%d,  
> >> %s!\n",
> >> +                        i915_modparams.enable_guc,
> >> +                        !HAS_HUC(dev_priv) ? "no HuC hardware" :
> >> +                                             "no HuC firmware");
> >> +       }
> >
> > With the intent that after the warning, the uc load will fail and the
> > module load will then abort? Just to be clear.
> >
> 
> Yes. As I'm not sure that we should ignore explicit user options and
> pretend that nothing really happen and continue with some fallback.
> 
> But I must admit that with this patch we can upset users when module is
> loaded with enable_guc=-1(auto) and driver has corresponding GuC/HuC fw
> definitions but firmware blobs are not present on the target machine.
> 
> Then we can wrongly read these options as explicit and abort module load
> due to fw upload failure.
> 
> But impact of this issue can be minimized if we try to fetch GuC/HuC
> firmwares much earlier (init_early). Then we can quickly update our
> "preferred" auto options into "available". However, if we fail on any
> later stage, after we decided to enable GuC/Huc, there still will be
> no fallback and we will abort driver load.

That worries me. But not enough to not give a r-b
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I have nightmares of users bricking their computers just because an
update goes wrong. Or more likely a kernel is updated without the
accompanying linux-firmware. I think we should definitely be testing
module-load with incorrect firmware and ensure we don't leave a system
with a blank-screen-of-doom.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 5/8] drm/i915/uc: Don't use -EIO to report missing firmware

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-05 16:38:41)
> -EIO has special meaning and is used when we want to allow
> engine initialization to fail and mark GPU as wedged.
> 
> However here at this function we should return error code
> that corresponds to upload status only, as any decision how
> to handle missing firmware should be done higher level function
> (silent fallback to non-GuC mode, fail into wedged mode, or
> abort driver load with fatal error).
> 
> v2: commit message update (Michal)
> 
> 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>

With my fears expressed elsewhere,
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 v3 3/8] drm/i915/guc: Introduce USES_GUC_xxx helper macros

[Chris Wilson]

Quoting Chris Wilson (2017-12-06 11:32:01)
> Quoting Michal Wajdeczko (2017-12-06 11:24:33)
> > On Tue, 05 Dec 2017 23:43:19 +0100, Chris Wilson  
> > <chris@chris-wilson.co.uk> wrote:
> > 
> > > Quoting Michal Wajdeczko (2017-12-05 16:38:39)
> > >> 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>
> > >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > >
> > > ... but can we please stop it with dev_priv :) We don't use it now or
> > > later, we are just making it more complicated for no imminent benefit,
> > > right?
> > 
> > Note that there are few benefits for keeping dev_priv:
> > 
> > a) it nicely matches all other existing HAS_|USES_ macros
> >     (even if dev_priv is not used/required: see USES_PPGTT or HAS_AUX_IRQ)
> >     (yes I know that you want to kill former and later is not widely used ;)
> > b) as "uses" suggests that it reflects state of the driver, I hope one day
> >     we will stop relying on modparam and read actual state of driver from
> >     dev_priv (or struct guc or struct uc or something else that can be
> >     reached out from this dev_priv)
> 
> But we don't and we are adding it to places where we didn't need, if I
> remember the patches correctly. My concern is that we are creating work
> and not saving it, as we^W I don't have a design for the future derived
> state checks.

To be clear, I'm happy if you see value in keeping dev_priv. Just
whinging, because first its dev_priv! and I could see no reason why we
need it. (So whether it is a "stitch in time" is debatable, it may
equally just cause more work later.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx