[Intel-gfx] [PATCH] drm/i915/uc: Move intel_uc_fw_fetch() to intel_uc.c

[Michal Wajdeczko]

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

Reviewed-by: Michal Wajdeczko <michal.wajdeczko at 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;
> +}