[Intel-gfx] [PATCH 08/11] drm/i915/guc: Add client support to enable/disable GuC interrupts
Sagar Arun Kamble
sagar.a.kamble at intel.com
Sat Oct 21 16:38:00 UTC 2017
On 10/19/2017 3:49 PM, Tvrtko Ursulin wrote:
>
> 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 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 | 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.
Yes. will update
>
> Also, spinlock not required here apart from documentation purposes?
Yes. Given the frequency of enable/disable of interrupts this spin_lock
is not so important.
>
>> + 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.
Yes.
>
>> +
>> 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.
Yes.
>
>> +
>> + 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 at intel.com>
>
> Regards,
>
> Tvrtko
More information about the Intel-gfx
mailing list