[PATCH v5 00/10] GuC Scrub vol. 1

[PATCH v5 00/10] GuC Scrub vol. 1

[Arkadiusz Hiler]

General GuC/HuC cleanup simplifying logic, and moving chunks around as the area
got pretty rusty.

A lot of logic were extracted from intel_guc_load() to other functions - it did
not only handle the actual loading but had WA implementations and the code
that enabled submission baked into it.

This is the first part of effort to clean it up.

v2: rebase after HuC merge + feedback
v3: even more renaming that aims to make things more semantic
v4: some naming improvements, some bikeshedding
v5: coding style, some cleanup
    module params for huc and guc firmware path,
    separate fw select step from actual prepare

Arkadiusz Hiler (10):
  drm/i915/uc: Rename intel_?uc_{setup,load}() to _init_hw()
  drm/i915/uc: Drop superfluous externs in intel_uc.h
  drm/i915/huc: Add huc_to_i915
  drm/i915/uc: Move intel_uc_fw_fetch() to intel_uc.c
  drm/i915/uc: Introduce intel_uc_init_fw()
  drm/i915/guc: Extract param logic form guc_init_fw()
  drm/i915/guc: Simplify intel_guc_init_hw()
  drm/i915/uc: Simplify firmware path handling
  drm/i915/uc: Separate firmware selection and preparation
  drm/i915/uc: Add params for specifying firmware

 drivers/gpu/drm/i915/i915_drv.c         |   5 +-
 drivers/gpu/drm/i915/i915_drv.h         |   5 +
 drivers/gpu/drm/i915/i915_gem.c         |   2 +-
 drivers/gpu/drm/i915/i915_params.c      |  10 +
 drivers/gpu/drm/i915/i915_params.h      |   2 +
 drivers/gpu/drm/i915/intel_guc_loader.c | 364 ++++----------------------------
 drivers/gpu/drm/i915/intel_huc.c        |  69 +++---
 drivers/gpu/drm/i915/intel_uc.c         | 288 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |  27 ++-
 9 files changed, 394 insertions(+), 378 deletions(-)

-- 
2.9.3

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

[PATCH 01/10] drm/i915/uc: Rename intel_?uc_{setup, load}() to _init_hw()

[Arkadiusz Hiler]

GuC historically has two "startup" functions called _init() and _setup()

Then HuC came with it's _init() and _load().

This commit renames intel_guc_setup() and intel_huc_load() to
*uc_init_hw() as they called from the i915_gem_init_hw().

The aim is to be consistent in that entry points called during
particular driver init phases (e.g. init_hw) are all suffixed by that
phase. When reading the leaf functions, it should be clear at what stage
during the driver load it is called and therefore what operations are
legal at that point.

v2: commit message update (Chris Wilson)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |  2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 14 +++++++-------
 drivers/gpu/drm/i915/intel_huc.c        |  6 +++---
 drivers/gpu/drm/i915/intel_uc.h         |  4 ++--
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a816700..5b36524 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4452,7 +4452,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 	intel_mocs_init_l3cc_table(dev_priv);
 
 	/* We can't enable contexts until all firmware is loaded */
-	ret = intel_guc_setup(dev_priv);
+	ret = intel_guc_init_hw(dev_priv);
 	if (ret)
 		goto out;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 9885f76..9f09e26 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -428,19 +428,19 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_guc_setup() - finish preparing the GuC for activity
+ * intel_guc_init_hw() - finish preparing the GuC for activity
  * @dev_priv:	i915 device private
  *
- * Called from gem_init_hw() during driver loading and also after a GPU reset.
+ * Called during driver loading and also after a GPU reset.
  *
  * The main action required here it to load the GuC uCode into the device.
  * The firmware image should have already been fetched into memory by the
- * earlier call to intel_guc_init(), so here we need only check that worked,
- * and then transfer the image to the h/w.
+ * earlier call to intel_guc_init(), so here we need only check that
+ * worked, and then transfer the image to the h/w.
  *
  * Return:	non-zero code on error
  */
-int intel_guc_setup(struct drm_i915_private *dev_priv)
+int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 	const char *fw_path = guc_fw->path;
@@ -506,7 +506,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 		if (err)
 			goto fail;
 
-		intel_huc_load(dev_priv);
+		intel_huc_init_hw(dev_priv);
 		err = guc_ucode_xfer(dev_priv);
 		if (!err)
 			break;
@@ -729,7 +729,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
  * Called early during driver load, but after GEM is initialised.
  *
  * The firmware will be transferred to the GuC's memory later,
- * when intel_guc_setup() is called.
+ * when intel_guc_init_hw() is called.
  */
 void intel_guc_init(struct drm_i915_private *dev_priv)
 {
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index c28543d..babe0eb 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -150,7 +150,7 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
  * is not capable or driver yet support it. And there will be no error message
  * for INTEL_UC_FIRMWARE_NONE cases.
  *
- * The DMA-copying to HW is done later when intel_huc_load() is called.
+ * The DMA-copying to HW is done later when intel_huc_init_hw() is called.
  */
 void intel_huc_init(struct drm_i915_private *dev_priv)
 {
@@ -191,7 +191,7 @@ void intel_huc_init(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_huc_load() - load HuC uCode to device
+ * intel_huc_init_hw() - load HuC uCode to device
  * @dev_priv: the drm_i915_private device
  *
  * Called from guc_setup() during driver loading and also after a GPU reset.
@@ -203,7 +203,7 @@ void intel_huc_init(struct drm_i915_private *dev_priv)
  *
  * Return:	non-zero code on error
  */
-int intel_huc_load(struct drm_i915_private *dev_priv)
+int intel_huc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
 	int err;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index d74f4d3..dd34a1b 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -190,7 +190,7 @@ int intel_guc_sample_forcewake(struct intel_guc *guc);
 
 /* intel_guc_loader.c */
 extern void intel_guc_init(struct drm_i915_private *dev_priv);
-extern int intel_guc_setup(struct drm_i915_private *dev_priv);
+extern int intel_guc_init_hw(struct drm_i915_private *dev_priv);
 extern void intel_guc_fini(struct drm_i915_private *dev_priv);
 extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
 extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
@@ -225,7 +225,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 /* intel_huc.c */
 void intel_huc_init(struct drm_i915_private *dev_priv);
 void intel_huc_fini(struct drm_i915_private  *dev_priv);
-int intel_huc_load(struct drm_i915_private *dev_priv);
+int intel_huc_init_hw(struct drm_i915_private *dev_priv);
 void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
 
 #endif
-- 
2.9.3

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

[PATCH 02/10] drm/i915/uc: Drop superfluous externs in intel_uc.h

[Arkadiusz Hiler]

Externs are implicit and we generally try to avoid them.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uc.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index dd34a1b..41b7351 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -189,12 +189,12 @@ int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 
 /* intel_guc_loader.c */
-extern void intel_guc_init(struct drm_i915_private *dev_priv);
-extern int intel_guc_init_hw(struct drm_i915_private *dev_priv);
-extern void intel_guc_fini(struct drm_i915_private *dev_priv);
-extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
-extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
-extern int intel_guc_resume(struct drm_i915_private *dev_priv);
+void intel_guc_init(struct drm_i915_private *dev_priv);
+int intel_guc_init_hw(struct drm_i915_private *dev_priv);
+void intel_guc_fini(struct drm_i915_private *dev_priv);
+const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
+int intel_guc_suspend(struct drm_i915_private *dev_priv);
+int intel_guc_resume(struct drm_i915_private *dev_priv);
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	struct intel_uc_fw *uc_fw);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
-- 
2.9.3

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

[PATCH 03/10] drm/i915/huc: Add huc_to_i915

[Arkadiusz Hiler]

Used to obtain "dev_priv" from huc struct pointer.
We already have similar thing for guc.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index eed9ead..baff495 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2529,6 +2529,11 @@ static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
 	return container_of(guc, struct drm_i915_private, guc);
 }
 
+static inline struct drm_i915_private *huc_to_i915(struct intel_huc *huc)
+{
+	return container_of(huc, struct drm_i915_private, huc);
+}
+
 /* Simple iterator over all initialised engines */
 #define for_each_engine(engine__, dev_priv__, id__) \
 	for ((id__) = 0; \
-- 
2.9.3

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

[PATCH 04/10] drm/i915/uc: Move intel_uc_fw_fetch() to intel_uc.c

[Arkadiusz Hiler]

The file fits better.

Additionally rename it to intel_uc_prepare_fw(), as the function does
more than simple fetch.

Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 137 +-------------------------------
 drivers/gpu/drm/i915/intel_huc.c        |   2 +-
 drivers/gpu/drm/i915/intel_uc.c         | 135 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |   6 +-
 4 files changed, 141 insertions(+), 139 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 9f09e26..20e3337 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -26,7 +26,6 @@
  *    Dave Gordon <david.s.gordon@intel.com>
  *    Alex Dai <yu.dai@intel.com>
  */
-#include <linux/firmware.h>
 #include "i915_drv.h"
 #include "intel_uc.h"
 
@@ -587,140 +586,6 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
-			 struct intel_uc_fw *uc_fw)
-{
-	struct pci_dev *pdev = dev_priv->drm.pdev;
-	struct drm_i915_gem_object *obj;
-	const struct firmware *fw = NULL;
-	struct uc_css_header *css;
-	size_t size;
-	int err;
-
-	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
-		intel_uc_fw_status_repr(uc_fw->fetch_status));
-
-	err = request_firmware(&fw, uc_fw->path, &pdev->dev);
-	if (err)
-		goto fail;
-	if (!fw)
-		goto fail;
-
-	DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw %p\n",
-		uc_fw->path, fw);
-
-	/* Check the size of the blob before examining buffer contents */
-	if (fw->size < sizeof(struct uc_css_header)) {
-		DRM_NOTE("Firmware header is missing\n");
-		goto fail;
-	}
-
-	css = (struct uc_css_header *)fw->data;
-
-	/* Firmware bits always start from header */
-	uc_fw->header_offset = 0;
-	uc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -
-		css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
-
-	if (uc_fw->header_size != sizeof(struct uc_css_header)) {
-		DRM_NOTE("CSS header definition mismatch\n");
-		goto fail;
-	}
-
-	/* then, uCode */
-	uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
-	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
-
-	/* now RSA */
-	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
-		DRM_NOTE("RSA key size is bad\n");
-		goto fail;
-	}
-	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
-	uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
-
-	/* At least, it should have header, uCode and RSA. Size of all three. */
-	size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
-	if (fw->size < size) {
-		DRM_NOTE("Missing firmware components\n");
-		goto fail;
-	}
-
-	/*
-	 * The GuC firmware image has the version number embedded at a well-known
-	 * offset within the firmware blob; note that major / minor version are
-	 * TWO bytes each (i.e. u16), although all pointers and offsets are defined
-	 * in terms of bytes (u8).
-	 */
-	switch (uc_fw->fw) {
-	case INTEL_UC_FW_TYPE_GUC:
-		/* Header and uCode will be loaded to WOPCM. Size of the two. */
-		size = uc_fw->header_size + uc_fw->ucode_size;
-
-		/* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */
-		if (size > intel_guc_wopcm_size(dev_priv)) {
-			DRM_ERROR("Firmware is too large to fit in WOPCM\n");
-			goto fail;
-		}
-		uc_fw->major_ver_found = css->guc.sw_version >> 16;
-		uc_fw->minor_ver_found = css->guc.sw_version & 0xFFFF;
-		break;
-
-	case INTEL_UC_FW_TYPE_HUC:
-		uc_fw->major_ver_found = css->huc.sw_version >> 16;
-		uc_fw->minor_ver_found = css->huc.sw_version & 0xFFFF;
-		break;
-
-	default:
-		DRM_ERROR("Unknown firmware type %d\n", uc_fw->fw);
-		err = -ENOEXEC;
-		goto fail;
-	}
-
-	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
-	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
-		DRM_NOTE("uC firmware version %d.%d, required %d.%d\n",
-			uc_fw->major_ver_found, uc_fw->minor_ver_found,
-			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
-		err = -ENOEXEC;
-		goto fail;
-	}
-
-	DRM_DEBUG_DRIVER("firmware version %d.%d OK (minimum %d.%d)\n",
-			uc_fw->major_ver_found, uc_fw->minor_ver_found,
-			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
-
-	mutex_lock(&dev_priv->drm.struct_mutex);
-	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-	if (IS_ERR_OR_NULL(obj)) {
-		err = obj ? PTR_ERR(obj) : -ENOMEM;
-		goto fail;
-	}
-
-	uc_fw->obj = obj;
-	uc_fw->size = fw->size;
-
-	DRM_DEBUG_DRIVER("uC fw fetch status SUCCESS, obj %p\n",
-			uc_fw->obj);
-
-	release_firmware(fw);
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
-	return;
-
-fail:
-	DRM_WARN("Failed to fetch valid uC firmware from %s (error %d)\n",
-		 uc_fw->path, err);
-	DRM_DEBUG_DRIVER("uC fw fetch status FAIL; err %d, fw %p, obj %p\n",
-		err, fw, uc_fw->obj);
-
-	obj = fetch_and_zero(&uc_fw->obj);
-	if (obj)
-		i915_gem_object_put(obj);
-
-	release_firmware(fw);		/* OK even if fw is NULL */
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
-}
 
 /**
  * intel_guc_init() - define parameters and fetch firmware
@@ -779,7 +644,7 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
 
 	guc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
 	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
-	intel_uc_fw_fetch(dev_priv, guc_fw);
+	intel_uc_prepare_fw(dev_priv, guc_fw);
 	/* status must now be FAIL or SUCCESS */
 }
 
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index babe0eb..566fa7e 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -187,7 +187,7 @@ void intel_huc_init(struct drm_i915_private *dev_priv)
 
 	WARN(huc_fw->path == NULL, "HuC present but no fw path\n");
 
-	intel_uc_fw_fetch(dev_priv, huc_fw);
+	intel_uc_prepare_fw(dev_priv, huc_fw);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index c46bc85..7155aa7 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -24,6 +24,7 @@
 
 #include "i915_drv.h"
 #include "intel_uc.h"
+#include <linux/firmware.h>
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
@@ -114,3 +115,137 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
+void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
+			 struct intel_uc_fw *uc_fw)
+{
+	struct pci_dev *pdev = dev_priv->drm.pdev;
+	struct drm_i915_gem_object *obj;
+	const struct firmware *fw = NULL;
+	struct uc_css_header *css;
+	size_t size;
+	int err;
+
+	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
+		intel_uc_fw_status_repr(uc_fw->fetch_status));
+
+	err = request_firmware(&fw, uc_fw->path, &pdev->dev);
+	if (err)
+		goto fail;
+	if (!fw)
+		goto fail;
+
+	DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw %p\n",
+		uc_fw->path, fw);
+
+	/* Check the size of the blob before examining buffer contents */
+	if (fw->size < sizeof(struct uc_css_header)) {
+		DRM_NOTE("Firmware header is missing\n");
+		goto fail;
+	}
+
+	css = (struct uc_css_header *)fw->data;
+
+	/* Firmware bits always start from header */
+	uc_fw->header_offset = 0;
+	uc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -
+		css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
+
+	if (uc_fw->header_size != sizeof(struct uc_css_header)) {
+		DRM_NOTE("CSS header definition mismatch\n");
+		goto fail;
+	}
+
+	/* then, uCode */
+	uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
+	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
+
+	/* now RSA */
+	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
+		DRM_NOTE("RSA key size is bad\n");
+		goto fail;
+	}
+	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
+	uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
+
+	/* At least, it should have header, uCode and RSA. Size of all three. */
+	size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
+	if (fw->size < size) {
+		DRM_NOTE("Missing firmware components\n");
+		goto fail;
+	}
+
+	/*
+	 * The GuC firmware image has the version number embedded at a
+	 * well-known offset within the firmware blob; note that major / minor
+	 * version are TWO bytes each (i.e. u16), although all pointers and
+	 * offsets are defined in terms of bytes (u8).
+	 */
+	switch (uc_fw->fw) {
+	case INTEL_UC_FW_TYPE_GUC:
+		/* Header and uCode will be loaded to WOPCM. Size of the two. */
+		size = uc_fw->header_size + uc_fw->ucode_size;
+
+		/* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */
+		if (size > intel_guc_wopcm_size(dev_priv)) {
+			DRM_ERROR("Firmware is too large to fit in WOPCM\n");
+			goto fail;
+		}
+		uc_fw->major_ver_found = css->guc.sw_version >> 16;
+		uc_fw->minor_ver_found = css->guc.sw_version & 0xFFFF;
+		break;
+
+	case INTEL_UC_FW_TYPE_HUC:
+		uc_fw->major_ver_found = css->huc.sw_version >> 16;
+		uc_fw->minor_ver_found = css->huc.sw_version & 0xFFFF;
+		break;
+
+	default:
+		DRM_ERROR("Unknown firmware type %d\n", uc_fw->fw);
+		err = -ENOEXEC;
+		goto fail;
+	}
+
+	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
+	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
+		DRM_NOTE("uC firmware version %d.%d, required %d.%d\n",
+			uc_fw->major_ver_found, uc_fw->minor_ver_found,
+			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
+		err = -ENOEXEC;
+		goto fail;
+	}
+
+	DRM_DEBUG_DRIVER("firmware version %d.%d OK (minimum %d.%d)\n",
+			uc_fw->major_ver_found, uc_fw->minor_ver_found,
+			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
+
+	mutex_lock(&dev_priv->drm.struct_mutex);
+	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+	if (IS_ERR_OR_NULL(obj)) {
+		err = obj ? PTR_ERR(obj) : -ENOMEM;
+		goto fail;
+	}
+
+	uc_fw->obj = obj;
+	uc_fw->size = fw->size;
+
+	DRM_DEBUG_DRIVER("uC fw fetch status SUCCESS, obj %p\n",
+			uc_fw->obj);
+
+	release_firmware(fw);
+	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
+	return;
+
+fail:
+	DRM_WARN("Failed to fetch valid uC firmware from %s (error %d)\n",
+		 uc_fw->path, err);
+	DRM_DEBUG_DRIVER("uC fw fetch status FAIL; err %d, fw %p, obj %p\n",
+		err, fw, uc_fw->obj);
+
+	obj = fetch_and_zero(&uc_fw->obj);
+	if (obj)
+		i915_gem_object_put(obj);
+
+	release_firmware(fw);		/* OK even if fw is NULL */
+	uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 41b7351..e411629 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -187,16 +187,18 @@ struct intel_huc {
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
+void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
+			 struct intel_uc_fw *uc_fw);
 
 /* intel_guc_loader.c */
 void intel_guc_init(struct drm_i915_private *dev_priv);
 int intel_guc_init_hw(struct drm_i915_private *dev_priv);
 void intel_guc_fini(struct drm_i915_private *dev_priv);
 const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
+void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
+			 struct intel_uc_fw *uc_fw);
 int intel_guc_suspend(struct drm_i915_private *dev_priv);
 int intel_guc_resume(struct drm_i915_private *dev_priv);
-void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
-	struct intel_uc_fw *uc_fw);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 /* i915_guc_submission.c */
-- 
2.9.3

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

[PATCH 05/10] drm/i915/uc: Introduce intel_uc_init_fw()

[Arkadiusz Hiler]

Instead of calling intel_guc_init() and intel_huc_init() one by one this
patch introduces intel_uc_init_fw() function that calls them both.

Called functions are renamed accordingly.

Trying to have subject_verb_object ordering and more descriptive names,
the intel_huc_init() and intel_guc_init() functions are renamed.

For guc_init():
 * `intel_guc` is the subject, so those functions now take intel_guc
   structure, instead of the dev_priv
 * init is the verb
 * fw is the object which better describes the function's role

huc_init() change follows the same reasoning.

v2: settle on intel_uc_fetch_fw name (M. Wajdeczko)
v3: yet another rename - intel_uc_init_fw (J. Lahtinen)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  3 +--
 drivers/gpu/drm/i915/intel_guc_loader.c | 30 +++++++++++++-------------
 drivers/gpu/drm/i915/intel_huc.c        | 37 ++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_uc.c         |  6 ++++++
 drivers/gpu/drm/i915/intel_uc.h         |  5 +++--
 5 files changed, 43 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b76e8f7..56624bf 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -609,8 +609,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_irq;
 
-	intel_huc_init(dev_priv);
-	intel_guc_init(dev_priv);
+	intel_uc_init_fw(dev_priv);
 
 	ret = i915_gem_init(dev_priv);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 20e3337..87b7a39 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -588,17 +588,17 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 
 
 /**
- * intel_guc_init() - define parameters and fetch firmware
- * @dev_priv:	i915 device private
+ * intel_guc_init_fw() - select and prepare firmware for loading
+ * @guc:	intel_guc struct
  *
  * Called early during driver load, but after GEM is initialised.
  *
  * The firmware will be transferred to the GuC's memory later,
  * when intel_guc_init_hw() is called.
  */
-void intel_guc_init(struct drm_i915_private *dev_priv)
+void intel_guc_init_fw(struct intel_guc *guc)
 {
-	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	const char *fw_path;
 
 	if (!HAS_GUC(dev_priv)) {
@@ -616,23 +616,23 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
 		fw_path = NULL;
 	} else if (IS_SKYLAKE(dev_priv)) {
 		fw_path = I915_SKL_GUC_UCODE;
-		guc_fw->major_ver_wanted = SKL_FW_MAJOR;
-		guc_fw->minor_ver_wanted = SKL_FW_MINOR;
+		guc->fw.major_ver_wanted = SKL_FW_MAJOR;
+		guc->fw.minor_ver_wanted = SKL_FW_MINOR;
 	} else if (IS_BROXTON(dev_priv)) {
 		fw_path = I915_BXT_GUC_UCODE;
-		guc_fw->major_ver_wanted = BXT_FW_MAJOR;
-		guc_fw->minor_ver_wanted = BXT_FW_MINOR;
+		guc->fw.major_ver_wanted = BXT_FW_MAJOR;
+		guc->fw.minor_ver_wanted = BXT_FW_MINOR;
 	} else if (IS_KABYLAKE(dev_priv)) {
 		fw_path = I915_KBL_GUC_UCODE;
-		guc_fw->major_ver_wanted = KBL_FW_MAJOR;
-		guc_fw->minor_ver_wanted = KBL_FW_MINOR;
+		guc->fw.major_ver_wanted = KBL_FW_MAJOR;
+		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
 	} else {
 		fw_path = "";	/* unknown device */
 	}
 
-	guc_fw->path = fw_path;
-	guc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
-	guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
+	guc->fw.path = fw_path;
+	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
+	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
 
 	/* Early (and silent) return if GuC loading is disabled */
 	if (!i915.enable_guc_loading)
@@ -642,9 +642,9 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
 	if (*fw_path == '\0')
 		return;
 
-	guc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
+	guc->fw.fetch_status = INTEL_UC_FIRMWARE_PENDING;
 	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
-	intel_uc_prepare_fw(dev_priv, guc_fw);
+	intel_uc_prepare_fw(dev_priv, &guc->fw);
 	/* status must now be FAIL or SUCCESS */
 }
 
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 566fa7e..6f0811c 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -141,8 +141,8 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_huc_init() - initiate HuC firmware loading request
- * @dev_priv: the drm_i915_private device
+ * intel_huc_init_fw() - select and prepare firmware for loading
+ * @huc:	intel_huc struct
  *
  * Called early during driver load, but after GEM is initialised. The loading
  * will continue only when driver explicitly specify firmware name and version.
@@ -152,42 +152,41 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
  *
  * The DMA-copying to HW is done later when intel_huc_init_hw() is called.
  */
-void intel_huc_init(struct drm_i915_private *dev_priv)
+void intel_huc_init_fw(struct intel_huc *huc)
 {
-	struct intel_huc *huc = &dev_priv->huc;
-	struct intel_uc_fw *huc_fw = &huc->fw;
+	struct drm_i915_private *dev_priv = huc_to_i915(huc);
 	const char *fw_path = NULL;
 
-	huc_fw->path = NULL;
-	huc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
-	huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
-	huc_fw->fw = INTEL_UC_FW_TYPE_HUC;
+	huc->fw.path = NULL;
+	huc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
+	huc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
+	huc->fw.fw = INTEL_UC_FW_TYPE_HUC;
 
 	if (!HAS_HUC_UCODE(dev_priv))
 		return;
 
 	if (IS_SKYLAKE(dev_priv)) {
 		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.major_ver_wanted = SKL_HUC_FW_MAJOR;
+		huc->fw.minor_ver_wanted = SKL_HUC_FW_MINOR;
 	} else if (IS_BROXTON(dev_priv)) {
 		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.major_ver_wanted = BXT_HUC_FW_MAJOR;
+		huc->fw.minor_ver_wanted = BXT_HUC_FW_MINOR;
 	} else if (IS_KABYLAKE(dev_priv)) {
 		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.major_ver_wanted = KBL_HUC_FW_MAJOR;
+		huc->fw.minor_ver_wanted = KBL_HUC_FW_MINOR;
 	}
 
-	huc_fw->path = fw_path;
-	huc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
+	huc->fw.path = fw_path;
+	huc->fw.fetch_status = INTEL_UC_FIRMWARE_PENDING;
 
 	DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path);
 
-	WARN(huc_fw->path == NULL, "HuC present but no fw path\n");
+	WARN(huc->fw.path == NULL, "HuC present but no fw path\n");
 
-	intel_uc_prepare_fw(dev_priv, huc_fw);
+	intel_uc_prepare_fw(dev_priv, &huc->fw);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 7155aa7..5c204e6 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -31,6 +31,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 	mutex_init(&dev_priv->guc.send_mutex);
 }
 
+void intel_uc_init_fw(struct drm_i915_private *dev_priv)
+{
+	intel_huc_init_fw(&dev_priv->huc);
+	intel_guc_init_fw(&dev_priv->guc);
+}
+
 /*
  * Read GuC command/status register (SOFT_SCRATCH_0)
  * Return true if it contains a response rather than a command
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index e411629..221f169 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -185,13 +185,14 @@ struct intel_huc {
 
 /* intel_uc.c */
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
+void intel_uc_init_fw(struct drm_i915_private *dev_priv);
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
 			 struct intel_uc_fw *uc_fw);
 
 /* intel_guc_loader.c */
-void intel_guc_init(struct drm_i915_private *dev_priv);
+void intel_guc_init_fw(struct intel_guc *guc);
 int intel_guc_init_hw(struct drm_i915_private *dev_priv);
 void intel_guc_fini(struct drm_i915_private *dev_priv);
 const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
@@ -225,7 +226,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 }
 
 /* intel_huc.c */
-void intel_huc_init(struct drm_i915_private *dev_priv);
+void intel_huc_init_fw(struct intel_huc *huc);
 void intel_huc_fini(struct drm_i915_private  *dev_priv);
 int intel_huc_init_hw(struct drm_i915_private *dev_priv);
 void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
-- 
2.9.3

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

[PATCH 06/10] drm/i915/guc: Extract param logic form guc_init_fw()

[Arkadiusz Hiler]

Let intel_guc_init_fw() focus on determining and fetching the correct
firmware.

This patch introduces intel_uc_sanitize_options() that is called from
intel_sanitize_options().

Then, if we have GuC, we can call intel_guc_intel_fw() conditionally and
we do not have to do the internal checks.

v2: fix comment, notify when nuking GuC explicitly enabled (M. Wajdeczko)
v3: fix comment again, change the nuke message (M. Wajdeczko)
v4: update title to reflect new function name + rebase

Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  2 ++
 drivers/gpu/drm/i915/intel_guc_loader.c | 14 --------------
 drivers/gpu/drm/i915/intel_uc.c         | 28 +++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_uc.h         |  1 +
 4 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 56624bf..7964e7f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -993,6 +993,8 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
 
 	i915.semaphores = intel_sanitize_semaphores(dev_priv, i915.semaphores);
 	DRM_DEBUG_DRIVER("use GPU semaphores? %s\n", yesno(i915.semaphores));
+
+	intel_uc_sanitize_options(dev_priv);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 87b7a39..5bd1e4a 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -601,17 +601,6 @@ void intel_guc_init_fw(struct intel_guc *guc)
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	const char *fw_path;
 
-	if (!HAS_GUC(dev_priv)) {
-		i915.enable_guc_loading = 0;
-		i915.enable_guc_submission = 0;
-	} else {
-		/* A negative value means "use platform default" */
-		if (i915.enable_guc_loading < 0)
-			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
-		if (i915.enable_guc_submission < 0)
-			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
-	}
-
 	if (!HAS_GUC_UCODE(dev_priv)) {
 		fw_path = NULL;
 	} else if (IS_SKYLAKE(dev_priv)) {
@@ -634,9 +623,6 @@ void intel_guc_init_fw(struct intel_guc *guc)
 	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
 	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
 
-	/* Early (and silent) return if GuC loading is disabled */
-	if (!i915.enable_guc_loading)
-		return;
 	if (fw_path == NULL)
 		return;
 	if (*fw_path == '\0')
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 5c204e6..8f5aa58 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -26,6 +26,27 @@
 #include "intel_uc.h"
 #include <linux/firmware.h>
 
+void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_GUC(dev_priv)) {
+		if (i915.enable_guc_loading > 0)
+			DRM_INFO("Ignoring GuC options, no hardware");
+
+		i915.enable_guc_loading = 0;
+		i915.enable_guc_submission = 0;
+	} else {
+		/* A negative value means "use platform default" */
+		if (i915.enable_guc_loading < 0)
+			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
+		if (i915.enable_guc_submission < 0)
+			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
+
+		/* can't enable guc submission without guc loaded */
+		if (!i915.enable_guc_loading)
+			i915.enable_guc_submission = 0;
+	}
+}
+
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
 	mutex_init(&dev_priv->guc.send_mutex);
@@ -33,7 +54,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 
 void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 {
-	intel_huc_init_fw(&dev_priv->huc);
+	if (!i915.enable_guc_loading)
+		return;
+
+	if (HAS_HUC_UCODE(dev_priv))
+		intel_huc_init_fw(&dev_priv->huc);
+
 	intel_guc_init_fw(&dev_priv->guc);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 221f169..f8d8816 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -184,6 +184,7 @@ struct intel_huc {
 };
 
 /* intel_uc.c */
+void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_uc_init_fw(struct drm_i915_private *dev_priv);
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
-- 
2.9.3

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

[PATCH 07/10] drm/i915/guc: Simplify intel_guc_init_hw()

[Arkadiusz Hiler]

Current version of intel_guc_init_hw() does a lot:
 - cares about submission
 - loads huc
 - implement WA

This change offloads some of the logic to intel_uc_init_hw(), which now
cares about the above.

v2: rename guc_hw_reset and fix typo in define name (M. Wajdeczko)
v3: rename once again
v4: remove spurious comments and add some style (J. Lahtinen)

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |   2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 144 ++++----------------------------
 drivers/gpu/drm/i915/intel_uc.c         | 110 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |   3 +
 4 files changed, 128 insertions(+), 131 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5b36524..8943c4e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4452,7 +4452,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 	intel_mocs_init_l3cc_table(dev_priv);
 
 	/* We can't enable contexts until all firmware is loaded */
-	ret = intel_guc_init_hw(dev_priv);
+	ret = intel_uc_init_hw(dev_priv);
 	if (ret)
 		goto out;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 5bd1e4a..8e9a2e29 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -90,7 +90,7 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 	}
 };
 
-static void guc_interrupts_release(struct drm_i915_private *dev_priv)
+void intel_guc_interrupts_release(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
@@ -108,7 +108,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
 	I915_WRITE(GUC_WD_VECS_IER, 0);
 }
 
-static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
+void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
@@ -408,24 +408,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-static int guc_hw_reset(struct drm_i915_private *dev_priv)
-{
-	int ret;
-	u32 guc_status;
-
-	ret = intel_guc_reset(dev_priv);
-	if (ret) {
-		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
-		return ret;
-	}
-
-	guc_status = I915_READ(GUC_STATUS);
-	WARN(!(guc_status & GS_MIA_IN_RESET),
-	     "GuC status: 0x%x, MIA core expected to be in reset\n", guc_status);
-
-	return ret;
-}
-
 /**
  * intel_guc_init_hw() - finish preparing the GuC for activity
  * @dev_priv:	i915 device private
@@ -443,42 +425,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 	const char *fw_path = guc_fw->path;
-	int retries, ret, err;
+	int ret;
 
 	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
 		fw_path,
 		intel_uc_fw_status_repr(guc_fw->fetch_status),
 		intel_uc_fw_status_repr(guc_fw->load_status));
 
-	/* Loading forbidden, or no firmware to load? */
-	if (!i915.enable_guc_loading) {
-		err = 0;
-		goto fail;
-	} else if (fw_path == NULL) {
-		/* Device is known to have no uCode (e.g. no GuC) */
-		err = -ENXIO;
-		goto fail;
+	if (!fw_path) {
+		return -ENXIO;
 	} else if (*fw_path == '\0') {
-		/* Device has a GuC but we don't know what f/w to load? */
 		WARN(1, "No GuC firmware known for this platform!\n");
-		err = -ENODEV;
-		goto fail;
+		return -ENODEV;
 	}
 
-	/* Fetch failed, or already fetched but failed to load? */
-	if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) {
-		err = -EIO;
-		goto fail;
-	} else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) {
-		err = -ENOEXEC;
-		goto fail;
-	}
-
-	guc_interrupts_release(dev_priv);
-	gen9_reset_guc_interrupts(dev_priv);
-
-	/* We need to notify the guc whenever we change the GGTT */
-	i915_ggtt_enable_guc(dev_priv);
+	if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+		return -EIO;
+	else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL)
+		return -ENOEXEC;
 
 	guc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
 
@@ -486,104 +450,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 		intel_uc_fw_status_repr(guc_fw->fetch_status),
 		intel_uc_fw_status_repr(guc_fw->load_status));
 
-	err = i915_guc_submission_init(dev_priv);
-	if (err)
-		goto fail;
-
 	/*
 	 * WaEnableuKernelHeaderValidFix:skl,bxt
 	 * For BXT, this is only upto B0 but below WA is required for later
 	 * steppings also so this is extended as well.
 	 */
-	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
-	for (retries = 3; ; ) {
-		/*
-		 * Always reset the GuC just before (re)loading, so
-		 * that the state and timing are fairly predictable
-		 */
-		err = guc_hw_reset(dev_priv);
-		if (err)
-			goto fail;
+	ret = guc_ucode_xfer(dev_priv);
 
-		intel_huc_init_hw(dev_priv);
-		err = guc_ucode_xfer(dev_priv);
-		if (!err)
-			break;
-
-		if (--retries == 0)
-			goto fail;
-
-		DRM_INFO("GuC fw load failed: %d; will reset and "
-			 "retry %d more time(s)\n", err, retries);
-	}
+	if (ret)
+		return -EAGAIN;
 
 	guc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS;
 
-	intel_guc_auth_huc(dev_priv);
-
-	if (i915.enable_guc_submission) {
-		if (i915.guc_log_level >= 0)
-			gen9_enable_guc_interrupts(dev_priv);
-
-		err = i915_guc_submission_enable(dev_priv);
-		if (err)
-			goto fail;
-		guc_interrupts_capture(dev_priv);
-	}
-
 	DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
 		 i915.enable_guc_submission ? "submission enabled" : "loaded",
 		 guc_fw->path,
 		 guc_fw->major_ver_found, guc_fw->minor_ver_found);
 
 	return 0;
-
-fail:
-	if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
-		guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
-
-	guc_interrupts_release(dev_priv);
-	i915_guc_submission_disable(dev_priv);
-	i915_guc_submission_fini(dev_priv);
-	i915_ggtt_disable_guc(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).
-	 */
-	if (i915.enable_guc_loading > 1) {
-		ret = -EIO;
-	} else if (i915.enable_guc_submission > 1) {
-		ret = -EIO;
-	} else {
-		ret = 0;
-	}
-
-	if (err == 0 && !HAS_GUC_UCODE(dev_priv))
-		;	/* Don't mention the GuC! */
-	else if (err == 0)
-		DRM_INFO("GuC firmware load skipped\n");
-	else if (ret != -EIO)
-		DRM_NOTE("GuC firmware load failed: %d\n", err);
-	else
-		DRM_WARN("GuC firmware load failed: %d\n", err);
-
-	if (i915.enable_guc_submission) {
-		if (fw_path == NULL)
-			DRM_INFO("GuC submission without firmware not supported\n");
-		if (ret == 0)
-			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
-		else
-			DRM_ERROR("GuC init failed: %d\n", ret);
-	}
-	i915.enable_guc_submission = 0;
-
-	return ret;
 }
 
 
@@ -644,7 +528,7 @@ void intel_guc_fini(struct drm_i915_private *dev_priv)
 	struct drm_i915_gem_object *obj;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	guc_interrupts_release(dev_priv);
+	intel_guc_interrupts_release(dev_priv);
 	i915_guc_submission_disable(dev_priv);
 	i915_guc_submission_fini(dev_priv);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 8f5aa58..709f964 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -26,6 +26,27 @@
 #include "intel_uc.h"
 #include <linux/firmware.h>
 
+/* Reset GuC providing us with fresh state for both GuC and HuC.
+ */
+static int __uc_hw_reset(struct drm_i915_private *dev_priv)
+{
+	int ret;
+	u32 guc_status;
+
+	ret = intel_guc_reset(dev_priv);
+	if (ret) {
+		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
+		return ret;
+	}
+
+	guc_status = I915_READ(GUC_STATUS);
+	WARN(!(guc_status & GS_MIA_IN_RESET),
+	     "GuC status: 0x%x, MIA core expected to be in reset\n",
+	     guc_status);
+
+	return ret;
+}
+
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_GUC(dev_priv)) {
@@ -63,6 +84,92 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 	intel_guc_init_fw(&dev_priv->guc);
 }
 
+int intel_uc_init_hw(struct drm_i915_private *dev_priv)
+{
+	int ret, retries;
+
+	/* guc not enabled, nothing to do */
+	if (!i915.enable_guc_loading)
+		return 0;
+
+	intel_guc_interrupts_release(dev_priv);
+	gen9_reset_guc_interrupts(dev_priv);
+
+	/* We need to notify the guc whenever we change the GGTT */
+	i915_ggtt_enable_guc(dev_priv);
+
+	if (i915.enable_guc_submission) {
+		ret = i915_guc_submission_init(dev_priv);
+		if (ret)
+			goto fail;
+	}
+
+	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
+	retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS;
+	while (retries--) {
+		/*
+		 * Always reset the GuC just before (re)loading, so
+		 * that the state and timing are fairly predictable
+		 */
+		ret = __uc_hw_reset(dev_priv);
+		if (ret)
+			goto fail;
+
+		intel_huc_init_hw(dev_priv);
+		ret = intel_guc_init_hw(dev_priv);
+		if (ret == 0 || ret != -EAGAIN)
+			break;
+
+		DRM_INFO("GuC fw load failed: %d; will reset and "
+			 "retry %d more time(s)\n", ret, retries);
+	}
+
+	/* did we succeded or run out of retries? */
+	if (ret)
+		goto fail;
+
+	intel_guc_auth_huc(dev_priv);
+	if (i915.enable_guc_submission) {
+		if (i915.guc_log_level >= 0)
+			gen9_enable_guc_interrupts(dev_priv);
+
+		ret = i915_guc_submission_enable(dev_priv);
+		if (ret)
+			goto fail;
+		intel_guc_interrupts_capture(dev_priv);
+	}
+
+	return 0;
+
+fail:
+	/*
+	 * 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).
+	 */
+	DRM_ERROR("GuC init failed\n");
+	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
+		ret = -EIO;
+	else
+		ret = 0;
+
+	if (i915.enable_guc_submission) {
+		i915.enable_guc_submission = 0;
+		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
+	}
+
+	i915_ggtt_disable_guc(dev_priv);
+	intel_guc_interrupts_release(dev_priv);
+	i915_guc_submission_disable(dev_priv);
+	i915_guc_submission_fini(dev_priv);
+
+	return ret;
+}
+
 /*
  * Read GuC command/status register (SOFT_SCRATCH_0)
  * Return true if it contains a response rather than a command
@@ -76,6 +183,9 @@ static bool intel_guc_recv(struct intel_guc *guc, u32 *status)
 	return INTEL_GUC_RECV_IS_RESPONSE(val);
 }
 
+/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
+#define GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS 3
+
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index f8d8816..d19c95e 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -187,6 +187,7 @@ struct intel_huc {
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_uc_init_fw(struct drm_i915_private *dev_priv);
+int intel_uc_init_hw(struct drm_i915_private *dev_priv);
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
@@ -201,6 +202,8 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
 			 struct intel_uc_fw *uc_fw);
 int intel_guc_suspend(struct drm_i915_private *dev_priv);
 int intel_guc_resume(struct drm_i915_private *dev_priv);
+void intel_guc_interrupts_release(struct drm_i915_private *dev_priv);
+void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 /* i915_guc_submission.c */
-- 
2.9.3

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

[PATCH 08/10] drm/i915/uc: Simplify firmware path handling

[Arkadiusz Hiler]

Currently fw->path values can represent one of three possible states:

 1) NULL - device without the uC
 2) '\0' - device with the uC but have no firmware
 3) else - device with the uC and we have firmware

Second case is used only to WARN at a later stage.

We can WARN right away and merge cases 1 and 2.

Code can be even further simplified and common (HuC/GuC logic) happening
right before the fetch can be offloaded to the common function.

v2: fewer temporary variables, more straightforward flow (M. Wajdeczko)
v3: DRM_ERROR instead of WARN (M. Wajdeczko)
v4: coding standard (J. Lahtinen)

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 39 +++++++++++----------------------
 drivers/gpu/drm/i915/intel_huc.c        | 20 +++++------------
 drivers/gpu/drm/i915/intel_uc.c         |  4 +++-
 3 files changed, 22 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 8e9a2e29..64f50bd 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -432,12 +432,8 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 		intel_uc_fw_status_repr(guc_fw->fetch_status),
 		intel_uc_fw_status_repr(guc_fw->load_status));
 
-	if (!fw_path) {
+	if (!fw_path)
 		return -ENXIO;
-	} else if (*fw_path == '\0') {
-		WARN(1, "No GuC firmware known for this platform!\n");
-		return -ENODEV;
-	}
 
 	if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return -EIO;
@@ -470,7 +466,6 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
-
 /**
  * intel_guc_init_fw() - select and prepare firmware for loading
  * @guc:	intel_guc struct
@@ -483,39 +478,31 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 void intel_guc_init_fw(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	const char *fw_path;
 
-	if (!HAS_GUC_UCODE(dev_priv)) {
-		fw_path = NULL;
-	} else if (IS_SKYLAKE(dev_priv)) {
-		fw_path = I915_SKL_GUC_UCODE;
+	guc->fw.path = NULL;
+	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
+	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
+	guc->fw.fw = INTEL_UC_FW_TYPE_GUC;
+
+	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;
 	} else if (IS_BROXTON(dev_priv)) {
-		fw_path = I915_BXT_GUC_UCODE;
+		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)) {
-		fw_path = I915_KBL_GUC_UCODE;
+		guc->fw.path = I915_KBL_GUC_UCODE;
 		guc->fw.major_ver_wanted = KBL_FW_MAJOR;
 		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
 	} else {
-		fw_path = "";	/* unknown device */
+		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
+		i915.enable_guc_loading = 0;
+		return;
 	}
 
-	guc->fw.path = fw_path;
-	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
-	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
-
-	if (fw_path == NULL)
-		return;
-	if (*fw_path == '\0')
-		return;
-
-	guc->fw.fetch_status = INTEL_UC_FIRMWARE_PENDING;
-	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
 	intel_uc_prepare_fw(dev_priv, &guc->fw);
-	/* status must now be FAIL or SUCCESS */
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 6f0811c..757f618 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -155,37 +155,29 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
 void intel_huc_init_fw(struct intel_huc *huc)
 {
 	struct drm_i915_private *dev_priv = huc_to_i915(huc);
-	const char *fw_path = NULL;
 
 	huc->fw.path = NULL;
 	huc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
 	huc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
 	huc->fw.fw = INTEL_UC_FW_TYPE_HUC;
 
-	if (!HAS_HUC_UCODE(dev_priv))
-		return;
-
 	if (IS_SKYLAKE(dev_priv)) {
-		fw_path = I915_SKL_HUC_UCODE;
+		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)) {
-		fw_path = I915_BXT_HUC_UCODE;
+		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)) {
-		fw_path = I915_KBL_HUC_UCODE;
+		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 {
+		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
+		return;
 	}
 
-	huc->fw.path = fw_path;
-	huc->fw.fetch_status = INTEL_UC_FIRMWARE_PENDING;
-
-	DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path);
-
-	WARN(huc->fw.path == NULL, "HuC present but no fw path\n");
-
 	intel_uc_prepare_fw(dev_priv, &huc->fw);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 709f964..ac9ad59 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -267,8 +267,10 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
 	size_t size;
 	int err;
 
+	uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
+
 	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
-		intel_uc_fw_status_repr(uc_fw->fetch_status));
+			 intel_uc_fw_status_repr(uc_fw->fetch_status));
 
 	err = request_firmware(&fw, uc_fw->path, &pdev->dev);
 	if (err)
-- 
2.9.3

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

[PATCH 09/10] drm/i915/uc: Separate firmware selection and preparation

[Arkadiusz Hiler]

intel_{h,g}uc_init_fw selects correct firmware and then triggers it's
preparation (fetch + initial parsing).

This change separates out select steps, so those can be called by
the sanitize_options().

Then, during the init_fw() we prepare the firmware if the firmware was
selected.

Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++----------
 drivers/gpu/drm/i915/intel_huc.c        | 14 ++------------
 drivers/gpu/drm/i915/intel_uc.c         | 20 ++++++++++++++------
 drivers/gpu/drm/i915/intel_uc.h         |  4 ++--
 4 files changed, 20 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 64f50bd..8ccd832 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -467,15 +467,10 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_guc_init_fw() - select and prepare firmware for loading
+ * intel_guc_select_fw() - selects GuC firmware for loading
  * @guc:	intel_guc struct
- *
- * Called early during driver load, but after GEM is initialised.
- *
- * The firmware will be transferred to the GuC's memory later,
- * when intel_guc_init_hw() is called.
  */
-void intel_guc_init_fw(struct intel_guc *guc)
+void intel_guc_select_fw(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
@@ -498,11 +493,8 @@ void intel_guc_init_fw(struct intel_guc *guc)
 		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
 	} else {
 		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
-		i915.enable_guc_loading = 0;
 		return;
 	}
-
-	intel_uc_prepare_fw(dev_priv, &guc->fw);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 757f618..d073a68 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -141,18 +141,10 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_huc_init_fw() - select and prepare firmware for loading
+ * intel_huc_select_fw() - selects HuC firmware for loading
  * @huc:	intel_huc struct
- *
- * Called early during driver load, but after GEM is initialised. The loading
- * will continue only when driver explicitly specify firmware name and version.
- * All other cases are considered as INTEL_UC_FIRMWARE_NONE either because HW
- * is not capable or driver yet support it. And there will be no error message
- * for INTEL_UC_FIRMWARE_NONE cases.
- *
- * The DMA-copying to HW is done later when intel_huc_init_hw() is called.
  */
-void intel_huc_init_fw(struct intel_huc *huc)
+void intel_huc_select_fw(struct intel_huc *huc)
 {
 	struct drm_i915_private *dev_priv = huc_to_i915(huc);
 
@@ -177,8 +169,6 @@ void intel_huc_init_fw(struct intel_huc *huc)
 		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
 		return;
 	}
-
-	intel_uc_prepare_fw(dev_priv, &huc->fw);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index ac9ad59..89681b37 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -66,6 +66,16 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 		if (!i915.enable_guc_loading)
 			i915.enable_guc_submission = 0;
 	}
+
+	if (i915.enable_guc_loading) {
+		if (HAS_HUC_UCODE(dev_priv))
+			intel_huc_select_fw(&dev_priv->huc);
+
+		intel_guc_select_fw(&dev_priv->guc);
+
+		if (!dev_priv->guc.fw.path)
+			i915.enable_guc_loading = 0;
+	}
 }
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
@@ -75,13 +85,11 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 
 void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 {
-	if (!i915.enable_guc_loading)
-		return;
+	if (dev_priv->huc.fw.path)
+		intel_uc_prepare_fw(dev_priv, &dev_priv->huc.fw);
 
-	if (HAS_HUC_UCODE(dev_priv))
-		intel_huc_init_fw(&dev_priv->huc);
-
-	intel_guc_init_fw(&dev_priv->guc);
+	if (dev_priv->guc.fw.path)
+		intel_uc_prepare_fw(dev_priv, &dev_priv->guc.fw);
 }
 
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index d19c95e..5f04ea1 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -194,7 +194,7 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
 			 struct intel_uc_fw *uc_fw);
 
 /* intel_guc_loader.c */
-void intel_guc_init_fw(struct intel_guc *guc);
+void intel_guc_select_fw(struct intel_guc *guc);
 int intel_guc_init_hw(struct drm_i915_private *dev_priv);
 void intel_guc_fini(struct drm_i915_private *dev_priv);
 const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
@@ -230,7 +230,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 }
 
 /* intel_huc.c */
-void intel_huc_init_fw(struct intel_huc *huc);
+void intel_huc_select_fw(struct intel_huc *huc);
 void intel_huc_fini(struct drm_i915_private  *dev_priv);
 int intel_huc_init_hw(struct drm_i915_private *dev_priv);
 void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
-- 
2.9.3

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

[PATCH 10/10] drm/i915/uc: Add params for specifying firmware

[Arkadiusz Hiler]

`guc_firmware_path` and `huc_firmware_path` module parameters are added.

Using the parameter disabled version checks and loads desired firmware
instead of the default one.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c      | 10 ++++++++++
 drivers/gpu/drm/i915/i915_params.h      |  2 ++
 drivers/gpu/drm/i915/intel_guc_loader.c |  6 +++++-
 drivers/gpu/drm/i915/intel_huc.c        |  6 +++++-
 drivers/gpu/drm/i915/intel_uc.c         |  5 +++--
 5 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 2e9645e..9c3ff3c 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -59,6 +59,8 @@ struct i915_params i915 __read_mostly = {
 	.enable_guc_loading = 0,
 	.enable_guc_submission = 0,
 	.guc_log_level = -1,
+	.guc_firmware_path = NULL,
+	.huc_firmware_path = NULL,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
 	.enable_dpcd_backlight = false,
@@ -230,6 +232,14 @@ module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
 MODULE_PARM_DESC(guc_log_level,
 	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
 
+module_param_named(guc_firmware_path, i915.guc_firmware_path, charp, 0400);
+MODULE_PARM_DESC(guc_firmware_path,
+	"GuC firmware path to use instead of the default one");
+
+module_param_named(huc_firmware_path, i915.huc_firmware_path, charp, 0400);
+MODULE_PARM_DESC(huc_firmware_path,
+	"HuC firmware path to use instead of the default one");
+
 module_param_named_unsafe(enable_dp_mst, i915.enable_dp_mst, bool, 0600);
 MODULE_PARM_DESC(enable_dp_mst,
 	"Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 55d47ee..34148cc 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -46,6 +46,8 @@
 	func(int, enable_guc_loading); \
 	func(int, enable_guc_submission); \
 	func(int, guc_log_level); \
+	func(char *, guc_firmware_path); \
+	func(char *, huc_firmware_path); \
 	func(int, use_mmio_flip); \
 	func(int, mmio_debug); \
 	func(int, edp_vswing); \
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 8ccd832..8a03836 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -479,7 +479,11 @@ void intel_guc_select_fw(struct intel_guc *guc)
 	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
 	guc->fw.fw = INTEL_UC_FW_TYPE_GUC;
 
-	if (IS_SKYLAKE(dev_priv)) {
+	if (i915.guc_firmware_path) {
+		guc->fw.path = i915.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;
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index d073a68..b2067bf 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -153,7 +153,11 @@ void intel_huc_select_fw(struct intel_huc *huc)
 	huc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
 	huc->fw.fw = INTEL_UC_FW_TYPE_HUC;
 
-	if (IS_SKYLAKE(dev_priv)) {
+	if (i915.huc_firmware_path) {
+		huc->fw.path = i915.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;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 89681b37..bbfb5b6 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -357,8 +357,9 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
 		goto fail;
 	}
 
-	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
-	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
+	if (!(uc_fw->major_ver_found == 0 && uc_fw->minor_ver_found == 0) &&
+	    (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
+	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted)) {
 		DRM_NOTE("uC firmware version %d.%d, required %d.%d\n",
 			uc_fw->major_ver_found, uc_fw->minor_ver_found,
 			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
-- 
2.9.3

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

✗ Fi.CI.BAT: failure for GuC Scrub vol. 1 (rev5)

[Patchwork]

== Series Details ==

Series: GuC Scrub vol. 1 (rev5)
URL   : https://patchwork.freedesktop.org/series/16856/
State : failure

== Summary ==

  CC [M]  drivers/gpu/drm/i915/gvt/gtt.o
  LD      drivers/thermal/thermal_sys.o
  CC [M]  drivers/gpu/drm/i915/gvt/cfg_space.o
  LD      kernel/sched/built-in.o
  CC [M]  drivers/gpu/drm/i915/gvt/opregion.o
  CC [M]  drivers/gpu/drm/i915/gvt/mmio.o
  CC [M]  drivers/gpu/drm/i915/gvt/display.o
  CC [M]  drivers/gpu/drm/i915/gvt/edid.o
  CC [M]  drivers/gpu/drm/i915/gvt/execlist.o
  CC [M]  drivers/gpu/drm/i915/gvt/scheduler.o
  CC [M]  drivers/gpu/drm/i915/gvt/sched_policy.o
  LD      drivers/thermal/built-in.o
  CC [M]  drivers/gpu/drm/i915/gvt/render.o
  CC [M]  drivers/gpu/drm/i915/gvt/cmd_parser.o
  CC [M]  drivers/gpu/drm/i915/intel_lpe_audio.o
  LD      kernel/built-in.o
  LD      lib/raid6/raid6_pq.o
  LD      drivers/tty/serial/8250/8250.o
  LD      lib/raid6/built-in.o
  LD      drivers/pci/pcie/pcieportdrv.o
  LD      drivers/usb/storage/usb-storage.o
  LD      drivers/usb/storage/built-in.o
  LD [M]  drivers/gpu/drm/vgem/vgem.o
  LD      drivers/video/fbdev/core/fb.o
  LD      net/ipv6/ipv6.o
  LD      drivers/video/fbdev/core/built-in.o
  LD      drivers/tty/serial/8250/8250_base.o
  LD      drivers/tty/serial/8250/built-in.o
  LD [M]  drivers/usb/serial/usbserial.o
  LD [M]  drivers/misc/mei/mei-me.o
  LD      drivers/misc/built-in.o
  LD      drivers/tty/serial/built-in.o
  LD      net/ipv6/built-in.o
  LD      drivers/pci/pcie/aer/aerdriver.o
  LD      drivers/pci/pcie/aer/built-in.o
  LD      drivers/pci/pcie/built-in.o
drivers/gpu/drm/i915/intel_uc.c: In function ‘intel_uc_init_hw’:
drivers/gpu/drm/i915/intel_uc.c:116:12: error: ‘GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS’ undeclared (first use in this function)
  retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS;
            ^
drivers/gpu/drm/i915/intel_uc.c:116:12: note: each undeclared identifier is reported only once for each function it appears in
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/intel_uc.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_uc.o] Error 1
make[4]: *** Waiting for unfinished jobs....
  LD      drivers/iommu/built-in.o
  LD      drivers/usb/gadget/libcomposite.o
  LD      drivers/spi/built-in.o
  LD      drivers/usb/gadget/udc/udc-core.o
  LD      drivers/usb/gadget/udc/built-in.o
  LD      drivers/usb/gadget/built-in.o
  LD [M]  drivers/mmc/core/mmc_block.o
  LD      drivers/mmc/built-in.o
  AR      lib/lib.a
  EXPORTS lib/lib-ksyms.o
  LD      drivers/scsi/scsi_mod.o
  LD      lib/built-in.o
  LD      fs/btrfs/btrfs.o
  LD      drivers/pci/built-in.o
  LD      fs/btrfs/built-in.o
  LD      drivers/video/fbdev/built-in.o
  LD [M]  drivers/net/ethernet/intel/igbvf/igbvf.o
  LD      drivers/video/console/built-in.o
  LD      drivers/scsi/sd_mod.o
  LD      drivers/video/built-in.o
  LD      drivers/scsi/built-in.o
  LD      drivers/tty/vt/built-in.o
  LD      net/ipv4/built-in.o
  LD      drivers/tty/built-in.o
  LD [M]  drivers/net/ethernet/broadcom/genet/genet.o
  LD      drivers/gpu/drm/drm.o
  LD [M]  drivers/net/ethernet/intel/e1000/e1000.o
  CC      arch/x86/kernel/cpu/capflags.o
  LD      net/core/built-in.o
  LD      arch/x86/kernel/cpu/built-in.o
  LD      arch/x86/kernel/built-in.o
  LD      net/built-in.o
  LD      arch/x86/built-in.o
  LD      drivers/usb/host/xhci-hcd.o
  LD      drivers/usb/core/usbcore.o
  LD      drivers/usb/core/built-in.o
  LD      fs/ext4/ext4.o
  LD      fs/ext4/built-in.o
  LD      fs/built-in.o
  LD [M]  drivers/net/ethernet/intel/igb/igb.o
  LD      drivers/md/md-mod.o
  LD      drivers/md/built-in.o
  LD      drivers/usb/host/built-in.o
  LD      drivers/usb/built-in.o
  LD [M]  drivers/net/ethernet/intel/e1000e/e1000e.o
scripts/Makefile.build:553: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:553: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:553: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
make[1]: *** Waiting for unfinished jobs....
  LD      drivers/net/ethernet/built-in.o
  LD      drivers/net/built-in.o
Makefile:988: recipe for target 'drivers' failed
make: *** [drivers] Error 2

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

Re: [PATCH 04/10] drm/i915/uc: Move intel_uc_fw_fetch() to intel_uc.c

[Michal Wajdeczko]

On Fri, Feb 24, 2017 at 04:39:58PM +0100, Arkadiusz Hiler wrote:
> The file fits better.
> 
> Additionally rename it to intel_uc_prepare_fw(), as the function does
> more than simple fetch.

Hmm, new function does not contain "fetch" verb and "prepare" alone is
not much meaningful 

> 
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---

<snip>

> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 41b7351..e411629 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -187,16 +187,18 @@ struct intel_huc {
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
>  int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
>  int intel_guc_sample_forcewake(struct intel_guc *guc);
> +void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
> +			 struct intel_uc_fw *uc_fw);

Can you keep "intel_uc_" function declarations together?


>  
>  /* intel_guc_loader.c */
>  void intel_guc_init(struct drm_i915_private *dev_priv);
>  int intel_guc_init_hw(struct drm_i915_private *dev_priv);
>  void intel_guc_fini(struct drm_i915_private *dev_priv);
>  const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
> +void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
> +			 struct intel_uc_fw *uc_fw);

This one is redundant ;)


-Michal 


>  int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  int intel_guc_resume(struct drm_i915_private *dev_priv);
> -void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> -	struct intel_uc_fw *uc_fw);
>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>  
>  /* i915_guc_submission.c */
> -- 
> 2.9.3
> 
> _______________________________________________
> 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 05/10] drm/i915/uc: Introduce intel_uc_init_fw()

[Michal Wajdeczko]

On Fri, Feb 24, 2017 at 04:39:59PM +0100, Arkadiusz Hiler wrote:
> Instead of calling intel_guc_init() and intel_huc_init() one by one this
> patch introduces intel_uc_init_fw() function that calls them both.
> 
> Called functions are renamed accordingly.
> 
> Trying to have subject_verb_object ordering and more descriptive names,
> the intel_huc_init() and intel_guc_init() functions are renamed.
> 
> For guc_init():
>  * `intel_guc` is the subject, so those functions now take intel_guc
>    structure, instead of the dev_priv
>  * init is the verb
>  * fw is the object which better describes the function's role
> 
> huc_init() change follows the same reasoning.
> 
> v2: settle on intel_uc_fetch_fw name (M. Wajdeczko)
> v3: yet another rename - intel_uc_init_fw (J. Lahtinen)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

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

Re: [PATCH 06/10] drm/i915/guc: Extract param logic form guc_init_fw()

[Michal Wajdeczko]

On Fri, Feb 24, 2017 at 04:40:00PM +0100, Arkadiusz Hiler wrote:
> Let intel_guc_init_fw() focus on determining and fetching the correct
> firmware.
> 
> This patch introduces intel_uc_sanitize_options() that is called from
> intel_sanitize_options().
> 
> Then, if we have GuC, we can call intel_guc_intel_fw() conditionally and

Typo.


> we do not have to do the internal checks.
> 
> v2: fix comment, notify when nuking GuC explicitly enabled (M. Wajdeczko)
> v3: fix comment again, change the nuke message (M. Wajdeczko)
> v4: update title to reflect new function name + rebase
> 
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         |  2 ++
>  drivers/gpu/drm/i915/intel_guc_loader.c | 14 --------------
>  drivers/gpu/drm/i915/intel_uc.c         | 28 +++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_uc.h         |  1 +
>  4 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 56624bf..7964e7f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -993,6 +993,8 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>  
>  	i915.semaphores = intel_sanitize_semaphores(dev_priv, i915.semaphores);
>  	DRM_DEBUG_DRIVER("use GPU semaphores? %s\n", yesno(i915.semaphores));
> +
> +	intel_uc_sanitize_options(dev_priv);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 87b7a39..5bd1e4a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -601,17 +601,6 @@ void intel_guc_init_fw(struct intel_guc *guc)
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>  	const char *fw_path;
>  
> -	if (!HAS_GUC(dev_priv)) {
> -		i915.enable_guc_loading = 0;
> -		i915.enable_guc_submission = 0;
> -	} else {
> -		/* A negative value means "use platform default" */
> -		if (i915.enable_guc_loading < 0)
> -			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> -		if (i915.enable_guc_submission < 0)
> -			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> -	}
> -
>  	if (!HAS_GUC_UCODE(dev_priv)) {
>  		fw_path = NULL;

Hmm, once sanitized we should not reach this point.


>  	} else if (IS_SKYLAKE(dev_priv)) {
> @@ -634,9 +623,6 @@ void intel_guc_init_fw(struct intel_guc *guc)
>  	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
>  	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
>  
> -	/* Early (and silent) return if GuC loading is disabled */
> -	if (!i915.enable_guc_loading)
> -		return;
>  	if (fw_path == NULL)
>  		return;
>  	if (*fw_path == '\0')
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 5c204e6..8f5aa58 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -26,6 +26,27 @@
>  #include "intel_uc.h"
>  #include <linux/firmware.h>
>  
> +void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
> +{
> +	if (!HAS_GUC(dev_priv)) {
> +		if (i915.enable_guc_loading > 0)
> +			DRM_INFO("Ignoring GuC options, no hardware");
> +
> +		i915.enable_guc_loading = 0;
> +		i915.enable_guc_submission = 0;
> +	} else {
> +		/* A negative value means "use platform default" */
> +		if (i915.enable_guc_loading < 0)
> +			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> +		if (i915.enable_guc_submission < 0)
> +			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> +
> +		/* can't enable guc submission without guc loaded */

s/can't/Can't

> +		if (!i915.enable_guc_loading)
> +			i915.enable_guc_submission = 0;
> +	}
> +}
> +
>  void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  {
>  	mutex_init(&dev_priv->guc.send_mutex);
> @@ -33,7 +54,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  
>  void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>  {
> -	intel_huc_init_fw(&dev_priv->huc);
> +	if (!i915.enable_guc_loading)
> +		return;
> +
> +	if (HAS_HUC_UCODE(dev_priv))
> +		intel_huc_init_fw(&dev_priv->huc);

Did you removed corresponding "if(HAS_HUC_UCODE)" from huc_init_fw function ?
In patch 05/10 it was still there.

-Michal

> +
>  	intel_guc_init_fw(&dev_priv->guc);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 221f169..f8d8816 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -184,6 +184,7 @@ struct intel_huc {
>  };
>  
>  /* intel_uc.c */
> +void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
>  void intel_uc_init_fw(struct drm_i915_private *dev_priv);
>  int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
> -- 
> 2.9.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/10] drm/i915/guc: Simplify intel_guc_init_hw()

[Michal Wajdeczko]

On Fri, Feb 24, 2017 at 04:40:01PM +0100, Arkadiusz Hiler wrote:
> Current version of intel_guc_init_hw() does a lot:
>  - cares about submission
>  - loads huc
>  - implement WA
> 
> This change offloads some of the logic to intel_uc_init_hw(), which now
> cares about the above.
> 
> v2: rename guc_hw_reset and fix typo in define name (M. Wajdeczko)
> v3: rename once again
> v4: remove spurious comments and add some style (J. Lahtinen)
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         |   2 +-
>  drivers/gpu/drm/i915/intel_guc_loader.c | 144 ++++----------------------------
>  drivers/gpu/drm/i915/intel_uc.c         | 110 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h         |   3 +
>  4 files changed, 128 insertions(+), 131 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5b36524..8943c4e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4452,7 +4452,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
>  	intel_mocs_init_l3cc_table(dev_priv);
>  
>  	/* We can't enable contexts until all firmware is loaded */
> -	ret = intel_guc_init_hw(dev_priv);
> +	ret = intel_uc_init_hw(dev_priv);
>  	if (ret)
>  		goto out;
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 5bd1e4a..8e9a2e29 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -90,7 +90,7 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
>  	}
>  };
>  
> -static void guc_interrupts_release(struct drm_i915_private *dev_priv)
> +void intel_guc_interrupts_release(struct drm_i915_private *dev_priv)

Hmm, maybe better name for this function would be "intel_release_guc_interrupts" ?
Then it will match your subject-verb-object pattern.


>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> @@ -108,7 +108,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GUC_WD_VECS_IER, 0);
>  }
>  
> -static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
> +void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> @@ -408,24 +408,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -static int guc_hw_reset(struct drm_i915_private *dev_priv)
> -{
> -	int ret;
> -	u32 guc_status;
> -
> -	ret = intel_guc_reset(dev_priv);
> -	if (ret) {
> -		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
> -		return ret;
> -	}
> -
> -	guc_status = I915_READ(GUC_STATUS);
> -	WARN(!(guc_status & GS_MIA_IN_RESET),
> -	     "GuC status: 0x%x, MIA core expected to be in reset\n", guc_status);
> -
> -	return ret;
> -}
> -
>  /**
>   * intel_guc_init_hw() - finish preparing the GuC for activity
>   * @dev_priv:	i915 device private
> @@ -443,42 +425,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>  	const char *fw_path = guc_fw->path;
> -	int retries, ret, err;
> +	int ret;
>  
>  	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
>  		fw_path,
>  		intel_uc_fw_status_repr(guc_fw->fetch_status),
>  		intel_uc_fw_status_repr(guc_fw->load_status));
>  
> -	/* Loading forbidden, or no firmware to load? */
> -	if (!i915.enable_guc_loading) {
> -		err = 0;
> -		goto fail;
> -	} else if (fw_path == NULL) {
> -		/* Device is known to have no uCode (e.g. no GuC) */
> -		err = -ENXIO;
> -		goto fail;
> +	if (!fw_path) {
> +		return -ENXIO;
>  	} else if (*fw_path == '\0') {

Hmm, is this case still possible?


> -		/* Device has a GuC but we don't know what f/w to load? */
>  		WARN(1, "No GuC firmware known for this platform!\n");
> -		err = -ENODEV;
> -		goto fail;
> +		return -ENODEV;
>  	}
>  
> -	/* Fetch failed, or already fetched but failed to load? */
> -	if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) {
> -		err = -EIO;
> -		goto fail;
> -	} else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) {
> -		err = -ENOEXEC;
> -		goto fail;
> -	}
> -
> -	guc_interrupts_release(dev_priv);
> -	gen9_reset_guc_interrupts(dev_priv);
> -
> -	/* We need to notify the guc whenever we change the GGTT */
> -	i915_ggtt_enable_guc(dev_priv);
> +	if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> +		return -EIO;
> +	else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL)
> +		return -ENOEXEC;

Hmm, it looks like you're checking for load failure here, but actual
load is about to start below ? Did I missed something ?


>  
>  	guc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;

I guess this status can be set in guc_ucode_xfer() as it uses guc_fw object.


>  
> @@ -486,104 +450,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
>  		intel_uc_fw_status_repr(guc_fw->fetch_status),
>  		intel_uc_fw_status_repr(guc_fw->load_status));
>  
> -	err = i915_guc_submission_init(dev_priv);
> -	if (err)
> -		goto fail;
> -
>  	/*
>  	 * WaEnableuKernelHeaderValidFix:skl,bxt
>  	 * For BXT, this is only upto B0 but below WA is required for later
>  	 * steppings also so this is extended as well.
>  	 */

Rebase issue?

> -	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> -	for (retries = 3; ; ) {
> -		/*
> -		 * Always reset the GuC just before (re)loading, so
> -		 * that the state and timing are fairly predictable
> -		 */
> -		err = guc_hw_reset(dev_priv);
> -		if (err)
> -			goto fail;
> +	ret = guc_ucode_xfer(dev_priv);
>  
> -		intel_huc_init_hw(dev_priv);
> -		err = guc_ucode_xfer(dev_priv);
> -		if (!err)
> -			break;
> -
> -		if (--retries == 0)
> -			goto fail;
> -
> -		DRM_INFO("GuC fw load failed: %d; will reset and "
> -			 "retry %d more time(s)\n", err, retries);
> -	}
> +	if (ret)
> +		return -EAGAIN;
>  
>  	guc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS;
>  
> -	intel_guc_auth_huc(dev_priv);
> -
> -	if (i915.enable_guc_submission) {
> -		if (i915.guc_log_level >= 0)
> -			gen9_enable_guc_interrupts(dev_priv);
> -
> -		err = i915_guc_submission_enable(dev_priv);
> -		if (err)
> -			goto fail;
> -		guc_interrupts_capture(dev_priv);
> -	}
> -
>  	DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
>  		 i915.enable_guc_submission ? "submission enabled" : "loaded",
>  		 guc_fw->path,
>  		 guc_fw->major_ver_found, guc_fw->minor_ver_found);
>  
>  	return 0;
> -
> -fail:
> -	if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
> -		guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
> -
> -	guc_interrupts_release(dev_priv);
> -	i915_guc_submission_disable(dev_priv);
> -	i915_guc_submission_fini(dev_priv);
> -	i915_ggtt_disable_guc(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).
> -	 */
> -	if (i915.enable_guc_loading > 1) {
> -		ret = -EIO;
> -	} else if (i915.enable_guc_submission > 1) {
> -		ret = -EIO;
> -	} else {
> -		ret = 0;
> -	}
> -
> -	if (err == 0 && !HAS_GUC_UCODE(dev_priv))
> -		;	/* Don't mention the GuC! */
> -	else if (err == 0)
> -		DRM_INFO("GuC firmware load skipped\n");
> -	else if (ret != -EIO)
> -		DRM_NOTE("GuC firmware load failed: %d\n", err);
> -	else
> -		DRM_WARN("GuC firmware load failed: %d\n", err);
> -
> -	if (i915.enable_guc_submission) {
> -		if (fw_path == NULL)
> -			DRM_INFO("GuC submission without firmware not supported\n");
> -		if (ret == 0)
> -			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> -		else
> -			DRM_ERROR("GuC init failed: %d\n", ret);
> -	}
> -	i915.enable_guc_submission = 0;
> -
> -	return ret;
>  }
>  
>  
> @@ -644,7 +528,7 @@ void intel_guc_fini(struct drm_i915_private *dev_priv)
>  	struct drm_i915_gem_object *obj;
>  
>  	mutex_lock(&dev_priv->drm.struct_mutex);
> -	guc_interrupts_release(dev_priv);
> +	intel_guc_interrupts_release(dev_priv);
>  	i915_guc_submission_disable(dev_priv);
>  	i915_guc_submission_fini(dev_priv);
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 8f5aa58..709f964 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -26,6 +26,27 @@
>  #include "intel_uc.h"
>  #include <linux/firmware.h>
>  
> +/* Reset GuC providing us with fresh state for both GuC and HuC.
> + */
> +static int __uc_hw_reset(struct drm_i915_private *dev_priv)

Hmm, maybe "intel_uc_reset_hw" to match other function names?


> +{
> +	int ret;
> +	u32 guc_status;
> +
> +	ret = intel_guc_reset(dev_priv);
> +	if (ret) {
> +		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	guc_status = I915_READ(GUC_STATUS);
> +	WARN(!(guc_status & GS_MIA_IN_RESET),
> +	     "GuC status: 0x%x, MIA core expected to be in reset\n",
> +	     guc_status);
> +
> +	return ret;
> +}
> +
>  void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>  {
>  	if (!HAS_GUC(dev_priv)) {
> @@ -63,6 +84,92 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>  	intel_guc_init_fw(&dev_priv->guc);
>  }
>  
> +int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> +{
> +	int ret, retries;
> +
> +	/* guc not enabled, nothing to do */
> +	if (!i915.enable_guc_loading)
> +		return 0;
> +
> +	intel_guc_interrupts_release(dev_priv);
> +	gen9_reset_guc_interrupts(dev_priv);
> +
> +	/* We need to notify the guc whenever we change the GGTT */
> +	i915_ggtt_enable_guc(dev_priv);
> +
> +	if (i915.enable_guc_submission) {
> +		ret = i915_guc_submission_init(dev_priv);
> +		if (ret)
> +			goto fail;
> +	}
> +
> +	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +	retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS;
> +	while (retries--) {
> +		/*
> +		 * Always reset the GuC just before (re)loading, so
> +		 * that the state and timing are fairly predictable
> +		 */
> +		ret = __uc_hw_reset(dev_priv);
> +		if (ret)
> +			goto fail;
> +
> +		intel_huc_init_hw(dev_priv);
> +		ret = intel_guc_init_hw(dev_priv);
> +		if (ret == 0 || ret != -EAGAIN)
> +			break;
> +
> +		DRM_INFO("GuC fw load failed: %d; will reset and "
> +			 "retry %d more time(s)\n", ret, retries);
> +	}
> +
> +	/* did we succeded or run out of retries? */
> +	if (ret)
> +		goto fail;
> +
> +	intel_guc_auth_huc(dev_priv);
> +	if (i915.enable_guc_submission) {
> +		if (i915.guc_log_level >= 0)
> +			gen9_enable_guc_interrupts(dev_priv);
> +
> +		ret = i915_guc_submission_enable(dev_priv);
> +		if (ret)
> +			goto fail;
> +		intel_guc_interrupts_capture(dev_priv);
> +	}
> +
> +	return 0;
> +
> +fail:
> +	/*
> +	 * 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).
> +	 */
> +	DRM_ERROR("GuC init failed\n");
> +	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)

Nonzero i915.enable_guc_loading is guaranteed (see your first "if" in this function)


> +		ret = -EIO;
> +	else
> +		ret = 0;
> +
> +	if (i915.enable_guc_submission) {
> +		i915.enable_guc_submission = 0;
> +		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +	}
> +
> +	i915_ggtt_disable_guc(dev_priv);
> +	intel_guc_interrupts_release(dev_priv);
> +	i915_guc_submission_disable(dev_priv);
> +	i915_guc_submission_fini(dev_priv);
> +
> +	return ret;
> +}
> +
>  /*
>   * Read GuC command/status register (SOFT_SCRATCH_0)
>   * Return true if it contains a response rather than a command
> @@ -76,6 +183,9 @@ static bool intel_guc_recv(struct intel_guc *guc, u32 *status)
>  	return INTEL_GUC_RECV_IS_RESPONSE(val);
>  }
>  
> +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS 3
> +

Hmm, is it worth to define this macro here as it is used only once and
it is very specific ? Do you plan to define more similar macros?


-Michal



>  int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index f8d8816..d19c95e 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -187,6 +187,7 @@ struct intel_huc {
>  void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
>  void intel_uc_init_fw(struct drm_i915_private *dev_priv);
> +int intel_uc_init_hw(struct drm_i915_private *dev_priv);
>  int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
>  int intel_guc_sample_forcewake(struct intel_guc *guc);
>  void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
> @@ -201,6 +202,8 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
>  			 struct intel_uc_fw *uc_fw);
>  int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  int intel_guc_resume(struct drm_i915_private *dev_priv);
> +void intel_guc_interrupts_release(struct drm_i915_private *dev_priv);
> +void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv);
>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>  
>  /* i915_guc_submission.c */
> -- 
> 2.9.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/10] drm/i915/uc: Simplify firmware path handling

[Michal Wajdeczko]

On Fri, Feb 24, 2017 at 04:40:02PM +0100, Arkadiusz Hiler wrote:
> Currently fw->path values can represent one of three possible states:
> 
>  1) NULL - device without the uC
>  2) '\0' - device with the uC but have no firmware
>  3) else - device with the uC and we have firmware
> 
> Second case is used only to WARN at a later stage.
> 
> We can WARN right away and merge cases 1 and 2.
> 
> Code can be even further simplified and common (HuC/GuC logic) happening
> right before the fetch can be offloaded to the common function.
> 
> v2: fewer temporary variables, more straightforward flow (M. Wajdeczko)
> v3: DRM_ERROR instead of WARN (M. Wajdeczko)
> v4: coding standard (J. Lahtinen)
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_loader.c | 39 +++++++++++----------------------
>  drivers/gpu/drm/i915/intel_huc.c        | 20 +++++------------
>  drivers/gpu/drm/i915/intel_uc.c         |  4 +++-
>  3 files changed, 22 insertions(+), 41 deletions(-)
> 

<snip>

> @@ -483,39 +478,31 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
>  void intel_guc_init_fw(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	const char *fw_path;
>  
> -	if (!HAS_GUC_UCODE(dev_priv)) {
> -		fw_path = NULL;
> -	} else if (IS_SKYLAKE(dev_priv)) {
> -		fw_path = I915_SKL_GUC_UCODE;
> +	guc->fw.path = NULL;
> +	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
> +	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
> +	guc->fw.fw = INTEL_UC_FW_TYPE_GUC;
> +
> +	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;
>  	} else if (IS_BROXTON(dev_priv)) {
> -		fw_path = I915_BXT_GUC_UCODE;
> +		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)) {
> -		fw_path = I915_KBL_GUC_UCODE;
> +		guc->fw.path = I915_KBL_GUC_UCODE;
>  		guc->fw.major_ver_wanted = KBL_FW_MAJOR;
>  		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
>  	} else {
> -		fw_path = "";	/* unknown device */
> +		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
> +		i915.enable_guc_loading = 0;

Hmm, I don't like that we're modifying param outside of intel_uc_sanitize_options()
If we split this function into fw filename selection and actual prepare/fetch then
we could do this fw initial sanitization in proper place as:

	i915.enable_guc_loading = intel_sanitize_guc_fw(dev_priv);


> +		return;
>  	}
>  
> -	guc->fw.path = fw_path;
> -	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
> -	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
> -
> -	if (fw_path == NULL)
> -		return;
> -	if (*fw_path == '\0')
> -		return;
> -
> -	guc->fw.fetch_status = INTEL_UC_FIRMWARE_PENDING;
> -	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
>  	intel_uc_prepare_fw(dev_priv, &guc->fw);
> -	/* status must now be FAIL or SUCCESS */
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 6f0811c..757f618 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -155,37 +155,29 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
>  void intel_huc_init_fw(struct intel_huc *huc)
>  {
>  	struct drm_i915_private *dev_priv = huc_to_i915(huc);
> -	const char *fw_path = NULL;
>  
>  	huc->fw.path = NULL;
>  	huc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
>  	huc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
>  	huc->fw.fw = INTEL_UC_FW_TYPE_HUC;
>  
> -	if (!HAS_HUC_UCODE(dev_priv))
> -		return;
> -
>  	if (IS_SKYLAKE(dev_priv)) {
> -		fw_path = I915_SKL_HUC_UCODE;
> +		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)) {
> -		fw_path = I915_BXT_HUC_UCODE;
> +		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)) {
> -		fw_path = I915_KBL_HUC_UCODE;
> +		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 {
> +		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
> +		return;
>  	}
>  
> -	huc->fw.path = fw_path;
> -	huc->fw.fetch_status = INTEL_UC_FIRMWARE_PENDING;
> -
> -	DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path);
> -
> -	WARN(huc->fw.path == NULL, "HuC present but no fw path\n");
> -
>  	intel_uc_prepare_fw(dev_priv, &huc->fw);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 709f964..ac9ad59 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -267,8 +267,10 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
>  	size_t size;
>  	int err;
>  
> +	uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
> +
>  	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
> -		intel_uc_fw_status_repr(uc_fw->fetch_status));
> +			 intel_uc_fw_status_repr(uc_fw->fetch_status));
>  
>  	err = request_firmware(&fw, uc_fw->path, &pdev->dev);
>  	if (err)


I think previous patches will look simpler if you start with this patch earlier.
This move will also address some previously posted comments.

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

Re: [PATCH 09/10] drm/i915/uc: Separate firmware selection and preparation

[Michal Wajdeczko]

On Fri, Feb 24, 2017 at 04:40:03PM +0100, Arkadiusz Hiler wrote:
> intel_{h,g}uc_init_fw selects correct firmware and then triggers it's
> preparation (fetch + initial parsing).
> 
> This change separates out select steps, so those can be called by
> the sanitize_options().
> 
> Then, during the init_fw() we prepare the firmware if the firmware was
> selected.
> 
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---

It looks strange that in one series you're changing function names
or their logic few times ... patch ordering really matters ;)

Please consider reorder/squash.


>  drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++----------
>  drivers/gpu/drm/i915/intel_huc.c        | 14 ++------------
>  drivers/gpu/drm/i915/intel_uc.c         | 20 ++++++++++++++------
>  drivers/gpu/drm/i915/intel_uc.h         |  4 ++--
>  4 files changed, 20 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 64f50bd..8ccd832 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -467,15 +467,10 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
>  }
>  
>  /**
> - * intel_guc_init_fw() - select and prepare firmware for loading
> + * intel_guc_select_fw() - selects GuC firmware for loading
>   * @guc:	intel_guc struct
> - *
> - * Called early during driver load, but after GEM is initialised.
> - *
> - * The firmware will be transferred to the GuC's memory later,
> - * when intel_guc_init_hw() is called.
>   */
> -void intel_guc_init_fw(struct intel_guc *guc)
> +void intel_guc_select_fw(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>  
> @@ -498,11 +493,8 @@ void intel_guc_init_fw(struct intel_guc *guc)
>  		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
>  	} else {
>  		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
> -		i915.enable_guc_loading = 0;
>  		return;
>  	}
> -
> -	intel_uc_prepare_fw(dev_priv, &guc->fw);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 757f618..d073a68 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -141,18 +141,10 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
>  }
>  
>  /**
> - * intel_huc_init_fw() - select and prepare firmware for loading
> + * intel_huc_select_fw() - selects HuC firmware for loading
>   * @huc:	intel_huc struct
> - *
> - * Called early during driver load, but after GEM is initialised. The loading
> - * will continue only when driver explicitly specify firmware name and version.
> - * All other cases are considered as INTEL_UC_FIRMWARE_NONE either because HW
> - * is not capable or driver yet support it. And there will be no error message
> - * for INTEL_UC_FIRMWARE_NONE cases.
> - *
> - * The DMA-copying to HW is done later when intel_huc_init_hw() is called.
>   */
> -void intel_huc_init_fw(struct intel_huc *huc)
> +void intel_huc_select_fw(struct intel_huc *huc)
>  {
>  	struct drm_i915_private *dev_priv = huc_to_i915(huc);
>  
> @@ -177,8 +169,6 @@ void intel_huc_init_fw(struct intel_huc *huc)
>  		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
>  		return;
>  	}
> -
> -	intel_uc_prepare_fw(dev_priv, &huc->fw);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index ac9ad59..89681b37 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -66,6 +66,16 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>  		if (!i915.enable_guc_loading)
>  			i915.enable_guc_submission = 0;
>  	}

Code below is only applicable to "else" from the above

> +
> +	if (i915.enable_guc_loading) {
> +		if (HAS_HUC_UCODE(dev_priv))
> +			intel_huc_select_fw(&dev_priv->huc);
> +
> +		intel_guc_select_fw(&dev_priv->guc);
> +
> +		if (!dev_priv->guc.fw.path)
> +			i915.enable_guc_loading = 0;

Maybe simpler like this:

	i915.enable_guc_loading = intel_guc_select_fw(guc);

and likely it can be done earlier too.


> +	}
>  }
>  
>  void intel_uc_init_early(struct drm_i915_private *dev_priv)
> @@ -75,13 +85,11 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  
>  void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>  {
> -	if (!i915.enable_guc_loading)
> -		return;
> +	if (dev_priv->huc.fw.path)

Maybe we can move this common "if" to "intel_uc_prepare_fw()" ?


-Michal

> +		intel_uc_prepare_fw(dev_priv, &dev_priv->huc.fw);
>  
> -	if (HAS_HUC_UCODE(dev_priv))
> -		intel_huc_init_fw(&dev_priv->huc);
> -
> -	intel_guc_init_fw(&dev_priv->guc);
> +	if (dev_priv->guc.fw.path)
> +		intel_uc_prepare_fw(dev_priv, &dev_priv->guc.fw);
>  }
>  
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index d19c95e..5f04ea1 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -194,7 +194,7 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
>  			 struct intel_uc_fw *uc_fw);
>  
>  /* intel_guc_loader.c */
> -void intel_guc_init_fw(struct intel_guc *guc);
> +void intel_guc_select_fw(struct intel_guc *guc);
>  int intel_guc_init_hw(struct drm_i915_private *dev_priv);
>  void intel_guc_fini(struct drm_i915_private *dev_priv);
>  const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
> @@ -230,7 +230,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>  }
>  
>  /* intel_huc.c */
> -void intel_huc_init_fw(struct intel_huc *huc);
> +void intel_huc_select_fw(struct intel_huc *huc);
>  void intel_huc_fini(struct drm_i915_private  *dev_priv);
>  int intel_huc_init_hw(struct drm_i915_private *dev_priv);
>  void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
> -- 
> 2.9.3
> 
> _______________________________________________
> 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 10/10] drm/i915/uc: Add params for specifying firmware

[Michal Wajdeczko]

