On Fri, Mar 10, 2017 at 12:46:06PM +0100, Arkadiusz Hiler wrote:
> Current version of intel_guc_init_hw() does a lot:
> - cares about submission
> - loads huc
> - implement WA
>
> This change offloads some of the logic to intel_uc_init_hw(), which now
> cares about the above.
>
> v2: rename guc_hw_reset and fix typo in define name (M. Wajdeczko)
> v3: rename once again
> v4: remove spurious comments and add some style (J. Lahtinen)
> v5: flow changes, got rid of dead checks (M. Wajdeczko)
> v6: rebase
>
> Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> Cc: Michal Winiarski <michal.winiarski@xxxxxxxxx>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx>
> ---
snip
> @@ -397,42 +379,22 @@ int intel_guc_init_hw(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> const char *fw_path = guc->fw.path;
> - int retries, ret, err;
> + int ret;
>
> DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> fw_path,
> intel_uc_fw_status_repr(guc->fw.fetch_status),
> intel_uc_fw_status_repr(guc->fw.load_status));
>
> - /* Loading forbidden, or no firmware to load? */
> - if (!i915.enable_guc_loading) {
> - err = 0;
> - goto fail;
> - } else if (fw_path == NULL) {
> - /* Device is known to have no uCode (e.g. no GuC) */
> - err = -ENXIO;
> - goto fail;
> + if (!fw_path) {
> + return -ENXIO;
> } else if (*fw_path == '\0') {
> - /* Device has a GuC but we don't know what f/w to load? */
> WARN(1, "No GuC firmware known for this platform!\n");
> - err = -ENODEV;
> - goto fail;
> + return -ENODEV;
> }
>
> - /* Fetch failed, or already fetched but failed to load? */
> - if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS) {
> - err = -EIO;
> - goto fail;
> - } else if (guc->fw.load_status == INTEL_UC_FIRMWARE_FAIL) {
> - err = -ENOEXEC;
> - goto fail;
> - }
> -
> - guc_interrupts_release(dev_priv);
> - gen9_reset_guc_interrupts(dev_priv);
> -
> - /* We need to notify the guc whenever we change the GGTT */
> - i915_ggtt_enable_guc(dev_priv);
> + if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> + return -EIO;
Above fw_path checks seem to be redundant as we also look for fetch status here.
...
> @@ -63,6 +84,98 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
> intel_guc_init_fw(&dev_priv->guc);
> }
>
> +int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> +{
> + int ret, attempts;
> + const int guc_wa_hash_check_not_set_attempts = 3;
In all other places, we put const vars definitions as first statements.
And maybe this const should be accompanied by the /* Wa */ comment
that is shown below. Also changing prefix to "gen9" maybe helpful
-Michal
> +
> +
> + /* GuC not enabled, nothing to do */
> + if (!i915.enable_guc_loading)
> + return 0;
> +
> + intel_guc_release_interrupts(dev_priv);
> + gen9_reset_guc_interrupts(dev_priv);
> +
> + /* We need to notify the guc whenever we change the GGTT */
> + i915_ggtt_enable_guc(dev_priv);
> +
> + if (i915.enable_guc_submission) {
> + ret = i915_guc_submission_init(dev_priv);
> + if (ret)
> + goto fail;
> + }
> +
> + /* WaEnableuKernelHeaderValidFix:skl */
> + /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
> + if (IS_GEN9(dev_priv))
> + attempts = guc_wa_hash_check_not_set_attempts;
> + else
> + attempts = 1;
> +
> + while (attempts--) {
> + /*
> + * Always reset the GuC just before (re)loading, so
> + * that the state and timing are fairly predictable
> + */
> + ret = __intel_uc_reset_hw(dev_priv);
> + if (ret)
> + goto fail;
> +
> + intel_huc_init_hw(&dev_priv->huc);
> + ret = intel_guc_init_hw(&dev_priv->guc);
> + if (ret == 0 || ret != -EAGAIN)
> + break;
> +
> + DRM_INFO("GuC fw load failed: %d; will reset and "
> + "retry %d more time(s)\n", ret, attempts);
> + }
> +
> + /* Did we succeded or run out of retries? */
> + if (ret)
> + goto fail;
> +
> + intel_guc_auth_huc(dev_priv);
> + if (i915.enable_guc_submission) {
> + if (i915.guc_log_level >= 0)
> + gen9_enable_guc_interrupts(dev_priv);
> +
> + ret = i915_guc_submission_enable(dev_priv);
> + if (ret)
> + goto fail;
> + }
> +
> + return 0;
> +
> +fail:
> + /*
> + * We've failed to load the firmware :(
> + *
> + * Decide whether to disable GuC submission and fall back to
> + * execlist mode, and whether to hide the error by returning
> + * zero or to return -EIO, which the caller will treat as a
> + * nonfatal error (i.e. it doesn't prevent driver load, but
> + * marks the GPU as wedged until reset).
> + */
> + DRM_ERROR("GuC init failed\n");
> + if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
> + ret = -EIO;
> + else
> + ret = 0;
> +
> + if (i915.enable_guc_submission) {
> + i915.enable_guc_submission = 0;
> + DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> + }
> +
> + i915_ggtt_disable_guc(dev_priv);
> + intel_guc_release_interrupts(dev_priv);
> + i915_guc_submission_disable(dev_priv);
> + i915_guc_submission_fini(dev_priv);
> +
> + return ret;
> +}
> +
> /*
> * Read GuC command/status register (SOFT_SCRATCH_0)
> * Return true if it contains a response rather than a command
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index c5179ef..a1f4960 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -187,6 +187,7 @@ struct intel_huc {
> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
> void intel_uc_init_early(struct drm_i915_private *dev_priv);
> void intel_uc_init_fw(struct drm_i915_private *dev_priv);
> +int intel_uc_init_hw(struct drm_i915_private *dev_priv);
> void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
> struct intel_uc_fw *uc_fw);
> int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
> @@ -199,6 +200,7 @@ void intel_guc_fini(struct drm_i915_private *dev_priv);
> const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
> int intel_guc_suspend(struct drm_i915_private *dev_priv);
> int intel_guc_resume(struct drm_i915_private *dev_priv);
> +void intel_guc_release_interrupts(struct drm_i915_private *dev_priv);
> u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>
> /* i915_guc_submission.c */
> --
> 2.9.3
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
