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

ASoC: SOF: ipc4-topology: Keep secondary core on until pipeline deletion #4692

Closed
wants to merge 1 commit into from
Closed

ASoC: SOF: ipc4-topology: Keep secondary core on until pipeline deletion #4692

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
16 changes: 16 additions & 0 deletions sound/soc/sof/ipc4-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,6 +887,7 @@ static int sof_ipc4_widget_setup_comp_process(struct snd_sof_widget *swidget)
{
struct snd_soc_component *scomp = swidget->scomp;
struct sof_ipc4_fw_module *fw_module;
struct snd_sof_widget *pipe_widget;
struct sof_ipc4_process *process;
void *cfg;
int ret;
Expand Down Expand Up @@ -943,6 +944,21 @@ static int sof_ipc4_widget_setup_comp_process(struct snd_sof_widget *swidget)

sof_ipc4_widget_update_kcontrol_module_id(swidget);

pipe_widget = swidget->spipe->pipe_widget;

/*
* A Data Processing (DP) domain widget can be scheduled to run on a secondary core while
* it's pipeline runs the primary core. In this case, the secondary core could get powered
Copy link
Member

Choose a reason for hiding this comment

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

the pipeline has nothing to do with cores. Never has, never will.

I think you meant "a data processing domain widget can be scheduled to run on a secondary core while other widgets of the same pipeline run on the primary core" ?

It would be useful to also explain why LL is not impacted, or what's so special about DP?

Copy link

Choose a reason for hiding this comment

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

Special is -
a pipeline has a core it is bind to, All LL modules that belongs to the pipeline are bind to the same core as the pipeline itself.
One of DP principals was that a single module from the pipeline can be offloaded to secondary core and executed there, i.e.

LL1 (core0) ---> LL2(core0) ---> DP1 (core1) ---> LL3 (core0) --> DP2 (core2) --> LL4 (core0)

* off before the pipeline is deleted resulting in a crash when the firmware tries to free
* the DP module during pipeline deletion. Prevent this by setting the pipe_widget's core
* ID to match that of the widget so that the core would be kept powered on until the
Copy link
Member

Choose a reason for hiding this comment

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

the pipe_widget->core is a concept that should not exist. The pipeline does not "run", only module instances are scheduled, and the scheduling does not happen at the pipeline level.

Copy link

@marcinszkudlinski marcinszkudlinski Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Pipeline core ID was added to make FW management of pipeline easier; it is just an info for FW on which core it should run pipeline management code.
EDIT: and all LLs of this pipeline should run on the same core as pipeline itself (at least for now)

Copy link
Member

Choose a reason for hiding this comment

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

EDIT: and all LLs of this pipeline should run on the same core as pipeline itself (at least for now)

We already know that changes are coming for LL on multiple cores so we don't want to have to revisit the core management multiple times.... Let's do the things right and future-proof, shall we?

* pipeline has been deleted. This would work only in the case where a pipeline only
* contains a single DP module that is scheduled on a secondary core.
Copy link
Member

Choose a reason for hiding this comment

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

This is not great....

At the very least we would need to have something that detects that there are more than 2 non-primary cores used by a pipeline. But clearly with the AEC/NS sequence it's quite likely that this configuration will happen.

But I would really not invest any time in this, it's a stop-gap which will be defeated by 3rd parties.

Copy link

Choose a reason for hiding this comment

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

I agree, a pipeline with couple of DPs spread on all cores will be needed probably very soon.

*/
if ((fw_module->man4_module_entry.type & SOF_IPC4_MODULE_DP) && !pipe_widget->core &&
swidget->core)
pipe_widget->core = swidget->core;

return 0;
free_base_cfg_ext:
kfree(process->base_config_ext);
Expand Down