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@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
---
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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
