Skip to content
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.

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

PM policy: Separate default policy and events #80576

Conversation

bjarki-andreasen
Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen commented Oct 29, 2024

The default policy currently directly references the private variable next_event from policy_events.c:

extern struct pm_policy_event *next_event;

to then convert the cycle of said event (if exists) to a cycle in the future:

if ((next_event) && (next_event->value_cyc >= 0)) {
uint32_t cyc_curr = k_cycle_get_32();
int64_t cyc_evt = next_event->value_cyc - cyc_curr;
/* event happening after cycle counter max value, pad */
if (next_event->value_cyc <= cyc_curr) {
cyc_evt += UINT32_MAX;
}
if (cyc_evt > 0) {
/* if there's no system wakeup event always wins,
* otherwise, who comes earlier wins
*/
if (cyc < 0) {
cyc = cyc_evt;
} else {
cyc = MIN(cyc, cyc_evt);
}
}
}

something policy_events.c already implements quite similarly and exposes through pm_policy_next_event_ticks():

int32_t pm_policy_next_event_ticks(void)
{
int32_t cyc_evt = -1;
if ((next_event) && (next_event->value_cyc > 0)) {
cyc_evt = next_event->value_cyc - k_cycle_get_32();
cyc_evt = MAX(0, cyc_evt);
BUILD_ASSERT(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC >= CONFIG_SYS_CLOCK_TICKS_PER_SEC,
"HW Cycles per sec should be greater that ticks per sec");
return k_cyc_to_ticks_floor32(cyc_evt);
}
return -1;
}

Additionally, the implementation of pm_policy_next_state() in policy_default.c already gets the nearest kernel tick, wherein the next event has already been accounted for, see implementation of pm_system_suspend():

zephyr/subsys/pm/pm.c

Lines 155 to 156 in e1389fa

events_ticks = pm_policy_next_event_ticks();
ticks = ticks_expiring_sooner(kernel_ticks, events_ticks);

info = pm_policy_next_state(id, ticks);

This commit removes the redundant and layer violating computation of the tick of the next event from policy_default.c and updates the test test_pm_policy_events to not use default policy to determine if pm_policy_next_event_ticks() is correct.

This PR updates the test suite to actually be able to detect the incorrect argument handling of pm_policy_event_register() fixed with PR #80556 so this PR includes said fix, to be dropped from here when fix is merged :)

The pm_policy_event_register() API takes absolute cycles as the
second arg, like pm_policy_event_update(), but the arg is renamed
time_us and treated as a relative time in us rather than abs
cycles.

Fix implementation of pm_policy_event_register() to treat cycles
like pm_policy_event_update() and API docs suggest.

Signed-off-by: Bjarki Arge Andreasen <[email protected]>
The default policy currently directly references the private
variable next_event from policy_events.c to then convert the cycle
of said event (if exists) to a kernel tick in the future, something
policy_events.c already implements and exposes through
pm_policy_next_event_ticks().

Additionally, the implementation of pm_policy_next_state() in
policy_default.c already gets the nearest kernel tick, wherein
the next event has already been accounted for in, see
implementation of pm_system_suspend().

This commit removes the redundant and layer violating computation
if the tick of the next event from policy_default.c and updates
the test test_pm_policy_events to not use default policy to
determine if pm_policy_next_event_ticks() is correct.

Signed-off-by: Bjarki Arge Andreasen <[email protected]>
@bjarki-andreasen bjarki-andreasen force-pushed the separate-default-policy-and-events branch from 3e63413 to 332c07e Compare October 29, 2024 16:24
@bjarki-andreasen
Copy link
Collaborator Author

added test for pm_policy_event_update()

Copy link
Member

@ceolin ceolin left a comment

Choose a reason for hiding this comment

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

Thanks again, same thing again, recent change makes subsystem to account for event out of the policy but the policy was not updated. My bad.

It looks legit.

@nashif nashif merged commit 14117b4 into zephyrproject-rtos:main Nov 16, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants