Skip to content

ASoC: SOF: topology: fix: handle DAI widget connections properly with multiple CPU DAI's #1863

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

Merged
ranj063 merged 1 commit into from
Mar 17, 2020
Merged

ASoC: SOF: topology: fix: handle DAI widget connections properly with multiple CPU DAI's #1863

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1256,15 +1256,43 @@ static int sof_connect_dai_widget(struct snd_soc_component *scomp,

switch (w->id) {
case snd_soc_dapm_dai_out:
for_each_rtd_cpu_dais(rtd, i, cpu_dai)
cpu_dai->capture_widget = w;
for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
/*
* Please create DAI widget in the right order
* to ensure BE will connect to the right DAI
* widget.
*/
Copy link
Collaborator

@lyakh lyakh Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I might be the only one who doesn't understand this, but the correct order should be between what and what? I understand that certain two sequences should be defined in a matching order, right? Are both of them in topology files? Are they created automatically by macros from the same code or are these two sequences actually coded by humans? Or is this one sequence in topology and one in a machine driver? If both are created automatically from the same source, then maybe it's ok to rely on this. If not, maybe we need a way to match?

Copy link
Collaborator Author

@bardliao bardliao Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I might be the only one who doesn't understand this, but the correct order should be between what and what? I understand that certain two sequences should be defined in a matching order, right? Are both of them in topology files? Are they created automatically by macros from the same code or are these two sequences actually coded by humans? Or is this one sequence in topology and one in a machine driver? If both are created automatically from the same source, then maybe it's ok to rely on this. If not, maybe we need a way to match?

@lyakh The correct order is we set it in the same order between the CPU dais defined in a dai link and the DAI widget defined in topology. e,g,

SND_SOC_DAILINK_DEFsdw1_codec,
	DAILINK_COMP_ARRAY(
		COMP_CODEC("sdw:1:25d:1308:0", "rt1308-aif1"),
		COMP_CODEC("sdw:2:25d:1308:0", "rt1308-aif1")));
in machine driver.
and
DAI_ADD(sof/pipe-dai-playback.m4,
	3, ALH, 0x102, SDW1-Playback,
	PIPELINE_SOURCE_3, 2, s24le,
	1000, 0, 0, SCHEDULE_TIME_DOMAIN_TIMER)
DAI_ADD(sof/pipe-dai-playback.m4,
	4, ALH, 0x202, SDW2-Playback,
	PIPELINE_SOURCE_4, 2, s24le,
	1000, 0, 0, SCHEDULE_TIME_DOMAIN_TIMER)
in topology m4 file.

Where we want to connect ALH0x102 to sdw:1:25d:1308:0 rt1308-aif1 and ALH0x202 to sdw:2:25d:1308:0 rt1308-aif1. So the pipe id (3 and 4) should be in the same order as COMP_CODEC("sdw:1:25d:1308:0", "rt1308-aif1") and COMP_CODEC("sdw:2:25d:1308:0", "rt1308-aif1"). Unfortunately, they are one in machine driver and the other in topology and both of them are created by human.

Copy link
Collaborator

@lyakh lyakh Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I might be the only one who doesn't understand this, but the correct order should be between what and what? I understand that certain two sequences should be defined in a matching order, right? Are both of them in topology files? Are they created automatically by macros from the same code or are these two sequences actually coded by humans? Or is this one sequence in topology and one in a machine driver? If both are created automatically from the same source, then maybe it's ok to rely on this. If not, maybe we need a way to match?

@lyakh [snip] Unfortunately, they are one in machine driver and the other in topology and both of them are created by human.

@bardliao ok, thanks for the explanation. So, as you said, both files are man-made so mistakes would be possible. It would be good to be able to verify matching here, but I don't know how difficult it would be to add nor whether this is actually a usual situation and we have more of them around, in which case this might be considered acceptable.

Copy link

@RanderWang RanderWang Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh we have debug message here to show the match info, so it is easy to find mismatch. And the algorithm in machine driver is fixed, we only need to design topology based on the machine driver. Now we can only seek some indirect info from the cpu dai name and widget name to match them, but this still depends on how we set name in topology and machine driver.

Copy link
Collaborator Author

@bardliao bardliao Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possible solution is to use a new token to identify the index. But it might need ipc structure change. So I would prefer leave it as it is.

Copy link
Collaborator

@lyakh lyakh Mar 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's similar to what just recently happened to apl-pcm512x: we replaced the kernel driver with a new one, that changed the order of the HDMIs, then we had to adjust the topology. Now they don't mix - you need either both new or both old, but new + old or old + new doesn't work...

if (!cpu_dai->capture_widget)
break;
}
if (i == rtd->num_cpus) {
dev_err(scomp->dev, "error: can't find BE for DAI %s\n",
w->name);

return -EINVAL;
}
cpu_dai->capture_widget = w;
dai->name = rtd->dai_link->name;
dev_dbg(scomp->dev, "tplg: connected widget %s -> DAI link %s\n",
w->name, rtd->dai_link->name);
break;
case snd_soc_dapm_dai_in:
for_each_rtd_cpu_dais(rtd, i, cpu_dai)
cpu_dai->playback_widget = w;
for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
/*
* Please create DAI widget in the right order
* to ensure BE will connect to the right DAI
* widget.
*/
if (!cpu_dai->playback_widget)
break;
}
if (i == rtd->num_cpus) {
dev_err(scomp->dev, "error: can't find BE for DAI %s\n",
w->name);

return -EINVAL;
}
cpu_dai->playback_widget = w;
dai->name = rtd->dai_link->name;
dev_dbg(scomp->dev, "tplg: connected widget %s -> DAI link %s\n",
w->name, rtd->dai_link->name);
Expand Down