| Message ID | 1381933553-19529-7-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 16 Oct 2013 17:25:53 +0300 Imre Deak <imre.deak@intel.com> wrote: > Currently we make sure that all power domains are enabled during driver > init and turn off unneded ones only after the first modeset. Similarly > during suspend we enable all power domains, which will remain on through > the following resume until the first modeset. > > This logic is supported by intel_set_power_well() in the power domain > framework. It would be nice to simplify the API, so that we only have > get/put functions and make it more explicit on the higher level how this > "power well on during init" logic works. This will make it also easier > if in the future we want to shorten the time the power wells are on. > > For this add a new device private flag tracking whether we have the > power wells on because of init/suspend and use only > intel_display_power_get()/put(). As nothing else uses > intel_set_power_well() we can remove it. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_dma.c | 2 +- > drivers/gpu/drm/i915/i915_drv.c | 2 +- > drivers/gpu/drm/i915/i915_drv.h | 7 ++++++- > drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 35 +---------------------------------- > 6 files changed, 27 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index b8bc887..dd7f1f6 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1708,7 +1708,7 @@ int i915_driver_unload(struct drm_device *dev) > /* The i915.ko module is still not prepared to be loaded when > * the power well is not enabled, so just enable it in case > * we're going to unload/reload. */ > - intel_set_power_well(dev, true); > + intel_display_set_init_power(dev, true); > i915_remove_power_well(dev); > } > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 59649c0..7299a96 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -469,7 +469,7 @@ static int i915_drm_freeze(struct drm_device *dev) > /* We do a lot of poking in a lot of registers, make sure they work > * properly. */ > hsw_disable_package_c8(dev_priv); > - intel_set_power_well(dev, true); > + intel_display_set_init_power(dev, true); > > drm_kms_helper_poll_disable(dev); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index e4354dd..0557c6b 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -100,6 +100,7 @@ enum intel_display_power_domain { > POWER_DOMAIN_TRANSCODER_C, > POWER_DOMAIN_TRANSCODER_EDP, > POWER_DOMAIN_VGA, > + POWER_DOMAIN_INIT, > > POWER_DOMAIN_NUM, > }; > @@ -913,7 +914,6 @@ struct i915_power_well { > struct mutex lock; > /* power well enable/disable usage count */ > int count; > - int i915_request; > }; > > struct i915_dri1_state { > @@ -1369,6 +1369,11 @@ typedef struct drm_i915_private { > * mchdev_lock in intel_pm.c */ > struct intel_ilk_power_mgmt ips; > > + /* > + * Power wells needed for initialization at driver init and suspend > + * time are on. They are kept on until after the first modeset. > + */ > + bool init_power_on; > /* Haswell power well */ > struct i915_power_well power_well; > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index e2a4f3b..e30db91 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6581,6 +6581,21 @@ static unsigned long get_pipe_power_domains(struct drm_device *dev, > return mask; > } > > +void intel_display_set_init_power(struct drm_device *dev, bool enable) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + if (dev_priv->init_power_on == enable) > + return; > + > + if (enable) > + intel_display_power_get(dev, POWER_DOMAIN_INIT); > + else > + intel_display_power_put(dev, POWER_DOMAIN_INIT); > + > + dev_priv->init_power_on = enable; > +} > + > static void modeset_update_power_wells(struct drm_device *dev) > { > unsigned long pipe_domains[I915_MAX_PIPES] = { 0, }; > @@ -6612,6 +6627,8 @@ static void modeset_update_power_wells(struct drm_device *dev) > > crtc->enabled_power_domains = pipe_domains[crtc->pipe]; > } > + > + intel_display_set_init_power(dev, false); > } > > static void haswell_modeset_global_resources(struct drm_device *dev) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 63a5bfd..e6306f5 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -680,6 +680,7 @@ bool intel_crtc_active(struct drm_crtc *crtc); > void i915_disable_vga_mem(struct drm_device *dev); > void hsw_enable_ips(struct intel_crtc *crtc); > void hsw_disable_ips(struct intel_crtc *crtc); > +void intel_display_set_init_power(struct drm_device *dev, bool enable); > > > /* intel_dp.c */ > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index f7363a8..9291a2e 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5542,39 +5542,6 @@ void i915_remove_power_well(struct drm_device *dev) > hsw_pwr = NULL; > } > > -void intel_set_power_well(struct drm_device *dev, bool enable) > -{ > - struct drm_i915_private *dev_priv = dev->dev_private; > - struct i915_power_well *power_well = &dev_priv->power_well; > - > - if (!HAS_POWER_WELL(dev)) > - return; > - > - if (!i915_disable_power_well && !enable) > - return; > - > - mutex_lock(&power_well->lock); > - > - /* > - * This function will only ever contribute one > - * to the power well reference count. i915_request > - * is what tracks whether we have or have not > - * added the one to the reference count. > - */ > - if (power_well->i915_request == enable) > - goto out; > - > - power_well->i915_request = enable; > - > - if (enable) > - __intel_power_well_get(power_well); > - else > - __intel_power_well_put(power_well); > - > - out: > - mutex_unlock(&power_well->lock); > -} > - > static void intel_resume_power_well(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -5602,7 +5569,7 @@ void intel_init_power_well(struct drm_device *dev) > return; > > /* For now, we need the power well to be always enabled. */ > - intel_set_power_well(dev, true); > + intel_display_set_init_power(dev, true); > intel_resume_power_well(dev); > > /* We're taking over the BIOS, so clear any requests made by it since And there it goes, yay! :) Now we just have to get rid of the _INIT domain and actually shut things down before the first mode set if nothing is in use (maybe with a delayed shutdown to avoid toggling too fast). Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On Wed, Oct 16, 2013 at 05:25:53PM +0300, Imre Deak wrote: > Currently we make sure that all power domains are enabled during driver > init and turn off unneded ones only after the first modeset. Similarly > during suspend we enable all power domains, which will remain on through > the following resume until the first modeset. > > This logic is supported by intel_set_power_well() in the power domain > framework. It would be nice to simplify the API, so that we only have > get/put functions and make it more explicit on the higher level how this > "power well on during init" logic works. This will make it also easier > if in the future we want to shorten the time the power wells are on. > > For this add a new device private flag tracking whether we have the > power wells on because of init/suspend and use only > intel_display_power_get()/put(). As nothing else uses > intel_set_power_well() we can remove it. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- [snip] > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index e4354dd..0557c6b 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -100,6 +100,7 @@ enum intel_display_power_domain { > POWER_DOMAIN_TRANSCODER_C, > POWER_DOMAIN_TRANSCODER_EDP, > POWER_DOMAIN_VGA, > + POWER_DOMAIN_INIT, > > POWER_DOMAIN_NUM, > }; > @@ -913,7 +914,6 @@ struct i915_power_well { > struct mutex lock; > /* power well enable/disable usage count */ > int count; > - int i915_request; > }; > > struct i915_dri1_state { > @@ -1369,6 +1369,11 @@ typedef struct drm_i915_private { > * mchdev_lock in intel_pm.c */ > struct intel_ilk_power_mgmt ips; > > + /* > + * Power wells needed for initialization at driver init and suspend > + * time are on. They are kept on until after the first modeset. > + */ > + bool init_power_on; Please move this into the nice power_well structure we have to avoid overtly polluting our i915_driver_private thing ... All patches up to this one merged, thanks. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index b8bc887..dd7f1f6 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1708,7 +1708,7 @@ int i915_driver_unload(struct drm_device *dev) /* The i915.ko module is still not prepared to be loaded when * the power well is not enabled, so just enable it in case * we're going to unload/reload. */ - intel_set_power_well(dev, true); + intel_display_set_init_power(dev, true); i915_remove_power_well(dev); } diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 59649c0..7299a96 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -469,7 +469,7 @@ static int i915_drm_freeze(struct drm_device *dev) /* We do a lot of poking in a lot of registers, make sure they work * properly. */ hsw_disable_package_c8(dev_priv); - intel_set_power_well(dev, true); + intel_display_set_init_power(dev, true); drm_kms_helper_poll_disable(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index e4354dd..0557c6b 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -100,6 +100,7 @@ enum intel_display_power_domain { POWER_DOMAIN_TRANSCODER_C, POWER_DOMAIN_TRANSCODER_EDP, POWER_DOMAIN_VGA, + POWER_DOMAIN_INIT, POWER_DOMAIN_NUM, }; @@ -913,7 +914,6 @@ struct i915_power_well { struct mutex lock; /* power well enable/disable usage count */ int count; - int i915_request; }; struct i915_dri1_state { @@ -1369,6 +1369,11 @@ typedef struct drm_i915_private { * mchdev_lock in intel_pm.c */ struct intel_ilk_power_mgmt ips; + /* + * Power wells needed for initialization at driver init and suspend + * time are on. They are kept on until after the first modeset. + */ + bool init_power_on; /* Haswell power well */ struct i915_power_well power_well; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e2a4f3b..e30db91 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6581,6 +6581,21 @@ static unsigned long get_pipe_power_domains(struct drm_device *dev, return mask; } +void intel_display_set_init_power(struct drm_device *dev, bool enable) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + if (dev_priv->init_power_on == enable) + return; + + if (enable) + intel_display_power_get(dev, POWER_DOMAIN_INIT); + else + intel_display_power_put(dev, POWER_DOMAIN_INIT); + + dev_priv->init_power_on = enable; +} + static void modeset_update_power_wells(struct drm_device *dev) { unsigned long pipe_domains[I915_MAX_PIPES] = { 0, }; @@ -6612,6 +6627,8 @@ static void modeset_update_power_wells(struct drm_device *dev) crtc->enabled_power_domains = pipe_domains[crtc->pipe]; } + + intel_display_set_init_power(dev, false); } static void haswell_modeset_global_resources(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 63a5bfd..e6306f5 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -680,6 +680,7 @@ bool intel_crtc_active(struct drm_crtc *crtc); void i915_disable_vga_mem(struct drm_device *dev); void hsw_enable_ips(struct intel_crtc *crtc); void hsw_disable_ips(struct intel_crtc *crtc); +void intel_display_set_init_power(struct drm_device *dev, bool enable); /* intel_dp.c */ diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index f7363a8..9291a2e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5542,39 +5542,6 @@ void i915_remove_power_well(struct drm_device *dev) hsw_pwr = NULL; } -void intel_set_power_well(struct drm_device *dev, bool enable) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - struct i915_power_well *power_well = &dev_priv->power_well; - - if (!HAS_POWER_WELL(dev)) - return; - - if (!i915_disable_power_well && !enable) - return; - - mutex_lock(&power_well->lock); - - /* - * This function will only ever contribute one - * to the power well reference count. i915_request - * is what tracks whether we have or have not - * added the one to the reference count. - */ - if (power_well->i915_request == enable) - goto out; - - power_well->i915_request = enable; - - if (enable) - __intel_power_well_get(power_well); - else - __intel_power_well_put(power_well); - - out: - mutex_unlock(&power_well->lock); -} - static void intel_resume_power_well(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -5602,7 +5569,7 @@ void intel_init_power_well(struct drm_device *dev) return; /* For now, we need the power well to be always enabled. */ - intel_set_power_well(dev, true); + intel_display_set_init_power(dev, true); intel_resume_power_well(dev); /* We're taking over the BIOS, so clear any requests made by it since
Currently we make sure that all power domains are enabled during driver init and turn off unneded ones only after the first modeset. Similarly during suspend we enable all power domains, which will remain on through the following resume until the first modeset. This logic is supported by intel_set_power_well() in the power domain framework. It would be nice to simplify the API, so that we only have get/put functions and make it more explicit on the higher level how this "power well on during init" logic works. This will make it also easier if in the future we want to shorten the time the power wells are on. For this add a new device private flag tracking whether we have the power wells on because of init/suspend and use only intel_display_power_get()/put(). As nothing else uses intel_set_power_well() we can remove it. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 7 ++++++- drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 35 +---------------------------------- 6 files changed, 27 insertions(+), 37 deletions(-)