Re: [PATCH 08/11] drm/i915/guc: Add client support to enable/disable GuC interrupts

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

On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> This patch adds support to enable/disable GuC interrupts for different
> features without impacting others needs. Currently GuC log capture and
> CT buffer receive mechanisms use the GuC interrupts. GuC interrupts are
> currently enabled and disabled in different Logging scenarios all gated
> by log level.
> 
> v2: Rebase with all GuC interrupt handlers moved to intel_guc.c. Handling
> multiple clients for GuC interrupts enable/disable.
> (Michal Wajdeczko)
> 
> 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/i915_debugfs.c  |  8 ++++++++
>   drivers/gpu/drm/i915/intel_guc.c     | 21 ++++++++++++++-------
>   drivers/gpu/drm/i915/intel_guc.h     | 11 ++++++++---
>   drivers/gpu/drm/i915/intel_guc_log.c |  6 +++---
>   drivers/gpu/drm/i915/intel_uc.c      |  6 +++---
>   5 files changed, 36 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0bb6e01..bd421da 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2531,6 +2531,14 @@ static int i915_guc_info(struct seq_file *m, void *data)
>   	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>   	const struct intel_guc *guc = &dev_priv->guc;
>   
> +	seq_puts(m, "GuC Interrupt Clients: ");
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	if (guc->interrupt_clients & BIT(GUC_INTR_CLIENT_LOG))

Could use test_bit.

Also, spinlock not required here apart from documentation purposes?

> +		seq_puts(m, "GuC Logging\n");
> +	else
> +		seq_puts(m, "None\n");
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +
>   	if (!check_guc_submission(m))
>   		return 0;
>   
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 959057a..fbd27ea 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -275,7 +275,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
>   		return 0;
>   
>   	if (i915_modparams.guc_log_level >= 0)
> -		intel_disable_guc_interrupts(guc);
> +		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>   
>   	ctx = dev_priv->kernel_context;
>   
> @@ -303,7 +303,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
>   		return 0;
>   
>   	if (i915_modparams.guc_log_level >= 0)
> -		intel_enable_guc_interrupts(guc);
> +		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>   
>   	ctx = dev_priv->kernel_context;
>   
> @@ -378,26 +378,33 @@ void intel_reset_guc_interrupts(struct intel_guc *guc)
>   	spin_unlock_irq(&dev_priv->irq_lock);
>   }
>   
> -void intel_enable_guc_interrupts(struct intel_guc *guc)
> +void intel_get_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   
>   	spin_lock_irq(&dev_priv->irq_lock);
> -	if (!dev_priv->guc.interrupts_enabled) {
> +	if (!guc->interrupt_clients) {
>   		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);
>   	}
> +	set_bit(id, &guc->interrupt_clients);

Can use __set_bit to save one atomic since it is already under the spinlock.

> +
>   	spin_unlock_irq(&dev_priv->irq_lock);
>   }
>   
> -void intel_disable_guc_interrupts(struct intel_guc *guc)
> +void intel_put_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   
>   	spin_lock_irq(&dev_priv->irq_lock);
> -	dev_priv->guc.interrupts_enabled = false;
> +
> +	clear_bit(id, &guc->interrupt_clients);

Equally as above __clear_bit would work.

> +
> +	if (guc->interrupt_clients) {
> +		spin_unlock_irq(&dev_priv->irq_lock);
> +		return;
> +	}
>   
>   	gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>   
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index e89b4ae..4d58bf7 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -34,6 +34,11 @@
>   #include "i915_guc_reg.h"
>   #include "i915_vma.h"
>   
> +enum guc_intr_client {
> +	GUC_INTR_CLIENT_LOG = 0,
> +	GUC_INTR_CLIENT_COUNT
> +};
> +
>   /*
>    * Top level structure of GuC. It handles firmware loading and manages client
>    * pool and doorbells. intel_guc owns a i915_guc_client to replace the legacy
> @@ -48,7 +53,7 @@ struct intel_guc {
>   	struct drm_i915_gem_object *load_err_log;
>   
>   	/* intel_guc_recv interrupt related state */
> -	bool interrupts_enabled;
> +	unsigned long interrupt_clients;
>   
>   	struct i915_vma *ads_vma;
>   	struct i915_vma *stage_desc_pool;
> @@ -117,8 +122,8 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>   struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
>   u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>   void intel_reset_guc_interrupts(struct intel_guc *guc);
> -void intel_enable_guc_interrupts(struct intel_guc *guc);
> -void intel_disable_guc_interrupts(struct intel_guc *guc);
> +void intel_get_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id);
> +void intel_put_guc_interrupts(struct intel_guc *guc, enum guc_intr_client id);
>   void intel_guc_irq_handler(struct intel_guc *guc, u32 pm_iir);
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index ed239cb..8c41d9a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -508,7 +508,7 @@ static void guc_flush_logs(struct intel_guc *guc)
>   		return;
>   
>   	/* First disable the interrupts, will be renabled afterwards */
> -	intel_disable_guc_interrupts(guc);
> +	intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>   
>   	/* Before initiating the forceful flush, wait for any pending/ongoing
>   	 * flush to complete otherwise forceful flush may not actually happen.
> @@ -626,7 +626,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>   		}
>   
>   		/* GuC logging is currently the only user of Guc2Host interrupts */
> -		intel_enable_guc_interrupts(guc);
> +		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>   	} else {
>   		/* Once logging is disabled, GuC won't generate logs & send an
>   		 * interrupt. But there could be some data in the log buffer
> @@ -658,7 +658,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>   	/* GuC logging is currently the only user of Guc2Host interrupts */
>   	if (i915_modparams.guc_log_level >= 0) {
>   		intel_runtime_pm_get(dev_priv);
> -		intel_disable_guc_interrupts(&dev_priv->guc);
> +		intel_put_guc_interrupts(&dev_priv->guc, GUC_INTR_CLIENT_LOG);
>   		intel_runtime_pm_put(dev_priv);
>   	}
>   	/*
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 95c5ec4..d96c847 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -213,7 +213,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   		goto err_log_capture;
>   
>   	if (i915_modparams.guc_log_level >= 0)
> -		intel_enable_guc_interrupts(guc);
> +		intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>   
>   	ret = guc_enable_communication(guc);
>   	if (ret)
> @@ -247,7 +247,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	guc_disable_communication(guc);
>   err_interrupts:
>   	if (i915_modparams.guc_log_level >= 0)
> -		intel_disable_guc_interrupts(guc);
> +		intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
>   err_log_capture:
>   	guc_capture_load_err_log(guc);
>   err_submission:
> @@ -288,7 +288,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>   	guc_disable_communication(&dev_priv->guc);
>   
>   	if (i915_modparams.guc_log_level >= 0)
> -		intel_disable_guc_interrupts(&dev_priv->guc);
> +		intel_put_guc_interrupts(&dev_priv->guc, GUC_INTR_CLIENT_LOG);
>   
>   	if (i915_modparams.enable_guc_submission)
>   		i915_guc_submission_fini(dev_priv);
> 

With the bit ops tidy:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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