IPC and keyword detection fixes by lyakh · Pull Request #4869 · thesofproject/sof

lyakh · 2021-10-13T12:03:59Z

No description provided.

kv2019i

Hmm, sensible problems to fix, but I'm left with many questions about the how the fixes work. Please see questions inline.

kv2019i · 2021-10-13T12:22:48Z

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?

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.

@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?

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

?

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

@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

kv2019i · 2021-10-13T12:26:06Z

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

true, let me fix

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.

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

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"

@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 :-)

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.

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

yes please, with a comment too :)

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

lyakh · 2021-10-13T16:03:23Z

CI failures:
QuickBuild keyword tests fail because they use the sample width of 0, which was previously ignored, but now it triggers an error. Should the tests be fixed?
Device test: two timeouts on CML_RVP_SDW, seems unrelated.

lyakh · 2021-10-14T07:27:03Z

@wszypelt can the sample width be fixed in KD tests?

lgirdwood · 2021-10-14T13:49:41Z

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"

lgirdwood · 2021-10-15T10:41:25Z

@kkarask @wszypelt any comments on QB using sample width of 0 (see above comments). May need an update on your side ?

wszypelt · 2021-10-15T10:49:45Z

@lyakh @lgirdwood we'll take a look at it together with Filip

lgirdwood

@lyakh pls ping @kv2019i when update is pushed. Should get merged on Monday

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: 82857a7 ("ipc: fix a IPC completion race") Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

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 <guennadi.liakhovetski@linux.intel.com>

test_keyword_get_threshold() can return errors, make sure to check its return value before using it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

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 <guennadi.liakhovetski@linux.intel.com>

cpu_is_primary() can be easier to parse than !cpu_is_secondary(), make the swap. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

kv2019i

Looks fine. I did leave a comment/question to the last patch, but not a blocker.

kv2019i · 2021-10-18T07:42:24Z


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


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

@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...

lgirdwood · 2021-10-18T14:08:15Z

CI showing known suspend timeouts.

lyakh requested review from bardliao, dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners October 13, 2021 12:03

lyakh mentioned this pull request Oct 13, 2021

Correctness and style fixes #4856

Merged

kv2019i requested changes Oct 13, 2021

View reviewed changes

lgirdwood reviewed Oct 14, 2021

View reviewed changes

lyakh force-pushed the 20211008-misc_3 branch from 9fc69b3 to e8a9015 Compare October 15, 2021 12:26

lyakh requested a review from mrajwa as a code owner October 15, 2021 12:26

lyakh requested a review from kv2019i October 15, 2021 12:39

lgirdwood reviewed Oct 15, 2021

View reviewed changes

lyakh added 5 commits October 15, 2021 16:35

keyword: check test_keyword_get_threshold() return value

35f05c3

test_keyword_get_threshold() can return errors, make sure to check its return value before using it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

ipc: ipc3: ignore PCM_PARAMS for active pipelines

c3e6d86

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 <guennadi.liakhovetski@linux.intel.com>

multicore: (cosmetic) replace cpu_is_secondary() with cpu_is_primary()

b2ef980

cpu_is_primary() can be easier to parse than !cpu_is_secondary(), make the swap. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the 20211008-misc_3 branch from e8a9015 to b2ef980 Compare October 15, 2021 14:41

lgirdwood approved these changes Oct 15, 2021

View reviewed changes

kv2019i approved these changes Oct 18, 2021

View reviewed changes

lgirdwood merged commit 8549c9c into thesofproject:main Oct 18, 2021

lyakh deleted the 20211008-misc_3 branch October 18, 2021 14:09

lyakh mentioned this pull request Oct 26, 2022

[FEATURE] Reject PCM_PARAMS IPC when in ACTIVE or PAUSED state #4796

Closed

Conversation

lyakh commented Oct 13, 2021

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh commented Oct 13, 2021

Uh oh!

lyakh commented Oct 14, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Oct 15, 2021

Uh oh!

wszypelt commented Oct 15, 2021

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Oct 18, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ranj063 Oct 13, 2021 •

edited

Loading