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

[Sagar Arun Kamble]

On 10/12/2017 11:47 AM, Sagar Arun Kamble wrote:
>
>
> On 10/12/2017 11:20 AM, Sagar Arun Kamble wrote:
>>
>>
>> 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 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.
>> 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.
> realized that disable_guc_interrupts is currently happening twice 
> during unload and that can make
> refcounting asymmetrical. So will stay with bool state for now and 
> will revisit during interrupts related
> changes may be as precursor to GuC CT series.
Based on current interrupt mgmt - pm_ier/pm_imr are already handled by 
intel_runtime_pm_*_interrupts.
And these are being done across suspend/reset properly. Just need to 
order w.r.t GuC suspend/resume.
So gen9_*_guc_interrupts should not be called during fini. They will be 
taken care of based on interrupt clients like
Logging, CT Buffer.
>>> 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>