ASoC: SOF: topology: fix cpu_dai_name for links with multi cpu DAIs by ranj063 · Pull Request #2832 · thesofproject/linux

ranj063 · 2021-04-08T23:25:17Z

Use the correct cpu_dai_name when a link has multiple cpu
DAIs.

RanderWang · 2021-04-09T02:15:13Z

+			if (link->num_cpus == 1)
+				dai->cpu_dai_name = link->cpus->dai_name;
+			else
+				dai->cpu_dai_name = link->cpus[j++].dai_name;


only "dai->cpu_dai_name = link->cpus[j++].dai_name;" can work ?

ha thats what I had but that will increment j for HDA DAIs and it wont work.

Should we test if (j < link->num_cpus)?

so does this mean that if .num_cpus > 1 then the number of DAI widgets must not exceed it, but if .num_cpus == 1 then we can have an arbitrary number of DAI widgets?

keqiaozhang · 2021-04-09T02:53:47Z

Confirmed that this PR can fix issue #2821.

keqiaozhang · 2021-04-09T05:52:14Z

Found a new issue after applying this PR. Please refer to:#2821 (comment)

lyakh · 2021-04-09T06:09:09Z

-			dai->cpu_dai_name = link->cpus->dai_name;
+			if (j >= link->num_cpus) {
+				dev_err(sdev->dev,
+					"error: DAI widget count exceeds link cpus count\n");


can we capitalise "CPUs" too, in fact wouldn't "link CPU count" (without the "s") be more correct - like in the first part "DAI widget count" - not "DAI widgets count?"

lyakh · 2021-04-09T06:15:31Z

+			if (link->num_cpus == 1)
+				dai->cpu_dai_name = link->cpus->dai_name;
+			else
+				dai->cpu_dai_name = link->cpus[j++].dai_name;


so does this mean that if .num_cpus > 1 then the number of DAI widgets must not exceed it, but if .num_cpus == 1 then we can have an arbitrary number of DAI widgets?

lyakh

just some optional comments

lyakh · 2021-04-12T07:07:36Z

+		if (ret < 0) {
+			dev_err(sdev->dev, "error: setting up DAI widget %s\n", w->name);
+			return ret;
+		}


the return value was ignored before this patch? So, it's a separate fix?

It's a fixup to the patch that added the function so I'd say ok to bake into this one.

yes, it willbe squashed anyway.

kv2019i

Looks good to me and easier to follow. These big fixups will be challenging to be send, but in this case should be ok as Ranjani you can do the cleanup (if needed) to the "fixed-up" commit messages. In general, might be a good practise to have an updated commit message to be part of the fixup commit (or a comment stating that no change is needed).

I did thumb-up one Guennadi's comment, but as that's minor enough, I'll go ahead and approve.

kv2019i · 2021-04-12T09:47:10Z

+		if (ret < 0) {
+			dev_err(sdev->dev, "error: setting up DAI widget %s\n", w->name);
+			return ret;
+		}


It's a fixup to the patch that added the function so I'd say ok to bake into this one.

kv2019i · 2021-04-12T09:48:34Z

-	struct sof_ipc_dai_config *config = dai->dai_config;
 	int ret;

+	config = &dai->dai_config[dai->current_config];


Yikes, good catch. FYI @juimonen

this should be a formal Fixes: at this point, the original commit was merged by Mark Brown if I am mistaken?

hmm thats right. I'll leave this one here and submit a separate PR to fix the upstream code.

Not following @ranj063. The fixup! mechanism is only useful if the base commit is still in topic/sof-dev-rebase. we will squash the result before sending upstream
If the ship has sailed and the base commit to be fixed is already in Mark's tree, then we can't use fixup! but only a bona fide patch with a Fixes tag.

plbossart

minor nits on 'Fix' and fixup!

plbossart · 2021-04-12T16:39:47Z

-	struct sof_ipc_dai_config *config = dai->dai_config;
 	int ret;

+	config = &dai->dai_config[dai->current_config];


this should be a formal Fixes: at this point, the original commit was merged by Mark Brown if I am mistaken?

plbossart · 2021-04-12T16:40:32Z

 				 */
 				if (!cpu_dai->playback_widget) {
 					cpu_dai->playback_widget = w;
+					dai->cpu_dai_name = cpu_dai->name;


do we need a Fixes tag as well here?

@plbossart I want to squash the first 2 commits and remove cpu_dai_name from struct snd_sof_dai altogether. With the new logic to use the playback/capture_widget assocuated with the CPU DAI, we dont really need this at all. What do you think? But then its not really a fx anymore but rather a change in logic.

Sounds good to me to remove something we don't need :-)

Simplify the widget lookup logic by simply using the playback/capture widget associated with the CPU DAI. With this change in logic the cpu_dai_name field in struct snd_sof_dai is no longer needed. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

…ai config set up Use the current config instead of the first config. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

lyakh · 2021-04-13T07:00:26Z

approval is subject to clarifying where and how the second patch should go, presumably it will be dropped from this PR.

plbossart

looks good to me, with the only open the point made by Rander

RanderWang

LGTM

ranj063 requested review from bardliao, juimonen and singalsu as code owners April 8, 2021 23:25

ranj063 mentioned this pull request Apr 8, 2021

SoundWire: add mockup codec support #2827

Merged

ranj063 requested a review from plbossart April 8, 2021 23:27

ranj063 force-pushed the fix/multi-cpu-dai branch from 6ad33e3 to 965fff5 Compare April 9, 2021 00:16

RanderWang reviewed Apr 9, 2021

View reviewed changes

ranj063 force-pushed the fix/multi-cpu-dai branch from 965fff5 to 51658a5 Compare April 9, 2021 03:23

bardliao previously approved these changes Apr 9, 2021

View reviewed changes

lyakh requested changes Apr 9, 2021

View reviewed changes

ranj063 dismissed bardliao’s stale review via 1ce58c6 April 9, 2021 21:38

ranj063 force-pushed the fix/multi-cpu-dai branch from 51658a5 to 1ce58c6 Compare April 9, 2021 21:38

ranj063 requested review from keyonjie and libinyang as code owners April 9, 2021 21:38

ranj063 mentioned this pull request Apr 9, 2021

[BUG][TGL][SDW] playback failed on speaker: failed to find SOF DAI for: SDW2 Pin2 #2821

Closed

ranj063 requested review from RanderWang, bardliao and lyakh April 9, 2021 21:40

ranj063 force-pushed the fix/multi-cpu-dai branch 2 times, most recently from 7bf5e3b to 31732eb Compare April 12, 2021 00:18

ranj063 requested review from dbaluta, kv2019i and lgirdwood as code owners April 12, 2021 00:18

ranj063 force-pushed the fix/multi-cpu-dai branch from 31732eb to f46c91d Compare April 12, 2021 01:04

lyakh reviewed Apr 12, 2021

View reviewed changes

kv2019i previously approved these changes Apr 12, 2021

View reviewed changes

bardliao reviewed Apr 12, 2021

View reviewed changes

Comment thread sound/soc/sof/intel/hda.c Resolved

RanderWang reviewed Apr 12, 2021

View reviewed changes

Comment thread sound/soc/sof/intel/hda-dai.c Resolved

ranj063 dismissed kv2019i’s stale review via 26b9e62 April 12, 2021 15:59

ranj063 force-pushed the fix/multi-cpu-dai branch from f46c91d to 26b9e62 Compare April 12, 2021 15:59

ranj063 requested review from RanderWang, bardliao, kv2019i and lyakh April 12, 2021 16:08

plbossart requested changes Apr 12, 2021

View reviewed changes

ranj063 added 2 commits April 12, 2021 09:58

fixup! ASoC: SOF: sof-audio: add helpers for widgets, kcontrols and d…

2f1fbf7

…ai config set up Use the current config instead of the first config. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/multi-cpu-dai branch from 26b9e62 to 2f1fbf7 Compare April 12, 2021 17:03

RanderWang reviewed Apr 13, 2021

View reviewed changes

Comment thread sound/soc/sof/intel/hda-dai.c Resolved

lyakh approved these changes Apr 13, 2021

View reviewed changes

plbossart approved these changes Apr 13, 2021

View reviewed changes

RanderWang approved these changes Apr 14, 2021

View reviewed changes

plbossart merged commit f8e3b2b into thesofproject:topic/sof-dev Apr 14, 2021

ranj063 deleted the fix/multi-cpu-dai branch May 12, 2021 15:54

Conversation

ranj063 commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

keqiaozhang commented Apr 9, 2021

Uh oh!

keqiaozhang commented Apr 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

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

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!

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!

lyakh commented Apr 13, 2021

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ranj063 commented Apr 8, 2021 •

edited

Loading