pm: policy: latency: Add support for immediate action #81856
Conversation
nordic-krch
force-pushed
the
mram_latency
branch
from
cd1e954 to
e7dd4c6
Compare
hubertmis
left a comment
hubertmis
left a comment
There was a problem hiding this comment.
I reviewed only the policy.h API. I think we should decide on changes there first.
include/zephyr/pm/policy.h
Outdated
There was a problem hiding this comment.
What's the "immediate control manager"? I didn't find the term in Zephyr docs.
There was a problem hiding this comment.
it refers to pm_policy_latency_immediate_ctrl_add
include/zephyr/pm/policy.h
Outdated
There was a problem hiding this comment.
I would say the default function should be synchronous (without a suffix) and the special one, callable from IRQs, should be _async().
include/zephyr/pm/policy.h
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
}
include/zephyr/pm/policy.h
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| struct onoff_client cli; | ||
| void *internal; | ||
| uint32_t flags; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| int32_t thr; | ||
| }; | ||
|
|
||
| struct pm_policy_latency_immediate_ctrl { |
There was a problem hiding this comment.
And this one pm_policy_latency_change_subscription?
| */ | ||
| 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); |
There was a problem hiding this comment.
And restore the name of this function? The previous one was much clearer to me.
There was a problem hiding this comment.
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.
Remove API for subscription to the latency change. This API was added to support executing immediate action on latency change but implementation was calling subscriber callbacks with interrupts locked which is not desirable. In general, it is non-trivial task to manage multiple requests and perform asynchronous action for the top request. On-off manager implements that for binary case. Additionally, API was missing a notifiction to the requestor when immediate action is completed and it is required for the used to know when requested latency requirement is fulfilled. Proposed API is adding a possible notification for request and update and adds option to add a manager for controlling immediate action. Added notifications make sense only if immediate action is used. Immediate action implements support for binary case where there is a single action that is performed when latency is below a given threshold. In future it can be extended to support gradual manager. Signed-off-by: Krzysztof Chruściński <[email protected]>
Add test for the case where latency change triggers an immediate action. Configuration is using a single threshold for triggering of the action. Signed-off-by: Krzysztof Chruściński <[email protected]>
|
For context, there is this PR #80580 which updates the behavior of the latency to be "sticky", meaning if the event time is set to a value less than current time, the latency request is immediately applied. Removing the callback I'm all for, since I have not found a usecase for it either :) Lastly, I'm working on a "competing"/parallel solution to this API which should cover the features added in this PR as well, it may make sense to wait for it to be ready for review to compare them (Draft PR within this week) :) |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
3 participants
Remove API for subscription to the latency change. This API was added to support executing immediate action on latency change but implementation was calling subscriber callbacks with interrupts locked which is not desirable. In general, it is non-trivial task to manage multiple requests and perform asynchronous action for the top request. For example, on-off manager implements that for binary case.
Additionally, API was missing a notification to the requester when immediate action is completed and it is required for the user to know when requested latency requirement is fulfilled.
Proposed API is adding a possible notification for request and update and adds option to add a manager for controlling immediate action. Added notifications make sense only if immediate action is used. Immediate action implements support for binary case where there is a single action that is performed when latency is below a given threshold. In future it can be extended to support gradual manager.
Latency policy API was added #42040 #48476 but never used beyond test and sample. One of the use cases is MRAM latency management (#81381) and current API could not handle that case for reasons described above.