Skip to content

IPC and keyword detection fixes #4869

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

Merged
lgirdwood merged 5 commits into from
Oct 18, 2021
Merged

IPC and keyword detection fixes #4869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
75fae95
ipc: properly account IPC responses on multicore
lyakh Sep 23, 2021
57d013a
ipc: print the incoming IPC code
lyakh Sep 23, 2021
35f05c3
keyword: check test_keyword_get_threshold() return value
lyakh Oct 8, 2021
c3e6d86
ipc: ipc3: ignore PCM_PARAMS for active pipelines
lyakh Sep 28, 2021
b2ef980
multicore: (cosmetic) replace cpu_is_secondary() with cpu_is_primary()
lyakh Oct 15, 2021
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
5 changes: 0 additions & 5 deletions src/drivers/intel/cavs/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,11 +200,6 @@ enum task_state ipc_platform_do_cmd(void *data)

void ipc_platform_complete_cmd(void *data)

@kv2019i kv2019i Oct 13, 2021

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 get the commit message description of the problem, but I've having hard time how moving the cpu-is-me check form one place to another actually fixes the problem as describe in commit message. Can you somehow clarify how this is supposed to work?

@lyakh lyakh Oct 13, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I can. Look at the .delayed_response flag below. As I describe in the commit message, ipc_complete_cmd() can be entered 3 times: first on the primary core, then on the secondary core from the IDC / IPC, then on the secondary core from the pipeline task. Before this patch the first call on the primary core would set .delayed_response to false, so that the second call to this function would already find .delayed_response set to false and send a response. Then the third call would again find it as false and send it again. After this patch the primary core won't set false, only the secondary core will.

@ranj063 ranj063 Oct 13, 2021 •
edited
Loading

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.

@lyakh it is true the function is entered 3 times but the delayed_response flag is actually set only on the core that the IPC gets executed on right? So what the primary core does is sort of immaterial right?

@lyakh lyakh Oct 14, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ipc is a global object. So, can something like this happen?

core 0                           core 1
IPC IRQ
IPC EDF task
ipc_cmd()
IDC to core 1
                                 IDC IRQ
timer IRQ
pipeline tasks
                                 ipc_cmd()
                                 ipc->delayed_response = 1
resume IPC
ipc->delayed_response = 0

?

@ranj063 ranj063 Oct 14, 2021

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.

IDC is non-blocking isnt it? And timer IRQ can preempt the IPC task?

@lyakh lyakh Oct 15, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@ranj063 It depends on the mode parameter. E.g. for STREAM_TRIGGER it's non-blocking:

static int ipc_stream_trigger(uint32_t header)
{
	...
	if (!cpu_is_me(pcm_dev->core))
		return ipc_process_on_core(pcm_dev->core, false);

false means non-blocking

{
struct ipc *ipc = data;

if (!cpu_is_me(ipc->core))
return;


/* write 1 to clear busy, and trigger interrupt to host*/
#if CAVS_VERSION == CAVS_VERSION_1_5
Expand Down
4 changes: 2 additions & 2 deletions src/include/sof/lib/cpu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ static inline int cpu_get_id(void)
return arch_cpu_get_id();
}

static inline bool cpu_is_secondary(int id)
static inline bool cpu_is_primary(int id)
{
return id != PLATFORM_PRIMARY_CORE_ID;
return id == PLATFORM_PRIMARY_CORE_ID;
}

static inline bool cpu_is_me(int id)
Expand Down
3 changes: 3 additions & 0 deletions src/ipc/ipc-common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,9 @@ void ipc_complete_cmd(void *data)
uint32_t flags;
bool skip_first_entry;

if (!cpu_is_me(ipc->core))
return;

spin_lock_irq(&ipc->lock, flags);

/*
Expand Down
2 changes: 1 addition & 1 deletion src/ipc/ipc3/dai.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ int ipc_comp_dai_config(struct ipc *ipc, struct ipc_config_dai *common_config,
}

/* message forwarded only by primary core */
if (!cpu_is_secondary(cpu_get_id())) {
if (cpu_is_primary(cpu_get_id())) {
for (i = 0; i < CONFIG_CORE_COUNT; ++i) {
if (!comp_on_core[i])
continue;
Expand Down
16 changes: 15 additions & 1 deletion src/ipc/ipc3/handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,18 @@ static int ipc_stream_pcm_params(uint32_t stream)
return -EINVAL;
}

switch (pcm_dev->cd->pipeline->status) {
case COMP_STATE_ACTIVE:
case COMP_STATE_PRE_ACTIVE:
/*
* IPC4 has a use-case when a PCM parameter change request can
* be sent on an active pipeline, ignore it
*/
pipe_dbg(pcm_dev->cd->pipeline,
"ipc: ignore PCM param change request on an active pipeline");
return 0;
}

@kv2019i kv2019i Oct 13, 2021

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.

Commit not matching code. Commit says ignore, code says "-EINVAL".

@lyakh lyakh Oct 13, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

true, let me fix

@plbossart plbossart Oct 13, 2021

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.

and -EINVAL is not a a success code either ;-)

ipc: ipc3: return error on PCM_PARAMS request for active pipelines
If a PCM_PARAMS IPC arrives on an active pipeline, log and return an error immediately.

@ranj063 ranj063 Oct 13, 2021

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.

but why should it be an error? can we not ignore the IPC and return succes?

@lgirdwood lgirdwood Oct 14, 2021

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.

we should also be a bit more verbose with the trace context, i.e. "ipc: pipe X is active but client requested PCM params change"

@lyakh lyakh Oct 15, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@lgirdwood could you please reply to @ranj063 's question, I'm not sure I asked for an explanation from you when you requested me to do this :-)

@lgirdwood lgirdwood Oct 15, 2021

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.

Sending PCM params during a running pipeline means the driver and FW are out of sync. Now that's an obvious eror, but IIUC an IPC4 use case, so we should return 0 and have a big comment about this in teh code.

@lyakh lyakh Oct 15, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@lgirdwood you want it back to returning zero?..

@lgirdwood lgirdwood Oct 15, 2021

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.

yes please, with a comment too :)

@lgirdwood lgirdwood Oct 15, 2021

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.

@lyakh ping - only this change is needed I think (and the commit message to align with return value).

#if CONFIG_HOST_PTABLE
cd = pcm_dev->cd;

Expand Down Expand Up @@ -1524,9 +1536,11 @@ void ipc_cmd(ipc_cmd_hdr *_hdr)
goto out;
}

if (!cpu_is_secondary(cpu_get_id()))
if (cpu_is_primary(cpu_get_id())) {
/* A new IPC from the host, delivered to the primary core */
ipc->core = PLATFORM_PRIMARY_CORE_ID;
tr_info(&ipc_tr, "ipc: new cmd 0x%x", hdr->cmd);
}
Comment thread
kv2019i marked this conversation as resolved.
Resolved

ipc->delayed_response = false;

Expand Down
31 changes: 25 additions & 6 deletions src/samples/audio/detect_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,14 @@ static int test_keyword_apply_config(struct comp_dev *dev,
cd->config.activation_shift = ACTIVATION_DEFAULT_SHIFT;

if (!cd->config.activation_threshold) {
cd->config.activation_threshold =
test_keyword_get_threshold(dev, sample_width);
ret = test_keyword_get_threshold(dev, sample_width);
if (ret < 0) {
comp_err(dev, "test_keyword_apply_config(): unsupported sample width %u",
sample_width);
return ret;
}

cd->config.activation_threshold = ret;
}

return 0;
Expand Down Expand Up @@ -412,8 +418,14 @@ static int test_keyword_params(struct comp_dev *dev,
cd->keyphrase_samples = KEYPHRASE_DEFAULT_PREAMBLE_LENGTH;
}

cd->config.activation_threshold =
test_keyword_get_threshold(dev, params->sample_valid_bytes * 8);
err = test_keyword_get_threshold(dev, params->sample_valid_bytes * 8);
if (err < 0) {
comp_err(dev, "test_keyword_params(): unsupported sample width %u",
params->sample_valid_bytes * 8);
return err;
}

cd->config.activation_threshold = err;

return 0;
}
Expand Down Expand Up @@ -663,8 +675,15 @@ static int test_keyword_prepare(struct comp_dev *dev)
/* Default threshold value has to be changed
* according to host new format.
*/
cd->config.activation_threshold =
test_keyword_get_threshold(dev, valid_bits);
int ret = test_keyword_get_threshold(dev, valid_bits);

if (ret < 0) {
comp_err(dev, "test_keyword_prepare(): unsupported sample width %u",
valid_bits);
return ret;
}

cd->config.activation_threshold = ret;
}

cd->data_blob = comp_get_data_blob(cd->model_handler,
Expand Down
2 changes: 1 addition & 1 deletion src/schedule/edf_schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ int schedule_task_init_edf(struct task *task, const struct sof_uuid_entry *uid,
goto error;

/* flush for secondary core */
if (cpu_is_secondary(task->core))
if (!cpu_is_primary(task->core))

@kv2019i kv2019i Oct 18, 2021

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.

Why this change @lyakh ? Isn't cpu_is_secondary() the easier to parse in this case?

@lyakh lyakh Oct 18, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@kv2019i sorry, I might not have the best taste for "simple" vs. "complex" :-) This is the only use of cpu_is_secondary() and I decided that !cpu_is_primary() is easy enough to understand to justify the removal of cpu_is_secondary() in one place...

dcache_writeback_invalidate_region(edf_pdata,
sizeof(*edf_pdata));
return 0;
Expand Down