[PATCH v3 0/8] GuC Scrub vol. 1

[PATCH v3 0/8] GuC Scrub vol. 1

[Arkadiusz Hiler]

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

A lot of logic were extracted from intel_guc_load() to other functions - it not
only did handle the actual loading but had WA implementations and had the code
thatenabled submission baked 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

Arkadiusz Hiler (8):
  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: Rename intel_?uc_init() to intel_?uc_fetch_fw()
  drm/i915/uc: Make intel_uc_fw_fetch() static
  drm/i915/guc: Extract param logic form guc_init
  drm/i915/guc: Simplify intel_guc_init_hw()
  drm/i915/uc: Simplify firwmare path handling

 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/intel_guc_loader.c | 354 ++++----------------------------
 drivers/gpu/drm/i915/intel_huc.c        |  57 +++--
 drivers/gpu/drm/i915/intel_uc.c         | 278 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |  27 ++-
 7 files changed, 368 insertions(+), 360 deletions(-)

-- 
2.9.3

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

[PATCH 1/8] 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>
---
 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 5f7b8c8..fa2ca05 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4499,7 +4499,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 2/8] drm/i915/uc: Drop superfluous externs in intel_uc.h

[Arkadiusz Hiler]

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@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 3/8] 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>
---
 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 7495a16..e17bee4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2508,6 +2508,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 4/8] drm/i915/uc: Rename intel_?uc_init() to intel_?uc_fetch_fw()

[Arkadiusz Hiler]

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

 * `intel_guc` is the subject, so those functions now take intel_guc
   structure, instead of the dev_priv
 * fetch is the verb
 * fw is the object which better describes the function's role

Same change is done for the huc counterpart.

Also we bulk call both functions from higher-level intel_fetch_uc_fw:
 * intel being the subject (taking the dev_priv as param)
 * fetch being the verb
 * uc_fw being the subject

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 43da9cf..45fae97 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_fetch_uc_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 9f09e26..753aeef 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -723,17 +723,17 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_guc_init() - define parameters and fetch firmware
- * @dev_priv:	i915 device private
+ * intel_guc_fetch_fw() - define parameters and fetch firmware
+ * @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_fetch_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)) {
@@ -751,23 +751,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)
@@ -777,9 +777,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_fw_fetch(dev_priv, guc_fw);
+	intel_uc_fw_fetch(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..0725e6f 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_fetch_fw() - initiate HuC firmware loading request
+ * @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_fetch_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_fw_fetch(dev_priv, huc_fw);
+	intel_uc_fw_fetch(dev_priv, &huc->fw);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index c46bc85..a089102 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -30,6 +30,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 	mutex_init(&dev_priv->guc.send_mutex);
 }
 
+void intel_fetch_uc_fw(struct drm_i915_private *dev_priv)
+{
+	intel_huc_fetch_fw(&dev_priv->huc);
+	intel_guc_fetch_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 41b7351..19b8966 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -185,11 +185,12 @@ struct intel_huc {
 
 /* intel_uc.c */
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
+void intel_fetch_uc_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);
 
 /* intel_guc_loader.c */
-void intel_guc_init(struct drm_i915_private *dev_priv);
+void intel_guc_fetch_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);
@@ -223,7 +224,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_fetch_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 5/8] drm/i915/uc: Make intel_uc_fw_fetch() static

[Arkadiusz Hiler]

intel_uc_fw_fetch() is confusingly named in the light of recent changes.

It's also in the worng place - 'guc_loader.h' - it's used for both guc
and huc, which was reflected in name, but not it's location, so let's
move it to 'intel_uc.c'.

We can make a intel_uc_fw callback out of it, to avoid leaking it
outside `intel_uc.c`

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/intel_guc_loader.c | 137 +------------------------------
 drivers/gpu/drm/i915/intel_huc.c        |   2 +-
 drivers/gpu/drm/i915/intel_uc.c         | 141 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |   4 +-
 4 files changed, 145 insertions(+), 139 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 753aeef..110dfd1 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_fetch_fw() - define parameters and fetch firmware
@@ -779,7 +644,7 @@ void intel_guc_fetch_fw(struct intel_guc *guc)
 
 	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);
+	guc->fw.fetch(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 0725e6f..7527988 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -186,7 +186,7 @@ void intel_huc_fetch_fw(struct intel_huc *huc)
 
 	WARN(huc->fw.path == NULL, "HuC present but no fw path\n");
 
-	intel_uc_fw_fetch(dev_priv, &huc->fw);
+	huc->fw.fetch(dev_priv, &huc->fw);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index a089102..d2d2b6c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -24,6 +24,10 @@
 
 #include "i915_drv.h"
 #include "intel_uc.h"
+#include <linux/firmware.h>
+
+static void uc_fetch_fw(struct drm_i915_private *dev_priv,
+			  struct intel_uc_fw *uc_fw);
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
@@ -32,7 +36,10 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 
 void intel_fetch_uc_fw(struct drm_i915_private *dev_priv)
 {
+	dev_priv->huc.fw.fetch = uc_fetch_fw;
 	intel_huc_fetch_fw(&dev_priv->huc);
+
+	dev_priv->guc.fw.fetch = uc_fetch_fw;
 	intel_guc_fetch_fw(&dev_priv->guc);
 }
 
@@ -120,3 +127,137 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
+static void uc_fetch_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 19b8966..c390663 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -110,6 +110,8 @@ enum intel_uc_fw_type {
  * of fetching, caching, and loading the firmware image into the GuC.
  */
 struct intel_uc_fw {
+	void (*fetch)(struct drm_i915_private *dev_priv,
+		      struct intel_uc_fw *uc_fw);
 	const char *path;
 	size_t size;
 	struct drm_i915_gem_object *obj;
@@ -196,8 +198,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

[PATCH 6/8] drm/i915/guc: Extract param logic form guc_init

[Arkadiusz Hiler]

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

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

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

v2: fix comment, notify when nuking GuC explicitly enabled (M. Wajdeczko)

Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@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 | 16 +---------------
 drivers/gpu/drm/i915/intel_uc.c         | 27 ++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_uc.h         |  1 +
 4 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 45fae97..687f7c0 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 sempahores? %s\n", yesno(i915.semaphores));
+
+	intel_uc_sanitize_params(dev_priv);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 110dfd1..e74c127 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -588,7 +588,7 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 
 
 /**
- * intel_guc_fetch_fw() - define parameters and fetch firmware
+ * intel_guc_fetch_fw() - determine and fetch firmware
  * @guc:	intel_guc struct
  *
  * Called early during driver load, but after GEM is initialised.
@@ -601,17 +601,6 @@ void intel_guc_fetch_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_fetch_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 d2d2b6c..ef9dc72 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -29,6 +29,27 @@
 static void uc_fetch_fw(struct drm_i915_private *dev_priv,
 			  struct intel_uc_fw *uc_fw);
 
+void intel_uc_sanitize_params(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_GUC(dev_priv)) {
+		if (i915.enable_guc_loading > 0)
+			DRM_INFO("Disabling GuC, 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);
@@ -36,8 +57,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 
 void intel_fetch_uc_fw(struct drm_i915_private *dev_priv)
 {
+	if (!i915.enable_guc_loading)
+		return;
+
 	dev_priv->huc.fw.fetch = uc_fetch_fw;
-	intel_huc_fetch_fw(&dev_priv->huc);
+	if (HAS_HUC_UCODE(dev_priv))
+		intel_huc_fetch_fw(&dev_priv->huc);
 
 	dev_priv->guc.fw.fetch = uc_fetch_fw;
 	intel_guc_fetch_fw(&dev_priv->guc);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index c390663..36653f3 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -186,6 +186,7 @@ struct intel_huc {
 };
 
 /* intel_uc.c */
+void intel_uc_sanitize_params(struct drm_i915_private *dev_priv);
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_fetch_uc_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 7/8] 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_load(), which now
cares about the above.

v2: rename guc_hw_reset and fix typo in define name (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>
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 | 136 +++-----------------------------
 drivers/gpu/drm/i915/intel_uc.c         | 105 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |   5 ++
 4 files changed, 123 insertions(+), 125 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fa2ca05..c3147f5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4499,7 +4499,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 e74c127..549a254 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,147 +425,53 @@ 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) {
+	if (fw_path == NULL) {
 		/* Device is known to have no uCode (e.g. no GuC) */
-		err = -ENXIO;
-		goto fail;
+		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;
+		return -EIO;
 	} else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) {
-		err = -ENOEXEC;
-		goto fail;
+		return -ENOEXEC;
 	}
 
-	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);
-
 	guc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
 
 	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
 		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 +532,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 ef9dc72..2bb49b7 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -29,6 +29,25 @@
 static void uc_fetch_fw(struct drm_i915_private *dev_priv,
 			  struct intel_uc_fw *uc_fw);
 
+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;
+}
+
 void intel_uc_sanitize_params(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_GUC(dev_priv)) {
@@ -68,6 +87,92 @@ void intel_fetch_uc_fw(struct drm_i915_private *dev_priv)
 	intel_guc_fetch_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 = guc_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
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 36653f3..99402c3 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -29,6 +29,8 @@
 #include "intel_ringbuffer.h"
 
 #include "i915_vma.h"
+/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
+#define GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS 3
 
 struct drm_i915_gem_request;
 
@@ -189,6 +191,7 @@ struct intel_huc {
 void intel_uc_sanitize_params(struct drm_i915_private *dev_priv);
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_fetch_uc_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);
 
@@ -199,6 +202,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_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 8/8] drm/i915/uc: Simplify firwmare 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)

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>
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         |  5 +++--
 3 files changed, 22 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 549a254..aade185 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -433,12 +433,8 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 		intel_uc_fw_status_repr(guc_fw->load_status));
 
 	if (fw_path == NULL) {
-		/* Device is known to have no uCode (e.g. no GuC) */
+		/* We do not have uCode for the device */
 		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");
-		return -ENODEV;
 	}
 
 	/* Fetch failed, or already fetched but failed to load? */
@@ -474,7 +470,6 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
-
 /**
  * intel_guc_fetch_fw() - determine and fetch firmware
  * @guc:	intel_guc struct
@@ -487,39 +482,31 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 void intel_guc_fetch_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 */
+		WARN(1, "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);
 	guc->fw.fetch(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 7527988..c94e22f 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_fetch_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 {
+		WARN(1, "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");
-
 	huc->fw.fetch(dev_priv, &huc->fw);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 2bb49b7..6f2d3f6 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -267,8 +267,9 @@ static void uc_fetch_fw(struct drm_i915_private *dev_priv,
 	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));
+	uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
+
+	DRM_DEBUG_DRIVER("uC firmware pending, path %s\n", uc_fw->path);
 
 	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

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

[Michal Wajdeczko]

On Fri, Feb 17, 2017 at 02:05:50PM +0100, Arkadiusz Hiler wrote:
> 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>

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

Re: [PATCH 2/8] drm/i915/uc: Drop superfluous externs in intel_uc.h

[Michal Wajdeczko]

On Fri, Feb 17, 2017 at 02:05:51PM +0100, Arkadiusz Hiler wrote:

I think one line with description will not kill us ;)

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

> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@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

Re: [PATCH 3/8] drm/i915/huc: Add huc_to_i915

[Michal Wajdeczko]

On Fri, Feb 17, 2017 at 02:05:52PM +0100, Arkadiusz Hiler wrote:
> 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>

-Michal

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

Re: [PATCH 4/8] drm/i915/uc: Rename intel_?uc_init() to intel_?uc_fetch_fw()

[Michal Wajdeczko]

On Fri, Feb 17, 2017 at 02:05:53PM +0100, Arkadiusz Hiler wrote:
> Trying to have subject_verb_object ordering and more descriptive names,
> the intel_huc_init() and intel_guc_init() functions are renamed:
> 
>  * `intel_guc` is the subject, so those functions now take intel_guc
>    structure, instead of the dev_priv
>  * fetch is the verb
>  * fw is the object which better describes the function's role
> 
> Same change is done for the huc counterpart.
> 
> Also we bulk call both functions from higher-level intel_fetch_uc_fw:
>  * intel being the subject (taking the dev_priv as param)
>  * fetch being the verb
>  * uc_fw being the subject
> 
> 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>
> ---

<SNIP>

> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 41b7351..19b8966 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -185,11 +185,12 @@ struct intel_huc {
>  
>  /* intel_uc.c */
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
> +void intel_fetch_uc_fw(struct drm_i915_private *dev_priv);

Hmm, names of these two functions above are inconsistent now.
Maybe they both should start with i915 as they take dev_priv:

	void i915_uc_init_early(struct drm_i915_private *dev_priv);
	void i915_uc_fetch_fw(struct drm_i915_private *dev_priv);

or treat intel_uc as a subject:

	void intel_uc_init_early(struct drm_i915_private *dev_priv);
	void intel_uc_fetch_fw(struct drm_i915_private *dev_priv);


-Michal 

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

Re: [PATCH 5/8] drm/i915/uc: Make intel_uc_fw_fetch() static

[Michal Wajdeczko]

On Fri, Feb 17, 2017 at 02:05:54PM +0100, Arkadiusz Hiler wrote:
> intel_uc_fw_fetch() is confusingly named in the light of recent changes.
> 
> It's also in the worng place - 'guc_loader.h' - it's used for both guc

Typo s/worng/wrong

> and huc, which was reflected in name, but not it's location, so let's
> move it to 'intel_uc.c'.
> 
> We can make a intel_uc_fw callback out of it, to avoid leaking it
> outside `intel_uc.c`

Hmm, why do you think it is a problem to expose this function outside of intel_uc.c?
I can't see any real gain, rather unnecessary code complexity

-Michal

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

Re: [PATCH 6/8] drm/i915/guc: Extract param logic form guc_init

[Michal Wajdeczko]

On Fri, Feb 17, 2017 at 02:05:55PM +0100, Arkadiusz Hiler wrote:
> Let intel_guc_fetch_fw() focus on determining and fetching the correct
> firmware.
> 
> This patch introduces intel_sanitize_uc_params() that is called from
> intel_sanitize_options().

Function name mentioned here does not match function in the patch.
Also, can we use "options" to match parent function name?


> 
> Then, if we have GuC, we can call intel_guc_fetch_fw() conditionally and
> we do not have to do the internal checks.
> 
> v2: fix comment, notify when nuking GuC explicitly enabled (M. Wajdeczko)
> 
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@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 | 16 +---------------
>  drivers/gpu/drm/i915/intel_uc.c         | 27 ++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_uc.h         |  1 +
>  4 files changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 45fae97..687f7c0 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 sempahores? %s\n", yesno(i915.semaphores));
> +
> +	intel_uc_sanitize_params(dev_priv);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 110dfd1..e74c127 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -588,7 +588,7 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
>  
>  
>  /**
> - * intel_guc_fetch_fw() - define parameters and fetch firmware
> + * intel_guc_fetch_fw() - determine and fetch firmware
>   * @guc:	intel_guc struct
>   *
>   * Called early during driver load, but after GEM is initialised.
> @@ -601,17 +601,6 @@ void intel_guc_fetch_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_fetch_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 d2d2b6c..ef9dc72 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -29,6 +29,27 @@
>  static void uc_fetch_fw(struct drm_i915_private *dev_priv,
>  			  struct intel_uc_fw *uc_fw);
>  
> +void intel_uc_sanitize_params(struct drm_i915_private *dev_priv)
> +{
> +	if (!HAS_GUC(dev_priv)) {
> +		if (i915.enable_guc_loading > 0)
> +			DRM_INFO("Disabling GuC, no hardware");

Hmm, I'm not sure you can disable something that does not exist...
Maybe message should be like this "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;

This extra condition should be moved into "else" above.

> +}
> +
>  void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  {
>  	mutex_init(&dev_priv->guc.send_mutex);
> @@ -36,8 +57,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  
>  void intel_fetch_uc_fw(struct drm_i915_private *dev_priv)
>  {
> +	if (!i915.enable_guc_loading)
> +		return;
> +
>  	dev_priv->huc.fw.fetch = uc_fetch_fw;
> -	intel_huc_fetch_fw(&dev_priv->huc);
> +	if (HAS_HUC_UCODE(dev_priv))
> +		intel_huc_fetch_fw(&dev_priv->huc);

I'm not sure how it goes after this series, but previously HAS_HUC check
was done inside huc_fetch_fw() function...


-Michal

>  
>  	dev_priv->guc.fw.fetch = uc_fetch_fw;
>  	intel_guc_fetch_fw(&dev_priv->guc);
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index c390663..36653f3 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -186,6 +186,7 @@ struct intel_huc {
>  };
>  
>  /* intel_uc.c */
> +void intel_uc_sanitize_params(struct drm_i915_private *dev_priv);
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
>  void intel_fetch_uc_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 7/8] drm/i915/guc: Simplify intel_guc_init_hw()

[Michal Wajdeczko]

On Fri, Feb 17, 2017 at 02:05:56PM +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_load(), which now
> cares about the above.
> 
> v2: rename guc_hw_reset and fix typo in define name (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>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---

<SNIP>

> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index ef9dc72..2bb49b7 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -29,6 +29,25 @@
>  static void uc_fetch_fw(struct drm_i915_private *dev_priv,
>  			  struct intel_uc_fw *uc_fw);
>  
> +static int guc_hw_reset(struct drm_i915_private *dev_priv)

This function looks like and is guc specific.
Maybe we should revisit the idea of having separate intel_guc.c file
that will hold only guc specific functions. And use intel_uc.c file
only for functions that are common across Guc, Huc...

-Michal

> +{
> +	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_params(struct drm_i915_private *dev_priv)
>  {
>  	if (!HAS_GUC(dev_priv)) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/8] drm/i915/uc: Simplify firwmare path handling

[Michal Wajdeczko]

On Fri, Feb 17, 2017 at 02:05:57PM +0100, Arkadiusz Hiler wrote:

Typo in subject s/firwmare/firmware


> 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)
> 
> 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>
> 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         |  5 +++--
>  3 files changed, 22 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 549a254..aade185 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -433,12 +433,8 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
>  		intel_uc_fw_status_repr(guc_fw->load_status));
>  
>  	if (fw_path == NULL) {
> -		/* Device is known to have no uCode (e.g. no GuC) */
> +		/* We do not have uCode for the device */
>  		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");
> -		return -ENODEV;
>  	}
>  
>  	/* Fetch failed, or already fetched but failed to load? */
> @@ -474,7 +470,6 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
>  	return 0;
>  }
>  
> -
>  /**
>   * intel_guc_fetch_fw() - determine and fetch firmware
>   * @guc:	intel_guc struct
> @@ -487,39 +482,31 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
>  void intel_guc_fetch_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;

Maybe for above code we can add new function:

	void intel_uc_fw_init_early(struct intel_uc_fw *fw,
	                            enum intel_uc_fw_type type);

and use it for both guc and huc:
	
	intel_uc_fw_init_early(&guc->fw, INTEL_UC_FW_TYPE_GUC);
	intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC);

> +
> +	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 */
> +		WARN(1, "No GuC firmware known for platform with GuC!\n");

