Re: [PATCH v13 13/21] drm/i915/uc: Support resume from sleep w/ and w/o GuC/HuC reload

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

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@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@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@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;
>> +    }
>>  }

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx