Skip to content

ipc4: multi pipeline set state on different cores #7962

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

Closed
abonislawski wants to merge 2 commits into from
Closed

ipc4: multi pipeline set state on different cores #7962

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6572a71
ipc4: multi pipeline set state on different cores
abonislawski May 16, 2023
4b6b449
Merge branch 'main' into multi_set_state
serhiy-katsyuba-intel Jul 26, 2023
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
30 changes: 30 additions & 0 deletions src/idc/idc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,33 @@ static int idc_reset(uint32_t comp_id)
return ret;
}

/**
* \brief Executes IDC pipeline set state message.
* \param[in] ppl_id Pipeline id to be triggered.
* \return Error code.
*/
static int idc_ppl_state(uint32_t ppl_id)
{
#if CONFIG_IPC_MAJOR_4

@abonislawski abonislawski Jul 20, 2023

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

struct ipc *ipc = ipc_get();
struct idc *idc = *idc_get();
struct idc_payload *payload = idc_payload_get(idc, cpu_get_id());
struct ipc_comp_dev *ppl_icd;
uint32_t cmd = *(uint32_t *)payload;
bool delayed = false;

ppl_icd = ipc_get_comp_by_ppl_id(ipc, COMP_TYPE_PIPELINE, ppl_id);
if (!ppl_icd) {
tr_err(&idc_tr, "idc: comp %d not found", ppl_id);
return IPC4_INVALID_RESOURCE_ID;
}

return set_pipeline_state(ppl_icd, cmd, &delayed);
#else
return 0;
#endif
}

static void idc_prepare_d0ix(void)
{
/* set prepare_d0ix flag, which indicates that in the next
Expand Down Expand Up @@ -400,6 +427,9 @@ void idc_cmd(struct idc_msg *msg)
case iTS(IDC_MSG_RESET):
ret = idc_reset(msg->extension);
break;
case iTS(IDC_MSG_PPL_STATE):
ret = idc_ppl_state(msg->extension);
break;
case iTS(IDC_MSG_PREPARE_D0ix):
idc_prepare_d0ix();
break;
Expand Down
2 changes: 2 additions & 0 deletions src/include/sof/ipc/topology.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ typedef uint32_t ipc_comp;
#define ipc_from_pipe_new(x) ((struct ipc4_pipeline_create *)x)
#define ipc_from_pipe_connect(x) ((struct ipc4_module_bind_unbind *)x)

struct ipc_comp_dev;
const struct comp_driver *ipc4_get_comp_drv(int module_id);
struct comp_dev *ipc4_get_comp_dev(uint32_t comp_id);
int ipc4_add_comp_dev(struct comp_dev *dev);
Expand All @@ -57,6 +58,7 @@ int ipc4_trigger_chain_dma(struct ipc *ipc, struct ipc4_chain_dma *cdma, bool *d
int ipc4_process_on_core(uint32_t core, bool blocking);
int ipc4_pipeline_complete(struct ipc *ipc, uint32_t comp_id);
int ipc4_find_dma_config(struct ipc_config_dai *dai, uint8_t *data_buffer, uint32_t size);
int set_pipeline_state(struct ipc_comp_dev *ppl_icd, uint32_t cmd, bool *delayed);
#else
#error "No or invalid IPC MAJOR version selected."
#endif
Expand Down
43 changes: 34 additions & 9 deletions src/ipc/ipc4/handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ static bool is_any_ppl_active(void)
* ERROR Stop EOS |______\ SAVE
* /
*/
static int set_pipeline_state(struct ipc_comp_dev *ppl_icd, uint32_t cmd,
int set_pipeline_state(struct ipc_comp_dev *ppl_icd, uint32_t cmd,
bool *delayed)
{
struct ipc_comp_dev *host = NULL;
Expand Down Expand Up @@ -404,6 +404,8 @@ static int ipc4_set_pipeline_state(struct ipc4_message_request *ipc4)
struct ipc *ipc = ipc_get();
uint32_t cmd, ppl_count, id;
const uint32_t *ppl_id;
bool use_idc = false;

@lyakh lyakh Jul 18, 2023

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

@kv2019i kv2019i Jul 20, 2023

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.

@lyakh lyakh Jul 27, 2023

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

uint32_t idx;
int ret = 0;
int i;

Expand All @@ -425,6 +427,21 @@ static int ipc4_set_pipeline_state(struct ipc4_message_request *ipc4)
ppl_id = &id;
}

for (i = 0; i < ppl_count; i++) {
ppl_icd = ipc_get_comp_by_ppl_id(ipc, COMP_TYPE_PIPELINE, ppl_id[i]);
if (!ppl_icd) {
tr_err(&ipc_tr, "ipc: comp %d not found", ppl_id[i]);
return IPC4_INVALID_RESOURCE_ID;
}

if (i) {
if (ppl_icd->core != idx)
use_idc = true;
} else {
idx = ppl_icd->core;
}
Comment thread
btian1 marked this conversation as resolved.
}

@ranj063 ranj063 Jul 20, 2023

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);

for (i = 0; i < ppl_count; i++) {
bool delayed = false;

Expand All @@ -435,15 +452,23 @@ static int ipc4_set_pipeline_state(struct ipc4_message_request *ipc4)
}

/* Pass IPC to target core
* Note: current implementation supports only a case with
* all pipelines in cmd allocated on the same core
* or use idc if more than one core used
*/
if (!cpu_is_me(ppl_icd->core))
return ipc4_process_on_core(ppl_icd->core, false);

ipc_compound_pre_start(state.primary.r.type);
ret = set_pipeline_state(ppl_icd, cmd, &delayed);
ipc_compound_post_start(state.primary.r.type, ret, delayed);
if (!cpu_is_me(ppl_icd->core)) {
if (use_idc) {
struct idc_msg msg = { IDC_MSG_PPL_STATE,
IDC_MSG_PPL_STATE_EXT(ppl_id[i]), ppl_icd->core, sizeof(cmd),
&cmd, };

ret = idc_send_msg(&msg, IDC_BLOCKING);

@lyakh lyakh Jul 18, 2023

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

@btian1 btian1 Jul 19, 2023

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.

@lyakh lyakh Jul 27, 2023

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.

} else {
return ipc4_process_on_core(ppl_icd->core, false);
}
} else {
ipc_compound_pre_start(state.primary.r.type);
ret = set_pipeline_state(ppl_icd, cmd, &delayed);
ipc_compound_post_start(state.primary.r.type, ret, delayed);
}

if (ret != 0)
return ret;
Expand Down
4 changes: 4 additions & 0 deletions zephyr/include/rtos/idc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@

#define IDC_HEADER_TO_AMS_SLOT_MASK(x) (x & 0xFFFF)

/** \brief IDC pipeline set state message. */
#define IDC_MSG_PPL_STATE IDC_TYPE(0xC)
#define IDC_MSG_PPL_STATE_EXT(x) IDC_EXTENSION(x)

#define IDC_MSG_BIND IDC_TYPE(0xD)
#define IDC_MSG_UNBIND IDC_TYPE(0xE)
#define IDC_MSG_GET_ATTRIBUTE IDC_TYPE(0xF)
Expand Down