| Message ID | 20190710005437.3496-5-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | GT-fy the uc code | expand |
On Wed, 10 Jul 2019 02:54:29 +0200, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote: > Instead of always checking in the device config is GuC and HuC are s/is/if > supported or not, we can save the state in the uc_fw structure and > avoid going through i915 every time from the low-level uc management > code. while at it FIRMWARE_NONE has been renamed to better indicate that s/while/While > we haven't started the fetch/load yet, but we might have already selected > a blob. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > --- > drivers/gpu/drm/i915/intel_guc_fw.c | 6 +++++- > drivers/gpu/drm/i915/intel_huc_fw.c | 6 +++++- > drivers/gpu/drm/i915/intel_uc.c | 25 ++++++++++++------------ > drivers/gpu/drm/i915/intel_uc_fw.c | 4 +++- > drivers/gpu/drm/i915/intel_uc_fw.h | 30 ++++++++++++++++++++++++----- > 5 files changed, 51 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c > b/drivers/gpu/drm/i915/intel_guc_fw.c > index db1e0daca7db..ee95d4960c5c 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fw.c > +++ b/drivers/gpu/drm/i915/intel_guc_fw.c > @@ -79,8 +79,12 @@ static void guc_fw_select(struct intel_uc_fw *guc_fw) > GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC); > - if (!HAS_GUC(i915)) > + if (!HAS_GUC(i915)) { > + guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; > return; > + } > + > + guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED; > if (i915_modparams.guc_firmware_path) { > guc_fw->path = i915_modparams.guc_firmware_path; > diff --git a/drivers/gpu/drm/i915/intel_huc_fw.c > b/drivers/gpu/drm/i915/intel_huc_fw.c > index 05cbf8338f53..06e726ba9863 100644 > --- a/drivers/gpu/drm/i915/intel_huc_fw.c > +++ b/drivers/gpu/drm/i915/intel_huc_fw.c > @@ -73,8 +73,12 @@ static void huc_fw_select(struct intel_uc_fw *huc_fw) > GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC); > - if (!HAS_HUC(dev_priv)) > + if (!HAS_HUC(dev_priv)) { > + huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; > return; > + } > + > + huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED; > if (i915_modparams.huc_firmware_path) { > huc_fw->path = i915_modparams.huc_firmware_path; > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index 789b0bccfb41..ef2a864b8990 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -71,7 +71,8 @@ static int __get_default_guc_log_level(struct > drm_i915_private *i915) > { > int guc_log_level; > - if (!HAS_GUC(i915) || !intel_uc_is_using_guc(i915)) > + if (!intel_uc_fw_supported(&i915->guc.fw) || this goes too far, we should limit number of direct accesses to .fw maybe we can have: inline bool intel_uc_has_guc(i915) { return intel_guc_is_present(&i915->guc); } inline bool intel_guc_is_present(guc) { return intel_uc_fw_is_defined(&guc->fw); } > + !intel_uc_is_using_guc(i915)) > guc_log_level = GUC_LOG_LEVEL_DISABLED; > else if (IS_ENABLED(CONFIG_DRM_I915_DEBUG) || > IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) > @@ -119,16 +120,16 @@ static void sanitize_options_early(struct > drm_i915_private *i915) > if (intel_uc_is_using_guc(i915) && !intel_uc_fw_is_selected(guc_fw)) { > DRM_WARN("Incompatible option detected: %s=%d, %s!\n", > "enable_guc", i915_modparams.enable_guc, > - !HAS_GUC(i915) ? "no GuC hardware" : > - "no GuC firmware"); > + !intel_uc_fw_supported(guc_fw) ? > + "no GuC hardware" : "no GuC firmware"); > } > /* Verify HuC firmware availability */ > if (intel_uc_is_using_huc(i915) && !intel_uc_fw_is_selected(huc_fw)) { > DRM_WARN("Incompatible option detected: %s=%d, %s!\n", > "enable_guc", i915_modparams.enable_guc, > - !HAS_HUC(i915) ? "no HuC hardware" : > - "no HuC firmware"); > + !intel_uc_fw_supported(huc_fw) ? > + "no HuC hardware" : "no HuC firmware"); > } > /* XXX: GuC submission is unavailable for now */ > @@ -148,8 +149,8 @@ static void sanitize_options_early(struct > drm_i915_private *i915) > if (i915_modparams.guc_log_level > 0 && !intel_uc_is_using_guc(i915)) { > DRM_WARN("Incompatible option detected: %s=%d, %s!\n", > "guc_log_level", i915_modparams.guc_log_level, > - !HAS_GUC(i915) ? "no GuC hardware" : > - "GuC not enabled"); > + !intel_uc_fw_supported(guc_fw) ? > + "no GuC hardware" : "GuC not enabled"); > i915_modparams.guc_log_level = 0; > } > @@ -376,7 +377,7 @@ int intel_uc_init(struct drm_i915_private *i915) > if (!USES_GUC(i915)) > return 0; > - if (!HAS_GUC(i915)) > + if (!intel_uc_fw_supported(&guc->fw)) > return -ENODEV; > /* XXX: GuC submission is unavailable for now */ > @@ -419,7 +420,7 @@ void intel_uc_fini(struct drm_i915_private *i915) > if (!USES_GUC(i915)) > return; > - GEM_BUG_ON(!HAS_GUC(i915)); > + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); > if (USES_GUC_SUBMISSION(i915)) > intel_guc_submission_fini(guc); > @@ -435,7 +436,7 @@ static void __uc_sanitize(struct drm_i915_private > *i915) > struct intel_guc *guc = &i915->guc; > struct intel_huc *huc = &i915->huc; > - GEM_BUG_ON(!HAS_GUC(i915)); > + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); > intel_huc_sanitize(huc); > intel_guc_sanitize(guc); > @@ -460,7 +461,7 @@ int intel_uc_init_hw(struct drm_i915_private *i915) > if (!USES_GUC(i915)) > return 0; > - GEM_BUG_ON(!HAS_GUC(i915)); > + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); > guc_reset_interrupts(guc); > @@ -557,7 +558,7 @@ void intel_uc_fini_hw(struct drm_i915_private *i915) > if (!intel_guc_is_loaded(guc)) > return; > - GEM_BUG_ON(!HAS_GUC(i915)); > + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); > if (USES_GUC_SUBMISSION(i915)) > intel_guc_submission_disable(guc); > diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c > b/drivers/gpu/drm/i915/intel_uc_fw.c > index f342ddd47df8..8ce7210907c0 100644 > --- a/drivers/gpu/drm/i915/intel_uc_fw.c > +++ b/drivers/gpu/drm/i915/intel_uc_fw.c > @@ -47,6 +47,8 @@ void intel_uc_fw_fetch(struct drm_i915_private > *dev_priv, > size_t size; > 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", > @@ -328,7 +330,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_NONE; > + uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED; > } > /** > diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h > b/drivers/gpu/drm/i915/intel_uc_fw.h > index 24e66469153c..833d04d06576 100644 > --- a/drivers/gpu/drm/i915/intel_uc_fw.h > +++ b/drivers/gpu/drm/i915/intel_uc_fw.h > @@ -26,6 +26,7 @@ > #define _INTEL_UC_FW_H_ > #include <linux/types.h> > +#include "i915_gem.h" > struct drm_printer; > struct drm_i915_private; > @@ -34,8 +35,10 @@ 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_NONE = 0, > + 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 > }; > @@ -79,10 +82,14 @@ static inline > const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status) > { > switch (status) { > + case INTEL_UC_FIRMWARE_NOT_SUPPORTED: maybe INTEL_UC_FIRMWARE_NOT_APPLICABLE, as used in string below? > + return "N/A - uc HW not available"; note that other states have short strings, so just "N/A" ? > case INTEL_UC_FIRMWARE_FAIL: > return "FAIL"; > - case INTEL_UC_FIRMWARE_NONE: > - return "NONE"; > + case INTEL_UC_FIRMWARE_UNINITIALIZED: > + return "UNINITIALIZED"; > + case INTEL_UC_FIRMWARE_NOT_STARTED: > + return "NOT_STARTED"; NOT_STARTED and PENDING are not that much different, see below > case INTEL_UC_FIRMWARE_PENDING: > return "PENDING"; > case INTEL_UC_FIRMWARE_SUCCESS: maybe we should drop split on fetch/load and use single status, something like: UNINITIALIZED --> N/A (no HW) | DEFINED ----> MISSING (no valid fw) | -> FETCHED/UNLOADED <- / | \ \ LOADING / \ / \ / \- READY FAILED -/ > @@ -106,9 +113,15 @@ static inline > 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 > + * 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_NONE; > - uc_fw->load_status = INTEL_UC_FIRMWARE_NONE; > + uc_fw->fetch_status = INTEL_UC_FIRMWARE_UNINITIALIZED; > + uc_fw->load_status = INTEL_UC_FIRMWARE_NOT_STARTED; > uc_fw->type = type; > } > @@ -122,6 +135,13 @@ static inline bool intel_uc_fw_is_loaded(struct > intel_uc_fw *uc_fw) > return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS; > } > +static inline bool intel_uc_fw_supported(struct intel_uc_fw *uc_fw) returning "supported" in case of "no blob/no defs" might be misleading maybe "defined" will clearer ? > +{ > + /* 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; > +} > + > static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) > { > if (intel_uc_fw_is_loaded(uc_fw))
On 7/10/19 9:57 AM, Michal Wajdeczko wrote: > On Wed, 10 Jul 2019 02:54:29 +0200, Daniele Ceraolo Spurio > <daniele.ceraolospurio@intel.com> wrote: > >> Instead of always checking in the device config is GuC and HuC are > > s/is/if > >> supported or not, we can save the state in the uc_fw structure and >> avoid going through i915 every time from the low-level uc management >> code. while at it FIRMWARE_NONE has been renamed to better indicate that > > s/while/While > >> we haven't started the fetch/load yet, but we might have already selected >> a blob. >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> --- >> drivers/gpu/drm/i915/intel_guc_fw.c | 6 +++++- >> drivers/gpu/drm/i915/intel_huc_fw.c | 6 +++++- >> drivers/gpu/drm/i915/intel_uc.c | 25 ++++++++++++------------ >> drivers/gpu/drm/i915/intel_uc_fw.c | 4 +++- >> drivers/gpu/drm/i915/intel_uc_fw.h | 30 ++++++++++++++++++++++++----- >> 5 files changed, 51 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c >> b/drivers/gpu/drm/i915/intel_guc_fw.c >> index db1e0daca7db..ee95d4960c5c 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_fw.c >> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c >> @@ -79,8 +79,12 @@ static void guc_fw_select(struct intel_uc_fw *guc_fw) >> GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC); >> - if (!HAS_GUC(i915)) >> + if (!HAS_GUC(i915)) { >> + guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; >> return; >> + } >> + >> + guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED; >> if (i915_modparams.guc_firmware_path) { >> guc_fw->path = i915_modparams.guc_firmware_path; >> diff --git a/drivers/gpu/drm/i915/intel_huc_fw.c >> b/drivers/gpu/drm/i915/intel_huc_fw.c >> index 05cbf8338f53..06e726ba9863 100644 >> --- a/drivers/gpu/drm/i915/intel_huc_fw.c >> +++ b/drivers/gpu/drm/i915/intel_huc_fw.c >> @@ -73,8 +73,12 @@ static void huc_fw_select(struct intel_uc_fw *huc_fw) >> GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC); >> - if (!HAS_HUC(dev_priv)) >> + if (!HAS_HUC(dev_priv)) { >> + huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; >> return; >> + } >> + >> + huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED; >> if (i915_modparams.huc_firmware_path) { >> huc_fw->path = i915_modparams.huc_firmware_path; >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c >> index 789b0bccfb41..ef2a864b8990 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -71,7 +71,8 @@ static int __get_default_guc_log_level(struct >> drm_i915_private *i915) >> { >> int guc_log_level; >> - if (!HAS_GUC(i915) || !intel_uc_is_using_guc(i915)) >> + if (!intel_uc_fw_supported(&i915->guc.fw) || > > this goes too far, we should limit number of direct accesses to .fw > maybe we can have: > > inline bool intel_uc_has_guc(i915) > { > return intel_guc_is_present(&i915->guc); > } > > inline bool intel_guc_is_present(guc) > { > return intel_uc_fw_is_defined(&guc->fw); > } > Maybe instead of saving this info in the uc_fw it'd be better to just change guc_init_early to not do anything if !HAS_GUC and then do something like: bool intel_guc_is_present(guc) { return !!guc->send; } What do you think? otherwise I'll split it like you suggested >> + !intel_uc_is_using_guc(i915)) >> guc_log_level = GUC_LOG_LEVEL_DISABLED; >> else if (IS_ENABLED(CONFIG_DRM_I915_DEBUG) || >> IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) >> @@ -119,16 +120,16 @@ static void sanitize_options_early(struct >> drm_i915_private *i915) >> if (intel_uc_is_using_guc(i915) && >> !intel_uc_fw_is_selected(guc_fw)) { >> DRM_WARN("Incompatible option detected: %s=%d, %s!\n", >> "enable_guc", i915_modparams.enable_guc, >> - !HAS_GUC(i915) ? "no GuC hardware" : >> - "no GuC firmware"); >> + !intel_uc_fw_supported(guc_fw) ? >> + "no GuC hardware" : "no GuC firmware"); >> } >> /* Verify HuC firmware availability */ >> if (intel_uc_is_using_huc(i915) && >> !intel_uc_fw_is_selected(huc_fw)) { >> DRM_WARN("Incompatible option detected: %s=%d, %s!\n", >> "enable_guc", i915_modparams.enable_guc, >> - !HAS_HUC(i915) ? "no HuC hardware" : >> - "no HuC firmware"); >> + !intel_uc_fw_supported(huc_fw) ? >> + "no HuC hardware" : "no HuC firmware"); >> } >> /* XXX: GuC submission is unavailable for now */ >> @@ -148,8 +149,8 @@ static void sanitize_options_early(struct >> drm_i915_private *i915) >> if (i915_modparams.guc_log_level > 0 && >> !intel_uc_is_using_guc(i915)) { >> DRM_WARN("Incompatible option detected: %s=%d, %s!\n", >> "guc_log_level", i915_modparams.guc_log_level, >> - !HAS_GUC(i915) ? "no GuC hardware" : >> - "GuC not enabled"); >> + !intel_uc_fw_supported(guc_fw) ? >> + "no GuC hardware" : "GuC not enabled"); >> i915_modparams.guc_log_level = 0; >> } >> @@ -376,7 +377,7 @@ int intel_uc_init(struct drm_i915_private *i915) >> if (!USES_GUC(i915)) >> return 0; >> - if (!HAS_GUC(i915)) >> + if (!intel_uc_fw_supported(&guc->fw)) >> return -ENODEV; >> /* XXX: GuC submission is unavailable for now */ >> @@ -419,7 +420,7 @@ void intel_uc_fini(struct drm_i915_private *i915) >> if (!USES_GUC(i915)) >> return; >> - GEM_BUG_ON(!HAS_GUC(i915)); >> + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); >> if (USES_GUC_SUBMISSION(i915)) >> intel_guc_submission_fini(guc); >> @@ -435,7 +436,7 @@ static void __uc_sanitize(struct drm_i915_private >> *i915) >> struct intel_guc *guc = &i915->guc; >> struct intel_huc *huc = &i915->huc; >> - GEM_BUG_ON(!HAS_GUC(i915)); >> + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); >> intel_huc_sanitize(huc); >> intel_guc_sanitize(guc); >> @@ -460,7 +461,7 @@ int intel_uc_init_hw(struct drm_i915_private *i915) >> if (!USES_GUC(i915)) >> return 0; >> - GEM_BUG_ON(!HAS_GUC(i915)); >> + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); >> guc_reset_interrupts(guc); >> @@ -557,7 +558,7 @@ void intel_uc_fini_hw(struct drm_i915_private *i915) >> if (!intel_guc_is_loaded(guc)) >> return; >> - GEM_BUG_ON(!HAS_GUC(i915)); >> + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); >> if (USES_GUC_SUBMISSION(i915)) >> intel_guc_submission_disable(guc); >> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c >> b/drivers/gpu/drm/i915/intel_uc_fw.c >> index f342ddd47df8..8ce7210907c0 100644 >> --- a/drivers/gpu/drm/i915/intel_uc_fw.c >> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c >> @@ -47,6 +47,8 @@ void intel_uc_fw_fetch(struct drm_i915_private >> *dev_priv, >> size_t size; >> 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", >> @@ -328,7 +330,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_NONE; >> + uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED; >> } >> /** >> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h >> b/drivers/gpu/drm/i915/intel_uc_fw.h >> index 24e66469153c..833d04d06576 100644 >> --- a/drivers/gpu/drm/i915/intel_uc_fw.h >> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h >> @@ -26,6 +26,7 @@ >> #define _INTEL_UC_FW_H_ >> #include <linux/types.h> >> +#include "i915_gem.h" >> struct drm_printer; >> struct drm_i915_private; >> @@ -34,8 +35,10 @@ 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_NONE = 0, >> + 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 >> }; >> @@ -79,10 +82,14 @@ static inline >> const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status) >> { >> switch (status) { >> + case INTEL_UC_FIRMWARE_NOT_SUPPORTED: > > maybe INTEL_UC_FIRMWARE_NOT_APPLICABLE, as used in string below? ok > >> + return "N/A - uc HW not available"; > > note that other states have short strings, so just "N/A" ? ok > >> case INTEL_UC_FIRMWARE_FAIL: >> return "FAIL"; >> - case INTEL_UC_FIRMWARE_NONE: >> - return "NONE"; >> + case INTEL_UC_FIRMWARE_UNINITIALIZED: >> + return "UNINITIALIZED"; >> + case INTEL_UC_FIRMWARE_NOT_STARTED: >> + return "NOT_STARTED"; > > NOT_STARTED and PENDING are not that much different, > see below NOT_STARTED is not a new case I've added, I've just renamed the NONE case to indicate that we hadn't even started the fetch process, but we might be past the FW selection. > >> case INTEL_UC_FIRMWARE_PENDING: >> return "PENDING"; >> case INTEL_UC_FIRMWARE_SUCCESS: > > maybe we should drop split on fetch/load and use > single status, something like: > > UNINITIALIZED --> N/A (no HW) > | > DEFINED ----> MISSING (no valid fw) > | > -> FETCHED/UNLOADED <- > / | \ > \ LOADING / > \ / \ / > \- READY FAILED -/ > > Nothing against this, but I don't think it belongs in this series. >> @@ -106,9 +113,15 @@ static inline >> 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 >> + * 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_NONE; >> - uc_fw->load_status = INTEL_UC_FIRMWARE_NONE; >> + uc_fw->fetch_status = INTEL_UC_FIRMWARE_UNINITIALIZED; >> + uc_fw->load_status = INTEL_UC_FIRMWARE_NOT_STARTED; >> uc_fw->type = type; >> } >> @@ -122,6 +135,13 @@ static inline bool intel_uc_fw_is_loaded(struct >> intel_uc_fw *uc_fw) >> return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS; >> } >> +static inline bool intel_uc_fw_supported(struct intel_uc_fw *uc_fw) > > returning "supported" in case of "no blob/no defs" might be misleading > maybe "defined" will clearer ? > My idea was to track if the HW had support for FW loading (i.e. if there is a guc/huc in the GT), independently from us defining or finding a firmware for it or not. hw_supported? Daniele >> +{ >> + /* 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; >> +} >> + >> static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) >> { >> if (intel_uc_fw_is_loaded(uc_fw))
On Wed, 10 Jul 2019 23:46:25 +0200, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote: > > Maybe instead of saving this info in the uc_fw it'd be better to just > change guc_init_early to not do anything if !HAS_GUC and then do > something like: > > bool intel_guc_is_present(guc) > { > return !!guc->send; > } > > What do you think? otherwise I'll split it like you suggested Yes, we can check send instead (but without !! to avoid complains from Chris, and maybe with some extra comments/GEM_BUG_ON in init_early to not missed that dual role) intel_uc_has_guc() will still be there as top level helper, right ? Thanks, Michal
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c index db1e0daca7db..ee95d4960c5c 100644 --- a/drivers/gpu/drm/i915/intel_guc_fw.c +++ b/drivers/gpu/drm/i915/intel_guc_fw.c @@ -79,8 +79,12 @@ static void guc_fw_select(struct intel_uc_fw *guc_fw) GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC); - if (!HAS_GUC(i915)) + if (!HAS_GUC(i915)) { + guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; return; + } + + guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED; if (i915_modparams.guc_firmware_path) { guc_fw->path = i915_modparams.guc_firmware_path; diff --git a/drivers/gpu/drm/i915/intel_huc_fw.c b/drivers/gpu/drm/i915/intel_huc_fw.c index 05cbf8338f53..06e726ba9863 100644 --- a/drivers/gpu/drm/i915/intel_huc_fw.c +++ b/drivers/gpu/drm/i915/intel_huc_fw.c @@ -73,8 +73,12 @@ static void huc_fw_select(struct intel_uc_fw *huc_fw) GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC); - if (!HAS_HUC(dev_priv)) + if (!HAS_HUC(dev_priv)) { + huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; return; + } + + huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED; if (i915_modparams.huc_firmware_path) { huc_fw->path = i915_modparams.huc_firmware_path; diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 789b0bccfb41..ef2a864b8990 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -71,7 +71,8 @@ static int __get_default_guc_log_level(struct drm_i915_private *i915) { int guc_log_level; - if (!HAS_GUC(i915) || !intel_uc_is_using_guc(i915)) + if (!intel_uc_fw_supported(&i915->guc.fw) || + !intel_uc_is_using_guc(i915)) guc_log_level = GUC_LOG_LEVEL_DISABLED; else if (IS_ENABLED(CONFIG_DRM_I915_DEBUG) || IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) @@ -119,16 +120,16 @@ static void sanitize_options_early(struct drm_i915_private *i915) if (intel_uc_is_using_guc(i915) && !intel_uc_fw_is_selected(guc_fw)) { DRM_WARN("Incompatible option detected: %s=%d, %s!\n", "enable_guc", i915_modparams.enable_guc, - !HAS_GUC(i915) ? "no GuC hardware" : - "no GuC firmware"); + !intel_uc_fw_supported(guc_fw) ? + "no GuC hardware" : "no GuC firmware"); } /* Verify HuC firmware availability */ if (intel_uc_is_using_huc(i915) && !intel_uc_fw_is_selected(huc_fw)) { DRM_WARN("Incompatible option detected: %s=%d, %s!\n", "enable_guc", i915_modparams.enable_guc, - !HAS_HUC(i915) ? "no HuC hardware" : - "no HuC firmware"); + !intel_uc_fw_supported(huc_fw) ? + "no HuC hardware" : "no HuC firmware"); } /* XXX: GuC submission is unavailable for now */ @@ -148,8 +149,8 @@ static void sanitize_options_early(struct drm_i915_private *i915) if (i915_modparams.guc_log_level > 0 && !intel_uc_is_using_guc(i915)) { DRM_WARN("Incompatible option detected: %s=%d, %s!\n", "guc_log_level", i915_modparams.guc_log_level, - !HAS_GUC(i915) ? "no GuC hardware" : - "GuC not enabled"); + !intel_uc_fw_supported(guc_fw) ? + "no GuC hardware" : "GuC not enabled"); i915_modparams.guc_log_level = 0; } @@ -376,7 +377,7 @@ int intel_uc_init(struct drm_i915_private *i915) if (!USES_GUC(i915)) return 0; - if (!HAS_GUC(i915)) + if (!intel_uc_fw_supported(&guc->fw)) return -ENODEV; /* XXX: GuC submission is unavailable for now */ @@ -419,7 +420,7 @@ void intel_uc_fini(struct drm_i915_private *i915) if (!USES_GUC(i915)) return; - GEM_BUG_ON(!HAS_GUC(i915)); + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); if (USES_GUC_SUBMISSION(i915)) intel_guc_submission_fini(guc); @@ -435,7 +436,7 @@ static void __uc_sanitize(struct drm_i915_private *i915) struct intel_guc *guc = &i915->guc; struct intel_huc *huc = &i915->huc; - GEM_BUG_ON(!HAS_GUC(i915)); + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); intel_huc_sanitize(huc); intel_guc_sanitize(guc); @@ -460,7 +461,7 @@ int intel_uc_init_hw(struct drm_i915_private *i915) if (!USES_GUC(i915)) return 0; - GEM_BUG_ON(!HAS_GUC(i915)); + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); guc_reset_interrupts(guc); @@ -557,7 +558,7 @@ void intel_uc_fini_hw(struct drm_i915_private *i915) if (!intel_guc_is_loaded(guc)) return; - GEM_BUG_ON(!HAS_GUC(i915)); + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); if (USES_GUC_SUBMISSION(i915)) intel_guc_submission_disable(guc); diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c index f342ddd47df8..8ce7210907c0 100644 --- a/drivers/gpu/drm/i915/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/intel_uc_fw.c @@ -47,6 +47,8 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, size_t size; 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", @@ -328,7 +330,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_NONE; + uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED; } /** diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h index 24e66469153c..833d04d06576 100644 --- a/drivers/gpu/drm/i915/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/intel_uc_fw.h @@ -26,6 +26,7 @@ #define _INTEL_UC_FW_H_ #include <linux/types.h> +#include "i915_gem.h" struct drm_printer; struct drm_i915_private; @@ -34,8 +35,10 @@ 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_NONE = 0, + 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 }; @@ -79,10 +82,14 @@ static inline const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status) { switch (status) { + case INTEL_UC_FIRMWARE_NOT_SUPPORTED: + return "N/A - uc HW not available"; case INTEL_UC_FIRMWARE_FAIL: return "FAIL"; - case INTEL_UC_FIRMWARE_NONE: - return "NONE"; + 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: @@ -106,9 +113,15 @@ static inline 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 + * 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_NONE; - uc_fw->load_status = INTEL_UC_FIRMWARE_NONE; + uc_fw->fetch_status = INTEL_UC_FIRMWARE_UNINITIALIZED; + uc_fw->load_status = INTEL_UC_FIRMWARE_NOT_STARTED; uc_fw->type = type; } @@ -122,6 +135,13 @@ static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw) return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS; } +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; +} + static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) { if (intel_uc_fw_is_loaded(uc_fw))
Instead of always checking in the device config is GuC and HuC are supported or not, we can save the state in the uc_fw structure and avoid going through i915 every time from the low-level uc management code. while at it FIRMWARE_NONE has been renamed to better indicate that we haven't started the fetch/load yet, but we might have already selected a blob. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> --- drivers/gpu/drm/i915/intel_guc_fw.c | 6 +++++- drivers/gpu/drm/i915/intel_huc_fw.c | 6 +++++- drivers/gpu/drm/i915/intel_uc.c | 25 ++++++++++++------------ drivers/gpu/drm/i915/intel_uc_fw.c | 4 +++- drivers/gpu/drm/i915/intel_uc_fw.h | 30 ++++++++++++++++++++++++----- 5 files changed, 51 insertions(+), 20 deletions(-)