diff mbox

[v3,1/6] drm/i915/guc: Make the GuC fw loading helper functions general

Message ID 82d2e605-87a9-ec3c-358b-206255f496d9@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dave Gordon July 29, 2016, 11:18 a.m. UTC 
On 06/07/16 15:24, Peter Antoine wrote:
> Rename some of the GuC fw loading code to make them more general. We
> will utilise them for HuC loading as well.
>      s/intel_guc_fw/intel_uc_fw/g
>      s/GUC_FIRMWARE/UC_FIRMWARE/g
>
> Struct intel_guc_fw is renamed to intel_uc_fw. Prefix of tts members,
> such as 'guc' or 'guc_fw' either is renamed to 'uc' or removed for
> same purpose.
>
> v2: rebased on top of nightly.
>     reapplied the search/replace as upstream code as changed.
> v3: rebased again on drm-nightly.
>
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        |  12 +--
>  drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
>  drivers/gpu/drm/i915/intel_guc.h           |  39 ++++----
>  drivers/gpu/drm/i915/intel_guc_loader.c    | 146 ++++++++++++++---------------
>  4 files changed, 101 insertions(+), 100 deletions(-)

As of yesterday, the odd-numbered patches 1 & 3 no longer apply cleanly 
and will need rebasing (again).

Also (as of last week's Tech Forum) any series containing more than a 
single patch should preferably have a cover letter that at least gives a 
summary of the patchset as a whole.

However the main problem with this patch it not what it changes, as what 
it fails to change. As I previously suggested (and provided code for) 
you need to change all the messages so they don't say "GuC" when we're 
actually dealing with the HuC. Updated fixup-patch attached ...

.Dave.

Comments

Peter Antoine Aug. 2, 2016, 8:27 a.m. UTC | #1 
This patch has nothing to do with HuC. It is changing the GuC code to allow for generic usage.
But, I will change the "GuC" in the messages for "uC".

I'll let you  add  the patch yourself as you seem to have other renames/refactors on the mailing list.

Peter.

-----Original Message-----
From: Gordon, David S 

Sent: Friday, July 29, 2016 12:19 PM
To: Antoine, Peter <peter.antoine@intel.com>; intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 1/6] drm/i915/guc: Make the GuC fw loading helper functions general

On 06/07/16 15:24, Peter Antoine wrote:
> Rename some of the GuC fw loading code to make them more general. We 

> will utilise them for HuC loading as well.

>      s/intel_guc_fw/intel_uc_fw/g

>      s/GUC_FIRMWARE/UC_FIRMWARE/g

>

> Struct intel_guc_fw is renamed to intel_uc_fw. Prefix of tts members, 

> such as 'guc' or 'guc_fw' either is renamed to 'uc' or removed for 

> same purpose.

>

> v2: rebased on top of nightly.

>     reapplied the search/replace as upstream code as changed.

> v3: rebased again on drm-nightly.

>

> Signed-off-by: Alex Dai <yu.dai@intel.com>

> Signed-off-by: Peter Antoine <peter.antoine@intel.com>

> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

> ---

>  drivers/gpu/drm/i915/i915_debugfs.c        |  12 +--

>  drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-

>  drivers/gpu/drm/i915/intel_guc.h           |  39 ++++----

>  drivers/gpu/drm/i915/intel_guc_loader.c    | 146 ++++++++++++++---------------

>  4 files changed, 101 insertions(+), 100 deletions(-)


As of yesterday, the odd-numbered patches 1 & 3 no longer apply cleanly and will need rebasing (again).

Also (as of last week's Tech Forum) any series containing more than a single patch should preferably have a cover letter that at least gives a summary of the patchset as a whole.

However the main problem with this patch it not what it changes, as what it fails to change. As I previously suggested (and provided code for) you need to change all the messages so they don't say "GuC" when we're actually dealing with the HuC. Updated fixup-patch attached ...

.Dave.
diff mbox

Patch

From 8a2e98098ff48ed1a9abec3159e630b3a8c18f64 Mon Sep 17 00:00:00 2001
From: Dave Gordon <david.s.gordon@intel.com>
Date: Tue, 28 Jun 2016 13:09:54 +0100
Subject: [PATCH v3] Tweaks to GuC/HuC loader code
Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ

1. Add uc_name to intel_uc_fw structure, so we can use the appropriate
name ("GuC" or "HuC") everywhere (obviously, it should match the fw_type
field).

2. Change all messages in intel_uc_fw_fetch() to use the uc_name.

3. Validate fw_type at the beginning to intel_uc_fw_fetch() rather than
midway. Is there a firmware type in the CSS header -- if so we should
use that and the relevant definitions.

4. Refactor printing the firmware fetch/load status into a macro.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.h        |  3 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 84 +++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_huc_loader.c |  3 +-
 3 files changed, 57 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index c7b2745..c04aef8 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -106,9 +106,10 @@  enum intel_uc_fw_status {
  */
 struct intel_uc_fw {
 	struct drm_device *uc_dev;
+	const char *uc_name;
 	const char *uc_fw_path;
-	size_t uc_fw_size;
 	struct drm_i915_gem_object *uc_fw_obj;
+	size_t uc_fw_size;
 	enum intel_uc_fw_status fetch_status;
 	enum intel_uc_fw_status load_status;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index c08b81a..b8c0df7 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -68,6 +68,15 @@ 
 #define I915_KBL_GUC_UCODE "i915/kbl_guc_ver9_14.bin"
 MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
 
+#define	DEBUG_UC_FW_STATUS(uc_fw)					\
+	{								\
+		DRM_DEBUG_DRIVER("%s fw status: path %s, fetch %s, load %s\n",	\
+			uc_fw->uc_name,					\
+			uc_fw->uc_fw_path,				\
+			intel_uc_fw_status_repr(uc_fw->fetch_status),	\
+			intel_uc_fw_status_repr(uc_fw->load_status));	\
+	}
+
 /* User-friendly representation of an enum */
 const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 {
@@ -156,7 +165,7 @@  static u32 get_core_family(struct drm_i915_private *dev_priv)
 		return GFXCORE_FAMILY_GEN9;
 
 	default:
-		DRM_ERROR("GUC: unsupported core family\n");
+		DRM_ERROR("GuC: unsupported core family\n");
 		return GFXCORE_FAMILY_UNKNOWN;
 	}
 }
@@ -421,10 +430,7 @@  int intel_guc_setup(struct drm_device *dev)
 	const char *fw_path = guc_fw->uc_fw_path;
 	int retries, ret, err;
 
-	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));
+	DEBUG_UC_FW_STATUS(guc_fw);
 
 	/* Loading forbidden, or no firmware to load? */
 	if (!i915.enable_guc_loading) {
@@ -454,9 +460,7 @@  int intel_guc_setup(struct drm_device *dev)
 
 	guc_fw->load_status = 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));
+	DEBUG_UC_FW_STATUS(guc_fw);
 
 	err = i915_guc_submission_init(dev_priv);
 	if (err)
@@ -493,9 +497,7 @@  int intel_guc_setup(struct drm_device *dev)
 
 	guc_fw->load_status = UC_FIRMWARE_SUCCESS;
 
-	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));
+	DEBUG_UC_FW_STATUS(guc_fw);
 
 	intel_huc_auth(dev);
 
@@ -563,8 +565,20 @@  void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
 	size_t size;
 	int err;
 
-	DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status %s\n",
-		intel_uc_fw_status_repr(uc_fw->fetch_status));
+	/* Validate fw_type first of all, so we can name the uC */
+	switch (uc_fw->fw_type) {
+	case UC_FW_TYPE_GUC:
+	case UC_FW_TYPE_HUC:
+		break;
+
+	default:
+		DRM_ERROR("Unknown firmware type %d\n", uc_fw->fw_type);
+		err = -ENOEXEC;
+		goto fail;
+	}
+
+	DRM_DEBUG_DRIVER("%s fw fetch status %s before requesting firmware\n",
+		uc_fw->uc_name, intel_uc_fw_status_repr(uc_fw->fetch_status));
 
 	err = request_firmware(&fw, uc_fw->uc_fw_path, &dev->pdev->dev);
 	if (err)
@@ -572,12 +586,12 @@  void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
 	if (!fw)
 		goto fail;
 
-	DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n",
-		uc_fw->uc_fw_path, fw);
+	DRM_DEBUG_DRIVER("%s fw fetch from %s succeeded, fw %p\n",
+		uc_fw->uc_name, uc_fw->uc_fw_path, fw);
 
 	/* Check the size of the blob before examining buffer contents */
 	if (fw->size < sizeof(struct uc_css_header)) {
-		DRM_ERROR("Firmware header is missing\n");
+		DRM_ERROR("%s firmware header is missing\n", uc_fw->uc_name);
 		goto fail;
 	}
 
