Re: [PATCH v3 4/4] pmdomain: add support system-wide resume latency constraints

[Kendall Willis]

Hi Kevin,
On 12:59-20260611, Kevin Hilman (TI) wrote:
> In addition to checking for CPU latency constraints when checking if
> OK to power down a domain, also check for QoS latency constraints in
> all devices of a domain and use that in determining the final latency
> constraint to use for the domain.
> 
> Since cpu_system_power_down_ok() is used for system-wide suspend, the
> per-device constratints are only relevant if the LATENCY_SYS QoS flag
> is set.

nit: s/constratints/constraints

> 
> Signed-off-by: Kevin Hilman (TI) <khilman@xxxxxxxxxxxx>
> ---

Reviewed-by: Kendall Willis <k-willis@xxxxxx>

Best,
Kendall

>  drivers/pmdomain/governor.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/pmdomain/governor.c b/drivers/pmdomain/governor.c
> index 96737abbb496..1a85fd375db9 100644
> --- a/drivers/pmdomain/governor.c
> +++ b/drivers/pmdomain/governor.c
> @@ -13,6 +13,8 @@
>  #include <linux/cpumask.h>
>  #include <linux/ktime.h>
>  
> +#include "core.h"
> +
>  static int dev_update_qos_constraint(struct device *dev, void *data)
>  {
>  	s64 *constraint_ns_p = data;
> @@ -425,17 +427,71 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd)
>  	return true;
>  }
>  
> +/**
> + * check_device_qos_latency - Callback to check device QoS latency constraints
> + * @dev: Device to check
> + * @data: Pointer to s32 variable holding minimum latency found so far
> + *
> + * This callback checks if the device has a system-wide resume latency QoS
> + * constraint and updates the minimum latency if this device has a stricter
> + * constraint.
> + *
> + * This runs in atomic context: for a CPU domain the genpd lock is a raw
> + * spinlock and the s2idle path runs in the syscore suspend window with
> + * interrupts disabled.  The lockless dev_pm_qos_raw_*() accessors must
> + * therefore be used here; the locked dev_pm_qos_read_value() /
> + * dev_pm_qos_flags() would take dev->power.lock, which is a sleeping lock
> + * on PREEMPT_RT and must not be acquired in this context.  The values read
> + * are best-effort, which matches the sibling cpu_power_down_ok() governor.
> + *
> + * The system-wide flag is checked first so that devices that have not opted
> + * in only incur a single lockless read.
> + *
> + * Returns: 0 to continue iteration.
> + */
> +static int check_device_qos_latency(struct device *dev, void *data)
> +{
> +	s32 *min_dev_latency = data;
> +	s32 dev_latency;
> +
> +	if (!(dev_pm_qos_raw_flags(dev) & PM_QOS_FLAG_LATENCY_SYS))
> +		return 0;
> +
> +	dev_latency = dev_pm_qos_raw_resume_latency(dev);
> +	if (dev_latency != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT) {
> +		dev_dbg(dev,
> +			"has QoS system-wide resume latency=%d\n",
> +			dev_latency);
> +		if (dev_latency < *min_dev_latency)
> +			*min_dev_latency = dev_latency;
> +	}
> +
> +	return 0;
> +}
> +
>  static bool cpu_system_power_down_ok(struct dev_pm_domain *pd)
>  {
>  	s64 constraint_ns = cpu_wakeup_latency_qos_limit() * NSEC_PER_USEC;
>  	struct generic_pm_domain *genpd = pd_to_genpd(pd);
>  	int state_idx = genpd->state_count - 1;
> +	s32 min_dev_latency = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> +	s64 min_dev_latency_ns = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS;
>  
>  	if (!(genpd->flags & GENPD_FLAG_CPU_DOMAIN)) {
>  		genpd->state_idx = state_idx;
>  		return true;
>  	}
>  
> +	genpd_for_each_child(genpd, check_device_qos_latency,
> +			     &min_dev_latency);
> +
> +	/* If device latency < CPU wakeup latency, use it instead */
> +	if (min_dev_latency != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT) {
> +		min_dev_latency_ns = min_dev_latency * NSEC_PER_USEC;
> +		if (min_dev_latency_ns < constraint_ns)
> +			constraint_ns = min_dev_latency_ns;
> +	}
> +
>  	/* Find the deepest state for the latency constraint. */
>  	while (state_idx >= 0) {
>  		s64 latency_ns = genpd->states[state_idx].power_off_latency_ns +
> 
> -- 
> 2.47.3
> 
>