sof icon indicating copy to clipboard operation
sof copied to clipboard

Topology2: add ssp multi stream capture

Open bardliao opened this issue 2 years ago • 12 comments

This PR added multi stream capture support on SSP.

bardliao avatar Sep 25 '22 02:09 bardliao

@bardliao @RanderWang which platform were you able to test this on ?

I tested it on ADL & MTL devices.

RanderWang avatar Sep 26 '22 06:09 RanderWang

@bardliao @RanderWang are those topologies used in CI? Or can you share what they look like with the png image?

plbossart avatar Sep 26 '22 08:09 plbossart

@bardliao @RanderWang are those topologies used in CI? Or can you share what they look like with the png image?

No, they are not used in CI. cavs-tgl-nocodec

bardliao avatar Sep 26 '22 12:09 bardliao

@bardliao @RanderWang which platform were you able to test this on ?

I tested it on TGL

bardliao avatar Sep 26 '22 12:09 bardliao

@bardliao not sure if you saw my comments last week but we'd want to have one 'master' gain and 2 'local' gains for the two PCMs. Once we've done the gains we can replace the master gain by EQIIR for DMICs, which is badly needed as confirmed by @singalsu

plbossart avatar Sep 26 '22 12:09 plbossart

@bardliao not sure if you saw my comments last week but we'd want to have one 'master' gain and 2 'local' gains for the two PCMs. Once we've done the gains we can replace the master gain by EQIIR for DMICs, which is badly needed as confirmed by @singalsu

@plbossart I updated my PR. New topology looks like cavs-tgl-nocodec Is this what you expect?

bardliao avatar Sep 27 '22 07:09 bardliao

@bardliao yes this is what I had in mind, I was wondering if we could actually use 3 copiers in this way,

One remark that I have for topologies is that we may want to be clearer on indices used. It's not clear to me is if the copier.SSP2.1 refers to pipeline2 or SSP2. We should have something that help bind copiers to interfaces, otherwise it'll be a support nightmare. @bardliao @RanderWang @ranj063 @singalsu @lgirdwood thoughts?

plbossart avatar Sep 27 '22 08:09 plbossart

@bardliao yes this is what I had in mind, I was wondering if we could actually use 3 copiers in this way,

@plbossart Currently, only DAI and host type copiers are supported by kernel. I am adding support for the copier "module" type.

bardliao avatar Sep 27 '22 09:09 bardliao

@bardliao yes this is what I had in mind, I was wondering if we could actually use 3 copiers in this way,

One remark that I have for topologies is that we may want to be clearer on indices used. It's not clear to me is if the copier.SSP2.1 refers to pipeline2 or SSP2. We should have something that help bind copiers to interfaces, otherwise it'll be a support nightmare. @bardliao @RanderWang @ranj063 @singalsu @lgirdwood thoughts?

@plbossart the name should be "component_name.pipeline_id.component_id" . so copier.SSP.2.1 means this copier.SSP1 is in pipeline 2 and copier.host.1.1 = copier.host1 in pipeline 1

RanderWang avatar Sep 28 '22 01:09 RanderWang

@bardliao yes this is what I had in mind, I was wondering if we could actually use 3 copiers in this way,

@plbossart Currently, only DAI and host type copiers are supported by kernel. I am adding support for the copier "module" type.

ok, sounds good.

plbossart avatar Sep 28 '22 08:09 plbossart

@bardliao yes this is what I had in mind, I was wondering if we could actually use 3 copiers in this way, One remark that I have for topologies is that we may want to be clearer on indices used. It's not clear to me is if the copier.SSP2.1 refers to pipeline2 or SSP2. We should have something that help bind copiers to interfaces, otherwise it'll be a support nightmare. @bardliao @RanderWang @ranj063 @singalsu @lgirdwood thoughts?

@plbossart the name should be "component_name.pipeline_id.component_id" . so copier.SSP.2.1 means this copier.SSP1 is in pipeline 2 and copier.host.1.1 = copier.host1 in pipeline 1

My point exactly, we have NOTHING that tells us which SSP this copier deals with. The .1 suffix is meaningless for a user trying to understand what the topology does.

plbossart avatar Sep 28 '22 08:09 plbossart

@bardliao not sure if you saw my comments last week but we'd want to have one 'master' gain and 2 'local' gains for the two PCMs. Once we've done the gains we can replace the master gain by EQIIR for DMICs, which is badly needed as confirmed by @singalsu

@plbossart I updated my PR. New topology looks like cavs-tgl-nocodec Is this what you expect?

@bardliao why do we need copier.module.8.2 and gain.8.1 at all?

ranj063 avatar Oct 14 '22 02:10 ranj063

@kv2019i @RanderWang @ranj063 pls review

@bardliao do you finalize the design ?

RanderWang avatar Oct 20 '22 01:10 RanderWang

@kv2019i @RanderWang @ranj063 pls review

@bardliao do you finalize the design ?

