* [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
> -----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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.