From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: VGA also requires the power well Date: Wed, 14 Aug 2013 16:41:36 +0200 Message-ID: <20130814144136.GV9296@phenom.ffwll.local> References: <1370466351-3683-1-git-send-email-przanoni@gmail.com> <20130606083804.GM5004@intel.com> <20130804194935.GB22035@phenom.ffwll.local> <2A60747E125A1447BDEB64729260025E25A8C87D@FMSMSX104.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ea0-f169.google.com (mail-ea0-f169.google.com [209.85.215.169]) by gabe.freedesktop.org (Postfix) with ESMTP id 7F533E5C7B for ; Wed, 14 Aug 2013 07:41:29 -0700 (PDT) Received: by mail-ea0-f169.google.com with SMTP id z7so4858627eaf.14 for ; Wed, 14 Aug 2013 07:41:28 -0700 (PDT) Content-Disposition: inline In-Reply-To: <2A60747E125A1447BDEB64729260025E25A8C87D@FMSMSX104.amr.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: "Zanoni, Paulo R" Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Mon, Aug 12, 2013 at 06:06:48PM +0000, Zanoni, Paulo R wrote: > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > > Vetter > > Sent: Sunday, August 04, 2013 4:50 PM > > To: Paulo Zanoni > > Cc: Ville Syrj=E4l=E4; intel-gfx@lists.freedesktop.org; Zanoni, Paulo R > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: VGA also requires the power = well > > = > > On Fri, Aug 02, 2013 at 02:17:53PM -0300, Paulo Zanoni wrote: > > > 2013/6/6 Ville Syrj=E4l=E4 : > > > > On Wed, Jun 05, 2013 at 06:05:51PM -0300, Paulo Zanoni wrote: > > > >> From: Paulo Zanoni > > > >> > > > >> So add a power domain and check for it before we try to read > > > >> VGA_CONTROL. > > > >> > > > >> This fixes unclaimed register messages that happen on > > suspend/resume. > > > >> > > > >> Signed-off-by: Paulo Zanoni > > > >> --- > > > >> drivers/gpu/drm/i915/i915_drv.h | 1 + > > > >> drivers/gpu/drm/i915/intel_display.c | 3 +++ > > > >> drivers/gpu/drm/i915/intel_pm.c | 1 + > > > >> 3 files changed, 5 insertions(+) > > > >> > > > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > > >> index 46b1f70..d51ce13 100644 > > > >> --- a/drivers/gpu/drm/i915/i915_drv.h > > > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > > > >> @@ -89,6 +89,7 @@ enum port { > > > >> #define port_name(p) ((p) + 'A') > > > >> > > > >> enum intel_display_power_domain { > > > >> + POWER_DOMAIN_VGA, > > > >> POWER_DOMAIN_PIPE_A, > > > >> POWER_DOMAIN_PIPE_B, > > > >> POWER_DOMAIN_PIPE_C, > > > >> diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > > >> index 4c8fcec..3719d99 100644 > > > >> --- a/drivers/gpu/drm/i915/intel_display.c > > > >> +++ b/drivers/gpu/drm/i915/intel_display.c > > > >> @@ -9950,6 +9950,9 @@ void i915_redisable_vga(struct drm_device > > *dev) > > > >> struct drm_i915_private *dev_priv =3D dev->dev_private; > > > >> u32 vga_reg =3D i915_vgacntrl_reg(dev); > > > >> > > > >> + if (!intel_display_power_enabled(dev, POWER_DOMAIN_VGA)) > > > >> + return; > > > >> + > > > > > > > > So it looks like you're essentially making intel_redisable_vga() a = nop > > > > for HSW. Shouldn't we instead enable the power well during resume? > > > > > > We enable the power well during resume, but it's only after this func= tion... > > = > > Hm, so better move the (temporary) power well enabling in the resume > > code > > up a bit to cover this? > = > I don't think so. If you look at i915_redisable_vga and commit > 0fde901f1ddd2ce0e380a6444f1fb7ca555859e9, you will start thinking that > just closing/opening the lid could make the BIOS somehow enable the > power well and then reenable VGA, so moving the check to "earlier in the > resume sequence" won't solve any problems, as VGA could be reenabled > later. = Well that's only for machines without opregion afaik. But we lack the check for that, which I didn't really realize. To avoid blocking this even longer I've just merged this patch here with a big note added to the commit message. Thanks, Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch