<div dir="ltr">On Tue, Jul 16, 2013 at 10:31 AM, Daniel Vetter <span dir="ltr"><<a href="mailto:daniel@ffwll.ch" target="_blank">daniel@ffwll.ch</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Sun, Jul 14, 2013 at 09:56:45PM +0400, Konstantin Khlebnikov wrote:<br>
> Daniel Vetter wrote:<br>
> >On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov<br>
> ><<a href="mailto:khlebnikov@openvz.org">khlebnikov@openvz.org</a>>  wrote:<br>
> >>This patch fixes regression in power consumtion of sandy bridge gpu, which<br>
> >>exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that<br>
> >>it's extremely busy. After that it never reaches rc6 state.<br>
> >><br>
> >>Bug was introduce by commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0<br>
> >>("drm/i915: load boot context at driver init time"). Without documentation<br>
> >>it's not clear what is happening here, probably this breaks internal state of<br>
> >>hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake<br>
> >>during whole initialization sequence in gen6_init_clock_gating() fixes this bug.<br>
> >><br>
> >>References: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=54089" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=54089</a><br>
> >>Signed-off-by: Konstantin Khlebnikov<<a href="mailto:khlebnikov@openvz.org">khlebnikov@openvz.org</a>><br>
> ><br>
> >We already hold an forcewake reference while setting up the rps stuff,<br>
> >should we maybe hold the forcewake for the entire duration, i.e. grab<br>
> >it here in clock_gating and release it only in gen6/vlv_enable_rps?<br>
> >Can you please test that version, too?<br>
><br>
> This will be racy because rps stuff is done in separate work which might be canceled<br>
> if intel_disable_gt_powersave() happens before its completion.<br>
<br>
</div>Can be fixed with a flush_delayed_work. And since that has the same<br>
requirements wrt locking to prevent deadlocks as cancel_work_sync it would<br>
be a drop-in replacement. Can I volunteer you to look into testing that<br>
out a bit? Otherwise I could volunteer someone from our team.<br>
<br>
In any case I think we should apply this trick to all platforms where<br>
we've added the MBCTL write (i.e. snb, ivb, hsw & vlv) since rps/rc6 works<br>
_very_ similar on all of those.<br></blockquote><div><br></div><div>I've tested that patch and it really works for me. If you want change something for other hardware or</div><div>extend range where forcewake is held prease do it in a separate patch.</div>
<div>This will be good for bisecting new bugs in the future.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks, Daniel<br>
<div class="HOEnZb"><div class="h5">--<br>
Daniel Vetter<br>
Software Engineer, Intel Corporation<br>
+41 (0) 79 365 57 48 - <a href="http://blog.ffwll.ch" target="_blank">http://blog.ffwll.ch</a><br>
</div></div></blockquote><br></div>