| Message ID | 20190724022153.8927-4-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uC fw path unification + misc clean-up | expand |
On Wed, 24 Jul 2019 04:21:48 +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. > > v2: rename states, add the running state (Michal), drop some logs in > the fetch path (Michal, Chris) > > 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_guc.h | 4 +- > drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 6 +- > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +- > drivers/gpu/drm/i915/gt/uc/intel_huc.c | 8 +- > drivers/gpu/drm/i915/gt/uc/intel_huc.h | 5 ++ > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 10 +-- > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 86 +++++++------------ > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 58 ++++++++----- > 8 files changed, 89 insertions(+), 90 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > index 6852352381ce..f51c4c3c1d0b 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > @@ -169,9 +169,9 @@ int intel_guc_suspend(struct intel_guc *guc); > int intel_guc_resume(struct intel_guc *guc); > struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 > size); > -static inline bool intel_guc_is_loaded(struct intel_guc *guc) > +static inline bool intel_guc_is_running(struct intel_guc *guc) > { > - return intel_uc_fw_is_loaded(&guc->fw); > + return intel_uc_fw_is_running(&guc->fw); > } > static inline int intel_guc_sanitize(struct intel_guc *guc) > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c > index a027deb80330..085e7842ef8a 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c > @@ -232,5 +232,9 @@ static int guc_fw_xfer(struct intel_uc_fw *guc_fw) > */ > int intel_guc_fw_upload(struct intel_guc *guc) > { > - return intel_uc_fw_upload(&guc->fw, guc_fw_xfer); > + int ret = intel_uc_fw_upload(&guc->fw, guc_fw_xfer); > + if (!ret) > + guc->fw.status = INTEL_UC_FIRMWARE_RUNNING; we should already know that in guc_fw_xfer/guc_xfer_ucode see below for details > + > + return ret; > } > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index a0f2a01365bc..b4238fe16a03 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -941,7 +941,7 @@ static void __guc_client_disable(struct > intel_guc_client *client) > * the case, instead of trying (in vain) to communicate with it, let's > * just cleanup the doorbell HW and our internal state. > */ > - if (intel_guc_is_loaded(client->guc)) > + if (intel_guc_is_running(client->guc)) > destroy_doorbell(client); > else > __fini_doorbell(client); > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c > b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > index ab6c1564b6a7..7804ea5f699c 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > @@ -117,8 +117,8 @@ 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) > - return -ENOEXEC; > + GEM_BUG_ON(!intel_uc_fw_is_loaded(&huc->fw)); > + GEM_BUG_ON(intel_huc_is_authenticated(huc)); > ret = intel_guc_auth_huc(guc, > intel_guc_ggtt_offset(guc, huc->rsa_data)); > @@ -138,10 +138,12 @@ int intel_huc_auth(struct intel_huc *huc) > goto fail; > } > + huc->fw.status = INTEL_UC_FIRMWARE_RUNNING; > + > 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_huc.h > b/drivers/gpu/drm/i915/gt/uc/intel_huc.h > index 9fa3d4629f2e..ea340f85bc46 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h > @@ -56,4 +56,9 @@ static inline int intel_huc_sanitize(struct intel_huc > *huc) > return 0; > } > +static inline bool intel_huc_is_authenticated(struct intel_huc *huc) > +{ > + return intel_uc_fw_is_running(&huc->fw); > +} > + > #endif > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index d60c56fd72e5..b761809946b1 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -559,7 +559,7 @@ void intel_uc_fini_hw(struct intel_uc *uc) > { > struct intel_guc *guc = &uc->guc; > - if (!intel_guc_is_loaded(guc)) > + if (!intel_guc_is_running(guc)) > return; > GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); > @@ -581,7 +581,7 @@ void intel_uc_reset_prepare(struct intel_uc *uc) > { > struct intel_guc *guc = &uc->guc; > - if (!intel_guc_is_loaded(guc)) > + if (!intel_guc_is_running(guc)) > return; > guc_stop_communication(guc); > @@ -593,7 +593,7 @@ void intel_uc_runtime_suspend(struct intel_uc *uc) > struct intel_guc *guc = &uc->guc; > int err; > - if (!intel_guc_is_loaded(guc)) > + if (!intel_guc_is_running(guc)) > return; > err = intel_guc_suspend(guc); > @@ -608,7 +608,7 @@ void intel_uc_suspend(struct intel_uc *uc) > struct intel_guc *guc = &uc->guc; > intel_wakeref_t wakeref; > - if (!intel_guc_is_loaded(guc)) > + if (!intel_guc_is_running(guc)) > return; > with_intel_runtime_pm(&uc_to_gt(uc)->i915->runtime_pm, wakeref) > @@ -620,7 +620,7 @@ int intel_uc_resume(struct intel_uc *uc) > struct intel_guc *guc = &uc->guc; > int err; > - if (!intel_guc_is_loaded(guc)) > + if (!intel_guc_is_running(guc)) > return 0; > guc_enable_communication(guc); > 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 48100dff466d..9fc72c2e50d1 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > @@ -130,7 +130,7 @@ __uc_fw_select(struct intel_uc_fw *uc_fw, enum > intel_platform p, u8 rev) > fw_blobs[i].first_rev); > uc_fw->path = NULL; > - uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; > + uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; > return; > } > } > @@ -139,15 +139,13 @@ __uc_fw_select(struct intel_uc_fw *uc_fw, enum > intel_platform p, u8 rev) > static void > uc_fw_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) > { > - GEM_BUG_ON(uc_fw->fetch_status != INTEL_UC_FIRMWARE_UNINITIALIZED); > + GEM_BUG_ON(uc_fw->status != INTEL_UC_FIRMWARE_UNINITIALIZED); > if (!HAS_GT_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; > @@ -156,6 +154,8 @@ uc_fw_select(struct drm_i915_private *i915, struct > intel_uc_fw *uc_fw) > uc_fw->path = i915_modparams.huc_firmware_path; > else > __uc_fw_select(uc_fw, INTEL_INFO(i915)->platform, INTEL_REVID(i915)); > + > + uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE; > } > /** > @@ -172,14 +172,13 @@ void intel_uc_fw_init_early(struct > drm_i915_private *i915, > 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; > uc_fw_select(i915, uc_fw); > @@ -204,29 +203,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; > - } > - > - 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)); > + GEM_BUG_ON(!intel_uc_fw_is_selected(uc_fw)); > err = request_firmware(&fw, uc_fw->path, &pdev->dev); > - if (err) { > - DRM_DEBUG_DRIVER("%s fw request_firmware err=%d\n", > - intel_uc_fw_type_repr(uc_fw->type), err); > + if (err) > goto fail; > - } > DRM_DEBUG_DRIVER("%s fw size %zu ptr %p\n", > intel_uc_fw_type_repr(uc_fw->type), fw->size, fw); > @@ -328,19 +309,13 @@ 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_AVAILABLE; > 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_WARN("%s: Failed to fetch firmware %s (error %d)\n", > intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err); > @@ -396,14 +371,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; > - > - 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)); > + /* make sure the status was cleared the last time we reset the uc */ > + GEM_BUG_ON(intel_uc_fw_is_loaded(uc_fw)); > + if (!intel_uc_fw_is_available(uc_fw)) > + return -ENOEXEC; > /* Call custom loader */ > intel_uc_fw_ggtt_bind(uc_fw); > err = xfer(uc_fw); > @@ -411,10 +383,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; maybe we can slightly modify xfer function agreement and use -EINPROGRESS to indicate whether fw is just loaded (HuC) or is already authenticated and running (GuC): if (!err) uc_fw->status = INTEL_UC_FIRMWARE_RUNNING; else if (err == -EINPROGRESS) uc_fw->status = INTEL_UC_FIRMWARE_LOADED; else goto fail; > + 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), > @@ -424,10 +395,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); > @@ -439,7 +409,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(intel_uc_fw_is_loaded(uc_fw)); > + > + if (!intel_uc_fw_is_available(uc_fw)) > return -ENOEXEC; > err = i915_gem_object_pin_pages(uc_fw->obj); > @@ -452,7 +425,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 (!intel_uc_fw_is_available(uc_fw)) > return; > i915_gem_object_unpin_pages(uc_fw->obj); > @@ -486,7 +459,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; > } > /** > @@ -500,9 +473,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 c2868ef15eda..ecdec4320260 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h > @@ -35,12 +35,14 @@ 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 */ why do you want to keep "No FW" case here ? when we know that there is no fw, we should not attempt to fetch it. so this is different state than "fw was selected, awaiting fetch" > + INTEL_UC_FIRMWARE_AVAILABLE, /* fetch done */ > + INTEL_UC_FIRMWARE_LOADED, /* dma xfer done */ > + INTEL_UC_FIRMWARE_RUNNING /* fw init/auth done */ > }; > enum intel_uc_fw_type { > @@ -57,8 +59,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 > @@ -83,18 +84,22 @@ 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"; nit: this is not my favorite, what was wrong with "PENDING" (known, awaiting fetch/load, look it's transient state!) "SELECTED" (shorter, applies to this fw object vs step) > + case INTEL_UC_FIRMWARE_AVAILABLE: > + return "AVAILABLE"; > + case INTEL_UC_FIRMWARE_LOADED: > + return "LOADED"; > + case INTEL_UC_FIRMWARE_RUNNING: > + return "RUNNING"; > } > return "<invalid>"; > } > @@ -113,25 +118,36 @@ static inline const char > *intel_uc_fw_type_repr(enum intel_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_available(struct intel_uc_fw *uc_fw) > +{ > + return uc_fw->status >= INTEL_UC_FIRMWARE_AVAILABLE; > +} > + > 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_is_running(struct intel_uc_fw *uc_fw) > +{ > + return uc_fw->status == INTEL_UC_FIRMWARE_RUNNING; > } > 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); shouldn't we have this check on all uc_fw_is_xxx() functions ? > + 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_AVAILABLE; > } > /** > @@ -144,7 +160,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 (!intel_uc_fw_is_available(uc_fw)) > return 0; > return uc_fw->header_size + uc_fw->ucode_size;
On 7/24/2019 5:35 AM, Michal Wajdeczko wrote: > On Wed, 24 Jul 2019 04:21:48 +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. >> >> v2: rename states, add the running state (Michal), drop some logs in >> the fetch path (Michal, Chris) >> >> 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_guc.h | 4 +- >> drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 6 +- >> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +- >> drivers/gpu/drm/i915/gt/uc/intel_huc.c | 8 +- >> drivers/gpu/drm/i915/gt/uc/intel_huc.h | 5 ++ >> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 10 +-- >> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 86 +++++++------------ >> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 58 ++++++++----- >> 8 files changed, 89 insertions(+), 90 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h >> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h >> index 6852352381ce..f51c4c3c1d0b 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h >> @@ -169,9 +169,9 @@ int intel_guc_suspend(struct intel_guc *guc); >> int intel_guc_resume(struct intel_guc *guc); >> struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 >> size); >> -static inline bool intel_guc_is_loaded(struct intel_guc *guc) >> +static inline bool intel_guc_is_running(struct intel_guc *guc) >> { >> - return intel_uc_fw_is_loaded(&guc->fw); >> + return intel_uc_fw_is_running(&guc->fw); >> } >> static inline int intel_guc_sanitize(struct intel_guc *guc) >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c >> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c >> index a027deb80330..085e7842ef8a 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c >> @@ -232,5 +232,9 @@ static int guc_fw_xfer(struct intel_uc_fw *guc_fw) >> */ >> int intel_guc_fw_upload(struct intel_guc *guc) >> { >> - return intel_uc_fw_upload(&guc->fw, guc_fw_xfer); >> + int ret = intel_uc_fw_upload(&guc->fw, guc_fw_xfer); >> + if (!ret) >> + guc->fw.status = INTEL_UC_FIRMWARE_RUNNING; > > we should already know that in guc_fw_xfer/guc_xfer_ucode > see below for details > >> + >> + return ret; >> } >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >> index a0f2a01365bc..b4238fe16a03 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >> @@ -941,7 +941,7 @@ static void __guc_client_disable(struct >> intel_guc_client *client) >> * the case, instead of trying (in vain) to communicate with it, >> let's >> * just cleanup the doorbell HW and our internal state. >> */ >> - if (intel_guc_is_loaded(client->guc)) >> + if (intel_guc_is_running(client->guc)) >> destroy_doorbell(client); >> else >> __fini_doorbell(client); >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >> index ab6c1564b6a7..7804ea5f699c 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >> @@ -117,8 +117,8 @@ 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) >> - return -ENOEXEC; >> + GEM_BUG_ON(!intel_uc_fw_is_loaded(&huc->fw)); >> + GEM_BUG_ON(intel_huc_is_authenticated(huc)); >> ret = intel_guc_auth_huc(guc, >> intel_guc_ggtt_offset(guc, huc->rsa_data)); >> @@ -138,10 +138,12 @@ int intel_huc_auth(struct intel_huc *huc) >> goto fail; >> } >> + huc->fw.status = INTEL_UC_FIRMWARE_RUNNING; >> + >> 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_huc.h >> b/drivers/gpu/drm/i915/gt/uc/intel_huc.h >> index 9fa3d4629f2e..ea340f85bc46 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h >> @@ -56,4 +56,9 @@ static inline int intel_huc_sanitize(struct >> intel_huc *huc) >> return 0; >> } >> +static inline bool intel_huc_is_authenticated(struct intel_huc *huc) >> +{ >> + return intel_uc_fw_is_running(&huc->fw); >> +} >> + >> #endif >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> index d60c56fd72e5..b761809946b1 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> @@ -559,7 +559,7 @@ void intel_uc_fini_hw(struct intel_uc *uc) >> { >> struct intel_guc *guc = &uc->guc; >> - if (!intel_guc_is_loaded(guc)) >> + if (!intel_guc_is_running(guc)) >> return; >> GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); >> @@ -581,7 +581,7 @@ void intel_uc_reset_prepare(struct intel_uc *uc) >> { >> struct intel_guc *guc = &uc->guc; >> - if (!intel_guc_is_loaded(guc)) >> + if (!intel_guc_is_running(guc)) >> return; >> guc_stop_communication(guc); >> @@ -593,7 +593,7 @@ void intel_uc_runtime_suspend(struct intel_uc *uc) >> struct intel_guc *guc = &uc->guc; >> int err; >> - if (!intel_guc_is_loaded(guc)) >> + if (!intel_guc_is_running(guc)) >> return; >> err = intel_guc_suspend(guc); >> @@ -608,7 +608,7 @@ void intel_uc_suspend(struct intel_uc *uc) >> struct intel_guc *guc = &uc->guc; >> intel_wakeref_t wakeref; >> - if (!intel_guc_is_loaded(guc)) >> + if (!intel_guc_is_running(guc)) >> return; >> with_intel_runtime_pm(&uc_to_gt(uc)->i915->runtime_pm, wakeref) >> @@ -620,7 +620,7 @@ int intel_uc_resume(struct intel_uc *uc) >> struct intel_guc *guc = &uc->guc; >> int err; >> - if (!intel_guc_is_loaded(guc)) >> + if (!intel_guc_is_running(guc)) >> return 0; >> guc_enable_communication(guc); >> 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 48100dff466d..9fc72c2e50d1 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> @@ -130,7 +130,7 @@ __uc_fw_select(struct intel_uc_fw *uc_fw, enum >> intel_platform p, u8 rev) >> fw_blobs[i].first_rev); >> uc_fw->path = NULL; >> - uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; >> + uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; >> return; >> } >> } >> @@ -139,15 +139,13 @@ __uc_fw_select(struct intel_uc_fw *uc_fw, enum >> intel_platform p, u8 rev) >> static void >> uc_fw_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) >> { >> - GEM_BUG_ON(uc_fw->fetch_status != INTEL_UC_FIRMWARE_UNINITIALIZED); >> + GEM_BUG_ON(uc_fw->status != INTEL_UC_FIRMWARE_UNINITIALIZED); >> if (!HAS_GT_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; >> @@ -156,6 +154,8 @@ uc_fw_select(struct drm_i915_private *i915, >> struct intel_uc_fw *uc_fw) >> uc_fw->path = i915_modparams.huc_firmware_path; >> else >> __uc_fw_select(uc_fw, INTEL_INFO(i915)->platform, >> INTEL_REVID(i915)); >> + >> + uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE; >> } >> /** >> @@ -172,14 +172,13 @@ void intel_uc_fw_init_early(struct >> drm_i915_private *i915, >> 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; >> uc_fw_select(i915, uc_fw); >> @@ -204,29 +203,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; >> - } >> - >> - 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)); >> + GEM_BUG_ON(!intel_uc_fw_is_selected(uc_fw)); >> err = request_firmware(&fw, uc_fw->path, &pdev->dev); >> - if (err) { >> - DRM_DEBUG_DRIVER("%s fw request_firmware err=%d\n", >> - intel_uc_fw_type_repr(uc_fw->type), err); >> + if (err) >> goto fail; >> - } >> DRM_DEBUG_DRIVER("%s fw size %zu ptr %p\n", >> intel_uc_fw_type_repr(uc_fw->type), fw->size, fw); >> @@ -328,19 +309,13 @@ 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_AVAILABLE; >> 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_WARN("%s: Failed to fetch firmware %s (error %d)\n", >> intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err); >> @@ -396,14 +371,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; >> - >> - 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)); >> + /* make sure the status was cleared the last time we reset the >> uc */ >> + GEM_BUG_ON(intel_uc_fw_is_loaded(uc_fw)); >> + if (!intel_uc_fw_is_available(uc_fw)) >> + return -ENOEXEC; >> /* Call custom loader */ >> intel_uc_fw_ggtt_bind(uc_fw); >> err = xfer(uc_fw); >> @@ -411,10 +383,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; > > maybe we can slightly modify xfer function agreement and use > -EINPROGRESS to indicate whether fw is just loaded (HuC) or > is already authenticated and running (GuC): > > if (!err) > uc_fw->status = INTEL_UC_FIRMWARE_RUNNING; > else if (err == -EINPROGRESS) > uc_fw->status = INTEL_UC_FIRMWARE_LOADED; > else > goto fail; > I've purposely kept the RUNNING state outside because in patch 8 I move the wait outside the xfer, so the switch to the running state will be done outside of here for both uC. Seemed like less churn to go directly with that. >> + 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), >> @@ -424,10 +395,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); >> @@ -439,7 +409,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(intel_uc_fw_is_loaded(uc_fw)); >> + >> + if (!intel_uc_fw_is_available(uc_fw)) >> return -ENOEXEC; >> err = i915_gem_object_pin_pages(uc_fw->obj); >> @@ -452,7 +425,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 (!intel_uc_fw_is_available(uc_fw)) >> return; >> i915_gem_object_unpin_pages(uc_fw->obj); >> @@ -486,7 +459,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; >> } >> /** >> @@ -500,9 +473,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 c2868ef15eda..ecdec4320260 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h >> @@ -35,12 +35,14 @@ 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 */ > > why do you want to keep "No FW" case here ? > when we know that there is no fw, we should not attempt to fetch it. > so this is different state than "fw was selected, awaiting fetch" We need a way to differentiate for the logging and I didn't want an extra state since we check fw->path anyway to make sure the fw was actually selected. > >> + INTEL_UC_FIRMWARE_AVAILABLE, /* fetch done */ >> + INTEL_UC_FIRMWARE_LOADED, /* dma xfer done */ >> + INTEL_UC_FIRMWARE_RUNNING /* fw init/auth done */ >> }; >> enum intel_uc_fw_type { >> @@ -57,8 +59,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 >> @@ -83,18 +84,22 @@ 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"; > > nit: this is not my favorite, what was wrong with > "PENDING" (known, awaiting fetch/load, look it's transient state!) > "SELECTED" (shorter, applies to this fw object vs step) I wanted to highlight the fact that the selection included the "no FW" case, the fw wasn't necessarily "selected". We just know that we've run through the selection code. > >> + case INTEL_UC_FIRMWARE_AVAILABLE: >> + return "AVAILABLE"; >> + case INTEL_UC_FIRMWARE_LOADED: >> + return "LOADED"; >> + case INTEL_UC_FIRMWARE_RUNNING: >> + return "RUNNING"; >> } >> return "<invalid>"; >> } >> @@ -113,25 +118,36 @@ static inline const char >> *intel_uc_fw_type_repr(enum intel_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_available(struct intel_uc_fw *uc_fw) >> +{ >> + return uc_fw->status >= INTEL_UC_FIRMWARE_AVAILABLE; >> +} >> + >> 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_is_running(struct intel_uc_fw *uc_fw) >> +{ >> + return uc_fw->status == INTEL_UC_FIRMWARE_RUNNING; >> } >> 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); > > shouldn't we have this check on all uc_fw_is_xxx() functions ? > I can add that in. Daniele >> + 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_AVAILABLE; >> } >> /** >> @@ -144,7 +160,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 (!intel_uc_fw_is_available(uc_fw)) >> return 0; >> return uc_fw->header_size + uc_fw->ucode_size;
On Wed, 24 Jul 2019 18:37:52 +0200, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote: >>> - 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; >> >> maybe we can slightly modify xfer function agreement and use >> -EINPROGRESS to indicate whether fw is just loaded (HuC) or >> is already authenticated and running (GuC): >> >> if (!err) >> uc_fw->status = INTEL_UC_FIRMWARE_RUNNING; >> else if (err == -EINPROGRESS) >> uc_fw->status = INTEL_UC_FIRMWARE_LOADED; >> else >> goto fail; >> > > I've purposely kept the RUNNING state outside because in patch 8 I move > the wait outside the xfer, so the switch to the running state will be > done outside of here for both uC. Seemed like less churn to go directly > with that. ok, I missed that move in diff 8/8 >>> @@ -35,12 +35,14 @@ 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 */ >> >> why do you want to keep "No FW" case here ? >> when we know that there is no fw, we should not attempt to fetch it. >> so this is different state than "fw was selected, awaiting fetch" > > We need a way to differentiate for the logging and I didn't want an > extra state since we check fw->path anyway to make sure the fw was > actually selected. But "N/A" state also means that we already pass over init_early step that includes selection, so we don't need to add any extra state. > >> >>> + INTEL_UC_FIRMWARE_AVAILABLE, /* fetch done */ >>> + INTEL_UC_FIRMWARE_LOADED, /* dma xfer done */ >>> + INTEL_UC_FIRMWARE_RUNNING /* fw init/auth done */ >>> }; >>> enum intel_uc_fw_type { >>> @@ -57,8 +59,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 >>> @@ -83,18 +84,22 @@ 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"; sorry for second thoughts, but with these names we could have: LOADED (user: hurray!) --> NOT_LOADED (user: but we were already loaded?!?) so maybe plain "FAIL" as this is user facing status ? >>> + case INTEL_UC_FIRMWARE_FETCH_FAIL: >>> + return "FETCH FAIL"; same here, "fetch" it's name of our internal step, "MISSING" sounds better imno >>> 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"; >> >> nit: this is not my favorite, what was wrong with >> "PENDING" (known, awaiting fetch/load, look it's transient state!) >> "SELECTED" (shorter, applies to this fw object vs step) > > I wanted to highlight the fact that the selection included the "no FW" > case, the fw wasn't necessarily "selected". We just know that we've run > through the selection code. but from the user pov this is internal detail, not sure if we should expose that, on other hand, PENDING clearly indicates that we are still going to do something with that firmware (fetch/xfer/auth) until we reach end state. > >> >>> + case INTEL_UC_FIRMWARE_AVAILABLE: >>> + return "AVAILABLE"; >>> + case INTEL_UC_FIRMWARE_LOADED: >>> + return "LOADED"; >>> + case INTEL_UC_FIRMWARE_RUNNING: >>> + return "RUNNING"; hmm, the difference between LOADED/RUNNING might be unnoticed by the user, as he may also treat LOADED as full success. so maybe s/LOADED/TRANSFERRED ? >>> } >>> return "<invalid>"; >>> }
On 7/24/19 10:24 AM, Michal Wajdeczko wrote: > On Wed, 24 Jul 2019 18:37:52 +0200, Daniele Ceraolo Spurio > <daniele.ceraolospurio@intel.com> wrote: > >>>> - 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; >>> >>> maybe we can slightly modify xfer function agreement and use >>> -EINPROGRESS to indicate whether fw is just loaded (HuC) or >>> is already authenticated and running (GuC): >>> >>> if (!err) >>> uc_fw->status = INTEL_UC_FIRMWARE_RUNNING; >>> else if (err == -EINPROGRESS) >>> uc_fw->status = INTEL_UC_FIRMWARE_LOADED; >>> else >>> goto fail; >>> >> >> I've purposely kept the RUNNING state outside because in patch 8 I >> move the wait outside the xfer, so the switch to the running state >> will be done outside of here for both uC. Seemed like less churn to go >> directly with that. > > ok, I missed that move in diff 8/8 > > >>>> @@ -35,12 +35,14 @@ 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 */ >>> >>> why do you want to keep "No FW" case here ? >>> when we know that there is no fw, we should not attempt to fetch it. >>> so this is different state than "fw was selected, awaiting fetch" >> >> We need a way to differentiate for the logging and I didn't want an >> extra state since we check fw->path anyway to make sure the fw was >> actually selected. > > But "N/A" state also means that we already pass over init_early step > that includes selection, so we don't need to add any extra state. > Yes, but we wouldn't know if N/A was set because we are on a platform with no uC HW or because the FW was not defined. I'm going to drop that distinction in the logs and be done with it, it's quite easy to find out based on the gen anyway (anything gen9+ has GuC/HuC HW) >> >>> >>>> + INTEL_UC_FIRMWARE_AVAILABLE, /* fetch done */ >>>> + INTEL_UC_FIRMWARE_LOADED, /* dma xfer done */ >>>> + INTEL_UC_FIRMWARE_RUNNING /* fw init/auth done */ >>>> }; >>>> enum intel_uc_fw_type { >>>> @@ -57,8 +59,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 >>>> @@ -83,18 +84,22 @@ 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"; > > sorry for second thoughts, but with these names we could have: > > LOADED (user: hurray!) --> NOT_LOADED (user: but we were already loaded?!?) > > so maybe plain "FAIL" as this is user facing status ? > ok >>>> + case INTEL_UC_FIRMWARE_FETCH_FAIL: >>>> + return "FETCH FAIL"; > > same here, "fetch" it's name of our internal step, > "MISSING" sounds better imno > ok >>>> 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"; >>> >>> nit: this is not my favorite, what was wrong with >>> "PENDING" (known, awaiting fetch/load, look it's transient state!) >>> "SELECTED" (shorter, applies to this fw object vs step) >> >> I wanted to highlight the fact that the selection included the "no FW" >> case, the fw wasn't necessarily "selected". We just know that we've >> run through the selection code. > > but from the user pov this is internal detail, not sure if we should expose > that, on other hand, PENDING clearly indicates that we are still going > to do > something with that firmware (fetch/xfer/auth) until we reach end state. > >> >>> >>>> + case INTEL_UC_FIRMWARE_AVAILABLE: >>>> + return "AVAILABLE"; >>>> + case INTEL_UC_FIRMWARE_LOADED: >>>> + return "LOADED"; >>>> + case INTEL_UC_FIRMWARE_RUNNING: >>>> + return "RUNNING"; > > hmm, the difference between LOADED/RUNNING might be unnoticed by the > user, as he may also treat LOADED as full success. > > so maybe s/LOADED/TRANSFERRED ? > ok Daniele >>>> } >>>> return "<invalid>"; >>>> }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 6852352381ce..f51c4c3c1d0b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -169,9 +169,9 @@ int intel_guc_suspend(struct intel_guc *guc); int intel_guc_resume(struct intel_guc *guc); struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size); -static inline bool intel_guc_is_loaded(struct intel_guc *guc) +static inline bool intel_guc_is_running(struct intel_guc *guc) { - return intel_uc_fw_is_loaded(&guc->fw); + return intel_uc_fw_is_running(&guc->fw); } static inline int intel_guc_sanitize(struct intel_guc *guc) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c index a027deb80330..085e7842ef8a 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c @@ -232,5 +232,9 @@ static int guc_fw_xfer(struct intel_uc_fw *guc_fw) */ int intel_guc_fw_upload(struct intel_guc *guc) { - return intel_uc_fw_upload(&guc->fw, guc_fw_xfer); + int ret = intel_uc_fw_upload(&guc->fw, guc_fw_xfer); + if (!ret) + guc->fw.status = INTEL_UC_FIRMWARE_RUNNING; + + return ret; } diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index a0f2a01365bc..b4238fe16a03 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -941,7 +941,7 @@ static void __guc_client_disable(struct intel_guc_client *client) * the case, instead of trying (in vain) to communicate with it, let's * just cleanup the doorbell HW and our internal state. */ - if (intel_guc_is_loaded(client->guc)) + if (intel_guc_is_running(client->guc)) destroy_doorbell(client); else __fini_doorbell(client); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index ab6c1564b6a7..7804ea5f699c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -117,8 +117,8 @@ 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) - return -ENOEXEC; + GEM_BUG_ON(!intel_uc_fw_is_loaded(&huc->fw)); + GEM_BUG_ON(intel_huc_is_authenticated(huc)); ret = intel_guc_auth_huc(guc, intel_guc_ggtt_offset(guc, huc->rsa_data)); @@ -138,10 +138,12 @@ int intel_huc_auth(struct intel_huc *huc) goto fail; } + huc->fw.status = INTEL_UC_FIRMWARE_RUNNING; + 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_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h index 9fa3d4629f2e..ea340f85bc46 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h @@ -56,4 +56,9 @@ static inline int intel_huc_sanitize(struct intel_huc *huc) return 0; } +static inline bool intel_huc_is_authenticated(struct intel_huc *huc) +{ + return intel_uc_fw_is_running(&huc->fw); +} + #endif diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index d60c56fd72e5..b761809946b1 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -559,7 +559,7 @@ void intel_uc_fini_hw(struct intel_uc *uc) { struct intel_guc *guc = &uc->guc; - if (!intel_guc_is_loaded(guc)) + if (!intel_guc_is_running(guc)) return; GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); @@ -581,7 +581,7 @@ void intel_uc_reset_prepare(struct intel_uc *uc) { struct intel_guc *guc = &uc->guc; - if (!intel_guc_is_loaded(guc)) + if (!intel_guc_is_running(guc)) return; guc_stop_communication(guc); @@ -593,7 +593,7 @@ void intel_uc_runtime_suspend(struct intel_uc *uc) struct intel_guc *guc = &uc->guc; int err; - if (!intel_guc_is_loaded(guc)) + if (!intel_guc_is_running(guc)) return; err = intel_guc_suspend(guc); @@ -608,7 +608,7 @@ void intel_uc_suspend(struct intel_uc *uc) struct intel_guc *guc = &uc->guc; intel_wakeref_t wakeref; - if (!intel_guc_is_loaded(guc)) + if (!intel_guc_is_running(guc)) return; with_intel_runtime_pm(&uc_to_gt(uc)->i915->runtime_pm, wakeref) @@ -620,7 +620,7 @@ int intel_uc_resume(struct intel_uc *uc) struct intel_guc *guc = &uc->guc; int err; - if (!intel_guc_is_loaded(guc)) + if (!intel_guc_is_running(guc)) return 0; guc_enable_communication(guc); 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 48100dff466d..9fc72c2e50d1 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -130,7 +130,7 @@ __uc_fw_select(struct intel_uc_fw *uc_fw, enum intel_platform p, u8 rev) fw_blobs[i].first_rev); uc_fw->path = NULL; - uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; + uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; return; } } @@ -139,15 +139,13 @@ __uc_fw_select(struct intel_uc_fw *uc_fw, enum intel_platform p, u8 rev) static void uc_fw_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) { - GEM_BUG_ON(uc_fw->fetch_status != INTEL_UC_FIRMWARE_UNINITIALIZED); + GEM_BUG_ON(uc_fw->status != INTEL_UC_FIRMWARE_UNINITIALIZED); if (!HAS_GT_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; @@ -156,6 +154,8 @@ uc_fw_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) uc_fw->path = i915_modparams.huc_firmware_path; else __uc_fw_select(uc_fw, INTEL_INFO(i915)->platform, INTEL_REVID(i915)); + + uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE; } /** @@ -172,14 +172,13 @@ void intel_uc_fw_init_early(struct drm_i915_private *i915, 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; uc_fw_select(i915, uc_fw); @@ -204,29 +203,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; - } - - 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)); + GEM_BUG_ON(!intel_uc_fw_is_selected(uc_fw)); err = request_firmware(&fw, uc_fw->path, &pdev->dev); - if (err) { - DRM_DEBUG_DRIVER("%s fw request_firmware err=%d\n", - intel_uc_fw_type_repr(uc_fw->type), err); + if (err) goto fail; - } DRM_DEBUG_DRIVER("%s fw size %zu ptr %p\n", intel_uc_fw_type_repr(uc_fw->type), fw->size, fw); @@ -328,19 +309,13 @@ 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_AVAILABLE; 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_WARN("%s: Failed to fetch firmware %s (error %d)\n", intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err); @@ -396,14 +371,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; - - 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)); + /* make sure the status was cleared the last time we reset the uc */ + GEM_BUG_ON(intel_uc_fw_is_loaded(uc_fw)); + if (!intel_uc_fw_is_available(uc_fw)) + return -ENOEXEC; /* Call custom loader */ intel_uc_fw_ggtt_bind(uc_fw); err = xfer(uc_fw); @@ -411,10 +383,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), @@ -424,10 +395,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); @@ -439,7 +409,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(intel_uc_fw_is_loaded(uc_fw)); + + if (!intel_uc_fw_is_available(uc_fw)) return -ENOEXEC; err = i915_gem_object_pin_pages(uc_fw->obj); @@ -452,7 +425,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 (!intel_uc_fw_is_available(uc_fw)) return; i915_gem_object_unpin_pages(uc_fw->obj); @@ -486,7 +459,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; } /** @@ -500,9 +473,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 c2868ef15eda..ecdec4320260 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h @@ -35,12 +35,14 @@ 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_AVAILABLE, /* fetch done */ + INTEL_UC_FIRMWARE_LOADED, /* dma xfer done */ + INTEL_UC_FIRMWARE_RUNNING /* fw init/auth done */ }; enum intel_uc_fw_type { @@ -57,8 +59,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 @@ -83,18 +84,22 @@ 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_AVAILABLE: + return "AVAILABLE"; + case INTEL_UC_FIRMWARE_LOADED: + return "LOADED"; + case INTEL_UC_FIRMWARE_RUNNING: + return "RUNNING"; } return "<invalid>"; } @@ -113,25 +118,36 @@ static inline const char *intel_uc_fw_type_repr(enum intel_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_available(struct intel_uc_fw *uc_fw) +{ + return uc_fw->status >= INTEL_UC_FIRMWARE_AVAILABLE; +} + 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_is_running(struct intel_uc_fw *uc_fw) +{ + return uc_fw->status == INTEL_UC_FIRMWARE_RUNNING; } 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_AVAILABLE; } /** @@ -144,7 +160,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 (!intel_uc_fw_is_available(uc_fw)) 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. v2: rename states, add the running state (Michal), drop some logs in the fetch path (Michal, Chris) 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_guc.h | 4 +- drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 6 +- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +- drivers/gpu/drm/i915/gt/uc/intel_huc.c | 8 +- drivers/gpu/drm/i915/gt/uc/intel_huc.h | 5 ++ drivers/gpu/drm/i915/gt/uc/intel_uc.c | 10 +-- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 86 +++++++------------ drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 58 ++++++++----- 8 files changed, 89 insertions(+), 90 deletions(-)