[v3,11/12] drm/i915/guc: Restore GuC interrupts across suspend/reset if enabled
diff mbox

Message ID 1515082914-4111-12-git-send-email-sagar.a.kamble@intel.com
State New
Headers show

Commit Message

sagar.a.kamble@intel.com Jan. 4, 2018, 4:21 p.m. UTC
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.

Further restructuring of runtime_pm_enable/disable_interrupts and
suspend/restore_guc_interrupts will be done in upcoming patches.

v2: Rebase.

v3: Updated suspend/restore with the new low level get/put functions.
(Tvrtko)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  2 ++
 drivers/gpu/drm/i915/intel_guc.c     | 32 ++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_guc.h     |  2 ++
 3 files changed, 32 insertions(+), 4 deletions(-)

Comments

Michal Wajdeczko Jan. 4, 2018, 5:49 p.m. UTC | #1
On Thu, 04 Jan 2018 17:21:53 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> 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.
>
> Further restructuring of runtime_pm_enable/disable_interrupts and
> suspend/restore_guc_interrupts will be done in upcoming patches.
>
> v2: Rebase.
>
> v3: Updated suspend/restore with the new low level get/put functions.
> (Tvrtko)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  2 ++
>  drivers/gpu/drm/i915/intel_guc.c     | 32  
> ++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_guc.h     |  2 ++
>  3 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c  
> b/drivers/gpu/drm/i915/intel_display.c
> index 0cd3559..2e0db53 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3676,8 +3676,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 d356c40..28a418a 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -406,8 +406,7 @@ int intel_guc_suspend(struct drm_i915_private  
> *dev_priv)
>  	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>  		return 0;
> -	if (guc->log.level >= 0)
> -		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
> +	intel_suspend_guc_interrupts(guc);

Hmm, maybe we should introduce

	intel_uc_suspend(struct drm_i915_private *dev_priv)

which will call separately

	intel_guc_suspend(guc); /* send suspend action */
	intel_guc_suspend_interrupts(guc);

> 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>  	/* any value greater than GUC_POWER_D0 */
> @@ -452,8 +451,7 @@ int intel_guc_resume(struct drm_i915_private  
> *dev_priv)
>  	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>  		return 0;
> -	if (guc->log.level >= 0)
> -		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
> +	intel_restore_guc_interrupts(guc);
> 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>  	data[1] = GUC_POWER_D0;
> @@ -548,6 +546,16 @@ void intel_get_guc_interrupts(struct intel_guc  
> *guc, enum guc_intr_client id)
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
> +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)
> +		__intel_get_guc_interrupts(guc);
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
>  static void __intel_put_guc_interrupts(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> @@ -576,6 +584,22 @@ void intel_put_guc_interrupts(struct intel_guc  
> *guc, enum guc_intr_client id)
>  	intel_reset_guc_interrupts(guc);
>  }
> +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;
> +	}
> +	__intel_put_guc_interrupts(guc);
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +	synchronize_irq(dev_priv->drm.irq);
> +
> +	intel_reset_guc_interrupts(guc);
> +}
> +
>  void intel_guc_irq_handler(struct intel_guc *guc, u32 gt_iir)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
> b/drivers/gpu/drm/i915/intel_guc.h
> index af74392..2c14781 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -140,5 +140,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);

To match obj-verb pattern:

intel_guc_suspend_interrupts
intel_guc_restore_interrupts

> #endif
sagar.a.kamble@intel.com Jan. 5, 2018, 5:13 a.m. UTC | #2
On 1/4/2018 11:19 PM, Michal Wajdeczko wrote:
> On Thu, 04 Jan 2018 17:21:53 +0100, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> 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.
>>
>> Further restructuring of runtime_pm_enable/disable_interrupts and
>> suspend/restore_guc_interrupts will be done in upcoming patches.
>>
>> v2: Rebase.
>>
>> v3: Updated suspend/restore with the new low level get/put functions.
>> (Tvrtko)
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |  2 ++
>>  drivers/gpu/drm/i915/intel_guc.c     | 32 
>> ++++++++++++++++++++++++++++----
>>  drivers/gpu/drm/i915/intel_guc.h     |  2 ++
>>  3 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c 
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 0cd3559..2e0db53 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3676,8 +3676,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 d356c40..28a418a 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -406,8 +406,7 @@ int intel_guc_suspend(struct drm_i915_private 
>> *dev_priv)
>>      if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>          return 0;
>> -    if (guc->log.level >= 0)
>> -        intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>> +    intel_suspend_guc_interrupts(guc);
>
> Hmm, maybe we should introduce
>
>     intel_uc_suspend(struct drm_i915_private *dev_priv)
>
> which will call separately
>
>     intel_guc_suspend(guc); /* send suspend action */
>     intel_guc_suspend_interrupts(guc);
>
Yes. Ordering was not correct. I have this change as part of 
suspend/resume restructuring from GuC/GEM which will follow
after this. Will keep as it is for now in this patch.
>>     data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>>      /* any value greater than GUC_POWER_D0 */
>> @@ -452,8 +451,7 @@ int intel_guc_resume(struct drm_i915_private 
>> *dev_priv)
>>      if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>          return 0;
>> -    if (guc->log.level >= 0)
>> -        intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>> +    intel_restore_guc_interrupts(guc);
>>     data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>>      data[1] = GUC_POWER_D0;
>> @@ -548,6 +546,16 @@ void intel_get_guc_interrupts(struct intel_guc 
>> *guc, enum guc_intr_client id)
>>      spin_unlock_irq(&dev_priv->irq_lock);
>>  }
>> +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)
>> +        __intel_get_guc_interrupts(guc);
>> +    spin_unlock_irq(&dev_priv->irq_lock);
>> +}
>> +
>>  static void __intel_put_guc_interrupts(struct intel_guc *guc)
>>  {
>>      struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> @@ -576,6 +584,22 @@ void intel_put_guc_interrupts(struct intel_guc 
>> *guc, enum guc_intr_client id)
>>      intel_reset_guc_interrupts(guc);
>>  }
>> +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;
>> +    }
>> +    __intel_put_guc_interrupts(guc);
>> +    spin_unlock_irq(&dev_priv->irq_lock);
>> +    synchronize_irq(dev_priv->drm.irq);
>> +
>> +    intel_reset_guc_interrupts(guc);
>> +}
>> +
>>  void intel_guc_irq_handler(struct intel_guc *guc, u32 gt_iir)
>>  {
>>      struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h 
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index af74392..2c14781 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -140,5 +140,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);
>
> To match obj-verb pattern:
>
> intel_guc_suspend_interrupts
> intel_guc_restore_interrupts
>
Yes. Will update.

Thanks
Sagar
>> #endif

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0cd3559..2e0db53 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3676,8 +3676,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 d356c40..28a418a 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -406,8 +406,7 @@  int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	if (guc->log.level >= 0)
-		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
+	intel_suspend_guc_interrupts(guc);
 
 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
 	/* any value greater than GUC_POWER_D0 */
@@ -452,8 +451,7 @@  int intel_guc_resume(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	if (guc->log.level >= 0)
-		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
+	intel_restore_guc_interrupts(guc);
 
 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
 	data[1] = GUC_POWER_D0;
@@ -548,6 +546,16 @@  void intel_get_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id)
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
+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)
+		__intel_get_guc_interrupts(guc);
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
 static void __intel_put_guc_interrupts(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -576,6 +584,22 @@  void intel_put_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id)
 	intel_reset_guc_interrupts(guc);
 }
 
+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;
+	}
+	__intel_put_guc_interrupts(guc);
+	spin_unlock_irq(&dev_priv->irq_lock);
+	synchronize_irq(dev_priv->drm.irq);
+
+	intel_reset_guc_interrupts(guc);
+}
+
 void intel_guc_irq_handler(struct intel_guc *guc, u32 gt_iir)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index af74392..2c14781 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -140,5 +140,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