[Intel-gfx] [PATCH 5/9] drm/i915/uc: Unify uc_fw status tracking
Michal Wajdeczko
michal.wajdeczko at intel.com
Tue Jul 23 14:20:13 UTC 2019
On Tue, 23 Jul 2019 01:20:44 +0200, Daniele Ceraolo Spurio
<daniele.ceraolospurio at intel.com> wrote:
> We currently track fetch and load status separately, but the 2 are
> actually sequential in the uc lifetime (fetch must complete before we
> can attempt the load!). Unifying the 2 variables we can better follow
> the sequential states and improve our trackng of the uC state.
>
> Also, sprinkle some GEM_BUG_ON to make sure we transition correctly
> between states.
>
> Suggested-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_huc.c | 4 +-
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 74 ++++++++++--------------
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 50 ++++++++--------
> 3 files changed, 57 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index bc14439173d7..1868f676d890 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -117,7 +117,7 @@ int intel_huc_auth(struct intel_huc *huc)
> struct intel_guc *guc = >->uc.guc;
> int ret;
> - if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> + if (huc->fw.status != INTEL_UC_FIRMWARE_LOADED)
iirc we have dedicated helper intel_uc_fw_is_loaded() for this
> return -ENOEXEC;
> ret = intel_guc_auth_huc(guc,
> @@ -141,7 +141,7 @@ int intel_huc_auth(struct intel_huc *huc)
> return 0;
> fail:
> - huc->fw.load_status = INTEL_UC_FIRMWARE_FAIL;
> + huc->fw.status = INTEL_UC_FIRMWARE_LOAD_FAIL;
hmm, so after we load bad HuC fw but before we authenticate it
we will report it as loaded (aka successful)?
maybe in addition to 'loaded' status there should add extra
'authenticated/running' status ? note that we can also load
the guc but then it can still not boot due to bad signature
> DRM_ERROR("HuC: Authentication failed %d\n", ret);
> return ret;
/.../
> @@ -95,23 +95,11 @@ void intel_uc_fw_fetch(struct drm_i915_private
> *dev_priv,
> int err;
> GEM_BUG_ON(!intel_uc_fw_supported(uc_fw));
> -
> - if (!uc_fw->path) {
> - dev_info(dev_priv->drm.dev,
> - "%s: No firmware was defined for %s!\n",
> - intel_uc_fw_type_repr(uc_fw->type),
> - intel_platform_name(INTEL_INFO(dev_priv)->platform));
> - return;
> - }
> + GEM_BUG_ON(!intel_uc_fw_is_selected(uc_fw));
> DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
I guess we can drop this debug log as request_firmware() will print
fw path on failure and actual loaded fw is printed elsewhere later
> - uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
> - 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));
> -
> err = request_firmware(&fw, uc_fw->path, &pdev->dev);
> if (err) {
> DRM_DEBUG_DRIVER("%s fw request_firmware err=%d\n",
> @@ -219,19 +207,17 @@ void intel_uc_fw_fetch(struct drm_i915_private
> *dev_priv,
> 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));
> + uc_fw->status = INTEL_UC_FIRMWARE_FETCHED;
> + DRM_DEBUG_DRIVER("%s fw fetch done\n",
> + intel_uc_fw_type_repr(uc_fw->type));
> release_firmware(fw);
> return;
> fail:
> - uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
> - 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));
> + uc_fw->status = INTEL_UC_FIRMWARE_FETCH_FAIL;
> + DRM_DEBUG_DRIVER("%s fw fetch failed\n",
> + intel_uc_fw_type_repr(uc_fw->type));
> DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
I'm wondering if we want to keep our above messages as request_firmware()
will report something like this
[ 12.198037] i915 0000:00:02.0: Direct firmware load for i915/XXX failed
with error -2
> intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
> @@ -287,13 +273,11 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
> 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 -ENOEXEC;
> + /* make sure the status was cleared the last time we reset the uc */
> + GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_LOADED);
> - 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));
> + if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
> + return -ENOEXEC;
> /* Call custom loader */
> intel_uc_fw_ggtt_bind(uc_fw);
> @@ -302,10 +286,9 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
> if (err)
> goto fail;
> - uc_fw->load_status = 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));
> + uc_fw->status = INTEL_UC_FIRMWARE_LOADED;
> + DRM_DEBUG_DRIVER("%s fw load completed\n",
> + intel_uc_fw_type_repr(uc_fw->type));
> DRM_INFO("%s: Loaded firmware %s (version %u.%u)\n",
> intel_uc_fw_type_repr(uc_fw->type),
> @@ -315,10 +298,9 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
> return 0;
> fail:
> - uc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
> - 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));
> + uc_fw->status = INTEL_UC_FIRMWARE_LOAD_FAIL;
> + DRM_DEBUG_DRIVER("%s fw load failed\n",
> + intel_uc_fw_type_repr(uc_fw->type));
maybe for debug purposes we can add helper like
static inline void intel_uc_fw_set_status(uc_fw, status)
{
#ifdef CONFIG_DRM_I915_DEBUG_GUC
DRM_DEBUG_DRIVER("%s: %s -> %s\n",
intel_uc_fw_type_repr(uc_fw->type),
intel_uc_fw_type_repr(uc_fw->status)
intel_uc_fw_type_repr(status));
#endif
uc_fw->status = status;
}
> DRM_WARN("%s: Failed to load firmware %s (error %d)\n",
> intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
> @@ -330,7 +312,10 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
> {
> int err;
> - if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> + /* this should happen before the load! */
> + GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_LOADED);
> +
> + if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
> return -ENOEXEC;
> err = i915_gem_object_pin_pages(uc_fw->obj);
> @@ -343,7 +328,7 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
> void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
> {
> - if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> + if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
> return;
> i915_gem_object_unpin_pages(uc_fw->obj);
> @@ -377,7 +362,7 @@ void intel_uc_fw_cleanup_fetch(struct intel_uc_fw
> *uc_fw)
> if (obj)
> i915_gem_object_put(obj);
> - uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> + uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE;
> }
> /**
> @@ -391,9 +376,8 @@ void intel_uc_fw_dump(const struct intel_uc_fw
> *uc_fw, struct drm_printer *p)
> {
> drm_printf(p, "%s firmware: %s\n",
> intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
> - drm_printf(p, "\tstatus: fetch %s, load %s\n",
> - intel_uc_fw_status_repr(uc_fw->fetch_status),
> - intel_uc_fw_status_repr(uc_fw->load_status));
> + drm_printf(p, "\tstatus: %s\n",
> + intel_uc_fw_status_repr(uc_fw->status));
> drm_printf(p, "\tversion: wanted %u.%u, found %u.%u\n",
> uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted,
> uc_fw->major_ver_found, uc_fw->minor_ver_found);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> index 550b626afb15..e0c5a38685e6 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> @@ -36,12 +36,13 @@ struct drm_i915_private;
> #define INTEL_UC_FIRMWARE_URL
> "https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915"
> enum intel_uc_fw_status {
> - INTEL_UC_FIRMWARE_NOT_SUPPORTED = -2, /* no uc HW */
> - INTEL_UC_FIRMWARE_FAIL = -1,
> + INTEL_UC_FIRMWARE_LOAD_FAIL = -3,
> + INTEL_UC_FIRMWARE_FETCH_FAIL = -2,
> + INTEL_UC_FIRMWARE_NOT_SUPPORTED = -1, /* no uc HW */
> INTEL_UC_FIRMWARE_UNINITIALIZED = 0, /* used to catch checks done too
> early */
> - INTEL_UC_FIRMWARE_NOT_STARTED = 1,
> - INTEL_UC_FIRMWARE_PENDING,
> - INTEL_UC_FIRMWARE_SUCCESS
> + INTEL_UC_FIRMWARE_SELECTION_DONE, /* selection include the "no FW"
> case */
> + INTEL_UC_FIRMWARE_FETCHED,
> + INTEL_UC_FIRMWARE_LOADED
I was working on something similar but my names were:
UNINITIALIZED
| \--> N/A (no hw or disabled)
DEFINED (aka selection done)
| \--> MISSING (aka fetch failed)
AVAILABLE (aka fetched)
|| \--> FAILED
UPLOADED
RUNNING (authenticated)
I was also trying to add extra flag like .auto_selected to decide
if continue with graceful fallback if something went wrong
/.../
> @@ -179,7 +181,7 @@ 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->status < INTEL_UC_FIRMWARE_FETCHED)
> return 0;
> return uc_fw->header_size + uc_fw->ucode_size;
btw, maybe we should add GEM_BUG_ON here too ?
More information about the Intel-gfx
mailing list