[Intel-gfx] [PATCH 6/6] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts

[Kamble, Sagar A]

On 9/14/2017 9:41 PM, Michal Wajdeczko wrote:
> On Thu, 14 Sep 2017 18:04:27 +0200, Kamble, Sagar A 
> <sagar.a.kamble at intel.com> wrote:
>
>>
>>
>> On 9/14/2017 6:07 PM, Michal Wajdeczko wrote:
>>> On Thu, 14 Sep 2017 11:55:08 +0200, Sagar Arun Kamble 
>>> <sagar.a.kamble at intel.com> wrote:
>>>
>>>> From: "Kamble, Sagar A" <sagar.a.kamble at intel.com>
>>>>
>>>> Disabling GuC interrupts involves access to GuC IRQ control registers
>>>> hence ensure device is RPM awake.
>>>>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_guc_log.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>>>> b/drivers/gpu/drm/i915/intel_guc_log.c
>>>> index ba36162..d7557b5 100644
>>>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>>>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>>>> @@ -657,8 +657,10 @@ void i915_guc_log_unregister(struct 
>>>> drm_i915_private *dev_priv)
>>>>          return;
>>>>     mutex_lock(&dev_priv->drm.struct_mutex);
>>>> +    intel_runtime_pm_get(dev_priv);
>>>>      /* GuC logging is currently the only user of Guc2Host 
>>>> interrupts */
>>>>      gen9_disable_guc_interrupts(dev_priv);
>>>> +    intel_runtime_pm_put(dev_priv);
>>>>      guc_log_runtime_destroy(&dev_priv->guc);
>>>
>>> Maybe we should just destroy runtime here, and allow irq to be disabled
>>> by intel_uc_fini_hw() which will be called right after. This will also
>>> fix the upcoming case when log will not be the only user of Guc irqs.
>>>
>>> See https://patchwork.freedesktop.org/patch/170390/
>>>
>>> Michal
>> Destroying runtime here may create issues as enabled GuC interrupts 
>> may be causing the use of the guc_log_runtime.
>
> Yes, but this should be easy to fix.
>
>> Should we move the i915_driver_unregister post i915_gem_unload?
>
> But then we will have to handle the case when gem was unloaded but
> driver will still be in registered state.
>
> Note that as guc log will not be the only user of the guc irqs,
> code that disables irq will be removed from this function anyway.
>
> Michal
I see now that guc_log_runtime_destroy already happen along 
intel_uc_fini_hw->i915_guc_submission_fini->intel_guc_log_destroy path too.
So we can remove guc_log_unregister completely with irq_disable 
happening as part of intel_guc_unload and guc_log_runtime_destroy too 
handled.
>
>>>
>>>> mutex_unlock(&dev_priv->drm.struct_mutex);
>>>>  }