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.

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

Revert "ipc4: relax the IPC timeout checks and be nicer to other thre… #8756

Merged
merged 1 commit into from
Jan 25, 2024
+2 −3
Merged

Revert "ipc4: relax the IPC timeout checks and be nicer to other thre… #8756

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 2 additions & 3 deletions src/ipc/ipc4/handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,13 +510,12 @@ static void ipc_compound_msg_done(uint32_t msg_id, int error)
}
}

/* wait for IPCs to complete on other cores and be nice to any LL work */
static int ipc_wait_for_compound_msg(void)
{
int try_count = 30; /* timeout out is 30 x 10ms so 300ms for IPC */
int try_count = 30;

while (atomic_read(&msg_data.delayed_reply)) {
k_sleep(Z_TIMEOUT_MS(10));
k_sleep(Z_TIMEOUT_US(250));
Copy link
Contributor

Choose a reason for hiding this comment

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

btian1

250 --> 10ms? it is 40 times enlarge? may miss something during sleep? how about 1ms?

above is the comments I added when the change first submitted, my question is: how 250us * 30 = 7.5ms comes from?

is it based on experience value to make total wait time < 10ms?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is set by experiments with test and log. Most waiting time is less than 1ms. I don't want to use 1ms since it will miss one schedule period in FW.

Copy link
Member

Choose a reason for hiding this comment

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

What context is this called from ? My expectation is the IPC thread and if so not sure how we can block LL from running ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lgirdwood This is the IPC thread. It doesn't block the LL thread, but it will block processing of IPC messages until the LL timer fires next time, runs the IPC processing (that needs to happen in LL context), acks it as done, and then the IPC thread can continue. Now this adds a mandatory 10ms delay to each such handover from IPC thread to LL thread.

Copy link
Member

Choose a reason for hiding this comment

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

ok, so we have a timing dependency between LL thread and IPC thread and this is convergence. This need to be spelled out in the inline comments alongside setting a better tries count as 30 means about 7.5 LL ticks. i.e. we need to say > 1.5 LL ticks would be a significant failure

Copy link
Contributor

Choose a reason for hiding this comment

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

@kv2019i , @RanderWang is it better if 150 * 50? to make IPC response more quick?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lgirdwood This was the mechanism added by @lyakh to synchronize pipeline trigger processing with LL thread (so that pipeline state changes are not done async with the pipeline exection). @lgirdwood @btian1 We can improve the logic (or even devise a different way to wait for next ll tick, I think the original concern of trying to avoid unnecessary thread context switches is as valid as ever), but can we do that as a follow-up and merge this plain revert first? This helps with multiple pressing issues (AV sync and this seems to help with multicore scenarios), and we have done significant testing of this version with @RanderWang . If we want a modified new mechanism, that needs to be tested well, pushing the merge out.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with a revert, but we need a folloup PR that spells out the relationships with some inline commentary.


if (!try_count--) {
atomic_set(&msg_data.delayed_reply, 0);
Expand Down
Loading