[Intel-gfx] [PATCH v13 13/21] drm/i915/uc: Support resume from sleep w/ and w/o GuC/HuC reload
Michal Wajdeczko
michal.wajdeczko at intel.com
Wed Oct 11 17:06:46 UTC 2017
On Wed, 11 Oct 2017 10:54:08 +0200, Sagar Arun Kamble
<sagar.a.kamble at intel.com> wrote:
> GuC/HuC resume operation depends on whether firmwares are available
> in the WOPCM region. This is known through register WOPCM_SIZE BIT(0).
>
> If it indicates WOPCM is locked (bit is set) we just need to send action
> to GuC to resume and enable other related GuC functionality such as
> communication, interrupts and submission.
>
> If it indicates WOPCM is not locked then we need to first reload the
> GuC/HuC and then do all resume tasks. Currently on resume from sleep,
> GuC/HuC are not loaded as GPU is reset at the end of suspend/early
> resume.
> So we will have to reload the firmware and send action to resume.
> Resume will be done through uc_init_hw from gem_init_hw based on newly
> introduced state "guc->suspended". During gem_init_hw firmware load will
> be skipped based on resume status during intel_uc_resume.
>
> Also updated the accesses to dev_priv->guc and dev_priv->huc structure by
> reusing initial declared pointer.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_guc_reg.h | 1 +
> drivers/gpu/drm/i915/intel_guc.c | 2 +
> drivers/gpu/drm/i915/intel_guc.h | 3 ++
> drivers/gpu/drm/i915/intel_uc.c | 99
> +++++++++++++++++++++++++++++--------
> 4 files changed, 85 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h
> b/drivers/gpu/drm/i915/i915_guc_reg.h
> index 35cf991..532296b 100644
> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
> @@ -75,6 +75,7 @@
> /* Defines WOPCM space available to GuC firmware */
> #define GUC_WOPCM_SIZE _MMIO(0xc050)
> +#define GUC_WOPCM_LOCKED BIT(0)
> /* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */
> #define GUC_WOPCM_TOP (0x80 << 12) /* 512KB */
> #define BXT_GUC_WOPCM_RC6_RESERVED (0x10 << 12) /* 64KB */
> diff --git a/drivers/gpu/drm/i915/intel_guc.c
> b/drivers/gpu/drm/i915/intel_guc.c
> index 55a0158..73be382 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -65,6 +65,8 @@ void intel_guc_init_early(struct intel_guc *guc)
> mutex_init(&guc->send_mutex);
> guc->send = intel_guc_send_nop;
> guc->notify = gen8_guc_raise_irq;
> + guc->suspended = false;
> + guc->skip_load_on_resume = false;
> }
> int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
> diff --git a/drivers/gpu/drm/i915/intel_guc.h
> b/drivers/gpu/drm/i915/intel_guc.h
> index a587210..9f84033 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -38,6 +38,9 @@ struct intel_guc {
> struct intel_guc_log log;
> struct intel_guc_ct ct;
> + bool suspended;
> + bool skip_load_on_resume;
maybe bool xxx:1 to save space.
> +
> /* Log snapshot if GuC errors during load */
> struct drm_i915_gem_object *load_err_log;
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index 1365724..f641872 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -152,14 +152,35 @@ static void guc_disable_communication(struct
> intel_guc *guc)
> guc->send = intel_guc_send_nop;
> }
> +static inline bool guc_wopcm_locked(struct intel_guc *guc)
> +{
> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> + return I915_READ(GUC_WOPCM_SIZE) & GUC_WOPCM_LOCKED;
what about adding !! trick?
> +}
> +
> int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> {
> struct intel_guc *guc = &dev_priv->guc;
> + struct intel_huc *huc = &dev_priv->huc;
> int ret, attempts;
> if (!i915_modparams.enable_guc_loading)
> return 0;
> + /*
> + * If on resume from sleep GuC was available we resumed GuC during
> + * i915_gem_resume. We need to skip load here. Reset
> skip_load_on_resume
> + * to allow load during module reload/reset/next resume behavior.
> + */
> + if (guc->skip_load_on_resume) {
> + guc->skip_load_on_resume = false;
> + return 0;
> + }
> +
> + WARN_ON_ONCE(guc->fw.load_status == INTEL_UC_FIRMWARE_SUCCESS);
> + WARN_ON_ONCE(huc->fw.load_status == INTEL_UC_FIRMWARE_SUCCESS);
> +
> guc_disable_communication(guc);
> gen9_reset_guc_interrupts(dev_priv);
> @@ -197,8 +218,8 @@ int intel_uc_init_hw(struct drm_i915_private
> *dev_priv)
> if (ret)
> goto err_submission;
> - intel_huc_init_hw(&dev_priv->huc);
> - ret = intel_guc_init_hw(&dev_priv->guc);
> + intel_huc_init_hw(huc);
> + ret = intel_guc_init_hw(guc);
> if (ret == 0 || ret != -EAGAIN)
> break;
> @@ -214,7 +235,21 @@ int intel_uc_init_hw(struct drm_i915_private
> *dev_priv)
> if (ret)
> goto err_log_capture;
> - intel_huc_auth(&dev_priv->huc);
> + /*
> + * If WOPCM was not locked during resume from sleep, GuC/HuC need to
> + * be reloaded. For this we are using intel_uc_init_hw path as we want
> + * to handle HuC/GuC reload, GuC resume, HuC authentication and
> + * submission enabling etc. all here.
> + */
> + if (guc->suspended) {
> + ret = intel_guc_resume(guc);
> + if (ret)
> + DRM_ERROR("GuC resume failed (%d)."
> + " GuC functions may not work\n", ret);
> + guc->suspended = false;
> + }
> +
> + intel_huc_auth(huc);
> if (i915_guc_submission_initialized(guc)) {
> if (i915_modparams.guc_log_level >= 0)
> gen9_enable_guc_interrupts(dev_priv);
> @@ -305,6 +340,8 @@ int intel_uc_suspend(struct drm_i915_private
> *dev_priv)
> gen9_disable_guc_interrupts(dev_priv);
> guc_disable_communication(guc);
> + guc->suspended = true;
> +
> goto out;
> out_suspend:
> @@ -326,28 +363,50 @@ int intel_uc_suspend(struct drm_i915_private
> *dev_priv)
> void intel_uc_resume(struct drm_i915_private *dev_priv)
> {
> struct intel_guc *guc = &dev_priv->guc;
> + struct intel_huc *huc = &dev_priv->huc;
> int ret;
> - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> - return;
> -
> - ret = guc_enable_communication(guc);
> - if (ret) {
> - DRM_DEBUG_DRIVER("GuC communication enable failed (%d)\n", ret);
> + if (!guc->suspended)
> return;
> - }
> - if (i915_modparams.guc_log_level >= 0)
> - gen9_enable_guc_interrupts(dev_priv);
> + /*
> + * If WOPCM is locked then GuC and HuC are still loaded. We just
> + * need to enable communication with GuC, enable interrupts,
> + * invoke GuC action to resume from sleep and enable submission.
> + * If WOPCM is not locked it is similar to fresh boot and we need
> + * reload the GuC/HuC firmwares and enable other GuC related
> + * mechanisms. Post reloading GuC we need to send action to resume
> + * from sleep for GuC to restore its state prior to suspend.
> + */
> + if (guc_wopcm_locked(guc)) {
> + huc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
> + guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
Hmm, as we didn't clear load_status (or at least I can't find it) then
load_status should still be INTEL_UC_FIRMWARE_SUCCESS here.
Maybe instead of introducing new skip_load_on_resume flag we can
just rely on load_status that is correctly maintained across suspend
resume.
> - ret = intel_guc_resume(guc);
> - if (ret)
> - DRM_ERROR("GuC resume failed (%d)."
> - "GuC functions may not work\n", ret);
> + ret = guc_enable_communication(guc);
> + if (ret) {
> + DRM_DEBUG_DRIVER("GuC communication enable failed"
> + " (%d)\n", ret);
Btw, the only path in guc_enable_communication that can fail reports
error so maybe this extra debug is not necessary ?
Maybe better option will be to reorg this code to include note about
forced GuC reload during init_hw ?
> + return;
> + }
> - i915_guc_submission_enable(dev_priv);
> + if (i915_modparams.guc_log_level >= 0)
> + gen9_enable_guc_interrupts(dev_priv);
> - DRM_DEBUG_DRIVER("GuC submission %s\n",
> - i915_guc_submission_enabled(guc) ?
> - "enabled" : "disabled");
> + ret = intel_guc_resume(guc);
> + if (ret)
> + DRM_ERROR("GuC resume failed (%d)."
> + " GuC functions may not work\n", ret);
> +
Hmm, is it safe to continue with submission after failing resume ?
> + i915_guc_submission_enable(dev_priv);
> +
> + DRM_DEBUG_DRIVER("GuC submission %s\n",
> + i915_guc_submission_enabled(guc) ?
> + "enabled" : "disabled");
> + guc->suspended = false;
> + guc->skip_load_on_resume = true;
> + } else {
> + DRM_DEBUG_DRIVER("GuC not available. Resume will be done"
> + " during i915_gem_init_hw\n");
> + guc->skip_load_on_resume = false;
> + }
> }
More information about the Intel-gfx
mailing list