@@ -589,7 +603,8 @@  void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
 		css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
 
 	if (uc_fw->header_size != sizeof(struct uc_css_header)) {
-		DRM_ERROR("CSS header definition mismatch\n");
+		DRM_ERROR("%s firmware CSS header definition mismatch\n",
+			uc_fw->uc_name);
 		goto fail;
 	}
 
@@ -599,7 +614,7 @@  void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
 
 	/* now RSA */
 	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
-		DRM_ERROR("RSA key size is bad\n");
+		DRM_ERROR("%s firmware RSA key size is bad\n", uc_fw->uc_name);
 		goto fail;
 	}
 	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
@@ -608,7 +623,7 @@  void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
 	/* 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_ERROR("Missing firmware components\n");
+		DRM_ERROR("%s firmware missing components\n", uc_fw->uc_name);
 		goto fail;
 	}
 
@@ -625,35 +640,40 @@  void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
 
 		/* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */
 		if (size > guc_wopcm_size(to_i915(dev))) {
-			DRM_ERROR("Firmware is too large to fit in WOPCM\n");
+			DRM_ERROR("%s firmware is too large to fit in WOPCM\n",
+				uc_fw->uc_name);
 			goto fail;
 		}
 
 		uc_fw->major_ver_found = css->guc_sw_version >> 16;
 		uc_fw->minor_ver_found = css->guc_sw_version & 0xFFFF;
 		break;
+
 	case 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_type);
+		/*NOTREACHED*/
 		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_ERROR("GuC firmware version %d.%d, required %d.%d\n",
+		DRM_ERROR("Found %s firmware version %d.%d, required version %d.%d\n",
+			uc_fw->uc_name,
 			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);
+	DRM_DEBUG_DRIVER("%s firmware version %d.%d OK (minimum %d.%d)\n",
+		uc_fw->uc_name,
+		uc_fw->major_ver_found, uc_fw->minor_ver_found,
+		uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
 
 	mutex_lock(&dev->struct_mutex);
 	obj = i915_gem_object_create_from_data(dev, fw->data, fw->size);
@@ -666,18 +686,18 @@  void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
 	uc_fw->uc_fw_obj = obj;
 	uc_fw->uc_fw_size = fw->size;
 
-	DRM_DEBUG_DRIVER("GuC fw fetch status SUCCESS, obj %p\n",
-			uc_fw->uc_fw_obj);
+	DRM_DEBUG_DRIVER("%s fw fetch status SUCCESS, obj %p\n",
+		uc_fw->uc_name, uc_fw->uc_fw_obj);
 
 	release_firmware(fw);
 	uc_fw->fetch_status = UC_FIRMWARE_SUCCESS;
 	return;
 
 fail:
-	DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj %p\n",
-		err, fw, uc_fw->uc_fw_obj);
-	DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
-		  uc_fw->uc_fw_path, err);
+	DRM_DEBUG_DRIVER("%s fw fetch status FAIL; err %d, fw %p, obj %p\n",
+		uc_fw->uc_name, err, fw, uc_fw->uc_fw_obj);
+	DRM_ERROR("Failed to fetch %s firmware from %s (error %d)\n",
+		uc_fw->uc_name, uc_fw->uc_fw_path, err);
 
 	mutex_lock(&dev->struct_mutex);
 	obj = uc_fw->uc_fw_obj;
@@ -730,6 +750,8 @@  void intel_guc_init(struct drm_device *dev)
 	}
 
 	guc_fw->uc_dev = dev;
+	guc_fw->uc_name = "GuC";
+	guc_fw->fw_type = UC_FW_TYPE_GUC;
 	guc_fw->uc_fw_path = fw_path;
 	guc_fw->fetch_status = UC_FIRMWARE_NONE;
 	guc_fw->load_status = UC_FIRMWARE_NONE;
diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c
index c6d53b3..2ae0542 100644
--- a/drivers/gpu/drm/i915/intel_huc_loader.c
+++ b/drivers/gpu/drm/i915/intel_huc_loader.c
@@ -148,10 +148,11 @@  void intel_huc_init(struct drm_device *dev)
 	const char *fw_path = NULL;
 
 	huc_fw->uc_dev = dev;
+	huc_fw->uc_name = "HuC";
+	huc_fw->fw_type = UC_FW_TYPE_HUC;
 	huc_fw->uc_fw_path = NULL;
 	huc_fw->fetch_status = UC_FIRMWARE_NONE;
 	huc_fw->load_status = UC_FIRMWARE_NONE;
-	huc_fw->fw_type = UC_FW_TYPE_HUC;
 
 	if (!HAS_HUC_UCODE(dev_priv))
 		return;
-- 
1.9.1