* [PATCH 0/2] PM / QoS / Domains: Fix and optimization @ 2012-04-27 21:46 Rafael J. Wysocki 2012-04-27 21:48 ` [PATCH 1/2] PM / QoS: Create device constraints objects on notifier registration Rafael J. Wysocki ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2012-04-27 21:46 UTC (permalink / raw) To: Linux PM list Cc: LKML, Kevin Hilman, Magnus Damm, Mark Brown, markgross, Jean Pihet Hi all, This series is on top of the PM domains governor functions fixup patches posted recently: http://marc.info/?l=linux-pm&m=133538218801804&w=4 http://marc.info/?l=linux-pm&m=133538220601826&w=4 http://marc.info/?l=linux-pm&m=133538218801805&w=4 Patch [1/2] is a fix that I think should be applied anyway. Patch [2/2], though, is an RFC. It is a bit subtle, although perhaps that's not immediately visible at first sight. Patch [2/2] hasn't been tested yet, but I have verified that it builds. Comments welcome! Thanks, Rafael ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] PM / QoS: Create device constraints objects on notifier registration 2012-04-27 21:46 [PATCH 0/2] PM / QoS / Domains: Fix and optimization Rafael J. Wysocki @ 2012-04-27 21:48 ` Rafael J. Wysocki 2012-04-28 15:33 ` mark gross 2012-04-27 21:53 ` [PATCH 2/2][RFC] PM / Domains: Cache device stop and domain power off governor results Rafael J. Wysocki 2012-04-29 1:08 ` [PATCH 0/3] PM / QoS / Domains: Fixes and optimization, take 2 Rafael J. Wysocki 2 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2012-04-27 21:48 UTC (permalink / raw) To: Linux PM list Cc: LKML, Kevin Hilman, Magnus Damm, Mark Brown, markgross, Jean Pihet From: Rafael J. Wysocki <rjw@sisk.pl> The current behavior of dev_pm_qos_add_notifier() makes device PM QoS notifiers less than useful. Namely, it silently returns success when called before any PM QoS constraints are added for the device, so the caller will assume that the notifier has been registered, but when someone actually adds some nontrivial constraints for the device eventually, the previous callers of dev_pm_qos_add_notifier() will not know about that and their notifier routines will not be executed (contrary to their expectations). To address this problem make dev_pm_qos_add_notifier() create the constraints object for the device if it is not present when the routine is called. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- drivers/base/power/qos.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) Index: linux/drivers/base/power/qos.c =================================================================== --- linux.orig/drivers/base/power/qos.c +++ linux/drivers/base/power/qos.c @@ -352,21 +352,26 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_requ * * Will register the notifier into a notification chain that gets called * upon changes to the target value for the device. + * + * If the device's constraints object doesn't exist when this routine is called, + * it will be created (or error code will be returned if that fails). */ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier) { - int retval = 0; + int ret = 0; mutex_lock(&dev_pm_qos_mtx); - /* Silently return if the constraints object is not present. */ - if (dev->power.constraints) - retval = blocking_notifier_chain_register( - dev->power.constraints->notifiers, - notifier); + if (!dev->power.constraints) + ret = dev->power.power_state.event != PM_EVENT_INVALID ? + dev_pm_qos_constraints_allocate(dev) : -ENODEV; + + if (!ret) + ret = blocking_notifier_chain_register( + dev->power.constraints->notifiers, notifier); mutex_unlock(&dev_pm_qos_mtx); - return retval; + return ret; } EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier); ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] PM / QoS: Create device constraints objects on notifier registration 2012-04-27 21:48 ` [PATCH 1/2] PM / QoS: Create device constraints objects on notifier registration Rafael J. Wysocki @ 2012-04-28 15:33 ` mark gross 2012-04-28 21:10 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: mark gross @ 2012-04-28 15:33 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux PM list, LKML, Kevin Hilman, Magnus Damm, Mark Brown, markgross, Jean Pihet On Fri, Apr 27, 2012 at 11:48:18PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rjw@sisk.pl> > > The current behavior of dev_pm_qos_add_notifier() makes device PM QoS > notifiers less than useful. Namely, it silently returns success when > called before any PM QoS constraints are added for the device, so the > caller will assume that the notifier has been registered, but when > someone actually adds some nontrivial constraints for the device > eventually, the previous callers of dev_pm_qos_add_notifier() > will not know about that and their notifier routines will not be > executed (contrary to their expectations). > > To address this problem make dev_pm_qos_add_notifier() create the > constraints object for the device if it is not present when the > routine is called. > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> > --- > drivers/base/power/qos.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > Index: linux/drivers/base/power/qos.c > =================================================================== > --- linux.orig/drivers/base/power/qos.c > +++ linux/drivers/base/power/qos.c > @@ -352,21 +352,26 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_requ > * > * Will register the notifier into a notification chain that gets called > * upon changes to the target value for the device. > + * > + * If the device's constraints object doesn't exist when this routine is called, > + * it will be created (or error code will be returned if that fails). > */ > int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier) > { > - int retval = 0; > + int ret = 0; > > mutex_lock(&dev_pm_qos_mtx); > > - /* Silently return if the constraints object is not present. */ > - if (dev->power.constraints) > - retval = blocking_notifier_chain_register( > - dev->power.constraints->notifiers, > - notifier); FWIW (and IIRC) I did this because some audio use of pm_qos had a series of re-allocation of constraints and clean up that would fall over so I had this silent hack to deal with that "complexity". > + if (!dev->power.constraints) > + ret = dev->power.power_state.event != PM_EVENT_INVALID ? > + dev_pm_qos_constraints_allocate(dev) : -ENODEV; > + > + if (!ret) > + ret = blocking_notifier_chain_register( > + dev->power.constraints->notifiers, notifier); > > mutex_unlock(&dev_pm_qos_mtx); > - return retval; > + return ret; > } > EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier); > Definitely a change for the better (but we should check that the audio stuff still works before committing this. I'll see if I can check that this weekend.) Acked-by : markgross <markgross@thegnar.org> --mark ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] PM / QoS: Create device constraints objects on notifier registration 2012-04-28 15:33 ` mark gross @ 2012-04-28 21:10 ` Rafael J. Wysocki 0 siblings, 0 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2012-04-28 21:10 UTC (permalink / raw) To: markgross Cc: Linux PM list, LKML, Kevin Hilman, Magnus Damm, Mark Brown, Jean Pihet On Saturday, April 28, 2012, mark gross wrote: > On Fri, Apr 27, 2012 at 11:48:18PM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rjw@sisk.pl> > > > > The current behavior of dev_pm_qos_add_notifier() makes device PM QoS > > notifiers less than useful. Namely, it silently returns success when > > called before any PM QoS constraints are added for the device, so the > > caller will assume that the notifier has been registered, but when > > someone actually adds some nontrivial constraints for the device > > eventually, the previous callers of dev_pm_qos_add_notifier() > > will not know about that and their notifier routines will not be > > executed (contrary to their expectations). > > > > To address this problem make dev_pm_qos_add_notifier() create the > > constraints object for the device if it is not present when the > > routine is called. > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> > > --- > > drivers/base/power/qos.c | 19 ++++++++++++------- > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > Index: linux/drivers/base/power/qos.c > > =================================================================== > > --- linux.orig/drivers/base/power/qos.c > > +++ linux/drivers/base/power/qos.c > > @@ -352,21 +352,26 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_requ > > * > > * Will register the notifier into a notification chain that gets called > > * upon changes to the target value for the device. > > + * > > + * If the device's constraints object doesn't exist when this routine is called, > > + * it will be created (or error code will be returned if that fails). > > */ > > int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier) > > { > > - int retval = 0; > > + int ret = 0; > > > > mutex_lock(&dev_pm_qos_mtx); > > > > - /* Silently return if the constraints object is not present. */ > > - if (dev->power.constraints) > > - retval = blocking_notifier_chain_register( > > - dev->power.constraints->notifiers, > > - notifier); > FWIW (and IIRC) I did this because some audio use of pm_qos had a series > of re-allocation of constraints and clean up that would fall over so I > had this silent hack to deal with that "complexity". > > > + if (!dev->power.constraints) > > + ret = dev->power.power_state.event != PM_EVENT_INVALID ? > > + dev_pm_qos_constraints_allocate(dev) : -ENODEV; > > + > > + if (!ret) > > + ret = blocking_notifier_chain_register( > > + dev->power.constraints->notifiers, notifier); > > > > mutex_unlock(&dev_pm_qos_mtx); > > - return retval; > > + return ret; > > } > > EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier); > > > Definitely a change for the better (but we should check that the audio > stuff still works before committing this. I'll see if I can check that > this weekend.) Well, I don't see why not. Obviously, we won't set the dev->power.constraints pointer more than once. :-) > Acked-by : markgross <markgross@thegnar.org> Thanks! Rafael ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2][RFC] PM / Domains: Cache device stop and domain power off governor results 2012-04-27 21:46 [PATCH 0/2] PM / QoS / Domains: Fix and optimization Rafael J. Wysocki 2012-04-27 21:48 ` [PATCH 1/2] PM / QoS: Create device constraints objects on notifier registration Rafael J. Wysocki @ 2012-04-27 21:53 ` Rafael J. Wysocki 2012-04-29 1:08 ` [PATCH 0/3] PM / QoS / Domains: Fixes and optimization, take 2 Rafael J. Wysocki 2 siblings, 0 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2012-04-27 21:53 UTC (permalink / raw) To: Linux PM list Cc: LKML, Kevin Hilman, Magnus Damm, Mark Brown, markgross, Jean Pihet From: Rafael J. Wysocki <rjw@sisk.pl> The results of the default device stop and domain power off governor functions for generic PM domains, default_stop_ok() and default_power_down_ok(), depend only on the timing data of devices, which are static, and on their PM QoS constraints. Thus, in theory, these functions only need to carry out their computations, which may be time consuming in general, when it is known that the PM QoS constraint of at least one of the devices in question has changed. Use the PM QoS notifiers of devices to implement that. First, introduce new fields, constraint_changed and max_off_time_changed, into struct gpd_timing_data and struct generic_pm_domain, respectively, and register a PM QoS notifier function when adding a device into a domain that will set those fields to 'true' whenever the device's PM QoS constraint is modified. Second, make default_stop_ok() and default_power_down_ok() use those fields to decide whether or not to carry out their computations from scratch. The device and PM domain hierarchies are taken into account in that and the expense is that the changes of PM QoS constraints of suspended devices will not be taken into account immediately, which isn't guaranteed anyway in general. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- drivers/base/power/domain.c | 61 +++++++++++++++++++++++++++++++++-- drivers/base/power/domain_governor.c | 40 +++++++++++++++++++++- include/linux/pm_domain.h | 6 +++ 3 files changed, 102 insertions(+), 5 deletions(-) Index: linux/include/linux/pm_domain.h =================================================================== --- linux.orig/include/linux/pm_domain.h +++ linux/include/linux/pm_domain.h @@ -14,6 +14,7 @@ #include <linux/pm.h> #include <linux/err.h> #include <linux/of.h> +#include <linux/notifier.h> enum gpd_status { GPD_STATE_ACTIVE = 0, /* PM domain is active */ @@ -71,6 +72,8 @@ struct generic_pm_domain { s64 power_on_latency_ns; struct gpd_dev_ops dev_ops; s64 max_off_time_ns; /* Maximum allowed "suspended" time. */ + bool max_off_time_changed; + bool cached_power_down_ok; struct device_node *of_node; /* Node in device tree */ }; @@ -92,12 +95,15 @@ struct gpd_timing_data { s64 save_state_latency_ns; s64 restore_state_latency_ns; s64 effective_constraint_ns; + bool constraint_changed; + bool cached_stop_ok; }; struct generic_pm_domain_data { struct pm_domain_data base; struct gpd_dev_ops ops; struct gpd_timing_data td; + struct notifier_block nb; bool need_restore; bool always_on; }; Index: linux/drivers/base/power/domain.c =================================================================== --- linux.orig/drivers/base/power/domain.c +++ linux/drivers/base/power/domain.c @@ -11,6 +11,7 @@ #include <linux/io.h> #include <linux/pm_runtime.h> #include <linux/pm_domain.h> +#include <linux/pm_qos.h> #include <linux/slab.h> #include <linux/err.h> #include <linux/sched.h> @@ -247,6 +248,39 @@ int pm_genpd_poweron(struct generic_pm_d #ifdef CONFIG_PM_RUNTIME +static int genpd_dev_pm_qos_notifier(struct notifier_block *nb, + unsigned long val, void *ptr) +{ + struct generic_pm_domain_data *gpd_data; + struct device *dev; + struct generic_pm_domain *genpd; + + gpd_data = container_of(nb, struct generic_pm_domain_data, nb); + dev = gpd_data->base.dev; + do { + /* + * Take the device power lock to prevent the possible race + * condition in which the PM QoS constraint of the device is + * changed twice in a row very quickly and default_stop_ok() + * runs in parallel with the second change. + */ + spin_lock_irq(&dev->power.lock); + gpd_data->td.constraint_changed = true; + spin_unlock_irq(&dev->power.lock); + + genpd = dev_to_genpd(dev); + if (!IS_ERR(genpd)) { + mutex_lock(&genpd->lock); + genpd->max_off_time_changed = true; + mutex_unlock(&genpd->lock); + } + + dev = dev->parent; + } while (dev && !dev->power.ignore_children); + + return NOTIFY_DONE; +} + /** * __pm_genpd_save_device - Save the pre-suspend state of a device. * @pdd: Domain data of the device to save the state of. @@ -381,7 +415,9 @@ static int pm_genpd_poweroff(struct gene return 0; } - genpd->max_off_time_ns = -1; + if (genpd->max_off_time_changed) + genpd->max_off_time_ns = -1; + if (genpd->gov && genpd->gov->power_down_ok) { if (!genpd->gov->power_down_ok(&genpd->domain)) return -EAGAIN; @@ -483,6 +519,7 @@ static int pm_genpd_runtime_suspend(stru { struct generic_pm_domain *genpd; bool (*stop_ok)(struct device *__dev); + struct gpd_timing_data *td; int ret; dev_dbg(dev, "%s()\n", __func__); @@ -496,7 +533,10 @@ static int pm_genpd_runtime_suspend(stru if (dev_gpd_data(dev)->always_on) return -EBUSY; - dev_gpd_data(dev)->td.effective_constraint_ns = -1; + td = &dev_gpd_data(dev)->td; + if (td->constraint_changed) + td->effective_constraint_ns = -1; + stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL; if (stop_ok && !stop_ok(dev)) return -EBUSY; @@ -601,6 +641,12 @@ void pm_genpd_poweroff_unused(void) #else +static inline int genpd_dev_pm_qos_notifier(struct notifier_block *nb, + unsigned long val, void *ptr) +{ + return NOTIFY_DONE; +} + static inline void genpd_power_off_work_fn(struct work_struct *work) {} #define pm_genpd_runtime_suspend NULL @@ -1232,6 +1278,10 @@ int __pm_genpd_add_device(struct generic if (td) gpd_data->td = *td; + gpd_data->td.constraint_changed = true; + gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier; + dev_pm_qos_add_notifier(dev, &gpd_data->nb); + out: genpd_release_lock(genpd); @@ -1294,14 +1344,18 @@ int pm_genpd_remove_device(struct generi } list_for_each_entry(pdd, &genpd->dev_list, list_node) { + struct generic_pm_domain_data *gpd_data; + if (pdd->dev != dev) continue; + gpd_data = to_gpd_data(pdd); + dev_pm_qos_remove_notifier(dev, &gpd_data->nb); list_del_init(&pdd->list_node); pdd->dev = NULL; dev_pm_put_subsys_data(dev); dev->pm_domain = NULL; - kfree(to_gpd_data(pdd)); + kfree(gpd_data); genpd->device_count--; @@ -1678,6 +1732,7 @@ void pm_genpd_init(struct generic_pm_dom genpd->resume_count = 0; genpd->device_count = 0; genpd->max_off_time_ns = -1; + genpd->max_off_time_changed = true; genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend; genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume; genpd->domain.ops.runtime_idle = pm_generic_runtime_idle; Index: linux/drivers/base/power/domain_governor.c =================================================================== --- linux.orig/drivers/base/power/domain_governor.c +++ linux/drivers/base/power/domain_governor.c @@ -46,11 +46,25 @@ static int dev_update_qos_constraint(str bool default_stop_ok(struct device *dev) { struct gpd_timing_data *td = &dev_gpd_data(dev)->td; + unsigned long flags; s64 constraint_ns; dev_dbg(dev, "%s()\n", __func__); - constraint_ns = dev_pm_qos_read_value(dev); + spin_lock_irqsave(&dev->power.lock, flags); + + if (!td->constraint_changed) { + bool ret = td->cached_stop_ok; + + spin_unlock_irqrestore(&dev->power.lock, flags); + return ret; + } + td->constraint_changed = false; + td->cached_stop_ok = false; + constraint_ns = __dev_pm_qos_read_value(dev); + + spin_unlock_irqrestore(&dev->power.lock, flags); + if (constraint_ns < 0) return false; @@ -69,11 +83,13 @@ bool default_stop_ok(struct device *dev) return false; } td->effective_constraint_ns = constraint_ns; + td->cached_stop_ok = constraint_ns > td->stop_latency_ns || + constraint_ns == 0; /* * The children have been suspended already, so we don't need to take * their stop latencies into account here. */ - return constraint_ns > td->stop_latency_ns || constraint_ns == 0; + return td->cached_stop_ok; } /** @@ -90,6 +106,24 @@ static bool default_power_down_ok(struct s64 min_dev_off_time_ns; s64 off_on_time_ns; + if (genpd->max_off_time_changed) { + struct gpd_link *link; + + /* + * We have to invalidate the cached results for the masters, so + * use the observation that default_power_down_ok() is not + * going to be called for any master until this instance + * returns. + */ + list_for_each_entry(link, &genpd->slave_links, slave_node) + link->master->max_off_time_changed = true; + + genpd->max_off_time_changed = false; + genpd->cached_power_down_ok = false; + } else { + return genpd->cached_power_down_ok; + } + off_on_time_ns = genpd->power_off_latency_ns + genpd->power_on_latency_ns; /* @@ -165,6 +199,8 @@ static bool default_power_down_ok(struct min_dev_off_time_ns = constraint_ns; } + genpd->cached_power_down_ok = true; + /* * If the computed minimum device off time is negative, there are no * latency constraints, so the domain can spend arbitrary time in the ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 0/3] PM / QoS / Domains: Fixes and optimization, take 2 2012-04-27 21:46 [PATCH 0/2] PM / QoS / Domains: Fix and optimization Rafael J. Wysocki 2012-04-27 21:48 ` [PATCH 1/2] PM / QoS: Create device constraints objects on notifier registration Rafael J. Wysocki 2012-04-27 21:53 ` [PATCH 2/2][RFC] PM / Domains: Cache device stop and domain power off governor results Rafael J. Wysocki @ 2012-04-29 1:08 ` Rafael J. Wysocki 2012-04-29 1:11 ` [PATCH 1/3] PM / QoS: Create device constraints objects on notifier registration Rafael J. Wysocki ` (2 more replies) 2 siblings, 3 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2012-04-29 1:08 UTC (permalink / raw) To: Linux PM list Cc: LKML, Kevin Hilman, Magnus Damm, Mark Brown, markgross, Jean Pihet Hi all, On Friday, April 27, 2012, Rafael J. Wysocki wrote: > > This series is on top of the PM domains governor functions fixup patches > posted recently: > > http://marc.info/?l=linux-pm&m=133538218801804&w=4 > http://marc.info/?l=linux-pm&m=133538220601826&w=4 > http://marc.info/?l=linux-pm&m=133538218801805&w=4 > > Patch [1/2] is a fix that I think should be applied anyway. Patch [2/2], > though, is an RFC. It is a bit subtle, although perhaps that's not immediately > visible at first sight. Patch [2/2] hasn't been tested yet, but I have > verified that it builds. As it has happened to me for a couple of times recently, unfortunately, I found a couple of issues with patch [2/2]. Most importantly, I forgot about the adaptive adjustments of device and domain latency numbers, which obviously had to be taken into consideration in that patch. Also, it turned out during testig that there was a locking issue in pm_genpd_remove_device() that wasn't very easy to address. In the end, I had to use an additional patch to help that, which is the current [2/3]. So, [1/3] is the previous [1/2] (with the Mark's ACK). [2/3] is a new patch, which is a pm_genpd_remove_device() cleanup that may be regarded as a fix too and [3/3] is the one that makes the default PM domain governors cache their results if possible (that's even more subtle than before, but hopefully I didn't break anything this time). Thanks, Rafael ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] PM / QoS: Create device constraints objects on notifier registration 2012-04-29 1:08 ` [PATCH 0/3] PM / QoS / Domains: Fixes and optimization, take 2 Rafael J. Wysocki @ 2012-04-29 1:11 ` Rafael J. Wysocki 2012-04-30 8:57 ` Jean Pihet 2012-04-29 1:12 ` [PATCH 2/3] PM / Domains: Make device removal more straightforward Rafael J. Wysocki 2012-04-29 1:14 ` [PATCH 3/3][RFC] PM / Domains: Cache device stop and domain power off governor results, v2 Rafael J. Wysocki 2 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2012-04-29 1:11 UTC (permalink / raw) To: Linux PM list Cc: LKML, Kevin Hilman, Magnus Damm, Mark Brown, markgross, Jean Pihet From: Rafael J. Wysocki <rjw@sisk.pl> The current behavior of dev_pm_qos_add_notifier() makes device PM QoS notifiers less than useful. Namely, it silently returns success when called before any PM QoS constraints are added for the device, so the caller will assume that the notifier has been registered, but when someone actually adds some nontrivial constraints for the device eventually, the previous callers of dev_pm_qos_add_notifier() will not know about that and their notifier routines will not be executed (contrary to their expectations). To address this problem make dev_pm_qos_add_notifier() create the constraints object for the device if it is not present when the routine is called. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> Acked-by : markgross <markgross@thegnar.org> --- drivers/base/power/qos.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) Index: linux/drivers/base/power/qos.c =================================================================== --- linux.orig/drivers/base/power/qos.c +++ linux/drivers/base/power/qos.c @@ -352,21 +352,26 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_requ * * Will register the notifier into a notification chain that gets called * upon changes to the target value for the device. + * + * If the device's constraints object doesn't exist when this routine is called, + * it will be created (or error code will be returned if that fails). */ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier) { - int retval = 0; + int ret = 0; mutex_lock(&dev_pm_qos_mtx); - /* Silently return if the constraints object is not present. */ - if (dev->power.constraints) - retval = blocking_notifier_chain_register( - dev->power.constraints->notifiers, - notifier); + if (!dev->power.constraints) + ret = dev->power.power_state.event != PM_EVENT_INVALID ? + dev_pm_qos_constraints_allocate(dev) : -ENODEV; + + if (!ret) + ret = blocking_notifier_chain_register( + dev->power.constraints->notifiers, notifier); mutex_unlock(&dev_pm_qos_mtx); - return retval; + return ret; } EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier); ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] PM / QoS: Create device constraints objects on notifier registration 2012-04-29 1:11 ` [PATCH 1/3] PM / QoS: Create device constraints objects on notifier registration Rafael J. Wysocki @ 2012-04-30 8:57 ` Jean Pihet 0 siblings, 0 replies; 11+ messages in thread From: Jean Pihet @ 2012-04-30 8:57 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux PM list, LKML, Kevin Hilman, Magnus Damm, Mark Brown, markgross, Jean Pihet HI Rafael, On Sun, Apr 29, 2012 at 3:11 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > From: Rafael J. Wysocki <rjw@sisk.pl> > > The current behavior of dev_pm_qos_add_notifier() makes device PM QoS > notifiers less than useful. Namely, it silently returns success when > called before any PM QoS constraints are added for the device, so the > caller will assume that the notifier has been registered, but when > someone actually adds some nontrivial constraints for the device > eventually, the previous callers of dev_pm_qos_add_notifier() > will not know about that and their notifier routines will not be > executed (contrary to their expectations). > > To address this problem make dev_pm_qos_add_notifier() create the > constraints object for the device if it is not present when the > routine is called. That is definitively more than needed! FWIW: Acked-by: Jean Pihet <j-pihet@ti.com> Thanks for the fix, Jean > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> > Acked-by : markgross <markgross@thegnar.org> > --- > drivers/base/power/qos.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > Index: linux/drivers/base/power/qos.c > =================================================================== > --- linux.orig/drivers/base/power/qos.c > +++ linux/drivers/base/power/qos.c > @@ -352,21 +352,26 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_requ > * > * Will register the notifier into a notification chain that gets called > * upon changes to the target value for the device. > + * > + * If the device's constraints object doesn't exist when this routine is called, > + * it will be created (or error code will be returned if that fails). > */ > int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier) > { > - int retval = 0; > + int ret = 0; > > mutex_lock(&dev_pm_qos_mtx); > > - /* Silently return if the constraints object is not present. */ > - if (dev->power.constraints) > - retval = blocking_notifier_chain_register( > - dev->power.constraints->notifiers, > - notifier); > + if (!dev->power.constraints) > + ret = dev->power.power_state.event != PM_EVENT_INVALID ? > + dev_pm_qos_constraints_allocate(dev) : -ENODEV; > + > + if (!ret) > + ret = blocking_notifier_chain_register( > + dev->power.constraints->notifiers, notifier); > > mutex_unlock(&dev_pm_qos_mtx); > - return retval; > + return ret; > } > EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier); > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] PM / Domains: Make device removal more straightforward 2012-04-29 1:08 ` [PATCH 0/3] PM / QoS / Domains: Fixes and optimization, take 2 Rafael J. Wysocki 2012-04-29 1:11 ` [PATCH 1/3] PM / QoS: Create device constraints objects on notifier registration Rafael J. Wysocki @ 2012-04-29 1:12 ` Rafael J. Wysocki 2012-04-29 1:14 ` [PATCH 3/3][RFC] PM / Domains: Cache device stop and domain power off governor results, v2 Rafael J. Wysocki 2 siblings, 0 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2012-04-29 1:12 UTC (permalink / raw) To: Linux PM list Cc: LKML, Kevin Hilman, Magnus Damm, Mark Brown, markgross, Jean Pihet From: Rafael J. Wysocki <rjw@sisk.pl> The removal of a device from a PM domain doesn't have to browse the domain's device list, because it can check directly if the device belongs to the given domain. Moreover, it should clear the domain_data pointer in dev->power.subsys_data, because dev_pm_put_subsys_data(dev) may not remove dev->power.subsys_data and the stale domain data pointer may cause problems to happen. Rework pm_genpd_remove_device() taking the above observations into account. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- drivers/base/power/domain.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) Index: linux/drivers/base/power/domain.c =================================================================== --- linux.orig/drivers/base/power/domain.c +++ linux/drivers/base/power/domain.c @@ -1279,11 +1279,13 @@ int pm_genpd_remove_device(struct generi struct device *dev) { struct pm_domain_data *pdd; - int ret = -EINVAL; + int ret = 0; dev_dbg(dev, "%s()\n", __func__); - if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)) + if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev) + || IS_ERR_OR_NULL(dev->pm_domain) + || pd_to_genpd(dev->pm_domain) != genpd) return -EINVAL; genpd_acquire_lock(genpd); @@ -1293,21 +1295,14 @@ int pm_genpd_remove_device(struct generi goto out; } - list_for_each_entry(pdd, &genpd->dev_list, list_node) { - if (pdd->dev != dev) - continue; - - list_del_init(&pdd->list_node); - pdd->dev = NULL; - dev_pm_put_subsys_data(dev); - dev->pm_domain = NULL; - kfree(to_gpd_data(pdd)); + dev->pm_domain = NULL; + pdd = dev->power.subsys_data->domain_data; + list_del_init(&pdd->list_node); + dev->power.subsys_data->domain_data = NULL; + dev_pm_put_subsys_data(dev); + kfree(to_gpd_data(pdd)); - genpd->device_count--; - - ret = 0; - break; - } + genpd->device_count--; out: genpd_release_lock(genpd); ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3][RFC] PM / Domains: Cache device stop and domain power off governor results, v2 2012-04-29 1:08 ` [PATCH 0/3] PM / QoS / Domains: Fixes and optimization, take 2 Rafael J. Wysocki 2012-04-29 1:11 ` [PATCH 1/3] PM / QoS: Create device constraints objects on notifier registration Rafael J. Wysocki 2012-04-29 1:12 ` [PATCH 2/3] PM / Domains: Make device removal more straightforward Rafael J. Wysocki @ 2012-04-29 1:14 ` Rafael J. Wysocki 2012-04-29 14:12 ` [Update][PATCH 3/3][RFC] PM / Domains: Cache device stop and domain power off governor results, v3 Rafael J. Wysocki 2 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2012-04-29 1:14 UTC (permalink / raw) To: Linux PM list Cc: LKML, Kevin Hilman, Magnus Damm, Mark Brown, markgross, Jean Pihet From: Rafael J. Wysocki <rjw@sisk.pl> The results of the default device stop and domain power off governor functions for generic PM domains, default_stop_ok() and default_power_down_ok(), depend only on the timing data of devices, which are static, and on their PM QoS constraints. Thus, in theory, these functions only need to carry out their computations, which may be time consuming in general, when it is known that the PM QoS constraint of at least one of the devices in question has changed. Use the PM QoS notifiers of devices to implement that. First, introduce new fields, constraint_changed and max_off_time_changed, into struct gpd_timing_data and struct generic_pm_domain, respectively, and register a PM QoS notifier function when adding a device into a domain that will set those fields to 'true' whenever the device's PM QoS constraint is modified. Second, make default_stop_ok() and default_power_down_ok() use those fields to decide whether or not to carry out their computations from scratch. The device and PM domain hierarchies are taken into account in that and the expense is that the changes of PM QoS constraints of suspended devices will not be taken into account immediately, which isn't guaranteed anyway in general. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- drivers/base/power/domain.c | 127 ++++++++++++++++++++++++++++++----- drivers/base/power/domain_governor.c | 43 +++++++++++ include/linux/pm_domain.h | 7 + 3 files changed, 158 insertions(+), 19 deletions(-) Index: linux/include/linux/pm_domain.h =================================================================== --- linux.orig/include/linux/pm_domain.h +++ linux/include/linux/pm_domain.h @@ -14,6 +14,7 @@ #include <linux/pm.h> #include <linux/err.h> #include <linux/of.h> +#include <linux/notifier.h> enum gpd_status { GPD_STATE_ACTIVE = 0, /* PM domain is active */ @@ -71,6 +72,8 @@ struct generic_pm_domain { s64 power_on_latency_ns; struct gpd_dev_ops dev_ops; s64 max_off_time_ns; /* Maximum allowed "suspended" time. */ + bool max_off_time_changed; + bool cached_power_down_ok; struct device_node *of_node; /* Node in device tree */ }; @@ -92,12 +95,16 @@ struct gpd_timing_data { s64 save_state_latency_ns; s64 restore_state_latency_ns; s64 effective_constraint_ns; + bool constraint_changed; + bool cached_stop_ok; }; struct generic_pm_domain_data { struct pm_domain_data base; struct gpd_dev_ops ops; struct gpd_timing_data td; + struct notifier_block nb; + struct mutex lock; bool need_restore; bool always_on; }; Index: linux/drivers/base/power/domain.c =================================================================== --- linux.orig/drivers/base/power/domain.c +++ linux/drivers/base/power/domain.c @@ -11,6 +11,7 @@ #include <linux/io.h> #include <linux/pm_runtime.h> #include <linux/pm_domain.h> +#include <linux/pm_qos.h> #include <linux/slab.h> #include <linux/err.h> #include <linux/sched.h> @@ -38,11 +39,13 @@ ktime_t __start = ktime_get(); \ type __retval = GENPD_DEV_CALLBACK(genpd, type, callback, dev); \ s64 __elapsed = ktime_to_ns(ktime_sub(ktime_get(), __start)); \ - struct generic_pm_domain_data *__gpd_data = dev_gpd_data(dev); \ - if (__elapsed > __gpd_data->td.field) { \ - __gpd_data->td.field = __elapsed; \ + struct gpd_timing_data *__td = &dev_gpd_data(dev)->td; \ + if (!__retval && __elapsed > __td->field) { \ + __td->field = __elapsed; \ dev_warn(dev, name " latency exceeded, new value %lld ns\n", \ __elapsed); \ + genpd->max_off_time_changed = true; \ + __td->constraint_changed = true; \ } \ __retval; \ }) @@ -211,6 +214,7 @@ int __pm_genpd_poweron(struct generic_pm elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start)); if (elapsed_ns > genpd->power_on_latency_ns) { genpd->power_on_latency_ns = elapsed_ns; + genpd->max_off_time_changed = true; if (genpd->name) pr_warning("%s: Power-on latency exceeded, " "new value %lld ns\n", genpd->name, @@ -247,6 +251,53 @@ int pm_genpd_poweron(struct generic_pm_d #ifdef CONFIG_PM_RUNTIME +static int genpd_dev_pm_qos_notifier(struct notifier_block *nb, + unsigned long val, void *ptr) +{ + struct generic_pm_domain_data *gpd_data; + struct device *dev; + + gpd_data = container_of(nb, struct generic_pm_domain_data, nb); + + mutex_lock(&gpd_data->lock); + dev = gpd_data->base.dev; + if (!dev) { + mutex_unlock(&gpd_data->lock); + return NOTIFY_DONE; + } + mutex_unlock(&gpd_data->lock); + + for (;;) { + struct generic_pm_domain *genpd; + struct pm_domain_data *pdd; + + spin_lock_irq(&dev->power.lock); + + pdd = dev->power.subsys_data ? + dev->power.subsys_data->domain_data : NULL; + if (pdd) { + to_gpd_data(pdd)->td.constraint_changed = true; + genpd = dev_to_genpd(dev); + } else { + genpd = ERR_PTR(-ENODATA); + } + + spin_unlock_irq(&dev->power.lock); + + if (!IS_ERR(genpd)) { + mutex_lock(&genpd->lock); + genpd->max_off_time_changed = true; + mutex_unlock(&genpd->lock); + } + + dev = dev->parent; + if (!dev || dev->power.ignore_children) + break; + } + + return NOTIFY_DONE; +} + /** * __pm_genpd_save_device - Save the pre-suspend state of a device. * @pdd: Domain data of the device to save the state of. @@ -381,7 +432,9 @@ static int pm_genpd_poweroff(struct gene return 0; } - genpd->max_off_time_ns = -1; + if (genpd->max_off_time_changed) + genpd->max_off_time_ns = -1; + if (genpd->gov && genpd->gov->power_down_ok) { if (!genpd->gov->power_down_ok(&genpd->domain)) return -EAGAIN; @@ -436,6 +489,7 @@ static int pm_genpd_poweroff(struct gene elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start)); if (elapsed_ns > genpd->power_off_latency_ns) { genpd->power_off_latency_ns = elapsed_ns; + genpd->max_off_time_changed = true; if (genpd->name) pr_warning("%s: Power-off latency exceeded, " "new value %lld ns\n", genpd->name, @@ -483,6 +537,7 @@ static int pm_genpd_runtime_suspend(stru { struct generic_pm_domain *genpd; bool (*stop_ok)(struct device *__dev); + struct gpd_timing_data *td; int ret; dev_dbg(dev, "%s()\n", __func__); @@ -496,7 +551,10 @@ static int pm_genpd_runtime_suspend(stru if (dev_gpd_data(dev)->always_on) return -EBUSY; - dev_gpd_data(dev)->td.effective_constraint_ns = -1; + td = &dev_gpd_data(dev)->td; + if (td->constraint_changed) + td->effective_constraint_ns = -1; + stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL; if (stop_ok && !stop_ok(dev)) return -EBUSY; @@ -601,6 +659,12 @@ void pm_genpd_poweroff_unused(void) #else +static inline int genpd_dev_pm_qos_notifier(struct notifier_block *nb, + unsigned long val, void *ptr) +{ + return NOTIFY_DONE; +} + static inline void genpd_power_off_work_fn(struct work_struct *work) {} #define pm_genpd_runtime_suspend NULL @@ -1197,6 +1261,14 @@ int __pm_genpd_add_device(struct generic if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)) return -EINVAL; + gpd_data = kzalloc(sizeof(*gpd_data), GFP_KERNEL); + if (!gpd_data) + return -ENOMEM; + + mutex_init(&gpd_data->lock); + gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier; + dev_pm_qos_add_notifier(dev, &gpd_data->nb); + genpd_acquire_lock(genpd); if (genpd->status == GPD_STATE_POWER_OFF) { @@ -1215,26 +1287,34 @@ int __pm_genpd_add_device(struct generic goto out; } - gpd_data = kzalloc(sizeof(*gpd_data), GFP_KERNEL); - if (!gpd_data) { - ret = -ENOMEM; - goto out; - } - genpd->device_count++; + genpd->max_off_time_changed = true; - dev->pm_domain = &genpd->domain; dev_pm_get_subsys_data(dev); + + mutex_lock(&gpd_data->lock); + spin_lock_irq(&dev->power.lock); + dev->pm_domain = &genpd->domain; dev->power.subsys_data->domain_data = &gpd_data->base; gpd_data->base.dev = dev; - gpd_data->need_restore = false; list_add_tail(&gpd_data->base.list_node, &genpd->dev_list); + gpd_data->need_restore = false; if (td) gpd_data->td = *td; + gpd_data->td.constraint_changed = true; + spin_unlock_irq(&dev->power.lock); + mutex_unlock(&gpd_data->lock); + + genpd_release_lock(genpd); + + return 0; + out: genpd_release_lock(genpd); + dev_pm_qos_remove_notifier(dev, &gpd_data->nb); + kfree(gpd_data); return ret; } @@ -1278,6 +1358,7 @@ int __pm_genpd_of_add_device(struct devi int pm_genpd_remove_device(struct generic_pm_domain *genpd, struct device *dev) { + struct generic_pm_domain_data *gpd_data; struct pm_domain_data *pdd; int ret = 0; @@ -1295,14 +1376,27 @@ int pm_genpd_remove_device(struct generi goto out; } + genpd->device_count--; + genpd->max_off_time_changed = true; + + spin_lock_irq(&dev->power.lock); dev->pm_domain = NULL; pdd = dev->power.subsys_data->domain_data; list_del_init(&pdd->list_node); dev->power.subsys_data->domain_data = NULL; - dev_pm_put_subsys_data(dev); - kfree(to_gpd_data(pdd)); + spin_unlock_irq(&dev->power.lock); - genpd->device_count--; + gpd_data = to_gpd_data(pdd); + mutex_lock(&gpd_data->lock); + pdd->dev = NULL; + mutex_unlock(&gpd_data->lock); + + genpd_release_lock(genpd); + + dev_pm_qos_remove_notifier(dev, &gpd_data->nb); + kfree(gpd_data); + dev_pm_put_subsys_data(dev); + return 0; out: genpd_release_lock(genpd); @@ -1673,6 +1767,7 @@ void pm_genpd_init(struct generic_pm_dom genpd->resume_count = 0; genpd->device_count = 0; genpd->max_off_time_ns = -1; + genpd->max_off_time_changed = true; genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend; genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume; genpd->domain.ops.runtime_idle = pm_generic_runtime_idle; Index: linux/drivers/base/power/domain_governor.c =================================================================== --- linux.orig/drivers/base/power/domain_governor.c +++ linux/drivers/base/power/domain_governor.c @@ -46,18 +46,33 @@ static int dev_update_qos_constraint(str bool default_stop_ok(struct device *dev) { struct gpd_timing_data *td = &dev_gpd_data(dev)->td; + unsigned long flags; s64 constraint_ns; dev_dbg(dev, "%s()\n", __func__); - constraint_ns = dev_pm_qos_read_value(dev); + spin_lock_irqsave(&dev->power.lock, flags); + + if (!td->constraint_changed) { + bool ret = td->cached_stop_ok; + + spin_unlock_irqrestore(&dev->power.lock, flags); + return ret; + } + //td->constraint_changed = false; + td->cached_stop_ok = false; + constraint_ns = __dev_pm_qos_read_value(dev); + + spin_unlock_irqrestore(&dev->power.lock, flags); + if (constraint_ns < 0) return false; constraint_ns *= NSEC_PER_USEC; /* * We can walk the children without any additional locking, because - * they all have been suspended at this point. + * they all have been suspended at this point and their + * effective_constraint_ns fields won't be modified in parallel with us. */ if (!dev->power.ignore_children) device_for_each_child(dev, &constraint_ns, @@ -69,11 +84,13 @@ bool default_stop_ok(struct device *dev) return false; } td->effective_constraint_ns = constraint_ns; + td->cached_stop_ok = constraint_ns > td->stop_latency_ns || + constraint_ns == 0; /* * The children have been suspended already, so we don't need to take * their stop latencies into account here. */ - return constraint_ns > td->stop_latency_ns || constraint_ns == 0; + return td->cached_stop_ok; } /** @@ -90,6 +107,24 @@ static bool default_power_down_ok(struct s64 min_dev_off_time_ns; s64 off_on_time_ns; + if (genpd->max_off_time_changed) { + struct gpd_link *link; + + /* + * We have to invalidate the cached results for the masters, so + * use the observation that default_power_down_ok() is not + * going to be called for any master until this instance + * returns. + */ + list_for_each_entry(link, &genpd->slave_links, slave_node) + link->master->max_off_time_changed = true; + + //genpd->max_off_time_changed = false; + genpd->cached_power_down_ok = false; + } else { + return genpd->cached_power_down_ok; + } + off_on_time_ns = genpd->power_off_latency_ns + genpd->power_on_latency_ns; /* @@ -165,6 +200,8 @@ static bool default_power_down_ok(struct min_dev_off_time_ns = constraint_ns; } + genpd->cached_power_down_ok = true; + /* * If the computed minimum device off time is negative, there are no * latency constraints, so the domain can spend arbitrary time in the ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Update][PATCH 3/3][RFC] PM / Domains: Cache device stop and domain power off governor results, v3 2012-04-29 1:14 ` [PATCH 3/3][RFC] PM / Domains: Cache device stop and domain power off governor results, v2 Rafael J. Wysocki @ 2012-04-29 14:12 ` Rafael J. Wysocki 0 siblings, 0 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2012-04-29 14:12 UTC (permalink / raw) To: Linux PM list Cc: LKML, Kevin Hilman, Magnus Damm, Mark Brown, markgross, Jean Pihet From: Rafael J. Wysocki <rjw@sisk.pl> The results of the default device stop and domain power off governor functions for generic PM domains, default_stop_ok() and default_power_down_ok(), depend only on the timing data of devices, which are static, and on their PM QoS constraints. Thus, in theory, these functions only need to carry out their computations, which may be time consuming in general, when it is known that the PM QoS constraint of at least one of the devices in question has changed. Use the PM QoS notifiers of devices to implement that. First, introduce new fields, constraint_changed and max_off_time_changed, into struct gpd_timing_data and struct generic_pm_domain, respectively, and register a PM QoS notifier function when adding a device into a domain that will set those fields to 'true' whenever the device's PM QoS constraint is modified. Second, make default_stop_ok() and default_power_down_ok() use those fields to decide whether or not to carry out their computations from scratch. The device and PM domain hierarchies are taken into account in that and the expense is that the changes of PM QoS constraints of suspended devices will not be taken into account immediately, which isn't guaranteed anyway in general. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- Well, shame on me, I shouldn't have posted patches after 3 AM. The below is the correct version of patch [3/3], please disregard the previous one. Thanks, Rafael --- drivers/base/power/domain.c | 120 ++++++++++++++++++++++++++++++----- drivers/base/power/domain_governor.c | 45 ++++++++++++- include/linux/pm_domain.h | 7 ++ 3 files changed, 153 insertions(+), 19 deletions(-) Index: linux/include/linux/pm_domain.h =================================================================== --- linux.orig/include/linux/pm_domain.h +++ linux/include/linux/pm_domain.h @@ -14,6 +14,7 @@ #include <linux/pm.h> #include <linux/err.h> #include <linux/of.h> +#include <linux/notifier.h> enum gpd_status { GPD_STATE_ACTIVE = 0, /* PM domain is active */ @@ -71,6 +72,8 @@ struct generic_pm_domain { s64 power_on_latency_ns; struct gpd_dev_ops dev_ops; s64 max_off_time_ns; /* Maximum allowed "suspended" time. */ + bool max_off_time_changed; + bool cached_power_down_ok; struct device_node *of_node; /* Node in device tree */ }; @@ -92,12 +95,16 @@ struct gpd_timing_data { s64 save_state_latency_ns; s64 restore_state_latency_ns; s64 effective_constraint_ns; + bool constraint_changed; + bool cached_stop_ok; }; struct generic_pm_domain_data { struct pm_domain_data base; struct gpd_dev_ops ops; struct gpd_timing_data td; + struct notifier_block nb; + struct mutex lock; bool need_restore; bool always_on; }; Index: linux/drivers/base/power/domain.c =================================================================== --- linux.orig/drivers/base/power/domain.c +++ linux/drivers/base/power/domain.c @@ -11,6 +11,7 @@ #include <linux/io.h> #include <linux/pm_runtime.h> #include <linux/pm_domain.h> +#include <linux/pm_qos.h> #include <linux/slab.h> #include <linux/err.h> #include <linux/sched.h> @@ -38,11 +39,13 @@ ktime_t __start = ktime_get(); \ type __retval = GENPD_DEV_CALLBACK(genpd, type, callback, dev); \ s64 __elapsed = ktime_to_ns(ktime_sub(ktime_get(), __start)); \ - struct generic_pm_domain_data *__gpd_data = dev_gpd_data(dev); \ - if (__elapsed > __gpd_data->td.field) { \ - __gpd_data->td.field = __elapsed; \ + struct gpd_timing_data *__td = &dev_gpd_data(dev)->td; \ + if (!__retval && __elapsed > __td->field) { \ + __td->field = __elapsed; \ dev_warn(dev, name " latency exceeded, new value %lld ns\n", \ __elapsed); \ + genpd->max_off_time_changed = true; \ + __td->constraint_changed = true; \ } \ __retval; \ }) @@ -211,6 +214,7 @@ int __pm_genpd_poweron(struct generic_pm elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start)); if (elapsed_ns > genpd->power_on_latency_ns) { genpd->power_on_latency_ns = elapsed_ns; + genpd->max_off_time_changed = true; if (genpd->name) pr_warning("%s: Power-on latency exceeded, " "new value %lld ns\n", genpd->name, @@ -247,6 +251,53 @@ int pm_genpd_poweron(struct generic_pm_d #ifdef CONFIG_PM_RUNTIME +static int genpd_dev_pm_qos_notifier(struct notifier_block *nb, + unsigned long val, void *ptr) +{ + struct generic_pm_domain_data *gpd_data; + struct device *dev; + + gpd_data = container_of(nb, struct generic_pm_domain_data, nb); + + mutex_lock(&gpd_data->lock); + dev = gpd_data->base.dev; + if (!dev) { + mutex_unlock(&gpd_data->lock); + return NOTIFY_DONE; + } + mutex_unlock(&gpd_data->lock); + + for (;;) { + struct generic_pm_domain *genpd; + struct pm_domain_data *pdd; + + spin_lock_irq(&dev->power.lock); + + pdd = dev->power.subsys_data ? + dev->power.subsys_data->domain_data : NULL; + if (pdd) { + to_gpd_data(pdd)->td.constraint_changed = true; + genpd = dev_to_genpd(dev); + } else { + genpd = ERR_PTR(-ENODATA); + } + + spin_unlock_irq(&dev->power.lock); + + if (!IS_ERR(genpd)) { + mutex_lock(&genpd->lock); + genpd->max_off_time_changed = true; + mutex_unlock(&genpd->lock); + } + + dev = dev->parent; + if (!dev || dev->power.ignore_children) + break; + } + + return NOTIFY_DONE; +} + /** * __pm_genpd_save_device - Save the pre-suspend state of a device. * @pdd: Domain data of the device to save the state of. @@ -381,7 +432,6 @@ static int pm_genpd_poweroff(struct gene return 0; } - genpd->max_off_time_ns = -1; if (genpd->gov && genpd->gov->power_down_ok) { if (!genpd->gov->power_down_ok(&genpd->domain)) return -EAGAIN; @@ -436,6 +486,7 @@ static int pm_genpd_poweroff(struct gene elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start)); if (elapsed_ns > genpd->power_off_latency_ns) { genpd->power_off_latency_ns = elapsed_ns; + genpd->max_off_time_changed = true; if (genpd->name) pr_warning("%s: Power-off latency exceeded, " "new value %lld ns\n", genpd->name, @@ -496,7 +547,6 @@ static int pm_genpd_runtime_suspend(stru if (dev_gpd_data(dev)->always_on) return -EBUSY; - dev_gpd_data(dev)->td.effective_constraint_ns = -1; stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL; if (stop_ok && !stop_ok(dev)) return -EBUSY; @@ -601,6 +651,12 @@ void pm_genpd_poweroff_unused(void) #else +static inline int genpd_dev_pm_qos_notifier(struct notifier_block *nb, + unsigned long val, void *ptr) +{ + return NOTIFY_DONE; +} + static inline void genpd_power_off_work_fn(struct work_struct *work) {} #define pm_genpd_runtime_suspend NULL @@ -1197,6 +1253,14 @@ int __pm_genpd_add_device(struct generic if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)) return -EINVAL; + gpd_data = kzalloc(sizeof(*gpd_data), GFP_KERNEL); + if (!gpd_data) + return -ENOMEM; + + mutex_init(&gpd_data->lock); + gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier; + dev_pm_qos_add_notifier(dev, &gpd_data->nb); + genpd_acquire_lock(genpd); if (genpd->status == GPD_STATE_POWER_OFF) { @@ -1215,26 +1279,35 @@ int __pm_genpd_add_device(struct generic goto out; } - gpd_data = kzalloc(sizeof(*gpd_data), GFP_KERNEL); - if (!gpd_data) { - ret = -ENOMEM; - goto out; - } - genpd->device_count++; + genpd->max_off_time_changed = true; - dev->pm_domain = &genpd->domain; dev_pm_get_subsys_data(dev); + + mutex_lock(&gpd_data->lock); + spin_lock_irq(&dev->power.lock); + dev->pm_domain = &genpd->domain; dev->power.subsys_data->domain_data = &gpd_data->base; gpd_data->base.dev = dev; - gpd_data->need_restore = false; list_add_tail(&gpd_data->base.list_node, &genpd->dev_list); + gpd_data->need_restore = false; if (td) gpd_data->td = *td; + gpd_data->td.constraint_changed = true; + gpd_data->td.effective_constraint_ns = -1; + spin_unlock_irq(&dev->power.lock); + mutex_unlock(&gpd_data->lock); + + genpd_release_lock(genpd); + + return 0; + out: genpd_release_lock(genpd); + dev_pm_qos_remove_notifier(dev, &gpd_data->nb); + kfree(gpd_data); return ret; } @@ -1278,6 +1351,7 @@ int __pm_genpd_of_add_device(struct devi int pm_genpd_remove_device(struct generic_pm_domain *genpd, struct device *dev) { + struct generic_pm_domain_data *gpd_data; struct pm_domain_data *pdd; int ret = 0; @@ -1295,14 +1369,27 @@ int pm_genpd_remove_device(struct generi goto out; } + genpd->device_count--; + genpd->max_off_time_changed = true; + + spin_lock_irq(&dev->power.lock); dev->pm_domain = NULL; pdd = dev->power.subsys_data->domain_data; list_del_init(&pdd->list_node); dev->power.subsys_data->domain_data = NULL; - dev_pm_put_subsys_data(dev); - kfree(to_gpd_data(pdd)); + spin_unlock_irq(&dev->power.lock); - genpd->device_count--; + gpd_data = to_gpd_data(pdd); + mutex_lock(&gpd_data->lock); + pdd->dev = NULL; + mutex_unlock(&gpd_data->lock); + + genpd_release_lock(genpd); + + dev_pm_qos_remove_notifier(dev, &gpd_data->nb); + kfree(gpd_data); + dev_pm_put_subsys_data(dev); + return 0; out: genpd_release_lock(genpd); @@ -1673,6 +1760,7 @@ void pm_genpd_init(struct generic_pm_dom genpd->resume_count = 0; genpd->device_count = 0; genpd->max_off_time_ns = -1; + genpd->max_off_time_changed = true; genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend; genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume; genpd->domain.ops.runtime_idle = pm_generic_runtime_idle; Index: linux/drivers/base/power/domain_governor.c =================================================================== --- linux.orig/drivers/base/power/domain_governor.c +++ linux/drivers/base/power/domain_governor.c @@ -46,18 +46,34 @@ static int dev_update_qos_constraint(str bool default_stop_ok(struct device *dev) { struct gpd_timing_data *td = &dev_gpd_data(dev)->td; + unsigned long flags; s64 constraint_ns; dev_dbg(dev, "%s()\n", __func__); - constraint_ns = dev_pm_qos_read_value(dev); + spin_lock_irqsave(&dev->power.lock, flags); + + if (!td->constraint_changed) { + bool ret = td->cached_stop_ok; + + spin_unlock_irqrestore(&dev->power.lock, flags); + return ret; + } + td->constraint_changed = false; + td->cached_stop_ok = false; + td->effective_constraint_ns = -1; + constraint_ns = __dev_pm_qos_read_value(dev); + + spin_unlock_irqrestore(&dev->power.lock, flags); + if (constraint_ns < 0) return false; constraint_ns *= NSEC_PER_USEC; /* * We can walk the children without any additional locking, because - * they all have been suspended at this point. + * they all have been suspended at this point and their + * effective_constraint_ns fields won't be modified in parallel with us. */ if (!dev->power.ignore_children) device_for_each_child(dev, &constraint_ns, @@ -69,11 +85,13 @@ bool default_stop_ok(struct device *dev) return false; } td->effective_constraint_ns = constraint_ns; + td->cached_stop_ok = constraint_ns > td->stop_latency_ns || + constraint_ns == 0; /* * The children have been suspended already, so we don't need to take * their stop latencies into account here. */ - return constraint_ns > td->stop_latency_ns || constraint_ns == 0; + return td->cached_stop_ok; } /** @@ -90,6 +108,25 @@ static bool default_power_down_ok(struct s64 min_dev_off_time_ns; s64 off_on_time_ns; + if (genpd->max_off_time_changed) { + struct gpd_link *link; + + /* + * We have to invalidate the cached results for the masters, so + * use the observation that default_power_down_ok() is not + * going to be called for any master until this instance + * returns. + */ + list_for_each_entry(link, &genpd->slave_links, slave_node) + link->master->max_off_time_changed = true; + + genpd->max_off_time_changed = false; + genpd->cached_power_down_ok = false; + genpd->max_off_time_ns = -1; + } else { + return genpd->cached_power_down_ok; + } + off_on_time_ns = genpd->power_off_latency_ns + genpd->power_on_latency_ns; /* @@ -165,6 +202,8 @@ static bool default_power_down_ok(struct min_dev_off_time_ns = constraint_ns; } + genpd->cached_power_down_ok = true; + /* * If the computed minimum device off time is negative, there are no * latency constraints, so the domain can spend arbitrary time in the ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-04-30 8:57 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-04-27 21:46 [PATCH 0/2] PM / QoS / Domains: Fix and optimization Rafael J. Wysocki 2012-04-27 21:48 ` [PATCH 1/2] PM / QoS: Create device constraints objects on notifier registration Rafael J. Wysocki 2012-04-28 15:33 ` mark gross 2012-04-28 21:10 ` Rafael J. Wysocki 2012-04-27 21:53 ` [PATCH 2/2][RFC] PM / Domains: Cache device stop and domain power off governor results Rafael J. Wysocki 2012-04-29 1:08 ` [PATCH 0/3] PM / QoS / Domains: Fixes and optimization, take 2 Rafael J. Wysocki 2012-04-29 1:11 ` [PATCH 1/3] PM / QoS: Create device constraints objects on notifier registration Rafael J. Wysocki 2012-04-30 8:57 ` Jean Pihet 2012-04-29 1:12 ` [PATCH 2/3] PM / Domains: Make device removal more straightforward Rafael J. Wysocki 2012-04-29 1:14 ` [PATCH 3/3][RFC] PM / Domains: Cache device stop and domain power off governor results, v2 Rafael J. Wysocki 2012-04-29 14:12 ` [Update][PATCH 3/3][RFC] PM / Domains: Cache device stop and domain power off governor results, v3 Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).