| Message ID | 1508309222-26406-12-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/10/2017 07:47, Sagar Arun Kamble wrote: > In order to override the disable/enable control of GuC interrupts during > suspend/reset cycle we are creating two new functions suspend/restore > guc_interrupts which check if interrupts were enabled and disable them > on suspend and enable them on resume. They are used to restore interrupts > across reset as well. > > 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/intel_display.c | 2 ++ > drivers/gpu/drm/i915/intel_guc.c | 40 ++++++++++++++++++++++++++++++++---- > drivers/gpu/drm/i915/intel_guc.h | 2 ++ > 3 files changed, 40 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 897fe7e..742ab5e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3768,8 +3768,10 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) > * The display has been reset as well, > * so need a full re-initialization. > */ > + intel_suspend_guc_interrupts(&dev_priv->guc); > intel_runtime_pm_disable_interrupts(dev_priv); > intel_runtime_pm_enable_interrupts(dev_priv); > + intel_restore_guc_interrupts(&dev_priv->guc); > > intel_pps_unlock_regs_wa(dev_priv); > intel_modeset_init_hw(dev); > diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c > index fbd27ea..1e5abf2 100644 > --- a/drivers/gpu/drm/i915/intel_guc.c > +++ b/drivers/gpu/drm/i915/intel_guc.c > @@ -261,6 +261,25 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset) > return intel_guc_send(guc, action, ARRAY_SIZE(action)); > } > > +void intel_suspend_guc_interrupts(struct intel_guc *guc) > +{ > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > + > + spin_lock_irq(&dev_priv->irq_lock); > + > + if (!guc->interrupt_clients) { > + spin_unlock_irq(&dev_priv->irq_lock); > + return; > + } > + > + gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events); > + > + spin_unlock_irq(&dev_priv->irq_lock); > + synchronize_irq(dev_priv->drm.irq); > + > + intel_reset_guc_interrupts(guc); > +} Looks awfully similar to intel_put_guc_interrupts, minus the guc->interrupt_clients handling and single bit vs all. I am thinking if it could be consolidated somehow... Maybe add __intel_put_guc_interrupts which does not touch guc->interrupt_clients and takes in a bitmask of all interrupt clients? void __intel_put_guc_interrupts(..., unsigned long mask) { assert_lock_held... if ((mask & guc->interrupt_clients) != guc->interrupt_clients) return; ... do the irq disable stuff.. } void intel_suspend_guc_interrupts(...) { spin_lock... __intel_put_guc_interrupts(.., guc->interrupt_clients); spin_unlock... } void intel_put_guc_interrupts(..., id) { spin_lock... __intel_put_guc_interrupts(.., BIT(id)); __clear_bit(guc->interrupt_clients, id); spin_unlock... } Maybe some logic mistakes here, beware. :) > + > /** > * intel_guc_suspend() - notify GuC entering suspend state > * @dev_priv: i915 device private > @@ -274,8 +293,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) > if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) > return 0; > > - if (i915_modparams.guc_log_level >= 0) > - intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG); > + intel_suspend_guc_interrupts(guc); > > ctx = dev_priv->kernel_context; > > @@ -289,6 +307,21 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) > return intel_guc_send(guc, data, ARRAY_SIZE(data)); > } > > +void intel_restore_guc_interrupts(struct intel_guc *guc) > +{ > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > + > + spin_lock_irq(&dev_priv->irq_lock); > + > + if (guc->interrupt_clients) { > + 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); > + } > + > + spin_unlock_irq(&dev_priv->irq_lock); > +} And ofc something similar like the above to consolidate the get/restore as well. > + > /** > * intel_guc_resume() - notify GuC resuming from suspend state > * @dev_priv: i915 device private > @@ -302,8 +335,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv) > if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) > return 0; > > - if (i915_modparams.guc_log_level >= 0) > - intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG); > + intel_restore_guc_interrupts(guc); > > ctx = dev_priv->kernel_context; > > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > index 4d58bf7..c55dcba 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -125,5 +125,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma) > 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); > +void intel_suspend_guc_interrupts(struct intel_guc *guc); > +void intel_restore_guc_interrupts(struct intel_guc *guc); > > #endif > Regards, Tvrtko
On 10/19/2017 4:33 PM, Tvrtko Ursulin wrote: > > On 18/10/2017 07:47, Sagar Arun Kamble wrote: >> In order to override the disable/enable control of GuC interrupts during >> suspend/reset cycle we are creating two new functions suspend/restore >> guc_interrupts which check if interrupts were enabled and disable them >> on suspend and enable them on resume. They are used to restore >> interrupts >> across reset as well. >> >> 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/intel_display.c | 2 ++ >> drivers/gpu/drm/i915/intel_guc.c | 40 >> ++++++++++++++++++++++++++++++++---- >> drivers/gpu/drm/i915/intel_guc.h | 2 ++ >> 3 files changed, 40 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index 897fe7e..742ab5e 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -3768,8 +3768,10 @@ void intel_finish_reset(struct >> drm_i915_private *dev_priv) >> * The display has been reset as well, >> * so need a full re-initialization. >> */ >> + intel_suspend_guc_interrupts(&dev_priv->guc); >> intel_runtime_pm_disable_interrupts(dev_priv); >> intel_runtime_pm_enable_interrupts(dev_priv); >> + intel_restore_guc_interrupts(&dev_priv->guc); >> intel_pps_unlock_regs_wa(dev_priv); >> intel_modeset_init_hw(dev); >> diff --git a/drivers/gpu/drm/i915/intel_guc.c >> b/drivers/gpu/drm/i915/intel_guc.c >> index fbd27ea..1e5abf2 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.c >> +++ b/drivers/gpu/drm/i915/intel_guc.c >> @@ -261,6 +261,25 @@ int intel_guc_auth_huc(struct intel_guc *guc, >> u32 rsa_offset) >> return intel_guc_send(guc, action, ARRAY_SIZE(action)); >> } >> +void intel_suspend_guc_interrupts(struct intel_guc *guc) >> +{ >> + struct drm_i915_private *dev_priv = guc_to_i915(guc); >> + >> + spin_lock_irq(&dev_priv->irq_lock); >> + >> + if (!guc->interrupt_clients) { >> + spin_unlock_irq(&dev_priv->irq_lock); >> + return; >> + } >> + >> + gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events); >> + >> + spin_unlock_irq(&dev_priv->irq_lock); >> + synchronize_irq(dev_priv->drm.irq); >> + >> + intel_reset_guc_interrupts(guc); >> +} > > Looks awfully similar to intel_put_guc_interrupts, minus the > guc->interrupt_clients handling and single bit vs all. I am thinking > if it could be consolidated somehow... > > Maybe add __intel_put_guc_interrupts which does not touch > guc->interrupt_clients and takes in a bitmask of all interrupt clients? > > void __intel_put_guc_interrupts(..., unsigned long mask) > { > assert_lock_held... > > if ((mask & guc->interrupt_clients) != guc->interrupt_clients) > return; > > ... do the irq disable stuff.. > } > > void intel_suspend_guc_interrupts(...) > { > spin_lock... > > __intel_put_guc_interrupts(.., guc->interrupt_clients); > > spin_unlock... > } > > void intel_put_guc_interrupts(..., id) > { > spin_lock... > > __intel_put_guc_interrupts(.., BIT(id)); > __clear_bit(guc->interrupt_clients, id); > > spin_unlock... > } > > Maybe some logic mistakes here, beware. :) Sure. will update. > >> + >> /** >> * intel_guc_suspend() - notify GuC entering suspend state >> * @dev_priv: i915 device private >> @@ -274,8 +293,7 @@ int intel_guc_suspend(struct drm_i915_private >> *dev_priv) >> if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) >> return 0; >> - if (i915_modparams.guc_log_level >= 0) >> - intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG); >> + intel_suspend_guc_interrupts(guc); >> ctx = dev_priv->kernel_context; >> @@ -289,6 +307,21 @@ int intel_guc_suspend(struct drm_i915_private >> *dev_priv) >> return intel_guc_send(guc, data, ARRAY_SIZE(data)); >> } >> +void intel_restore_guc_interrupts(struct intel_guc *guc) >> +{ >> + struct drm_i915_private *dev_priv = guc_to_i915(guc); >> + >> + spin_lock_irq(&dev_priv->irq_lock); >> + >> + if (guc->interrupt_clients) { >> + 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); >> + } >> + >> + spin_unlock_irq(&dev_priv->irq_lock); >> +} > > And ofc something similar like the above to consolidate the > get/restore as well. Yes. > >> + >> /** >> * intel_guc_resume() - notify GuC resuming from suspend state >> * @dev_priv: i915 device private >> @@ -302,8 +335,7 @@ int intel_guc_resume(struct drm_i915_private >> *dev_priv) >> if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) >> return 0; >> - if (i915_modparams.guc_log_level >= 0) >> - intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG); >> + intel_restore_guc_interrupts(guc); >> ctx = dev_priv->kernel_context; >> diff --git a/drivers/gpu/drm/i915/intel_guc.h >> b/drivers/gpu/drm/i915/intel_guc.h >> index 4d58bf7..c55dcba 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.h >> +++ b/drivers/gpu/drm/i915/intel_guc.h >> @@ -125,5 +125,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma >> *vma) >> 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); >> +void intel_suspend_guc_interrupts(struct intel_guc *guc); >> +void intel_restore_guc_interrupts(struct intel_guc *guc); >> #endif >> > > Regards, > > Tvrtko Thanks again for review on the series.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 897fe7e..742ab5e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3768,8 +3768,10 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) * The display has been reset as well, * so need a full re-initialization. */ + intel_suspend_guc_interrupts(&dev_priv->guc); intel_runtime_pm_disable_interrupts(dev_priv); intel_runtime_pm_enable_interrupts(dev_priv); + intel_restore_guc_interrupts(&dev_priv->guc); intel_pps_unlock_regs_wa(dev_priv); intel_modeset_init_hw(dev); diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index fbd27ea..1e5abf2 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -261,6 +261,25 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset) return intel_guc_send(guc, action, ARRAY_SIZE(action)); } +void intel_suspend_guc_interrupts(struct intel_guc *guc) +{ + struct drm_i915_private *dev_priv = guc_to_i915(guc); + + spin_lock_irq(&dev_priv->irq_lock); + + if (!guc->interrupt_clients) { + spin_unlock_irq(&dev_priv->irq_lock); + return; + } + + gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events); + + spin_unlock_irq(&dev_priv->irq_lock); + synchronize_irq(dev_priv->drm.irq); + + intel_reset_guc_interrupts(guc); +} + /** * intel_guc_suspend() - notify GuC entering suspend state * @dev_priv: i915 device private @@ -274,8 +293,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) return 0; - if (i915_modparams.guc_log_level >= 0) - intel_put_guc_interrupts(guc, GUC_INTR_CLIENT_LOG); + intel_suspend_guc_interrupts(guc); ctx = dev_priv->kernel_context; @@ -289,6 +307,21 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) return intel_guc_send(guc, data, ARRAY_SIZE(data)); } +void intel_restore_guc_interrupts(struct intel_guc *guc) +{ + struct drm_i915_private *dev_priv = guc_to_i915(guc); + + spin_lock_irq(&dev_priv->irq_lock); + + if (guc->interrupt_clients) { + 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); + } + + spin_unlock_irq(&dev_priv->irq_lock); +} + /** * intel_guc_resume() - notify GuC resuming from suspend state * @dev_priv: i915 device private @@ -302,8 +335,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv) if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) return 0; - if (i915_modparams.guc_log_level >= 0) - intel_get_guc_interrupts(guc, GUC_INTR_CLIENT_LOG); + intel_restore_guc_interrupts(guc); ctx = dev_priv->kernel_context; diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index 4d58bf7..c55dcba 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -125,5 +125,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma) 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); +void intel_suspend_guc_interrupts(struct intel_guc *guc); +void intel_restore_guc_interrupts(struct intel_guc *guc); #endif
In order to override the disable/enable control of GuC interrupts during suspend/reset cycle we are creating two new functions suspend/restore guc_interrupts which check if interrupts were enabled and disable them on suspend and enable them on resume. They are used to restore interrupts across reset as well. 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/intel_display.c | 2 ++ drivers/gpu/drm/i915/intel_guc.c | 40 ++++++++++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_guc.h | 2 ++ 3 files changed, 40 insertions(+), 4 deletions(-)