Message ID | 20190722232048.9970-6-daniele.ceraolospurio@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | uC fw path unification + misc clean-up | expand |
Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:44) > @@ -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)); I don't see much value in the success message, they just get followed by more success messages. Pointless spam imo. -Chris
On Tue, 23 Jul 2019 01:20:44 +0200, Daniele Ceraolo Spurio <daniele.ceraolospurio@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@intel.com> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@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 ?
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) 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; DRM_ERROR("HuC: Authentication failed %d\n", ret); return ret; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c index faa96748aed3..91b1d9663481 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -45,15 +45,13 @@ void intel_uc_fw_select(struct drm_i915_private *i915, const struct intel_uc_platform_requirement *list, unsigned int length) { - GEM_BUG_ON(uc_fw->fetch_status != INTEL_UC_FIRMWARE_UNINITIALIZED); + GEM_BUG_ON(uc_fw->status != INTEL_UC_FIRMWARE_UNINITIALIZED); if (!HAS_UC(i915)) { - uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; + uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; return; } - uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED; - if (unlikely(i915_modparams.guc_firmware_path && uc_fw->type == INTEL_UC_FW_TYPE_GUC)) { uc_fw->path = i915_modparams.guc_firmware_path; @@ -74,6 +72,8 @@ void intel_uc_fw_select(struct drm_i915_private *i915, } } } + + uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE; } /** @@ -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); - 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", 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)); 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 }; enum intel_uc_fw_type { @@ -77,8 +78,7 @@ struct intel_uc_fw { const char *path; size_t size; struct drm_i915_gem_object *obj; - enum intel_uc_fw_status fetch_status; - enum intel_uc_fw_status load_status; + enum intel_uc_fw_status status; /* * The firmware build process will generate a version header file with major and @@ -103,18 +103,20 @@ static inline const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status) { switch (status) { + case INTEL_UC_FIRMWARE_LOAD_FAIL: + return "LOAD FAIL"; + case INTEL_UC_FIRMWARE_FETCH_FAIL: + return "FETCH FAIL"; case INTEL_UC_FIRMWARE_NOT_SUPPORTED: - return "N/A - uc HW not available"; - case INTEL_UC_FIRMWARE_FAIL: - return "FAIL"; + return "N/A"; case INTEL_UC_FIRMWARE_UNINITIALIZED: return "UNINITIALIZED"; - case INTEL_UC_FIRMWARE_NOT_STARTED: - return "NOT_STARTED"; - case INTEL_UC_FIRMWARE_PENDING: - return "PENDING"; - case INTEL_UC_FIRMWARE_SUCCESS: - return "SUCCESS"; + case INTEL_UC_FIRMWARE_SELECTION_DONE: + return "SELECTION DONE"; + case INTEL_UC_FIRMWARE_FETCHED: + return "FETCHED"; + case INTEL_UC_FIRMWARE_LOADED: + return "LOADED"; } return "<invalid>"; } @@ -135,38 +137,38 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw, enum intel_uc_fw_type type) { /* - * we use FIRMWARE_UNINITIALIZED to detect checks against fetch_status + * we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status * before we're looked at the HW caps to see if we have uc support */ BUILD_BUG_ON(INTEL_UC_FIRMWARE_UNINITIALIZED); uc_fw->path = NULL; - uc_fw->fetch_status = INTEL_UC_FIRMWARE_UNINITIALIZED; - uc_fw->load_status = INTEL_UC_FIRMWARE_NOT_STARTED; + uc_fw->status = INTEL_UC_FIRMWARE_UNINITIALIZED; uc_fw->type = type; } static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw) { + GEM_BUG_ON(uc_fw->path && uc_fw->status < INTEL_UC_FIRMWARE_SELECTION_DONE); return uc_fw->path != NULL; } static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw) { - return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS; + return uc_fw->status == INTEL_UC_FIRMWARE_LOADED; } static inline bool intel_uc_fw_supported(struct intel_uc_fw *uc_fw) { /* shouldn't call this before checking hw/blob availability */ - GEM_BUG_ON(uc_fw->fetch_status == INTEL_UC_FIRMWARE_UNINITIALIZED); - return uc_fw->fetch_status != INTEL_UC_FIRMWARE_NOT_SUPPORTED; + GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_UNINITIALIZED); + return uc_fw->status != INTEL_UC_FIRMWARE_NOT_SUPPORTED; } static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) { if (intel_uc_fw_is_loaded(uc_fw)) - uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING; + uc_fw->status = INTEL_UC_FIRMWARE_FETCHED; } /** @@ -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;
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@intel.com> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@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(-)