On Fri, Feb 24, 2017 at 04:40:04PM +0100, Arkadiusz Hiler wrote:
> `guc_firmware_path` and `huc_firmware_path` module parameters are added.
> 
> Using the parameter disabled version checks and loads desired firmware
> instead of the default one.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_params.c      | 10 ++++++++++
>  drivers/gpu/drm/i915/i915_params.h      |  2 ++
>  drivers/gpu/drm/i915/intel_guc_loader.c |  6 +++++-
>  drivers/gpu/drm/i915/intel_huc.c        |  6 +++++-
>  drivers/gpu/drm/i915/intel_uc.c         |  5 +++--
>  5 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 2e9645e..9c3ff3c 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -59,6 +59,8 @@ struct i915_params i915 __read_mostly = {
>  	.enable_guc_loading = 0,
>  	.enable_guc_submission = 0,
>  	.guc_log_level = -1,
> +	.guc_firmware_path = NULL,
> +	.huc_firmware_path = NULL,
>  	.enable_dp_mst = true,
>  	.inject_load_failure = 0,
>  	.enable_dpcd_backlight = false,
> @@ -230,6 +232,14 @@ module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
>  MODULE_PARM_DESC(guc_log_level,
>  	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
>  
> +module_param_named(guc_firmware_path, i915.guc_firmware_path, charp, 0400);
> +MODULE_PARM_DESC(guc_firmware_path,
> +	"GuC firmware path to use instead of the default one");
> +
> +module_param_named(huc_firmware_path, i915.huc_firmware_path, charp, 0400);
> +MODULE_PARM_DESC(huc_firmware_path,
> +	"HuC firmware path to use instead of the default one");
> +
>  module_param_named_unsafe(enable_dp_mst, i915.enable_dp_mst, bool, 0600);
>  MODULE_PARM_DESC(enable_dp_mst,
>  	"Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 55d47ee..34148cc 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -46,6 +46,8 @@
>  	func(int, enable_guc_loading); \
>  	func(int, enable_guc_submission); \
>  	func(int, guc_log_level); \
> +	func(char *, guc_firmware_path); \
> +	func(char *, huc_firmware_path); \
>  	func(int, use_mmio_flip); \
>  	func(int, mmio_debug); \
>  	func(int, edp_vswing); \
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 8ccd832..8a03836 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -479,7 +479,11 @@ void intel_guc_select_fw(struct intel_guc *guc)
>  	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
>  	guc->fw.fw = INTEL_UC_FW_TYPE_GUC;
>  
> -	if (IS_SKYLAKE(dev_priv)) {
> +	if (i915.guc_firmware_path) {
> +		guc->fw.path = i915.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;
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index d073a68..b2067bf 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -153,7 +153,11 @@ void intel_huc_select_fw(struct intel_huc *huc)
>  	huc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
>  	huc->fw.fw = INTEL_UC_FW_TYPE_HUC;
>  
> -	if (IS_SKYLAKE(dev_priv)) {
> +	if (i915.huc_firmware_path) {
> +		huc->fw.path = i915.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;
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 89681b37..bbfb5b6 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -357,8 +357,9 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
>  		goto fail;
>  	}
>  
> -	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> -	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
> +	if (!(uc_fw->major_ver_found == 0 && uc_fw->minor_ver_found == 0) &&

Hmm, is this correct condition ?


-Michal

> +	    (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> +	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted)) {
>  		DRM_NOTE("uC firmware version %d.%d, required %d.%d\n",
>  			uc_fw->major_ver_found, uc_fw->minor_ver_found,
>  			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> -- 
> 2.9.3
> 
> _______________________________________________
> 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 05/10] drm/i915/uc: Introduce intel_uc_init_fw()

[Joonas Lahtinen]

On pe, 2017-02-24 at 16:39 +0100, Arkadiusz Hiler wrote:
> Instead of calling intel_guc_init() and intel_huc_init() one by one this
> patch introduces intel_uc_init_fw() function that calls them both.
> 
> Called functions are renamed accordingly.
> 
> Trying to have subject_verb_object ordering and more descriptive names,
> the intel_huc_init() and intel_guc_init() functions are renamed.
> 
> For guc_init():
>  * `intel_guc` is the subject, so those functions now take intel_guc
>    structure, instead of the dev_priv
>  * init is the verb
>  * fw is the object which better describes the function's role
> 
> huc_init() change follows the same reasoning.
> 
> v2: settle on intel_uc_fetch_fw name (M. Wajdeczko)
> v3: yet another rename - intel_uc_init_fw (J. Lahtinen)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/10] drm/i915/uc: Separate firmware selection and preparation

[Joonas Lahtinen]

On pe, 2017-02-24 at 16:40 +0100, Arkadiusz Hiler wrote:
> intel_{h,g}uc_init_fw selects correct firmware and then triggers it's
> preparation (fetch + initial parsing).
> 
> This change separates out select steps, so those can be called by
> the sanitize_options().
> 
> Then, during the init_fw() we prepare the firmware if the firmware was
> selected.
> 
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

One comment below.

> @@ -66,6 +66,16 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>  		if (!i915.enable_guc_loading)
>  			i915.enable_guc_submission = 0;
>  	}
> +
> +	if (i915.enable_guc_loading) {
> +		if (HAS_HUC_UCODE(dev_priv))
> +			intel_huc_select_fw(&dev_priv->huc);
> +
> +		intel_guc_select_fw(&dev_priv->guc);
> +
> +		if (!dev_priv->guc.fw.path)
> +			i915.enable_guc_loading = 0;

Maybe make this (as suggested by Michal):

	if (intel_guc_select_fw(&dev_priv->guc) < 0)
		i915.enable_guc_loading = 0;

I think the select_fw is appropriate in this place once we introduce
the option to override the FW path, it'll part of sanitization.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915/uc: Move intel_uc_fw_fetch() to intel_uc.c

[Arkadiusz Hiler]

The file fits better.

Additionally rename it to intel_uc_prepare_fw(), as the function does
more than simple fetch.

v2: remove second declaration, reorder (M. Wajdeczko)

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 137 +-------------------------------
 drivers/gpu/drm/i915/intel_huc.c        |   2 +-
 drivers/gpu/drm/i915/intel_uc.c         | 135 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |   4 +-
 4 files changed, 139 insertions(+), 139 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 9f09e26..20e3337 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -26,7 +26,6 @@
  *    Dave Gordon <david.s.gordon@intel.com>
  *    Alex Dai <yu.dai@intel.com>
  */
-#include <linux/firmware.h>
 #include "i915_drv.h"
 #include "intel_uc.h"
 
@@ -587,140 +586,6 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
-			 struct intel_uc_fw *uc_fw)
-{
-	struct pci_dev *pdev = dev_priv->drm.pdev;
-	struct drm_i915_gem_object *obj;
-	const struct firmware *fw = NULL;
-	struct uc_css_header *css;
-	size_t size;
-	int err;
-
-	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
-		intel_uc_fw_status_repr(uc_fw->fetch_status));
-
-	err = request_firmware(&fw, uc_fw->path, &pdev->dev);
-	if (err)
-		goto fail;
-	if (!fw)
-		goto fail;
-
-	DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw %p\n",
-		uc_fw->path, fw);
-
-	/* Check the size of the blob before examining buffer contents */
-	if (fw->size < sizeof(struct uc_css_header)) {
-		DRM_NOTE("Firmware header is missing\n");
-		goto fail;
-	}
-
-	css = (struct uc_css_header *)fw->data;
-
-	/* Firmware bits always start from header */
-	uc_fw->header_offset = 0;
-	uc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -
-		css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
-
-	if (uc_fw->header_size != sizeof(struct uc_css_header)) {
-		DRM_NOTE("CSS header definition mismatch\n");
-		goto fail;
-	}
-
-	/* then, uCode */
-	uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
-	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
-
-	/* now RSA */
-	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
-		DRM_NOTE("RSA key size is bad\n");
-		goto fail;
-	}
-	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
-	uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
-
-	/* At least, it should have header, uCode and RSA. Size of all three. */
-	size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
-	if (fw->size < size) {
-		DRM_NOTE("Missing firmware components\n");
-		goto fail;
-	}
-
-	/*
-	 * The GuC firmware image has the version number embedded at a well-known
-	 * offset within the firmware blob; note that major / minor version are
-	 * TWO bytes each (i.e. u16), although all pointers and offsets are defined
-	 * in terms of bytes (u8).
-	 */
-	switch (uc_fw->fw) {
-	case INTEL_UC_FW_TYPE_GUC:
-		/* Header and uCode will be loaded to WOPCM. Size of the two. */
-		size = uc_fw->header_size + uc_fw->ucode_size;
-
-		/* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */
-		if (size > intel_guc_wopcm_size(dev_priv)) {
-			DRM_ERROR("Firmware is too large to fit in WOPCM\n");
-			goto fail;
-		}
-		uc_fw->major_ver_found = css->guc.sw_version >> 16;
-		uc_fw->minor_ver_found = css->guc.sw_version & 0xFFFF;
-		break;
-
-	case INTEL_UC_FW_TYPE_HUC:
-		uc_fw->major_ver_found = css->huc.sw_version >> 16;
-		uc_fw->minor_ver_found = css->huc.sw_version & 0xFFFF;
-		break;
-
-	default:
-		DRM_ERROR("Unknown firmware type %d\n", uc_fw->fw);
-		err = -ENOEXEC;
-		goto fail;
-	}
-
-	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
-	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
-		DRM_NOTE("uC firmware version %d.%d, required %d.%d\n",
-			uc_fw->major_ver_found, uc_fw->minor_ver_found,
-			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
-		err = -ENOEXEC;
-		goto fail;
-	}
-
-	DRM_DEBUG_DRIVER("firmware version %d.%d OK (minimum %d.%d)\n",
-			uc_fw->major_ver_found, uc_fw->minor_ver_found,
-			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
-
-	mutex_lock(&dev_priv->drm.struct_mutex);
-	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-	if (IS_ERR_OR_NULL(obj)) {
-		err = obj ? PTR_ERR(obj) : -ENOMEM;
-		goto fail;
-	}
-
-	uc_fw->obj = obj;
-	uc_fw->size = fw->size;
-
-	DRM_DEBUG_DRIVER("uC fw fetch status SUCCESS, obj %p\n",
-			uc_fw->obj);
-
-	release_firmware(fw);
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
-	return;
-
-fail:
-	DRM_WARN("Failed to fetch valid uC firmware from %s (error %d)\n",
-		 uc_fw->path, err);
-	DRM_DEBUG_DRIVER("uC fw fetch status FAIL; err %d, fw %p, obj %p\n",
-		err, fw, uc_fw->obj);
-
-	obj = fetch_and_zero(&uc_fw->obj);
-	if (obj)
-		i915_gem_object_put(obj);
-
-	release_firmware(fw);		/* OK even if fw is NULL */
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
-}
 
 /**
  * intel_guc_init() - define parameters and fetch firmware
@@ -779,7 +644,7 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
 
 	guc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
 	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
-	intel_uc_fw_fetch(dev_priv, guc_fw);
+	intel_uc_prepare_fw(dev_priv, guc_fw);
 	/* status must now be FAIL or SUCCESS */
 }
 
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index babe0eb..566fa7e 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -187,7 +187,7 @@ void intel_huc_init(struct drm_i915_private *dev_priv)
 
 	WARN(huc_fw->path == NULL, "HuC present but no fw path\n");
 
-	intel_uc_fw_fetch(dev_priv, huc_fw);
+	intel_uc_prepare_fw(dev_priv, huc_fw);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index c46bc85..7155aa7 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -24,6 +24,7 @@
 
 #include "i915_drv.h"
 #include "intel_uc.h"
+#include <linux/firmware.h>
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
@@ -114,3 +115,137 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
+void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
+			 struct intel_uc_fw *uc_fw)
+{
+	struct pci_dev *pdev = dev_priv->drm.pdev;
+	struct drm_i915_gem_object *obj;
+	const struct firmware *fw = NULL;
+	struct uc_css_header *css;
+	size_t size;
+	int err;
+
+	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
+		intel_uc_fw_status_repr(uc_fw->fetch_status));
+
+	err = request_firmware(&fw, uc_fw->path, &pdev->dev);
+	if (err)
+		goto fail;
+	if (!fw)
+		goto fail;
+
+	DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw %p\n",
+		uc_fw->path, fw);
+
+	/* Check the size of the blob before examining buffer contents */
+	if (fw->size < sizeof(struct uc_css_header)) {
+		DRM_NOTE("Firmware header is missing\n");
+		goto fail;
+	}
+
+	css = (struct uc_css_header *)fw->data;
+
+	/* Firmware bits always start from header */
+	uc_fw->header_offset = 0;
+	uc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -
+		css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
+
+	if (uc_fw->header_size != sizeof(struct uc_css_header)) {
+		DRM_NOTE("CSS header definition mismatch\n");
+		goto fail;
+	}
+
+	/* then, uCode */
+	uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
+	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
+
+	/* now RSA */
+	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
+		DRM_NOTE("RSA key size is bad\n");
+		goto fail;
+	}
+	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
+	uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
+
+	/* At least, it should have header, uCode and RSA. Size of all three. */
+	size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
+	if (fw->size < size) {
+		DRM_NOTE("Missing firmware components\n");
+		goto fail;
+	}
+
+	/*
+	 * The GuC firmware image has the version number embedded at a
+	 * well-known offset within the firmware blob; note that major / minor
+	 * version are TWO bytes each (i.e. u16), although all pointers and
+	 * offsets are defined in terms of bytes (u8).
+	 */
+	switch (uc_fw->fw) {
+	case INTEL_UC_FW_TYPE_GUC:
+		/* Header and uCode will be loaded to WOPCM. Size of the two. */
+		size = uc_fw->header_size + uc_fw->ucode_size;
+
+		/* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */
+		if (size > intel_guc_wopcm_size(dev_priv)) {
+			DRM_ERROR("Firmware is too large to fit in WOPCM\n");
+			goto fail;
+		}
+		uc_fw->major_ver_found = css->guc.sw_version >> 16;
+		uc_fw->minor_ver_found = css->guc.sw_version & 0xFFFF;
+		break;
+
+	case INTEL_UC_FW_TYPE_HUC:
+		uc_fw->major_ver_found = css->huc.sw_version >> 16;
+		uc_fw->minor_ver_found = css->huc.sw_version & 0xFFFF;
+		break;
+
+	default:
+		DRM_ERROR("Unknown firmware type %d\n", uc_fw->fw);
+		err = -ENOEXEC;
+		goto fail;
+	}
+
+	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
+	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
+		DRM_NOTE("uC firmware version %d.%d, required %d.%d\n",
+			uc_fw->major_ver_found, uc_fw->minor_ver_found,
+			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
+		err = -ENOEXEC;
+		goto fail;
+	}
+
+	DRM_DEBUG_DRIVER("firmware version %d.%d OK (minimum %d.%d)\n",
+			uc_fw->major_ver_found, uc_fw->minor_ver_found,
+			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
+
+	mutex_lock(&dev_priv->drm.struct_mutex);
+	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+	if (IS_ERR_OR_NULL(obj)) {
+		err = obj ? PTR_ERR(obj) : -ENOMEM;
+		goto fail;
+	}
+
+	uc_fw->obj = obj;
+	uc_fw->size = fw->size;
+
+	DRM_DEBUG_DRIVER("uC fw fetch status SUCCESS, obj %p\n",
+			uc_fw->obj);
+
+	release_firmware(fw);
+	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
+	return;
+
+fail:
+	DRM_WARN("Failed to fetch valid uC firmware from %s (error %d)\n",
+		 uc_fw->path, err);
+	DRM_DEBUG_DRIVER("uC fw fetch status FAIL; err %d, fw %p, obj %p\n",
+		err, fw, uc_fw->obj);
+
+	obj = fetch_and_zero(&uc_fw->obj);
+	if (obj)
+		i915_gem_object_put(obj);
+
+	release_firmware(fw);		/* OK even if fw is NULL */
+	uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 41b7351..5fa13dc 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -185,6 +185,8 @@ struct intel_huc {
 
 /* intel_uc.c */
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
+void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
+			 struct intel_uc_fw *uc_fw);
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 
@@ -195,8 +197,6 @@ void intel_guc_fini(struct drm_i915_private *dev_priv);
 const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
 int intel_guc_suspend(struct drm_i915_private *dev_priv);
 int intel_guc_resume(struct drm_i915_private *dev_priv);
-void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
-	struct intel_uc_fw *uc_fw);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 /* i915_guc_submission.c */
-- 
2.9.3

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

Re: [PATCH 10/10] drm/i915/uc: Add params for specifying firmware

[Joonas Lahtinen]

On pe, 2017-02-24 at 16:40 +0100, Arkadiusz Hiler wrote:
> `guc_firmware_path` and `huc_firmware_path` module parameters are added.
> 
> Using the parameter disabled version checks and loads desired firmware
> instead of the default one.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

<SNIP>

> @@ -230,6 +232,14 @@ module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
>  MODULE_PARM_DESC(guc_log_level,
>  	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
>  
> +module_param_named(guc_firmware_path, i915.guc_firmware_path, charp, 0400);

I'm pretty sure this should be _unsafe, because it overrides the
version checks. Cc'd Jani for this.

> @@ -479,7 +479,11 @@ void intel_guc_select_fw(struct intel_guc *guc)
>  	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
>  	guc->fw.fw = INTEL_UC_FW_TYPE_GUC;
>  
> -	if (IS_SKYLAKE(dev_priv)) {
> +	if (i915.guc_firmware_path) {
> +		guc->fw.path = i915.guc_firmware_path;
> +		guc->fw.major_ver_wanted = 0;
> +		guc->fw.minor_ver_wanted = 0;

Or, we could keep the wanted version number, only replace the path, and
spit out WARN/taint kernel if some other version was detected?

But I guess the main purpose is to override version (not provide
request_firmware workarounds), so my vote is to make the param _unsafe.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v5] drm/i915/guc: Extract param logic form guc_init_fw()

[Arkadiusz Hiler]

Let intel_guc_init_fw() focus on determining and fetching the correct
firmware.

This patch introduces intel_uc_sanitize_options() that is called from
intel_sanitize_options().

Then, if we have GuC, we can call intel_guc_init_fw() conditionally
and we do not have to do the internal checks.

v2: fix comment, notify when nuking GuC explicitly enabled (M. Wajdeczko)
v3: fix comment again, change the nuke message (M. Wajdeczko)
v4: update title to reflect new function name + rebase
v5: text && remove 2 uneccessary checks (M. Wajeczko)

Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  2 ++
 drivers/gpu/drm/i915/intel_guc_loader.c | 18 +-----------------
 drivers/gpu/drm/i915/intel_huc.c        |  3 ---
 drivers/gpu/drm/i915/intel_uc.c         | 28 +++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_uc.h         |  1 +
 5 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 56624bf..7964e7f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -993,6 +993,8 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
 
 	i915.semaphores = intel_sanitize_semaphores(dev_priv, i915.semaphores);
 	DRM_DEBUG_DRIVER("use GPU semaphores? %s\n", yesno(i915.semaphores));
+
+	intel_uc_sanitize_options(dev_priv);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 87b7a39..fb518e0 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -601,20 +601,7 @@ void intel_guc_init_fw(struct intel_guc *guc)
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	const char *fw_path;
 
-	if (!HAS_GUC(dev_priv)) {
-		i915.enable_guc_loading = 0;
-		i915.enable_guc_submission = 0;
-	} else {
-		/* A negative value means "use platform default" */
-		if (i915.enable_guc_loading < 0)
-			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
-		if (i915.enable_guc_submission < 0)
-			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
-	}
-
-	if (!HAS_GUC_UCODE(dev_priv)) {
-		fw_path = NULL;
-	} else if (IS_SKYLAKE(dev_priv)) {
+	if (IS_SKYLAKE(dev_priv)) {
 		fw_path = I915_SKL_GUC_UCODE;
 		guc->fw.major_ver_wanted = SKL_FW_MAJOR;
 		guc->fw.minor_ver_wanted = SKL_FW_MINOR;
@@ -634,9 +621,6 @@ void intel_guc_init_fw(struct intel_guc *guc)
 	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
 	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
 
-	/* Early (and silent) return if GuC loading is disabled */
-	if (!i915.enable_guc_loading)
-		return;
 	if (fw_path == NULL)
 		return;
 	if (*fw_path == '\0')
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 6f0811c..9707da3 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -162,9 +162,6 @@ void intel_huc_init_fw(struct intel_huc *huc)
 	huc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
 	huc->fw.fw = INTEL_UC_FW_TYPE_HUC;
 
-	if (!HAS_HUC_UCODE(dev_priv))
-		return;
-
 	if (IS_SKYLAKE(dev_priv)) {
 		fw_path = I915_SKL_HUC_UCODE;
 		huc->fw.major_ver_wanted = SKL_HUC_FW_MAJOR;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 5c204e6..7453a648 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -26,6 +26,27 @@
 #include "intel_uc.h"
 #include <linux/firmware.h>
 
+void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_GUC(dev_priv)) {
+		if (i915.enable_guc_loading > 0)
+			DRM_INFO("Ignoring GuC options, no hardware");
+
+		i915.enable_guc_loading = 0;
+		i915.enable_guc_submission = 0;
+	} else {
+		/* A negative value means "use platform default" */
+		if (i915.enable_guc_loading < 0)
+			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
+		if (i915.enable_guc_submission < 0)
+			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
+
+		/* Can't enable guc submission without guc loaded */
+		if (!i915.enable_guc_loading)
+			i915.enable_guc_submission = 0;
+	}
+}
+
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
 	mutex_init(&dev_priv->guc.send_mutex);
@@ -33,7 +54,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 
 void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 {
-	intel_huc_init_fw(&dev_priv->huc);
+	if (!i915.enable_guc_loading)
+		return;
+
+	if (HAS_HUC_UCODE(dev_priv))
+		intel_huc_init_fw(&dev_priv->huc);
+
 	intel_guc_init_fw(&dev_priv->guc);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 28ea8c9..2ee148c 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -184,6 +184,7 @@ struct intel_huc {
 };
 
 /* intel_uc.c */
+void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_uc_init_fw(struct drm_i915_private *dev_priv);
 void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
-- 
2.9.3

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

Re: [PATCH 10/10] drm/i915/uc: Add params for specifying firmware

[Jani Nikula]

On Mon, 27 Feb 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> On pe, 2017-02-24 at 16:40 +0100, Arkadiusz Hiler wrote:
>> `guc_firmware_path` and `huc_firmware_path` module parameters are added.
>> 
>> Using the parameter disabled version checks and loads desired firmware
>> instead of the default one.
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Michal Winiarski <michal.winiarski@intel.com>
>> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>
> <SNIP>
>
>> @@ -230,6 +232,14 @@ module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
>>  MODULE_PARM_DESC(guc_log_level,
>>  	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
>>  
>> +module_param_named(guc_firmware_path, i915.guc_firmware_path, charp, 0400);
>
> I'm pretty sure this should be _unsafe, because it overrides the
> version checks. Cc'd Jani for this.

Yes, please. I replied the same thing to Chris' patches adding the same
thing.

BR,
Jani.

>
>> @@ -479,7 +479,11 @@ void intel_guc_select_fw(struct intel_guc *guc)
>>  	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
>>  	guc->fw.fw = INTEL_UC_FW_TYPE_GUC;
>>  
>> -	if (IS_SKYLAKE(dev_priv)) {
>> +	if (i915.guc_firmware_path) {
>> +		guc->fw.path = i915.guc_firmware_path;
>> +		guc->fw.major_ver_wanted = 0;
>> +		guc->fw.minor_ver_wanted = 0;
>
> Or, we could keep the wanted version number, only replace the path, and
> spit out WARN/taint kernel if some other version was detected?
>
> But I guess the main purpose is to override version (not provide
> request_firmware workarounds), so my vote is to make the param _unsafe.
>
> Regards, Joonas

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for GuC Scrub vol. 1 (rev7)

[Patchwork]

== Series Details ==

Series: GuC Scrub vol. 1 (rev7)
URL   : https://patchwork.freedesktop.org/series/16856/
State : failure

== Summary ==

In file included from drivers/gpu/drm/i915/i915_drv.h:60:0,
                 from drivers/gpu/drm/i915/i915_gem_execbuffer.c:37:
drivers/gpu/drm/i915/intel_uc.h:188:1: error: expected identifier or ‘(’ before ‘<<’ token
 <<<<<<< df1dec2d5faca106a651e75b02b845fa246ec77f
 ^
drivers/gpu/drm/i915/intel_uc.h:193:1: error: expected identifier or ‘(’ before ‘>>’ token
 >>>>>>> drm/i915/uc: Introduce intel_uc_init_fw()
 ^
  LD      drivers/pci/pcie/aer/built-in.o
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/intel_device_info.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_device_info.o] Error 1
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/intel_csr.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_csr.o] Error 1
  LD      drivers/pci/pcie/built-in.o
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/i915_gem_batch_pool.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_gem_batch_pool.o] Error 1
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/i915_gem_evict.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_gem_evict.o] Error 1
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/intel_pipe_crc.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_pipe_crc.o] Error 1
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/i915_cmd_parser.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_cmd_parser.o] Error 1
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/i915_debugfs.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_debugfs.o] Error 1
  LD      drivers/acpi/acpica/built-in.o
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/i915_ioc32.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_ioc32.o] Error 1
  LD      drivers/acpi/built-in.o
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/i915_gem_clflush.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_gem_clflush.o] Error 1
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/i915_gem_dmabuf.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_gem_dmabuf.o] Error 1
  LD      drivers/scsi/scsi_mod.o
  LD      drivers/usb/gadget/udc/udc-core.o
  LD      drivers/usb/gadget/udc/built-in.o
  LD      net/netlink/built-in.o
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/intel_runtime_pm.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_runtime_pm.o] Error 1
  LD      drivers/usb/gadget/built-in.o
  LD      drivers/video/fbdev/core/fb.o
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/i915_gem_execbuffer.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_gem_execbuffer.o] Error 1
  LD      drivers/video/fbdev/core/built-in.o
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/i915_gem_context.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_gem_context.o] Error 1
  LD      drivers/tty/serial/8250/8250_base.o
  LD      drivers/video/fbdev/built-in.o
  LD      drivers/tty/serial/8250/built-in.o
  LD      drivers/tty/serial/built-in.o
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/intel_pm.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_pm.o] Error 1
scripts/Makefile.build:553: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:553: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:553: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
make[1]: *** Waiting for unfinished jobs....
  LD      net/unix/unix.o
  AR      lib/lib.a
  LD      drivers/pci/built-in.o
  LD      net/unix/built-in.o
  EXPORTS lib/lib-ksyms.o
  LD      lib/built-in.o
  LD      drivers/usb/core/usbcore.o
  LD      drivers/video/console/built-in.o
  LD      drivers/video/built-in.o
  LD      drivers/usb/core/built-in.o
  LD [M]  drivers/net/ethernet/intel/e1000/e1000.o
  LD      drivers/scsi/sd_mod.o
  LD      drivers/scsi/built-in.o
  LD      net/xfrm/built-in.o
  LD      net/packet/built-in.o
  LD      drivers/tty/vt/built-in.o
  CC      arch/x86/kernel/cpu/capflags.o
  LD      drivers/tty/built-in.o
  LD      arch/x86/kernel/cpu/built-in.o
  LD      fs/btrfs/btrfs.o
  LD      arch/x86/kernel/built-in.o
  LD      fs/btrfs/built-in.o
  LD      net/ipv6/ipv6.o
  LD      arch/x86/built-in.o
  LD      net/ipv6/built-in.o
  LD      drivers/usb/host/xhci-hcd.o
  LD      drivers/md/md-mod.o
  LD      drivers/md/built-in.o
  LD [M]  drivers/net/ethernet/intel/igb/igb.o
  LD      drivers/usb/host/built-in.o
  LD      drivers/usb/built-in.o
  LD      fs/ext4/ext4.o
  LD      fs/ext4/built-in.o
  LD      fs/built-in.o
  LD      net/core/built-in.o
  LD      net/ipv4/built-in.o
  LD [M]  drivers/net/ethernet/intel/e1000e/e1000e.o
  LD      net/built-in.o
  LD      drivers/net/ethernet/built-in.o
  LD      drivers/net/built-in.o
Makefile:988: recipe for target 'drivers' failed
make: *** [drivers] Error 2

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

Re: [PATCH 07/10] drm/i915/guc: Simplify intel_guc_init_hw()

[Arkadiusz Hiler]

On Fri, Feb 24, 2017 at 06:26:10PM +0100, Michal Wajdeczko wrote:
> On Fri, Feb 24, 2017 at 04:40:01PM +0100, Arkadiusz Hiler wrote:
> > Current version of intel_guc_init_hw() does a lot:
> >  - cares about submission
> >  - loads huc
> >  - implement WA
> > 
> > This change offloads some of the logic to intel_uc_init_hw(), which now
> > cares about the above.
> > 
> > v2: rename guc_hw_reset and fix typo in define name (M. Wajdeczko)
> > v3: rename once again
> > v4: remove spurious comments and add some style (J. Lahtinen)
> > 
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c         |   2 +-
> >  drivers/gpu/drm/i915/intel_guc_loader.c | 144 ++++----------------------------
> >  drivers/gpu/drm/i915/intel_uc.c         | 110 ++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_uc.h         |   3 +
> >  4 files changed, 128 insertions(+), 131 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 5b36524..8943c4e 100644
> > @@ -443,42 +425,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
> >  {
> <SNIP>
> > -	/* Loading forbidden, or no firmware to load? */
> > -	if (!i915.enable_guc_loading) {
> > -		err = 0;
> > -		goto fail;
> > -	} else if (fw_path == NULL) {
> > -		/* Device is known to have no uCode (e.g. no GuC) */
> > -		err = -ENXIO;
> > -		goto fail;
> > +	if (!fw_path) {
> > +		return -ENXIO;
> >  	} else if (*fw_path == '\0') {
> 
> Hmm, is this case still possible?

In this revision - yes, but unlikely.

HAS_GUC() is true but firmware for the device is not known (see
init_fw()). I think that the initial flow was designed in this case in
mind.

> > -		/* Device has a GuC but we don't know what f/w to load? */
> >  		WARN(1, "No GuC firmware known for this platform!\n");
> > -		err = -ENODEV;
> > -		goto fail;
> > +		return -ENODEV;
> >  	}
> >  
> > -	/* Fetch failed, or already fetched but failed to load? */
> > -	if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) {
> > -		err = -EIO;
> > -		goto fail;
> > -	} else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) {
> > -		err = -ENOEXEC;
> > -		goto fail;
> > -	}
> > -
> > -	guc_interrupts_release(dev_priv);
> > -	gen9_reset_guc_interrupts(dev_priv);
> > -
> > -	/* We need to notify the guc whenever we change the GGTT */
> > -	i915_ggtt_enable_guc(dev_priv);
> > +	if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> > +		return -EIO;
> > +	else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL)
> > +		return -ENOEXEC;
> 
> Hmm, it looks like you're checking for load failure here, but actual
> load is about to start below ? Did I missed something ?

The status FIRMWARE_FAIL is not used at all in the GuC path (HuC) uses
it. Noted down to give it a closer look.

The check is gone, as it does not make sense, thanks.

> >  	guc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
> 
> I guess this status can be set in guc_ucode_xfer() as it uses guc_fw object.

> >  
> > @@ -486,104 +450,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
> >  		intel_uc_fw_status_repr(guc_fw->fetch_status),
> >  		intel_uc_fw_status_repr(guc_fw->load_status));
> >  
> > -	err = i915_guc_submission_init(dev_priv);
> > -	if (err)
> > -		goto fail;
> > -
> >  	/*
> >  	 * WaEnableuKernelHeaderValidFix:skl,bxt
> >  	 * For BXT, this is only upto B0 but below WA is required for later
> >  	 * steppings also so this is extended as well.
> >  	 */
> 
> Rebase issue?

Yeah. I've streamlined the whole thing to be:

----------------------------------------------------------
int attempts;
const int guc_wa_hash_check_not_set_attempts = 3;

<SNIP>

/* WaEnableuKernelHeaderValidFix:skl */
/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
if (IS_GEN9(dev_priv))
        attempts = guc_wa_hash_check_not_set_attempts;
else
        attempts = 1;

while (attempts--) ...
----------------------------------------------------------


and dropped the WA comments in the guc_init_hw()



> > +int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> > +{
> <SNIP>
> > +fail:
> > +	/*
> > +	 * 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).
> > +	 */
> > +	DRM_ERROR("GuC init failed\n");
> > +	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
> 
> Nonzero i915.enable_guc_loading is guaranteed (see your first "if" in this function)

That's why this not a check for "non-zeroness".

It's cheeking for "greater than one", adhering to existing behavior:
 0   = disabled
 1   = enabled with possible fallback (i.e. ignore the error and use execlists)
 > 1 = wedge if this fail

> > +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> > +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS 3
> > +
> 
> Hmm, is it worth to define this macro here as it is used only once and
> it is very specific ? Do you plan to define more similar macros?

Changed to const, but i915 seems to be generally define-biased for
similar things.

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

Re: [PATCH v5] drm/i915/guc: Extract param logic form guc_init_fw()

[Joonas Lahtinen]

On ma, 2017-02-27 at 13:50 +0100, Arkadiusz Hiler wrote:
> Let intel_guc_init_fw() focus on determining and fetching the correct
> firmware.
> 
> This patch introduces intel_uc_sanitize_options() that is called from
> intel_sanitize_options().
> 
> Then, if we have GuC, we can call intel_guc_init_fw() conditionally
> and we do not have to do the internal checks.
> 
> v2: fix comment, notify when nuking GuC explicitly enabled (M. Wajdeczko)
> v3: fix comment again, change the nuke message (M. Wajdeczko)
> v4: update title to reflect new function name + rebase
> v5: text && remove 2 uneccessary checks (M. Wajeczko)
> 
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/10] drm/i915/uc: Separate firmware selection and preparation

[Arkadiusz Hiler]

On Fri, Feb 24, 2017 at 07:29:57PM +0100, Michal Wajdeczko wrote:
> On Fri, Feb 24, 2017 at 04:40:03PM +0100, Arkadiusz Hiler wrote:
> > intel_{h,g}uc_init_fw selects correct firmware and then triggers it's
> > preparation (fetch + initial parsing).
> > 
> > This change separates out select steps, so those can be called by
> > the sanitize_options().
> > 
> > Then, during the init_fw() we prepare the firmware if the firmware was
> > selected.
> > 
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > ---
> 
> It looks strange that in one series you're changing function names
> or their logic few times ... patch ordering really matters ;)

Is this really that bad? I do not see that much disadvantage to the
current state.

I am seeing the series as a whole. Titles of the commits are pretty
informative (and you have them both in you email client when you are
viewing the whole thread, and in the cover letter) - e.g. you can see that
one of the later commits reworks firmware path handling, and can assume
that the previous changes to that code were intermediary (changing
argument it takes and removing one or two checks as they are now on the
level above).

As of the name changes - those follow a pattern.

If I went with the original name until the final change it looks
inconsistent, and quite probably someone would've commented on that.

If I have had used the final names straight away that would be simply
misleading (in case of intel_guc_init -> init_fw -> select_fw).

> Please consider reorder/squash.

I've tried to rebase with squashing and resplitting two times, and what
I came with was pretty similar to the current series.

But I might have hard-wired my brain to follow those exact steps,
therefore the repetition...

If you have any suggestion how to do that properly, keeping changes
small, logical and ordered please share. I'd love to learn how to make
my series less painful for future reviewers.

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

[PATCH 07/10] drm/i915/guc: Simplify intel_guc_init_hw()

[Arkadiusz Hiler]

Current version of intel_guc_init_hw() does a lot:
 - cares about submission
 - loads huc
 - implement WA

This change offloads some of the logic to intel_uc_init_hw(), which now
cares about the above.

v2: rename guc_hw_reset and fix typo in define name (M. Wajdeczko)
v3: rename once again
v4: remove spurious comments and add some style (J. Lahtinen)
v5: flow changes, got rid of dead checks (M. Wajdeczko)

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |   2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 147 +++-----------------------------
 drivers/gpu/drm/i915/intel_uc.c         | 114 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |   3 +
 4 files changed, 130 insertions(+), 136 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2c3057c..ef9f8e5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4454,7 +4454,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 	intel_mocs_init_l3cc_table(dev_priv);
 
 	/* We can't enable contexts until all firmware is loaded */
-	ret = intel_guc_init_hw(&dev_priv->guc);
+	ret = intel_uc_init_hw(dev_priv);
 	if (ret)
 		goto out;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 2761a76..f63e7e8 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -90,7 +90,7 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 	}
 };
 
-static void guc_interrupts_release(struct drm_i915_private *dev_priv)
+void intel_guc_release_interrupts(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
@@ -108,7 +108,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
 	I915_WRITE(GUC_WD_VECS_IER, 0);
 }
 
-static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
+void intel_guc_capture_interrupts(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
@@ -408,24 +408,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-static int guc_hw_reset(struct drm_i915_private *dev_priv)
-{
-	int ret;
-	u32 guc_status;
-
-	ret = intel_guc_reset(dev_priv);
-	if (ret) {
-		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
-		return ret;
-	}
-
-	guc_status = I915_READ(GUC_STATUS);
-	WARN(!(guc_status & GS_MIA_IN_RESET),
-	     "GuC status: 0x%x, MIA core expected to be in reset\n", guc_status);
-
-	return ret;
-}
-
 /**
  * intel_guc_init_hw() - finish preparing the GuC for activity
  * @guc: intel_guc structure
@@ -443,42 +425,22 @@ int intel_guc_init_hw(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	const char *fw_path = guc->fw.path;
-	int retries, ret, err;
+	int ret;
 
 	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
 		fw_path,
 		intel_uc_fw_status_repr(guc->fw.fetch_status),
 		intel_uc_fw_status_repr(guc->fw.load_status));
 
-	/* Loading forbidden, or no firmware to load? */
-	if (!i915.enable_guc_loading) {
-		err = 0;
-		goto fail;
-	} else if (fw_path == NULL) {
-		/* Device is known to have no uCode (e.g. no GuC) */
-		err = -ENXIO;
-		goto fail;
+	if (!fw_path) {
+		return -ENXIO;
 	} else if (*fw_path == '\0') {
-		/* Device has a GuC but we don't know what f/w to load? */
 		WARN(1, "No GuC firmware known for this platform!\n");
-		err = -ENODEV;
-		goto fail;
+		return -ENODEV;
 	}
 
-	/* Fetch failed, or already fetched but failed to load? */
-	if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS) {
-		err = -EIO;
-		goto fail;
-	} else if (guc->fw.load_status == INTEL_UC_FIRMWARE_FAIL) {
-		err = -ENOEXEC;
-		goto fail;
-	}
-
-	guc_interrupts_release(dev_priv);
-	gen9_reset_guc_interrupts(dev_priv);
-
-	/* We need to notify the guc whenever we change the GGTT */
-	i915_ggtt_enable_guc(dev_priv);
+	if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+		return -EIO;
 
 	guc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
 
@@ -486,104 +448,19 @@ int intel_guc_init_hw(struct intel_guc *guc)
 		intel_uc_fw_status_repr(guc->fw.fetch_status),
 		intel_uc_fw_status_repr(guc->fw.load_status));
 
-	err = i915_guc_submission_init(dev_priv);
-	if (err)
-		goto fail;
+	ret = guc_ucode_xfer(dev_priv);
 
-	/*
-	 * WaEnableuKernelHeaderValidFix:skl,bxt
-	 * For BXT, this is only upto B0 but below WA is required for later
-	 * steppings also so this is extended as well.
-	 */
-	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
-	for (retries = 3; ; ) {
-		/*
-		 * Always reset the GuC just before (re)loading, so
-		 * that the state and timing are fairly predictable
-		 */
-		err = guc_hw_reset(dev_priv);
-		if (err)
-			goto fail;
-
-		intel_huc_init_hw(&dev_priv->huc);
-		err = guc_ucode_xfer(dev_priv);
-		if (!err)
-			break;
-
-		if (--retries == 0)
-			goto fail;
-
-		DRM_INFO("GuC fw load failed: %d; will reset and "
-			 "retry %d more time(s)\n", err, retries);
-	}
+	if (ret)
+		return -EAGAIN;
 
 	guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
 
-	intel_guc_auth_huc(dev_priv);
-
-	if (i915.enable_guc_submission) {
-		if (i915.guc_log_level >= 0)
-			gen9_enable_guc_interrupts(dev_priv);
-
-		err = i915_guc_submission_enable(dev_priv);
-		if (err)
-			goto fail;
-		guc_interrupts_capture(dev_priv);
-	}
-
 	DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
 		 i915.enable_guc_submission ? "submission enabled" : "loaded",
 		 guc->fw.path,
 		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
 
 	return 0;
-
-fail:
-	if (guc->fw.load_status == INTEL_UC_FIRMWARE_PENDING)
-		guc->fw.load_status = INTEL_UC_FIRMWARE_FAIL;
-
-	guc_interrupts_release(dev_priv);
-	i915_guc_submission_disable(dev_priv);
-	i915_guc_submission_fini(dev_priv);
-	i915_ggtt_disable_guc(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).
-	 */
-	if (i915.enable_guc_loading > 1) {
-		ret = -EIO;
-	} else if (i915.enable_guc_submission > 1) {
-		ret = -EIO;
-	} else {
-		ret = 0;
-	}
-
-	if (err == 0 && !HAS_GUC_UCODE(dev_priv))
-		;	/* Don't mention the GuC! */
-	else if (err == 0)
-		DRM_INFO("GuC firmware load skipped\n");
-	else if (ret != -EIO)
-		DRM_NOTE("GuC firmware load failed: %d\n", err);
-	else
-		DRM_WARN("GuC firmware load failed: %d\n", err);
-
-	if (i915.enable_guc_submission) {
-		if (fw_path == NULL)
-			DRM_INFO("GuC submission without firmware not supported\n");
-		if (ret == 0)
-			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
-		else
-			DRM_ERROR("GuC init failed: %d\n", ret);
-	}
-	i915.enable_guc_submission = 0;
-
-	return ret;
 }
 
 
@@ -642,7 +519,7 @@ void intel_guc_fini(struct drm_i915_private *dev_priv)
 	struct drm_i915_gem_object *obj;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	guc_interrupts_release(dev_priv);
+	intel_guc_release_interrupts(dev_priv);
 	i915_guc_submission_disable(dev_priv);
 	i915_guc_submission_fini(dev_priv);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index f0a69d4..5988f00 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -26,6 +26,27 @@
 #include "intel_uc.h"
 #include <linux/firmware.h>
 
+/* Reset GuC providing us with fresh state for both GuC and HuC.
+ */
+static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
+{
+	int ret;
+	u32 guc_status;
+
+	ret = intel_guc_reset(dev_priv);
+	if (ret) {
+		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
+		return ret;
+	}
+
+	guc_status = I915_READ(GUC_STATUS);
+	WARN(!(guc_status & GS_MIA_IN_RESET),
+	     "GuC status: 0x%x, MIA core expected to be in reset\n",
+	     guc_status);
+
+	return ret;
+}
+
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_GUC(dev_priv)) {
@@ -63,6 +84,99 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 	intel_guc_init_fw(&dev_priv->guc);
 }
 
+int intel_uc_init_hw(struct drm_i915_private *dev_priv)
+{
+	int ret, attempts;
+	const int guc_wa_hash_check_not_set_attempts = 3;
+
+
+	/* GuC not enabled, nothing to do */
+	if (!i915.enable_guc_loading)
+		return 0;
+
+	intel_guc_release_interrupts(dev_priv);
+	gen9_reset_guc_interrupts(dev_priv);
+
+	/* We need to notify the guc whenever we change the GGTT */
+	i915_ggtt_enable_guc(dev_priv);
+
+	if (i915.enable_guc_submission) {
+		ret = i915_guc_submission_init(dev_priv);
+		if (ret)
+			goto fail;
+	}
+
+	/* WaEnableuKernelHeaderValidFix:skl */
+	/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
+	if (IS_GEN9(dev_priv))
+		attempts = guc_wa_hash_check_not_set_attempts;
+	else
+		attempts = 1;
+
+	while (attempts--) {
+		/*
+		 * Always reset the GuC just before (re)loading, so
+		 * that the state and timing are fairly predictable
+		 */
+		ret = __intel_uc_reset_hw(dev_priv);
+		if (ret)
+			goto fail;
+
+		intel_huc_init_hw(&dev_priv->huc);
+		ret = intel_guc_init_hw(&dev_priv->guc);
+		if (ret == 0 || ret != -EAGAIN)
+			break;
+
+		DRM_INFO("GuC fw load failed: %d; will reset and "
+			 "retry %d more time(s)\n", ret, attempts);
+	}
+
+	/* Did we succeded or run out of retries? */
+	if (ret)
+		goto fail;
+
+	intel_guc_auth_huc(dev_priv);
+	if (i915.enable_guc_submission) {
+		if (i915.guc_log_level >= 0)
+			gen9_enable_guc_interrupts(dev_priv);
+
+		ret = i915_guc_submission_enable(dev_priv);
+		if (ret)
+			goto fail;
+		intel_guc_capture_interrupts(dev_priv);
+	}
+
+	return 0;
+
+fail:
+	/*
+	 * 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).
+	 */
+	DRM_ERROR("GuC init failed\n");
+	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
+		ret = -EIO;
+	else
+		ret = 0;
+
+	if (i915.enable_guc_submission) {
+		i915.enable_guc_submission = 0;
+		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
+	}
+
+	i915_ggtt_disable_guc(dev_priv);
+	intel_guc_release_interrupts(dev_priv);
+	i915_guc_submission_disable(dev_priv);
+	i915_guc_submission_fini(dev_priv);
+
+	return ret;
+}
+
 /*
  * Read GuC command/status register (SOFT_SCRATCH_0)
  * Return true if it contains a response rather than a command
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index c5179ef..a9a4188 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -187,6 +187,7 @@ struct intel_huc {
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_uc_init_fw(struct drm_i915_private *dev_priv);
+int intel_uc_init_hw(struct drm_i915_private *dev_priv);
 void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
 			 struct intel_uc_fw *uc_fw);
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
@@ -199,6 +200,8 @@ void intel_guc_fini(struct drm_i915_private *dev_priv);
 const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
 int intel_guc_suspend(struct drm_i915_private *dev_priv);
 int intel_guc_resume(struct drm_i915_private *dev_priv);
+void intel_guc_release_interrupts(struct drm_i915_private *dev_priv);
+void intel_guc_capture_interrupts(struct drm_i915_private *dev_priv);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 /* i915_guc_submission.c */
-- 
2.9.3

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

[PATCH 07/10] drm/i915/guc: Simplify intel_guc_init_hw()

[Arkadiusz Hiler]

Current version of intel_guc_init_hw() does a lot:
 - cares about submission
 - loads huc
 - implement WA

This change offloads some of the logic to intel_uc_init_hw(), which now
cares about the above.

v2: rename guc_hw_reset and fix typo in define name (M. Wajdeczko)
v3: rename once again
v4: remove spurious comments and add some style (J. Lahtinen)
v5: flow changes, got rid of dead checks (M. Wajdeczko)

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |   2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 147 +++-----------------------------
 drivers/gpu/drm/i915/intel_uc.c         | 114 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |   3 +
 4 files changed, 130 insertions(+), 136 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 29c3bba..e91be58 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4454,7 +4454,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 	intel_mocs_init_l3cc_table(dev_priv);
 
 	/* We can't enable contexts until all firmware is loaded */
-	ret = intel_guc_init_hw(dev_priv);
+	ret = intel_uc_init_hw(dev_priv);
 	if (ret)
 		goto out;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index fb518e0..192f1af 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -90,7 +90,7 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 	}
 };
 
-static void guc_interrupts_release(struct drm_i915_private *dev_priv)
+void intel_guc_release_interrupts(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
@@ -108,7 +108,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
 	I915_WRITE(GUC_WD_VECS_IER, 0);
 }
 
-static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
+void intel_guc_capture_interrupts(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
@@ -408,24 +408,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-static int guc_hw_reset(struct drm_i915_private *dev_priv)
-{
-	int ret;
-	u32 guc_status;
-
-	ret = intel_guc_reset(dev_priv);
-	if (ret) {
-		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
-		return ret;
-	}
-
-	guc_status = I915_READ(GUC_STATUS);
-	WARN(!(guc_status & GS_MIA_IN_RESET),
-	     "GuC status: 0x%x, MIA core expected to be in reset\n", guc_status);
-
-	return ret;
-}
-
 /**
  * intel_guc_init_hw() - finish preparing the GuC for activity
  * @dev_priv:	i915 device private
@@ -443,42 +425,22 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 	const char *fw_path = guc_fw->path;
-	int retries, ret, err;
+	int ret;
 
 	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
 		fw_path,
 		intel_uc_fw_status_repr(guc_fw->fetch_status),
 		intel_uc_fw_status_repr(guc_fw->load_status));
 
-	/* Loading forbidden, or no firmware to load? */
-	if (!i915.enable_guc_loading) {
-		err = 0;
-		goto fail;
-	} else if (fw_path == NULL) {
-		/* Device is known to have no uCode (e.g. no GuC) */
-		err = -ENXIO;
-		goto fail;
+	if (!fw_path) {
+		return -ENXIO;
 	} else if (*fw_path == '\0') {
-		/* Device has a GuC but we don't know what f/w to load? */
 		WARN(1, "No GuC firmware known for this platform!\n");
-		err = -ENODEV;
-		goto fail;
+		return -ENODEV;
 	}
 
-	/* Fetch failed, or already fetched but failed to load? */
-	if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) {
-		err = -EIO;
-		goto fail;
-	} else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) {
-		err = -ENOEXEC;
-		goto fail;
-	}
-
-	guc_interrupts_release(dev_priv);
-	gen9_reset_guc_interrupts(dev_priv);
-
-	/* We need to notify the guc whenever we change the GGTT */
-	i915_ggtt_enable_guc(dev_priv);
+	if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+		return -EIO;
 
 	guc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
 
@@ -486,104 +448,19 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 		intel_uc_fw_status_repr(guc_fw->fetch_status),
 		intel_uc_fw_status_repr(guc_fw->load_status));
 
-	err = i915_guc_submission_init(dev_priv);
-	if (err)
-		goto fail;
+	ret = guc_ucode_xfer(dev_priv);
 
-	/*
-	 * WaEnableuKernelHeaderValidFix:skl,bxt
-	 * For BXT, this is only upto B0 but below WA is required for later
-	 * steppings also so this is extended as well.
-	 */
-	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
-	for (retries = 3; ; ) {
-		/*
-		 * Always reset the GuC just before (re)loading, so
-		 * that the state and timing are fairly predictable
-		 */
-		err = guc_hw_reset(dev_priv);
-		if (err)
-			goto fail;
-
-		intel_huc_init_hw(dev_priv);
-		err = guc_ucode_xfer(dev_priv);
-		if (!err)
-			break;
-
-		if (--retries == 0)
-			goto fail;
-
-		DRM_INFO("GuC fw load failed: %d; will reset and "
-			 "retry %d more time(s)\n", err, retries);
-	}
+	if (ret)
+		return -EAGAIN;
 
 	guc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS;
 
-	intel_guc_auth_huc(dev_priv);
-
-	if (i915.enable_guc_submission) {
-		if (i915.guc_log_level >= 0)
-			gen9_enable_guc_interrupts(dev_priv);
-
-		err = i915_guc_submission_enable(dev_priv);
-		if (err)
-			goto fail;
-		guc_interrupts_capture(dev_priv);
-	}
-
 	DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
 		 i915.enable_guc_submission ? "submission enabled" : "loaded",
 		 guc_fw->path,
 		 guc_fw->major_ver_found, guc_fw->minor_ver_found);
 
 	return 0;
-
-fail:
-	if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
-		guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
-
-	guc_interrupts_release(dev_priv);
-	i915_guc_submission_disable(dev_priv);
-	i915_guc_submission_fini(dev_priv);
-	i915_ggtt_disable_guc(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).
-	 */
-	if (i915.enable_guc_loading > 1) {
-		ret = -EIO;
-	} else if (i915.enable_guc_submission > 1) {
-		ret = -EIO;
-	} else {
-		ret = 0;
-	}
-
-	if (err == 0 && !HAS_GUC_UCODE(dev_priv))
-		;	/* Don't mention the GuC! */
-	else if (err == 0)
-		DRM_INFO("GuC firmware load skipped\n");
-	else if (ret != -EIO)
-		DRM_NOTE("GuC firmware load failed: %d\n", err);
-	else
-		DRM_WARN("GuC firmware load failed: %d\n", err);
-
-	if (i915.enable_guc_submission) {
-		if (fw_path == NULL)
-			DRM_INFO("GuC submission without firmware not supported\n");
-		if (ret == 0)
-			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
-		else
-			DRM_ERROR("GuC init failed: %d\n", ret);
-	}
-	i915.enable_guc_submission = 0;
-
-	return ret;
 }
 
 
@@ -642,7 +519,7 @@ void intel_guc_fini(struct drm_i915_private *dev_priv)
 	struct drm_i915_gem_object *obj;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	guc_interrupts_release(dev_priv);
+	intel_guc_release_interrupts(dev_priv);
 	i915_guc_submission_disable(dev_priv);
 	i915_guc_submission_fini(dev_priv);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 7453a648..62a2eac 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -26,6 +26,27 @@
 #include "intel_uc.h"
 #include <linux/firmware.h>
 
+/* Reset GuC providing us with fresh state for both GuC and HuC.
+ */
+static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
+{
+	int ret;
+	u32 guc_status;
+
+	ret = intel_guc_reset(dev_priv);
+	if (ret) {
+		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
+		return ret;
+	}
+
+	guc_status = I915_READ(GUC_STATUS);
+	WARN(!(guc_status & GS_MIA_IN_RESET),
+	     "GuC status: 0x%x, MIA core expected to be in reset\n",
+	     guc_status);
+
+	return ret;
+}
+
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_GUC(dev_priv)) {
@@ -63,6 +84,99 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 	intel_guc_init_fw(&dev_priv->guc);
 }
 
+int intel_uc_init_hw(struct drm_i915_private *dev_priv)
+{
+	int ret, attempts;
+	const int guc_wa_hash_check_not_set_attempts = 3;
+
+
+	/* GuC not enabled, nothing to do */
+	if (!i915.enable_guc_loading)
+		return 0;
+
+	intel_guc_release_interrupts(dev_priv);
+	gen9_reset_guc_interrupts(dev_priv);
+
+	/* We need to notify the guc whenever we change the GGTT */
+	i915_ggtt_enable_guc(dev_priv);
+
+	if (i915.enable_guc_submission) {
+		ret = i915_guc_submission_init(dev_priv);
+		if (ret)
+			goto fail;
+	}
+
+	/* WaEnableuKernelHeaderValidFix:skl */
+	/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
+	if (IS_GEN9(dev_priv))
+		attempts = guc_wa_hash_check_not_set_attempts;
+	else
+		attempts = 1;
+
+	while (attempts--) {
+		/*
+		 * Always reset the GuC just before (re)loading, so
+		 * that the state and timing are fairly predictable
+		 */
+		ret = __intel_uc_reset_hw(dev_priv);
+		if (ret)
+			goto fail;
+
+		intel_huc_init_hw(dev_priv);
+		ret = intel_guc_init_hw(dev_priv);
+		if (ret == 0 || ret != -EAGAIN)
+			break;
+
+		DRM_INFO("GuC fw load failed: %d; will reset and "
+			 "retry %d more time(s)\n", ret, attempts);
+	}
+
+	/* Did we succeded or run out of retries? */
+	if (ret)
+		goto fail;
+
+	intel_guc_auth_huc(dev_priv);
+	if (i915.enable_guc_submission) {
+		if (i915.guc_log_level >= 0)
+			gen9_enable_guc_interrupts(dev_priv);
+
+		ret = i915_guc_submission_enable(dev_priv);
+		if (ret)
+			goto fail;
+		intel_guc_capture_interrupts(dev_priv);
+	}
+
+	return 0;
+
+fail:
+	/*
+	 * 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).
+	 */
+	DRM_ERROR("GuC init failed\n");
+	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
+		ret = -EIO;
+	else
+		ret = 0;
+
+	if (i915.enable_guc_submission) {
+		i915.enable_guc_submission = 0;
+		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
+	}
+
+	i915_ggtt_disable_guc(dev_priv);
+	intel_guc_release_interrupts(dev_priv);
+	i915_guc_submission_disable(dev_priv);
+	i915_guc_submission_fini(dev_priv);
+
+	return ret;
+}
+
 /*
  * Read GuC command/status register (SOFT_SCRATCH_0)
  * Return true if it contains a response rather than a command
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 2ee148c..6f97ded 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -187,6 +187,7 @@ struct intel_huc {
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_uc_init_fw(struct drm_i915_private *dev_priv);
+int intel_uc_init_hw(struct drm_i915_private *dev_priv);
 void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
 			 struct intel_uc_fw *uc_fw);
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
@@ -199,6 +200,8 @@ void intel_guc_fini(struct drm_i915_private *dev_priv);
 const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
 int intel_guc_suspend(struct drm_i915_private *dev_priv);
 int intel_guc_resume(struct drm_i915_private *dev_priv);
+void intel_guc_release_interrupts(struct drm_i915_private *dev_priv);
+void intel_guc_capture_interrupts(struct drm_i915_private *dev_priv);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 /* i915_guc_submission.c */
-- 
2.9.3

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