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

From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
To: Michal Wajdeczko <michal.wajdeczko@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v13 03/21] drm/i915/guc: Add status checks to enable/disable_guc_interrupts
Date: Thu, 12 Oct 2017 11:20:49 +0530	[thread overview]
Message-ID: <0ba36524-dc7d-75d9-e0ce-5342e3eacb10@intel.com> (raw)
In-Reply-To: <op.y7x970j0xaggs7@mwajdecz-mobl1.ger.corp.intel.com>


On 10/11/2017 8:50 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:53:58 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@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@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@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.
Sure will move to 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.
>
Will add refcounting then.
> 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;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx