Re: [PATCH 07/11] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

On 10/19/2017 3:39 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>> Disabling GuC interrupts involves access to GuC IRQ control registers
>> hence ensure device is RPM awake.
>>
>> v2: Add comment about need to synchronize flush work and log runtime
>>      destroy
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_guc_log.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index f87e9f5..ed239cb 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -656,8 +656,17 @@ void i915_guc_log_unregister(struct 
>> drm_i915_private *dev_priv)
>>   {
>>       mutex_lock(&dev_priv->drm.struct_mutex);
>>       /* GuC logging is currently the only user of Guc2Host 
>> interrupts */
>> -    if (i915_modparams.guc_log_level >= 0)
>> +    if (i915_modparams.guc_log_level >= 0) {
>> +        intel_runtime_pm_get(dev_priv);
>>           intel_disable_guc_interrupts(&dev_priv->guc);
>> +        intel_runtime_pm_put(dev_priv);
>
> Is it possible to trigger the assert from I915_WRITE today and if so 
> which test case?
drv_module_reload/basic-reload
>
>> +    }
>> +    /*
>> +     * TODO: Need to synchronize access to relay channel from flush 
>> work
>> +     * and release here if interrupt stays enabled from hereon.
>> +     * Possibly with GuC CT recv. interrupts will stay enabled until 
>> GEM
>> +     * suspend/unload.
>> +     */
>
> I think we normally don't put such reminders in code. Regardless if it 
> is going away in this patch series or not it looks equally pointless 
> to me.
Ok. Will remove this. Will need to be noted when we add CT buf recv 
support.
>
>> guc_log_runtime_destroy(&dev_priv->guc);
>
> Ha right, this is how the cleanup happens what I was wondering in the 
> previous patch. So intel_guc_log_destroy is pretty pointless now since 
> it effectively does only this. Or I missed something?
Yes. Part of intel_guc_log_destroy to unpin and release log vma is 
needed during uc_fini_hw. Will remove this function then as it is only 
used during init failure and we can open code there.
>
>> mutex_unlock(&dev_priv->drm.struct_mutex);
>>   }
>>
>
> Regards,
>
> Tvrtko

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