* [PATCH] drm/i915: VGA also requires the power well @ 2013-06-05 21:05 Paulo Zanoni 2013-06-06 8:38 ` Ville Syrjälä 0 siblings, 1 reply; 9+ messages in thread From: Paulo Zanoni @ 2013-06-05 21:05 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni 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; + if (I915_READ(vga_reg) != VGA_DISP_DISABLE) { DRM_DEBUG_KMS("Something enabled VGA plane, disabling it\n"); i915_disable_vga(dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 50fe3d7..47ef4a6 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5000,6 +5000,7 @@ bool intel_display_power_enabled(struct drm_device *dev, case POWER_DOMAIN_PIPE_A: case POWER_DOMAIN_TRANSCODER_EDP: return true; + case POWER_DOMAIN_VGA: case POWER_DOMAIN_PIPE_B: case POWER_DOMAIN_PIPE_C: case POWER_DOMAIN_PIPE_A_PANEL_FITTER: -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: VGA also requires the power well 2013-06-05 21:05 [PATCH] drm/i915: VGA also requires the power well Paulo Zanoni @ 2013-06-06 8:38 ` Ville Syrjälä 2013-06-06 14:35 ` Paulo Zanoni 2013-08-02 17:17 ` Paulo Zanoni 0 siblings, 2 replies; 9+ messages in thread From: Ville Syrjälä @ 2013-06-06 8:38 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni 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? > if (I915_READ(vga_reg) != VGA_DISP_DISABLE) { > DRM_DEBUG_KMS("Something enabled VGA plane, disabling it\n"); > i915_disable_vga(dev); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 50fe3d7..47ef4a6 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5000,6 +5000,7 @@ bool intel_display_power_enabled(struct drm_device *dev, > case POWER_DOMAIN_PIPE_A: > case POWER_DOMAIN_TRANSCODER_EDP: > return true; > + case POWER_DOMAIN_VGA: > case POWER_DOMAIN_PIPE_B: > case POWER_DOMAIN_PIPE_C: > case POWER_DOMAIN_PIPE_A_PANEL_FITTER: > -- > 1.8.1.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: VGA also requires the power well 2013-06-06 8:38 ` Ville Syrjälä @ 2013-06-06 14:35 ` Paulo Zanoni 2013-06-06 14:47 ` Ville Syrjälä 2013-08-02 17:17 ` Paulo Zanoni 1 sibling, 1 reply; 9+ messages in thread From: Paulo Zanoni @ 2013-06-06 14:35 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, Paulo Zanoni 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. It's not a nop for HSW, it's only a nop if the power well is disabled, which means VGA is disabled, so it's a nop if VGA is disabled. But if you look at the current function it's also a nop if VGA is disabled. So we're keeping the same behavior, but checking the power well before checking vga_reg. VGA mode requires the power well to be enabled, we can be sure that if the power well is disabled, then VGA is disabled, so you don't need to do the check for VGA_DISP_DISABLE. > Shouldn't we instead enable the power well during resume? So far we don't need it. I hope it stays this way. > >> if (I915_READ(vga_reg) != VGA_DISP_DISABLE) { >> DRM_DEBUG_KMS("Something enabled VGA plane, disabling it\n"); >> i915_disable_vga(dev); >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 50fe3d7..47ef4a6 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -5000,6 +5000,7 @@ bool intel_display_power_enabled(struct drm_device *dev, >> case POWER_DOMAIN_PIPE_A: >> case POWER_DOMAIN_TRANSCODER_EDP: >> return true; >> + case POWER_DOMAIN_VGA: >> case POWER_DOMAIN_PIPE_B: >> case POWER_DOMAIN_PIPE_C: >> case POWER_DOMAIN_PIPE_A_PANEL_FITTER: >> -- >> 1.8.1.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC -- Paulo Zanoni ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: VGA also requires the power well 2013-06-06 14:35 ` Paulo Zanoni @ 2013-06-06 14:47 ` Ville Syrjälä 2013-08-02 17:17 ` Paulo Zanoni 0 siblings, 1 reply; 9+ messages in thread From: Ville Syrjälä @ 2013-06-06 14:47 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni On Thu, Jun 06, 2013 at 11:35:15AM -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. > > It's not a nop for HSW, it's only a nop if the power well is disabled, > which means VGA is disabled, so it's a nop if VGA is disabled. But if > you look at the current function it's also a nop if VGA is disabled. > So we're keeping the same behavior, but checking the power well before > checking vga_reg. > > VGA mode requires the power well to be enabled, we can be sure that if > the power well is disabled, then VGA is disabled, so you don't need to > do the check for VGA_DISP_DISABLE. Rigth, but intel_display_power_enabled() only checks the driver power well register. If BIOS can leave VGA enabled, then I guess it could've left the power well on too. So I'm thinking we should check for that rather than the what the driver requested. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: VGA also requires the power well 2013-06-06 14:47 ` Ville Syrjälä @ 2013-08-02 17:17 ` Paulo Zanoni 0 siblings, 0 replies; 9+ messages in thread From: Paulo Zanoni @ 2013-08-02 17:17 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, Paulo Zanoni 2013/6/6 Ville Syrjälä <ville.syrjala@linux.intel.com>: > On Thu, Jun 06, 2013 at 11:35:15AM -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. >> >> It's not a nop for HSW, it's only a nop if the power well is disabled, >> which means VGA is disabled, so it's a nop if VGA is disabled. But if >> you look at the current function it's also a nop if VGA is disabled. >> So we're keeping the same behavior, but checking the power well before >> checking vga_reg. >> >> VGA mode requires the power well to be enabled, we can be sure that if >> the power well is disabled, then VGA is disabled, so you don't need to >> do the check for VGA_DISP_DISABLE. > > Rigth, but intel_display_power_enabled() only checks the driver power > well register. If BIOS can leave VGA enabled, then I guess it could've > left the power well on too. So I'm thinking we should check for that > rather than the what the driver requested. Oh, right. I guess you found one case where our "if we don't want the power well enabled then we don' touch its registers" policy is not enough. It's kinda hard to deal with this "something might just re-enable VGA while you're distracted" case. There are many possible racing conditions, even more if you add the power well. And this case + the case on the PSR patches are examples that our "power domains" abstraction doesn't seem to be a fit-all approach. > > -- > Ville Syrjälä > Intel OTC -- Paulo Zanoni ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: VGA also requires the power well 2013-06-06 8:38 ` Ville Syrjälä 2013-06-06 14:35 ` Paulo Zanoni @ 2013-08-02 17:17 ` Paulo Zanoni 2013-08-04 19:49 ` Daniel Vetter 1 sibling, 1 reply; 9+ messages in thread From: Paulo Zanoni @ 2013-08-02 17:17 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, Paulo Zanoni 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... > >> if (I915_READ(vga_reg) != VGA_DISP_DISABLE) { >> DRM_DEBUG_KMS("Something enabled VGA plane, disabling it\n"); >> i915_disable_vga(dev); >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 50fe3d7..47ef4a6 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -5000,6 +5000,7 @@ bool intel_display_power_enabled(struct drm_device *dev, >> case POWER_DOMAIN_PIPE_A: >> case POWER_DOMAIN_TRANSCODER_EDP: >> return true; >> + case POWER_DOMAIN_VGA: >> case POWER_DOMAIN_PIPE_B: >> case POWER_DOMAIN_PIPE_C: >> case POWER_DOMAIN_PIPE_A_PANEL_FITTER: >> -- >> 1.8.1.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC -- Paulo Zanoni ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: VGA also requires the power well 2013-08-02 17:17 ` Paulo Zanoni @ 2013-08-04 19:49 ` Daniel Vetter 2013-08-12 18:06 ` Zanoni, Paulo R 0 siblings, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2013-08-04 19:49 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni 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? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: VGA also requires the power well 2013-08-04 19:49 ` Daniel Vetter @ 2013-08-12 18:06 ` Zanoni, Paulo R 2013-08-14 14:41 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Zanoni, Paulo R @ 2013-08-12 18:06 UTC (permalink / raw) To: Daniel Vetter, Paulo Zanoni; +Cc: intel-gfx@lists.freedesktop.org > -----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. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: VGA also requires the power well 2013-08-12 18:06 ` Zanoni, Paulo R @ 2013-08-14 14:41 ` Daniel Vetter 0 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2013-08-14 14:41 UTC (permalink / raw) To: Zanoni, Paulo R; +Cc: 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ä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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-08-14 14:41 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-06-05 21:05 [PATCH] drm/i915: VGA also requires the power well Paulo Zanoni 2013-06-06 8:38 ` Ville Syrjälä 2013-06-06 14:35 ` Paulo Zanoni 2013-06-06 14:47 ` Ville Syrjälä 2013-08-02 17:17 ` Paulo Zanoni 2013-08-02 17:17 ` Paulo Zanoni 2013-08-04 19:49 ` Daniel Vetter 2013-08-12 18:06 ` Zanoni, Paulo R 2013-08-14 14:41 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox