topology2: cavs-sdw: add Speaker and Microphone pcm #6549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

lgirdwood merged 3 commits into thesofproject:main from bardliao:topology2-sdw-spk

Dec 9, 2022

Collaborator

bardliao commented Nov 7, 2022

Add Speaker PCM to cavs-sdw topology. The default is for no aggregated speaker and AGGREGATED=1 should be set in CMakeLists.txt for aggregated speaker.
The 2nd ALH copier stream name should be the same as the 1st ALH copier, and no need to connect to the route.
The 2nd ALH copier data will be set to the 1st ALH copier's gateway config in the aggregated mode.

Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com

bardliao requested review from RanderWang and ranj063

November 7, 2022 12:27

bardliao requested review from aborisovich, dbaluta, lbetlej, lgirdwood, marc-hb, mmaka1 and plbossart as code owners

November 7, 2022 12:27

lgirdwood reviewed

View reviewed changes

tools/topology/topology2/cavs/cavs-sdw.conf Outdated

Member

lgirdwood Nov 7, 2022

Should we prefix with SDW_ ?

Collaborator Author

bardliao Nov 8, 2022

Should we prefix with SDW_ ?

ack

plbossart requested changes

View reviewed changes

Member

plbossart left a comment

@bardliao @ranj063 I believe it's time we clean-up the topology and make them user/developer friendly. It's nearly impossible to figure out which control is related to what pipe/PCM/dai.

tools/topology/topology2/cavs/cavs-sdw.conf Outdated

Member

plbossart Nov 7, 2022

@bardliao we need to start thinking about possible variations of the hardware layout. I think we should really create an 'amplifier' reference file and set the actual link/PDI number with macros, as we did for topology1.

Collaborator Author

bardliao Nov 8, 2022

@bardliao we need to start thinking about possible variations of the hardware layout. I think we should really create an 'amplifier' reference file and set the actual link/PDI number with macros, as we did for topology1.

Updated

Collaborator Author

bardliao Nov 8, 2022

The ALH index is not used by driver. link_id will be provided by callback function.

tools/topology/topology2/cavs/cavs-sdw.conf Outdated

Member

plbossart Nov 7, 2022

naming seems weird here, what does copier have to do with volume?

Collaborator Author

bardliao Nov 8, 2022

naming seems weird here, what does copier have to do with volume?

Renamed to "sdw amplifiers"

tools/topology/topology2/cavs/cavs-sdw.conf Outdated

Member

plbossart Nov 7, 2022

we really need to have meaningful PGA control names.

Member

lgirdwood Nov 8, 2022 •

edited

Loading

ack - the tplg compiler needs to be clever enough to name accordingly i.e. base class names would be templates where instance names (from toplevel) would be used. This would become
Playback Volume $ where $ could be the instance name like Headphones
All of this can be done for v2.5/v2.6

tools/topology/topology2/cavs/cavs-sdw.conf Outdated

Member

plbossart Nov 7, 2022

Amplifier Volume or something.

Collaborator Author

bardliao Nov 8, 2022

Amplifier Volume or something.

Updated

bardliao force-pushed the topology2-sdw-spk branch from ed8693b to 3dbbf65 Compare

November 8, 2022 09:55

plbossart requested changes

View reviewed changes

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

Member

plbossart Nov 8, 2022

S24_LE?

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

Member

plbossart Nov 8, 2022

too many closing braces, does this work?

Collaborator Author

bardliao Nov 9, 2022

too many closing braces, does this work?

Thanks @plbossart This is an indentation issue. Fixing it now.

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

Member

plbossart Nov 8, 2022

this stream_name seems useless. @ranj063 do we need this?

Collaborator Author

bardliao Nov 9, 2022

this stream_name seems useless. @ranj063 do we need this?

As I know, the stream_name maps to pcm_caps name. So, it is needed.

tools/topology/topology2/cavs/CMakeLists.txt Outdated

Member

plbossart Nov 8, 2022

I think you missed my feedback @bardliao

We should start using topology names that can be directly used by the kernel, so something like rt711-l0-rt1316-l12-rt715-l3 is needed. It's not just about adding a variable number of amplifiers, we also need to deal with the NOJACK case. In other words all variations we had for topology1 need to be supported here.

tools/topology/topology2/cavs/platform/intel/alh-2nd-spk.conf Outdated

Member

plbossart Nov 8, 2022

is this really passthrough here?

Collaborator Author

bardliao Nov 9, 2022

is this really passthrough here?

Yes, basically, passthrough-be pipeline contains a BE dai copier and that's what we need.

bardliao force-pushed the topology2-sdw-spk branch 3 times, most recently from 82a6e56 to 933570c Compare

November 9, 2022 08:08

jairaj-arava mentioned this pull request

topology2: cavs: Add SDW speaker,headset,hdmi and DMIC support #6562

Closed

bardliao force-pushed the topology2-sdw-spk branch 2 times, most recently from c06ed44 to 3714fd6 Compare

November 14, 2022 10:14

bardliao changed the title ~~topology2: cavs-sdw: add speaker pcm~~ topology2: cavs-sdw: add Speaker and Microphone pcm

bardliao force-pushed the topology2-sdw-spk branch 3 times, most recently from 6d99b40 to 0b51ff5 Compare

November 14, 2022 12:53

Collaborator Author

bardliao commented Nov 14, 2022

@plbossart @ranj063 I use macro to define BE id and name. Could you review it?

Collaborator

ranj063 commented Nov 14, 2022

@plbossart @ranj063 I use macro to define BE id and name. Could you review it?

@bardliao I think the macro still won't be enough. What we need to do is remove the dai_index from being the unique attribute in the class DAI's and then set them using the formula to compute the link ID in the top-level topologies just like we do for topology 1

Collaborator Author

bardliao commented Nov 15, 2022

@plbossart @ranj063 I use macro to define BE id and name. Could you review it?

@bardliao I think the macro still won't be enough. What we need to do is remove the dai_index from being the unique attribute in the class DAI's and then set them using the formula to compute the link ID in the top-level topologies just like we do for topology 1

@ranj063 dai_index is not used by kernel. We don't need to compute link ID in topology at all. We use the .params_stream callback to get the index from SoundWire bus driver. See sdw_params_stream

Collaborator

ranj063 commented Nov 17, 2022

@plbossart @ranj063 I use macro to define BE id and name. Could you review it?

@bardliao I think the macro still won't be enough. What we need to do is remove the dai_index from being the unique attribute in the class DAI's and then set them using the formula to compute the link ID in the top-level topologies just like we do for topology 1

@ranj063 dai_index is not used by kernel. We don't need to compute link ID in topology at all. We use the .params_stream callback to get the index from SoundWire bus driver. See sdw_params_stream

@bardliao for ALH the dai_index is of course used by the kernel
https://github.com/thesofproject/linux/blob/0802a7fba40856d3f262001143143eaf18f89f58/sound/soc/sof/ipc4-topology.c#L2144

What am I missing?

Member

plbossart commented Nov 17, 2022

@ranj063 I agree that for IPC3 the kernel does not "use" any dai_index coming from topology, the only thing that the kernel does is pass the 'stream_id', rate and channels in the dai_config, and the 'stream_id' is generated from the PDI ID+linkID.

I must admit I have no idea why we encoded the stream_id in the topology,

DAI_CONFIG(ALH, eval(UAJ_LINK * 256 + 2), 0, `SDW'eval(UAJ_LINK)`-Playback',
	ALH_CONFIG(ALH_CONFIG_DATA(ALH, eval(UAJ_LINK * 256 + 2), 48000, 2)))

because we absolutely override whatever is provided by topology.

No idea how IPC4 works for SoundWire.

Collaborator

ranj063 commented Nov 17, 2022 •

edited

Loading

@ranj063 I agree that for IPC3 the kernel does not "use" any dai_index coming from topology, the only thing that the kernel does is pass the 'stream_id', rate and channels in the dai_config, and the 'stream_id' is generated from the PDI ID+linkID.

I must admit I have no idea why we encoded the stream_id in the topology,
DAI_CONFIG(ALH, eval(UAJ_LINK * 256 + 2), 0, `SDW'eval(UAJ_LINK)`-Playback',
	ALH_CONFIG(ALH_CONFIG_DATA(ALH, eval(UAJ_LINK * 256 + 2), 48000, 2)))
because we absolutely override whatever is provided by topology.

No idea how IPC4 works for SoundWire.

I think I understand Bard's point now. So we use the DAI index but not the one coming from topology and it is true for both IPC3 and IPC4. As far as the encoding in the topology goes, that also makes sense, right? We need it to set the DAI widget names.

Now, this PR is OK as it is but I plan to rename the copiers with the DAI_INDEX, see #6627.

So after that Im not sure if harcoding the DAI index in the DAI definition makes sense.

Member

plbossart commented Nov 17, 2022

Now, this PR is OK as it is but I plan to rename the copiers with the DAI_INDEX, see #6627.

So after that Im not sure if harcoding the DAI index in the DAI definition makes sense.

Indeed. The dai_index is intuitive for SSPs only. For DMICs it's rather confusing, since the hardware can support two streams. For HDaudio it makes no sense, and for SoundWire not so much either. we could make the dai_index an option to be used where it helps (i.e. SSP only) and leave it out. My proposal to use pipeline_id.position_in_pipeline.[pcm/dai_index] would cover most cases /me/ thinks?

Collaborator

ranj063 commented Nov 17, 2022

Now, this PR is OK as it is but I plan to rename the copiers with the DAI_INDEX, see #6627.
So after that Im not sure if harcoding the DAI index in the DAI definition makes sense.

Indeed. The dai_index is intuitive for SSPs only. For DMICs it's rather confusing, since the hardware can support two streams. For HDaudio it makes no sense, and for SoundWire not so much either. we could make the dai_index an option to be used where it helps (i.e. SSP only) and leave it out. My proposal to use pipeline_id.position_in_pipeline.[pcm/dai_index] would cover most cases /me/ thinks?

@plbossart Can you please give me an example of position_in_pipeline?

Member

plbossart commented Nov 17, 2022 •

edited

Loading

Now, this PR is OK as it is but I plan to rename the copiers with the DAI_INDEX, see #6627.
So after that Im not sure if harcoding the DAI index in the DAI definition makes sense.

Indeed. The dai_index is intuitive for SSPs only. For DMICs it's rather confusing, since the hardware can support two streams. For HDaudio it makes no sense, and for SoundWire not so much either. we could make the dai_index an option to be used where it helps (i.e. SSP only) and leave it out. My proposal to use pipeline_id.position_in_pipeline.[pcm/dai_index] would cover most cases /me/ thinks?

@plbossart Can you please give me an example of position_in_pipeline?

sure, using pipeline 10, we'd have

copier.hda.10.1
gain.10.2
eq.10.3
demux.10.4

or

mixer.10.1
gain.10.2
copier.playback.10.3.0 (SSP0)

Collaborator

ranj063 commented Nov 17, 2022

sure, using pipeline 10, we'd have

Oh you meant the actual position in the pipeline. Now having an optional value in the name is something the tplg2 compiler doesnt support. But we can just use a separate class for SSP type DAI copier with some code duplication but not too bad I guess

Collaborator Author

bardliao commented Nov 18, 2022

@ranj063 @plbossart
https://github.com/thesofproject/linux/blob/0802a7fba40856d3f262001143143eaf18f89f58/sound/soc/sof/ipc4-topology.c#L2144
https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/intel/hda.c#L67
data->dai_data is from the .params_stream callback, not from topology. And in fact, the ALH dai_index is always 0 from topology.

[    4.613768] snd_sof:sof_ipc4_widget_setup_comp_dai: sof-audio-pci-intel-tgl 0000:00:1f.3: dai copier.ALH.41.1 node_type 17 dai_type 4 dai_index 0
[    4.613853] snd_sof:sof_ipc4_widget_setup_comp_dai: sof-audio-pci-intel-tgl 0000:00:1f.3: dai copier.ALH.22.1 node_type 16 dai_type 4 dai_index 0
[    4.613910] snd_sof:sof_ipc4_widget_setup_comp_dai: sof-audio-pci-intel-tgl 0000:00:1f.3: dai copier.ALH.21.1 node_type 16 dai_type 4 dai_index 0
[    4.614013] snd_sof:sof_ipc4_widget_setup_comp_dai: sof-audio-pci-intel-tgl 0000:00:1f.3: dai copier.ALH.11.1 node_type 17 dai_type 4 dai_index 0
[    4.614134] snd_sof:sof_ipc4_widget_setup_comp_dai: sof-audio-pci-intel-tgl 0000:00:1f.3: dai copier.ALH.1.1 node_type 16 dai_type 4 dai_index 0

But for IPC3, we do need dai_index from topology. Because we need to send IPC before the .params_stream callback is called.

Member

plbossart commented Nov 18, 2022

But for IPC3, we do need dai_index from topology. Because we need to send IPC before the .params_stream callback is called.

which IPC is needed @bardliao, this is not clear to me?

Collaborator Author

bardliao commented Nov 22, 2022

But for IPC3, we do need dai_index from topology. Because we need to send IPC before the .params_stream callback is called.

which IPC is needed @bardliao, this is not clear to me?

We need comp_dai->dai_index to setup the widget in sof_ipc3_widget_setup. The IPC is ipc tx: 0x30010000: GLB_TPLG_MSG: COMP_NEW.
sof_ipc3_widget_setup will be called after sof_ipc3_dai_config where we get config->dai_index and config->alh.stream_id from the .params_stream callback. But we get comp_dai->dai_index from topology in sof_ipc3_widget_setup_comp_dai when topology is loaded.

We may overwrite comp_dai->dai_index in sof_ipc3_dai_config and send the right dai_index in sof_ipc3_widget_setup. I will work a PR for it.

Collaborator Author

bardliao commented Nov 22, 2022

But for IPC3, we do need dai_index from topology. Because we need to send IPC before the .params_stream callback is called.

which IPC is needed @bardliao, this is not clear to me?

We need comp_dai->dai_index to setup the widget in sof_ipc3_widget_setup. The IPC is ipc tx: 0x30010000: GLB_TPLG_MSG: COMP_NEW. sof_ipc3_widget_setup will be called after sof_ipc3_dai_config where we get config->dai_index and config->alh.stream_id from the .params_stream callback. But we get comp_dai->dai_index from topology in sof_ipc3_widget_setup_comp_dai when topology is loaded.

We may overwrite comp_dai->dai_index in sof_ipc3_dai_config and send the right dai_index in sof_ipc3_widget_setup. I will work a PR for it.

PR: thesofproject/linux#4036

Collaborator

RanderWang commented Nov 23, 2022 •

edited

Loading

@ranj063 I agree that for IPC3 the kernel does not "use" any dai_index coming from topology, the only thing that the kernel does is pass the 'stream_id', rate and channels in the dai_config, and the 'stream_id' is generated from the PDI ID+linkID.

I must admit I have no idea why we encoded the stream_id in the topology,
DAI_CONFIG(ALH, eval(UAJ_LINK * 256 + 2), 0, `SDW'eval(UAJ_LINK)`-Playback',
	ALH_CONFIG(ALH_CONFIG_DATA(ALH, eval(UAJ_LINK * 256 + 2), 48000, 2)))
because we absolutely override whatever is provided by topology.

No idea how IPC4 works for SoundWire.

@plbossart dai index is built in fw based on node_id in copier

dai_index = IPC4_ALH_DAI_INDEX(node_id.f.v_index);

/* copier id = (group id << 4) + codec id + IPC4_ALH_DAI_INDEX_OFFSET
 * dai_index = (group id << 8) + codec id;
 */
#define IPC4_ALH_DAI_INDEX(x) ((((x) & 0xF0) << DAI_NUM_ALH_BI_DIR_LINKS_GROUP) + \
				(((x) & 0xF) - IPC4_ALH_DAI_INDEX_OFFSET))

bardliao force-pushed the topology2-sdw-spk branch from 0b51ff5 to 76a9f21 Compare

December 1, 2022 02:18

ranj063 reviewed

View reviewed changes

tools/topology/topology2/platform/intel/sdw-dmic-generic.conf Outdated

Collaborator

ranj063 Dec 1, 2022

is this really needed?

Collaborator Author

bardliao Dec 2, 2022

is this really needed?

Removed. Thanks

ranj063 reviewed

View reviewed changes

tools/topology/topology2/avs-tplg/CMakeLists.txt Outdated Show resolved Hide resolved

bardliao force-pushed the topology2-sdw-spk branch from 76a9f21 to 65e2b5d Compare

December 2, 2022 10:08

RanderWang reviewed

View reviewed changes

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

Collaborator

RanderWang Dec 2, 2022 •

edited

Loading

this 'Amplifier Volume' is the same as above 'Amplifier Volume", you can call it "Main Amplifier Volume"

Collaborator Author

bardliao Dec 5, 2022

@RanderWang @ranj063 The widget name will be added in the control name. So the two 'Amplifier Volume' controls are gain.20.1 Amplifier Volume and gain.21.1 Amplifier Volume. Do we need to add different prefix for them?

Collaborator

RanderWang Dec 5, 2022

gain.20 or gain21 are not meaning and will confuse user

Collaborator Author

bardliao Dec 6, 2022

gain.20 or gain21 are not meaning and will confuse user

ok. updated.

bardliao force-pushed the topology2-sdw-spk branch from 65e2b5d to 8238fc0 Compare

December 6, 2022 01:19

Collaborator

ranj063 commented Dec 6, 2022

@bardliao conflicts?

bardliao added 3 commits

December 7, 2022 14:37


          topology2: cavs-sdw: group route and pipeline index

f554a2f

A topology may be constructed by some .conf files. We may use a
duplicated route index or pipeline index by accident. This commit
suggests a rule to assign route and pipeline index.
We have a consistent pcm id. For example,
Jack out: 0
Jack in: 1
Speaker: 2
Microphone: 4

We can use a simple formula to assign the route and pipeline index
for each pcm.
The formula this commit suppests is pcm id * 10 ~ pcm id * 10 + 9.
That is 0 ~ 9 for pcm 0, 10 ~ 19 for pcm 1, and so on.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>


          topology2: cavs-sdw: use macro to define SDW jack stream and id

6d451b2

Define macro for SoundWire jack stream name and BE id.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>


          topology2: cavs-sdw: add Speaker and Microphone pcm

c196047

Add Speaker PCM to cavs-sdw topology. We need to set NUM_SDW_AMPS=1 in
CMakeLists.txt if there is only one amplifier in the device and set
NUM_SDW_AMPS=2 if there are two amplifiers.
Set SDW_DMIC=1 if there is a SDW DMIC, like rt714 in the device.
The 2nd ALH copier stream name should be the same as the 1st ALH
copier, and no need to connect to the route.
The 2nd ALH copier data will be set to the 1st ALH copier's gateway config
in the aggregated mode.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the topology2-sdw-spk branch from 8238fc0 to c196047 Compare

December 7, 2022 06:46

Collaborator Author

bardliao commented Dec 7, 2022

@bardliao conflicts?

Fixed.

ranj063 approved these changes

View reviewed changes

lgirdwood approved these changes

View reviewed changes

Member

lgirdwood commented Dec 7, 2022

@plbossart good for you ?

bardliao requested a review from plbossart

December 8, 2022 02:37

RanderWang approved these changes

View reviewed changes

plbossart approved these changes

View reviewed changes

Member

plbossart left a comment

blindly approving, all the index management needs to be cleaned-up.

lgirdwood merged commit 113064c into thesofproject:main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

plbossart plbossart approved these changes

ranj063 ranj063 approved these changes

lgirdwood lgirdwood approved these changes

RanderWang RanderWang approved these changes

marc-hb Awaiting requested review from marc-hb

aborisovich Awaiting requested review from aborisovich

mmaka1 Awaiting requested review from mmaka1

lbetlej Awaiting requested review from lbetlej

dbaluta Awaiting requested review from dbaluta

Labels

None yet