ASoC: SOF: topology: fix: handle DAI widget connections properly with multiple CPU DAI's by bardliao · Pull Request #1863 · thesofproject/linux

bardliao · 2020-03-05T07:04:52Z

Currently, when connecting a DAI widget to the BE CPU DAI, we overwrite
the previous connections. This worked because we only ever had 1 CPU DAI
for each rtd until now. But with multiple CPU DAI's, a new connection
between a BE CPU DAI and the DAI widget should be established without
affecting the previous connections. So, modify the loop to set the
playback/capture widget for the first BE CPU DAI that does have a
connection established previously.

Fixes: 4a7e26a ("ASoC: SOF: topology: connect dai widget to all
cpu-dais")

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

sound/soc/sof/topology.c

plbossart

not fully clear on what this fixes?

plbossart · 2020-03-05T13:19:32Z

sound/soc/sof/topology.c

-			for_each_rtd_cpu_dai(rtd, i, cpu_dai)
-				cpu_dai->capture_widget = w;
+			for_each_rtd_cpu_dai(rtd, i, cpu_dai) {
+				/* find the first empty capture_widget */


probably want to explain that the order does not matter.

sound/soc/sof/topology.c

ranj063

@bardliao the change looks good to me I'd modify the commit message to say something like this:
ASoC: SOF: topology: handle DAI widget connections properly with multiple CPU DAI's
" Currently, when connecting a DAI widget to the BE CPU DAI, we overwrite the previous connections. This worked because we only ever had 1 CPU DAI for each rtd until now. But with multiple CPU DAI's, a new connection between a BE CPU DAI and the DAI widget should be established without affecting the previous connections. So, modify the loop to set the playback/capture widget for the first BE CPU DAI that does have a connection established previously."

bardliao · 2020-03-06T00:45:23Z

@bardliao the change looks good to me I'd modify the commit message to say something like this:
ASoC: SOF: topology: handle DAI widget connections properly with multiple CPU DAI's
" Currently, when connecting a DAI widget to the BE CPU DAI, we overwrite the previous connections. This worked because we only ever had 1 CPU DAI for each rtd until now. But with multiple CPU DAI's, a new connection between a BE CPU DAI and the DAI widget should be established without affecting the previous connections. So, modify the loop to set the playback/capture widget for the first BE CPU DAI that does have a connection established previously."

Thanks @ranj063 That's much clear than mine.

ranj063 · 2020-03-06T02:13:49Z

sound/soc/sof/topology.c

+				 * We expect that DAI widget will define in
+				 * right order by topology, however, it is also
+				 * fine if it doesn't since all DAI widgets
+				 * will be enabled/disabled at the same time


@bardliao better to just say "the order of DAI widgets defined in topology doesnt matter as all the CPU DAI's will be enabled/disabled simultaneously". same thing below

lyakh · 2020-03-06T10:30:22Z

sound/soc/sof/topology.c

+				if (!cpu_dai->capture_widget)
+					break;
+			}
+			if (cpu_dai->capture_widget) {


This isn't safe. If you didn't find a free slot i.e. if you didn't break out of the above loop and it ran until its termination, it means the stop condition of the loop was triggered, which can mean, that cpu_dai == NULL. In general it isn't safe to dereference the iterator after a "list_for_each()" style loop has possibly terminated normally. This isn't a list-traversing loop, but it still doesn't look safe.

lyakh · 2020-03-06T10:30:57Z

sound/soc/sof/topology.c

+				if (!cpu_dai->playback_widget)
+					break;
+			}
+			if (cpu_dai->playback_widget) {


ditto, you need a better detection of loop terminating normally.

kv2019i

Some improvements needed still, see inline

sound/soc/sof/topology.c

kv2019i

Seems better now, waiting for consensus on the couple of inline discussions.

plbossart

note to maintainers: need to remove the fixes tag and squash this with initial version before upstream

lyakh · 2020-03-10T07:40:20Z

sound/soc/sof/topology.c

+				 * Please create DAI widget in the right order
+				 * to ensure BE will connect to the right DAI
+				 * widget.
+				 */


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?

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.

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.

@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.

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.

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...

RanderWang

I understand : we need to design topology according to the behavior of machine driver to match them correctly

kv2019i · 2020-03-11T12:20:24Z

Seems ready for merging @lyakh are you ok with the explanation to the ordering question? You still have this marked as changes-requested...

plbossart · 2020-03-11T15:26:11Z

CI seems completely in the weeds @wenqingfu

lyakh · 2020-03-12T07:46:56Z

Seems ready for merging @lyakh are you ok with the explanation to the ordering question? You still have this marked as changes-requested...

@kv2019i the ordering issue seems to be not new, it isn't added by this patch. IMHO it would be good to remove that ordering dependency and replace it with a reliable matching, but that would be a separate piece of work. So probably that issue shouldn't be a blocker for this patch.

plbossart · 2020-03-12T11:01:16Z

Seems ready for merging @lyakh are you ok with the explanation to the ordering question? You still have this marked as changes-requested...

@kv2019i the ordering issue seems to be not new, it isn't added by this patch. IMHO it would be good to remove that ordering dependency and replace it with a reliable matching, but that would be a separate piece of work. So probably that issue shouldn't be a blocker for this patch.

@lyakh do you mind using the 'approve' button then? Requesting changes means we have to use admin superpowers to merge, which should be an exception.

ranj063 · 2020-03-13T16:47:45Z

@plbossart @kv2019i unfortunately there's a conflict and force pushing new changes will dismiss all approvals. Im wondering is resolving conflicts using the web editor will retain the approvals

plbossart · 2020-03-13T17:46:40Z

@plbossart @kv2019i unfortunately there's a conflict and force pushing new changes will dismiss all approvals. Im wondering is resolving conflicts using the web editor will retain the approvals

I don't think doing a force-push will dismiss approvals? Anyways, it's better if bard does it. I've had nothing but issues with the web editor, let's not go there unless it's a true emergency.

Mark merged the initial version, I missed this one, so we'll have to do it right and add the correct Fixes: tag from his tree.

ranj063 · 2020-03-13T17:49:00Z

@plbossart @kv2019i unfortunately there's a conflict and force pushing new changes will dismiss all approvals. Im wondering is resolving conflicts using the web editor will retain the approvals

I don't think doing a force-push will dismiss approvals? Anyways, it's better if bard does it. I've had nothing but issues with the web editor, let's not go there unless it's a true emergency.

Mark merged the initial version, I missed this one, so we'll have to do it right and add the correct Fixes: tag from his tree.

OK better for @bardliao to do it but it will certainly dismiss all approvals.

… multiple CPU DAI's Currently, when connecting a DAI widget to the BE CPU DAI, we overwrite the previous connections. This worked because we only ever had 1 CPU DAI for each rtd until now. But with multiple CPU DAI's, a new connection between a BE CPU DAI and the DAI widget should be established without affecting the previous connections. So, modify the loop to set the playback/capture widget for the first BE CPU DAI that does not have a connection established previously. Fixes: 4a7e26a ("ASoC: SOF: topology: connect dai widget to all cpu-dais") Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao · 2020-03-16T01:08:31Z

OK better for @bardliao to do it but it will certainly dismiss all approvals.

Done. Tested on CML.

plbossart

LGTM

bardliao requested review from RanderWang and plbossart March 5, 2020 07:04

bardliao requested review from juimonen, ranj063 and singalsu as code owners March 5, 2020 07:04

lyakh reviewed Mar 5, 2020

View reviewed changes

sound/soc/sof/topology.c Show resolved Hide resolved

plbossart requested changes Mar 5, 2020

View reviewed changes

ranj063 reviewed Mar 5, 2020

View reviewed changes

bardliao force-pushed the multi-be-dais branch from d18e911 to 0969605 Compare March 6, 2020 01:41

bardliao changed the title ~~ASoC: SOF: topology: Assign BE widget sequentially~~ ASoC: SOF: topology: fix: handle DAI widget connections properly with multiple CPU DAI's Mar 6, 2020

ranj063 reviewed Mar 6, 2020

View reviewed changes

bardliao force-pushed the multi-be-dais branch from 0969605 to 85118d9 Compare March 6, 2020 05:41

lyakh requested changes Mar 6, 2020

View reviewed changes

kv2019i requested changes Mar 6, 2020

View reviewed changes

sound/soc/sof/topology.c Show resolved Hide resolved

bardliao force-pushed the multi-be-dais branch from 85118d9 to 4abd66c Compare March 9, 2020 05:20

kv2019i reviewed Mar 9, 2020

View reviewed changes

plbossart previously approved these changes Mar 9, 2020

View reviewed changes

kv2019i previously approved these changes Mar 9, 2020

View reviewed changes

plbossart requested a review from lyakh March 9, 2020 20:13

bardliao mentioned this pull request Mar 10, 2020

ASoC: SOF: intel: hda-dai: Remove stream name from dai driver #1874

Merged

lyakh reviewed Mar 10, 2020

View reviewed changes

plbossart requested a review from ranj063 March 10, 2020 20:53

RanderWang previously approved these changes Mar 11, 2020

View reviewed changes

bardliao mentioned this pull request Mar 11, 2020

[BUG] Distortion on the second pipeline connected to demux thesofproject/sof#2529

Closed

lyakh previously approved these changes Mar 12, 2020

View reviewed changes

ranj063 previously approved these changes Mar 13, 2020

View reviewed changes

bardliao dismissed stale reviews from ranj063, lyakh, RanderWang, kv2019i, and plbossart via 0558853 March 16, 2020 01:06

bardliao force-pushed the multi-be-dais branch from 4abd66c to 0558853 Compare March 16, 2020 01:06

ranj063 approved these changes Mar 16, 2020

View reviewed changes

plbossart requested review from RanderWang, kv2019i and lyakh March 17, 2020 14:45

plbossart approved these changes Mar 17, 2020

View reviewed changes

ranj063 merged commit 45f6424 into thesofproject:topic/sof-dev Mar 17, 2020

Conversation

bardliao commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ranj063 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bardliao commented Mar 6, 2020

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!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kv2019i 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!

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!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Mar 11, 2020

Uh oh!

plbossart commented Mar 11, 2020

Uh oh!

lyakh commented Mar 12, 2020

Uh oh!

plbossart commented Mar 12, 2020

Uh oh!

ranj063 commented Mar 13, 2020

Uh oh!

plbossart commented Mar 13, 2020

Uh oh!

ranj063 commented Mar 13, 2020

Uh oh!

bardliao commented Mar 16, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bardliao commented Mar 5, 2020 •

edited

Loading

ranj063 left a comment •

edited

Loading