On 7/10/19 9:57 AM, Michal Wajdeczko wrote:
On Wed, 10 Jul 2019 02:54:29 +0200, Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> wrote:Instead of always checking in the device config is GuC and HuC ares/is/ifsupported 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 thats/while/Whilewe haven't started the fetch/load yet, but we might have already selected a blob. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> --- 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.cindex 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.cindex 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.cindex 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.cindex 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.hindex 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))
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
