[Intel-gfx] [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions

[Sagar Arun Kamble]

On 10/11/2017 11:05 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:54:14 +0200, Sagar Arun Kamble 
> <sagar.a.kamble at intel.com> wrote:
>
>> i915_ggtt_enable_guc has to happen first during i915_gem_resume
>> if GuC loading is enabled before GTT restore. In case GuC is not
>> loaded this enabling happening during intel_uc_init_hw need to
>> skipped. (avoid the GEM_BUG_ON)
>> i915_ggtt_disable_guc at the end of reset/suspend/unload is needed
>> post GGTT suspend operations. Calling it during uc_sanitize covers
>> all scenarios. Hence, it is removed from intel_uc_fini_hw. Also these
>> needto be protected by struct_mutex. Hence struct_mutex locking is
>> added in i915_gem_sanitize while sanitizing uC. struct_mutex is already
>> held during i915_gem_reset_prepare.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Michał Winiarski <michal.winiarski at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem.c |  4 ++++
>>  drivers/gpu/drm/i915/intel_uc.c | 16 +++++++++++-----
>>  2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index a4bbf6c..77a0746 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4759,6 +4759,10 @@ void i915_gem_resume(struct drm_i915_private 
>> *dev_priv)
>>      WARN_ON(dev_priv->gt.awake);
>>     mutex_lock(&dev->struct_mutex);
>> +    /* We need to notify the guc whenever we change the GGTT */
>> +    if (i915_modparams.enable_guc_loading)
>> +        i915_ggtt_enable_guc(dev_priv);
>> +
>>      i915_gem_restore_gtt_mappings(dev_priv);
>>      i915_gem_restore_fences(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 9010ab5..0b799fe 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -184,8 +184,14 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>      guc_disable_communication(guc);
>>      gen9_reset_guc_interrupts(dev_priv);
>> -    /* We need to notify the guc whenever we change the GGTT */
>> -    i915_ggtt_enable_guc(dev_priv);
>> +    /*
>> +     * We need to notify the guc whenever we change the GGTT.
>> +     * During resume from sleep we would have already updated the
>> +     * GGTT invalidate function for GuC during i915_gem_resume so
>> +     * we need to skip here. Will enable here on driver load/reset.
>> +     */
>> +    if (!guc->suspended)
>> +        i915_ggtt_enable_guc(dev_priv);
>>     if (i915_modparams.enable_guc_submission) {
>>          /*
>> @@ -309,9 +315,6 @@ void intel_uc_cleanup(struct drm_i915_private 
>> *dev_priv)
>>      guc_free_load_err_log(guc);
>>     i915_guc_submission_cleanup(dev_priv);
>> -
>> -    if (i915_modparams.enable_guc_loading)
>> -        i915_ggtt_disable_guc(dev_priv);
>>  }
>> /**
>> @@ -452,6 +455,9 @@ void intel_uc_sanitize(struct drm_i915_private 
>> *dev_priv)
>>      struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>>     if (i915_modparams.enable_guc_loading) {
>> +        if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
>
> Hmm, isn't that check redundant ?
uc_sanitize can happen without firmware loaded too in which case we 
don't want to ggtt_disable_guc.
if we want to ggtt_disable_guc then we should remove the GEM_BUG_ON in it.
>
>> + i915_ggtt_disable_guc(dev_priv);
>> +
>>          guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>>          huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>>      }
>
> Btw, what should we do with "suspended" flag during sanitize ?
suspended flag is set to true on suspend and false on resume.
sanitize is done post suspend and before resume so we should not touch it.
initializing it to false during guc_init_early should take care of 
reload (during unload we are suspending gem but we wont resume)