ASoC: intel: sof_sdw: Use a fixed DAI link id for AMP #3203

bardliao · 2021-10-08T07:19:14Z

Currently, we assign SoundWire DAI link id according to the order in
snd_soc_acpi_link_adr. It will varies between devices and we have to
modify topologies for the different DAI link ids. Fix a fixed DAI link id
can reduce the possibility of editing topologies.
This patch has the assumption that the codec order in snd_soc_acpi_link_adr
is always headset -> amp -> mic. We have to follow the order when we
add new link adrs in the future.

libinyang · 2021-10-08T08:14:02Z

thesofproject/sof#4824 is using the dynamic BE id now. Please see thesofproject/sof@6329628

So either this PR or thesofproject/sof@6329628 should be merged.

plbossart · 2021-10-08T14:14:31Z

sound/soc/intel/boards/sof_sdw.c

@@ -895,6 +912,9 @@ static int create_sdw_dailink(struct snd_soc_card *card,
 	if (codec_info_list[codec_index].ignore_pch_dmic)
 		*ignore_pch_dmic = true;

+	if (is_amp(codec_info_list[codec_index].part_id) && link_id < SDW_AMP_DAI_ID)


it would be simpler to add a field in codec_info_list, so that we have all the information in a single table.

we could even unify the rt715 handling with different flags for smart amp and smart mic.

it would be simpler to add a field in codec_info_list, so that we have all the information in a single table.

we could even unify the rt715 handling with different flags for smart amp and smart mic.

Done

plbossart · 2021-10-08T15:18:59Z

thesofproject/sof#4824 is using the dynamic BE id now. Please see thesofproject/sof@6329628

So either this PR or thesofproject/sof@6329628 should be merged.

I don't think it makes sense to change the ID in the topology.

One main reason: it would prevent us from testing the 'noajck' option on platforms that do have a jack. The rule is that the machine driver can expose as many links as possible, and the topology selects from the links. If we change the topology to match the machine driver we are adding an exception to the rule and creating more problems.

It's really simpler if we assume that the machine driver has a pre-defined link numbering.

sound/soc/intel/boards/sof_sdw.c

RanderWang

Can you add some logic to check the sequence of "headphone - AMP - MIC". A error message is helpful to users

bardliao · 2021-10-12T08:47:39Z

Can you add some logic to check the sequence of "headphone - AMP - MIC". A error message is helpful to users

Yes, added.

plbossart

sorry @bardliao my previous comments seem to have be parked, re-adding them.

plbossart · 2021-10-12T13:57:15Z

sound/soc/intel/boards/sof_sdw.c

@@ -845,6 +860,7 @@ static int create_sdw_dailink(struct snd_soc_card *card,
 	struct snd_soc_dai_link_component *codecs;
 	int cpu_dai_id[SDW_MAX_CPU_DAIS];
 	int cpu_dai_num, cpu_dai_index;
+	static int link_id = 0;


can we try to avoid using a static here?

can we try to avoid using a static here?

I used static is because create_sdw_dailink() will be called multiple time and I want to keep the value of link_id. But I don't like static inside a function either. Using link_id as an argument of create_sdw_dailink() now.

plbossart · 2021-10-12T13:58:52Z

sound/soc/intel/boards/sof_sdw.c

+	 * keep sdw DMIC and HDMI setting static in UCM
+	 */
+	if (codec_info_list[codec_index].codec_type == SOF_SDW_CODEC_TYPE_MIC &&
+	    link_id < SDW_DMIC_DAI_ID && (sof_sdw_quirk & SOF_RT715_DAI_ID_FIX))


should we remove this quirk now and use only the codec_type?

plbossart · 2021-10-12T14:03:13Z

sound/soc/intel/boards/sof_sdw.c

@@ -920,7 +907,7 @@ static int create_sdw_dailink(struct snd_soc_card *card,
 	 * keep sdw DMIC and HDMI setting static in UCM
 	 */
 	if (codec_info_list[codec_index].codec_type == SOF_SDW_CODEC_TYPE_MIC &&
-	    link_id < SDW_DMIC_DAI_ID && (sof_sdw_quirk & SOF_RT715_DAI_ID_FIX))
+	    link_id < SDW_DMIC_DAI_ID)


I would do this directly in the previous patch, and here only cleanup the quirks

I would do this directly in the previous patch, and here only cleanup the quirks

Done

sound/soc/intel/boards/sof_sdw.c

RanderWang

I am in bad status and can't figure out link_id, link_index now. Can you test on all sdw platforms to confirm the patch ? thanks.

bardliao · 2021-10-14T09:45:47Z

I am in bad status and can't figure out link_id, link_index now.

link_index is the index of dai_links, link_id is the be_id argument of init_dai_link() and it will be dai_links->id. BTW, IMHO, it will be more intuitive if we rename be_id to link_id or just id in init_dai_link().

Can you test on all sdw platforms to confirm the patch ? thanks.

I think CI will do it for me. I will check the result once it is finished.

bardliao · 2021-10-14T10:28:02Z

Can you test on all sdw platforms to confirm the patch ? thanks.

I think CI will do it for me. I will check the result once it is finished.

The only fail on sdw device is https://sof-ci.01.org/linuxpr/PR3203/build6565/devicetest/?model=ADLP_RVP_SDW&testcase=verify-kernel-boot-log. But it seems not be an audio issue, and I checked the dmesg

[    7.301601] kernel: sof_sdw sof_sdw: create dai link SDW0-Playback, id 0
[    7.301789] kernel: sof_sdw sof_sdw: create dai link SDW0-Capture, id 1
[    7.301801] kernel: sof_sdw sof_sdw: create dai link dmic01, id 2
[    7.301813] kernel: sof_sdw sof_sdw: create dai link dmic16k, id 3
[    7.301842] kernel: sof_sdw sof_sdw: create dai link iDisp1, id 4
[    7.301866] kernel: sof_sdw sof_sdw: create dai link iDisp2, id 5
[    7.301889] kernel: sof_sdw sof_sdw: create dai link iDisp3, id 6
[    7.301913] kernel: sof_sdw sof_sdw: create dai link iDisp4, id 7
[    7.301936] kernel: sof_sdw sof_sdw: create dai link SSP2-BT, id 8

It is exactly the same as the PASS case on another PR's pr-device-test result. Besides, all other test cases are PASS.
So I think my PR should be fine.

plbossart

minor nit-pick and question below.

I think we should also allow the user to specify a quirk, as we did with bytcr_rt5640, that would allow us to solve user problems without waiting for a patched kernel to be available.

plbossart · 2021-10-14T12:21:45Z

sound/soc/intel/boards/sof_sdw.c

 	/* get BE ID for non-sdw DAI */
-	be_id = get_next_be_id(links, be_id);
+	be_id = get_next_be_id(links, link_id);


can we rename this variable 'link_index' to be consistent with the rest of the uses where it's a pointer?
same comment for line 1152

sound/soc/intel/boards/sof_sdw.c

bardliao · 2021-10-15T01:41:51Z

minor nit-pick and question below.

I think we should also allow the user to specify a quirk, as we did with bytcr_rt5640, that would allow us to solve user problems without waiting for a patched kernel to be available.

Sorry @plbossart what bytcr_rt5640 quirk you referring to? We do allow user to overwrite the quirk by module param. Or do you think we miss some quick? Allowing user to specific DAI link ids for example?

plbossart · 2021-10-15T11:28:27Z

minor nit-pick and question below.
I think we should also allow the user to specify a quirk, as we did with bytcr_rt5640, that would allow us to solve user problems without waiting for a patched kernel to be available.

Sorry @plbossart what bytcr_rt5640 quirk you referring to? We do allow user to overwrite the quirk by module param. Or do you think we miss some quick? Allowing user to specific DAI link ids for example?

Discard this comment, I didn't recall we already had a quirk module parameter :-)

lyakh · 2021-10-18T07:17:19Z

sound/soc/intel/boards/sof_sdw.c

@@ -1157,7 +1176,7 @@ static int sof_card_dai_links_create(struct device *dev,
 					 sdw_cpu_dai_num, cpus, adr_link,
 					 &cpu_id, group_generated,
 					 codec_conf, codec_conf_count,
-					 &codec_conf_index,
+					 &be_id, &codec_conf_index,
 					 &ignore_pch_dmic);


15 parameters... Can we use a structure? Unrelated, of course...

kv2019i

I think this is a welcome change. I have a few minor comments and questions. Please see inline.

kv2019i · 2021-10-18T10:47:59Z

sound/soc/intel/boards/sof_sdw.c

@@ -404,6 +404,7 @@ static struct sof_sdw_codec_info codec_info_list[] = {
 		.direction = {true, true},


Commit message needs a bit of work:

"It will varies between" -> "It will vary between"

"Fix a fixed DAI link can reduce .. possibility of editing topologies" -> ? not sure what this means

Also while you're at it, can you change the first line to something like "Currently, we assign SoundWire DAI link id according to the order in the link address table or array"

"Fix a fixed DAI link can reduce .. possibility of editing topologies" -> ? not sure what this means

That was a typo of "Use a fixed DAI link..."

Hmm, this still seems wrong: "Use a fixed DAI link id can reduce the possibility of editing topologies."

This seems like a pretty important part of the patchset, why are we moving to a fixed DAI link id. I'm not 100% sure what the you mean here. Would this be correct:
"Use of a fixed DAI link id can reduce the need to edit topologies."

suggested commit message edit:

Currently, we assign SoundWire DAI link id according to the order in
the link address table, with the assumption that the headset codec is listed first, then amplifiers and last capture devices. If the headset codec is not present in a platform, the dai link for amplifiers will be shifted, which can be handled in two ways
a) modify the topology to renumber the dailink changes
b) keep the dailink numbers constant in topology but also avoid the variations in the machine driver.

This patch adds support for option b), the dailink index for amplifiers and capture devices becomes fixed.

@plbossart Commit message changed. Thanks for the suggestion.

sound/soc/intel/boards/sof_sdw.c

ranj063 · 2021-10-18T17:01:27Z

sound/soc/intel/boards/sof_sdw.c

@@ -404,6 +404,7 @@ static struct sof_sdw_codec_info codec_info_list[] = {
 		.direction = {true, true},


Also while you're at it, can you change the first line to something like "Currently, we assign SoundWire DAI link id according to the order in the link address table or array"

sound/soc/intel/boards/sof_sdw.c

The link_id variable in sof_card_dai_links_create() and be_index argument in create_sdw_dailink() is actually links' index. Rename them to link_index to be consistent. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

ranj063

looks good @bardliao

sound/soc/intel/boards/sof_sdw.c

kv2019i

Still one commit message clarification needed.

sound/soc/intel/boards/sof_sdw.c

kv2019i · 2021-10-19T19:24:33Z

sound/soc/intel/boards/sof_sdw.c

@@ -404,6 +404,7 @@ static struct sof_sdw_codec_info codec_info_list[] = {
 		.direction = {true, true},


Hmm, this still seems wrong: "Use a fixed DAI link id can reduce the possibility of editing topologies."

This seems like a pretty important part of the patchset, why are we moving to a fixed DAI link id. I'm not 100% sure what the you mean here. Would this be correct:
"Use of a fixed DAI link id can reduce the need to edit topologies."

Currently, we assign SoundWire DAI link id according to the order in the link address table, with the assumption that the headset codec is listed first, then amplifiers and last capture devices. If the headset codec is not present in a platform, the dai link for amplifiers will be shifted, which can be handled in two ways a) modify the topology to renumber the dailink changes b) keep the dailink numbers constant in topology but also avoid the variations in the machine driver. This patch adds support for option b), the dailink index for amplifiers and capture devices becomes fixed. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

