On Tue, Feb 14, 2017 at 05:15:40PM +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_load(), which now
> cares about the above.
>
> Cc: Anusha Srivatsa <anusha.sriva...@intel.com>
> Cc: Michal Winiarski <michal.winiar...@intel.com>
> Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hi...@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> drivers/gpu/drm/i915/intel_guc_loader.c | 136
> +++-----------------------------
> drivers/gpu/drm/i915/intel_uc.c | 105 ++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_uc.h | 6 ++
> 4 files changed, 124 insertions(+), 125 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5234c76..afc7e14 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4478,7 +4478,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
> intel_mocs_init_l3cc_table(dev_priv);
>
> /* We can't enable contexts until all firmware is loaded */
> - ret = intel_guc_init_hw(dev_priv);
> + ret = intel_uc_init_hw(dev_priv);
> if (ret)
> goto out;
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
> b/drivers/gpu/drm/i915/intel_guc_loader.c
> index db5713c..4ea705d 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -91,7 +91,7 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status
> status)
> }
> };
>
> -static void guc_interrupts_release(struct drm_i915_private *dev_priv)
> +void intel_guc_interrupts_release(struct drm_i915_private *dev_priv)
> {
> struct intel_engine_cs *engine;
> enum intel_engine_id id;
> @@ -109,7 +109,7 @@ static void guc_interrupts_release(struct
> drm_i915_private *dev_priv)
> I915_WRITE(GUC_WD_VECS_IER, 0);
> }
>
> -static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
> +void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv)
> {
> struct intel_engine_cs *engine;
> enum intel_engine_id id;
> @@ -409,24 +409,6 @@ static int guc_ucode_xfer(struct drm_i915_private
> *dev_priv)
> return ret;
> }
>
> -static int guc_hw_reset(struct drm_i915_private *dev_priv)
> -{
> - int ret;
> - u32 guc_status;
> -
> - ret = intel_guc_reset(dev_priv);
> - if (ret) {
> - DRM_ERROR("GuC reset failed, ret = %d\n", ret);
> - return ret;
> - }
> -
> - guc_status = I915_READ(GUC_STATUS);
> - WARN(!(guc_status & GS_MIA_IN_RESET),
> - "GuC status: 0x%x, MIA core expected to be in reset\n",
> guc_status);
> -
> - return ret;
> -}
> -
> /**
> * intel_guc_init_hw() - finish preparing the GuC for activity
> * @dev_priv: i915 device private
> @@ -444,147 +426,53 @@ int intel_guc_init_hw(struct drm_i915_private
> *dev_priv)
> {
> struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> 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) {
> + if (fw_path == NULL) {
> /* Device is known to have no uCode (e.g. no GuC) */
> - err = -ENXIO;
> - goto fail;
> + 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;
> + return -EIO;
> } else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) {
> - err = -ENOEXEC;
> - goto fail;
> + return -ENOEXEC;
> }
>
> - 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);
> -
> guc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
>
> DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> intel_uc_fw_status_repr(guc_fw->fetch_status),
> intel_uc_fw_status_repr(guc_fw->load_status));
>
> - err = i915_guc_submission_init(dev_priv);
> - if (err)
> - goto fail;
> -
> /*
> * WaEnableuKernelHeaderValidFix:skl,bxt
> * For BXT, this is only upto B0 but below WA is required for later
> * steppings also so this is extended as well.
> */
> - /* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> - for (retries = 3; ; ) {
> - /*
> - * Always reset the GuC just before (re)loading, so
> - * that the state and timing are fairly predictable
> - */
> - err = guc_hw_reset(dev_priv);
> - if (err)
> - goto fail;
> + ret = guc_ucode_xfer(dev_priv);
>
> - intel_huc_init_hw(dev_priv);
> - err = guc_ucode_xfer(dev_priv);
> - if (!err)
> - break;
> -
> - if (--retries == 0)
> - goto fail;
> -
> - DRM_INFO("GuC fw load failed: %d; will reset and "
> - "retry %d more time(s)\n", err, retries);
> - }
> + if (ret)
> + return -EAGAIN;
>
> guc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS;
>
> - intel_guc_auth_huc(dev_priv);
> -
> - if (i915.enable_guc_submission) {
> - if (i915.guc_log_level >= 0)
> - gen9_enable_guc_interrupts(dev_priv);
> -
> - err = i915_guc_submission_enable(dev_priv);
> - if (err)
> - goto fail;
> - guc_interrupts_capture(dev_priv);
> - }
> -
> DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
> i915.enable_guc_submission ? "submission enabled" : "loaded",
> guc_fw->path,
> guc_fw->major_ver_found, guc_fw->minor_ver_found);
>
> return 0;
> -
> -fail:
> - if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
> - guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
> -
> - guc_interrupts_release(dev_priv);
> - i915_guc_submission_disable(dev_priv);
> - i915_guc_submission_fini(dev_priv);
> - i915_ggtt_disable_guc(dev_priv);
> -
> - /*
> - * 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).
> - */
> - if (i915.enable_guc_loading > 1) {
> - ret = -EIO;
> - } else if (i915.enable_guc_submission > 1) {
> - ret = -EIO;
> - } else {
> - ret = 0;
> - }
> -
> - if (err == 0 && !HAS_GUC_UCODE(dev_priv))
> - ; /* Don't mention the GuC! */
> - else if (err == 0)
> - DRM_INFO("GuC firmware load skipped\n");
> - else if (ret != -EIO)
> - DRM_NOTE("GuC firmware load failed: %d\n", err);
> - else
> - DRM_WARN("GuC firmware load failed: %d\n", err);
> -
> - if (i915.enable_guc_submission) {
> - if (fw_path == NULL)
> - DRM_INFO("GuC submission without firmware not
> supported\n");
> - if (ret == 0)
> - DRM_NOTE("Falling back from GuC submission to execlist
> mode\n");
> - else
> - DRM_ERROR("GuC init failed: %d\n", ret);
> - }
> - i915.enable_guc_submission = 0;
> -
> - return ret;
> }
>
> void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> @@ -782,7 +670,7 @@ void intel_guc_fini(struct drm_i915_private *dev_priv)
> struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>
> mutex_lock(&dev_priv->drm.struct_mutex);
> - guc_interrupts_release(dev_priv);
> + intel_guc_interrupts_release(dev_priv);
> i915_guc_submission_disable(dev_priv);
> i915_guc_submission_fini(dev_priv);
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index d1ca41d..bba220c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -25,6 +25,25 @@
> #include "i915_drv.h"
> #include "intel_uc.h"
>
> +static int uc_hw_reset(struct drm_i915_private *dev_priv)
Do you plan to put more resets here?
If this is only for Guc then I would stay with old name
> +{
> + int ret;
> + u32 guc_status;
> +
> + ret = intel_guc_reset(dev_priv);
> + if (ret) {
> + DRM_ERROR("GuC reset failed, ret = %d\n", ret);
> + return ret;
> + }
> +
> + guc_status = I915_READ(GUC_STATUS);
> + WARN(!(guc_status & GS_MIA_IN_RESET),
> + "GuC status: 0x%x, MIA core expected to be in reset\n",
> + guc_status);
> +
> + return ret;
> +}
> +
> void intel_uc_sanitize_params(struct drm_i915_private *dev_priv)
> {
> if (!HAS_GUC(dev_priv)) {
> @@ -59,6 +78,92 @@ void intel_uc_init(struct drm_i915_private *dev_priv)
> intel_guc_init(dev_priv);
> }
>
> +int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> +{
> + int ret, retries;
> +
> + /* guc not enabled, nothing to do */
> + if (!i915.enable_guc_loading)
> + return 0;
> +
> + intel_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 (i915.enable_guc_submission) {
Hmm, if I'm not wrong same condition is inside _init() function...
> + ret = i915_guc_submission_init(dev_priv);
> + if (ret)
> + goto fail;
> + }
> +
> + /* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> + retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS;
> + while (retries--) {
> + /*
> + * Always reset the GuC just before (re)loading, so
> + * that the state and timing are fairly predictable
> + */
> + ret = uc_hw_reset(dev_priv);
> + if (ret)
> + goto fail;
> +
> + intel_huc_init_hw(dev_priv);
> + ret = intel_guc_init_hw(dev_priv);
> + if (ret == 0 || ret != -EAGAIN)
> + break;
> +
> + DRM_INFO("GuC fw load failed: %d; will reset and "
> + "retry %d more time(s)\n", ret, retries);
> + }
> +
> + /* 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;
> + intel_guc_interrupts_capture(dev_priv);
> + }
> +
> + 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_interrupts_release(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 264761a..fbe774e 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -29,6 +29,8 @@
> #include "intel_ringbuffer.h"
>
> #include "i915_vma.h"
> +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3
Typo
Regards,
Michal
>
> struct drm_i915_gem_request;
>
> @@ -184,8 +186,10 @@ struct intel_huc {
> };
>
> /* intel_uc.c */
> +void intel_uc_sanitize_params(struct drm_i915_private *dev_priv);
> void intel_uc_init_early(struct drm_i915_private *dev_priv);
> void intel_uc_init(struct drm_i915_private *dev_priv);
> +int intel_uc_init_hw(struct drm_i915_private *dev_priv);
> int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
> int intel_guc_sample_forcewake(struct intel_guc *guc);
>
> @@ -198,6 +202,8 @@ extern int intel_guc_suspend(struct drm_i915_private
> *dev_priv);
> extern int intel_guc_resume(struct drm_i915_private *dev_priv);
> void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> struct intel_uc_fw *uc_fw);
> +void intel_guc_interrupts_release(struct drm_i915_private *dev_priv);
> +void intel_guc_interrupts_capture(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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
