ASoC: Intel: sof_sdw: correct mach_params->dmic_num #5241

bardliao · 2024-11-13T01:45:47Z

mach_params->dmic_num will be used to set the cfg-mics value of card->components string which should be the dmic channels. And dmic_num is the number of the dmic dai links which is always 2. Only overwrite mach_params->dmic_num when mach_params->dmic_num is 0 and dmic_num is set.

ujfalusi · 2024-11-13T10:17:03Z

sound/soc/intel/boards/sof_sdw.c

 	 */
-	mach_params->dmic_num = dmic_num;
+	mach_params->dmic_num = mach_params->dmic_num ? mach_params->dmic_num : dmic_num;


So the change is this, right?

if (!mach_params->dmic_num) mach_params->dmic_num = dmic_num;

But this is all very confusing and I'm not sure if it is correct.
The mach_params->dmic_num have two different meaning:
A) if SOC_SDW_PCH_DMIC is set, is the number of DMICs connected to PCH
B) if SOC_SDW_PCH_DMIC is not set, is the number of DMICs connected to SDW codec
The dmic_num (local variable) is the number of DMICs connected to PCH

I think the logic should be:

if (!dmic_num) mach_params->dmic_num = dmic_num;

iow, we fix up the number of dmics to 2 (???? why two, if the mach_params->dmic_num was 1 or 4??? - anyways).

or I'm lost an I don't get what mach_params->dmic_num means if SOC_SDW_PCH_DMIC is not set and mach_params->dmic_num is not 0...

Right, reading it up-side-down.
mach_params->dmic_num is the number of PCH connected DMICs. But now I'm more confused.

The local dmic_num represents the number of DAI links not number of mics, perhaps we should also rename it to dmic_dai_num in this patch? And mach_params->dmic_num should apparently be the number of mics, although you do raise a good point how is mach_params->dmic_num set in the case the mics are connected to the codec? Is that missing here as well?

Ah wait yeah, this only covers the host dmic so I think that is fine.

charleskeepax · 2024-11-13T10:36:29Z

sound/soc/intel/boards/sof_sdw.c

 	 */
-	mach_params->dmic_num = dmic_num;
+	mach_params->dmic_num = mach_params->dmic_num ? mach_params->dmic_num : dmic_num;


I think this is still a little broken since it will leave mach_params->dmic_num set, thus adding "mic:dmic cfg-mic:" even in the case ignore_internal_dmic is set, how about this as an alternative:

/* enable dmic01 & dmic16k */ if (ctx->ignore_internal_dmic) { dev_warn(dev, "Ignoring PCH DMIC\n"); mach_params->dmic_num = 0; } else if (sof_sdw_quirk & SOC_SDW_PCH_DMIC) { /* * mach_params->dmic_num will be used to set the cfg-mics value * of card->components string. Overwrite it to the actual number * of PCH DMICs used in the device. */ dmic_num = 2; mach_params->dmic_num = dmic_num; } else if (mach_params->dmic_num) { dmic_num = 2; }

Not totally sure whether the quirk or mach_params->dmic_num should take precedence, but I went with the quirk since I assume it should not be present unless one wants to use it.

I think this is still a little broken since it will leave mach_params->dmic_num set, thus adding "mic:dmic cfg-mic:" even in the case ignore_internal_dmic is set, how about this as an alternative:

Indeed, my commit didn't set mach_params->dmic_num = 0; when ctx->ignore_internal_dmic is true. Thanks for pointing this out.

/* enable dmic01 & dmic16k */ if (ctx->ignore_internal_dmic) { dev_warn(dev, "Ignoring PCH DMIC\n"); mach_params->dmic_num = 0; } else if (sof_sdw_quirk & SOC_SDW_PCH_DMIC) { /* * mach_params->dmic_num will be used to set the cfg-mics value * of card->components string. Overwrite it to the actual number * of PCH DMICs used in the device. */ dmic_num = 2; mach_params->dmic_num = dmic_num;

I will only overwrite mach_params->dmic_num when it is not set.

} else if (mach_params->dmic_num) { dmic_num = 2; }

Not totally sure whether the quirk or mach_params->dmic_num should take precedence, but I went with the quirk since I assume it should not be present unless one wants to use it.

Good question, I think mach_params->dmic_num should take precedence. The quirk is mainly used for Chrome where the BIOS (coreboot) dosen't provide the DMIC information. The quirk is added if a device want to use the host DMIC. However, there is no DMIC channel information in the quirk.

charleskeepax

Is the ignore_internal_dmic flag and mach_params->dmic_num mutually exclusive for some reason I an unaware of? Because if not this still has the same problem as the last patch, if ignore_internal_dmic is true, but mach_param->dmic_num is non-zero, then the info string will be generated with "mic:dmic cfg-mics:X", although no DAI links will be created by the machine driver.

kv2019i

A bit stale comment, I wrote this in the morning but did not submit.

kv2019i · 2024-11-13T09:55:50Z

sound/soc/intel/boards/sof_sdw.c

 	 */
-	mach_params->dmic_num = dmic_num;
+	mach_params->dmic_num = mach_params->dmic_num ? mach_params->dmic_num : dmic_num;


The dual use of dmic_num confuses me quite a bit (dmic_num is used to count DAI links that can be upto 2) while mach_params->dmic_num is number of mics (upto 4). So it just happens the default is 2 DAI links ad 2 channels, so this is functionally correct.

How about if this override (now on L1147) is moved to LL140, and add a new "DMIC_DEFAULT_CHANNELS 2" define that is used to override mach_params->dmic_num if it is zero...? This would make it obvious what is happening.
UPDATE: I'm ok with the updated patchset as well.

mach_params->dmic_num will be used to set the cfg-mics value of card->components string which should be the dmic channels. However dmic_num is dmic link number and could be set due to the SOC_SDW_PCH_DMIC quirk. Set mach_params->dmic_num to the default value if the dmic link is created due to the SOC_SDW_PCH_DMIC quirk. Fixes: 7db9f63 ("ASoC: Intel: sof_sdw: overwrite mach_params->dmic_num") Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

sound/soc/intel/boards/sof_sdw.c

simontrimmer · 2024-11-13T17:47:13Z

Quick drop in to say I've just pulled the change into a test tree and tried it out on a Lenovo laptop that has four mics in NHLT and this works for me. Will try out more tomorrow.

Though, do you think that
dev_dbg(dev, "sdw %d, ssp %d, dmic %d, hdmi %d, bt: %d\n",
sdw_be_num, ssp_num, dmic_num,
intel_ctx->hdmi.idisp_codec ? hdmi_num : 0, bt_num);

Should be more explicit that it's talking about dai links rather than counts of mics?

ctx->ignore_internal_dmic is set when there is a dedicated SoundWire DMIC is in the system. In other words, ignoring internal DMIC is expected, not an error. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

The log shows the number for different type of DAIs. Add "DAI link numbers:" to make the log be more explicit. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao · 2024-11-14T02:20:17Z

Though, do you think that dev_dbg(dev, "sdw %d, ssp %d, dmic %d, hdmi %d, bt: %d\n", sdw_be_num, ssp_num, dmic_num, intel_ctx->hdmi.idisp_codec ? hdmi_num : 0, bt_num);

Should be more explicit that it's talking about dai links rather than counts of mics?

Added DAI link numbers: to the log

ujfalusi · 2024-11-14T07:48:46Z

sound/soc/intel/boards/sof_sdw.c

