ASoC: SOF: Intel: don't check number of sdw links when set dmic_fixup by bardliao · Pull Request #5287 · thesofproject/linux

bardliao · 2025-01-09T05:22:37Z

Currently, we assume that the PCH DMIC pins are pin-muxed with SoundWire links. However, we do see a HW design that use PCH DMIC along with 3 SoundWire links. Remove the check now.
With this change BIOS should only report the presence of PCH DMI when it is truly exist.

bardliao · 2025-01-16T08:44:53Z

@ranj063 Do you see any risk that we merge this PR?

ranj063 · 2025-01-16T16:47:29Z

sound/soc/sof/intel/hda.c

-			if (mach->tplg_quirk_mask & SND_SOC_ACPI_TPLG_INTEL_DMIC_NUMBER)
-				dmic_fixup = true;
-		}
+		if (sdw_mach_found || mach->tplg_quirk_mask & SND_SOC_ACPI_TPLG_INTEL_DMIC_NUMBER)


@bardliao can we please fix the typo in commit message "PCH DMI". Also, the last line somehow reads as though the BIOS is going to report differently because of this change. But rather what you're trying to say is that we check for PCM DMIC reported by the BIOS irrespective of whether there are SDW links present or not.

and also in the case where the SDW and DMIC are pin-muxed, will this change have a detrimental effect on machine driver probe because we ended up choosing the wrong topology name?

Yes, if the BIOS report PCH DMIC but there is no PCH DMIC, we will choose the wrong topology name. That's why I am not confident with this change. But on the other hand, that is the BIOS's responsibility to report the real HW configuration and kernel should trust the BIOS.

I think we don't have a choice but to trust the bios @ujfalusi @lgirdwood what do you think?

I vaguely remember we did this for RVPs. Not sure if it happens in real products. The worst case is that someone reports an issue that no sound card is created, and we may need to fix it with some quirk.

@bardliao, you are too optimistic ;) We have seen broken BIOS information and I'm sure we are continue to see them.

But imho we still need to trust the information BIOS provides, it is most likely correct..

@ranj063 @ujfalusi Alternative solution is to add SOC_SDW_PCH_DMIC quirk to this specific device. What do you think?

I would make any assumption obvious in the logs i.e. if we BIOS says we have DMIC & SDW then we can log this with a if this is wrong pls set "force_fix_something=1" in module command line.
This way it should be obvious to a user on how to remedy if BIOS is reporting incorrect data.

lgirdwood · 2025-01-21T12:39:09Z

sound/soc/sof/intel/hda.c

 	dev_info(sdev->dev, "DMICs detected in NHLT tables: %d\n", dmic_num);
+	dev_info(sdev->dev,
+		 "If the DMIC number doesn't match to the real HW design, please overwrite it with \"dmic_num\" kernel param.\n");


I would change this too

dev_info(sdev->dev, "DMICs detected in NHLT tables: %d, can be overidden with \"dmic_num\" kernel param if incorrect\n", dmic_num);

and I would move the new larger warning to the code block when we detect both SDW and DMIC

dev_info(sdev->dev, "NHLT reporting both SDW and DMIC endpoints, if incorrect, pls override with .....);

Update: only show the warning when both SDW mic and PCH DMIC are present.

ranj063 · 2025-01-21T17:19:10Z

sound/soc/sof/intel/hda.c

-			if (mach->tplg_quirk_mask & SND_SOC_ACPI_TPLG_INTEL_DMIC_NUMBER)
-				dmic_fixup = true;
-		}
+		if (sdw_mach_found || mach->tplg_quirk_mask & SND_SOC_ACPI_TPLG_INTEL_DMIC_NUMBER)


@bardliao is the check for sdw_mach_found important? I think we only add the dmic prefix to the tplg name if either the tplg quirk is set or the num_dmics reported is non-zero isnt it?

Yes, sdw_mach_found is important because we don't add -2ch/-4ch postfix for SSP machines.

lgirdwood

LGTM, just need a string update

lgirdwood · 2025-01-22T14:56:05Z

sound/soc/intel/boards/sof_sdw.c

+		for (i = 0; i < num_ends; i++) {
+			if (sof_end->dai_info->dai_type == SOC_SDW_DAI_TYPE_MIC) {
+				dev_warn(dev,
+					 "Both SDW DMIC and PCH DMIC are present, if incorrect, please override with kernel params\n");


which params ? lets be more helpful.

plbossart · 2025-01-22T21:17:31Z

sound/soc/sof/intel/hda.c

-			 * link 2 and 3, or link 1 and 2, thus we only try to enable dmics
-			 * if all conditions are true:
-			 * a) 2 or fewer links are used by SoundWire
-			 * b) the NHLT table reports the presence of microphones


maybe you only need to refine these criteria?

You can have 3 SoundWire links along 1 DMIC link. But I don't think you can have 3 SoundWire links with 2 DMIC links - since in the latter case you need at least 3 wires, 4 if the clock isn't shared.

@plbossart Hmm, the question is that do DMIC links always share with SoundWire links and will the SoundWire link number increase? Is it possible that someday we have 3 SoundWire links and 2 DMIC links?

Currently, we assume that the PCH DMIC pins are pin-muxed with SoundWire links. However, we do see a HW design that use PCH DMIC along with 3 SoundWire links. Remove the check now. With this change the PCM DMIC will be presented if it is reported by the BIOS irrespective of whether there are SDW links present or not. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Typically, SoundWire MIC and PCH DMIC will not coexist. However, we may want to use both of them in some special cases. Add a warning to let users know that SoundWire MIC and PCH DMIC are both present and they could overwrite it with kernel params. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao · 2025-02-05T03:03:27Z

@ujfalusi @ranj063 Are you good with the PR?

sound/soc/intel/boards/sof_sdw.c

bardliao mentioned this pull request Jan 9, 2025

[BUG] ASUS Zenbook S 14: No sound from DMIC #5325

Closed

bardliao requested review from kv2019i, plbossart, ranj063 and ujfalusi January 10, 2025 12:35

ujfalusi previously approved these changes Jan 10, 2025

View reviewed changes

bardliao marked this pull request as ready for review January 14, 2025 07:44

ranj063 reviewed Jan 16, 2025

View reviewed changes

bardliao force-pushed the dmic_fixup branch from 308ae70 to abe0370 Compare January 17, 2025 01:11

bardliao dismissed ujfalusi’s stale review via 1b1f291 January 21, 2025 09:47

bardliao force-pushed the dmic_fixup branch from abe0370 to 1b1f291 Compare January 21, 2025 09:47

lgirdwood reviewed Jan 21, 2025

View reviewed changes

bardliao force-pushed the dmic_fixup branch 2 times, most recently from 26e48f7 to d515cbb Compare January 21, 2025 14:12

bardliao requested a review from dbaluta as a code owner January 21, 2025 14:12

bardliao force-pushed the dmic_fixup branch from d515cbb to 042096d Compare January 21, 2025 14:21

ranj063 reviewed Jan 21, 2025

View reviewed changes

lgirdwood reviewed Jan 22, 2025

View reviewed changes

plbossart reviewed Jan 22, 2025

View reviewed changes

bardliao force-pushed the dmic_fixup branch from 042096d to 046d3bc Compare January 23, 2025 02:45

bardliao added 2 commits February 4, 2025 15:54

bardliao force-pushed the dmic_fixup branch from 046d3bc to 9473f72 Compare February 4, 2025 07:55

ranj063 approved these changes Feb 5, 2025

View reviewed changes

ujfalusi reviewed Feb 6, 2025

View reviewed changes

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

ujfalusi approved these changes Feb 6, 2025

View reviewed changes

bardliao merged commit b8e11f2 into thesofproject:topic/sof-dev Feb 6, 2025
10 of 14 checks passed

Conversation

bardliao commented Jan 9, 2025

Uh oh!

bardliao commented Jan 16, 2025

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!

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!

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

bardliao commented Feb 5, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants