Attachment 108692 Details for Bug 82939 – fix rps interrupt race wrt. rps work when disabling rps

[patch] fix rps interrupt race wrt. rps work when disabling rps

0003-drm-i915-fix-rps-interrupt-race-wrt.-rps-work.patch (text/plain), 8.96 KB, created by Imre Deak on 2014-10-30 15:02:58 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Imre Deak

Created: 2014-10-30 15:02:58 UTC

Size: 8.96 KB

patch

obsolete

>From 246b7fa83c998eedbbae5a0f7105448d34d331dd Mon Sep 17 00:00:00 2001
>From: Imre Deak <imre.deak@intel.com>
>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 <imre.deak@intel.com>
>---
> 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
>

Actions: View | Splinter Review

Attachments on bug 82939: 105087 | 108690 | 108691 | 108692 | 108933

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.