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

ranj063 · 2023-08-25T22:22:16Z

IPC4 expects the pipelines to be triggered in the order starting from the sink to the source. Today, we set up the widgets in the order from source->sink and save the pipelines that the widgets belong to in a list. This list is later traversed in the reverse order for triggering the pipelines. Instead of this, this patch proposes to change the order of widget set up from sink->source so that the pipelines will be added to the list in the same order as well.

This is also important when there are 2 DAIs in the topology connected to the same host copier as in the case of echo-ref.

SSP DAI copieri (Pipeline 0) -> AEC module -> host-copier (Pipeline 2)
^
|
module-copier <- DMIC DAI copier (Pipeline 1)

In this case, we want both the DAI pipelines (0 and 1) to be triggered after pipeline 2 has been started. So modifying the widget set up to start from the sink will guarantee that the pipelines will be triggered in the order 2 -> 1 -> 0 or 2 -> 0 -> 1. The order of trigger between the 2 DAI pipelines does not matter and it will depend on which order they appear in the list of connected DAPM widgets.

kv2019i

Code looks good, but some comments on the rationale and impacts of this patch.

sound/soc/sof/ipc4-pcm.c

plbossart

Additional explanations are really needed, it's not clear why we started with the source->sink order and now the other way around is better.

There's got to be cases where the source->sink was thought to be easier to handle or something.

plbossart · 2023-08-28T16:14:33Z

sound/soc/sof/ipc4-pcm.c

@@ -332,12 +332,12 @@ static int sof_ipc4_trigger_pipelines(struct snd_soc_component *component,
 	 * guaranteed for each fork independently.


commit message

typo on "copieri"

Also why do we represent the SSP capture and DMIC capture in different directions? This is confusing to me, not sure if there is any design intent.

it's also not clear to me what happens if there are multiple sink pipelines? what happens if the AEC module is connected to a NS module and a separate PCM on capture. How would the order be modified?

plbossart · 2023-08-28T16:18:10Z

sound/soc/sof/ipc4-pcm.c

 			spipe = pipeline_list->pipelines[i];
 			sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list);
 		}
 	else
-		for (i = 0; i < pipeline_list->count; i++) {
+		for (i = pipeline_list->count - 1; i >= 0; i--) {


I am not following why we did all this and why the 'new order' is better.

Surely there was something that led us to go from source to sink initially?

@plbossart I think the key point I missed in the explanation is that the order of widget setup doesnt matter. But what really matters is the order of pipeline trigger and this is determined by the order in which widgets are set up. So when the widgets are set up in the order source->sink, pipelines are added to the trigger list in the same order and we trigger in the reverse order to honor the sink->source requirement with IPC4.

So what happens today is that if you had 2 DAI's for capture (ie DMIC capture and SSP capture for reference as in the example in my commit message), the source to sink widget setup order adds the pipelines in the order 1->2->0.
So triggering in reverse would mean the reference capture pipeline goes first. This fails with an IPC timeout in the firmware. What we really want is for the pipeline 2 to be triggered first.

still struggling to figure out why we didn't use the same order for setup and trigger. Is this because it was less complicated to do so initially to follow the source->sink links?

also isn't the IPC timeout in the firmware an issue? Shouldn't there be some sort of state check to discard IPCs that make no sense?

We could have (and should have) used the sink->source for set up earlier but the reason we didnt is likely that we never had a situation where we have multiple sources/sinks in the list of DAPM widgets before and so it was convenient to just to set up in the source->sink order and just pick the reverse order for triggering.

The IPC timeout is absolutely a firmware issue and I am going to debug that as well but I think this PR makes itthe triggering order more logical anyway

I see what you mean now, for the capture part we indeed had a single source split in different streams. The AEC reference coming from a different DAI is a new configuration. Maybe that's something we should add to the nocodec topology by tying one SSP dai pipeline to an AEC pretend module.

yes, let me add the mock aec module to the DMIC capture path in the nocodec topology to enable testing this PR in CI

@ranj063 This clarification addressed my comment. It's also worth checking whether thesofproject/sof#8084 (just merged) has impact to FW behaviour.

@kv2019i I will check with 8084 once I add the mock aec to nocodec. But this PR is still in the right direction I think.

@kv2019i : this alone did not help.
This PR is very much needed and its critical for the AEC feature to work on the chromebook

RanderWang · 2023-08-29T10:12:36Z

"this patch proposes to change the order of widget set up from sink->source so that the pipelines will be added to the list in the same order as well." @ranj063 can you help to test REF FW ? When I developed IPC4 with REF FW, I got XRUN issue with different order. please test both playback and capture. Thanks!

IPC4 expects the pipelines to be triggered in the order starting from the sink to the source. The order of triggering pipelines during start is determined by the order in which the widgets are set up. Today, we set up the widgets in the order from source->sink and save the pipelines that the widgets belong to in a list. This list is later traversed in the reverse order for triggering the pipelines. Instead of this, this patch proposes to change the order of widget set up from sink->source so that the pipelines will be added to the list in the same order as well. The FW can handle widgets getting set up in any order (source->sink or sink->source). So reversing the current order will not affect functionality. This change is particularly important in cases where there are 2 DAIs in the topology connected to the same host copier as in the case of AEC with reference capture. SSP DAI copier (Pipeline 0) -> AEC module -> host-copier (Pipeline 2) ^ | DMIC DAI copier (Pipeline 1) In this case, we want both the DAI pipelines (0 and 1) to be started after pipeline 2 has been started. With the order of widget setup from source to sink, the pipelines would be added to the trigger list in the order, pipeline 1, pipeline 2 and finally pipeline 0. This would trigger pipeline 0 first and result in an IPC timeout during start. Modifying the widget set up to start from the sink will guarantee that the pipelines will be added to the trigger list in the order 2 -> 1 -> 0 or 2 -> 0 -> 1 i.e pipeline 2 will always be first. The order of trigger between the 2 DAI pipelines does not matter and it will depend on which order they appear in the list of connected DAPM widgets. Signed-off-by: Ranjani Sridharan <[email protected]>

ranj063 · 2023-08-29T16:39:15Z

"this patch proposes to change the order of widget set up from sink->source so that the pipelines will be added to the list in the same order as well." @ranj063 can you help to test REF FW ? When I developed IPC4 with REF FW, I got XRUN issue with different order. please test both playback and capture. Thanks!

@RanderWang as far as simple host->dai or dai->host pipelines are concerned nothing should change in terms of behavior.

RDharageswari

tried the following sequence:
1)speaker playback
2)AEC capture.
speaker playback stops when the capture ends

plbossart · 2023-09-06T13:02:32Z

@ranj063 so what's the conclusion here? Is this PR still relevant or is there still more work to do as @RDharageswari seems to suggest?

ranj063 · 2023-09-06T21:11:51Z

@ranj063 so what's the conclusion here? Is this PR still relevant or is there still more work to do as @RDharageswari seems to suggest?

@plbossart this PR is complete. The issue Dhara reported is due to a missing patch on her tree.

RanderWang

I agree that we should trigger pipeline from sink to source but the algorithm is not correct. In IPC4 each pipeline has a priority setting which should be used for trigger order and this is the logic in REF driver. We should set it in topology, but not now.

We should keep setup module from source to sink. Change the widget setup order to change the pipeline trigger order , It is not a good idea for me.

I had a struggling experience when enabling ipc4 capture at the very first of ipc4 development. IPC4 playback worked, but capture couldn't. I spent a few weeks to found that it was caused by the widget setup order. I always set up widget from host to dai for both playback & capture (old IPC3 logic at that time). It was wrong and should be from source to sink, dai to host for capture

RanderWang · 2023-09-07T05:22:12Z

Copied from windows topology in each laptop production (c:\windows\system32\driverstore\intcOEDxxxx\topology). Different pipeline different priority. It is much smart than depending on widget order in list.

<Path Name="Render system pin" Id="eIntcPathRenderSystem">
   <Caps HostLink="INTC_OE_PATH_HOSTLINK_HOST" Direction="INTC_OE_PATH_DIRECTION_RENDER" Priority="0" Longbuf="false" />
......
<Path Name="Render link" Id="eIntcPathRenderLink">
    <Caps EFX="1" FXF="1" HostLink="INTC_OE_PATH_HOSTLINK_LINK" Direction="INTC_OE_PATH_DIRECTION_RENDER" Priority="2" Longbuf="false" />

<Path Name="Capture reference" Id="eIntcPathCaptureReference">
    <Caps HostLink="INTC_OE_PATH_HOSTLINK_OTHER" Direction="INTC_OE_PATH_DIRECTION_CAPTURE" Priority="4" Longbuf="false" />
 .....

REF Driver log:

 [IntcOED] DEBUG SendPipelinesState  Nr: 0, PipelineId: 2, PipelinePriority: [2].
 [IntcOED] DEBUG SendPipelinesState Nr: 1, PipelineId: 3, PipelinePriority: [0].
.....
 [IntcOED] INFO SendMessage  Message data offset: 0x00000000 = 0x00000002
 [IntcOED] INFO SendMessage Message data offset: 0x00000004 = 0x00000002
 [IntcOED] INFO SendMessage  Message data offset: 0x00000008 = 0x00000003
 [IntcOED] INFO SendMessage Send IPC message: primary: 0x93000004 extension: 0x00000001 [SET_PIPELINE_STATE]

plbossart · 2023-09-07T12:54:38Z

@RanderWang are you sure about your assertion?

The pipeline priority is used by the firmware to schedule LL modules in an order defined by the pipeline priority.

That's different to saying that the pipelines are triggered in the order defined by the pipeline priority.

It'd like to make sure we don't overload a concept required by the LL firmware scheduling with a concept that is required on the host driver side.

RanderWang · 2023-09-08T03:28:09Z

@RanderWang are you sure about your assertion?

The pipeline priority is used by the firmware to schedule LL modules in an order defined by the pipeline priority.

That's different to saying that the pipelines are triggered in the order defined by the pipeline priority.

It'd like to make sure we don't overload a concept required by the LL firmware scheduling with a concept that is required on the host driver side.

In REF and cSOF FW, we process pipeline by order in the payload for IPC message. The first one has the highest priority and will be triggered first. In REF driver, it sorts all pipeline line according to priority. Yes, the priority is for LL scheduler since it is included in IPC msg, but it is also for trigger. If the low priority pipeline was put at first entry, it would be triggered first based on order, how can priority make effect at this time ? The ll scheduler doesn't manage trigger order.

plbossart · 2023-09-12T13:09:14Z

Sorry @RanderWang I am not following how firmware framework could trigger pipelines, when its the host driver that's require to send the IPC to each pipeline.

Maybe @ranj063 understands the feedback?

RanderWang · 2023-09-13T01:58:25Z

Sorry @RanderWang I am not following how firmware framework could trigger pipelines, when its the host driver that's require to send the IPC to each pipeline.

Maybe @ranj063 understands the feedback?

@plbossart Firmware triggers pipelines based on the pipeline in ipc message payload (ppl_id[]), the first entry is triggered first

struct ipc4_pipeline_set_state_data {
	/**< Number of items in ppl_id[] */
	uint32_t pipelines_count;
	/**< Pipeline ids */
	uint32_t ppl_id[];
} __attribute__((packed, aligned(4)));

for(i = 0; i < pipelines_count; i++)
     trigger(ppl_id[i]);

RanderWang · 2023-09-13T02:06:23Z

sort pipeline based on priority is a simple method and user can control it in topology. Change widget setup order to change pipeline trigger order is a indirectly way and may have bugs for a batch of widgets in a stream with multiple pipelines.

ujfalusi · 2023-09-13T12:43:46Z

@RanderWang, so basically when we have sorted the pipelines in whatever order, we run a sorting algorithm on it based on the priorities and that double sorted list should be passed to firmware, right?

ujfalusi · 2023-09-13T12:48:04Z

Sorry @RanderWang I am not following how firmware framework could trigger pipelines, when its the host driver that's require to send the IPC to each pipeline.

If we send individual set state messages then firmware will do the trigger based on that.
If we send the state change with multiple components (pipelines) then it is just a for() loop to trigger them one by one.
According to @RanderWang this is the 'algorithm' used by the reference firmware as well, so the host should be sorting the components in a specific order for the firmware to trigger.

plbossart · 2023-09-13T13:11:51Z

I am not able to follow this thread. we all agree we need to trigger from sink to source, but if the priority also comes into play then what happens? Does the priority for a pipeline have precedence on the position of the pipeline?

I just don't get where this is going, so removing myself from the reviewers at this point.

ranj063 · 2023-09-13T14:28:54Z

I am not able to follow this thread. we all agree we need to trigger from sink to source, but if the priority also comes into play then what happens? Does the priority for a pipeline have precedence on the position of the pipeline?

I just don't get where this is going, so removing myself from the reviewers at this point.

@ujfalusi @RanderWang @plbossart I dont understand the feedback either. Basically this PR is critical for the AEC pipeline to work with the SOF PR 8101. @RanderWang please feel free to submit an alternate solution. @RDharageswari and @yongzhi1 can help you verify your solution. Until then I will keep this PR open

RanderWang · 2023-09-14T11:59:53Z

so basically when we have sorted the pipelines in whatever order, we run a sorting algorithm on it based on the priorities and that double sorted list should be passed to firmware, right?

"double sorted list should be passed to firmware" ? I can't fully understand it. Send sorted pipeline index array to FW.

RanderWang · 2023-09-14T12:01:31Z

I am not able to follow this thread. we all agree we need to trigger from sink to source, but if the priority also comes into play then what happens? Does the priority for a pipeline have precedence on the position of the pipeline?
I just don't get where this is going, so removing myself from the reviewers at this point.

@ujfalusi @RanderWang @plbossart I dont understand the feedback either. Basically this PR is critical for the AEC pipeline to work with the SOF PR 8101. @RanderWang please feel free to submit an alternate solution. @RDharageswari and @yongzhi1 can help you verify your solution. Until then I will keep this PR open

Sure, I will prepare a PR.

ujfalusi · 2023-09-14T12:12:56Z

so basically when we have sorted the pipelines in whatever order, we run a sorting algorithm on it based on the priorities and that double sorted list should be passed to firmware, right?

"double sorted list should be passed to firmware" ? I can't fully understand it. Send sorted pipeline index array to FW.

We sort the pipeline from sink to source (or source to sink, I got lost) this is the first sort, then we sort this again based on 'priority', that's the second sort, so double sorting, no?

RanderWang · 2023-09-14T14:09:36Z

so basically when we have sorted the pipelines in whatever order, we run a sorting algorithm on it based on the priorities and that double sorted list should be passed to firmware, right?

"double sorted list should be passed to firmware" ? I can't fully understand it. Send sorted pipeline index array to FW.

We sort the pipeline from sink to source (or source to sink, I got lost) this is the first sort, then we sort this again based on 'priority', that's the second sort, so double sorting, no?

@ujfalusi we don't need the first sort of sink to source, just sort with priority

One problem is that all topology set priority with zero now, I will reserve current sort and sort with priority again (zero priority doesn't change sort result) so that it will not break current topology setting. Then we can add priority for AEC topology.

RDharageswari

the following sequence fails:
1)start speaker playback
2)start aec capture
3)ctrl+c capture
4)start aec capture
i/o error is observed for the capture

RanderWang · 2023-09-15T09:00:41Z

hi all, please check : #4593 and thesofproject/sof#8204. Thanks!

ranj063 · 2023-09-27T17:26:54Z

replaced by #4593

ranj063 requested review from lgirdwood, plbossart, kv2019i, dbaluta, bardliao, ujfalusi, vijendarmukunda, kuanhsuncheng and yaochunhung as code owners August 25, 2023 22:22

dbaluta previously approved these changes Aug 28, 2023

View reviewed changes

kv2019i reviewed Aug 28, 2023

View reviewed changes

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

bardliao requested a review from RanderWang August 28, 2023 12:03

This was referenced Aug 28, 2023

Modify the echo reference capture pipeline thesofproject/sof#8101

Merged

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

Closed

plbossart reviewed Aug 28, 2023

View reviewed changes

ranj063 dismissed dbaluta’s stale review via d7af406 August 29, 2023 00:58

ranj063 force-pushed the sof-dev-082523 branch from 4c3e3eb to d7af406 Compare August 29, 2023 00:58

ranj063 force-pushed the sof-dev-082523 branch from d7af406 to 0ee3d4a Compare August 29, 2023 16:39

RDharageswari reviewed Aug 30, 2023

View reviewed changes

yongzhi1 mentioned this pull request Sep 6, 2023

[BUG] [MTL] Playback clips captured from mock RTC AEC device is double speeded thesofproject/sof#8022

Closed

RanderWang reviewed Sep 7, 2023

View reviewed changes

RDharageswari suggested changes Sep 14, 2023

View reviewed changes

ranj063 closed this Sep 27, 2023

yongzhi1 mentioned this pull request Sep 29, 2023

Audio: Convert Google RTC audio processing to module API thesofproject/sof#8258

Merged

1 task

		@@ -332,12 +332,12 @@ static int sof_ipc4_trigger_pipelines(struct snd_soc_component *component,
		* guaranteed for each fork independently.

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

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

Conversation

ranj063 commented Aug 25, 2023

kv2019i left a comment

Choose a reason for hiding this comment

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang commented Aug 29, 2023

ranj063 commented Aug 29, 2023

RDharageswari left a comment

Choose a reason for hiding this comment

plbossart commented Sep 6, 2023

ranj063 commented Sep 6, 2023

RanderWang left a comment • edited Loading

Choose a reason for hiding this comment

RanderWang commented Sep 7, 2023

plbossart commented Sep 7, 2023

RanderWang commented Sep 8, 2023 • edited Loading

plbossart commented Sep 12, 2023

RanderWang commented Sep 13, 2023

RanderWang commented Sep 13, 2023

ujfalusi commented Sep 13, 2023

ujfalusi commented Sep 13, 2023

plbossart commented Sep 13, 2023

ranj063 commented Sep 13, 2023

RanderWang commented Sep 14, 2023

RanderWang commented Sep 14, 2023

ujfalusi commented Sep 14, 2023

RanderWang commented Sep 14, 2023 • edited Loading

RDharageswari left a comment

Choose a reason for hiding this comment

RanderWang commented Sep 15, 2023

ranj063 commented Sep 27, 2023

ranj063 Aug 28, 2023 •

edited

Loading

RanderWang left a comment •

edited

Loading

RanderWang commented Sep 8, 2023 •

edited

Loading

RanderWang commented Sep 14, 2023 •

edited

Loading