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

[Tvrtko Ursulin]

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. :)

> +
>   /**
>    * 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.

> +
>   /**
>    * 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