Enable runtime PM with DPMS on HSW
Subsystems (2)
cover Enable runtime PM with DPMS on HSW
From: Paulo Zanoni <[email protected]>
Hi
Daniel's patch series for this series still has 27 patches to be merged. Since we reached a point on the code review where we have some disagreements over the coding style, I think it is a better idea to allow people to start using the bikeshed while we're still arguing over which color we should paint it.
I want to make it clear that I am _not_ saying we should skip/discard Daniel's patches. We should still continue discussing them, but since they don't really affect the feature we need to merge, we can merge the feature earlier and unblock the people depending on it. This will also give QA more time to test stuff before the next Kernel version, and it will also make it easier for people wanting to backport our patches, since they will have 27 less patches to backport.
I tested these patches on top of -nightly. They pass IGT's pm_rpm test, and the only Kernel WARN I see when running it is also present without these patches.
Thanks,
Paulo
Daniel Vetter (1):
drm/i915: runtime PM support for DPMS
Paulo Zanoni (1):
drm/i915: move HSW DDI pll enabling to haswell_crtc_enable
drivers/gpu/drm/i915/intel_display.c | 41 ++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)
Signature · 1 line
1.9.0
PATCH 1/2 drm/i915: move HSW DDI pll enabling to haswell_crtc_enable
From: Paulo Zanoni <[email protected]>
Now that we moved all the register writing to haswell_crtc_enable, we can also move the PLL enabling there. This is the last step to do before allowing runtime PM on DPMS.
Daniel has some patches to do this while also moving all the HSW PLL code to the shared DPLL structs, but while we still discuss the color of those patches, we can apply this quick one and at least get runtime PM on DPMS running in the meantime.
--- drivers/gpu/drm/i915/intel_display.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_display.c +4 -2
@@ -4052,6 +4052,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)if (intel_crtc->active)return ; +intel_ddi_pll_enable (intel_crtc); +if (intel_crtc->config.has_dp_encoder)intel_dp_set_m_n (intel_crtc);
@@ -4243,6 +4245,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)intel_update_fbc (dev);intel_edp_psr_update (dev);mutex_unlock (&dev->struct_mutex); + +intel_ddi_put_crtc_pll (crtc); }static void ironlake_crtc_off (struct drm_crtc *crtc)
@@ -4253,7 +4257,6 @@ static void ironlake_crtc_off(struct drm_crtc *crtc)static void haswell_crtc_off (struct drm_crtc *crtc) { -intel_ddi_put_crtc_pll (crtc); }static void i9xx_pfit_enable (struct intel_crtc *crtc)
@@ -7471,7 +7474,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,if (!intel_ddi_pll_select (intel_crtc))return -EINVAL ; -intel_ddi_pll_enable (intel_crtc); intel_crtc->lowfreq_avail = false;
Signature · 1 line
1.9.0
Re: [PATCH 1/2] drm/i915: move HSW DDI pll enabling to haswell_crtc_enable
Quoted 6 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Now that we moved all the register writing to
haswell_crtc_enable, we can also move the PLL enabling there. This is the last step to do before allowing runtime PM on DPMS.
Quoted 6 lines
Daniel has some patches to do this while also moving all the HSW PLL code to the shared DPLL structs, but while we still discuss the color of those patches, we can apply this quick one and at least get runtime PM on DPMS running in the meantime.
Signed-off-by Paulo Zanoni <[email protected]>
Nope. If you want to do this you need 2 refcounts per shared dpll:
- One that tracks whether a crtc has a mode set that needs a given shared dpll, whether the crtc is actually enabled at the hw level or no. You need to increment that refcount in ->crtc_mode_set and decrement it in ->crtc_off or ->crtc_mode_set.
- A 2nd refcount which tracks active usage and gets incremented in ->crtc_enable and decremented in ->crtc_disable.
This need is the entire point of the shared dpll infrastructure. I haven't tested you patch on actual hw but from reading through it you still keep that single refcount and increment it in crtc_mode_set and decrement it in crtc_disable. That means:
- Multiple dpms off/on cycles will underrun the refcount since it's unbalanced.
- Sharing is broken since if you dpms off a crtc its pll can be stolen and reused, which means upon dpms on you get a completely wrong pll setting.
I think the kms_flip/*dpms* and kms_flip/2x*dpms* tests should be able to hit this. Wrt testing please also note that even for just testing runtime pm running only pm_rpm isn't good enough - some of the new rpm tests I've added are subtests of kms_flip (since I wanted to test vblanks and similar stuff). So you really need to use piglits filtering to get all the rpm tests.
Cheers, Daniel
Quoted 49 lines
--- drivers/gpu/drm/i915/intel_display.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/intel_display.c +4
-3@@ -4052,6 +4052,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)if (intel_crtc->active)return ; +intel_ddi_pll_enable (intel_crtc); +if (intel_crtc->config.has_dp_encoder)intel_dp_set_m_n (intel_crtc);@@ -4243,6 +4245,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)intel_update_fbc (dev);intel_edp_psr_update (dev);mutex_unlock (&dev->struct_mutex); + +intel_ddi_put_crtc_pll (crtc); }static void ironlake_crtc_off (struct drm_crtc *crtc)@@ -4253,7 +4257,6 @@ static void ironlake_crtc_off(struct drm_crtc *crtc)static void haswell_crtc_off (struct drm_crtc *crtc) { -intel_ddi_put_crtc_pll (crtc); }static void i9xx_pfit_enable (struct intel_crtc *crtc)@@ -7471,7 +7474,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,if (!intel_ddi_pll_select (intel_crtc))return -EINVAL ; -intel_ddi_pll_enable (intel_crtc); intel_crtc->lowfreq_avail = false; --1.9.0
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH 2/2 drm/i915: runtime PM support for DPMS
In reply to Paulo ZanoniFrom: Daniel Vetter <[email protected]>
Keeping track of the power domains is a bit messy since crtc->active is currently updated by the platform hooks, but we need to be aware of which state transition exactly is going on. Maybe we simply need to shovel all the power domain handling down into platform code to simplify this. But doing that requires some more auditing since currently the ->mode_set callbacks still read some random registers (to e.g. figure out the reference clocks).
Also note that intel_crtc_update_dpms is always call first/last even for encoders which have their own dpms functions. Hence we really only need to update this place here.
Being a quick "does it blow up?" run not really tested yet.
v2, made by Paulo:
- IVB and older are not ready yet for this.
--- drivers/gpu/drm/i915/intel_display.c | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/intel_display.c +31 -4
@@ -4845,16 +4845,43 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc) {struct drm_device *dev = crtc->dev;struct drm_i915_private *dev_priv = dev->dev_private; +struct intel_crtc *intel_crtc =to_intel_crtc (crtc);struct intel_encoder *intel_encoder; +enum intel_display_power_domain domain; +unsigned long domains;bool enable = false;for_each_encoder_on_crtc (dev, crtc, intel_encoder) enable |= intel_encoder->connectors_active; -if (enable) - dev_priv->display.crtc_enable (crtc); -else - dev_priv->display.crtc_disable (crtc); +if (enable) { +if (!intel_crtc->active) { + +/* IVB and older still don't support RPM on DPMS. */ +if (IS_HASWELL (dev) ||INTEL_INFO (dev)->gen >=8 ) { + domains =get_crtc_power_domains (crtc); +for_each_power_domain (domain, domains) +intel_display_power_get (dev_priv, + domain); + intel_crtc->enabled_power_domains = domains; + } + + dev_priv->display.crtc_enable (crtc); + } + }else { +if (intel_crtc->active) { + dev_priv->display.crtc_disable (crtc); + +/* IVB and older still don't support RPM on DPMS. */ +if (IS_HASWELL (dev) ||INTEL_INFO (dev)->gen >=8 ) { + domains = intel_crtc->enabled_power_domains; +for_each_power_domain (domain, domains) +intel_display_power_put (dev_priv, + domain); + intel_crtc->enabled_power_domains =0 ; + } + } + }intel_crtc_update_sarea (crtc, enable); }
Signature · 1 line
1.9.0
Re: [PATCH 2/2] drm/i915: runtime PM support for DPMS
Quoted 10 lines · Paulo Zanoni
From: Daniel Vetter <[email protected]>
Keeping track of the power domains is a bit messy since
crtc->activeis currently updated by the platform hooks, but we need to be aware of which state transition exactly is going on. Maybe we simply need to shovel all the power domain handling down into platform code to simplify this. But doing that requires some more auditing since currently the ->mode_set callbacks still read some random registers (to e.g. figure out the reference clocks).
Quoted 8 lines
Also note that
intel_crtc_update_dpmsis always call first/last even for encoders which have their own dpms functions. Hence we really only need to update this place here.Being a quick "does it blow up?" run not really tested yet.
v2, made by Paulo:
- IVB and older are not ready yet for this.
I've actually tested this on snb. Not yet tested on byt (due to lack of hw on my side), but it should also work (as long as the normal runtime pm works).
So if you want to unblock runtime pm support for dpms I think this patch is good, but instead we should disallow runtime pm for HAS_DDI until the wrpll code can cope.
-Daniel
Quoted 62 lines
Signed-off-by Daniel Vetter <[email protected]>Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-)drivers/gpu/drm/i915/intel_display.c +31
-5@@ -4845,16 +4845,43 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc) {struct drm_device *dev = crtc->dev;struct drm_i915_private *dev_priv = dev->dev_private; +struct intel_crtc *intel_crtc =to_intel_crtc (crtc);struct intel_encoder *intel_encoder; +enum intel_display_power_domain domain; +unsigned long domains;bool enable = false;for_each_encoder_on_crtc (dev, crtc, intel_encoder) enable |= intel_encoder->connectors_active; -if (enable) - dev_priv->display.crtc_enable (crtc); -else - dev_priv->display.crtc_disable (crtc); +if (enable) { +if (!intel_crtc->active) { + +/* IVB and older still don't support RPM on DPMS. */ +if (IS_HASWELL (dev) ||INTEL_INFO (dev)->gen >=8 ) { + domains =get_crtc_power_domains (crtc); +for_each_power_domain (domain, domains) +intel_display_power_get (dev_priv, + domain); + intel_crtc->enabled_power_domains = domains; + } + + dev_priv->display.crtc_enable (crtc); + } + }else { +if (intel_crtc->active) { + dev_priv->display.crtc_disable (crtc); + +/* IVB and older still don't support RPM on DPMS. */ +if (IS_HASWELL (dev) ||INTEL_INFO (dev)->gen >=8 ) { + domains = intel_crtc->enabled_power_domains; +for_each_power_domain (domain, domains) +intel_display_power_put (dev_priv, + domain); + intel_crtc->enabled_power_domains =0 ; + } + } + }intel_crtc_update_sarea (crtc, enable); } --1.9.0
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch