Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Konstantin Khlebnikov <khlebnikov@openvz.org>,
	David Airlie <airlied@linux.ie>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
Date: Sun, 14 Jul 2013 22:48:24 +0100	[thread overview]
Message-ID: <20130714214824.GB31828@cantiga.alporthouse.com> (raw)
In-Reply-To: <CAKMK7uEtReZxvzJj9LsSXEJEBNiK492CCONdsF5VFK2cA3u0FA@mail.gmail.com>

On Sun, Jul 14, 2013 at 06:52:39PM +0200, Daniel Vetter wrote:
> On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov
> <khlebnikov@openvz.org> wrote:
> > This patch fixes regression in power consumtion of sandy bridge gpu, which
> > exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that
> > it's extremely busy. After that it never reaches rc6 state.
> >
> > Bug was introduce by commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0
> > ("drm/i915: load boot context at driver init time"). Without documentation
> > it's not clear what is happening here, probably this breaks internal state of
> > hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake
> > during whole initialization sequence in gen6_init_clock_gating() fixes this bug.
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
> > Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> 
> We already hold an forcewake reference while setting up the rps stuff,
> should we maybe hold the forcewake for the entire duration, i.e. grab
> it here in clock_gating and release it only in gen6/vlv_enable_rps?
> Can you please test that version, too?
> 
> In any case the forcewake grabbing here in the clock gating function
> needs a big comment that otherwise setting the MCTL register might
> break rc6 entry.

It is not clear why the forcewake works, but is easy to imagine one of
the operations in that sequence requires the GPU to be awake at the time
of programming for it to succeed. MBCTL:EnableBootFetch does seem the most
suspicious from its wording in the bspec. I guess all instances of
poking this bit should be protected similary (snb, ivb, vlv, hsw).

Based on that reasoning and that waking the GPU up here has no negative
consequences, and so long as all paths are fixed, I am happy to give this
an Acked-by: Chris Wilson <chris@chris-wilson.co.uk>

Also, we need to reapply the w/a after a Function Level Reset, in other
words we do need to repeat the init_clock_gating after
intel_gpu_reset().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre