ASoC: SOF: Intel: sort pipeline based on priority #4593

RanderWang · 2023-09-15T02:14:23Z

The pipeline priority is set in topology and driver should sort pipeline based on priority for trigger order.

Topology PR: thesofproject/sof#8204

sound/soc/sof/ipc4-pcm.c

RanderWang · 2023-09-15T09:01:03Z

@ujfalusi update it , thanks!

sound/soc/sof/ipc4-topology.c

sound/soc/sof/ipc4-pcm.c

ranj063 · 2023-09-15T14:39:47Z

@RanderWang could you please make sure this PR works as expected with the aec pipeline? untested PRs are not ready for review

sound/soc/sof/ipc4-pcm.c

RanderWang · 2023-09-18T01:16:47Z

@RanderWang could you please make sure this PR works as expected with the aec pipeline? untested PRs are not ready for review

yes, I tested with cavs-rt5682.tplg with nocodec dai and it passed all CI test

RanderWang · 2023-09-18T01:20:21Z

@RDharageswari can you help to validate it with Ranjani's topology PR thesofproject/sof#8101 and thesofproject/sof#8204

RanderWang · 2023-09-18T06:50:58Z

@plbossart @ujfalusi @ranj063 the tplg is thesofproject/sof#8204. Based on @ranj063 's advice,
host capture pipeline (priority 3)-> AEC pipeline ( priority 2)-> BE DMIC DAI pipeline (priority 1)
-> ref capture pipeline (priority 0).

The following log is for AEC test on MTL, please check the payload for pipeline id. (the first 00000004 indicates 4 pipelines)

//host capture pipeline (pipeline id 2)-> AEC pipeline ( pipeline id  1)
-> BE DMIC DAI pipeline (pipeline id  0) -> ref capture pipeline (pipeline id  3).

[ 3623.375537] snd_sof:sof_pcm_trigger: sof-audio-pci-intel-mtl : pcm: trigger stream 27 dir 1 cmd 1
[ 3623.375543] snd_sof:sof_ipc4_trigger_pipelines: sof-audio-pci-intel-mtl : trigger cmd: 1 state: 4
[ 3623.375581] : sof-audio-pci-intel-mtl : ipc tx      : 0x13000003|0x1: GLB_SET_PIPELINE_STATE [data size: 20]
[ 3623.377158] : sof-audio-pci-intel-mtl : ipc tx reply: 0x33000000|0x1: GLB_SET_PIPELINE_STATE
[ 3623.377319] : sof-audio-pci-intel-mtl : ipc tx done : 0x13000003|0x1: GLB_SET_PIPELINE_STATE [data size: 20]
[ 3623.377329] Message payload: 00000000: 00000004 00000002 00000001 00000000 // the first 00000004 indicates 4 pipeline
[ 3623.377333] Message payload: 00000010: 00000003
[ 3623.377338] : sof-audio-pci-intel-mtl : ipc tx      : 0x13000004|0x1: GLB_SET_PIPELINE_STATE [data size: 20]
[ 3623.388855] : sof-audio-pci-intel-mtl : ipc tx reply: 0x33000000|0x1: GLB_SET_PIPELINE_STATE
[ 3623.389051] : sof-audio-pci-intel-mtl : ipc tx done : 0x13000004|0x1: GLB_SET_PIPELINE_STATE [data size: 20]
[ 3623.389059] Message payload: 00000000: 00000004 00000002 00000001 00000000
[ 3623.389062] Message payload: 00000010: 00000003

[ 3624.473963] snd_sof:sof_pcm_trigger: sof-audio-pci-intel-mtl : pcm: trigger stream 27 dir 1 cmd 0
[ 3624.473978] snd_sof:sof_ipc4_trigger_pipelines: sof-audio-pci-intel-mtl : trigger cmd: 0 state: 3
[ 3624.473993] : sof-audio-pci-intel-mtl : ipc tx      : 0x13000003|0x1: GLB_SET_PIPELINE_STATE [data size: 20]
[ 3624.484598] : sof-audio-pci-intel-mtl : ipc tx reply: 0x33000000|0x1: GLB_SET_PIPELINE_STATE
[ 3624.484794] : sof-audio-pci-intel-mtl : ipc tx done : 0x13000003|0x1: GLB_SET_PIPELINE_STATE [data size: 20]
[ 3624.484805] Message payload: 00000000: 00000004 00000003 00000000 00000001
[ 3624.484808] Message payload: 00000010: 00000002
[ 3624.484902] snd_sof:sof_pcm_hw_free: sof-audio-pci-intel-mtl : pcm: free stream 27 dir 1

[ 3624.484920] snd_sof:sof_ipc4_trigger_pipelines: sof-audio-pci-intel-mtl : trigger cmd: 0 state: 2
[ 3624.484930] : sof-audio-pci-intel-mtl : ipc tx      : 0x13000002|0x1: GLB_SET_PIPELINE_STATE [data size: 20]
[ 3624.488590] : sof-audio-pci-intel-mtl : ipc tx reply: 0x33000000|0x1: GLB_SET_PIPELINE_STATE
[ 3624.488811] : sof-audio-pci-intel-mtl : ipc tx done : 0x13000002|0x1: GLB_SET_PIPELINE_STATE [data size: 20]
[ 3624.488820] Message payload: 00000000: 00000004 00000002 00000001 00000000
[ 3624.488823] Message payload: 00000010: 00000003

plbossart

too many questions, this needs a lot more work @RanderWang

sound/soc/sof/ipc4-topology.c

sound/soc/sof/ipc4-pcm.c

plbossart

Sorry, not getting the picture @RanderWang

You have a very detailed understanding of IPC4, I don't, and I just can't figure out what this PR brings and when the order needs to be low->high or high->low.

sound/soc/sof/ipc4-pcm.c

plbossart · 2023-09-21T20:32:16Z

sound/soc/sof/ipc4-pcm.c

can you clarify why the order needs to be different between PAUSED and RESET?

I also spent time to check it. For REF FW, it doesn't care about the order for PAUSED or RESET and only care about START since START can result to XRUN. Although REF FW doesn't care about PAUSED or RESET, it still use the reverse order compared to START. This behavior is commented in REF driver.

For SOF, we always pause pipeline first then reset. After all the pipeline are paused or are just ready, the order of RESET is unimportant. I follow the current logic. @ranj063 can give more idea.

/* * IPC4 requires pipelines to be triggered in order starting at the sink and * walking all the way to the source. So traverse the pipeline_list in the order * sink->source when starting PCM's and in the reverse order to pause/stop PCM's. * Skip the pipelines that have their skip_during_fe_trigger flag set. If there is a fork * in the pipeline, the order of triggering between the left/right paths will be * indeterministic. But the sink->source trigger order sink->source would still be * guaranteed for each fork independently. */ if (state == SOF_IPC4_PIPE_RUNNING || state == SOF_IPC4_PIPE_RESET) for (i = pipeline_list->count - 1; i >= 0; i--) { spipe = pipeline_list->pipelines[i]; sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list, pipe_priority); } else for (i = 0; i < pipeline_list->count; i++) { spipe = pipeline_list->pipelines[i]; sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list, pipe_priority); }

i am completely lost. The answer contains two conflicting explanations

A) the reference IPC4 implementation uses the same order for pause and reset
B) the suggested code uses a different order for pause and reset - even though the order for reset does not matter.

if the intent was to follow the reference to the letter, then obviously something went sideways.

I give up at this point, someone else needs to review.

@plbossart let's fully follow the reference. Adopt the same order for pause & resume case.

Driver set pipeline priority according to priority setting in topology. Signed-off-by: Rander Wang <rander.wang@intel.com>

RanderWang · 2023-09-25T07:53:12Z

let's fully follow the reference. Adopt the same order for pause & resume case: set pipeline from low priority to high. Start trigger uses order from high to low

someone else needs to review

RanderWang · 2023-09-26T01:50:01Z

states update:
(1) if topology doesn't set priority (default 0), we still go with the current way. This make sure no regression issue will happen.
(2) else trigger start by order from high to low priority, trigger reset & pause from low to high priority. Fully align with REF implementation.

plbossart · 2023-09-26T13:25:00Z

@ranj063 @RDharageswari have you reviewed and possibly tested this suggested change?

ranj063 · 2023-09-26T15:20:44Z

@ranj063 @RDharageswari have you reviewed and possibly tested this suggested change?

@plbossart this is still under testing. Please give us one more day to confirm

ranj063 · 2023-09-26T15:35:49Z

sound/soc/sof/ipc4-pcm.c

@RanderWang I had a though about this second patch. Instead of sorting the pipelines based on priority in the kernel while ending the IPC, can we handle this in the FW? When the pipeline is set up, the FW is already aware of the priority. We can use that information in the firmware to trigger in decreasing order of priorities, right?

(1) We can do it in FW, but we need some search operation to find the priority in FW. As you know, host X86 is much faster than dsp core. (5GHZ vs 400MHZ , DDR4800 vs DDR400).
(2) According to above discussing, we should fully align with REF implementation which does it in driver and it should be good for REF FW also.

Verified working fine with randomly start/stop spk playback and DMIC0 RTC AEC.

ranj063

just one comment @RanderWang to fix an error path. Otherwise looks good to go.

sound/soc/sof/ipc4-pcm.c

The pipeline priority is set in topology and driver should sort pipeline based on priority for trigger order. Signed-off-by: Rander Wang <rander.wang@intel.com>

RanderWang · 2023-09-28T08:49:34Z

updated memory free. Thanks!

ujfalusi · 2023-09-29T13:29:27Z

sound/soc/sof/ipc4-pcm.c

+
+	for (j = trigger_list->count - 1; j >= i; j--) {
+		trigger_list->pipeline_instance_ids[j + 1] = trigger_list->pipeline_instance_ids[j];
+		pipe_priority[j + 1] = pipe_priority[j];


trigger_list->count - 1 + 1 == trigger_list->count
trigger_list->pipeline_instance_ids[trigger_list->count] and pipe_priority[trigger_list->count] is out of bounds access?

i had the same suspicion when I looked at the code. But the array size is pipeline_list->count not trigger_list->count. So I think it is safe here

At the end we are not going to put all pipelines in the trigger list? Right, when we add the last pipeline the trigger_list->count is pipeline_list->count, I guess it is OK, but super confusing.

ujfalusi · 2023-10-02T05:59:58Z

sound/soc/sof/ipc4-pcm.c

+
+	for (j = trigger_list->count - 1; j >= i; j--) {
+		trigger_list->pipeline_instance_ids[j + 1] = trigger_list->pipeline_instance_ids[j];
+		pipe_priority[j + 1] = pipe_priority[j];


At the end we are not going to put all pipelines in the trigger list? Right, when we add the last pipeline the trigger_list->count is pipeline_list->count, I guess it is OK, but super confusing.

ranj063 · 2023-10-03T15:11:20Z

usual timeouts in suspend/resume tests. merging

RanderWang requested review from bardliao, dbaluta, kuanhsuncheng, kv2019i, lgirdwood, plbossart, ranj063, ujfalusi, vijendarmukunda and yaochunhung as code owners September 15, 2023 02:14

ujfalusi reviewed Sep 15, 2023

View reviewed changes

sound/soc/sof/ipc4-pcm.c Outdated Show resolved Hide resolved

RanderWang force-pushed the ipc4-pipeline-sort branch 2 times, most recently from 4268980 to 2ce530d Compare September 15, 2023 08:34

This was referenced Sep 15, 2023

topology2: set pipeline priority for google aec capture stream thesofproject/sof#8204

Merged

ASoC: SOF: sof-audio: Modify the order of widget set up #4550

Closed

ujfalusi reviewed Sep 15, 2023

View reviewed changes

sound/soc/sof/ipc4-topology.c Outdated Show resolved Hide resolved

sound/soc/sof/ipc4-pcm.c Outdated Show resolved Hide resolved

sound/soc/sof/ipc4-pcm.c Outdated Show resolved Hide resolved

ranj063 reviewed Sep 15, 2023

View reviewed changes

sound/soc/sof/ipc4-pcm.c Outdated Show resolved Hide resolved

sound/soc/sof/ipc4-pcm.c Outdated Show resolved Hide resolved

RanderWang force-pushed the ipc4-pipeline-sort branch 2 times, most recently from c473966 to 6bc6b06 Compare September 18, 2023 03:21

plbossart reviewed Sep 18, 2023

View reviewed changes

RanderWang force-pushed the ipc4-pipeline-sort branch from 6bc6b06 to 9d28d15 Compare September 19, 2023 02:20

plbossart previously requested changes Sep 21, 2023

View reviewed changes

RanderWang mentioned this pull request Sep 22, 2023

[BUG] [MTL] AEC functionality depends on Echo reference endpoint to be started thesofproject/sof#8075

Closed

RanderWang force-pushed the ipc4-pipeline-sort branch from 9d28d15 to 172c97e Compare September 22, 2023 09:05

ASoC: SOF: IPC4: get pipeline priority from topology

8c0770b

Driver set pipeline priority according to priority setting in topology. Signed-off-by: Rander Wang <rander.wang@intel.com>

RanderWang force-pushed the ipc4-pipeline-sort branch from 172c97e to e4fe334 Compare September 25, 2023 07:49

ranj063 reviewed Sep 26, 2023

View reviewed changes

ranj063 reviewed Sep 27, 2023

View reviewed changes

sound/soc/sof/ipc4-pcm.c Outdated Show resolved Hide resolved

ASoC: SOF: IPC4: sort pipeline based on priority

b7d0610

The pipeline priority is set in topology and driver should sort pipeline based on priority for trigger order. Signed-off-by: Rander Wang <rander.wang@intel.com>

RanderWang force-pushed the ipc4-pipeline-sort branch from e4fe334 to b7d0610 Compare September 28, 2023 05:27

ranj063 approved these changes Sep 28, 2023

View reviewed changes

ujfalusi reviewed Sep 29, 2023

View reviewed changes

ujfalusi approved these changes Oct 2, 2023

View reviewed changes

ranj063 merged commit 5c85a87 into thesofproject:topic/sof-dev Oct 3, 2023

ASoC: SOF: Intel: sort pipeline based on priority #4593

ASoC: SOF: Intel: sort pipeline based on priority #4593

Uh oh!

Conversation

RanderWang commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

RanderWang commented Sep 15, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ranj063 commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RanderWang commented Sep 18, 2023

Uh oh!

RanderWang commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Sep 18, 2023

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang Sep 22, 2023 • 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!

RanderWang commented Sep 25, 2023

Uh oh!

RanderWang commented Sep 26, 2023

Uh oh!

plbossart commented Sep 26, 2023

Uh oh!

ranj063 commented Sep 26, 2023

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!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RanderWang commented Sep 28, 2023

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!

ranj063 commented Oct 3, 2023

Uh oh!

Reviewers

Assignees

Labels

RanderWang commented Sep 15, 2023 •

edited

Loading

ranj063 commented Sep 15, 2023 •

edited

Loading

RanderWang commented Sep 18, 2023 •

edited

Loading

RanderWang Sep 22, 2023 •

edited

Loading