drm/i915: VGA also requires the power well
Subsystems (2)
PATCH 1/1 drm/i915: VGA also requires the power well
From: Paulo Zanoni <[email protected]>
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.
--- 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(+)
drivers/gpu/drm/i915/i915_drv.h +1 -0
@@ -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 ,
drivers/gpu/drm/i915/intel_display.c +3 -0
@@ -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);
drivers/gpu/drm/i915/intel_pm.c +1 -0
@@ -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 :
Signature · 1 line
1.8.1.2
Quoted 31 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- 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(+)drivers/gpu/drm/i915/i915_drv.h +1
-0@@ -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 ,drivers/gpu/drm/i915/intel_display.c +0
-0
drivers/gpu/drm/i915/intel_display.c void i915_redisable_vga(struct drm_device *dev) · line 9955
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c +3
-0@@ -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?
Quoted 22 lines
drivers/gpu/drm/i915/intel_display.c +0
-0if (I915_READ (vga_reg) !=VGA_DISP_DISABLE ) {DRM_DEBUG_KMS ("Something enabled VGA plane, disabling it\n" );i915_disable_vga (dev);drivers/gpu/drm/i915/intel_pm.c +1
-0@@ -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 :Signature · 6 lines
1.8.1.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 2 lines
Ville Syrjälä Intel OTC
2013/6/6 Ville Syrjälä <[email protected]>:
Quoted 35 lines · Paulo Zanoni
Quoted 34 lines
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- 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(+)drivers/gpu/drm/i915/i915_drv.h +1
-0@@ -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 ,drivers/gpu/drm/i915/intel_display.c +0
-0@@ -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);
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c void i915_redisable_vga(struct drm_device *dev) · line 9955
Quoted 3 lines
drivers/gpu/drm/i915/intel_display.c +3
-0+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.
Quoted 1 line
Shouldn't we instead enable the power well during resume?
So far we don't need it. I hope it stays this way.
Quoted 27 lines
Quoted 19 lines
drivers/gpu/drm/i915/intel_display.c +0
-0if (I915_READ (vga_reg) !=VGA_DISP_DISABLE ) {DRM_DEBUG_KMS ("Something enabled VGA plane, disabling it\n" );i915_disable_vga (dev);drivers/gpu/drm/i915/intel_pm.c +1
-1@@ -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
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Ville Syrjälä
Intel OTC
Signature · 1 line
Paulo Zanoni
Quoted 49 lines · Paulo Zanoni
2013/6/6 Ville Syrjälä <[email protected]>:
Quoted 35 lines · Paulo Zanoni
Quoted 34 lines
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- 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(+)drivers/gpu/drm/i915/i915_drv.h +1
-0@@ -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 ,drivers/gpu/drm/i915/intel_display.c +0
-0@@ -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);Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c void i915_redisable_vga(struct drm_device *dev) · line 9955
Quoted 3 lines
drivers/gpu/drm/i915/intel_display.c +3
-0+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.
Quoted 3 lines
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.
Signature · 2 lines
Ville Syrjälä Intel OTC
2013/6/6 Ville Syrjälä <[email protected]>:
Quoted 50 lines · Paulo Zanoni
Quoted 49 lines
2013/6/6 Ville Syrjälä <[email protected]>:
Quoted 35 lines · Paulo Zanoni
Quoted 34 lines
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- 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(+)drivers/gpu/drm/i915/i915_drv.h +1
-0@@ -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 ,drivers/gpu/drm/i915/intel_display.c +0
-0@@ -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);Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c void i915_redisable_vga(struct drm_device *dev) · line 9955
Quoted 3 lines
drivers/gpu/drm/i915/intel_display.c +3
-0+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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 4 lines
--
Ville Syrjälä
Intel OTC
Signature · 1 line
Paulo Zanoni
2013/6/6 Ville Syrjälä <[email protected]>:
Quoted 35 lines · Paulo Zanoni
Quoted 34 lines
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- 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(+)drivers/gpu/drm/i915/i915_drv.h +1
-0@@ -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 ,drivers/gpu/drm/i915/intel_display.c +0
-0@@ -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);
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c void i915_redisable_vga(struct drm_device *dev) · line 9955
Quoted 3 lines
drivers/gpu/drm/i915/intel_display.c +3
-0+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...
Quoted 27 lines
Quoted 19 lines
drivers/gpu/drm/i915/intel_display.c +0
-0if (I915_READ (vga_reg) !=VGA_DISP_DISABLE ) {DRM_DEBUG_KMS ("Something enabled VGA plane, disabling it\n" );i915_disable_vga (dev);drivers/gpu/drm/i915/intel_pm.c +1
-1@@ -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
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Ville Syrjälä
Intel OTC
Signature · 1 line
Paulo Zanoni
Quoted 36 lines · Paulo Zanoni
2013/6/6 Ville Syrjälä <[email protected]>:
Quoted 35 lines · Paulo Zanoni
Quoted 34 lines
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- 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(+)drivers/gpu/drm/i915/i915_drv.h +1
-0@@ -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 ,drivers/gpu/drm/i915/intel_display.c +0
-0@@ -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);
Quoted 8 lines
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c void i915_redisable_vga(struct drm_device *dev) · line 9955
Quoted 3 lines
drivers/gpu/drm/i915/intel_display.c +3
-0+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
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 53 lines
-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Sunday, August 04, 2013 4:50 PM
To: Paulo Zanoni
Cc: Ville Syrjälä; [email protected]; Zanoni, Paulo R
Subject: Re: [Intel-gfx] [PATCH] drm/i915: VGA also requires the power wellQuoted 8 lines · Paulo Zanoni
2013/6/6 Ville Syrjälä <[email protected]>:
Quoted 7 lines · Paulo Zanoni
Quoted 6 lines
From: Paulo Zanoni <[email protected]>
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.
Quoted 9 lines
Quoted 9 lines
Review
Quoted 9 lines
Signed-off-by Paulo Zanoni <[email protected]>--- 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(+)(quoted diff) +0
-0
b/drivers/gpu/drm/i915/i915_drv.hQuoted 6 lines
Quoted 6 lines
Quoted 6 lines
index 46b1f70..d51ce13 100644
drivers/gpu/drm/i915/i915_drv.h +0
-0@@ -89,6 +89,7 @@ enum port {#define port_name (p) ((p) + 'A')Quoted 6 lines
Quoted 6 lines
Review
Quoted 6 lines
drivers/gpu/drm/i915/i915_drv.h +1
-0enum intel_display_power_domain { +POWER_DOMAIN_VGA ,POWER_DOMAIN_PIPE_A ,POWER_DOMAIN_PIPE_B ,POWER_DOMAIN_PIPE_C ,(quoted diff) +0
-0
b/drivers/gpu/drm/i915/intel_display.cQuoted 4 lines
Quoted 4 lines
drivers/gpu/drm/i915/intel_display.c void i915_redisable_vga(struct drm_device
Quoted 4 lines
index 4c8fcec..3719d99 100644
drivers/gpu/drm/i915/intel_display.c +0
-0@@ -9950,6 +9950,9 @@ void i915_redisable_vga(struct drm_device*dev)
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c +3
-0struct 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 ; +
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 5 lines
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 62 lines · Paulo R
Quoted 53 lines
-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Sunday, August 04, 2013 4:50 PM
To: Paulo Zanoni
Cc: Ville Syrjälä; [email protected]; Zanoni, Paulo R
Subject: Re: [Intel-gfx] [PATCH] drm/i915: VGA also requires the power wellQuoted 8 lines · Paulo Zanoni
2013/6/6 Ville Syrjälä <[email protected]>:
Quoted 7 lines · Paulo Zanoni
Quoted 6 lines
From: Paulo Zanoni <[email protected]>
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.
Quoted 9 lines
Quoted 9 lines
Review
Quoted 9 lines
Signed-off-by Paulo Zanoni <[email protected]>--- 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(+)(quoted diff) +0
-0
b/drivers/gpu/drm/i915/i915_drv.hQuoted 6 lines
Quoted 6 lines
Quoted 6 lines
index 46b1f70..d51ce13 100644
drivers/gpu/drm/i915/i915_drv.h +0
-0@@ -89,6 +89,7 @@ enum port {#define port_name (p) ((p) + 'A')Quoted 6 lines
Quoted 6 lines
Review
Quoted 6 lines
drivers/gpu/drm/i915/i915_drv.h +1
-0enum intel_display_power_domain { +POWER_DOMAIN_VGA ,POWER_DOMAIN_PIPE_A ,POWER_DOMAIN_PIPE_B ,POWER_DOMAIN_PIPE_C ,(quoted diff) +0
-0
b/drivers/gpu/drm/i915/intel_display.cQuoted 4 lines
Quoted 4 lines
drivers/gpu/drm/i915/intel_display.c void i915_redisable_vga(struct drm_device
Quoted 4 lines
index 4c8fcec..3719d99 100644
drivers/gpu/drm/i915/intel_display.c +0
-0@@ -9950,6 +9950,9 @@ void i915_redisable_vga(struct drm_device*dev)
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c +3
-0struct 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 ; +Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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?
Quoted 6 lines
I don't think so. If you look at
i915_redisable_vgaand commit0fde901f1ddd2ce0e380a6444f1fb7ca555859e9, 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
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch