diff mbox

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

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

Commit Message

Arkadiusz Hiler Feb. 22, 2017, 12:41 p.m. UTC
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(-)

Comments

Joonas Lahtinen Feb. 22, 2017, 2:17 p.m. UTC | #1
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
Arkadiusz Hiler Feb. 22, 2017, 3:04 p.m. UTC | #2
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 :-)
diff mbox

Patch

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