[Intel-gfx] [PATCH v3 10/12] drm/i915/guc: Add client support to enable/disable GuC interrupts
Sagar Arun Kamble
sagar.a.kamble at intel.com
Fri Jan 5 05:07:50 UTC 2018
On 1/4/2018 11:09 PM, Michal Wajdeczko wrote:
> 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);
>
Idea is to know as part of GuC global information, which all features
have enabled GuC to Host Interrupts.
I can add it in the i915_guc_log_dump as to whether interrupt is enabled
but then I think it will be good to have
it here to know interrupt status together at once we have CTB as well.
>
>> 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?
>
enabling/disabling more than once should not be a problem. will update
the names
as suggested as we don't refcount.
>> +
>> 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 ?
>
Forgot about this. will update.
>> + 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.
>
Yes. will remove.
Thanks
Sagar
>> 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