diff mbox

drm/i915/uc: Move intel_uc_fw_fetch() to intel_uc.c

Message ID 20170303123634.13970-1-arkadiusz.hiler@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Arkadiusz Hiler March 3, 2017, 12:36 p.m. UTC
The file fits better.

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

`obj` cleanup in the function is also fixed (i.e. removed). In the fail
scenario it was always 'put' but there's no possible flow that
initializes the obj properly and then goes to the fail label.

v2: remove second declaration, reorder (M. Wajdeczko)
v3: non-trivial rebase
v4: remove obj cleanup in the fail scenario (C. Wilson)

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

Comments

Michal Wajdeczko March 3, 2017, 2:13 p.m. UTC | #1
On Fri, Mar 03, 2017 at 01:36:34PM +0100, Arkadiusz Hiler wrote:
> The file fits better.
> 
> Additionally rename it to intel_uc_prepare_fw(), as the function does
> more than simple fetch.
> 
> `obj` cleanup in the function is also fixed (i.e. removed). In the fail
> scenario it was always 'put' but there's no possible flow that
> initializes the obj properly and then goes to the fail label.
> 
> v2: remove second declaration, reorder (M. Wajdeczko)
> v3: non-trivial rebase
> v4: remove obj cleanup in the fail scenario (C. Wilson)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---

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

but also added some more comments how to improve of existing messages.

-Michal

<snip>


> @@ -114,3 +115,133 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
>  	return intel_guc_send(guc, action, ARRAY_SIZE(action));
>  }
>  
> +void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
> +			 struct intel_uc_fw *uc_fw)
> +{
> +	struct pci_dev *pdev = dev_priv->drm.pdev;
> +	struct drm_i915_gem_object *obj;
> +	const struct firmware *fw = NULL;
> +	struct uc_css_header *css;
> +	size_t size;
> +	int err;
> +
> +	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
> +		intel_uc_fw_status_repr(uc_fw->fetch_status));
> +
> +	err = request_firmware(&fw, uc_fw->path, &pdev->dev);
> +	if (err)
> +		goto fail;
> +	if (!fw)
> +		goto fail;

I'm not sure that we need this extra check for null fw.

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

Btw, this message is little inaccurate. What about
	DRM_ERROR("uC firmare is too small"

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

Btw, this message is inaccurate too. What about
	DRM_ERROR("uC firmware header is invalid"

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

Btw, this message can be improved too. What about
	DRM_ERROR("uC firmware signature is corrupted"

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

Btw, this message can be improved too. What about
	"uC firmware blob is truncated"

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

Hmm, is it right place to perform such check ?
Maybe we can move it to guc_ucode_xfer() ?

> +			DRM_ERROR("Firmware is too large to fit in WOPCM\n");

Btw, this message can be improved too. s/Firmware/uC firmware/

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

s/DRM_NOTE/DRM_ERROR ?

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

Please drop "obj" from the message (as it will be always invalid at this point)
Also drop "err" as it is already in DRM_WARN.
Then reconsider if we still need to log "fw" pointer.


> +
> +	release_firmware(fw);		/* OK even if fw is NULL */
> +	uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
> +}
diff mbox

Patch

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