From 75fae9515a692732c9eb67a2afe3ef0b0dead43e Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Thu, 23 Sep 2021 14:32:39 +0200 Subject: [PATCH 1/5] ipc: properly account IPC responses on multicore It is possible, that ipc_complete_cmd() is entered 3 times while processing a single IPC message in multicore configurations: once by the primary core, then by the IDC IPC processing context, and finally by the pipeline thread, actually executing the IPC. To make sure that the response is only sent once by the core, that actually processes the IPC, we must not clear the .delayed_response flag on the primary core in such cases, it should only be done by the IDC IPC context before the pipeline task actually sends the response. Fixes: 82857a78ba04 ("ipc: fix a IPC completion race") Signed-off-by: Guennadi Liakhovetski --- src/drivers/intel/cavs/ipc.c | 5 ----- src/ipc/ipc-common.c | 3 +++ 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/drivers/intel/cavs/ipc.c b/src/drivers/intel/cavs/ipc.c index f22b9fbad151..8ef7af49cd35 100644 --- a/src/drivers/intel/cavs/ipc.c +++ b/src/drivers/intel/cavs/ipc.c @@ -200,11 +200,6 @@ enum task_state ipc_platform_do_cmd(void *data) void ipc_platform_complete_cmd(void *data) { - 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 diff --git a/src/ipc/ipc-common.c b/src/ipc/ipc-common.c index dd29110e18c3..fec419268f24 100644 --- a/src/ipc/ipc-common.c +++ b/src/ipc/ipc-common.c @@ -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); /* From 57d013ad6cea0e918628cd75234f37d8818315f1 Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Thu, 23 Sep 2021 15:07:26 +0200 Subject: [PATCH 2/5] ipc: print the incoming IPC code IPCs from the host aren't very frequent during normal operation, and having all their codes in firmware trace output is extremely useful for debugging. Add a trace print for arriving IPCs. Signed-off-by: Guennadi Liakhovetski --- src/ipc/ipc3/handler.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ipc/ipc3/handler.c b/src/ipc/ipc3/handler.c index a87dd30ef9d7..ac3604e341d3 100644 --- a/src/ipc/ipc3/handler.c +++ b/src/ipc/ipc3/handler.c @@ -1524,9 +1524,11 @@ void ipc_cmd(ipc_cmd_hdr *_hdr) goto out; } - if (!cpu_is_secondary(cpu_get_id())) + if (!cpu_is_secondary(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); + } ipc->delayed_response = false; From 35f05c311fd81ccaa459a5279423c3940c377c80 Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Fri, 8 Oct 2021 16:05:46 +0200 Subject: [PATCH 3/5] keyword: check test_keyword_get_threshold() return value test_keyword_get_threshold() can return errors, make sure to check its return value before using it. Signed-off-by: Guennadi Liakhovetski --- src/samples/audio/detect_test.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/src/samples/audio/detect_test.c b/src/samples/audio/detect_test.c index 845f13af9e3f..046d8def3b51 100644 --- a/src/samples/audio/detect_test.c +++ b/src/samples/audio/detect_test.c @@ -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; @@ -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; } @@ -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, From c3e6d86aaee01b4f6ef38fb3dd8a6b411ea5703c Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Tue, 28 Sep 2021 13:57:28 +0200 Subject: [PATCH 4/5] ipc: ipc3: ignore PCM_PARAMS for active pipelines With IPC3 it is an error to send a PCM_PARAMS IPC on an active pipeline, but it is a valid use-case with IPC4, ignore such IPCs. Signed-off-by: Guennadi Liakhovetski --- src/ipc/ipc3/handler.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/ipc/ipc3/handler.c b/src/ipc/ipc3/handler.c index ac3604e341d3..3e55d38b2199 100644 --- a/src/ipc/ipc3/handler.c +++ b/src/ipc/ipc3/handler.c @@ -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; + } + #if CONFIG_HOST_PTABLE cd = pcm_dev->cd; From b2ef9809a9a8d72a01480afc27408a9a8782be72 Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Fri, 15 Oct 2021 14:06:36 +0200 Subject: [PATCH 5/5] multicore: (cosmetic) replace cpu_is_secondary() with cpu_is_primary() cpu_is_primary() can be easier to parse than !cpu_is_secondary(), make the swap. Signed-off-by: Guennadi Liakhovetski --- src/include/sof/lib/cpu.h | 4 ++-- src/ipc/ipc3/dai.c | 2 +- src/ipc/ipc3/handler.c | 2 +- src/schedule/edf_schedule.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/include/sof/lib/cpu.h b/src/include/sof/lib/cpu.h index 21df5fa06141..e0468e029235 100644 --- a/src/include/sof/lib/cpu.h +++ b/src/include/sof/lib/cpu.h @@ -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) diff --git a/src/ipc/ipc3/dai.c b/src/ipc/ipc3/dai.c index 3f920e924e00..f0d392cec275 100644 --- a/src/ipc/ipc3/dai.c +++ b/src/ipc/ipc3/dai.c @@ -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; diff --git a/src/ipc/ipc3/handler.c b/src/ipc/ipc3/handler.c index 3e55d38b2199..92da53b9b708 100644 --- a/src/ipc/ipc3/handler.c +++ b/src/ipc/ipc3/handler.c @@ -1536,7 +1536,7 @@ 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); diff --git a/src/schedule/edf_schedule.c b/src/schedule/edf_schedule.c index a0a3b46ebedf..14d552e123f4 100644 --- a/src/schedule/edf_schedule.c +++ b/src/schedule/edf_schedule.c @@ -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)) dcache_writeback_invalidate_region(edf_pdata, sizeof(*edf_pdata)); return 0;