+		dev_warn(dev, "Ignoring internal DMIC\n");
+		mach_params->dmic_num = 0;
+	} else if (mach_params->dmic_num) {
+		dmic_num = 2;


Regardless of the value of mach_params->dmic_num dmic_num is wired to 2? This might be obvious, but it is really not to me, especially when looking at the next else branch where both dmic_num is set to two.

dmic_num here means the dmic dai link number. We always create 2 dmic dai links if internal dmic is used, one is "dmic01" and the other is "dmic16k" that's why dmic_num is 2.

@bardliao is mach_params->dmic_num 0 by default? If not, what could it be set to before we get to line 1140?

@bardliao is mach_params->dmic_num 0 by default? If not, what could it be set to before we get to line 1140?

mach->mach_params.dmic_num = check_dmic_num(sdev); is set when selecting machine driver which is before machine driver is probed.

sorry to keep prolonging the discussion @bardliao . But if mach_params->dmic is 0 and sof_sdw_quirk & SOC_SDW_PCH_DMIC is set, why is the maach_params->dmic_num set to 2? and when will this case happen?

bardliao requested review from charleskeepax, ranj063 and ujfalusi November 13, 2024 01:45

bardliao requested review from lgirdwood, plbossart, kv2019i and dbaluta as code owners November 13, 2024 01:45

bardliao force-pushed the dmic_num branch from 9826573 to c7c6a4a Compare November 13, 2024 01:54

ujfalusi reviewed Nov 13, 2024

View reviewed changes

charleskeepax suggested changes Nov 13, 2024

View reviewed changes

bardliao force-pushed the dmic_num branch from c7c6a4a to 78e4396 Compare November 13, 2024 11:41

charleskeepax suggested changes Nov 13, 2024

View reviewed changes

kv2019i reviewed Nov 13, 2024

View reviewed changes

bardliao force-pushed the dmic_num branch from 78e4396 to 07fc5ee Compare November 13, 2024 15:01

lgirdwood reviewed Nov 13, 2024

View reviewed changes

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

charleskeepax approved these changes Nov 13, 2024

View reviewed changes

bardliao added 2 commits November 14, 2024 09:59

ASoC: Intel: sof_sdw: reduce log level for not using internal dmic

c3d9b80

ctx->ignore_internal_dmic is set when there is a dedicated SoundWire DMIC is in the system. In other words, ignoring internal DMIC is expected, not an error. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

ASoC: Intel: sof_sdw: improve the log of DAI link numbers

277eda0

The log shows the number for different type of DAIs. Add "DAI link numbers:" to make the log be more explicit. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

ujfalusi reviewed Nov 14, 2024

View reviewed changes

kv2019i approved these changes Nov 14, 2024

View reviewed changes

ujfalusi approved these changes Nov 14, 2024

View reviewed changes

lgirdwood approved these changes Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASoC: Intel: sof_sdw: correct mach_params->dmic_num #5241

ASoC: Intel: sof_sdw: correct mach_params->dmic_num #5241

bardliao commented Nov 13, 2024

ujfalusi Nov 13, 2024

ujfalusi Nov 13, 2024

charleskeepax Nov 13, 2024

charleskeepax Nov 13, 2024

charleskeepax Nov 13, 2024 •

edited

Loading

bardliao Nov 13, 2024

charleskeepax left a comment

kv2019i left a comment

kv2019i Nov 13, 2024 •

edited

Loading

simontrimmer commented Nov 13, 2024

bardliao commented Nov 14, 2024

ujfalusi Nov 14, 2024

bardliao Nov 14, 2024

ranj063 Nov 14, 2024

bardliao Nov 15, 2024

ranj063 Nov 15, 2024

ASoC: Intel: sof_sdw: correct mach_params->dmic_num #5241

Are you sure you want to change the base?

ASoC: Intel: sof_sdw: correct mach_params->dmic_num #5241

Conversation

bardliao commented Nov 13, 2024

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

charleskeepax Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charleskeepax left a comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

kv2019i Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

simontrimmer commented Nov 13, 2024

bardliao commented Nov 14, 2024

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

charleskeepax Nov 13, 2024 •

edited

Loading

kv2019i Nov 13, 2024 •

edited

Loading