@lgirdwood @RanderWang That is a good question. I hope this is the final design. But the IPC4 tester will report bind error with this topology. Removing the gain module in the host pipelines can fix the error. But the IPC4 tester will crash when the second capture PCM is started after fixing the bind error issue. I am not sure if I need to modify the topology or we need to fix the IPC4 tester issue.

bardliao avatar Oct 20 '22 09:10 bardliao

@kv2019i @RanderWang @ranj063 pls review

@bardliao do you finalize the design ?

@lgirdwood @RanderWang That is a good question. I hope this is the final design. But the IPC4 tester will report bind error with this topology. Removing the gain module in the host pipelines can fix the error. But the IPC4 tester will crash when the second capture PCM is started after fixing the bind error issue. I am not sure if I need to modify the topology or we need to fix the IPC4 tester issue.

ok, does the topology work on main branch and not on tester ?

lgirdwood avatar Oct 24 '22 14:10 lgirdwood

ok, does the topology work on main branch and not on tester ?

Yes

bardliao avatar Oct 25 '22 02:10 bardliao

ok, does the topology work on main branch and not on tester ?

Yes

ok, this is fine. It can be merged.

lgirdwood avatar Oct 25 '22 09:10 lgirdwood

I am not comfortable with differences in behavior between different implementations of the IPC4 protocol and sequences.

We already have a number of issues related to trigger, now this is with bind. This probably highlights a conceptual miss somewhere.

plbossart avatar Oct 25 '22 13:10 plbossart

Can one of the admins verify this patch?

gkbldcig avatar Oct 26 '22 02:10 gkbldcig

ok, does the topology work on main branch and not on tester ?

Yes

ok, this is fine. It can be merged.

@lgirdwood if we merge this with the tester failing, we would lose our reference point. I have a feeling we're missing something trivial in tplg. Let me take a look

ranj063 avatar Oct 26 '22 02:10 ranj063

@bardliao this is the fix you need and this is how the tplg will look like: 0001-gain-capture-Add-copier-module.txt tplg

@plbossart @RanderWang what do you think?

ranj063 avatar Oct 26 '22 03:10 ranj063

@bardliao this is the fix you need and this is how the tplg will look like: 0001-gain-capture-Add-copier-module.txt tplg

This fixed the binding error, but the IPC4 tester will still crash when the second capture PCM is started.

bardliao avatar Oct 26 '22 10:10 bardliao

This fixed the binding error, but the IPC4 tester will still crash when the second capture PCM is started.

@RanderWang pls let the tester folks know about this tester bug and CC @ranj063 . @ranj063 the topology looks valid and is permitted in the ABI, it could be a restriction in the tester though.

lgirdwood avatar Oct 26 '22 10:10 lgirdwood

This fixed the binding error, but the IPC4 tester will still crash when the second capture PCM is started.

@RanderWang pls let the tester folks know about this tester bug and CC @ranj063 . @ranj063 the topology looks valid and is permitted in the ABI, it could be a restriction in the tester though.

@bardliao pls let the tester folks know about this tester bug and CC

RanderWang avatar Oct 27 '22 11:10 RanderWang

@bardliao @RanderWang @ranj063 any blocker now (except for the conflict) ?

lgirdwood avatar Oct 27 '22 20:10 lgirdwood

@bardliao @RanderWang @ranj063 any blocker now (except for the conflict) ?

@lgirdwood we fixed one issue with the copier yesterday to make this tplg work. We'd have to run more test cases before we merge this. I'll let @bardliao start a test run and report back when things look decent

ranj063 avatar Oct 27 '22 20:10 ranj063

@bardliao @RanderWang @ranj063 any blocker now (except for the conflict) ?

@lgirdwood we fixed one issue with the copier yesterday to make this tplg work. We'd have to run more test cases before we merge this. I'll let @bardliao start a test run and report back when things look decent

and oh I forgot the kernel PR #3957 is a pre-requisite for this

ranj063 avatar Oct 27 '22 20:10 ranj063

This fixed the binding error, but the IPC4 tester will still crash when the second capture PCM is started.

@RanderWang pls let the tester folks know about this tester bug and CC @ranj063 . @ranj063 the topology looks valid and is permitted in the ABI, it could be a restriction in the tester though.

@bardliao pls let the tester folks know about this tester bug and CC

Yes, I sent email to them and CC you and Ranjani on Oct. 26th.

bardliao avatar Oct 28 '22 06:10 bardliao

@bardliao @RanderWang @ranj063 any blocker now (except for the conflict) ?

@lgirdwood @ranj063 The conflict will be fixed if https://github.com/thesofproject/sof/pull/6499 is merged.

I tested with https://github.com/thesofproject/linux/pull/3888 + https://github.com/thesofproject/linux/pull/3957 and didn't find any issue.

bardliao avatar Oct 28 '22 07:10 bardliao

@bardliao @RanderWang @ranj063 any blocker now (except for the conflict) ?

@lgirdwood @ranj063 The conflict will be fixed if #6499 is merged.

I tested with thesofproject/linux#3888 + thesofproject/linux#3957 and didn't find any issue.

All dependencies now merged. Lets get it in daily test for a few days.

lgirdwood avatar Nov 09 '22 14:11 lgirdwood