* [PATCH] drm/i915: flush delayed_resume_work when suspending
@ 2014-06-27 21:51 Paulo Zanoni
2014-06-27 22:30 ` Rodrigo Vivi
2014-07-01 14:49 ` Jani Nikula
0 siblings, 2 replies; 6+ messages in thread
From: Paulo Zanoni @ 2014-06-27 21:51 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
It is possible that, by the time we run i915_drm_freeze(),
delayed_resume_work was already queued but did not run yet. If it
still didn't run after intel_runtime_pm_disable_interrupts(), by the
time it runs it will try to change the interrupt registers with the
interrupts already disabled, which will trigger a WARN. We can
reliably reproduce this with the pm_rpm system-suspend test case.
In order to avoid the problem, we have to flush the work before
disabling the interrupts. We could also cancel the work instead of
flushing it, but that would require us to put a runtime PM reference -
and any other resource we may need in the future - in case the work
was already queued, so I believe flushing the work is more
future-proof, although less efficient. But I can also change this part
if someone requests.
Another thing I tried was to move the intel_suspend_gt_powersave()
call to before intel_runtime_pm_disable_interrupts(), but since that
function needs to be called after the interrupts are already disabled,
due to dev_priv->rps.work, this strategy didn't work.
Testcase: igt/pm_rpm/system-suspend
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80517
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e64547e..672694b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -524,6 +524,8 @@ static int i915_drm_freeze(struct drm_device *dev)
return error;
}
+ flush_delayed_work(&dev_priv->rps.delayed_resume_work);
+
intel_runtime_pm_disable_interrupts(dev);
dev_priv->enable_hotplug_processing = false;
--
2.0.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: flush delayed_resume_work when suspending
2014-06-27 21:51 [PATCH] drm/i915: flush delayed_resume_work when suspending Paulo Zanoni
@ 2014-06-27 22:30 ` Rodrigo Vivi
2014-06-30 19:22 ` Paulo Zanoni
2014-07-01 14:49 ` Jani Nikula
1 sibling, 1 reply; 6+ messages in thread
From: Rodrigo Vivi @ 2014-06-27 22:30 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
[-- Attachment #1.1: Type: text/plain, Size: 2583 bytes --]
I have the feeling the safest side would be disable rc6 on resume instead
of force its enabling... or am I missing something?
why don't you just cancel the work? and put another after resume?
but if the patch really solves the problem and this is what you meant feel
free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
On Fri, Jun 27, 2014 at 2:51 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> It is possible that, by the time we run i915_drm_freeze(),
> delayed_resume_work was already queued but did not run yet. If it
> still didn't run after intel_runtime_pm_disable_interrupts(), by the
> time it runs it will try to change the interrupt registers with the
> interrupts already disabled, which will trigger a WARN. We can
> reliably reproduce this with the pm_rpm system-suspend test case.
>
> In order to avoid the problem, we have to flush the work before
> disabling the interrupts. We could also cancel the work instead of
> flushing it, but that would require us to put a runtime PM reference -
> and any other resource we may need in the future - in case the work
> was already queued, so I believe flushing the work is more
> future-proof, although less efficient. But I can also change this part
> if someone requests.
>
> Another thing I tried was to move the intel_suspend_gt_powersave()
> call to before intel_runtime_pm_disable_interrupts(), but since that
> function needs to be called after the interrupts are already disabled,
> due to dev_priv->rps.work, this strategy didn't work.
>
> Testcase: igt/pm_rpm/system-suspend
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80517
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index e64547e..672694b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -524,6 +524,8 @@ static int i915_drm_freeze(struct drm_device *dev)
> return error;
> }
>
> + flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
> intel_runtime_pm_disable_interrupts(dev);
> dev_priv->enable_hotplug_processing = false;
>
> --
> 2.0.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
[-- Attachment #1.2: Type: text/html, Size: 3723 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: flush delayed_resume_work when suspending
2014-06-27 22:30 ` Rodrigo Vivi
@ 2014-06-30 19:22 ` Paulo Zanoni
2014-06-30 19:54 ` [PATCH] drm/i915: cancel " Paulo Zanoni
2014-06-30 20:10 ` [PATCH] drm/i915: flush " Rodrigo Vivi
0 siblings, 2 replies; 6+ messages in thread
From: Paulo Zanoni @ 2014-06-30 19:22 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx, Paulo Zanoni
2014-06-27 19:30 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> I have the feeling the safest side would be disable rc6 on resume instead of
> force its enabling... or am I missing something?
It will be enabled, then disabled.
> why don't you just cancel the work? and put another after resume?
>
> but if the patch really solves the problem and this is what you meant feel
> free to use:
What you're suggesting is the "We could also" case mentioned in the
second paragraph of the commit message. I even wrote and tested that
patch, but Jesse seemed to prefer the "flush" version instead of the
"cancel" one. I'll send the other version to the list, then reviewers
and maintainers can decide which one they prefer :)
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
>
>
> On Fri, Jun 27, 2014 at 2:51 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> It is possible that, by the time we run i915_drm_freeze(),
>> delayed_resume_work was already queued but did not run yet. If it
>> still didn't run after intel_runtime_pm_disable_interrupts(), by the
>> time it runs it will try to change the interrupt registers with the
>> interrupts already disabled, which will trigger a WARN. We can
>> reliably reproduce this with the pm_rpm system-suspend test case.
>>
>> In order to avoid the problem, we have to flush the work before
>> disabling the interrupts. We could also cancel the work instead of
>> flushing it, but that would require us to put a runtime PM reference -
>> and any other resource we may need in the future - in case the work
>> was already queued, so I believe flushing the work is more
>> future-proof, although less efficient. But I can also change this part
>> if someone requests.
>>
>> Another thing I tried was to move the intel_suspend_gt_powersave()
>> call to before intel_runtime_pm_disable_interrupts(), but since that
>> function needs to be called after the interrupts are already disabled,
>> due to dev_priv->rps.work, this strategy didn't work.
>>
>> Testcase: igt/pm_rpm/system-suspend
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80517
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index e64547e..672694b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -524,6 +524,8 @@ static int i915_drm_freeze(struct drm_device *dev)
>> return error;
>> }
>>
>> + flush_delayed_work(&dev_priv->rps.delayed_resume_work);
>> +
>> intel_runtime_pm_disable_interrupts(dev);
>> dev_priv->enable_hotplug_processing = false;
>>
>> --
>> 2.0.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
>
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] drm/i915: cancel delayed_resume_work when suspending
2014-06-30 19:22 ` Paulo Zanoni
@ 2014-06-30 19:54 ` Paulo Zanoni
2014-06-30 20:10 ` [PATCH] drm/i915: flush " Rodrigo Vivi
1 sibling, 0 replies; 6+ messages in thread
From: Paulo Zanoni @ 2014-06-30 19:54 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
It is possible that, by the time we run i915_drm_freeze(),
delayed_resume_work was already queued but did not run yet. If it
still didn't run after intel_runtime_pm_disable_interrupts(), by the
time it runs it will try to change the interrupt registers with the
interrupts already disabled, which will trigger a WARN. We can
reliably reproduce this with the pm_rpm system-suspend test case.
In order to avoid the probelm, we flush the work and put the necessary
resources in case the work was already scheduled.
Another thing I tried was to move the intel_suspend_gt_powersave()
call to before intel_runtime_pm_disable_interrupts(), but since that
function needs to be called after the interrupts are already disabled,
due to dev_priv->rps.work, this strategy didn't work.
Testcase: igt/pm_rpm/system-suspend
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80517
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 2 ++
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_pm.c | 8 ++++++++
3 files changed, 11 insertions(+)
Now the maintainers and reviewers can choose if they prefer the "cancel" or the
"flush" version of the patch.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6eb45ac..dad7e8b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -520,6 +520,8 @@ static int i915_drm_freeze(struct drm_device *dev)
return error;
}
+ intel_cancel_rps_resume_work(dev_priv);
+
intel_runtime_pm_disable_interrupts(dev);
dev_priv->enable_hotplug_processing = false;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5f7c7bd..b122b12 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -993,6 +993,7 @@ void intel_enable_gt_powersave(struct drm_device *dev);
void intel_disable_gt_powersave(struct drm_device *dev);
void intel_suspend_gt_powersave(struct drm_device *dev);
void intel_reset_gt_powersave(struct drm_device *dev);
+void intel_cancel_rps_resume_work(struct drm_i915_private *dev_priv);
void ironlake_teardown_rc6(struct drm_device *dev);
void gen6_update_ring_freq(struct drm_device *dev);
void gen6_rps_idle(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a90fdbd..664099f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4911,6 +4911,14 @@ void intel_disable_gt_powersave(struct drm_device *dev)
}
}
+/* This function needs to put/clear any resources that the
+ * rps.delayed_resume_work function puts/clears. */
+void intel_cancel_rps_resume_work(struct drm_i915_private *dev_priv)
+{
+ if (cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work))
+ intel_runtime_pm_put(dev_priv);
+}
+
static void intel_gen6_powersave_work(struct work_struct *work)
{
struct drm_i915_private *dev_priv =
--
2.0.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: flush delayed_resume_work when suspending
2014-06-30 19:22 ` Paulo Zanoni
2014-06-30 19:54 ` [PATCH] drm/i915: cancel " Paulo Zanoni
@ 2014-06-30 20:10 ` Rodrigo Vivi
1 sibling, 0 replies; 6+ messages in thread
From: Rodrigo Vivi @ 2014-06-30 20:10 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
[-- Attachment #1.1: Type: text/plain, Size: 3649 bytes --]
On Mon, Jun 30, 2014 at 12:22 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2014-06-27 19:30 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> > I have the feeling the safest side would be disable rc6 on resume
> instead of
> > force its enabling... or am I missing something?
>
> It will be enabled, then disabled.
>
oh that's true!
>
> > why don't you just cancel the work? and put another after resume?
> >
> > but if the patch really solves the problem and this is what you meant
> feel
> > free to use:
>
> What you're suggesting is the "We could also" case mentioned in the
> second paragraph of the commit message. I even wrote and tested that
> patch,
Yeah, reading again this is exactly what I had in mind.
> but Jesse seemed to prefer the "flush" version instead of the
> "cancel" one. I'll send the other version to the list, then reviewers
> and maintainers can decide which one they prefer :)
>
I don't have stronger preferences. So, feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Thanks for the explanations,
Rodrigo.
>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >
> >
> >
> > On Fri, Jun 27, 2014 at 2:51 PM, Paulo Zanoni <przanoni@gmail.com>
> wrote:
> >>
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> It is possible that, by the time we run i915_drm_freeze(),
> >> delayed_resume_work was already queued but did not run yet. If it
> >> still didn't run after intel_runtime_pm_disable_interrupts(), by the
> >> time it runs it will try to change the interrupt registers with the
> >> interrupts already disabled, which will trigger a WARN. We can
> >> reliably reproduce this with the pm_rpm system-suspend test case.
> >>
> >> In order to avoid the problem, we have to flush the work before
> >> disabling the interrupts. We could also cancel the work instead of
> >> flushing it, but that would require us to put a runtime PM reference -
> >> and any other resource we may need in the future - in case the work
> >> was already queued, so I believe flushing the work is more
> >> future-proof, although less efficient. But I can also change this part
> >> if someone requests.
> >>
> >> Another thing I tried was to move the intel_suspend_gt_powersave()
> >> call to before intel_runtime_pm_disable_interrupts(), but since that
> >> function needs to be called after the interrupts are already disabled,
> >> due to dev_priv->rps.work, this strategy didn't work.
> >>
> >> Testcase: igt/pm_rpm/system-suspend
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80517
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >> drivers/gpu/drm/i915/i915_drv.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> >> b/drivers/gpu/drm/i915/i915_drv.c
> >> index e64547e..672694b 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -524,6 +524,8 @@ static int i915_drm_freeze(struct drm_device *dev)
> >> return error;
> >> }
> >>
> >> + flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> >> +
> >> intel_runtime_pm_disable_interrupts(dev);
> >> dev_priv->enable_hotplug_processing = false;
> >>
> >> --
> >> 2.0.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> >
> > --
> > Rodrigo Vivi
> > Blog: http://blog.vivi.eng.br
> >
>
>
>
> --
> Paulo Zanoni
>
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
[-- Attachment #1.2: Type: text/html, Size: 6022 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: flush delayed_resume_work when suspending
2014-06-27 21:51 [PATCH] drm/i915: flush delayed_resume_work when suspending Paulo Zanoni
2014-06-27 22:30 ` Rodrigo Vivi
@ 2014-07-01 14:49 ` Jani Nikula
1 sibling, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2014-07-01 14:49 UTC (permalink / raw)
To: Paulo Zanoni, intel-gfx; +Cc: Paulo Zanoni
On Sat, 28 Jun 2014, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> It is possible that, by the time we run i915_drm_freeze(),
> delayed_resume_work was already queued but did not run yet. If it
> still didn't run after intel_runtime_pm_disable_interrupts(), by the
> time it runs it will try to change the interrupt registers with the
> interrupts already disabled, which will trigger a WARN. We can
> reliably reproduce this with the pm_rpm system-suspend test case.
>
> In order to avoid the problem, we have to flush the work before
> disabling the interrupts. We could also cancel the work instead of
> flushing it, but that would require us to put a runtime PM reference -
> and any other resource we may need in the future - in case the work
> was already queued, so I believe flushing the work is more
> future-proof, although less efficient. But I can also change this part
> if someone requests.
>
> Another thing I tried was to move the intel_suspend_gt_powersave()
> call to before intel_runtime_pm_disable_interrupts(), but since that
> function needs to be called after the interrupts are already disabled,
> due to dev_priv->rps.work, this strategy didn't work.
>
> Testcase: igt/pm_rpm/system-suspend
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80517
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Pushed to dinq, thanks for the patch and review.
BR,
Jani.
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e64547e..672694b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -524,6 +524,8 @@ static int i915_drm_freeze(struct drm_device *dev)
> return error;
> }
>
> + flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
> intel_runtime_pm_disable_interrupts(dev);
> dev_priv->enable_hotplug_processing = false;
>
> --
> 2.0.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-01 14:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-27 21:51 [PATCH] drm/i915: flush delayed_resume_work when suspending Paulo Zanoni
2014-06-27 22:30 ` Rodrigo Vivi
2014-06-30 19:22 ` Paulo Zanoni
2014-06-30 19:54 ` [PATCH] drm/i915: cancel " Paulo Zanoni
2014-06-30 20:10 ` [PATCH] drm/i915: flush " Rodrigo Vivi
2014-07-01 14:49 ` Jani Nikula
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox