[PATCH] drm/i915: Fix runtime pm inbalance due to reg I/O forcewake dance

[PATCH] drm/i915: Fix runtime pm inbalance due to reg I/O forcewake dance

[Daniel Vetter]

This regression has been introduced in

commit 8232644ccf099548710843e97360a3fcd6d28e04
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Mar 5 12:00:39 2014 +0000

    drm/i915: Convert the forcewake worker into a timer func

which started to use the delayed forcewake put also for the register
I/O forcewake dance. But this conflicts functionally with the
(reviewed in parallel)

commit 6d88064edcfc5e5893371f7c06b9f3078dc1edf6
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Fri Feb 21 17:58:29 2014 -0300

    drm/i915: put runtime PM only when we actually release force_wake

which moved the runtime pm put calls into the delayed forcewake put
function to avoid an inversion between these. The problem is that the
register I/O function do _not_ grab a runtime pm reference, hence
dropping it is a bug.

So split the timer into two to re-balance the runtime pm refcounting.
The tricky bit to ensure is that the _raw timer doesn't run after
we've runtime-suspended the device. After all it only has an implicit
runtime pm reference provided by its caller. But the del_timer_sync in
the runtime suspend code will ensure this.

Add a comment to document this all.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76151
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h     |  9 +++++++++
 drivers/gpu/drm/i915/intel_uncore.c | 14 ++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 70fbe904016f..c2789702b9a5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -511,7 +511,16 @@ struct intel_uncore {
 	unsigned fw_rendercount;
 	unsigned fw_mediacount;
 
+	/*
+	 * Delayed forcewake put timers. The _raw one is for register I/O
+	 * functions which don't grab a runtime pm reference, instead presuming
+	 * that someone else holds that already.
+	 *
+	 * Synchronization with runtime pm actually suspended the device happens
+	 * in the runtime suspend path in intel_uncore_forcewake_reset.
+	 */
 	struct timer_list force_wake_timer;
+	struct timer_list force_wake_timer_raw;
 };
 
 #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e6bb421a3dbd..8010e2caf821 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -293,7 +293,7 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv,
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
-static void gen6_force_wake_timer(unsigned long arg)
+static void gen6_force_wake_timer_raw(unsigned long arg)
 {
 	struct drm_i915_private *dev_priv = (void *)arg;
 	unsigned long irqflags;
@@ -304,6 +304,13 @@ static void gen6_force_wake_timer(unsigned long arg)
 	if (--dev_priv->uncore.forcewake_count == 0)
 		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+}
+
+static void gen6_force_wake_timer(unsigned long arg)
+{
+	struct drm_i915_private *dev_priv = (void *)arg;
+
+	gen6_force_wake_timer_raw(arg);
 
 	intel_runtime_pm_put(dev_priv);
 }
@@ -314,6 +321,7 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 	unsigned long irqflags;
 
 	del_timer_sync(&dev_priv->uncore.force_wake_timer);
+	del_timer_sync(&dev_priv->uncore.force_wake_timer_raw);
 
 	/* Hold uncore.lock across reset to prevent any register access
 	 * with forcewake not set correctly
@@ -542,7 +550,7 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 		dev_priv->uncore.funcs.force_wake_get(dev_priv, \
 						      FORCEWAKE_ALL); \
 		dev_priv->uncore.forcewake_count++; \
-		mod_timer_pinned(&dev_priv->uncore.force_wake_timer, \
+		mod_timer_pinned(&dev_priv->uncore.force_wake_timer_raw, \
 				 jiffies + 1); \
 	} \
 	val = __raw_i915_read##x(dev_priv, reg); \
@@ -726,6 +734,8 @@ void intel_uncore_init(struct drm_device *dev)
 
 	setup_timer(&dev_priv->uncore.force_wake_timer,
 		    gen6_force_wake_timer, (unsigned long)dev_priv);
+	setup_timer(&dev_priv->uncore.force_wake_timer_raw,
+		    gen6_force_wake_timer_raw, (unsigned long)dev_priv);
 
 	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
-- 
1.8.5.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix runtime pm inbalance due to reg I/O forcewake dance

[Ville Syrjälä]

On Fri, Mar 14, 2014 at 08:48:46AM +0100, Daniel Vetter wrote:
> This regression has been introduced in
> 
> commit 8232644ccf099548710843e97360a3fcd6d28e04
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Wed Mar 5 12:00:39 2014 +0000
> 
>     drm/i915: Convert the forcewake worker into a timer func
> 
> which started to use the delayed forcewake put also for the register
> I/O forcewake dance. But this conflicts functionally with the
> (reviewed in parallel)
> 
> commit 6d88064edcfc5e5893371f7c06b9f3078dc1edf6
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Fri Feb 21 17:58:29 2014 -0300
> 
>     drm/i915: put runtime PM only when we actually release force_wake
> 
> which moved the runtime pm put calls into the delayed forcewake put
> function to avoid an inversion between these. The problem is that the
> register I/O function do _not_ grab a runtime pm reference, hence
> dropping it is a bug.
> 
> So split the timer into two to re-balance the runtime pm refcounting.
> The tricky bit to ensure is that the _raw timer doesn't run after
> we've runtime-suspended the device. After all it only has an implicit
> runtime pm reference provided by its caller. But the del_timer_sync in
> the runtime suspend code will ensure this.
> 
> Add a comment to document this all.

Yuck. Why don't we just stick the runtime pm put into
gen6_gt_force_wake_put() and let Chris's timer cancellation +
forcewake reset fixes take care of things?

> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76151
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  9 +++++++++
>  drivers/gpu/drm/i915/intel_uncore.c | 14 ++++++++++++--
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 70fbe904016f..c2789702b9a5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -511,7 +511,16 @@ struct intel_uncore {
>  	unsigned fw_rendercount;
>  	unsigned fw_mediacount;
>  
> +	/*
> +	 * Delayed forcewake put timers. The _raw one is for register I/O
> +	 * functions which don't grab a runtime pm reference, instead presuming
> +	 * that someone else holds that already.
> +	 *
> +	 * Synchronization with runtime pm actually suspended the device happens
> +	 * in the runtime suspend path in intel_uncore_forcewake_reset.
> +	 */
>  	struct timer_list force_wake_timer;
> +	struct timer_list force_wake_timer_raw;
>  };
>  
>  #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index e6bb421a3dbd..8010e2caf821 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -293,7 +293,7 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv,
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
> -static void gen6_force_wake_timer(unsigned long arg)
> +static void gen6_force_wake_timer_raw(unsigned long arg)
>  {
>  	struct drm_i915_private *dev_priv = (void *)arg;
>  	unsigned long irqflags;
> @@ -304,6 +304,13 @@ static void gen6_force_wake_timer(unsigned long arg)
>  	if (--dev_priv->uncore.forcewake_count == 0)
>  		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +}
> +
> +static void gen6_force_wake_timer(unsigned long arg)
> +{
> +	struct drm_i915_private *dev_priv = (void *)arg;
> +
> +	gen6_force_wake_timer_raw(arg);
>  
>  	intel_runtime_pm_put(dev_priv);
>  }
> @@ -314,6 +321,7 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>  	unsigned long irqflags;
>  
>  	del_timer_sync(&dev_priv->uncore.force_wake_timer);
> +	del_timer_sync(&dev_priv->uncore.force_wake_timer_raw);
>  
>  	/* Hold uncore.lock across reset to prevent any register access
>  	 * with forcewake not set correctly
> @@ -542,7 +550,7 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  		dev_priv->uncore.funcs.force_wake_get(dev_priv, \
>  						      FORCEWAKE_ALL); \
>  		dev_priv->uncore.forcewake_count++; \
> -		mod_timer_pinned(&dev_priv->uncore.force_wake_timer, \
> +		mod_timer_pinned(&dev_priv->uncore.force_wake_timer_raw, \
>  				 jiffies + 1); \
>  	} \
>  	val = __raw_i915_read##x(dev_priv, reg); \
> @@ -726,6 +734,8 @@ void intel_uncore_init(struct drm_device *dev)
>  
>  	setup_timer(&dev_priv->uncore.force_wake_timer,
>  		    gen6_force_wake_timer, (unsigned long)dev_priv);
> +	setup_timer(&dev_priv->uncore.force_wake_timer_raw,
> +		    gen6_force_wake_timer_raw, (unsigned long)dev_priv);
>  
>  	if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
> -- 
> 1.8.5.2

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Fix runtime pm inbalance due to reg I/O forcewake dance

[Chris Wilson]

On Fri, Mar 14, 2014 at 10:18:47AM +0200, Ville Syrjälä wrote:
> On Fri, Mar 14, 2014 at 08:48:46AM +0100, Daniel Vetter wrote:
> > This regression has been introduced in
> > 
> > commit 8232644ccf099548710843e97360a3fcd6d28e04
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Wed Mar 5 12:00:39 2014 +0000
> > 
> >     drm/i915: Convert the forcewake worker into a timer func
> > 
> > which started to use the delayed forcewake put also for the register
> > I/O forcewake dance. But this conflicts functionally with the
> > (reviewed in parallel)
> > 
> > commit 6d88064edcfc5e5893371f7c06b9f3078dc1edf6
> > Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Date:   Fri Feb 21 17:58:29 2014 -0300
> > 
> >     drm/i915: put runtime PM only when we actually release force_wake
> > 
> > which moved the runtime pm put calls into the delayed forcewake put
> > function to avoid an inversion between these. The problem is that the
> > register I/O function do _not_ grab a runtime pm reference, hence
> > dropping it is a bug.
> > 
> > So split the timer into two to re-balance the runtime pm refcounting.
> > The tricky bit to ensure is that the _raw timer doesn't run after
> > we've runtime-suspended the device. After all it only has an implicit
> > runtime pm reference provided by its caller. But the del_timer_sync in
> > the runtime suspend code will ensure this.
> > 
> > Add a comment to document this all.
> 
> Yuck. Why don't we just stick the runtime pm put into
> gen6_gt_force_wake_put() and let Chris's timer cancellation +
> forcewake reset fixes take care of things?

Heh, I went the other way round, dropped the runtime pm put from the
timer and flushed the forcewaker timer when we suspended the device...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Fix runtime pm inbalance due to reg I/O forcewake dance

[Ville Syrjälä]

On Fri, Mar 14, 2014 at 08:24:00AM +0000, Chris Wilson wrote:
> On Fri, Mar 14, 2014 at 10:18:47AM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 14, 2014 at 08:48:46AM +0100, Daniel Vetter wrote:
> > > This regression has been introduced in
> > > 
> > > commit 8232644ccf099548710843e97360a3fcd6d28e04
> > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > Date:   Wed Mar 5 12:00:39 2014 +0000
> > > 
> > >     drm/i915: Convert the forcewake worker into a timer func
> > > 
> > > which started to use the delayed forcewake put also for the register
> > > I/O forcewake dance. But this conflicts functionally with the
> > > (reviewed in parallel)
> > > 
> > > commit 6d88064edcfc5e5893371f7c06b9f3078dc1edf6
> > > Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Date:   Fri Feb 21 17:58:29 2014 -0300
> > > 
> > >     drm/i915: put runtime PM only when we actually release force_wake
> > > 
> > > which moved the runtime pm put calls into the delayed forcewake put
> > > function to avoid an inversion between these. The problem is that the
> > > register I/O function do _not_ grab a runtime pm reference, hence
> > > dropping it is a bug.
> > > 
> > > So split the timer into two to re-balance the runtime pm refcounting.
> > > The tricky bit to ensure is that the _raw timer doesn't run after
> > > we've runtime-suspended the device. After all it only has an implicit
> > > runtime pm reference provided by its caller. But the del_timer_sync in
> > > the runtime suspend code will ensure this.
> > > 
> > > Add a comment to document this all.
> > 
> > Yuck. Why don't we just stick the runtime pm put into
> > gen6_gt_force_wake_put() and let Chris's timer cancellation +
> > forcewake reset fixes take care of things?
> 
> Heh, I went the other way round, dropped the runtime pm put from the
> timer and flushed the forcewaker timer when we suspended the device...

That's what I meant. No delayed runtime_pm_put.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Fix runtime pm inbalance due to reg I/O forcewake dance

[Daniel Vetter]

On Fri, Mar 14, 2014 at 9:34 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> Heh, I went the other way round, dropped the runtime pm put from the
>> timer and flushed the forcewaker timer when we suspended the device...
>
> That's what I meant. No delayed runtime_pm_put.

Well I've figured we want to keep this ... have things changed
sufficiently meanwhile? I've lost a bit track in all that recent
shuffling ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Fix runtime pm inbalance due to reg I/O forcewake dance

[Jesse Barnes]

On Fri, 14 Mar 2014 09:52:47 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> On Fri, Mar 14, 2014 at 9:34 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >> Heh, I went the other way round, dropped the runtime pm put from the
> >> timer and flushed the forcewaker timer when we suspended the device...
> >
> > That's what I meant. No delayed runtime_pm_put.
> 
> Well I've figured we want to keep this ... have things changed
> sufficiently meanwhile? I've lost a bit track in all that recent
> shuffling ...

Can we pull the forcewake bits out of the reg functions and just do a
check and WARN instead?

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Rebalance runtime pm vs forcewake

[Chris Wilson]

---
 drivers/gpu/drm/i915/i915_drv.c     | 2 +-
 drivers/gpu/drm/i915/intel_uncore.c | 9 ++-------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5a0d34c47885..3fbf8aa8d119 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	WARN_ON(!HAS_RUNTIME_PM(dev));
-	assert_force_wake_inactive(dev_priv);
 
 	DRM_DEBUG_KMS("Suspending device\n");
 
 	i915_gem_release_all_mmaps(dev_priv);
+	intel_uncore_fini(dev);
 
 	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
 	dev_priv->pm.suspended = true;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e6bb421a3dbd..527527382361 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -304,8 +304,6 @@ static void gen6_force_wake_timer(unsigned long arg)
 	if (--dev_priv->uncore.forcewake_count == 0)
 		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
-
-	intel_runtime_pm_put(dev_priv);
 }
 
 static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
@@ -439,7 +437,6 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
 void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 {
 	unsigned long irqflags;
-	bool delayed = false;
 
 	if (!dev_priv->uncore.funcs.force_wake_put)
 		return;
@@ -450,19 +447,17 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 		goto out;
 	}
 
-
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 	if (--dev_priv->uncore.forcewake_count == 0) {
 		dev_priv->uncore.forcewake_count++;
-		delayed = true;
 		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
 				 jiffies + 1);
 	}
+
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 
 out:
-	if (!delayed)
-		intel_runtime_pm_put(dev_priv);
+	intel_runtime_pm_put(dev_priv);
 }
 
 void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
-- 
1.9.0

Re: [PATCH] drm/i915: Rebalance runtime pm vs forcewake

[Daniel Vetter]

On Fri, Mar 14, 2014 at 08:37:15AM +0000, Chris Wilson wrote:
> ---
>  drivers/gpu/drm/i915/i915_drv.c     | 2 +-
>  drivers/gpu/drm/i915/intel_uncore.c | 9 ++-------
>  2 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5a0d34c47885..3fbf8aa8d119 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	WARN_ON(!HAS_RUNTIME_PM(dev));
> -	assert_force_wake_inactive(dev_priv);

Why is this necessary? Also I've already pushed a pile of other patches on
top of all this, so I think a full commit is better. Also gives us an
excuse to document our flailing here a bit better in a neat commit message
... Imo we should also mention that the forcewake_put here isn't really
perf critical any more (if this is really the case).

Cheers, Daniel
>  
>  	DRM_DEBUG_KMS("Suspending device\n");
>  
>  	i915_gem_release_all_mmaps(dev_priv);
> +	intel_uncore_fini(dev);
>  
>  	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
>  	dev_priv->pm.suspended = true;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index e6bb421a3dbd..527527382361 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -304,8 +304,6 @@ static void gen6_force_wake_timer(unsigned long arg)
>  	if (--dev_priv->uncore.forcewake_count == 0)
>  		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> -
> -	intel_runtime_pm_put(dev_priv);
>  }
>  
>  static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> @@ -439,7 +437,6 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
>  void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  {
>  	unsigned long irqflags;
> -	bool delayed = false;
>  
>  	if (!dev_priv->uncore.funcs.force_wake_put)
>  		return;
> @@ -450,19 +447,17 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  		goto out;
>  	}
>  
> -
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  	if (--dev_priv->uncore.forcewake_count == 0) {
>  		dev_priv->uncore.forcewake_count++;
> -		delayed = true;
>  		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
>  				 jiffies + 1);
>  	}
> +
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  
>  out:
> -	if (!delayed)
> -		intel_runtime_pm_put(dev_priv);
> +	intel_runtime_pm_put(dev_priv);
>  }
>  
>  void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
> -- 
> 1.9.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Rebalance runtime pm vs forcewake

[Chris Wilson]

On Fri, Mar 14, 2014 at 04:51:16PM +0100, Daniel Vetter wrote:
> On Fri, Mar 14, 2014 at 08:37:15AM +0000, Chris Wilson wrote:
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c     | 2 +-
> >  drivers/gpu/drm/i915/intel_uncore.c | 9 ++-------
> >  2 files changed, 3 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 5a0d34c47885..3fbf8aa8d119 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> >  	WARN_ON(!HAS_RUNTIME_PM(dev));
> > -	assert_force_wake_inactive(dev_priv);
> 
> Why is this necessary? Also I've already pushed a pile of other patches on
> top of all this, so I think a full commit is better. Also gives us an
> excuse to document our flailing here a bit better in a neat commit message
> ... Imo we should also mention that the forcewake_put here isn't really
> perf critical any more (if this is really the case).

I was continuing the conversation with example code... This is, I think,
the simplest method for removing the pm_put from the forcewake timer,
and just wanted to make sure that we were in agreement before writing a
paragraph to explain the problem.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Rebalance runtime pm vs forcewake

[Daniel Vetter]

On Fri, Mar 14, 2014 at 5:13 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Mar 14, 2014 at 04:51:16PM +0100, Daniel Vetter wrote:
>> On Fri, Mar 14, 2014 at 08:37:15AM +0000, Chris Wilson wrote:
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.c     | 2 +-
>> >  drivers/gpu/drm/i915/intel_uncore.c | 9 ++-------
>> >  2 files changed, 3 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> > index 5a0d34c47885..3fbf8aa8d119 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.c
>> > +++ b/drivers/gpu/drm/i915/i915_drv.c
>> > @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device)
>> >     struct drm_i915_private *dev_priv = dev->dev_private;
>> >
>> >     WARN_ON(!HAS_RUNTIME_PM(dev));
>> > -   assert_force_wake_inactive(dev_priv);
>>
>> Why is this necessary? Also I've already pushed a pile of other patches on
>> top of all this, so I think a full commit is better. Also gives us an
>> excuse to document our flailing here a bit better in a neat commit message
>> ... Imo we should also mention that the forcewake_put here isn't really
>> perf critical any more (if this is really the case).
>
> I was continuing the conversation with example code... This is, I think,
> the simplest method for removing the pm_put from the forcewake timer,
> and just wanted to make sure that we were in agreement before writing a
> paragraph to explain the problem.

Ah, with closer reading of your patch I've noticed that the
uncore_fini is after the above assert, so this indeed has to go.

I'm also ok with the overall patch if that doesn't cause another round
back to reinstate the delayed forcewake put here ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Rebalance runtime pm vs forcewake

[Paulo Zanoni]

2014-03-14 15:43 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Mar 14, 2014 at 5:13 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> On Fri, Mar 14, 2014 at 04:51:16PM +0100, Daniel Vetter wrote:
>>> On Fri, Mar 14, 2014 at 08:37:15AM +0000, Chris Wilson wrote:
>>> > ---
>>> >  drivers/gpu/drm/i915/i915_drv.c     | 2 +-
>>> >  drivers/gpu/drm/i915/intel_uncore.c | 9 ++-------
>>> >  2 files changed, 3 insertions(+), 8 deletions(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>> > index 5a0d34c47885..3fbf8aa8d119 100644
>>> > --- a/drivers/gpu/drm/i915/i915_drv.c
>>> > +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> > @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device)
>>> >     struct drm_i915_private *dev_priv = dev->dev_private;
>>> >
>>> >     WARN_ON(!HAS_RUNTIME_PM(dev));
>>> > -   assert_force_wake_inactive(dev_priv);
>>>
>>> Why is this necessary? Also I've already pushed a pile of other patches on
>>> top of all this, so I think a full commit is better. Also gives us an
>>> excuse to document our flailing here a bit better in a neat commit message
>>> ... Imo we should also mention that the forcewake_put here isn't really
>>> perf critical any more (if this is really the case).
>>
>> I was continuing the conversation with example code... This is, I think,
>> the simplest method for removing the pm_put from the forcewake timer,
>> and just wanted to make sure that we were in agreement before writing a
>> paragraph to explain the problem.
>
> Ah, with closer reading of your patch I've noticed that the
> uncore_fini is after the above assert, so this indeed has to go.
>
> I'm also ok with the overall patch if that doesn't cause another round
> back to reinstate the delayed forcewake put here ;-)

Hi

Last week Chris sent me a rebased version of this patch on pastebin. I
tested it, and when I run the "rte" subtest from pm_pc8, I get many
instances of the "WARN_ON(dev->irq_enabled)" that happens inside
intel_disable_gt_powersave().

I also tried to apply "drm/i915: Fix runtime PM inbalance due to reg
I/O forcewake dance", but the patch does not apply cleanly.

That leaves us with the original "drm/i915: don't schedule
force_wake_timer at gen6_read".

I'd really like to get this bug fixed ASAP as it completely prevents
runtime PM from working at all, and we already have fixes for it since
weeks ago. Daniel? Chris?

Thanks,
Paulo

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Rebalance runtime pm vs forcewake

[Daniel Vetter]

On Mon, Mar 31, 2014 at 03:22:36PM -0300, Paulo Zanoni wrote:
> 2014-03-14 15:43 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Fri, Mar 14, 2014 at 5:13 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> On Fri, Mar 14, 2014 at 04:51:16PM +0100, Daniel Vetter wrote:
> >>> On Fri, Mar 14, 2014 at 08:37:15AM +0000, Chris Wilson wrote:
> >>> > ---
> >>> >  drivers/gpu/drm/i915/i915_drv.c     | 2 +-
> >>> >  drivers/gpu/drm/i915/intel_uncore.c | 9 ++-------
> >>> >  2 files changed, 3 insertions(+), 8 deletions(-)
> >>> >
> >>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >>> > index 5a0d34c47885..3fbf8aa8d119 100644
> >>> > --- a/drivers/gpu/drm/i915/i915_drv.c
> >>> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> >>> > @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device)
> >>> >     struct drm_i915_private *dev_priv = dev->dev_private;
> >>> >
> >>> >     WARN_ON(!HAS_RUNTIME_PM(dev));
> >>> > -   assert_force_wake_inactive(dev_priv);
> >>>
> >>> Why is this necessary? Also I've already pushed a pile of other patches on
> >>> top of all this, so I think a full commit is better. Also gives us an
> >>> excuse to document our flailing here a bit better in a neat commit message
> >>> ... Imo we should also mention that the forcewake_put here isn't really
> >>> perf critical any more (if this is really the case).
> >>
> >> I was continuing the conversation with example code... This is, I think,
> >> the simplest method for removing the pm_put from the forcewake timer,
> >> and just wanted to make sure that we were in agreement before writing a
> >> paragraph to explain the problem.
> >
> > Ah, with closer reading of your patch I've noticed that the
> > uncore_fini is after the above assert, so this indeed has to go.
> >
> > I'm also ok with the overall patch if that doesn't cause another round
> > back to reinstate the delayed forcewake put here ;-)
> 
> Hi
> 
> Last week Chris sent me a rebased version of this patch on pastebin. I
> tested it, and when I run the "rte" subtest from pm_pc8, I get many
> instances of the "WARN_ON(dev->irq_enabled)" that happens inside
> intel_disable_gt_powersave().
> 
> I also tried to apply "drm/i915: Fix runtime PM inbalance due to reg
> I/O forcewake dance", but the patch does not apply cleanly.
> 
> That leaves us with the original "drm/i915: don't schedule
> force_wake_timer at gen6_read".
> 
> I'd really like to get this bug fixed ASAP as it completely prevents
> runtime PM from working at all, and we already have fixes for it since
> weeks ago. Daniel? Chris?

Agreed that the your patch which essentially reverts stuff is the best
course for getting 3.15 into shape. We can frob things however we want to
for 3.16.

On that topic, qa has finally found the drv_suspend/forcewake issue. Chris
can you please pick out the minimal fix for that out of your tree? Maybe
on top of Paulo's fixes so that I don't have to wreak the patch applying
;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Rebalance runtime pm vs forcewake

[Chris Wilson]

On Mon, Mar 31, 2014 at 08:59:29PM +0200, Daniel Vetter wrote:
> On that topic, qa has finally found the drv_suspend/forcewake issue. Chris
> can you please pick out the minimal fix for that out of your tree? Maybe
> on top of Paulo's fixes so that I don't have to wreak the patch applying
> ;-)

To be clear, are these the fixes in the tree already, or should I pull
in another branch first? Paulo?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Rebalance runtime pm vs forcewake

[Paulo Zanoni]

2014-04-01 5:14 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Mon, Mar 31, 2014 at 08:59:29PM +0200, Daniel Vetter wrote:
>> On that topic, qa has finally found the drv_suspend/forcewake issue. Chris
>> can you please pick out the minimal fix for that out of your tree? Maybe
>> on top of Paulo's fixes so that I don't have to wreak the patch applying
>> ;-)
>
> To be clear, are these the fixes in the tree already, or should I pull
> in another branch first? Paulo?

I think he wanted you to base your patches on top of just this
specific commit:
http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=bdw-runtime-pm-2014-03-28&id=a4f3ebda1df2a0f24a7d480f67b153df8fc0c9f0
.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Rebalance runtime pm vs forcewake

[Chris Wilson]

On Tue, Apr 01, 2014 at 09:32:50AM -0300, Paulo Zanoni wrote:
> 2014-04-01 5:14 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Mon, Mar 31, 2014 at 08:59:29PM +0200, Daniel Vetter wrote:
> >> On that topic, qa has finally found the drv_suspend/forcewake issue. Chris
> >> can you please pick out the minimal fix for that out of your tree? Maybe
> >> on top of Paulo's fixes so that I don't have to wreak the patch applying
> >> ;-)
> >
> > To be clear, are these the fixes in the tree already, or should I pull
> > in another branch first? Paulo?
> 
> I think he wanted you to base your patches on top of just this
> specific commit:
> http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=bdw-runtime-pm-2014-03-28&id=a4f3ebda1df2a0f24a7d480f67b153df8fc0c9f0

But the whole point of the rebalancing fix was to avoid having to apply
that patch?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Rebalance runtime pm vs forcewake

[Daniel Vetter]

On Tue, Apr 1, 2014 at 2:42 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Apr 01, 2014 at 09:32:50AM -0300, Paulo Zanoni wrote:
>> 2014-04-01 5:14 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> > On Mon, Mar 31, 2014 at 08:59:29PM +0200, Daniel Vetter wrote:
>> >> On that topic, qa has finally found the drv_suspend/forcewake issue. Chris
>> >> can you please pick out the minimal fix for that out of your tree? Maybe
>> >> on top of Paulo's fixes so that I don't have to wreak the patch applying
>> >> ;-)
>> >
>> > To be clear, are these the fixes in the tree already, or should I pull
>> > in another branch first? Paulo?
>>
>> I think he wanted you to base your patches on top of just this
>> specific commit:
>> http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=bdw-runtime-pm-2014-03-28&id=a4f3ebda1df2a0f24a7d480f67b153df8fc0c9f0
>
> But the whole point of the rebalancing fix was to avoid having to apply
> that patch?

Management put down the hammer and told me that "runtime pm for bdw
must go in _now_". So it's going to be this one apparently.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch