[Intel-gfx] [PATCH v13 13/21] drm/i915/uc: Support resume from sleep w/ and w/o GuC/HuC reload
Sagar Arun Kamble
sagar.a.kamble at intel.com
Thu Oct 12 06:48:48 UTC 2017
On 10/11/2017 10:36 PM, Michal Wajdeczko wrote:
> 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.
Ok. Will update.
>
>> +
>> /* 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?
Sure. Will add.
>
>> +}
>> +
>> 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.
load status will be cleared in the uc_sanitize and that patch was later
in the sequence.
Have pulled earlier in the v14.
We can drop the skip_load_on_resume as you suggested. will update.
>
>> - 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 ?
ok will remove.
>
> Maybe better option will be to reorg this code to include note about
> forced GuC reload during init_hw ?
ok.
>
>> + 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 ?
It may not so will revert all earlier steps and fall back to execlists.
>
>> + 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