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@xxxxxxxxxxxxxxxxxx>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx>
> ---
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
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;
> +}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
