[Intel-gfx] [PATCH 11/11] drm/i915/guc: Restore GuC interrupts across suspend/reset if enabled

[Sagar Arun Kamble]

On 10/19/2017 4:33 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:47, Sagar Arun Kamble wrote:
>> In order to override the disable/enable control of GuC interrupts during
>> suspend/reset cycle we are creating two new functions suspend/restore
>> guc_interrupts which check if interrupts were enabled and disable them
>> on suspend and enable them on resume. They are used to restore 
>> interrupts
>> across reset as well.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin 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/intel_display.c |  2 ++
>>   drivers/gpu/drm/i915/intel_guc.c     | 40 
>> ++++++++++++++++++++++++++++++++----
>>   drivers/gpu/drm/i915/intel_guc.h     |  2 ++
>>   3 files changed, 40 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c 
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 897fe7e..742ab5e 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3768,8 +3768,10 @@ void intel_finish_reset(struct 
>> drm_i915_private *dev_priv)
>>            * The display has been reset as well,
>>            * so need a full re-initialization.
>>            */
>> +        intel_suspend_guc_interrupts(&dev_priv->guc);
>>           intel_runtime_pm_disable_interrupts(dev_priv);
>>           intel_runtime_pm_enable_interrupts(dev_priv);
>> +        intel_restore_guc_interrupts(&dev_priv->guc);
>>             intel_pps_unlock_regs_wa(dev_priv);
>>           intel_modeset_init_hw(dev);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index fbd27ea..1e5abf2 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -261,6 +261,25 @@ int intel_guc_auth_huc(struct intel_guc *guc, 
>> u32 rsa_offset)
>>       return intel_guc_send(guc, action, ARRAY_SIZE(action));
>>   }
>>   +void intel_suspend_guc_interrupts(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>> +    spin_lock_irq(&dev_priv->irq_lock);
>> +
>> +    if (!guc->interrupt_clients) {
>> +        spin_unlock_irq(&dev_priv->irq_lock);
>> +        return;
>> +    }
>> +
>> +    gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>> +
>> +    spin_unlock_irq(&dev_priv->irq_lock);
>> +    synchronize_irq(dev_priv->drm.irq);
>> +
>> +    intel_reset_guc_interrupts(guc);
>> +}
>
> Looks awfully similar to intel_put_guc_interrupts, minus the 
> guc->interrupt_clients handling and single bit vs all. I am thinking 
> if it could be consolidated somehow...
>
> Maybe add __intel_put_guc_interrupts which does not touch 
> guc->interrupt_clients and takes in a bitmask of all interrupt clients?
>
> void __intel_put_guc_interrupts(..., unsigned long mask)
> {
>     assert_lock_held...
>
>     if ((mask & guc->interrupt_clients) != guc->interrupt_clients)
>         return;
>
>     ... do the irq disable stuff..
> }
>
> void intel_suspend_guc_interrupts(...)
> {
>     spin_lock...
>
>     __intel_put_guc_interrupts(.., guc->interrupt_clients);
>
>     spin_unlock...
> }
>
> void intel_put_guc_interrupts(..., id)
> {
>     spin_lock...
>
>     __intel_put_guc_interrupts(.., BIT(id));
>     __clear_bit(guc->interrupt_clients, id);
>
>     spin_unlock...
> }
>
> Maybe some logic mistakes here, beware. :)
Sure. will update.
>
>> +
>>   /**
>>    * intel_guc_suspend() - notify GuC entering suspend state
>>    * @dev_priv:    i915 device private
>> @@ -274,8 +293,7 @@ int intel_guc_suspend(struct drm_i915_private 
>> *dev_priv)
>>       if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>           return 0;
>>   -    if (i915_modparams.guc_log_level >= 0)
>> -        intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>> +    intel_suspend_guc_interrupts(guc);
>>         ctx = dev_priv->kernel_context;
>>   @@ -289,6 +307,21 @@ int intel_guc_suspend(struct drm_i915_private 
>> *dev_priv)
>>       return intel_guc_send(guc, data, ARRAY_SIZE(data));
>>   }
>>   +void intel_restore_guc_interrupts(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>> +    spin_lock_irq(&dev_priv->irq_lock);
>> +
>> +    if (guc->interrupt_clients) {
>> +        WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>> +                       dev_priv->pm_guc_events);
>> +        gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>> +    }
>> +
>> +    spin_unlock_irq(&dev_priv->irq_lock);
>> +}
>
> And ofc something similar like the above to consolidate the 
> get/restore as well.
Yes.
>
>> +
>>   /**
>>    * intel_guc_resume() - notify GuC resuming from suspend state
>>    * @dev_priv:    i915 device private
>> @@ -302,8 +335,7 @@ int intel_guc_resume(struct drm_i915_private 
>> *dev_priv)
>>       if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>           return 0;
>>   -    if (i915_modparams.guc_log_level >= 0)
>> -        intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>> +    intel_restore_guc_interrupts(guc);
>>         ctx = dev_priv->kernel_context;
>>   diff --git a/drivers/gpu/drm/i915/intel_guc.h 
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index 4d58bf7..c55dcba 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -125,5 +125,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma 
>> *vma)
>>   void intel_get_guc_interrupts(struct intel_guc *guc, enum 
>> guc_intr_client id);
>>   void intel_put_guc_interrupts(struct intel_guc *guc, enum 
>> guc_intr_client id);
>>   void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
>> +void intel_suspend_guc_interrupts(struct intel_guc *guc);
>> +void intel_restore_guc_interrupts(struct intel_guc *guc);
>>     #endif
>>
>
> Regards,
>
> Tvrtko
Thanks again for review on the series.