We can apply the fixed dai link id for DMICs in create_sdw_dailink(). No need to set it in each DMIC's callback. The fixed dai link id is not only for rt715 and rt715-sdca, but for all DMICs, therefore we remove the SOF_RT715_DAI_ID_FIX check as well. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

SOF_RT715_DAI_ID_FIX is not used anywhere. Remove it. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Now, we set DAI link id as SDW_DMIC_DAI_ID for all DMICs. No need to set it in sof_sdw_mic_codec_mockup_init. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

DAI link id will not be set from sdw codec init feedback function, and be_id is changed by create_sdw_dailink() now. So we don't need get_next_be_id() anymore. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

We assume the adr order described in a snd_soc_acpi_link_adr array is jack -> amp -> mic. We follow the same order to implement the topology. We will need a special topology if we configure a snd_soc_acpi_link_adr array with different order. Adding a check and a warning message can remind people to keep the order when adding a new snd_soc_acpi_link_adr array. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao requested review from plbossart and RanderWang October 8, 2021 07:19

bardliao requested review from dbaluta, kv2019i, lgirdwood and ranj063 as code owners October 8, 2021 07:19

plbossart requested changes Oct 8, 2021

View reviewed changes

libinyang reviewed Oct 9, 2021

View reviewed changes

sound/soc/intel/boards/sof_sdw.c Outdated Show resolved Hide resolved

bardliao force-pushed the fixed-amp-id branch 2 times, most recently from 50ba7dd to 5dba5be Compare October 12, 2021 02:39

RanderWang reviewed Oct 12, 2021

View reviewed changes

bardliao changed the title ~~[RFC] ASoC: intel: sof_sdw: Use a fixed DAI link id for AMP~~ ASoC: intel: sof_sdw: Use a fixed DAI link id for AMP Oct 13, 2021

bardliao requested a review from plbossart October 13, 2021 08:13

plbossart requested changes Oct 13, 2021

View reviewed changes

bardliao force-pushed the fixed-amp-id branch from 1eef822 to 93654fa Compare October 14, 2021 05:25

RanderWang reviewed Oct 14, 2021

View reviewed changes

sound/soc/intel/boards/sof_sdw.c Show resolved Hide resolved

bardliao force-pushed the fixed-amp-id branch from 93654fa to 96e109e Compare October 14, 2021 08:28

RanderWang reviewed Oct 14, 2021

View reviewed changes

bardliao force-pushed the fixed-amp-id branch from 96e109e to cf6c2c2 Compare October 14, 2021 09:31

plbossart requested changes Oct 14, 2021

View reviewed changes

bardliao force-pushed the fixed-amp-id branch from cf6c2c2 to 015aa50 Compare October 15, 2021 01:01

bardliao force-pushed the fixed-amp-id branch from 015aa50 to 1c45196 Compare October 15, 2021 01:47

plbossart previously approved these changes Oct 15, 2021

View reviewed changes

plbossart requested review from RanderWang and libinyang October 15, 2021 11:30

lyakh reviewed Oct 18, 2021

View reviewed changes

kv2019i requested changes Oct 18, 2021

View reviewed changes

ranj063 reviewed Oct 18, 2021

View reviewed changes

bardliao dismissed plbossart’s stale review via cdcb468 October 19, 2021 02:38

bardliao force-pushed the fixed-amp-id branch from 1c45196 to cdcb468 Compare October 19, 2021 02:38

RanderWang reviewed Oct 19, 2021

View reviewed changes

sound/soc/intel/boards/sof_sdw.c Outdated Show resolved Hide resolved

ASoC: intel: sof_sdw: rename be_index/link_id to link_index

8b9bfa5

The link_id variable in sof_card_dai_links_create() and be_index argument in create_sdw_dailink() is actually links' index. Rename them to link_index to be consistent. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the fixed-amp-id branch from cdcb468 to 269fc52 Compare October 19, 2021 09:38

ranj063 previously approved these changes Oct 19, 2021

View reviewed changes

sound/soc/intel/boards/sof_sdw.c Show resolved Hide resolved

kv2019i requested changes Oct 19, 2021

View reviewed changes

bardliao dismissed ranj063’s stale review via 673928f October 20, 2021 01:51

bardliao force-pushed the fixed-amp-id branch from 269fc52 to 673928f Compare October 20, 2021 01:51

bardliao added 6 commits October 20, 2021 10:01

ASoC: intel: sof_sdw: remove SOF_RT715_DAI_ID_FIX quirk

90945e0

SOF_RT715_DAI_ID_FIX is not used anywhere. Remove it. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

ASoC: intel: sof_sdw: remove sof_sdw_mic_codec_mockup_init

55a6014

Now, we set DAI link id as SDW_DMIC_DAI_ID for all DMICs. No need to set it in sof_sdw_mic_codec_mockup_init. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

ASoC: intel: sof_sdw: remove get_next_be_id

27bc829

DAI link id will not be set from sdw codec init feedback function, and be_id is changed by create_sdw_dailink() now. So we don't need get_next_be_id() anymore. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the fixed-amp-id branch from 673928f to 9a1c053 Compare October 20, 2021 02:02

kv2019i approved these changes Oct 20, 2021

View reviewed changes

ranj063 approved these changes Oct 20, 2021

View reviewed changes

plbossart approved these changes Oct 20, 2021

View reviewed changes

plbossart merged commit 1fdbaea into thesofproject:topic/sof-dev Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASoC: intel: sof_sdw: Use a fixed DAI link id for AMP #3203

ASoC: intel: sof_sdw: Use a fixed DAI link id for AMP #3203

bardliao commented Oct 8, 2021

libinyang commented Oct 8, 2021 •

edited

Loading

plbossart Oct 8, 2021

bardliao Oct 12, 2021

plbossart commented Oct 8, 2021

RanderWang left a comment

bardliao commented Oct 12, 2021

plbossart left a comment

plbossart Oct 12, 2021

bardliao Oct 14, 2021

plbossart Oct 12, 2021

plbossart Oct 12, 2021

bardliao Oct 14, 2021

RanderWang left a comment

bardliao commented Oct 14, 2021

bardliao commented Oct 14, 2021

plbossart left a comment

plbossart Oct 14, 2021

bardliao commented Oct 15, 2021

plbossart commented Oct 15, 2021

lyakh Oct 18, 2021

kv2019i left a comment

kv2019i Oct 18, 2021

ranj063 Oct 18, 2021

bardliao Oct 19, 2021

kv2019i Oct 19, 2021

plbossart Oct 19, 2021 •

edited

Loading

bardliao Oct 20, 2021

ranj063 Oct 18, 2021

ranj063 left a comment

kv2019i left a comment

kv2019i Oct 19, 2021

		@@ -404,6 +404,7 @@ static struct sof_sdw_codec_info codec_info_list[] = {
		.direction = {true, true},

ASoC: intel: sof_sdw: Use a fixed DAI link id for AMP #3203

ASoC: intel: sof_sdw: Use a fixed DAI link id for AMP #3203

Conversation

bardliao commented Oct 8, 2021

libinyang commented Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart commented Oct 8, 2021

RanderWang left a comment

Choose a reason for hiding this comment

bardliao commented Oct 12, 2021

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang left a comment

Choose a reason for hiding this comment

bardliao commented Oct 14, 2021

bardliao commented Oct 14, 2021

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bardliao commented Oct 15, 2021

plbossart commented Oct 15, 2021

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 left a comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libinyang commented Oct 8, 2021 •

edited

Loading

plbossart Oct 19, 2021 •

edited

Loading