| Message ID | 1405139547-13043-1-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jul 12, 2014 at 10:02:27AM +0530, sagar.a.kamble@intel.com wrote: > From: Borun Fu <borun.fu@intel.com> > > On VLV, after i915_pm_suspend display power wells are staying > power ungated. So, after initiating mem sleep "echo mem > /sys/power/state" > Display is staing D0 State. There might be better way/place to power gate > these wells. Also, we need to make sure that if wells are power gated due to > DPMS OFF sequence, they need not be turned off by i915_pm_suspend again. > > v2: Extracted helper for intel_crtc_disable and power gating CRTC power wells. > [Daniel] > > Cc: Imre Deak <imre.deak@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Change-Id: I34c80da66aa24c423a5576c68aa1f3a8d0f43848 > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> intel_crtc_control looks like a neat idea - I've started also thinking about the enable side and noticed that we'll have similar issues there. But complicated with modeset_global_resources and the differences between the dpms paths and modeset paths. But that's work for another day. Queued for -next, thanks for the patch. -Daniel > --- > drivers/gpu/drm/i915/i915_drv.c | 7 +++---- > drivers/gpu/drm/i915/i915_drv.h | 4 ++++ > drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++------------- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 4 files changed, 25 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 83cb43a..5e4fefd 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -524,12 +524,11 @@ static int i915_drm_freeze(struct drm_device *dev) > > /* > * Disable CRTCs directly since we want to preserve sw state > - * for _thaw. > + * for _thaw. Also, power gate the CRTC power wells. > */ > drm_modeset_lock_all(dev); > - for_each_crtc(dev, crtc) { > - dev_priv->display.crtc_disable(crtc); > - } > + for_each_crtc(dev, crtc) > + intel_crtc_control(crtc, false); > drm_modeset_unlock_all(dev); > > intel_modeset_suspend_hw(dev); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a89c912..54f98d3 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -179,6 +179,10 @@ enum hpd_pin { > list_for_each_entry((intel_connector), &(dev)->mode_config.connector_list, base.head) \ > if ((intel_connector)->base.encoder == (__encoder)) > > +#define for_each_power_domain(domain, mask) \ > + for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \ > + if ((1 << (domain)) & (mask)) > + > struct drm_i915_private; > struct i915_mmu_object; > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index fe6f1db..7a1f14f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4300,10 +4300,6 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc) > I915_WRITE(BCLRPAT(crtc->pipe), 0); > } > > -#define for_each_power_domain(domain, mask) \ > - for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \ > - if ((1 << (domain)) & (mask)) > - > enum intel_display_power_domain > intel_display_port_power_domain(struct intel_encoder *intel_encoder) > { > @@ -4872,21 +4868,14 @@ static void intel_crtc_update_sarea(struct drm_crtc *crtc, > } > } > > -/** > - * Sets the power management mode of the pipe and plane. > - */ > -void intel_crtc_update_dpms(struct drm_crtc *crtc) > +/* Master function to enable/disable CRTC and corresponding power wells */ > +void intel_crtc_control(struct drm_crtc *crtc, bool enable) > { > 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) { > if (!intel_crtc->active) { > @@ -4907,6 +4896,21 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc) > intel_crtc->enabled_power_domains = 0; > } > } > +} > + > +/** > + * Sets the power management mode of the pipe and plane. > + */ > +void intel_crtc_update_dpms(struct drm_crtc *crtc) > +{ > + struct drm_device *dev = crtc->dev; > + struct intel_encoder *intel_encoder; > + bool enable = false; > + > + for_each_encoder_on_crtc(dev, crtc, intel_encoder) > + enable |= intel_encoder->connectors_active; > + > + intel_crtc_control(crtc, enable); > > intel_crtc_update_sarea(crtc, enable); > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 016d894..4c24f88 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -755,6 +755,7 @@ void intel_frontbuffer_flip(struct drm_device *dev, > void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire); > void intel_mark_idle(struct drm_device *dev); > void intel_crtc_restore_mode(struct drm_crtc *crtc); > +void intel_crtc_control(struct drm_crtc *crtc, bool enable); > void intel_crtc_update_dpms(struct drm_crtc *crtc); > void intel_encoder_destroy(struct drm_encoder *encoder); > void intel_connector_dpms(struct drm_connector *, int mode); > -- > 1.8.5 >
On Sat, Jul 12, 2014 at 10:02:27AM +0530, sagar.a.kamble@intel.com wrote: > From: Borun Fu <borun.fu@intel.com> > > On VLV, after i915_pm_suspend display power wells are staying > power ungated. So, after initiating mem sleep "echo mem > /sys/power/state" > Display is staing D0 State. There might be better way/place to power gate > these wells. Also, we need to make sure that if wells are power gated due to > DPMS OFF sequence, they need not be turned off by i915_pm_suspend again. > > v2: Extracted helper for intel_crtc_disable and power gating CRTC power wells. > [Daniel] > > Cc: Imre Deak <imre.deak@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Change-Id: I34c80da66aa24c423a5576c68aa1f3a8d0f43848 s-o-b from the original author (Borun Fu) missing. Added myself since we all work for the same company, but please don't forget this. Every person including the original author, who handles a patch must add their sob line. -Daniel > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 7 +++---- > drivers/gpu/drm/i915/i915_drv.h | 4 ++++ > drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++------------- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 4 files changed, 25 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 83cb43a..5e4fefd 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -524,12 +524,11 @@ static int i915_drm_freeze(struct drm_device *dev) > > /* > * Disable CRTCs directly since we want to preserve sw state > - * for _thaw. > + * for _thaw. Also, power gate the CRTC power wells. > */ > drm_modeset_lock_all(dev); > - for_each_crtc(dev, crtc) { > - dev_priv->display.crtc_disable(crtc); > - } > + for_each_crtc(dev, crtc) > + intel_crtc_control(crtc, false); > drm_modeset_unlock_all(dev); > > intel_modeset_suspend_hw(dev); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a89c912..54f98d3 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -179,6 +179,10 @@ enum hpd_pin { > list_for_each_entry((intel_connector), &(dev)->mode_config.connector_list, base.head) \ > if ((intel_connector)->base.encoder == (__encoder)) > > +#define for_each_power_domain(domain, mask) \ > + for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \ > + if ((1 << (domain)) & (mask)) > + > struct drm_i915_private; > struct i915_mmu_object; > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index fe6f1db..7a1f14f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4300,10 +4300,6 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc) > I915_WRITE(BCLRPAT(crtc->pipe), 0); > } > > -#define for_each_power_domain(domain, mask) \ > - for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \ > - if ((1 << (domain)) & (mask)) > - > enum intel_display_power_domain > intel_display_port_power_domain(struct intel_encoder *intel_encoder) > { > @@ -4872,21 +4868,14 @@ static void intel_crtc_update_sarea(struct drm_crtc *crtc, > } > } > > -/** > - * Sets the power management mode of the pipe and plane. > - */ > -void intel_crtc_update_dpms(struct drm_crtc *crtc) > +/* Master function to enable/disable CRTC and corresponding power wells */ > +void intel_crtc_control(struct drm_crtc *crtc, bool enable) > { > 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) { > if (!intel_crtc->active) { > @@ -4907,6 +4896,21 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc) > intel_crtc->enabled_power_domains = 0; > } > } > +} > + > +/** > + * Sets the power management mode of the pipe and plane. > + */ > +void intel_crtc_update_dpms(struct drm_crtc *crtc) > +{ > + struct drm_device *dev = crtc->dev; > + struct intel_encoder *intel_encoder; > + bool enable = false; > + > + for_each_encoder_on_crtc(dev, crtc, intel_encoder) > + enable |= intel_encoder->connectors_active; > + > + intel_crtc_control(crtc, enable); > > intel_crtc_update_sarea(crtc, enable); > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 016d894..4c24f88 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -755,6 +755,7 @@ void intel_frontbuffer_flip(struct drm_device *dev, > void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire); > void intel_mark_idle(struct drm_device *dev); > void intel_crtc_restore_mode(struct drm_crtc *crtc); > +void intel_crtc_control(struct drm_crtc *crtc, bool enable); > void intel_crtc_update_dpms(struct drm_crtc *crtc); > void intel_encoder_destroy(struct drm_encoder *encoder); > void intel_connector_dpms(struct drm_connector *, int mode); > -- > 1.8.5 >
On 23 July 2014 15:11, Daniel Vetter <daniel@ffwll.ch> wrote: > On Sat, Jul 12, 2014 at 10:02:27AM +0530, sagar.a.kamble@intel.com wrote: >> From: Borun Fu <borun.fu@intel.com> >> >> On VLV, after i915_pm_suspend display power wells are staying >> power ungated. So, after initiating mem sleep "echo mem > /sys/power/state" >> Display is staing D0 State. There might be better way/place to power gate >> these wells. Also, we need to make sure that if wells are power gated due to >> DPMS OFF sequence, they need not be turned off by i915_pm_suspend again. >> >> v2: Extracted helper for intel_crtc_disable and power gating CRTC power wells. >> [Daniel] >> >> Cc: Imre Deak <imre.deak@intel.com> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Change-Id: I34c80da66aa24c423a5576c68aa1f3a8d0f43848 > > s-o-b from the original author (Borun Fu) missing. Added myself since we > all work for the same company, but please don't forget this. Every person > including the original author, who handles a patch must add their sob > line. > -Daniel Is this queued or on its way, I was getting a warning on HSW about not entering pc8+ due to display with MST enabled, and I thought it was MSTs fault, but I suspect its just this, mode set turns the global resources power well on, but nothing ever turns it off. Dave.
On Fri, Jul 25, 2014 at 01:28:48PM +1000, Dave Airlie wrote: > On 23 July 2014 15:11, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Sat, Jul 12, 2014 at 10:02:27AM +0530, sagar.a.kamble@intel.com wrote: > >> From: Borun Fu <borun.fu@intel.com> > >> > >> On VLV, after i915_pm_suspend display power wells are staying > >> power ungated. So, after initiating mem sleep "echo mem > /sys/power/state" > >> Display is staing D0 State. There might be better way/place to power gate > >> these wells. Also, we need to make sure that if wells are power gated due to > >> DPMS OFF sequence, they need not be turned off by i915_pm_suspend again. > >> > >> v2: Extracted helper for intel_crtc_disable and power gating CRTC power wells. > >> [Daniel] > >> > >> Cc: Imre Deak <imre.deak@intel.com> > >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Cc: Jani Nikula <jani.nikula@linux.intel.com> > >> Change-Id: I34c80da66aa24c423a5576c68aa1f3a8d0f43848 > > > > s-o-b from the original author (Borun Fu) missing. Added myself since we > > all work for the same company, but please don't forget this. Every person > > including the original author, who handles a patch must add their sob > > line. > > -Daniel > > Is this queued or on its way, I was getting a warning on HSW about not > entering pc8+ > due to display with MST enabled, and I thought it was MSTs fault, but I suspect > its just this, > > mode set turns the global resources power well on, but nothing ever > turns it off. mst doesn't factor into this. Might need to double-check but from what I've seen mst was only wired into the system s/r paths, not the runtime pm. They're unfortunately not sufficiently shared. So I'd suspect that. The bug here happens when you move/update the cursor (or sprite/primary plane) while the display is off. You'll get unclaimed register read/write errors, no WARNINGs if you hit this one. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 83cb43a..5e4fefd 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -524,12 +524,11 @@ static int i915_drm_freeze(struct drm_device *dev) /* * Disable CRTCs directly since we want to preserve sw state - * for _thaw. + * for _thaw. Also, power gate the CRTC power wells. */ drm_modeset_lock_all(dev); - for_each_crtc(dev, crtc) { - dev_priv->display.crtc_disable(crtc); - } + for_each_crtc(dev, crtc) + intel_crtc_control(crtc, false); drm_modeset_unlock_all(dev); intel_modeset_suspend_hw(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a89c912..54f98d3 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -179,6 +179,10 @@ enum hpd_pin { list_for_each_entry((intel_connector), &(dev)->mode_config.connector_list, base.head) \ if ((intel_connector)->base.encoder == (__encoder)) +#define for_each_power_domain(domain, mask) \ + for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \ + if ((1 << (domain)) & (mask)) + struct drm_i915_private; struct i915_mmu_object; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fe6f1db..7a1f14f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4300,10 +4300,6 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc) I915_WRITE(BCLRPAT(crtc->pipe), 0); } -#define for_each_power_domain(domain, mask) \ - for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \ - if ((1 << (domain)) & (mask)) - enum intel_display_power_domain intel_display_port_power_domain(struct intel_encoder *intel_encoder) { @@ -4872,21 +4868,14 @@ static void intel_crtc_update_sarea(struct drm_crtc *crtc, } } -/** - * Sets the power management mode of the pipe and plane. - */ -void intel_crtc_update_dpms(struct drm_crtc *crtc) +/* Master function to enable/disable CRTC and corresponding power wells */ +void intel_crtc_control(struct drm_crtc *crtc, bool enable) { 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) { if (!intel_crtc->active) { @@ -4907,6 +4896,21 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc) intel_crtc->enabled_power_domains = 0; } } +} + +/** + * Sets the power management mode of the pipe and plane. + */ +void intel_crtc_update_dpms(struct drm_crtc *crtc) +{ + struct drm_device *dev = crtc->dev; + struct intel_encoder *intel_encoder; + bool enable = false; + + for_each_encoder_on_crtc(dev, crtc, intel_encoder) + enable |= intel_encoder->connectors_active; + + intel_crtc_control(crtc, enable); intel_crtc_update_sarea(crtc, enable); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 016d894..4c24f88 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -755,6 +755,7 @@ void intel_frontbuffer_flip(struct drm_device *dev, void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire); void intel_mark_idle(struct drm_device *dev); void intel_crtc_restore_mode(struct drm_crtc *crtc); +void intel_crtc_control(struct drm_crtc *crtc, bool enable); void intel_crtc_update_dpms(struct drm_crtc *crtc); void intel_encoder_destroy(struct drm_encoder *encoder); void intel_connector_dpms(struct drm_connector *, int mode);