From 246b7fa83c998eedbbae5a0f7105448d34d331dd Mon Sep 17 00:00:00 2001 From: Imre Deak Date: Thu, 30 Oct 2014 16:19:20 +0200 Subject: [PATCH 3/3] drm/i915: fix rps interrupt race wrt. rps work Signed-off-by: Imre Deak --- drivers/gpu/drm/i915/i915_drv.c | 7 +-- drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/i915_irq.c | 6 +++ drivers/gpu/drm/i915/intel_display.c | 4 +- drivers/gpu/drm/i915/intel_pm.c | 84 +++++++++++++++++++----------------- 5 files changed, 55 insertions(+), 49 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 035ec94..64513ea 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1409,12 +1409,7 @@ static int intel_runtime_suspend(struct device *device) i915_gem_release_all_mmaps(dev_priv); mutex_unlock(&dev->struct_mutex); - /* - * rps.work can't be rearmed here, since we get here only after making - * sure the GPU is idle and the RPS freq is set to the minimum. See - * intel_mark_idle(). - */ - cancel_work_sync(&dev_priv->rps.work); + intel_suspend_gt_powersave(dev); intel_runtime_pm_disable_interrupts(dev_priv); ret = intel_suspend_complete(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 6a73803..7e53bce 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -970,8 +970,9 @@ struct intel_rps_ei { }; struct intel_gen6_power_mgmt { - /* work and pm_iir are protected by dev_priv->irq_lock */ + /* work, interrupts_enabled and pm_iir are protected by dev_priv->irq_lock */ struct work_struct work; + bool interrupts_enabled; u32 pm_iir; /* Frequencies are stored in potentially platform dependent multiples. diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index a2b013d..e4b5d7c 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1119,6 +1119,11 @@ static void gen6_pm_rps_work(struct work_struct *work) int new_delay, adj; spin_lock_irq(&dev_priv->irq_lock); + + if (!dev_priv->rps.interrupts_enabled) { + spin_unlock_irq(&dev_priv->irq_lock); + return; + } pm_iir = dev_priv->rps.pm_iir; dev_priv->rps.pm_iir = 0; if (INTEL_INFO(dev_priv->dev)->gen >= 8) @@ -1127,6 +1132,7 @@ static void gen6_pm_rps_work(struct work_struct *work) /* Make sure not to corrupt PMIMR state used by ringbuffer */ gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events); } + spin_unlock_irq(&dev_priv->irq_lock); /* Make sure we didn't queue anything we're not going to process. */ diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 963c902..3ab9b8f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13222,6 +13222,8 @@ void intel_modeset_cleanup(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; struct drm_connector *connector; + intel_disable_gt_powersave(dev); + /* * Interrupts and polling as the first thing to avoid creating havoc. * Too much stuff here (turning of rps, connectors, ...) would @@ -13241,8 +13243,6 @@ void intel_modeset_cleanup(struct drm_device *dev) intel_disable_fbc(dev); - intel_disable_gt_powersave(dev); - ironlake_teardown_rc6(dev); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 7a69eba..137daa8 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3562,17 +3562,9 @@ static void gen8_disable_rps_interrupts(struct drm_device *dev) I915_WRITE(GEN6_PMINTRMSK, ~GEN8_PMINTR_REDIRECT_TO_NON_DISP); I915_WRITE(GEN8_GT_IER(2), I915_READ(GEN8_GT_IER(2)) & ~dev_priv->pm_rps_events); - /* Complete PM interrupt masking here doesn't race with the rps work - * item again unmasking PM interrupts because that is using a different - * register (GEN8_GT_IMR(2)) to mask PM interrupts. The only risk is in - * leaving stale bits in GEN8_GT_IIR(2) and GEN8_GT_IMR(2) which - * gen8_enable_rps will clean up. */ - - spin_lock_irq(&dev_priv->irq_lock); - dev_priv->rps.pm_iir = 0; - spin_unlock_irq(&dev_priv->irq_lock); - I915_WRITE(GEN8_GT_IIR(2), dev_priv->pm_rps_events); + + POSTING_READ(GEN8_GT_IIR(2)); } static void gen6_disable_rps_interrupts(struct drm_device *dev) @@ -3582,29 +3574,39 @@ static void gen6_disable_rps_interrupts(struct drm_device *dev) I915_WRITE(GEN6_PMINTRMSK, 0xffffffff); I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) & ~dev_priv->pm_rps_events); - /* Complete PM interrupt masking here doesn't race with the rps work - * item again unmasking PM interrupts because that is using a different - * register (PMIMR) to mask PM interrupts. The only risk is in leaving - * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */ - - spin_lock_irq(&dev_priv->irq_lock); - dev_priv->rps.pm_iir = 0; - spin_unlock_irq(&dev_priv->irq_lock); - I915_WRITE(GEN6_PMIIR, dev_priv->pm_rps_events); + + POSTING_READ(GEN6_PMIIR); } -static void gen6_disable_rps(struct drm_device *dev) +static void intel_disable_rps_interrupts(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - I915_WRITE(GEN6_RC_CONTROL, 0); - I915_WRITE(GEN6_RPNSWREQ, 1 << 31); + spin_lock_irq(&dev_priv->irq_lock); + dev_priv->rps.interrupts_enabled = false; + spin_unlock_irq(&dev_priv->irq_lock); - if (IS_BROADWELL(dev)) + cancel_work_sync(&dev_priv->rps.work); + + spin_lock_irq(&dev_priv->irq_lock); + + if (INTEL_INFO(dev)->gen == 8) gen8_disable_rps_interrupts(dev); - else + else if (INTEL_INFO(dev)->gen >= 6) gen6_disable_rps_interrupts(dev); + + dev_priv->rps.pm_iir = 0; + + spin_unlock_irq(&dev_priv->irq_lock); +} + +static void gen6_disable_rps(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + I915_WRITE(GEN6_RC_CONTROL, 0); + I915_WRITE(GEN6_RPNSWREQ, 1 << 31); } static void cherryview_disable_rps(struct drm_device *dev) @@ -3612,8 +3614,6 @@ static void cherryview_disable_rps(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; I915_WRITE(GEN6_RC_CONTROL, 0); - - gen8_disable_rps_interrupts(dev); } static void valleyview_disable_rps(struct drm_device *dev) @@ -3627,8 +3627,6 @@ static void valleyview_disable_rps(struct drm_device *dev) I915_WRITE(GEN6_RC_CONTROL, 0); gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); - - gen6_disable_rps_interrupts(dev); } static void intel_print_rc6_info(struct drm_device *dev, u32 mode) @@ -3714,6 +3712,20 @@ static void gen6_enable_rps_interrupts(struct drm_device *dev) spin_unlock_irq(&dev_priv->irq_lock); } +static void intel_enable_rps_interrupts(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + spin_lock_irq(&dev_priv->irq_lock); + dev_priv->rps.interrupts_enabled = true; + spin_unlock_irq(&dev_priv->irq_lock); + + if (INTEL_INFO(dev)->gen == 8) + gen8_enable_rps_interrupts(dev); + else if (INTEL_INFO(dev)->gen >= 6) + gen6_enable_rps_interrupts(dev); +} + static void parse_rp_state_cap(struct drm_i915_private *dev_priv, u32 rp_state_cap) { /* All of these values are in units of 50MHz */ @@ -3813,8 +3825,6 @@ static void gen8_enable_rps(struct drm_device *dev) gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) & 0xff00) >> 8); - gen8_enable_rps_interrupts(dev); - gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); } @@ -3911,8 +3921,6 @@ static void gen6_enable_rps(struct drm_device *dev) dev_priv->rps.power = HIGH_POWER; /* force a reset */ gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit); - gen6_enable_rps_interrupts(dev); - rc6vids = 0; ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids); if (IS_GEN6(dev) && ret) { @@ -4404,8 +4412,6 @@ static void cherryview_enable_rps(struct drm_device *dev) valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq); - gen8_enable_rps_interrupts(dev); - gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); } @@ -4484,8 +4490,6 @@ static void valleyview_enable_rps(struct drm_device *dev) valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq); - gen6_enable_rps_interrupts(dev); - gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); } @@ -5244,12 +5248,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - /* Interrupts should be disabled already to avoid re-arming. */ - WARN_ON(intel_irqs_enabled(dev_priv)); - flush_delayed_work(&dev_priv->rps.delayed_resume_work); - cancel_work_sync(&dev_priv->rps.work); + intel_disable_rps_interrupts(dev); /* Force GPU to min freq during suspend */ gen6_rps_idle(dev_priv); @@ -5301,6 +5302,9 @@ static void intel_gen6_powersave_work(struct work_struct *work) __gen6_update_ring_freq(dev); } dev_priv->rps.enabled = true; + + intel_enable_rps_interrupts(dev); + mutex_unlock(&dev_priv->rps.hw_lock); intel_runtime_pm_put(dev_priv); -- 1.8.4