[PATCH 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[PATCH 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[Imre Deak]

If the timer putting the last forcewake refcount was pending and we
canceled it, we'll leak the corresponding forcewake and RPM references.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c90222d..389e235 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -299,9 +299,8 @@ static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
-static void gen6_force_wake_timer(unsigned long arg)
+static void __gen6_force_wake_timer(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv = (void *)arg;
 	unsigned long irqflags;
 
 	assert_device_not_suspended(dev_priv);
@@ -316,12 +315,20 @@ static void gen6_force_wake_timer(unsigned long arg)
 	intel_runtime_pm_put(dev_priv);
 }
 
+static void gen6_force_wake_timer(unsigned long arg)
+{
+	struct drm_i915_private *dev_priv = (void *)arg;
+
+	__gen6_force_wake_timer(dev_priv);
+}
+
 static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
 
-	del_timer_sync(&dev_priv->uncore.force_wake_timer);
+	if (del_timer_sync(&dev_priv->uncore.force_wake_timer))
+		__gen6_force_wake_timer(dev_priv);
 
 	/* Hold uncore.lock across reset to prevent any register access
 	 * with forcewake not set correctly
-- 
1.8.4

[PATCH 2/2] drm/i915: preserve user forcewake over system suspend/resume

[Imre Deak]

Atm, the forcewake refcount will be incorrectly set to zero during
system suspend if there is any reference held via the
i915_forcewake_user debugfs entry.

Fix this by simply not zeroing the sw counters during suspend and
restoring the original state using them. Note that the only other
places where we zeroed the counters were driver load and unload time,
where it was redundant anyway.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78059
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     |  2 +-
 drivers/gpu/drm/i915/i915_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_uncore.c | 10 +++-------
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 615b62f..5a08c86 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -600,7 +600,7 @@ static int i915_drm_thaw_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	intel_uncore_early_sanitize(dev);
+	intel_uncore_early_sanitize(dev, true);
 	intel_uncore_sanitize(dev);
 	intel_power_domains_init_hw(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8631fb3..e79f7bb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2078,7 +2078,8 @@ extern void intel_irq_init(struct drm_device *dev);
 extern void intel_hpd_init(struct drm_device *dev);
 
 extern void intel_uncore_sanitize(struct drm_device *dev);
-extern void intel_uncore_early_sanitize(struct drm_device *dev);
+extern void intel_uncore_early_sanitize(struct drm_device *dev,
+					bool restore_forcewake);
 extern void intel_uncore_init(struct drm_device *dev);
 extern void intel_uncore_check_errors(struct drm_device *dev);
 extern void intel_uncore_fini(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 389e235..14ae557 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -364,16 +364,12 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 			dev_priv->uncore.fifo_count =
 				__raw_i915_read32(dev_priv, GTFIFOCTL) &
 				GT_FIFO_FREE_ENTRIES_MASK;
-	} else {
-		dev_priv->uncore.forcewake_count = 0;
-		dev_priv->uncore.fw_rendercount = 0;
-		dev_priv->uncore.fw_mediacount = 0;
 	}
 
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
-void intel_uncore_early_sanitize(struct drm_device *dev)
+void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -396,7 +392,7 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
 		__raw_i915_write32(dev_priv, GTFIFODBG,
 				   __raw_i915_read32(dev_priv, GTFIFODBG));
 
-	intel_uncore_forcewake_reset(dev, false);
+	intel_uncore_forcewake_reset(dev, restore_forcewake);
 }
 
 void intel_uncore_sanitize(struct drm_device *dev)
@@ -827,7 +823,7 @@ void intel_uncore_init(struct drm_device *dev)
 	setup_timer(&dev_priv->uncore.force_wake_timer,
 		    gen6_force_wake_timer, (unsigned long)dev_priv);
 
-	intel_uncore_early_sanitize(dev);
+	intel_uncore_early_sanitize(dev, false);
 
 	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
-- 
1.8.4

Re: [PATCH 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[Chris Wilson]

On Fri, Jun 06, 2014 at 12:59:38PM +0300, Imre Deak wrote:
> If the timer putting the last forcewake refcount was pending and we
> canceled it, we'll leak the corresponding forcewake and RPM references.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c90222d..389e235 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -299,9 +299,8 @@ static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
> -static void gen6_force_wake_timer(unsigned long arg)
> +static void __gen6_force_wake_timer(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_i915_private *dev_priv = (void *)arg;
>  	unsigned long irqflags;
>  
>  	assert_device_not_suspended(dev_priv);
> @@ -316,12 +315,20 @@ static void gen6_force_wake_timer(unsigned long arg)
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> +static void gen6_force_wake_timer(unsigned long arg)
> +{
> +	struct drm_i915_private *dev_priv = (void *)arg;
> +
> +	__gen6_force_wake_timer(dev_priv);
> +}
> +
>  static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
>  
> -	del_timer_sync(&dev_priv->uncore.force_wake_timer);
> +	if (del_timer_sync(&dev_priv->uncore.force_wake_timer))
> +		__gen6_force_wake_timer(dev_priv);

Just do the cast for the exception case.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[Imre Deak]

If the timer putting the last forcewake refcount was pending and we
canceled it, we'll leak the corresponding forcewake and RPM references.

v2:
- do the ptr casting at the caller instead of adding a separate helper
  for this (Chris)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c90222d..6a73078 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -321,7 +321,8 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
 
-	del_timer_sync(&dev_priv->uncore.force_wake_timer);
+	if (del_timer_sync(&dev_priv->uncore.force_wake_timer))
+		gen6_force_wake_timer((unsigned long)dev_priv);
 
 	/* Hold uncore.lock across reset to prevent any register access
 	 * with forcewake not set correctly
-- 
1.8.4

Re: [PATCH v2 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[Chris Wilson]

On Fri, Jun 06, 2014 at 02:04:37PM +0300, Imre Deak wrote:
> If the timer putting the last forcewake refcount was pending and we
> canceled it, we'll leak the corresponding forcewake and RPM references.
> 
> v2:
> - do the ptr casting at the caller instead of adding a separate helper
>   for this (Chris)
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[Daniel Vetter]

On Fri, Jun 06, 2014 at 12:08:43PM +0100, Chris Wilson wrote:
> On Fri, Jun 06, 2014 at 02:04:37PM +0300, Imre Deak wrote:
> > If the timer putting the last forcewake refcount was pending and we
> > canceled it, we'll leak the corresponding forcewake and RPM references.
> > 
> > v2:
> > - do the ptr casting at the caller instead of adding a separate helper
> >   for this (Chris)
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Both patches merged to dinq (Chris clarified on irc that his r-b is for
both). Since this only blows up in a super-contrived testcase I don't
think this is material for -fixes.

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

Re: [PATCH v2 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[Imre Deak]

On Fri, 2014-06-06 at 19:46 +0200, Daniel Vetter wrote:
> On Fri, Jun 06, 2014 at 12:08:43PM +0100, Chris Wilson wrote:
> > On Fri, Jun 06, 2014 at 02:04:37PM +0300, Imre Deak wrote:
> > > If the timer putting the last forcewake refcount was pending and we
> > > canceled it, we'll leak the corresponding forcewake and RPM references.
> > > 
> > > v2:
> > > - do the ptr casting at the caller instead of adding a separate helper
> > >   for this (Chris)
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Both patches merged to dinq (Chris clarified on irc that his r-b is for
> both).
>
> Since this only blows up in a super-contrived testcase I don't
> think this is material for -fixes.

Note that the issue fixed by 1/2 could also happen normally, though the
window for race is small. One scenario would be runtime resume
->deferred rps_enable followed directly by system suspend or gpu reset.

--Imre 

Re: [PATCH v2 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[Daniel Vetter]

On Fri, Jun 06, 2014 at 09:38:26PM +0300, Imre Deak wrote:
> On Fri, 2014-06-06 at 19:46 +0200, Daniel Vetter wrote:
> > On Fri, Jun 06, 2014 at 12:08:43PM +0100, Chris Wilson wrote:
> > > On Fri, Jun 06, 2014 at 02:04:37PM +0300, Imre Deak wrote:
> > > > If the timer putting the last forcewake refcount was pending and we
> > > > canceled it, we'll leak the corresponding forcewake and RPM references.
> > > > 
> > > > v2:
> > > > - do the ptr casting at the caller instead of adding a separate helper
> > > >   for this (Chris)
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Both patches merged to dinq (Chris clarified on irc that his r-b is for
> > both).
> >
> > Since this only blows up in a super-contrived testcase I don't
> > think this is material for -fixes.
> 
> Note that the issue fixed by 1/2 could also happen normally, though the
> window for race is small. One scenario would be runtime resume
> ->deferred rps_enable followed directly by system suspend or gpu reset.

The default runtime pm autosuspend delay is longer than the delayed rps
enable, so for all practical purposes I think this is impossible.

But once we set the autoresume delay to something more sane (100ms maybe,
patches anyone?) we definitely need this patch here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[Imre Deak]

On Fri, 2014-06-06 at 22:15 +0200, Daniel Vetter wrote:
> On Fri, Jun 06, 2014 at 09:38:26PM +0300, Imre Deak wrote:
> > On Fri, 2014-06-06 at 19:46 +0200, Daniel Vetter wrote:
> > > On Fri, Jun 06, 2014 at 12:08:43PM +0100, Chris Wilson wrote:
> > > > On Fri, Jun 06, 2014 at 02:04:37PM +0300, Imre Deak wrote:
> > > > > If the timer putting the last forcewake refcount was pending and we
> > > > > canceled it, we'll leak the corresponding forcewake and RPM references.
> > > > > 
> > > > > v2:
> > > > > - do the ptr casting at the caller instead of adding a separate helper
> > > > >   for this (Chris)
> > > > > 
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > 
> > > Both patches merged to dinq (Chris clarified on irc that his r-b is for
> > > both).
> > >
> > > Since this only blows up in a super-contrived testcase I don't
> > > think this is material for -fixes.
> > 
> > Note that the issue fixed by 1/2 could also happen normally, though the
> > window for race is small. One scenario would be runtime resume
> > ->deferred rps_enable followed directly by system suspend or gpu reset.
> 
> The default runtime pm autosuspend delay is longer than the delayed rps
> enable, so for all practical purposes I think this is impossible.

But system suspend is not affected by that delay, it can happen right
after a runtime resume.

> But once we set the autoresume delay to something more sane (100ms maybe,
> patches anyone?) we definitely need this patch here.
> -Daniel

Re: [PATCH v2 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[Daniel Vetter]

On Fri, Jun 06, 2014 at 11:19:28PM +0300, Imre Deak wrote:
> On Fri, 2014-06-06 at 22:15 +0200, Daniel Vetter wrote:
> > On Fri, Jun 06, 2014 at 09:38:26PM +0300, Imre Deak wrote:
> > > On Fri, 2014-06-06 at 19:46 +0200, Daniel Vetter wrote:
> > > > On Fri, Jun 06, 2014 at 12:08:43PM +0100, Chris Wilson wrote:
> > > > > On Fri, Jun 06, 2014 at 02:04:37PM +0300, Imre Deak wrote:
> > > > > > If the timer putting the last forcewake refcount was pending and we
> > > > > > canceled it, we'll leak the corresponding forcewake and RPM references.
> > > > > > 
> > > > > > v2:
> > > > > > - do the ptr casting at the caller instead of adding a separate helper
> > > > > >   for this (Chris)
> > > > > > 
> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > 
> > > > Both patches merged to dinq (Chris clarified on irc that his r-b is for
> > > > both).
> > > >
> > > > Since this only blows up in a super-contrived testcase I don't
> > > > think this is material for -fixes.
> > > 
> > > Note that the issue fixed by 1/2 could also happen normally, though the
> > > window for race is small. One scenario would be runtime resume
> > > ->deferred rps_enable followed directly by system suspend or gpu reset.
> > 
> > The default runtime pm autosuspend delay is longer than the delayed rps
> > enable, so for all practical purposes I think this is impossible.
> 
> But system suspend is not affected by that delay, it can happen right
> after a runtime resume.

Until we drop the forcewake time we hold a runtime pm ref. So I don't
think there's an issue either on that side of the system resume ... Or do
I miss something?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[Imre Deak]

On Fri, 2014-06-06 at 22:35 +0200, Daniel Vetter wrote:
> On Fri, Jun 06, 2014 at 11:19:28PM +0300, Imre Deak wrote:
> > On Fri, 2014-06-06 at 22:15 +0200, Daniel Vetter wrote:
> > > On Fri, Jun 06, 2014 at 09:38:26PM +0300, Imre Deak wrote:
> > > > On Fri, 2014-06-06 at 19:46 +0200, Daniel Vetter wrote:
> > > > > On Fri, Jun 06, 2014 at 12:08:43PM +0100, Chris Wilson wrote:
> > > > > > On Fri, Jun 06, 2014 at 02:04:37PM +0300, Imre Deak wrote:
> > > > > > > If the timer putting the last forcewake refcount was pending and we
> > > > > > > canceled it, we'll leak the corresponding forcewake and RPM references.
> > > > > > > 
> > > > > > > v2:
> > > > > > > - do the ptr casting at the caller instead of adding a separate helper
> > > > > > >   for this (Chris)
> > > > > > > 
> > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > 
> > > > > Both patches merged to dinq (Chris clarified on irc that his r-b is for
> > > > > both).
> > > > >
> > > > > Since this only blows up in a super-contrived testcase I don't
> > > > > think this is material for -fixes.
> > > > 
> > > > Note that the issue fixed by 1/2 could also happen normally, though the
> > > > window for race is small. One scenario would be runtime resume
> > > > ->deferred rps_enable followed directly by system suspend or gpu reset.
> > > 
> > > The default runtime pm autosuspend delay is longer than the delayed rps
> > > enable, so for all practical purposes I think this is impossible.
> > 
> > But system suspend is not affected by that delay, it can happen right
> > after a runtime resume.
> 
> Until we drop the forcewake time we hold a runtime pm ref. So I don't
> think there's an issue either on that side of the system resume ... Or do
> I miss something?

Let's say that forcewake timer is pending, holding the runtime pm ref.
System suspend is called - it's not prevented by either this ref or the
above autosuspend delay - in the suspend handler we eventually call
force_wake_reset which cancels the timer, leaking the runtime pm ref.

--Imre 

Re: [PATCH v2 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[Daniel Vetter]

On Fri, Jun 6, 2014 at 10:44 PM, Imre Deak <imre.deak@intel.com> wrote:
> Let's say that forcewake timer is pending, holding the runtime pm ref.
> System suspend is called - it's not prevented by either this ref or the
> above autosuspend delay - in the suspend handler we eventually call
> force_wake_reset which cancels the timer, leaking the runtime pm ref.

Hm, I indeed mixed things up. I guess the window is small with the
short timeout we have for the forcewake timer, but still the first
patch makes sense for -fixes. Jani?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 1/2] drm/i915: fix possible refcount leak when resetting forcewake

[Jani Nikula]

On Fri, 06 Jun 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Jun 6, 2014 at 10:44 PM, Imre Deak <imre.deak@intel.com> wrote:
>> Let's say that forcewake timer is pending, holding the runtime pm ref.
>> System suspend is called - it's not prevented by either this ref or the
>> above autosuspend delay - in the suspend handler we eventually call
>> force_wake_reset which cancels the timer, leaking the runtime pm ref.
>
> Hm, I indeed mixed things up. I guess the window is small with the
> short timeout we have for the forcewake timer, but still the first
> patch makes sense for -fixes. Jani?

v2 of 1/2 pushed to -fixes, thanks for that patch and review.

BR,
Jani.


> -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

-- 
Jani Nikula, Intel Open Source Technology Center