topology2: use a macro to config if a sdw amplifier support feedback #6915

bardliao · 2023-01-05T05:45:56Z

Not all amplifiers support feedback. We should not add feedback support
on topology to shch amplifiers.

plbossart · 2023-01-05T15:08:29Z

tools/topology/topology2/platform/intel/sdw-amp-generic.conf

indentation?

plbossart · 2023-01-05T15:11:38Z

tools/topology/topology2/platform/intel/sdw-amp-feedback.conf

I know I asked this before, but I don't recall what the answer was

Can we really encode this '515' value which is related to ALH channels? this might depend on which link and ALH channel is used for the feedback, no? So if this is in an include file should this be a parameter?

I know I asked this before, but I don't recall what the answer was

Can we really encode this '515' value which is related to ALH channels? this might depend on which link and ALH channel is used for the feedback, no? So if this is in an include file should this be a parameter?

@plbossart The ALH index (515 here) is not related to ALH channels. We get alh_stream_id from sdw_params_stream and set copier_data->gtw_cfg.node_id |= SOF_IPC4_NODE_INDEX(data->dai_data);
The only restriction of ALH index is that we can't have duplicated ALH index in a topology. In other words, we can have ALH.0, ALH.1, ALH.2, ALH.3, ALH.4 if we want.

oh, so if this is a convention that's totally arbitrary we should not use 515 which was from another era and tied to a specific link/PDI usage. Should we use the same numbers as for PCM devices then in our ALH. numbering? I mean the speaker could be ALH.2 and mic ALH.4.

oh, so if this is a convention that's totally arbitrary we should not use 515 which was from another era and tied to a specific link/PDI usage. Should we use the same numbers as for PCM devices then in our ALH. numbering? I mean the speaker could be ALH.2 and mic ALH.4.

I choose the same ALH. number as for IPC3 ALH number is because that way can guarantee we will not use duplicated ALH. number. A problem of using the same number as for PCM devices is that we need two ALH in an aggregated speaker device. For example, currently we have ALH.258 and ALH.514 for one aggregated speaker device, and we can't use ALH.2-1 and ALH.2-2

the ALH number is not fixed for IPC3, it's based on macros. I don't mind if we use the same convention but we don't want to encode the link position in the topology, because we'll have to redo this when we support different hardware layouts.

the ALH number is not fixed for IPC3, it's based on macros. I don't mind if we use the same convention but we don't want to encode the link position in the topology, because we'll have to redo this when we support different hardware layouts.

@plbossart Yeah, I know the ALH number is not fixed for IPC3. I meant in the most common case, we have

SDW0 DP2 for headset playback
SDW0 DP3 for headset capture
SDW1 DP2 for 1st speaker playback
SDW2 DP2 for 2nd speaker playback
SDW3 DP3 for dmic capture

Based on above layout I assigned IPC4 ALH number with the same formula as IPC3. i.e. 0x002 for headset playback, 0x003 for headset capture, 0x102 for 1st speaker, etc.

But we don't really care about the real HW layouts. We can still use ALH.2 for headset playback if the headset codec is not on SDW0.
In other words, the ALH index number doesn't need to be changed when the HW layouts are changed.
And we can use the same convention to add a new ALH dai.

Does it make sense?

sorry, I am not following what IPC3 convention you are referring to @bardliao since we don't have one?

Or maybe you are referring to the initial CML designs with 1 and 2 amps?

sorry, I am not following what IPC3 convention you are referring to @bardliao since we don't have one?

Or maybe you are referring to the initial CML designs with 1 and 2 amps?

@plbossart I was referring to the (link_id * 256 + data port) formula. But I just realize that we can use the same rule as the pipeline id and the route id. I.e. pcm id * 10 ~ pcm id * 10 + 9.
I will update the PR to follow the rule.

tools/topology/topology2/platform/intel/sdw-amp-feedback.conf

