Re: [PATCH] drm/i915: VGA also requires the power well

[Daniel Vetter <daniel@ffwll.ch>]

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älä; 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älä <ville.syrjala@linux.intel.com>:
> > > > On Wed, Jun 05, 2013 at 06:05:51PM -0300, Paulo Zanoni wrote:
> > > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > >>
> > > >> 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 <paulo.r.zanoni@intel.com>
> > > >> ---
> > > >>  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 = dev->dev_private;
> > > >>       u32 vga_reg = 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 function...
> > 
> > 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