[PATCH 1/2] drm/i915: fix possible RPM ref leaking during RPS disabling

[PATCH 1/2] drm/i915: fix possible RPM ref leaking during RPS disabling

[Imre Deak]

In

commit c6df39b5ea6342323a42edfbeeca0a28c643d7ae
Author: Imre Deak <imre.deak@intel.com>
Date:   Mon Apr 14 20:24:29 2014 +0300

    drm/i915: get a runtime PM ref for the deferred GT powersave enabling

I added an RPM get-ref when enabling RPS from a deferred work, but forgot
to add the corresponding put-ref when canceling the work. This may leave
RPM disabled.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 834c49c..9170039 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4609,7 +4609,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
 		ironlake_disable_drps(dev);
 		ironlake_disable_rc6(dev);
 	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
-		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
+		if (cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work))
+			intel_runtime_pm_put(dev_priv);
+
 		cancel_work_sync(&dev_priv->rps.work);
 		mutex_lock(&dev_priv->rps.hw_lock);
 		if (IS_VALLEYVIEW(dev))
-- 
1.8.4

[PATCH 2/2] drm/i915: disable GT power saving early during system suspend

[Imre Deak]

Atm, we disable GT power saving during the end of the suspend sequence
in i915_save_state(). Doing the disabling at that point seems arbitrary.
One reason to disable it early though is to have a quiescent HW state
before we do anything else (for example save registers). So move the
disabling earlier, which also takes care canceling of the deferred RPS
enabling work done by intel_disable_gt_powersave().

Note that after the move we'll call intel_disable_gt_powersave() only
in case modeset is enabled, but that's anyway the only case where we
have it enabled in the first place.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     | 5 +++--
 drivers/gpu/drm/i915/i915_suspend.c | 2 --
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2413fc3..855295a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -468,10 +468,11 @@ static int i915_drm_freeze(struct drm_device *dev)
 			return error;
 		}
 
-		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
-
 		drm_irq_uninstall(dev);
 		dev_priv->enable_hotplug_processing = false;
+
+		intel_disable_gt_powersave(dev);
+
 		/*
 		 * Disable CRTCs directly since we want to preserve sw state
 		 * for _thaw.
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 56785e8..043123c 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -328,8 +328,6 @@ int i915_save_state(struct drm_device *dev)
 		}
 	}
 
-	intel_disable_gt_powersave(dev);
-
 	/* Cache mode state */
 	if (INTEL_INFO(dev)->gen < 7)
 		dev_priv->regfile.saveCACHE_MODE_0 = I915_READ(CACHE_MODE_0);
-- 
1.8.4

Re: [PATCH 1/2] drm/i915: fix possible RPM ref leaking during RPS disabling

[Daniel Vetter]

On Mon, May 12, 2014 at 06:35:04PM +0300, Imre Deak wrote:
> In
> 
> commit c6df39b5ea6342323a42edfbeeca0a28c643d7ae
> Author: Imre Deak <imre.deak@intel.com>
> Date:   Mon Apr 14 20:24:29 2014 +0300
> 
>     drm/i915: get a runtime PM ref for the deferred GT powersave enabling
> 
> I added an RPM get-ref when enabling RPS from a deferred work, but forgot
> to add the corresponding put-ref when canceling the work. This may leave
> RPM disabled.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Could we intentionally hit this by e.g. racing a suspend/resume against
runtime pm? E.g.
1. disable all outputs, make sure we've entered runtime pm
2. set runtime autosuspend delay to 0
3. suspend/resume
4. autosuspend (hopefully, my understanding is a bit unclear)

->Boom

Would look nice as an igt subtest if it works ;-)
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 834c49c..9170039 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4609,7 +4609,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
>  		ironlake_disable_drps(dev);
>  		ironlake_disable_rc6(dev);
>  	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
> -		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
> +		if (cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work))
> +			intel_runtime_pm_put(dev_priv);
> +
>  		cancel_work_sync(&dev_priv->rps.work);
>  		mutex_lock(&dev_priv->rps.hw_lock);
>  		if (IS_VALLEYVIEW(dev))
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/2] drm/i915: fix possible RPM ref leaking during RPS disabling

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 2187 bytes --]

On Mon, 2014-05-12 at 19:51 +0200, Daniel Vetter wrote:
> On Mon, May 12, 2014 at 06:35:04PM +0300, Imre Deak wrote:
> > In
> > 
> > commit c6df39b5ea6342323a42edfbeeca0a28c643d7ae
> > Author: Imre Deak <imre.deak@intel.com>
> > Date:   Mon Apr 14 20:24:29 2014 +0300
> > 
> >     drm/i915: get a runtime PM ref for the deferred GT powersave enabling
> > 
> > I added an RPM get-ref when enabling RPS from a deferred work, but forgot
> > to add the corresponding put-ref when canceling the work. This may leave
> > RPM disabled.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Could we intentionally hit this by e.g. racing a suspend/resume against
> runtime pm? E.g.
> 1. disable all outputs, make sure we've entered runtime pm
> 2. set runtime autosuspend delay to 0
> 3. suspend/resume
> 4. autosuspend (hopefully, my understanding is a bit unclear)
> 
> ->Boom
> 
> Would look nice as an igt subtest if it works ;-)

Yep, works consistently as expected both before and after the fix. I
pushed the new subtest, please add here:
Testcase: igt/pm_pc8/system-suspend

--Imre

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 834c49c..9170039 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4609,7 +4609,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
> >  		ironlake_disable_drps(dev);
> >  		ironlake_disable_rc6(dev);
> >  	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
> > -		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
> > +		if (cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work))
> > +			intel_runtime_pm_put(dev_priv);
> > +
> >  		cancel_work_sync(&dev_priv->rps.work);
> >  		mutex_lock(&dev_priv->rps.hw_lock);
> >  		if (IS_VALLEYVIEW(dev))
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/2] drm/i915: fix possible RPM ref leaking during RPS disabling

[Daniel Vetter]

On Tue, May 13, 2014 at 04:46:10PM +0300, Imre Deak wrote:
> On Mon, 2014-05-12 at 19:51 +0200, Daniel Vetter wrote:
> > On Mon, May 12, 2014 at 06:35:04PM +0300, Imre Deak wrote:
> > > In
> > > 
> > > commit c6df39b5ea6342323a42edfbeeca0a28c643d7ae
> > > Author: Imre Deak <imre.deak@intel.com>
> > > Date:   Mon Apr 14 20:24:29 2014 +0300
> > > 
> > >     drm/i915: get a runtime PM ref for the deferred GT powersave enabling
> > > 
> > > I added an RPM get-ref when enabling RPS from a deferred work, but forgot
> > > to add the corresponding put-ref when canceling the work. This may leave
> > > RPM disabled.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > Could we intentionally hit this by e.g. racing a suspend/resume against
> > runtime pm? E.g.
> > 1. disable all outputs, make sure we've entered runtime pm
> > 2. set runtime autosuspend delay to 0
> > 3. suspend/resume
> > 4. autosuspend (hopefully, my understanding is a bit unclear)
> > 
> > ->Boom
> > 
> > Would look nice as an igt subtest if it works ;-)
> 
> Yep, works consistently as expected both before and after the fix. I
> pushed the new subtest, please add here:
> Testcase: igt/pm_pc8/system-suspend

Now I'm confused: I expected this to blow up without your fix here, and
not work with or without it?! Please unconfuse ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: fix possible RPM ref leaking during RPS disabling

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 1578 bytes --]

On Tue, 2014-05-13 at 15:54 +0200, Daniel Vetter wrote:
> On Tue, May 13, 2014 at 04:46:10PM +0300, Imre Deak wrote:
> > On Mon, 2014-05-12 at 19:51 +0200, Daniel Vetter wrote:
> > > On Mon, May 12, 2014 at 06:35:04PM +0300, Imre Deak wrote:
> > > > In
> > > > 
> > > > commit c6df39b5ea6342323a42edfbeeca0a28c643d7ae
> > > > Author: Imre Deak <imre.deak@intel.com>
> > > > Date:   Mon Apr 14 20:24:29 2014 +0300
> > > > 
> > > >     drm/i915: get a runtime PM ref for the deferred GT powersave enabling
> > > > 
> > > > I added an RPM get-ref when enabling RPS from a deferred work, but forgot
> > > > to add the corresponding put-ref when canceling the work. This may leave
> > > > RPM disabled.
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > 
> > > Could we intentionally hit this by e.g. racing a suspend/resume against
> > > runtime pm? E.g.
> > > 1. disable all outputs, make sure we've entered runtime pm
> > > 2. set runtime autosuspend delay to 0
> > > 3. suspend/resume
> > > 4. autosuspend (hopefully, my understanding is a bit unclear)
> > > 
> > > ->Boom
> > > 
> > > Would look nice as an igt subtest if it works ;-)
> > 
> > Yep, works consistently as expected both before and after the fix. I
> > pushed the new subtest, please add here:
> > Testcase: igt/pm_pc8/system-suspend
> 
> Now I'm confused: I expected this to blow up without your fix here, and
> not work with or without it?! Please unconfuse ...

Blows up consistently before the fix and passes consistently after.

--Imre

> -Daniel


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/2] drm/i915: fix possible RPM ref leaking during RPS disabling

[Robert Beckett]

On 13/05/2014 14:54, Daniel Vetter wrote:
> On Tue, May 13, 2014 at 04:46:10PM +0300, Imre Deak wrote:
>> On Mon, 2014-05-12 at 19:51 +0200, Daniel Vetter wrote:
>>> On Mon, May 12, 2014 at 06:35:04PM +0300, Imre Deak wrote:
>>>> In
>>>>
>>>> commit c6df39b5ea6342323a42edfbeeca0a28c643d7ae
>>>> Author: Imre Deak <imre.deak@intel.com>
>>>> Date:   Mon Apr 14 20:24:29 2014 +0300
>>>>
>>>>      drm/i915: get a runtime PM ref for the deferred GT powersave enabling
>>>>
>>>> I added an RPM get-ref when enabling RPS from a deferred work, but forgot
>>>> to add the corresponding put-ref when canceling the work. This may leave
>>>> RPM disabled.
>>>>
>>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>>
>>> Could we intentionally hit this by e.g. racing a suspend/resume against
>>> runtime pm? E.g.
>>> 1. disable all outputs, make sure we've entered runtime pm
>>> 2. set runtime autosuspend delay to 0
>>> 3. suspend/resume
>>> 4. autosuspend (hopefully, my understanding is a bit unclear)
>>>
>>> ->Boom
>>>
>>> Would look nice as an igt subtest if it works ;-)
>>
>> Yep, works consistently as expected both before and after the fix. I
>> pushed the new subtest, please add here:
>> Testcase: igt/pm_pc8/system-suspend
>
> Now I'm confused: I expected this to blow up without your fix here, and
> not work with or without it?! Please unconfuse ...
> -Daniel
>
Im not sure what you are expecting to go boom.
It is pretty difficult to get a race between suspend/resume and runtime 
pm. During system suspend, the pm core grabs a runtime ref without 
resume callbacks, and releases it on the resume. (see 
https://www.kernel.org/doc/Documentation/power/runtime_pm.txt part 6).

Also, as i915_drm_freeze does a runtime pm get, at step 3, it will cause 
a runtime resume before doing the system suspend. On the system resume, 
it will do a runtime pm put, which will cause a runtime suspend due to 
autosuspend delay being 0, and so step 4, autosuspend will occur. This 
should all work fine (assuming the patch is applied to handle any 
refcounting between back to back resume->suspend which cancels the 
enable before it is executed).

It all looks fine to me.

Reviewed-by: Robert Beckett <robert.beckett@intel.com>

Re: [PATCH 1/2] drm/i915: fix possible RPM ref leaking during RPS disabling

[Daniel Vetter]

On Thu, May 22, 2014 at 01:56:17PM +0100, Robert Beckett wrote:
> On 13/05/2014 14:54, Daniel Vetter wrote:
> >On Tue, May 13, 2014 at 04:46:10PM +0300, Imre Deak wrote:
> >>On Mon, 2014-05-12 at 19:51 +0200, Daniel Vetter wrote:
> >>>On Mon, May 12, 2014 at 06:35:04PM +0300, Imre Deak wrote:
> >>>>In
> >>>>
> >>>>commit c6df39b5ea6342323a42edfbeeca0a28c643d7ae
> >>>>Author: Imre Deak <imre.deak@intel.com>
> >>>>Date:   Mon Apr 14 20:24:29 2014 +0300
> >>>>
> >>>>     drm/i915: get a runtime PM ref for the deferred GT powersave enabling
> >>>>
> >>>>I added an RPM get-ref when enabling RPS from a deferred work, but forgot
> >>>>to add the corresponding put-ref when canceling the work. This may leave
> >>>>RPM disabled.
> >>>>
> >>>>Signed-off-by: Imre Deak <imre.deak@intel.com>
> >>>
> >>>Could we intentionally hit this by e.g. racing a suspend/resume against
> >>>runtime pm? E.g.
> >>>1. disable all outputs, make sure we've entered runtime pm
> >>>2. set runtime autosuspend delay to 0
> >>>3. suspend/resume
> >>>4. autosuspend (hopefully, my understanding is a bit unclear)
> >>>
> >>>->Boom
> >>>
> >>>Would look nice as an igt subtest if it works ;-)
> >>
> >>Yep, works consistently as expected both before and after the fix. I
> >>pushed the new subtest, please add here:
> >>Testcase: igt/pm_pc8/system-suspend
> >
> >Now I'm confused: I expected this to blow up without your fix here, and
> >not work with or without it?! Please unconfuse ...
> >-Daniel
> >
> Im not sure what you are expecting to go boom.
> It is pretty difficult to get a race between suspend/resume and runtime pm.
> During system suspend, the pm core grabs a runtime ref without resume
> callbacks, and releases it on the resume. (see
> https://www.kernel.org/doc/Documentation/power/runtime_pm.txt part 6).
> 
> Also, as i915_drm_freeze does a runtime pm get, at step 3, it will cause a
> runtime resume before doing the system suspend. On the system resume, it
> will do a runtime pm put, which will cause a runtime suspend due to
> autosuspend delay being 0, and so step 4, autosuspend will occur. This
> should all work fine (assuming the patch is applied to handle any
> refcounting between back to back resume->suspend which cancels the enable
> before it is executed).

Rest assured that Imre's igt goes indeed boom. We run the rps stuff
delayed which makes the race possible, as long as you autosuspend quickly
enough. You might want to run the testcase to confirm that ;-)

And thanks a lot for mentioning your concerns, this kind of information
and questions are exactly what I expect from patch reviewers. I will
augment the commit message a bit to make it clear how stuff blows up.

> It all looks fine to me.
> 
> Reviewed-by: Robert Beckett <robert.beckett@intel.com>

Thanks, Daniel

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

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