plbossart · 2023-01-05T15:12:54Z

tools/topology/topology2/platform/intel/sdw-amp-feedback.conf

comment is very cryptic, what are you trying to say here?

comment is very cryptic, what are you trying to say here?

That was copy paste mistake. I will remove the comment.

plbossart · 2023-01-05T15:14:14Z

tools/topology/topology2/platform/intel/sdw-amp-feedback.conf

what happens if we want to split the feedback? This is fine for an initial start, but we do know this is going to have to be split between the dai and host parts

what happens if we want to split the feedback? This is fine for an initial start, but we do know this is going to have to be split between the dai and host parts

@plbossart I am not sure if I get your question. The dai and host are in different pipelines now. If we want to add some widgets between dai and host, we can add the widgets in the pipeline and add routes for them.

you can add some macro definition like linking HOST_PIPELINE_MODULE to DAI_PIPELINE_MODULE

you can add some macro definition like linking HOST_PIPELINE_MODULE to DAI_PIPELINE_MODULE

@RanderWang I don't think it is necessary. source "copier.ALH.31.1" and sink "copier.host.30.1" are fixed. The pipelines are defined here in the same .conf file and the index are fixed.

tools/topology/topology2/avs-tplg/CMakeLists.txt

lgirdwood

topology syntax looks fine, but needs @plbossart approval for sdw.

plbossart

This looks mostly good, just two small opens that should be trivial to fix or confirm as correct. Thanks @bardliao

tools/topology/topology2/cavs-sdw.conf

plbossart · 2023-01-11T23:46:30Z

tools/topology/topology2/platform/intel/sdw-amp-feedback.conf

is the 515 here intentional? I thought these were removed

is the 515 here intentional? I thought these were removed

No. I forgot to change the index in hw_config. However, somehow it still works.

plbossart · 2023-01-12T02:47:30Z

tools/topology/topology2/cavs-sdw-src-gain-mixin.conf

not completely following the formula.

a) what is the relationship between pcm_id and ALH? there could be M:1 and 1:N topologies.
b) what is a route_id
c) why does the pipeline come into play.

That was a long day, bear with me

not completely following the formula.

@plbossart Please see f554a2f for your reference. I am trying to follow the same customary to assign ALH index.

a) what is the relationship between pcm_id and ALH? there could be M:1 and 1:N topologies.

ALH index = pcm_id * 10 + N, where N could be 0 to 9.
For example, say there is an aggregated speaker device
pcm_id = 2, and the ALH index will be 20, 21.

b) what is a route_id

For example,
route.0 {
source "gain.1.1"
sink "copier.ALH.1.1"
}
The route_id above is 0

c) why does the pipeline come into play.

The pipeline id also follows the same rule.

That was a long day, bear with me

@plbossart Are you good with this PR?

ranj063 · 2023-02-16T15:08:49Z

@bardliao @lgirdwood Can you please wait before merging this PR? I have a more elegant solution for avoid clashes between included file objects and top-level objects.

ranj063 · 2023-02-16T18:31:00Z

@bardliao @lgirdwood Can you please wait before merging this PR? I have a more elegant solution for avoid clashes between included file objects and top-level objects.

@bardliao this is the change I was talking about. Could you please update your PR based on that one? #7113

bardliao · 2023-02-17T02:31:20Z

@bardliao @lgirdwood Can you please wait before merging this PR? I have a more elegant solution for avoid clashes between included file objects and top-level objects.

@bardliao this is the change I was talking about. Could you please update your PR based on that one? #7113

@ranj063 I updated my PR based on #7113, will push after #7113 is merged.

kv2019i · 2023-02-17T07:17:15Z

@bardliao array-support merged in #7113 so rebase will be needed

bardliao · 2023-02-17T08:02:40Z

@bardliao array-support merged in #7113 so rebase will be needed

done

ranj063 · 2023-02-17T15:56:35Z

tools/topology/topology2/cavs-sdw-src-gain-mixin.conf

the commit message talks about pipeline ID and route ID but all im seeing is dai_index updates.

the commit message talks about pipeline ID and route ID but all im seeing is dai_index updates.

@ranj063 The pipeline ID and route ID is already grouped by f0a0100. This commit follows the same rule for ALH dai index. I will update the commit message to make it more intuition.

index We use the "pcm id * 10 + N" formula where N is from 0 to 9 to assign pipeline and route id in f0a0100 ("topology2: cavs-sdw: group route and pipeline index"). Now apply to ALH dai index. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Not all amplifiers support feedback. We should not add feedback support on topology to shch amplifiers. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

sof-adl-rt711-l0-rt1316-l12-rt714-l3.tplg is the same as sof-tgl-rt711-rt1316-rt714.tplg Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao requested review from RanderWang, plbossart and ranj063 January 5, 2023 05:45

bardliao requested review from aborisovich, dbaluta, lbetlej, lgirdwood, marc-hb and mmaka1 as code owners January 5, 2023 05:45

plbossart reviewed Jan 5, 2023

View reviewed changes

bardliao force-pushed the topology2-sdw-fixup branch 3 times, most recently from 75f1bd5 to 40e0ab2 Compare January 11, 2023 09:34

lgirdwood approved these changes Jan 11, 2023

View reviewed changes

plbossart requested changes Jan 11, 2023

View reviewed changes

bardliao force-pushed the topology2-sdw-fixup branch from 40e0ab2 to eb35cf6 Compare January 12, 2023 01:08

RanderWang approved these changes Jan 12, 2023

View reviewed changes

plbossart reviewed Jan 12, 2023

View reviewed changes

bardliao force-pushed the topology2-sdw-fixup branch from eb35cf6 to 3c20b66 Compare February 14, 2023 12:21

lgirdwood approved these changes Feb 16, 2023

View reviewed changes

lgirdwood changed the title ~~topology2: use a macro to config if a sdw amplifier support feedback~~ [DNM] topology2: use a macro to config if a sdw amplifier support feedback Feb 16, 2023

bardliao force-pushed the topology2-sdw-fixup branch from 3c20b66 to 60b3c5f Compare February 17, 2023 08:01

bardliao changed the title ~~[DNM] topology2: use a macro to config if a sdw amplifier support feedback~~ topology2: use a macro to config if a sdw amplifier support feedback Feb 17, 2023

ranj063 reviewed Feb 17, 2023

View reviewed changes

bardliao force-pushed the topology2-sdw-fixup branch from 60b3c5f to 74184ff Compare February 20, 2023 05:19

bardliao added 3 commits February 20, 2023 13:28

topology2: use a macro to config if a sdw amplifier support feedback

e40a68e

Not all amplifiers support feedback. We should not add feedback support on topology to shch amplifiers. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

topology2: add sof-adl-rt711-l0-rt1316-l12-rt714-l3 support

74184ff

sof-adl-rt711-l0-rt1316-l12-rt714-l3.tplg is the same as sof-tgl-rt711-rt1316-rt714.tplg Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

lgirdwood approved these changes Feb 20, 2023

View reviewed changes

ranj063 approved these changes Feb 21, 2023

View reviewed changes

lgirdwood merged commit 0235673 into thesofproject:main Feb 21, 2023

topology2: use a macro to config if a sdw amplifier support feedback #6915

topology2: use a macro to config if a sdw amplifier support feedback #6915

Uh oh!

Conversation

bardliao commented Jan 5, 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!

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!

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!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

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!

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 Feb 16, 2023

Uh oh!

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

Uh oh!

Uh oh!

bardliao commented Feb 17, 2023

Uh oh!

kv2019i commented Feb 17, 2023

Uh oh!

bardliao commented Feb 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ranj063 commented Feb 16, 2023 •

edited

Loading