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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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?

> +	}
> +	/*
> +	 * 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.

>   	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?

>   	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