Skip to content

ipc4: multi pipeline set state on different cores - #7962

Closed
abonislawski wants to merge 2 commits into
thesofproject:mainfrom
abonislawski:multi_set_state
Closed

abonislawski wants to merge 2 commits into
thesofproject:mainfrom
abonislawski:multi_set_state

Conversation

@abonislawski

Copy link
Copy Markdown
Member

This will allow to correctly set multiple pipelines state even if they are allocated on different cores.

ipc4_set_pipeline_state will check if several cores are involved

  • set ppl state if only current core requested
  • process IPC on another core if only single secondary core requested
  • send IDC messages if several secondary cores involved

Cherry-pick from mtl-005 branch

Comment thread src/idc/idc.c Outdated
Comment thread src/ipc/ipc4/handler.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ignoring error? And this is not an IPC4 error code

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

line 473-474, althrough have a long distance to reach.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

still idc_send_msg() returns POSIX error codes, so they cannot be passed up to the caller here.

Comment thread src/ipc/ipc4/handler.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it is not about using or not using IDCs, it's about whether all pipelines are on the same core or on different. Maybe call the variable like multi_core or cross_core or similar

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

... it's also about IDC usage as that's what in the end happens. Cause-versus-effect , I'm happy with both namings.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@kv2019i sorry, I meant, that IDCs are used in both cases, just that in one case the use is explicit and in the other case it's hidden under Zephyr API calls. So, using IDCs isn't a differentiating factor between the cases

Comment thread src/ipc/ipc4/handler.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

... it's also about IDC usage as that's what in the end happens. Cause-versus-effect , I'm happy with both namings.

This will allow to correctly set multiple pipelines state
even if they are allocated on different cores.

ipc4_set_pipeline_state will check if several cores are involved
- set ppl state if only current core requested
- process IPC on another core if only single secondary core requested
- send IDC messages if several secondary cores involved

Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
Comment thread src/idc/idc.c
*/
static int idc_ppl_state(uint32_t ppl_id)
{
#if CONFIG_IPC_MAJOR_4

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

zephyr + tgl IPC3 failed so I added this check

Comment thread src/ipc/ipc4/handler.c
idx = ppl_icd->core;
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can we simplify the loop below to do this right after the first one?

	/* execute IPC on the secondary core if all pipelines are on the same core */
	if (!use_idc && !cpu_is_me(idx))
		return ipc4_process_on_core(idx, false);

@lyakh

lyakh commented Jul 21, 2023

Copy link
Copy Markdown
Collaborator

... it's also about IDC usage as that's what in the end happens. Cause-versus-effect , I'm happy with both namings.

@kv2019i the thing is, that in the other case it's also using IDC - when all of the pipelines in a set are processed on a different core and ipc_process_on_core() is used, so the flag isn't doing what its name is suggesting - it isn't distinguishing "use IDC" against "don't use IDC" cases

Comment thread src/ipc/ipc4/handler.c
Comment thread src/ipc/ipc4/handler.c
@serhiy-katsyuba-intel
serhiy-katsyuba-intel self-requested a review July 26, 2023 10:45
@lgirdwood

Copy link
Copy Markdown
Member

@abonislawski conflicts

Signed-off-by: serhiy-katsyuba-intel <101654961+serhiy-katsyuba-intel@users.noreply.github.com>
@serhiy-katsyuba-intel

Copy link
Copy Markdown
Contributor

Adrian is on vacation this week. The conflict was with my commit merged half an hour ago. I resolved the conflict here on GitHub, however, this resulted in addition merge commit, which, probably, will not look nice in history. So let's wait for Adrian to rebase its commit and update the PR.

@lgirdwood lgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@abonislawski can you rebase. Thanks !

@juimonen

juimonen commented Aug 2, 2023

Copy link
Copy Markdown

closing as merged in #7995

@juimonen juimonen closed this Aug 2, 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.

10 participants