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

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

[Arkadiusz Hiler]

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

A lot of logic were extracted from intel_guc_load() to other functions - it 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
v4: some naming improvements, some bikeshedding


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 firmware 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         | 280 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |  27 ++-
 7 files changed, 370 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>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |  2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 14 +++++++-------
 drivers/gpu/drm/i915/intel_huc.c        |  6 +++---
 drivers/gpu/drm/i915/intel_uc.h         |  4 ++--
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d930328..bbe2df9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4512,7 +4512,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]

Externs are implicit and we generally try to avoid them.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 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 2/8] drm/i915/uc: Drop superfluous externs in intel_uc.h

[Joonas Lahtinen]

On ke, 2017-02-22 at 13:41 +0100, Arkadiusz Hiler wrote:
> Externs are implicit and we generally try to avoid them.
> 
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

Yeah, for actual "extern" there's EXPORT_SYMBOL (which is not needed
here).

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

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

[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>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4e7046d..7366ada 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2528,6 +2528,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_uc_fetch_fw:
 * intel being the subject (taking the dev_priv as param)
 * fetch being the verb
 * uc_fw being the subject

v2: settle on intel_uc_fetch_fw name (M. Wajdeczko)

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b76e8f7..078ac3e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -609,8 +609,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_irq;
 
-	intel_huc_init(dev_priv);
-	intel_guc_init(dev_priv);
+	intel_uc_fetch_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..5bb9149 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_uc_fetch_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..db596b6 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_uc_fetch_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

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

[Joonas Lahtinen]

On ke, 2017-02-22 at 13:41 +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_uc_fetch_fw:
>  * intel being the subject (taking the dev_priv as param)
>  * fetch being the verb
>  * uc_fw being the subject
> 
> v2: settle on intel_uc_fetch_fw name (M. Wajdeczko)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

<SNIP>

> @@ -609,8 +609,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	if (ret)
>  		goto cleanup_irq;
>  
> -	intel_huc_init(dev_priv);
> -	intel_guc_init(dev_priv);
> +	intel_uc_fetch_fw(dev_priv);

intel_uc_init fits this context. (See below)

>  /**
> - * 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.
>   */

"define parameters" is little vague, maybe "select and fetch firmware"?

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

This parameter dance needs to be moved away from guc_fetch_fw function,
into intel_sanitize_options (I'm pretty sure I've mentioned this
earlier).

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

Just get rid of fw_path variable and assign directly, also hoist the
warning to the else branch, which can then do "return;"
 
> +	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
> +	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;

Hoist this assignment before the if block, so no need to special for
the early return from else branch.

<SNIP>
 
>  /**
> - * intel_huc_init() - initiate HuC firmware loading request
> - * @dev_priv: the drm_i915_private device
> + * intel_huc_fetch_fw() - initiate HuC firmware loading request

Correct this commit too to be more descriptive.

> + * @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;

Similarly arrange to get rid of fw_path here.

<SNIP>

> @@ -30,6 +30,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  	mutex_init(&dev_priv->guc.send_mutex);
>  }
>  
> +void intel_uc_fetch_fw(struct drm_i915_private *dev_priv)

This function might be worth calling intel_uc_init (See above), if the
need comes to add other stuff. But either way.

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

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

[Arkadiusz Hiler]

On Wed, Feb 22, 2017 at 03:59:01PM +0200, Joonas Lahtinen wrote:
> On ke, 2017-02-22 at 13:41 +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_uc_fetch_fw:
> >  * intel being the subject (taking the dev_priv as param)
> >  * fetch being the verb
> >  * uc_fw being the subject
> > 
> > v2: settle on intel_uc_fetch_fw name (M. Wajdeczko)
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> 
> <SNIP>
> 
> > @@ -609,8 +609,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >  	if (ret)
> >  		goto cleanup_irq;
> >  
> > -	intel_huc_init(dev_priv);
> > -	intel_guc_init(dev_priv);
> > +	intel_uc_fetch_fw(dev_priv);
> 
> intel_uc_init fits this context. (See below)

Answer below.

> 
> >  /**
> > - * 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.
> >   */
> 
> "define parameters" is little vague, maybe "select and fetch firmware"?

I like those verbs. Let start using it through the whole thing!

> 
> > -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)) {
> 
> This parameter dance needs to be moved away from guc_fetch_fw function,
> into intel_sanitize_options (I'm pretty sure I've mentioned this
> earlier).

This is removed in patch 8, as the fetch_fw is called conditionally.

> > @@ -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;
> 
> Just get rid of fw_path variable and assign directly, also hoist the
> warning to the else branch, which can then do "return;"

This is done done in patch 8.

> > +	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
> > +	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
> 
> Hoist this assignment before the if block, so no need to special for
> the early return from else branch.

This is done done in patch 8.

> <SNIP>
> 
> >  /**
> > - * intel_huc_init() - initiate HuC firmware loading request
> > - * @dev_priv: the drm_i915_private device
> > + * intel_huc_fetch_fw() - initiate HuC firmware loading request
> 
> Correct this commit too to be more descriptive.

Okay.

> > + * @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;
> 
> Similarly arrange to get rid of fw_path here.

Patch 8 in the series addresses that issue as well. Maybe I should move
them around?

> <SNIP>
> 
> > @@ -30,6 +30,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> >  	mutex_init(&dev_priv->guc.send_mutex);
> >  }
> >  
> > +void intel_uc_fetch_fw(struct drm_i915_private *dev_priv)
> 
> This function might be worth calling intel_uc_init (See above), if the
> need comes to add other stuff. But either way.

This is quite confusing now. I was fine it being named init, someone
suggested to be more descriptive with the name, as it is not general
enough to be "init". Seemed reasonable enough for me, so I incorporated
that in the respin.

This is turning into some heavy bikeshedding now...

I agree that it's more than fetch, it actually selects + fetches +
populates the structures.

I'll gladly ignore previous feedback on being to vague with name and
just go with init, but let give the _fw postfix one last chance:


intel_guc_init_fw {
    intel_guc_select_fw
    if (NULL != guc.fw.path)
	    intel_uc_prepare_fw
}

Where select does what the guc's fetch fw does sans the uc_fetch call.

Also intel_{g,h}uc_select_fw can be made part of the sanitize options,
but I think it better belongs here.

That's is basing on your suggestions for the other patch.


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

[Joonas Lahtinen]

On ke, 2017-02-22 at 16:29 +0100, Arkadiusz Hiler wrote:
> On Wed, Feb 22, 2017 at 03:59:01PM +0200, Joonas Lahtinen wrote:
> > 
> > > + * @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;
> > 
> > Similarly arrange to get rid of fw_path here.
> 
> Patch 8 in the series addresses that issue as well. Maybe I should move
> them around?

Nah, it's fine, the intermediary steps need to be working (for
bisecting), but not necessarily 100% pretty. If it's addressed later,
it's good.

> > > @@ -30,6 +30,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> > >  	mutex_init(&dev_priv->guc.send_mutex);
> > >  }
> > >  
> > > +void intel_uc_fetch_fw(struct drm_i915_private *dev_priv)
> > 
> > This function might be worth calling intel_uc_init (See above), if the
> > need comes to add other stuff. But either way.
> 
> This is quite confusing now. I was fine it being named init, someone
> suggested to be more descriptive with the name, as it is not general
> enough to be "init". Seemed reasonable enough for me, so I incorporated
> that in the respin.
> 
> This is turning into some heavy bikeshedding now...

That's why actual code in the mailing list is the only right way,
discussion in IRC can be misleading :)

> 
> I agree that it's more than fetch, it actually selects + fetches +
> populates the structures.
> 
> I'll gladly ignore previous feedback on being to vague with name and
> just go with init, but let give the _fw postfix one last chance:
> 
> 
> intel_guc_init_fw {
>     intel_guc_select_fw
>     if (NULL != guc.fw.path)

if (guc.fw.patch) to stick to coding style.

> 	    intel_uc_prepare_fw
> }
> 
> Where select does what the guc's fetch fw does sans the uc_fetch call.

Sounds good to me.

> Also intel_{g,h}uc_select_fw can be made part of the sanitize options,
> but I think it better belongs here.
> 
> That's is basing on your suggestions for the other patch.

Thats, correct, select_fw should be here.


        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);
        }

This part is a perfect fit to the sanitize_options function, because
that's what it does, makes sure we don't try to enable something we
don't have.

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 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 wrong 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 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 5bb9149..222a928 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_uc_fetch_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 db596b6..b9d2230 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

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

[Joonas Lahtinen]

On ke, 2017-02-22 at 13:41 +0100, Arkadiusz Hiler wrote:
> intel_uc_fw_fetch() is confusingly named in the light of recent changes.
> 
> It's also in the wrong 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 Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

<SNIP>

> @@ -32,7 +36,10 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  
>  void intel_uc_fetch_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;

I'm bit confused, I was under impression these functions were going to
be different for each uC? If they're not, then it most definitely
should not be a function pointer.

	int ret;

	ret = intel_huc_select_fw(dev_priv->huc);
	if (ret)
		goto err;
	ret = intel_uc_fw_prepare(&dev_priv->huc->fw);
	if (ret)
		goto err;

	ret = intel_guc_select_fw(dev_priv->guc);
	if (ret)
		goto err_huc;
	ret = intel_uc_fw_prepare(&dev_priv->guc->fw);
	if (ret)
		
goto err_huc;

	return 0;

err_huc:
	intel_uc_fw_unprepare(&dev_priv->huc->fw);
err:
	return ret;

By always involving proper onion teardown, no dangling objects are left
around.

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

This function should be named differently, because it doesn't simply
fetch it, but also validates and makes an object of it (which may be
left dangling).

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

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

[Arkadiusz Hiler]

On Wed, Feb 22, 2017 at 04:17:20PM +0200, Joonas Lahtinen wrote:
> On ke, 2017-02-22 at 13:41 +0100, Arkadiusz Hiler wrote:
> > intel_uc_fw_fetch() is confusingly named in the light of recent changes.
> > 
> > It's also in the wrong 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 Winiarski <michal.winiarski@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> 
> <SNIP>
> 
> > @@ -32,7 +36,10 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> >  
> >  void intel_uc_fetch_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;
> 
> I'm bit confused, I was under impression these functions were going to
> be different for each uC? If they're not, then it most definitely
> should not be a function pointer.

The issue I presented on the IRC (maybe not clearly enough) was just
placement and naming of that function. The proposition with callback
seemd odd, but since it was backed by couple of people I commited to it.

Glad you spoted that here.

> 	int ret;
> 
> 	ret = intel_huc_select_fw(dev_priv->huc);
> 	if (ret)
> 		goto err;
> 	ret = intel_uc_fw_prepare(&dev_priv->huc->fw);
> 	if (ret)
> 		goto err;
> 
> 	ret = intel_guc_select_fw(dev_priv->guc);
> 	if (ret)
> 		goto err_huc;
> 	ret = intel_uc_fw_prepare(&dev_priv->guc->fw);
> 	if (ret)
> 		
> goto err_huc;
> 
> 	return 0;
> 
> err_huc:
> 	intel_uc_fw_unprepare(&dev_priv->huc->fw);
> err:
> 	return ret;
> 
> By always involving proper onion teardown, no dangling objects are left
> around.

That's exactly what I was missing in my approach. Thanks!

> >  	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;
> 
> This function should be named differently, because it doesn't simply
> fetch it, but also validates and makes an object of it (which may be
> left dangling).

intel_uc_fw_prepare you've used in the example above seems like a good
name.

Onto fixing this mess. Thanks for the input :-)

-- 
Cheers,
Arek
_______________________________________________
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

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 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)
v3: rename once again

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         | 107 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |   5 ++
 4 files changed, 125 insertions(+), 125 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index bbe2df9..ec958cf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4512,7 +4512,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 9355907..3de5fba 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);
 
+/* Reset GuC providing us with fresh state for both GuC and HuC.
+ */
+static int __uc_hw_reset(struct drm_i915_private *dev_priv)
+{
+	int ret;
+	u32 guc_status;
+
+	ret = intel_guc_reset(dev_priv);
+	if (ret) {
+		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
+		return ret;
+	}
+
+	guc_status = I915_READ(GUC_STATUS);
+	WARN(!(guc_status & GS_MIA_IN_RESET),
+	     "GuC status: 0x%x, MIA core expected to be in reset\n",
+	     guc_status);
+
+	return ret;
+}
+
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_GUC(dev_priv)) {
@@ -68,6 +89,92 @@ void intel_uc_fetch_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 = __uc_hw_reset(dev_priv);
+		if (ret)
+			goto fail;
+
+		intel_huc_init_hw(dev_priv);
+		ret = intel_guc_init_hw(dev_priv);
+		if (ret == 0 || ret != -EAGAIN)
+			break;
+
+		DRM_INFO("GuC fw load failed: %d; will reset and "
+			 "retry %d more time(s)\n", ret, retries);
+	}
+
+	/* did we succeded or run out of retries? */
+	if (ret)
+		goto fail;
+
+	intel_guc_auth_huc(dev_priv);
+	if (i915.enable_guc_submission) {
+		if (i915.guc_log_level >= 0)
+			gen9_enable_guc_interrupts(dev_priv);
+
+		ret = i915_guc_submission_enable(dev_priv);
+		if (ret)
+			goto fail;
+		intel_guc_interrupts_capture(dev_priv);
+	}
+
+	return 0;
+
+fail:
+	/*
+	 * We've failed to load the firmware :(
+	 *
+	 * Decide whether to disable GuC submission and fall back to
+	 * execlist mode, and whether to hide the error by returning
+	 * zero or to return -EIO, which the caller will treat as a
+	 * nonfatal error (i.e. it doesn't prevent driver load, but
+	 * marks the GPU as wedged until reset).
+	 */
+	DRM_ERROR("GuC init failed\n");
+	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
+		ret = -EIO;
+	else
+		ret = 0;
+
+	if (i915.enable_guc_submission) {
+		i915.enable_guc_submission = 0;
+		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
+	}
+
+	i915_ggtt_disable_guc(dev_priv);
+	intel_guc_interrupts_release(dev_priv);
+	i915_guc_submission_disable(dev_priv);
+	i915_guc_submission_fini(dev_priv);
+
+	return ret;
+}
+
 /*
  * Read GuC command/status register (SOFT_SCRATCH_0)
  * Return true if it contains a response rather than a command
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index cf5d0f4..721a6c2 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_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_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

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

[Joonas Lahtinen]

On ke, 2017-02-22 at 13:41 +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)
> v3: rename once again
> 
> 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>

> @@ -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) {

if (!fw_path)

>  		/* Device is known to have no uCode (e.g. no GuC) */

Spurious comment.

> -		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? */

Even more spurious comment, as there's WARN :)

>  		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? */

Code is again self-documenting here.

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

Single line braces can be dropped.

> +int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> +{

This function is still lacking onion tear-down (because it's so
convoluted), but it's in the better direction (and it was previously
so).

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

As this is a W/A only, I'd keep the code local to single spot.

With above stylistic nitpicks;

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

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

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

[Arkadiusz Hiler]

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

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

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

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

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

v2: fewer temporary variables, more straightforward flow (M. Wajdeczko)
v3: DRM_ERROR instead of WARN (M. Wajdeczko)

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..336e97d 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 */
+		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
+		i915.enable_guc_loading = 0;
+		return;
 	}
 
-	guc->fw.path = fw_path;
-	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
-	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
-
-	if (fw_path == NULL)
-		return;
-	if (*fw_path == '\0')
-		return;
-
-	guc->fw.fetch_status = INTEL_UC_FIRMWARE_PENDING;
-	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
 	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..411595d 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 {
+		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
+		return;
 	}
 
-	huc->fw.path = fw_path;
-	huc->fw.fetch_status = INTEL_UC_FIRMWARE_PENDING;
-
-	DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path);
-
-	WARN(huc->fw.path == NULL, "HuC present but no fw path\n");
-
 	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 3de5fba..4073549 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -269,8 +269,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 8/8] drm/i915/uc: Simplify firmware path handling

[Chris Wilson]

On Wed, Feb 22, 2017 at 01:41:25PM +0100, Arkadiusz Hiler wrote:
> Currently fw->path values can represent one of three possible states:
> 
>  1) NULL - device without the uC
>  2) '\0' - device with the uC but have no firmware
>  3) else - device with the uC and we have firmware
> 
> Second case is used only to WARN at a later stage.
> 
> We can WARN right away and merge cases 1 and 2.
> 
> Code can be even further simplified and common (HuC/GuC logic) happening
> right before the fetch can be offloaded to the common function.
> 
> v2: fewer temporary variables, more straightforward flow (M. Wajdeczko)
> v3: DRM_ERROR instead of WARN (M. Wajdeczko)
> 
> 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..336e97d 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 */
> +		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
> +		i915.enable_guc_loading = 0;
> +		return;

Now plan for having fw_path overriden by a i915_param.guc_firmware.

Perhaps something like
	if (i915_param.guc_firmware) {
		guc->fw.path = i915_param.guc_firmware; /* needs 0400! */
		guc->fw.major_ver_wanted = -1;
		guc->fw.minor_ver_wanted = -1;
	} else if (IS_SKYLAKE....
works?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
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 firmware path handling

[Arkadiusz Hiler]

On Wed, Feb 22, 2017 at 12:53:47PM +0000, Chris Wilson wrote:
> On Wed, Feb 22, 2017 at 01:41:25PM +0100, Arkadiusz Hiler wrote:
> > Currently fw->path values can represent one of three possible states:
> > 
> >  1) NULL - device without the uC
> >  2) '\0' - device with the uC but have no firmware
> >  3) else - device with the uC and we have firmware
> > 
> > Second case is used only to WARN at a later stage.
> > 
> > We can WARN right away and merge cases 1 and 2.
> > 
> > Code can be even further simplified and common (HuC/GuC logic) happening
> > right before the fetch can be offloaded to the common function.
> > 
> > v2: fewer temporary variables, more straightforward flow (M. Wajdeczko)
> > v3: DRM_ERROR instead of WARN (M. Wajdeczko)
> > 
> > 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..336e97d 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 */
> > +		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
> > +		i915.enable_guc_loading = 0;
> > +		return;
> 
> Now plan for having fw_path overriden by a i915_param.guc_firmware.
> 
> Perhaps something like
> 	if (i915_param.guc_firmware) {
> 		guc->fw.path = i915_param.guc_firmware; /* needs 0400! */
> 		guc->fw.major_ver_wanted = -1;
> 		guc->fw.minor_ver_wanted = -1;
> 	} else if (IS_SKYLAKE....
> works?

Sorry, I do not quite understand the comment. Can you elaborate?

-- 
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 firmware path handling

[Arkadiusz Hiler]

On Wed, Feb 22, 2017 at 04:30:49PM +0100, Arkadiusz Hiler wrote:
> On Wed, Feb 22, 2017 at 12:53:47PM +0000, Chris Wilson wrote:
> > On Wed, Feb 22, 2017 at 01:41:25PM +0100, Arkadiusz Hiler wrote:
> > > Currently fw->path values can represent one of three possible states:
> > > 
> > >  1) NULL - device without the uC
> > >  2) '\0' - device with the uC but have no firmware
> > >  3) else - device with the uC and we have firmware
> > > 
> > > Second case is used only to WARN at a later stage.
> > > 
> > > We can WARN right away and merge cases 1 and 2.
> > > 
> > > Code can be even further simplified and common (HuC/GuC logic) happening
> > > right before the fetch can be offloaded to the common function.
> > > 
> > > v2: fewer temporary variables, more straightforward flow (M. Wajdeczko)
> > > v3: DRM_ERROR instead of WARN (M. Wajdeczko)
> > > 
> > > 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..336e97d 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 */
> > > +		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
> > > +		i915.enable_guc_loading = 0;
> > > +		return;
> > 
> > Now plan for having fw_path overriden by a i915_param.guc_firmware.
> > 
> > Perhaps something like
> > 	if (i915_param.guc_firmware) {
> > 		guc->fw.path = i915_param.guc_firmware; /* needs 0400! */
> > 		guc->fw.major_ver_wanted = -1;
> > 		guc->fw.minor_ver_wanted = -1;
> > 	} else if (IS_SKYLAKE....
> > works?
> 
> Sorry, I do not quite understand the comment. Can you elaborate?

Nevermind, got it.

LGTM, but for it to fully work we need to make uc_fetch_fw (or however
it will end up being named) aware that -1 have special meaning.

Now the version cross-check looks like that:

if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {


-- 
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 firmware path handling

[Joonas Lahtinen]

On ke, 2017-02-22 at 16:52 +0100, Arkadiusz Hiler wrote:
> On Wed, Feb 22, 2017 at 04:30:49PM +0100, Arkadiusz Hiler wrote:
> > 
> > On Wed, Feb 22, 2017 at 12:53:47PM +0000, Chris Wilson wrote:
> > > 
> > > Now plan for having fw_path overriden by a i915_param.guc_firmware.
> > > 
> > > Perhaps something like
> > > 	if (i915_param.guc_firmware) {
> > > 		guc->fw.path = i915_param.guc_firmware; /* needs 0400! */
> > > 		guc->fw.major_ver_wanted = -1;
> > > 		guc->fw.minor_ver_wanted = -1;
> > > 	} else if (IS_SKYLAKE....
> > > works?
> > 
> > Sorry, I do not quite understand the comment. Can you elaborate?
> 
> Nevermind, got it.
> 
> LGTM, but for it to fully work we need to make uc_fetch_fw (or however
> it will end up being named) aware that -1 have special meaning.
> 
> Now the version cross-check looks like that:
> 
> if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
>     uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {

What Chris proposed, together with tweaking the check sounds good.

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

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

[Joonas Lahtinen]

On ke, 2017-02-22 at 13:41 +0100, Arkadiusz Hiler wrote:
> Currently fw->path values can represent one of three possible states:
> 
>  1) NULL - device without the uC
>  2) '\0' - device with the uC but have no firmware
>  3) else - device with the uC and we have firmware
> 
> Second case is used only to WARN at a later stage.
> 
> We can WARN right away and merge cases 1 and 2.
> 
> Code can be even further simplified and common (HuC/GuC logic) happening
> right before the fetch can be offloaded to the common function.
> 
> v2: fewer temporary variables, more straightforward flow (M. Wajdeczko)
> v3: DRM_ERROR instead of WARN (M. Wajdeczko)
> 
> 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>

<SNIP>

> @@ -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 (!guc_fw->path)
	return -ENXIO;

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

<SNIP>

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

This was function pointer was discussed in other thread.

With above addressed;

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

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

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

[Patchwork]

== Series Details ==

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

== Summary ==

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

fi-bdw-5557u     total:253  pass:242  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:253  pass:214  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:253  pass:234  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:253  pass:226  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:253  pass:222  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:253  pass:237  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:253  pass:237  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:253  pass:203  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:253  pass:243  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:253  pass:236  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:253  pass:231  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:253  pass:243  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:253  pass:225  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:253  pass:224  dwarn:0   dfail:0   fail:0   skip:29 

bf89ec45d0822835b03910371ac0baf46c4efa2d drm-tip: 2017y-02m-22d-14h-30m-04s UTC integration manifest
de86964 drm/i915/uc: Simplify firmware path handling
3bd35f1 drm/i915/guc: Simplify intel_guc_init_hw()
9d55149 drm/i915/guc: Extract param logic form guc_init
e5b5068 drm/i915/uc: Make intel_uc_fw_fetch() static
f32b9e4 drm/i915/uc: Rename intel_?uc_init() to intel_?uc_fetch_fw()
d2ca92a drm/i915/huc: Add huc_to_i915
20c4531 drm/i915/uc: Drop superfluous externs in intel_uc.h
437508e drm/i915/uc: Rename intel_?uc_{setup, load}() to _init_hw()

== Logs ==

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

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

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