Re: [Intel-gfx] [PATCH] drm/i915: flush delayed_resume_work when suspending
On Sat, 28 Jun 2014, 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 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: flush delayed_resume_work when suspending
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. 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.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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: flush delayed_resume_work when suspending
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
Re: [Intel-gfx] [PATCH] drm/i915: flush delayed_resume_work when suspending
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.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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
