On Mon, Jun 30, 2014 at 12:22 PM, Paulo Zanoni <przan...@gmail.com> wrote:
> 2014-06-27 19:30 GMT-03:00 Rodrigo Vivi <rodrigo.v...@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.v...@intel.com> Thanks for the explanations, Rodrigo. > > > Reviewed-by: Rodrigo Vivi <rodrigo.v...@intel.com> > > > > > > > > On Fri, Jun 27, 2014 at 2:51 PM, Paulo Zanoni <przan...@gmail.com> > wrote: > >> > >> From: Paulo Zanoni <paulo.r.zan...@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.zan...@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
_______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx