| Message ID | 1515082914-4111-11-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 04 Jan 2018 17:21:52 +0100, Sagar Arun Kamble <sagar.a.kamble@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@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 | 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:
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@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@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 | 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:
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"); + 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) { 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); + 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 { + 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 */ - 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 */ 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:
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@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 | 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(-)