<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jun 30, 2014 at 12:22 PM, Paulo Zanoni <span dir="ltr"><<a href="mailto:przanoni@gmail.com" target="_blank">przanoni@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">2014-06-27 19:30 GMT-03:00 Rodrigo Vivi <<a href="mailto:rodrigo.vivi@gmail.com">rodrigo.vivi@gmail.com</a>>:<br>

<div class="">> I have the feeling the safest side would be disable rc6 on resume instead of<br>
> force its enabling... or am I missing something?<br>
<br>
</div>It will be enabled, then disabled.<br></blockquote><div><br></div><div>oh that's true!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
> why don't you just cancel the work? and put another after resume?<br>
><br>
> but if the patch really solves the problem and this is what you meant feel<br>
> free to use:<br>
<br>
</div>What you're suggesting is the "We could also" case mentioned in the<br>
second paragraph of the commit message. I even wrote and tested that<br>
patch, </blockquote><div><br></div><div>Yeah, reading again this is exactly what I had in mind.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">but Jesse seemed to prefer the "flush" version instead of the<br>

"cancel" one. I'll send the other version to the list, then reviewers<br>
and maintainers can decide which one they prefer :)<br></blockquote><div><br></div><div>I don't have stronger preferences. So, feel free to use:</div><div>Reviewed-by: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com">rodrigo.vivi@intel.com</a>></div>
<div><br></div><div>Thanks for the explanations,</div><div>Rodrigo.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> Reviewed-by: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com">rodrigo.vivi@intel.com</a>><br>
><br>
><br>
><br>
> On Fri, Jun 27, 2014 at 2:51 PM, Paulo Zanoni <<a href="mailto:przanoni@gmail.com">przanoni@gmail.com</a>> wrote:<br>
>><br>
>> From: Paulo Zanoni <<a href="mailto:paulo.r.zanoni@intel.com">paulo.r.zanoni@intel.com</a>><br>
>><br>
>> It is possible that, by the time we run i915_drm_freeze(),<br>
>> delayed_resume_work was already queued but did not run yet. If it<br>
>> still didn't run after intel_runtime_pm_disable_interrupts(), by the<br>
>> time it runs it will try to change the interrupt registers with the<br>
>> interrupts already disabled, which will trigger a WARN. We can<br>
>> reliably reproduce this with the pm_rpm system-suspend test case.<br>
>><br>
>> In order to avoid the problem, we have to flush the work before<br>
>> disabling the interrupts. We could also cancel the work instead of<br>
>> flushing it, but that would require us to put a runtime PM reference -<br>
>> and any other resource we may need in the future - in case the work<br>
>> was already queued, so I believe flushing the work is more<br>
>> future-proof, although less efficient. But I can also change this part<br>
>> if someone requests.<br>
>><br>
>> Another thing I tried was to move the intel_suspend_gt_powersave()<br>
>> call to before intel_runtime_pm_disable_interrupts(), but since that<br>
>> function needs to be called after the interrupts are already disabled,<br>
>> due to dev_priv->rps.work, this strategy didn't work.<br>
>><br>
>> Testcase: igt/pm_rpm/system-suspend<br>
>> Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=80517" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=80517</a><br>
>> Signed-off-by: Paulo Zanoni <<a href="mailto:paulo.r.zanoni@intel.com">paulo.r.zanoni@intel.com</a>><br>
>> ---<br>
>>  drivers/gpu/drm/i915/i915_drv.c | 2 ++<br>
>>  1 file changed, 2 insertions(+)<br>
>><br>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c<br>
>> b/drivers/gpu/drm/i915/i915_drv.c<br>
>> index e64547e..672694b 100644<br>
>> --- a/drivers/gpu/drm/i915/i915_drv.c<br>
>> +++ b/drivers/gpu/drm/i915/i915_drv.c<br>
>> @@ -524,6 +524,8 @@ static int i915_drm_freeze(struct drm_device *dev)<br>
>>                         return error;<br>
>>                 }<br>
>><br>
>> +               flush_delayed_work(&dev_priv->rps.delayed_resume_work);<br>
>> +<br>
>>                 intel_runtime_pm_disable_interrupts(dev);<br>
>>                 dev_priv->enable_hotplug_processing = false;<br>
>><br>
>> --<br>
>> 2.0.0<br>
>><br>
>> _______________________________________________<br>
>> Intel-gfx mailing list<br>
>> <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a><br>
>> <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a><br>
><br>
><br>
><br>
><br>
> --<br>
> Rodrigo Vivi<br>
> Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a><br>
><br>
<br>
<br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Paulo Zanoni<br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div><div> </div>
</div></div>