-
Notifications
You must be signed in to change notification settings - Fork 349
Multicore IPC #4469
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
Multicore IPC #4469
Conversation
IPC task completion notifies the host CPU about the completion of IPC processing. Move powering down DSP there to properly inform the host before disabling the DSP. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Currently when an IPC message arrives, the primary core receives the interrupt, schedules a task and begins processing the message. If it identifies, that the message is for another core, it sends an IDC message to it and waits in a busy loop until the other core completes processing the message and writes a response into the mailbox. After that the primary core notifies the host about completing the processing. This patch lets the processing secondary core notify the host directly, which also frees the primary core to handle other tasks instead of waiting in a busy loop. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
The single CI alsabat failure on ADL seems to be unrelated |
| /* no return - memory will be powered off and IPC sent */ | ||
| platform_pm_runtime_power_off(); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lyakh this part was specifically written this way by @mmaka1. IIRC correctly sending the host notification first before powering off caused some issues back in the CML days. Thats why we power off the memory and then send the notification to the host. And we ensure not to check the reply for the CTX_SAVE IPC because the memory is already power gated for this case. @mmaka1 can you please confirm if this change is OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in any case, what we could do is move the power_off to before the ipc_write to keep the same sequence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that patch isn't crucial for this series. If it's wrong, we can easily drop it or I can move it out to a separate PR for more thorough consideration. FWIW all tests passed so far, except one, likely unrelated. And if it is indeed wrong, we might want to add a test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree. this patch is not that critical. The CTX_SAVE IPC is the only one that will be power gating the memory and it should never be executing on a secondary core in the first place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ranj063 so what do we do with this? @mmaka1 ? @lgirdwood any ideas?
lgirdwood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good improvement !
| * because currently that's the only field that is modified at run-time, | ||
| * but flushing the complete cache is more future-proof. | ||
| */ | ||
| dcache_invalidate_region(ipc, sizeof(*ipc)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does ipc point to the mailbox or to a local core 0 copy of the mailbox ? the latter will need a wb+inv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lgirdwood ipc points to a global IPC context and it's allocated as "shared," so, actually no manual cache-management is required for it. Sorry, I actually forgot about this PR and instead fixed this commit in #4471 - let's close this PR and continue there.
|
I first intended to keep this PR small and merge it quickly enough, but since it hasn't happened until now, let's continue in #4471 instead, which is based on this one and contains further SMP development. |
Currently when an IPC message arrives, the primary core receives the interrupt, schedules a task and begins processing the message. If it identifies, that the message is for another core, it sends an IDC message to it and waits in a busy loop until the other core completes processing the message and writes a response into the mailbox. After that the primary core notifies the host about completing the processing. This patch lets the processing secondary core notify the host directly, which also frees the primary core to handle other tasks instead of waiting in a busy loop.