[Intel-gfx] [PATCH v13 03/21] drm/i915/guc: Add status checks to enable/disable_guc_interrupts

[Michal Wajdeczko]

On Wed, 11 Oct 2017 10:53:58 +0200, Sagar Arun Kamble  
<sagar.a.kamble at intel.com> wrote:

> GuC interrupts are currently enabled by Logging and disabled in different
> scenarios. Make disabling check whether interrupts were already disabled
> and similar for enable path. This will remove the state tracking for the
> callers of these functions based on kernel parameters.
>
> 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_irq.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c  
> b/drivers/gpu/drm/i915/i915_irq.c
> index a3de408..6cf417c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -455,18 +455,22 @@ void gen9_reset_guc_interrupts(struct  
> drm_i915_private *dev_priv)
> void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
>  {
> +	if (READ_ONCE(dev_priv->guc.interrupts_enabled))

Hmm, I don't like that functions from irq.c read and modify guc internal
members directly. I would expect that functions here just do their job
and any state is maintained by the helper function(s) in guc.c.

Also note that this change will not help scenario where one client will
try to disable irqs while other client still depends on them.

Michal

> +		return;
> +
>  	spin_lock_irq(&dev_priv->irq_lock);
> -	if (!dev_priv->guc.interrupts_enabled) {
> -		WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> -				       dev_priv->pm_guc_events);
> -		dev_priv->guc.interrupts_enabled = true;
> -		gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> -	}
> +	WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> +			       dev_priv->pm_guc_events);
> +	dev_priv->guc.interrupts_enabled = true;
> +	gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
> void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
>  {
> +	if (!READ_ONCE(dev_priv->guc.interrupts_enabled))
> +		return;
> +
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	dev_priv->guc.interrupts_enabled = false;