[Intel-gfx] [PATCH v3 2/8] drm/i915: Fix handling of non-supported uC
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Jul 25 05:51:14 UTC 2019
On Thu, 25 Jul 2019 02:18:07 +0200, Daniele Ceraolo Spurio
<daniele.ceraolospurio at intel.com> wrote:
> There are 2 issues around handling of missing uC support:
>
> - We treat lack of uC HW and lack of uC FW definition as 2 different
> cases, but both of them mean that we don't support the uC on the
> platform we're running on.
>
> - We rely on the modparam to decide if we can take uC paths or not, but
> we don't sanitize it if it is set incorrectly on platform with no uC
> support.
>
> To fix both of them, unify the 2 cases in a single one and sanitize the
> modparam on invalid configuration (after printing an error message).
> The log has been adapted as well, since the user doesn't care why we
> don't support GuC/HuC (no HW or no FW), just that we do not. Developers
> can easily find the answer based on the platform, so we can simplify the
> log.
>
> Correcting the modparam has been preferred over failing the load since
> this is what we usually do for non-supported feature (e.g. the now gone
> enable_ppgtt would fall back to the highest supported PPGTT mode if the
> selected one was not available).
>
> Note that this patch purposely doesn't change the behavior for platforms
> that do have uC support, in which case we will still fail if enable_guc
> is set and the firmware is not available on the system.
>
> Suggested-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
Note that more changes are planned around this code,
Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 11 ++++---
> drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 11 ++++---
> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 37 ++++++++++++-----------
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 8 -----
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 5 ---
> 5 files changed, 31 insertions(+), 41 deletions(-)
>
> 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 87169e826747..17ce78240cf8 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> @@ -80,12 +80,10 @@ static void guc_fw_select(struct intel_uc_fw *guc_fw)
> GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
> - if (!HAS_GT_UC(i915)) {
> - guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> - return;
> - }
> + guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> - guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> + if (!HAS_GT_UC(i915))
> + return;
> if (i915_modparams.guc_firmware_path) {
> guc_fw->path = i915_modparams.guc_firmware_path;
> @@ -112,6 +110,9 @@ static void guc_fw_select(struct intel_uc_fw *guc_fw)
> guc_fw->major_ver_wanted = SKL_GUC_FW_MAJOR;
> guc_fw->minor_ver_wanted = SKL_GUC_FW_MINOR;
> }
> +
> + if (guc_fw->path)
> + guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> }
> /**
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> index ff6f7b157ecb..c3a7bd57fb55 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> @@ -74,12 +74,10 @@ static void huc_fw_select(struct intel_uc_fw *huc_fw)
> GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
> - if (!HAS_GT_UC(dev_priv)) {
> - huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> - return;
> - }
> + huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> - huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> + if (!HAS_GT_UC(dev_priv))
> + return;
> if (i915_modparams.huc_firmware_path) {
> huc_fw->path = i915_modparams.huc_firmware_path;
> @@ -106,6 +104,9 @@ static void huc_fw_select(struct intel_uc_fw *huc_fw)
> huc_fw->major_ver_wanted = ICL_HUC_FW_MAJOR;
> huc_fw->minor_ver_wanted = ICL_HUC_FW_MINOR;
> }
> +
> + if (huc_fw->path)
> + huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> }
> /**
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index bdb171c3f36e..3f672ea7456b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -68,7 +68,7 @@ static int __get_platform_enable_guc(struct intel_uc
> *uc)
> if (INTEL_GEN(uc_to_gt(uc)->i915) < 11)
> return 0;
> - if (intel_uc_fw_is_selected(guc_fw) && intel_uc_fw_is_selected(huc_fw))
> + if (intel_uc_fw_supported(guc_fw) && intel_uc_fw_supported(huc_fw))
> enable_guc |= ENABLE_GUC_LOAD_HUC;
> return enable_guc;
> @@ -123,26 +123,28 @@ static void sanitize_options_early(struct intel_uc
> *uc)
> yesno(intel_uc_is_using_huc(uc)));
> /* Verify GuC firmware availability */
> - if (intel_uc_is_using_guc(uc) && !intel_uc_fw_is_selected(guc_fw)) {
> - DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
> - "enable_guc", i915_modparams.enable_guc,
> - !intel_uc_fw_supported(guc_fw) ?
> - "no GuC hardware" : "no GuC firmware");
> + if (intel_uc_is_using_guc(uc) && !intel_uc_fw_supported(guc_fw)) {
> + DRM_WARN("Incompatible option detected: enable_guc=%d, "
> + "but GuC is not supported!\n",
> + i915_modparams.enable_guc);
> + DRM_INFO("Disabling GuC/HuC loading!\n");
> + i915_modparams.enable_guc = 0;
> }
> /* Verify HuC firmware availability */
> - if (intel_uc_is_using_huc(uc) && !intel_uc_fw_is_selected(huc_fw)) {
> - DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
> - "enable_guc", i915_modparams.enable_guc,
> - !intel_uc_fw_supported(huc_fw) ?
> - "no HuC hardware" : "no HuC firmware");
> + if (intel_uc_is_using_huc(uc) && !intel_uc_fw_supported(huc_fw)) {
> + DRM_WARN("Incompatible option detected: enable_guc=%d, "
> + "but HuC is not supported!\n",
> + i915_modparams.enable_guc);
> + DRM_INFO("Disabling HuC loading!\n");
> + i915_modparams.enable_guc &= ~ENABLE_GUC_LOAD_HUC;
> }
> /* XXX: GuC submission is unavailable for now */
> if (intel_uc_is_using_guc_submission(uc)) {
> - DRM_INFO("Incompatible option detected: %s=%d, %s!\n",
> - "enable_guc", i915_modparams.enable_guc,
> - "GuC submission not supported");
> + DRM_INFO("Incompatible option detected: enable_guc=%d, "
> + "but GuC submission is not supported!\n",
> + i915_modparams.enable_guc);
> DRM_INFO("Switching to non-GuC submission mode!\n");
> i915_modparams.enable_guc &= ~ENABLE_GUC_SUBMISSION;
> }
> @@ -153,10 +155,9 @@ static void sanitize_options_early(struct intel_uc
> *uc)
> __get_default_guc_log_level(uc);
> if (i915_modparams.guc_log_level > 0 && !intel_uc_is_using_guc(uc)) {
> - DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
> - "guc_log_level", i915_modparams.guc_log_level,
> - !intel_uc_fw_supported(guc_fw) ?
> - "no GuC hardware" : "GuC not enabled");
> + DRM_WARN("Incompatible option detected: guc_log_level=%d, "
> + "but GuC is not enabled!\n",
> + i915_modparams.guc_log_level);
> i915_modparams.guc_log_level = 0;
> }
> 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 8ce7210907c0..432b632b04c0 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -49,14 +49,6 @@ void intel_uc_fw_fetch(struct drm_i915_private
> *dev_priv,
> 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);
> 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 833d04d06576..55ac9eeab440 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> @@ -125,11 +125,6 @@ void intel_uc_fw_init_early(struct intel_uc_fw
> *uc_fw,
> uc_fw->type = type;
> }
> -static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw)
> -{
> - return uc_fw->path != NULL;
> -}
> -
> static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw)
> {
> return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS;
More information about the Intel-gfx
mailing list