Re: [P v4 09/11] drm/i915/uc: Unify firmware loading

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Michal Wajdeczko (2017-10-10 15:51:33)
> Firmware loading for GuC and Huc are similar. Move common code
> into generic function for maximum reuse.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_fw.c | 52 +++---------------------
>  drivers/gpu/drm/i915/intel_huc.c    | 53 +++---------------------
>  drivers/gpu/drm/i915/intel_uc_fw.c  | 81 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc_fw.h  |  4 ++
>  4 files changed, 95 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
> index 51be318..4629ff3 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
> @@ -193,24 +193,13 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>  /*
>   * Load the GuC firmware blob into the MinuteIA.
>   */
> -static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
> +static int guc_ucode_xfer(struct intel_uc_fw *guc_fw, struct i915_vma *vma)
>  {
> -       struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> -       struct i915_vma *vma;
> +       struct intel_guc *guc = container_of(guc_fw, struct intel_guc, fw);
> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>         int ret;
>  
> -       ret = i915_gem_object_set_to_gtt_domain(guc_fw->obj, false);
> -       if (ret) {
> -               DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
> -               return ret;
> -       }
> -
> -       vma = i915_gem_object_ggtt_pin(guc_fw->obj, NULL, 0, 0,
> -                                      PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> -       if (IS_ERR(vma)) {
> -               DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
> -               return PTR_ERR(vma);
> -       }
> +       GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
>  
>         intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>  
> @@ -245,12 +234,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  
>         intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  
> -       /*
> -        * We keep the object pages for reuse during resume. But we can unpin it
> -        * now that DMA has completed, so it doesn't continue to take up space.
> -        */
> -       i915_vma_unpin(vma);
> -
>         return ret;
>  }
>  
> @@ -269,30 +252,5 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>   */
>  int intel_guc_fw_upload(struct intel_guc *guc)
>  {
> -       struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -       const char *fw_path = guc->fw.path;
> -       int ret;
> -
> -       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));
> -
> -       if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> -               return -EIO;
> -
> -       guc->fw.load_status = INTEL_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));
> -
> -       ret = guc_ucode_xfer(dev_priv);
> -
> -       if (ret)
> -               return -EAGAIN;
> -
> -       guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
> -
> -       return 0;
> +       return intel_uc_fw_upload(&guc->fw, guc_ucode_xfer);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 4b4cf56..8be3bfe 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -85,26 +85,15 @@ MODULE_FIRMWARE(I915_KBL_HUC_UCODE);
>   *
>   * Return: 0 on success, non-zero on failure
>   */
> -static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
> +static int huc_ucode_xfer(struct intel_uc_fw *huc_fw, struct i915_vma *vma)
>  {
> -       struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> -       struct i915_vma *vma;
> +       struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw);
> +       struct drm_i915_private *dev_priv = huc_to_i915(huc);
>         unsigned long offset = 0;
>         u32 size;
>         int ret;
>  
> -       ret = i915_gem_object_set_to_gtt_domain(huc_fw->obj, false);
> -       if (ret) {
> -               DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
> -               return ret;
> -       }
> -
> -       vma = i915_gem_object_ggtt_pin(huc_fw->obj, NULL, 0, 0,
> -                               PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> -       if (IS_ERR(vma)) {
> -               DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
> -               return PTR_ERR(vma);
> -       }
> +       GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
>  
>         intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>  
> @@ -135,12 +124,6 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
>  
>         intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  
> -       /*
> -        * We keep the object pages for reuse during resume. But we can unpin it
> -        * now that DMA has completed, so it doesn't continue to take up space.
> -        */
> -       i915_vma_unpin(vma);
> -
>         return ret;
>  }
>  
> @@ -194,33 +177,7 @@ void intel_huc_select_fw(struct intel_huc *huc)
>   */
>  void intel_huc_init_hw(struct intel_huc *huc)
>  {
> -       struct drm_i915_private *dev_priv = huc_to_i915(huc);
> -       int err;
> -
> -       DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
> -               huc->fw.path,
> -               intel_uc_fw_status_repr(huc->fw.fetch_status),
> -               intel_uc_fw_status_repr(huc->fw.load_status));
> -
> -       if (huc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> -               return;
> -
> -       huc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
> -
> -       err = huc_ucode_xfer(dev_priv);
> -
> -       huc->fw.load_status = err ?
> -               INTEL_UC_FIRMWARE_FAIL : INTEL_UC_FIRMWARE_SUCCESS;
> -
> -       DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
> -               huc->fw.path,
> -               intel_uc_fw_status_repr(huc->fw.fetch_status),
> -               intel_uc_fw_status_repr(huc->fw.load_status));
> -
> -       if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> -               DRM_ERROR("Failed to complete HuC uCode load with ret %d\n", err);
> -
> -       return;
> +       intel_uc_fw_upload(&huc->fw, huc_ucode_xfer);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
> index b52d6b6..16dc970 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
> @@ -182,6 +182,87 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>  }
>  
>  /**
> + * intel_uc_fw_upload - load uC firmware using custom loader
> + *
> + * @uc_fw: uC firmware
> + * @loader: custom uC firmware loader function
> + *
> + * Loads uC firmware using custom loader and updates internal flags.
> + */
> +int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
> +                      int (*loader)(struct intel_uc_fw *uc_fw,
> +                                    struct i915_vma *vma))

Suggestion s/loader/xfer/, it's a bit more commonplace.

> +{
> +       struct i915_vma *vma;
> +       int err;
> +
> +       DRM_DEBUG_DRIVER("%s fw load %s\n",
> +                        intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
> +
> +       if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> +               return -EIO;
> +
> +       uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
> +       DRM_DEBUG_DRIVER("%s fw load %s\n",
> +                        intel_uc_fw_type_repr(uc_fw->type),
> +                        intel_uc_fw_status_repr(uc_fw->load_status));
> +
> +       /* Pin object with firmware */
> +       err = i915_gem_object_set_to_gtt_domain(uc_fw->obj, false);
> +       if (err) {
> +               DRM_DEBUG_DRIVER("%s fw set-domain failed %d\n",
> +                                intel_uc_fw_type_repr(uc_fw->type), err);
> +               goto fail;
> +       }
> +
> +       vma = i915_gem_object_ggtt_pin(uc_fw->obj, NULL, 0, 0,
> +                                      PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> +       if (IS_ERR(vma)) {
> +               err = PTR_ERR(vma);
> +               DRM_DEBUG_DRIVER("%s fw pin failed %d\n",
> +                                intel_uc_fw_type_repr(uc_fw->type),
> +                                err);
> +               goto fail;
> +       }
> +
> +       /* Call custom loader */
> +       err = loader(uc_fw, vma);
> +       uc_fw->load_status = err ?
> +                            INTEL_UC_FIRMWARE_FAIL :
> +                            INTEL_UC_FIRMWARE_SUCCESS;
> +       DRM_DEBUG_DRIVER("%s fw load %s\n",
> +                        intel_uc_fw_type_repr(uc_fw->type),
> +                        intel_uc_fw_status_repr(uc_fw->load_status));
> +
> +       /*
> +        * We keep the object pages for reuse during resume. But we can unpin it
> +        * now that DMA has completed, so it doesn't continue to take up space.
> +        */
> +       i915_vma_unpin(vma);
> +
> +       if (err)
> +               goto fail;
> +
> +       DRM_INFO("%s: Loaded firmware %s (version %u.%u)\n",
> +                intel_uc_fw_type_repr(uc_fw->type),
> +                uc_fw->path,
> +                uc_fw->major_ver_found, uc_fw->minor_ver_found);
> +
> +       return 0;
> +
> +fail:
> +       uc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;

?  You just set this before.

So maybe,

	err = xfer(uc_fw, vma);
	i915_vma_unpin(vma);
	if (err)
		goto fail;

	uc_fw->last_status = INTEL_UC_FIRMWARE_SUCCESS;
	return 0;

fail:
	uc_fw->last_status = INTEL_UC_FIRMWARE_FAIL;
	...
	return err;

> +       DRM_DEBUG_DRIVER("%s fw load %s\n",
> +                        intel_uc_fw_type_repr(uc_fw->type),
> +                        intel_uc_fw_status_repr(uc_fw->load_status));
> +
> +       DRM_ERROR("%s: Failed to load firmware %s (error %d)\n",
> +                 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);

It was WARN before AND NOW ITS AN ERROR! PANIC, PANIC, PANIC!

I thought you were aiming to present a clear and consistent message to
the user :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx