Skip to content

pm: policy: latency: Add support for immediate action #81856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nordic-krch wants to merge 2 commits into from
Closed

pm: policy: latency: Add support for immediate action #81856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b995c2c
pm: policy: latency: Add support for immediate action
nordic-krch Nov 25, 2024
e7dd4c6
tests: pm: policy_api: Add test for latency immediate action
nordic-krch Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 101 additions & 31 deletions include/zephyr/pm/policy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <zephyr/device.h>
#include <zephyr/pm/state.h>
#include <zephyr/sys/slist.h>
#include <zephyr/sys/onoff.h>
#include <zephyr/toolchain.h>

#ifdef __cplusplus
Expand All @@ -27,37 +28,29 @@
*/

/**
* @brief Callback to notify when maximum latency changes.
*
* @param latency New maximum latency. Positive value represents latency in
* microseconds. SYS_FOREVER_US value lifts the latency constraint. Other values
* are forbidden.
*/
typedef void (*pm_policy_latency_changed_cb_t)(int32_t latency);

/**
* @brief Latency change subscription.
* @brief Latency request.
*
* @note All fields in this structure are meant for private usage.
*/
struct pm_policy_latency_subscription {
struct pm_policy_latency_request {
/** @cond INTERNAL_HIDDEN */
sys_snode_t node;
pm_policy_latency_changed_cb_t cb;
uint32_t value_us;
struct onoff_client cli;
void *internal;
uint32_t flags;
Comment on lines +39 to +41
Copy link
Member

@hubertmis hubertmis Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onoff_client and two 32-bit variables per latency requestor seem like high memory overhead. Wouldn't it be enough to track these data per subscriber instead of per requestor?

Copy link
Contributor Author

@nordic-krch nordic-krch Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if immediate action/subscription is needed then it is required. We can add kconfig option which enables that feature and have those fields conditionally compiled.

/** @endcond */
};

/**
* @brief Latency request.
* @brief Callback to notify when maximum latency changes.
*
* @note All fields in this structure are meant for private usage.
* @param latency New maximum latency. Positive value represents latency in
* microseconds. SYS_FOREVER_US value lifts the latency constraint. Other values
* are forbidden.
*/
struct pm_policy_latency_request {
/** @cond INTERNAL_HIDDEN */
sys_snode_t node;
uint32_t value_us;
/** @endcond */
};
typedef void (*pm_policy_latency_changed_cb_t)(struct pm_policy_latency_request *req,
int32_t latency);

/**
* @brief Event.
Expand Down Expand Up @@ -275,63 +268,140 @@
* The system will not enter any power state that would make the system to
* exceed the given latency value.
*
* If immediate control manager is provided request may trigger an action asynchronous action
Copy link
Member

@hubertmis hubertmis Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the "immediate control manager"? I didn't find the term in Zephyr docs.

Copy link
Contributor Author

@nordic-krch nordic-krch Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it refers to pm_policy_latency_immediate_ctrl_add

* to reconfigure the system runtime to fullfil requested latency. @p req contains an
* asynchronous notification field that must be initialize prior to the call to get the
* notification.
*
* @param req Latency request.
* @param value_us Maximum allowed latency in microseconds.
*
* @retval 0 if request is applied.
* @retval 1 if request required immediate action that is not completed. Configured completion
* notification will inform about completion.
Comment on lines +279 to +281
Copy link
Member

@hubertmis hubertmis Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In async functions, I would expect that the callback is called regardless of the returned value. So this could be simplified to retval 0 if the request is pending. When the callback is applied, the callback notification is called with the result.

Copy link
Contributor Author

@nordic-krch nordic-krch Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied it from onoff manager which notifies regardless of current state but returns current state. User may choose to not use this information with if (request_add(...) < 0) error but if needed information is there. For example if signal or semaphore is used in the callback then checking return value may be faster.

err = request_add(...);
if (err == 0) {
  //do nothing
} else if (err == 1) {
  k_sem_take(&sem, K_MSEC);
} else {
   //error
}

* @retval -EIO immediate action returned error.
*/
int pm_policy_latency_request_add(struct pm_policy_latency_request *req, uint32_t value_us);

/**
* @brief Synchronously add a new latency requirement.
*
* If immediate control manager is not used then it is an equivalent of
* @ref pm_policy_latency_request_add. If immediate control manager is used then context
* blocks until request is completed. It can only be called from the thread context.
* Asynchronous notification field in the @p req is configured and used internally.
*
* @retval 0 if request required immediate action that is not completed. Configured completion
* notification will inform about completion.
* @retval -EIO immediate action returned error.
*/
void pm_policy_latency_request_add(struct pm_policy_latency_request *req,
uint32_t value_us);
int pm_policy_latency_request_add_sync(struct pm_policy_latency_request *req, uint32_t value_us);
Copy link
Member

@hubertmis hubertmis Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say the default function should be synchronous (without a suffix) and the special one, callable from IRQs, should be _async().


/**
* @brief Update a latency requirement.
*
* @param req Latency request.
* @param value_us New maximum allowed latency in microseconds.
*
* @retval 0 if request is applied.
* @retval 1 if request required immediate action that is not completed. Configured completion
* notification will inform about completion.
* @retval -EIO immediate action returned error.
*/
int pm_policy_latency_request_update(struct pm_policy_latency_request *req, uint32_t value_us);

/**
* @brief Synchronously update a latency requirement.
*
* If immediate control manager is not used then it is an equivalent of
* @ref pm_policy_latency_request_update. If immediate control manager is used then context
* blocks until request is completed. It can only be called from the thread context.
* Asynchronous notification field in the @p req is configured and used internally.
*
* @retval 0 if request required immediate action that is not completed. Configured completion
* notification will inform about completion.
* @retval -EIO immediate action returned error.
*/
void pm_policy_latency_request_update(struct pm_policy_latency_request *req,
uint32_t value_us);
int pm_policy_latency_request_update_sync(struct pm_policy_latency_request *req, uint32_t value_us);

/**
* @brief Remove a latency requirement.
*
* @param req Latency request.
*
* @retval 0 if request removed successfully.
* @retval -EALREADY if request is not active and cannot be removed.
* @retval -EIO immediate action returned error.
*/
int pm_policy_latency_request_remove(struct pm_policy_latency_request *req);

/** @brief Immediate action manager for single threshold.
Copy link
Member

@hubertmis hubertmis Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "immediate" word used around max latency changes subscription is confusing to me. Is there any "delayed max latency changes subscription" to differentiate the "immediate" one from?

Maybe it would be enough to name this structure pm_policy_latency_change_binary_subscription?

Copy link
Contributor Author

@nordic-krch nordic-krch Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when latency is used only to select sleep state when entering idle then there is no immediate action. Lowest requested latency can change multiple times and nothing happens as long as cpu does not go to idle. "Immediate" means that when new lowest latency is selected it triggers immediate asynchronous action.

Copy link
Member

@hubertmis hubertmis Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully agree. When latency is used to select a CPU sleep state, the immediate action is locking a set of CPU sleep states that are characterized by greater latency. I agree, that the visible effect of this action is only when the CPU enters the idle thread, but we can treat locking as an immediate action.

The same way with NVM controllers: the immediate action is locking their sleep states that are characterized by greater latency. But this action has visible effect after the NVM inactivity timeout fires.

So, you can treat both of these actions as "immediate" - locking states are immediate in both, or as "deferred" - the actual sleep states are entered after inactivity is detected.

The main difference between these two is that CPU-idle handling is CPU-specific, while NVM-idle handling is SoC-specific or even PCB-specific if an NVM controller is external.

*
* If there is only a single threshold that triggers an action then @ref onoff_manager
* can handle that. Structure must be persistent.
*/
void pm_policy_latency_request_remove(struct pm_policy_latency_request *req);
struct pm_policy_latency_immediate_binary {
/** Onoff manager. */
struct onoff_manager *mgr;
/** Latency threshold. Value below threshold trigger 'on' action. */
int32_t thr;
};

struct pm_policy_latency_immediate_ctrl {
Copy link
Member

@hubertmis hubertmis Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this one pm_policy_latency_change_subscription?

/** Indication that onoff manager is used. In future gradual manager might
* also be supported.
*/
bool onoff;
/** Pointer to the manager which controls immediate action. */
union {
struct pm_policy_latency_immediate_binary *bin_mgr;
void *mgr;
};
};

/**
* @brief Subscribe to maximum latency changes.
*
* @param req Subscription request.
* @param cb Callback function (NULL to disable).
*/
void pm_policy_latency_changed_subscribe(struct pm_policy_latency_subscription *req,
pm_policy_latency_changed_cb_t cb);
int pm_policy_latency_immediate_ctrl_add(struct pm_policy_latency_immediate_ctrl *mgr);
Copy link
Member

@hubertmis hubertmis Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And restore the name of this function? The previous one was much clearer to me.

Copy link
Contributor Author

@nordic-krch nordic-krch Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, subscriber was getting a callback when latency changed. This module was not aware of what is done in the callback. Proposed patch changes it. When latency changes that triggers an asynchronous immediate action (in contrast to latency being only used when entering idle so there is no action as long as cpu does not go to idle). Imo, subscription name would be misleading now.


/**
* @brief Unsubscribe to maximum latency changes.
*
* @param req Subscription request.
*/
void pm_policy_latency_changed_unsubscribe(struct pm_policy_latency_subscription *req);
#else
static inline void pm_policy_latency_request_add(
static inline int pm_policy_latency_request_add(
struct pm_policy_latency_request *req, uint32_t value_us)
{
ARG_UNUSED(req);
ARG_UNUSED(value_us);
return 0;
}

static inline void pm_policy_latency_request_update(
static inline int pm_policy_latency_request_update(
struct pm_policy_latency_request *req, uint32_t value_us)
{
ARG_UNUSED(req);
ARG_UNUSED(value_us);
return 0;
}

static inline void pm_policy_latency_request_remove(
static inline int pm_policy_latency_request_remove(
struct pm_policy_latency_request *req)
{

Check notice on line 394 in include/zephyr/pm/policy.h

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

include/zephyr/pm/policy.h:394 
-static inline int pm_policy_latency_request_add(
-	struct pm_policy_latency_request *req, uint32_t value_us)
+static inline int pm_policy_latency_request_add(struct pm_policy_latency_request *req,
+						uint32_t value_us)
 {
 	ARG_UNUSED(req);
 	ARG_UNUSED(value_us);
 	return 0;
 }
 
-static inline int pm_policy_latency_request_update(
-	struct pm_policy_latency_request *req, uint32_t value_us)
+static inline int pm_policy_latency_request_update(struct pm_policy_latency_request *req,
+						   uint32_t value_us)
 {
 	ARG_UNUSED(req);
 	ARG_UNUSED(value_us);
 	return 0;
 }
 
-static inline int pm_policy_latency_request_remove(
-	struct pm_policy_latency_request *req)
+static inline int pm_policy_latency_request_remove(struct pm_policy_latency_request *req)
ARG_UNUSED(req);
return 0;
}

static inline int pm_policy_latency_immediate_ctrl_add(struct pm_policy_latency_immediate_ctrl *mgr)
{
ARG_UNUSED(mgr);
return 0;
}

#endif /* CONFIG_PM CONFIG_PM_POLICY_LATENCY_STANDALONE */

/**
Expand Down
9 changes: 0 additions & 9 deletions samples/subsys/pm/latency/sample.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,34 +25,25 @@ tests:
- "<inf> app: Sleeping for 1.3 seconds, we should enter SUSPEND_TO_IDLE"
- "<inf> soc_pm: Entering SUSPEND_TO_IDLE{0}"
- "<inf> app: Opening test device"
- "<inf> dev_test: Adding latency constraint: 20ms"
- "<inf> app: Latency constraint changed: 20ms"
- "<inf> app: Sleeping for 1.1 seconds, we should enter RUNTIME_IDLE"
- "<inf> soc_pm: Entering RUNTIME_IDLE{0}"
- "<inf> app: Sleeping for 1.2 seconds, we should enter RUNTIME_IDLE"
- "<inf> soc_pm: Entering RUNTIME_IDLE{0}"
- "<inf> app: Sleeping for 1.3 seconds, we should enter RUNTIME_IDLE"
- "<inf> soc_pm: Entering RUNTIME_IDLE{0}"
- "<inf> app: Updating latency constraint: 10ms"
- "<inf> app: Latency constraint changed: 10ms"
- "<inf> dev_test: Latency constraint changed: 10ms"
- "<inf> app: Sleeping for 1.1 seconds, we should stay ACTIVE"
- "<inf> app: Sleeping for 1.2 seconds, we should stay ACTIVE"
- "<inf> app: Sleeping for 1.3 seconds, we should stay ACTIVE"
- "<inf> app: Updating latency constraint: 30ms"
- "<inf> app: Latency constraint changed: 20ms"
- "<inf> dev_test: Latency constraint changed: 20ms"
- "<inf> app: Closing test device"
- "<inf> dev_test: Removing latency constraint"
- "<inf> app: Latency constraint changed: 30ms"
- "<inf> dev_test: Latency constraint changed: 30ms"
- "<inf> app: Sleeping for 1.1 seconds, we should enter RUNTIME_IDLE"
- "<inf> soc_pm: Entering RUNTIME_IDLE{0}"
- "<inf> app: Sleeping for 1.2 seconds, we should enter SUSPEND_TO_IDLE"
- "<inf> soc_pm: Entering SUSPEND_TO_IDLE{0}"
- "<inf> app: Sleeping for 1.3 seconds, we should enter SUSPEND_TO_IDLE"
- "<inf> soc_pm: Entering SUSPEND_TO_IDLE{0}"
- "<inf> app: Removing latency constraint"
- "<inf> app: Latency constraint changed: none"
- "<inf> app: Finished, we should now enter STANDBY"
- "<inf> soc_pm: Entering STANDBY{0}"
15 changes: 0 additions & 15 deletions samples/subsys/pm/latency/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,8 @@

LOG_MODULE_REGISTER(app, CONFIG_APP_LOG_LEVEL);

static void on_latency_changed(int32_t latency)
{
if (latency == SYS_FOREVER_US) {
LOG_INF("Latency constraint changed: none");
} else {
LOG_INF("Latency constraint changed: %" PRId32 "ms",
latency / USEC_PER_MSEC);
}
}

int main(void)
{
struct pm_policy_latency_subscription subs;
struct pm_policy_latency_request req;
const struct device *dev;

Expand All @@ -35,8 +24,6 @@ int main(void)
return 0;
}

pm_policy_latency_changed_subscribe(&subs, on_latency_changed);

/* test without any latency constraint */
LOG_INF("Sleeping for 1.1 seconds, we should enter RUNTIME_IDLE");
k_msleep(1100);
Expand Down Expand Up @@ -101,8 +88,6 @@ int main(void)
LOG_INF("Removing latency constraints");
pm_policy_latency_request_remove(&req);

pm_policy_latency_changed_unsubscribe(&subs);

LOG_INF("Finished, we should now enter STANDBY");
return 0;
}
15 changes: 0 additions & 15 deletions samples/subsys/pm/latency/src/test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,28 +13,15 @@
LOG_MODULE_REGISTER(dev_test, CONFIG_APP_LOG_LEVEL);

struct dev_test_data {
struct pm_policy_latency_subscription subs;
struct pm_policy_latency_request req;
};

static void on_latency_changed(int32_t latency)
{
if (latency == SYS_FOREVER_US) {
LOG_INF("Latency constraint changed: none");
} else {
LOG_INF("Latency constraint changed: %" PRId32 "ms",
latency / USEC_PER_MSEC);
}
}

static void dev_test_open(const struct device *dev)
{
struct dev_test_data *data = dev->data;

LOG_INF("Adding latency constraint: 20ms");
pm_policy_latency_request_add(&data->req, 20000);

pm_policy_latency_changed_subscribe(&data->subs, on_latency_changed);
}

static void dev_test_close(const struct device *dev)
Expand All @@ -43,8 +30,6 @@ static void dev_test_close(const struct device *dev)

LOG_INF("Removing latency constraint");
pm_policy_latency_request_remove(&data->req);

pm_policy_latency_changed_unsubscribe(&data->subs);
}

static const struct test_api dev_test_api = {
Expand Down
5 changes: 5 additions & 0 deletions subsys/pm/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ config PM_NEED_ALL_DEVICES_IDLE
When this option is enabled, check that no devices are busy before
entering into system low power mode.

config PM_POLICY_LATENCY_IMMEDIATE_BIN_ACTION
bool "Support latency change action"
select ONOFF
imply POLL

endif # PM

config PM_DEVICE
Expand Down
Loading
Loading