Skip to content

Conversation

@RanderWang
Copy link

No description provided.

@RanderWang
Copy link
Author

RanderWang commented Feb 17, 2023

updated, thanks. Let's wait for thesofproject/sof#7108. I don't think it is caused by driver since core 1 is maintained by dsp fw and no such issue happen on tgl platform, but it is better to wait fw ready.

@RanderWang RanderWang force-pushed the ipc4-multi-core branch 2 times, most recently from 057366b to 2aa6159 Compare March 15, 2023 13:36
@RanderWang
Copy link
Author

@plbossart @ranj063 Since FW has fixed multi-core issue, I refine this PR according to comments.

@ranj063
Copy link
Collaborator

ranj063 commented Mar 17, 2023

@plbossart @ranj063 Since FW has fixed multi-core issue, I refine this PR according to comments.

@RanderWang is the FW fix on main yet?

@RanderWang
Copy link
Author

@plbossart @ranj063 Since FW has fixed multi-core issue, I refine this PR according to comments.

@RanderWang is the FW fix on main yet?

@ranj063 The fixed is not included by main now and I validated this PR on mtl-004 since it included the fix. And topology change is not submitted so no regression issue will happen now.

Copy link
Collaborator

@ranj063 ranj063 left a comment

Choose a reason for hiding this comment

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

just a minor nit-pick about using inclusive terms in the commit message @RanderWang

Set primary core mask and refcount.

Signed-off-by: Rander Wang <rander.wang@intel.com>
In core_get case, driver can power up primary core and don't need to send
ipc message to fw. Non-primary core should be powered up by fw with ipc
message.

In core_put case, driver should first send ipc message to fw to disable dsp
core then power down primary core if the target is primary core.

Signed-off-by: Rander Wang <rander.wang@intel.com>
Firmware may do context saving before powering off primary core, so driver
needs to send ipc msg by set_core_state. In IPC4 path, firmware needs to
save current context to IMR before powering off primary core. Firmware
does nothing for set_core_state message in IPC3 path. So IPC4 and IPC3
can share the same operation sequence.

Signed-off-by: Rander Wang <rander.wang@intel.com>
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

Thanks @RanderWang

The only open I have is "how is this tested"? It's not clear to me if we have any example topologies with multi-core nor if the firmware actually supports multi-core for now.

@RanderWang
Copy link
Author

RanderWang commented Mar 29, 2023

Thanks @RanderWang

The only open I have is "how is this tested"? It's not clear to me if we have any example topologies with multi-core nor if the firmware actually supports multi-core for now.

The change in topology is very simple, just add "core_id 1" to each module and "core 1" to pipeline in a simple pass-through stream and then the stream will work on core 1 which was confirmed by IPC message. At first I found fw crash in a few cycles test. FW team found dsp context was not maintained correctly. They fixed the issue in both cSOF FW and zephyr. These patches were merged in MTL-004 branch, but are still reviewed in main branch. I validated it with MTL-004 branch.

@plbossart
Copy link
Member

So to be clear @RanderWang, there is no risk with merging this PR. It's only when we will have a modified topology used by CI that we will see potential issues.

Did I get this right? Thanks!

Copy link
Collaborator

@ranj063 ranj063 left a comment

Choose a reason for hiding this comment

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

just one nit-pick. The second patch has a typo in teh commit title. Something to fix when we upstream or do the next upstream merge @ujfalusi FYI

@RanderWang
Copy link
Author

s

So to be clear @RanderWang, there is no risk with merging this PR. It's only when we will have a modified topology used by CI that we will see potential issues.

Did I get this right? Thanks!

yes, exactly

@plbossart plbossart merged commit 4bf8b69 into thesofproject:topic/sof-dev Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants