Skip to content

ASoC: SOF: Intel: don't check number of sdw links when set dmic_fixup #5287

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
bardliao merged 2 commits into from
Feb 6, 2025
Merged

ASoC: SOF: Intel: don't check number of sdw links when set dmic_fixup #5287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27b9cfb
ASoC: SOF: Intel: don't check number of sdw links when set dmic_fixup
bardliao Jan 9, 2025
9473f72
ASoC: Intel: sof_sdw: warn both sdw and pch dmic are used
bardliao Jan 21, 2025
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
7 changes: 7 additions & 0 deletions sound/soc/intel/boards/sof_sdw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -822,7 +822,9 @@ static int create_sdw_dailink(struct snd_soc_card *card,
int *be_id, struct snd_soc_codec_conf **codec_conf)
{
struct device *dev = card->dev;
struct snd_soc_acpi_mach *mach = dev_get_platdata(card->dev);
struct asoc_sdw_mc_private *ctx = snd_soc_card_get_drvdata(card);
struct snd_soc_acpi_mach_params *mach_params = &mach->mach_params;
struct intel_mc_ctx *intel_ctx = (struct intel_mc_ctx *)ctx->private;
struct asoc_sdw_endpoint *sof_end;
int stream;
Expand Down Expand Up @@ -919,6 +921,11 @@ static int create_sdw_dailink(struct snd_soc_card *card,

codecs[j].name = sof_end->codec_name;
codecs[j].dai_name = sof_end->dai_info->dai_name;
if (sof_end->dai_info->dai_type == SOC_SDW_DAI_TYPE_MIC &&
mach_params->dmic_num > 0) {
dev_warn(dev,
"Both SDW DMIC and PCH DMIC are present, if incorrect, please set kernel params snd_sof_intel_hda_generic dmic_num=0 to disable PCH DMIC\n");
}
j++;
}

Expand Down
18 changes: 2 additions & 16 deletions sound/soc/sof/intel/hda.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1321,22 +1321,8 @@ struct snd_soc_acpi_mach *hda_machine_select(struct snd_sof_dev *sdev)
/* report to machine driver if any DMICs are found */
mach->mach_params.dmic_num = check_dmic_num(sdev);

if (sdw_mach_found) {
/*
* DMICs use up to 4 pins and are typically pin-muxed with SoundWire
* 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
Copy link
Member

@plbossart plbossart Jan 22, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@bardliao bardliao Jan 23, 2025

Choose a reason for hiding this comment

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

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

*/
if (hweight_long(mach->link_mask) <= 2)
dmic_fixup = true;
else
mach->mach_params.dmic_num = 0;
} else {
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)
Copy link
Collaborator

@ranj063 ranj063 Jan 16, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@ranj063 ranj063 Jan 16, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@bardliao bardliao Jan 17, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@ranj063 ranj063 Jan 17, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@bardliao bardliao Jan 17, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@ujfalusi ujfalusi Jan 20, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@bardliao bardliao Jan 20, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

@lgirdwood lgirdwood Jan 20, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@bardliao bardliao Jan 21, 2025

Choose a reason for hiding this comment

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

Log added

Copy link
Collaborator

@ranj063 ranj063 Jan 21, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@bardliao bardliao Jan 22, 2025

Choose a reason for hiding this comment

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

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

dmic_fixup = true;

if (tplg_fixup &&
dmic_fixup &&
Expand Down
Loading