[Intel-gfx] [PATCH] drm/i915: flush delayed_resume_work when suspending

[Rodrigo Vivi]

On Mon, Jun 30, 2014 at 12:22 PM, Paulo Zanoni <przanoni at gmail.com> wrote:

> 2014-06-27 19:30 GMT-03:00 Rodrigo Vivi <rodrigo.vivi at 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 at intel.com>

Thanks for the explanations,
Rodrigo.


>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> >
> >
> >
> > On Fri, Jun 27, 2014 at 2:51 PM, Paulo Zanoni <przanoni at gmail.com>
> wrote:
> >>
> >> From: Paulo Zanoni <paulo.r.zanoni at 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 at 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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140630/10668d9d/attachment.html>