[Intel-gfx] [PATCH v4 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Apr 19 15:45:39 UTC 2018
On Wed, 18 Apr 2018 19:01:31 +0200, Jackie Li <yaodong.li at intel.com> wrote:
> After enabled the WOPCM write-once registers locking status checking,
> reloading of the i915 module will fail with modparam enable_guc set to 3
> (enable GuC and HuC firmware loading) if the module was originally loaded
> with enable_guc set to 1 (only enable GuC firmware loading). This is
> because WOPCM registers were updated and locked without considering the
> HuC
> FW size. Since we need both GuC and HuC FW sizes to determine the final
> layout of WOPCM, we should always calculate the WOPCM layout based on the
> actual sizes of the GuC and HuC firmware available for a specific
> platform
> if we need continue to support enable/disable HuC FW loading dynamically
> with enable_guc modparam.
>
> This patch splits uC firmware fetching into two stages. First stage is to
> fetch the firmware image and verify the firmware header. uC firmware will
> be marked as verified and this will make FW info available for following
> WOPCM layout calculation. The second stage is to create a GEM object and
> copy the FW data into the created GEM object which will only be available
> when GuC/HuC loading is enabled by enable_guc modparam. This will
> guarantee
> that the WOPCM layout will be always be calculated correctly without
> making
> any assumptions to the GuC and HuC firmware sizes.
>
> v3:
> - Rebase
>
> v4:
> - Renamed the new parameter add to intel_uc_fw_fetch (Michal)
>
> Signed-off-by: Jackie Li <yaodong.li at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Michal Winiarski <michal.winiarski at intel.com>
> Cc: John Spotswood <john.a.spotswood at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Reviewed-by: John Spotswood <john.a.spotswood at intel.com>
> ---
> drivers/gpu/drm/i915/intel_uc.c | 14 ++++----------
> drivers/gpu/drm/i915/intel_uc_fw.c | 31 ++++++++++++++++++++-----------
> drivers/gpu/drm/i915/intel_uc_fw.h | 7 +++++--
> 3 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index 1cffaf7..73b8f6c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private
> *i915)
> sanitize_options_early(i915);
> - if (USES_GUC(i915))
> - intel_uc_fw_fetch(i915, &guc->fw);
> -
> - if (USES_HUC(i915))
> - intel_uc_fw_fetch(i915, &huc->fw);
> + intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
> + intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));
Again: I'm don't think that unconditional fetch of HuC fw is a right
choice.
We should look for other options how to support enable_guc 1-->3 scenario.
> }
> void intel_uc_cleanup_early(struct drm_i915_private *i915)
> @@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct drm_i915_private
> *i915)
> struct intel_guc *guc = &i915->guc;
> struct intel_huc *huc = &i915->huc;
> - if (USES_HUC(i915))
> - intel_uc_fw_fini(&huc->fw);
> -
> - if (USES_GUC(i915))
> - intel_uc_fw_fini(&guc->fw);
> + intel_uc_fw_fini(&huc->fw);
> + intel_uc_fw_fini(&guc->fw);
> guc_free_load_err_log(guc);
> }
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c
> b/drivers/gpu/drm/i915/intel_uc_fw.c
> index 6e8e0b5..c1fed06 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
> @@ -33,11 +33,13 @@
> *
> * @dev_priv: device private
> * @uc_fw: uC firmware
> + * @fetch: whether fetch uC firmware into GEM object or not
> *
> - * Fetch uC firmware into GEM obj.
> + * Fetch and verify uC firmware and copy firmware data into GEM object
> if
> + * @fetch is true.
> */
> void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> - struct intel_uc_fw *uc_fw)
> + struct intel_uc_fw *uc_fw, bool fetch)
> {
> struct pci_dev *pdev = dev_priv->drm.pdev;
> struct drm_i915_gem_object *obj;
> @@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private
> *dev_priv,
> goto fail;
> }
> - obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
> - if (IS_ERR(obj)) {
> - err = PTR_ERR(obj);
> - DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
> - intel_uc_fw_type_repr(uc_fw->type), err);
> - goto fail;
> + uc_fw->size = fw->size;
> + uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;
btw, I'm not sure that this new state is needed at all, as you don't
plan to use that fw obj if you only loaded fw blob to read header...
> +
> + if (fetch) {
> + obj = i915_gem_object_create_from_data(dev_priv, fw->data,
> + fw->size);
> + if (IS_ERR(obj)) {
> + err = PTR_ERR(obj);
> + DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
> + intel_uc_fw_type_repr(uc_fw->type),
> + err);
> + goto fail;
> + }
> +
> + uc_fw->obj = obj;
> + uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
> }
> - uc_fw->obj = obj;
> - uc_fw->size = fw->size;
> - uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
> DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> intel_uc_fw_type_repr(uc_fw->type),
> intel_uc_fw_status_repr(uc_fw->fetch_status));
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h
> b/drivers/gpu/drm/i915/intel_uc_fw.h
> index dc33b12..a1308c0 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
> @@ -36,6 +36,7 @@ enum intel_uc_fw_status {
> INTEL_UC_FIRMWARE_FAIL = -1,
> INTEL_UC_FIRMWARE_NONE = 0,
> INTEL_UC_FIRMWARE_PENDING,
> + INTEL_UC_FIRMWARE_VERIFIED,
> INTEL_UC_FIRMWARE_SUCCESS
> };
> @@ -84,6 +85,8 @@ const char *intel_uc_fw_status_repr(enum
> intel_uc_fw_status status)
> return "NONE";
> case INTEL_UC_FIRMWARE_PENDING:
> return "PENDING";
> + case INTEL_UC_FIRMWARE_VERIFIED:
> + return "VERIFIED";
> case INTEL_UC_FIRMWARE_SUCCESS:
> return "SUCCESS";
> }
> @@ -131,14 +134,14 @@ static inline void intel_uc_fw_sanitize(struct
> intel_uc_fw *uc_fw)
> */
> static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
> {
> - if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> + if (uc_fw->fetch_status < INTEL_UC_FIRMWARE_VERIFIED)
> return 0;
> return uc_fw->header_size + uc_fw->ucode_size;
> }
> void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> - struct intel_uc_fw *uc_fw);
> + struct intel_uc_fw *uc_fw, bool fetch);
> int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
> int (*xfer)(struct intel_uc_fw *uc_fw,
> struct i915_vma *vma));
More information about the Intel-gfx
mailing list