[Intel-gfx] [PATCH v3 10/12] drm/i915/guc: Add client support to enable/disable GuC interrupts
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Jan 4 17:39:34 UTC 2018
On Thu, 04 Jan 2018 17:21:52 +0100, Sagar Arun Kamble
<sagar.a.kamble at intel.com> wrote:
> This patch adds support to enable/disable GuC interrupts for different
> features without impacting other's need. 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)
>
> v3: Removed spin lock and using test_bit in i915_guc_info. Prepared low
> level helpers to get/put GuC interrupts that can be reused during
> suspend/resume. (Tvrtko)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin 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_debugfs.c | 6 +++++
> drivers/gpu/drm/i915/intel_guc.c | 47
> +++++++++++++++++++++++++++---------
> drivers/gpu/drm/i915/intel_guc.h | 11 ++++++---
> drivers/gpu/drm/i915/intel_guc_log.c | 6 ++---
> drivers/gpu/drm/i915/intel_uc.c | 4 +--
> 5 files changed, 54 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 16f9a95..eef4c8b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2340,6 +2340,12 @@ static int i915_guc_info(struct seq_file *m, void
> *data)
> GEM_BUG_ON(!guc->execbuf_client);
> GEM_BUG_ON(!guc->preempt_client);
> + seq_puts(m, "GuC Interrupt Clients: ");
> + if (test_bit(GUC_INTR_CLIENT_LOG, &guc->interrupt_clients))
> + seq_puts(m, "GuC Logging\n");
> + else
> + seq_puts(m, "None\n");
> +
Maybe this can be done in intel_guc_log.c as part of:
void intel_guc_log_dump(const struct intel_guc_log *log, struct
drm_printer *p);
> seq_printf(m, "Doorbell map:\n");
> seq_printf(m, "\t%*pb\n", GUC_NUM_DOORBELLS, guc->doorbell_bitmap);
> seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", guc->db_cacheline);
> diff --git a/drivers/gpu/drm/i915/intel_guc.c
> b/drivers/gpu/drm/i915/intel_guc.c
> index 36d1bca..d356c40 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -407,7 +407,7 @@ int intel_guc_suspend(struct drm_i915_private
> *dev_priv)
> return 0;
> if (guc->log.level >= 0)
> - intel_disable_guc_interrupts(guc);
> + intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
> data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
> /* any value greater than GUC_POWER_D0 */
> @@ -453,7 +453,7 @@ int intel_guc_resume(struct drm_i915_private
> *dev_priv)
> return 0;
> if (guc->log.level >= 0)
> - intel_enable_guc_interrupts(guc);
> + intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG);
> data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
> data[1] = GUC_POWER_D0;
> @@ -524,28 +524,51 @@ 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)
> +static void __intel_get_guc_interrupts(struct intel_guc *guc)
> +{
> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> + lockdep_assert_held(&dev_priv->irq_lock);
> +
> + WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> + dev_priv->pm_guc_events);
> + gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> +}
> +
> +void intel_get_guc_interrupts(struct intel_guc *guc, enum
> guc_intr_client id)
What about intel_guc_interrupts_get(guc, id) ?
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> 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);
> - }
> +
> + if (!guc->interrupt_clients)
> + __intel_get_guc_interrupts(guc);
> + __set_bit(id, &guc->interrupt_clients);
Do we care about scenarios when we call "get" more than once?
Maybe we need GEM_WARN_ON at the minimum ?
Then I'm wondering if "get/put" are correct if we don't do any
refcounting... maybe "enable/disable" are more appropriate then?
> +
> spin_unlock_irq(&dev_priv->irq_lock);
> }
> -void intel_disable_guc_interrupts(struct intel_guc *guc)
> +static void __intel_put_guc_interrupts(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> - spin_lock_irq(&dev_priv->irq_lock);
> - dev_priv->guc.interrupts_enabled = false;
> + lockdep_assert_held(&dev_priv->irq_lock);
> gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> +}
> +
> +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);
> +
> + __clear_bit(id, &guc->interrupt_clients);
> + if (guc->interrupt_clients) {
> + spin_unlock_irq(&dev_priv->irq_lock);
> + return;
> + }
> + __intel_put_guc_interrupts(guc);
> spin_unlock_irq(&dev_priv->irq_lock);
> synchronize_irq(dev_priv->drm.irq);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h
> b/drivers/gpu/drm/i915/intel_guc.h
> index 49f33b9..af74392 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -34,6 +34,11 @@
> #include "intel_uc_fw.h"
> #include "i915_vma.h"
> +enum guc_intr_client {
Hmm, what about adding intel_ prefix ?
> + GUC_INTR_CLIENT_LOG = 0,
> + GUC_INTR_CLIENT_COUNT
> +};
> +
> struct guc_preempt_work {
> struct work_struct work;
> struct intel_engine_cs *engine;
> @@ -53,7 +58,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;
> @@ -132,8 +137,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 7bc0065..ddd4f6d 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -488,7 +488,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.
> @@ -594,7 +594,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 */
I guess above comment is now obsolete ;)
> - 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
> @@ -626,7 +626,7 @@ void i915_guc_log_unregister(struct drm_i915_private
> *dev_priv)
> /* GuC logging is currently the only user of Guc2Host interrupts */
Same here.
> if (dev_priv->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 fb5edcc..93ecbf1 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -319,7 +319,7 @@ int intel_uc_init_hw(struct drm_i915_private
> *dev_priv)
> goto err_log_capture;
> if (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)
> @@ -353,7 +353,7 @@ int intel_uc_init_hw(struct drm_i915_private
> *dev_priv)
> guc_disable_communication(guc);
> err_interrupts:
> if (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_out:
More information about the Intel-gfx
mailing list