Maybe simpler DRM_ERROR will be sufficient? We don't need callstack.


> +		i915.enable_guc_loading = 0;

What about making this firmware path guess work part of the early init or
sanitize options function? Note that actual fetch is already done by in 
different function, so mostly we just need to pick nice name for the
new function. Maybe

	int intel_guc_init_fw() ?

Note that changing i915.enable_guc param here has implication on other
actions (like Huc loading) and thus forcing redundant checks elsewhere

> +		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);
>  	guc->fw.fetch(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 7527988..c94e22f 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_fetch_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 {
> +		WARN(1, "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");
> -
>  	huc->fw.fetch(dev_priv, &huc->fw);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 2bb49b7..6f2d3f6 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -267,8 +267,9 @@ static void uc_fetch_fw(struct drm_i915_private *dev_priv,
>  	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));
> +	uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
> +
> +	DRM_DEBUG_DRIVER("uC firmware pending, path %s\n", uc_fw->path);

If you are not planning to remove "intel_uc_fw_status_repr()" then please it.

-Michal

>  
>  	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

✓ Fi.CI.BAT: success for GuC Scrub vol. 1 (rev3)

[Patchwork]

== Series Details ==

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

== Summary ==

Series 16856v3 GuC Scrub vol. 1
https://patchwork.freedesktop.org/api/1.0/series/16856/revisions/3/mbox/

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

2b7ce9512d9770350bc2a59652cc7bf469bc544a drm-tip: 2017y-02m-17d-12h-20m-31s UTC integration manifest
24f57ae drm/i915/uc: Simplify firwmare path handling
592aa50c drm/i915/guc: Simplify intel_guc_init_hw()
f8dd825 drm/i915/guc: Extract param logic form guc_init
ed62ebf drm/i915/uc: Make intel_uc_fw_fetch() static
a29ea96 drm/i915/uc: Rename intel_?uc_init() to intel_?uc_fetch_fw()
4433687 drm/i915/huc: Add huc_to_i915
6d6c305 drm/i915/uc: Drop superfluous externs in intel_uc.h
2d116de drm/i915/uc: Rename intel_?uc_{setup, load}() to _init_hw()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3879/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915/uc: Drop superfluous externs in intel_uc.h

[Arkadiusz Hiler]

On Fri, Feb 17, 2017 at 02:13:06PM +0100, Michal Wajdeczko wrote:
> On Fri, Feb 17, 2017 at 02:05:51PM +0100, Arkadiusz Hiler wrote:
> 
> I think one line with description will not kill us ;)

Comment title is exactly that. If you have something that won't be just
simple restatement of the title / saying what this trivial diff says
then please share.

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

Thanks.

> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@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
> > 

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

Re: [PATCH 4/8] drm/i915/uc: Rename intel_?uc_init() to intel_?uc_fetch_fw()

[Arkadiusz Hiler]

On Fri, Feb 17, 2017 at 02:31:09PM +0100, Michal Wajdeczko wrote:
> On Fri, Feb 17, 2017 at 02:05:53PM +0100, Arkadiusz Hiler wrote:
> > Trying to have subject_verb_object ordering and more descriptive names,
> > the intel_huc_init() and intel_guc_init() functions are renamed:
> > 
> >  * `intel_guc` is the subject, so those functions now take intel_guc
> >    structure, instead of the dev_priv
> >  * fetch is the verb
> >  * fw is the object which better describes the function's role
> > 
> > Same change is done for the huc counterpart.
> > 
> > Also we bulk call both functions from higher-level intel_fetch_uc_fw:
> >  * intel being the subject (taking the dev_priv as param)
> >  * fetch being the verb
> >  * uc_fw being the subject
> > 
> > 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>
> > ---
> 
> <SNIP>
> 
> > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > index 41b7351..19b8966 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -185,11 +185,12 @@ struct intel_huc {
> >  
> >  /* intel_uc.c */
> >  void intel_uc_init_early(struct drm_i915_private *dev_priv);
> > +void intel_fetch_uc_fw(struct drm_i915_private *dev_priv);
> 
> Hmm, names of these two functions above are inconsistent now.
> Maybe they both should start with i915 as they take dev_priv:
> 
> 	void i915_uc_init_early(struct drm_i915_private *dev_priv);
> 	void i915_uc_fetch_fw(struct drm_i915_private *dev_priv);

I had similar idea. I've asked on IRC on friday and general tendency is
to use intel_ for driver internals and i915_ for anything user farcing.

So, sadly, this is not a solution.

> or treat intel_uc as a subject:
> 
> 	void intel_uc_init_early(struct drm_i915_private *dev_priv);
> 	void intel_uc_fetch_fw(struct drm_i915_private *dev_priv);

It's still not quite there as it suggests that it acts on struct
intel_uc, but I'll go with it.

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

Re: [PATCH 5/8] drm/i915/uc: Make intel_uc_fw_fetch() static

[Arkadiusz Hiler]

On Fri, Feb 17, 2017 at 02:39:35PM +0100, Michal Wajdeczko wrote:
> On Fri, Feb 17, 2017 at 02:05:54PM +0100, Arkadiusz Hiler wrote:
> > intel_uc_fw_fetch() is confusingly named in the light of recent changes.
> > 
> > It's also in the worng place - 'guc_loader.h' - it's used for both guc
> 
> Typo s/worng/wrong

Done.

> > and huc, which was reflected in name, but not it's location, so let's
> > move it to 'intel_uc.c'.
> > 
> > We can make a intel_uc_fw callback out of it, to avoid leaking it
> > outside `intel_uc.c`
> 
> Hmm, why do you think it is a problem to expose this function outside of intel_uc.c?
> I can't see any real gain, rather unnecessary code complexity

I was trying to figure out a good name for it (to not confuse it with
intel_uc_fw_fetch) and maybe prefix it with __ to denote that nobody
should really bother with it, but that's reserved for statics.

I brought the topic on #intel-gfx and Joonas suggested this approach.

That seems to be general trend with i915.

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

Re: [PATCH 6/8] drm/i915/guc: Extract param logic form guc_init

[Arkadiusz Hiler]

On Fri, Feb 17, 2017 at 02:52:00PM +0100, Michal Wajdeczko wrote:
> On Fri, Feb 17, 2017 at 02:05:55PM +0100, Arkadiusz Hiler wrote:
> > Let intel_guc_fetch_fw() focus on determining and fetching the correct
> > firmware.
> > 
> > This patch introduces intel_sanitize_uc_params() that is called from
> > intel_sanitize_options().
> 
> Function name mentioned here does not match function in the patch.
> Also, can we use "options" to match parent function name?
> 
> 
> > 
> > Then, if we have GuC, we can call intel_guc_fetch_fw() conditionally and
> > we do not have to do the internal checks.
> > 
> > v2: fix comment, notify when nuking GuC explicitly enabled (M. Wajdeczko)
> > 
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@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 | 16 +---------------
> >  drivers/gpu/drm/i915/intel_uc.c         | 27 ++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_uc.h         |  1 +
> >  4 files changed, 30 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index d2d2b6c..ef9dc72 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -36,8 +57,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> >  
> >  void intel_fetch_uc_fw(struct drm_i915_private *dev_priv)
> >  {
> > +	if (!i915.enable_guc_loading)
> > +		return;
> > +
> >  	dev_priv->huc.fw.fetch = uc_fetch_fw;
> > -	intel_huc_fetch_fw(&dev_priv->huc);
> > +	if (HAS_HUC_UCODE(dev_priv))
> > +		intel_huc_fetch_fw(&dev_priv->huc);
> 
> I'm not sure how it goes after this series, but previously HAS_HUC check
> was done inside huc_fetch_fw() function...

The check in huc_fetch_fw() is removed in later patch. The one that is
cleaning both ?uc_fetch_fw() functions.

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

Re: [PATCH 7/8] drm/i915/guc: Simplify intel_guc_init_hw()

[Arkadiusz Hiler]

On Fri, Feb 17, 2017 at 03:03:04PM +0100, Michal Wajdeczko wrote:
> On Fri, Feb 17, 2017 at 02:05:56PM +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_load(), which now
> > cares about the above.
> > 
> > v2: rename guc_hw_reset and fix typo in define name (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>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > ---
> 
> <SNIP>
> 
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index ef9dc72..2bb49b7 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -29,6 +29,25 @@
> >  static void uc_fetch_fw(struct drm_i915_private *dev_priv,
> >  			  struct intel_uc_fw *uc_fw);
> >  
> > +static int guc_hw_reset(struct drm_i915_private *dev_priv)
> 
> This function looks like and is guc specific.

It seems to restarts both guc and huc hardware (including memory)
nulling the state of both.

I've inlined intel_guc_reset() (as it's the only place it's used) and
named it __uc_reset_hw() for next revision.

> Maybe we should revisit the idea of having separate intel_guc.c file
> that will hold only guc specific functions. And use intel_uc.c file
> only for functions that are common across Guc, Huc...

That's what intel_guc_loader.c pretty much is nowadays. It can be
renamed along the road.

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

Re: [PATCH 7/8] drm/i915/guc: Simplify intel_guc_init_hw()

[Arkadiusz Hiler]

On Tue, Feb 21, 2017 at 02:56:46PM +0100, Arkadiusz Hiler wrote:
> On Fri, Feb 17, 2017 at 03:03:04PM +0100, Michal Wajdeczko wrote:
> > On Fri, Feb 17, 2017 at 02:05:56PM +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_load(), which now
> > > cares about the above.
> > > 
> > > v2: rename guc_hw_reset and fix typo in define name (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>
> > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > > ---
> > 
> > <SNIP>
> > 
> > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > > index ef9dc72..2bb49b7 100644
> > > --- a/drivers/gpu/drm/i915/intel_uc.c
> > > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > > @@ -29,6 +29,25 @@
> > >  static void uc_fetch_fw(struct drm_i915_private *dev_priv,
> > >  			  struct intel_uc_fw *uc_fw);
> > >  
> > > +static int guc_hw_reset(struct drm_i915_private *dev_priv)
> > 
> > This function looks like and is guc specific.
> 
> It seems to restarts both guc and huc hardware (including memory)
> nulling the state of both.
> 
> I've inlined intel_guc_reset() (as it's the only place it's used) and
> named it __uc_reset_hw() for next revision.

I am not inlining intel_guc_reset() as it uses something native and
static to uncore. Other than that it still stands.

> > Maybe we should revisit the idea of having separate intel_guc.c file
> > that will hold only guc specific functions. And use intel_uc.c file
> > only for functions that are common across Guc, Huc...
> 
> That's what intel_guc_loader.c pretty much is nowadays. It can be
> renamed along the road.
> 
> -- 
> Cheers,
> Arek
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 8/8] drm/i915/uc: Simplify firwmare path handling

[Arkadiusz Hiler]

On Fri, Feb 17, 2017 at 03:29:17PM +0100, Michal Wajdeczko wrote:
> On Fri, Feb 17, 2017 at 02:05:57PM +0100, Arkadiusz Hiler wrote:
> 
> Typo in subject s/firwmare/firmware
> 
> 
> > 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)
> > 
> > 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>
> > 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         |  5 +++--
> >  3 files changed, 22 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index 549a254..aade185 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -433,12 +433,8 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
> >  		intel_uc_fw_status_repr(guc_fw->load_status));
> >  
> >  	if (fw_path == NULL) {
> > -		/* Device is known to have no uCode (e.g. no GuC) */
> > +		/* We do not have uCode for the device */
> >  		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");
> > -		return -ENODEV;
> >  	}
> >  
> >  	/* Fetch failed, or already fetched but failed to load? */
> > @@ -474,7 +470,6 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
> >  	return 0;
> >  }
> >  
> > -
> >  /**
> >   * intel_guc_fetch_fw() - determine and fetch firmware
> >   * @guc:	intel_guc struct
> > @@ -487,39 +482,31 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
> >  void intel_guc_fetch_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;
> 
> Maybe for above code we can add new function:
> 
> 	void intel_uc_fw_init_early(struct intel_uc_fw *fw,
> 	                            enum intel_uc_fw_type type);
> 
> and use it for both guc and huc:
> 	
> 	intel_uc_fw_init_early(&guc->fw, INTEL_UC_FW_TYPE_GUC);
> 	intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC);

That's something I want to get to, but this serries is already getting
pretty swollen and this would mean yet another patch in the series, yet
another round of review

Let stick not only to digestible patches but digestible series as well.

> > +
> > +	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 */
> > +		WARN(1, "No GuC firmware known for platform with GuC!\n");
> 
> Maybe simpler DRM_ERROR will be sufficient? We don't need callstack.

I just stuck to what was already used, but DRM_ERROR should be
sufficient.

> > +		i915.enable_guc_loading = 0;
> 
> What about making this firmware path guess work part of the early init or
> sanitize options function? Note that actual fetch is already done by in 
> different function, so mostly we just need to pick nice name for the
> new function. Maybe
> 
> 	int intel_guc_init_fw() ?
> 
> Note that changing i915.enable_guc param here has implication on other
> actions (like Huc loading) and thus forcing redundant checks elsewhere

Same point as with intel_uc_fw_init_early. On my TODO for vol. 2.

> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index 2bb49b7..6f2d3f6 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -267,8 +267,9 @@ static void uc_fetch_fw(struct drm_i915_private *dev_priv,
> >  	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));
> > +	uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
> > +
> > +	DRM_DEBUG_DRIVER("uC firmware pending, path %s\n", uc_fw->path);
> 
> If you are not planning to remove "intel_uc_fw_status_repr()" then please it.

What?

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

Re: [PATCH 6/8] drm/i915/guc: Extract param logic form guc_init

[Joonas Lahtinen]

On ke, 2017-02-22 at 13:41 +0100, Arkadiusz Hiler wrote:
> Let intel_guc_fetch_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_fetch_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)
> 
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

Ah, you already had this patch :)

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

One note below.

<SNIP>

> @@ -36,8 +57,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  
>  void intel_uc_fetch_fw(struct drm_i915_private *dev_priv)
>  {
> +	if (!i915.enable_guc_loading)
> +		return;
> +
>  	dev_priv->huc.fw.fetch = uc_fetch_fw;
> -	intel_huc_fetch_fw(&dev_priv->huc);
> +	if (HAS_HUC_UCODE(dev_priv))
> +		intel_huc_fetch_fw(&dev_priv->huc);

With the addition of select_fw, the check would become

if (dev_priv->huc->fw.path)

to allow specifying the firmware from command line.

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 6/8] drm/i915/guc: Extract param logic form guc_init

[Arkadiusz Hiler]

Let intel_guc_fetch_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_fetch_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)

Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@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 | 16 +---------------
 drivers/gpu/drm/i915/intel_uc.c         | 27 ++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_uc.h         |  1 +
 4 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 078ac3e..2559ada 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 110dfd1..e74c127 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -588,7 +588,7 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 
 
 /**
- * intel_guc_fetch_fw() - define parameters and fetch firmware
+ * intel_guc_fetch_fw() - determine and fetch firmware
  * @guc:	intel_guc struct
  *
  * Called early during driver load, but after GEM is initialised.
@@ -601,17 +601,6 @@ void intel_guc_fetch_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_fetch_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 222a928..9355907 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -29,6 +29,27 @@
 static void uc_fetch_fw(struct drm_i915_private *dev_priv,
 			  struct intel_uc_fw *uc_fw);
 
+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);
@@ -36,8 +57,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 
 void intel_uc_fetch_fw(struct drm_i915_private *dev_priv)
 {
+	if (!i915.enable_guc_loading)
+		return;
+
 	dev_priv->huc.fw.fetch = uc_fetch_fw;
-	intel_huc_fetch_fw(&dev_priv->huc);
+	if (HAS_HUC_UCODE(dev_priv))
+		intel_huc_fetch_fw(&dev_priv->huc);
 
 	dev_priv->guc.fw.fetch = uc_fetch_fw;
 	intel_guc_fetch_fw(&dev_priv->guc);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index b9d2230..cf5d0f4 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -186,6 +186,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_fetch_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