[PATCH 0/3] ASoC: SOF: Intel: HDA/DMIC updates
The first patch handles a problematic configuration where the wrong machine driver/topology is used: when the hardware reports an external HDaudio codec the direction is to ignore/discard ACPI SoundWire devices.
The last two patch deal with DMIC format configurations and allow users to select S16_LE even if the DMIC and internal copiers only support 24 or 32-bits. The code changes are located in sound/soc/sof/ but in the scope of Intel DAIs.
Peter Ujfalusi (2): ASoC: SOF: ipc4-topology: Allow selective update in sof_ipc4_update_hw_params ASoC: SOF: ipc4-topology: Correct DAI copier config and NHLT blob request
Pierre-Louis Bossart (1): ASoC: SOF: Intel: discard SoundWire configuration if HDaudio codec is reported
sound/soc/sof/intel/hda.c | 9 +- sound/soc/sof/intel/hda.h | 2 + sound/soc/sof/ipc4-topology.c | 245 ++++++++++++++++++++++++++-------- 3 files changed, 196 insertions(+), 60 deletions(-)
The machine driver and topology selection starts with I2S, then SoundWire and last uses HDaudio as a fallback. That assumes that the ACPI information is correct but there are of course exceptions to the rule.
On a Lenovo platform, an external HDaudio codec is detected, but the ACPI tables expose TWO RT711 jack codecs. This patch skips the SoundWire selection in case an external HDaudio codec is detected - which only works with the additional assumption that no one will mix HDaudio and SoundWire.
Closes: https://github.com/thesofproject/linux/issues/4962 Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/intel/hda.c | 9 +++++++-- sound/soc/sof/intel/hda.h | 2 ++ 2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 8ac8c10b83bd..d4f039ed4ad4 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -1809,6 +1809,7 @@ struct snd_soc_acpi_mach *hda_machine_select(struct snd_sof_dev *sdev) u32 interface_mask = hda_get_interface_mask(sdev); struct snd_sof_pdata *sof_pdata = sdev->pdata; const struct sof_dev_desc *desc = sof_pdata->desc; + struct hdac_bus *bus = sof_to_bus(sdev); struct snd_soc_acpi_mach *mach = NULL; enum snd_soc_acpi_intel_codec codec_type; const char *tplg_filename; @@ -1981,8 +1982,12 @@ struct snd_soc_acpi_mach *hda_machine_select(struct snd_sof_dev *sdev) } }
- /* If I2S fails, try SoundWire if it is supported */ - if (!mach && (interface_mask & BIT(SOF_DAI_INTEL_ALH))) + /* + * If I2S fails and no external HDaudio codec is detected, + * try SoundWire if it is supported + */ + if (!mach && !HDA_EXT_CODEC(bus->codec_mask) && + (interface_mask & BIT(SOF_DAI_INTEL_ALH))) mach = hda_sdw_machine_select(sdev);
/* diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index 7804c5b50c26..8b495264f23d 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -454,6 +454,8 @@ #define SSP_SET_SFRM_CONSUMER BIT(24) #define SSP_SET_CBP_CFP (SSP_SET_SCLK_CONSUMER | SSP_SET_SFRM_CONSUMER)
+#define HDA_EXT_ADDR 0 +#define HDA_EXT_CODEC(x) ((x) & BIT(HDA_EXT_ADDR)) #define HDA_IDISP_ADDR 2 #define HDA_IDISP_CODEC(x) ((x) & BIT(HDA_IDISP_ADDR))
From: Peter Ujfalusi peter.ujfalusi@linux.intel.com
Add a bitmask parameter to sof_ipc4_update_hw_params() to be able to select the param to be updated. This feature can be used when not all params should be updated, for example if caller only wants to update the format in the params, leaving the channels and rates untouched.
Reviewed-by: Seppo Ingalsuo seppo.ingalsuo@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/ipc4-topology.c | 83 +++++++++++++++++++++-------------- 1 file changed, 51 insertions(+), 32 deletions(-)
diff --git a/sound/soc/sof/ipc4-topology.c b/sound/soc/sof/ipc4-topology.c index a972b2b5122a..f56b7fbbe10f 100644 --- a/sound/soc/sof/ipc4-topology.c +++ b/sound/soc/sof/ipc4-topology.c @@ -1119,42 +1119,50 @@ static int sof_ipc4_widget_assign_instance_id(struct snd_sof_dev *sdev,
/* update hw_params based on the audio stream format */ static int sof_ipc4_update_hw_params(struct snd_sof_dev *sdev, struct snd_pcm_hw_params *params, - struct sof_ipc4_audio_format *fmt) + struct sof_ipc4_audio_format *fmt, u32 param_to_update) { - snd_pcm_format_t snd_fmt; struct snd_interval *i; - struct snd_mask *m; - int valid_bits = SOF_IPC4_AUDIO_FORMAT_CFG_V_BIT_DEPTH(fmt->fmt_cfg); - unsigned int channels, rate;
- switch (valid_bits) { - case 16: - snd_fmt = SNDRV_PCM_FORMAT_S16_LE; - break; - case 24: - snd_fmt = SNDRV_PCM_FORMAT_S24_LE; - break; - case 32: - snd_fmt = SNDRV_PCM_FORMAT_S32_LE; - break; - default: - dev_err(sdev->dev, "invalid PCM valid_bits %d\n", valid_bits); - return -EINVAL; + if (param_to_update & BIT(SNDRV_PCM_HW_PARAM_FORMAT)) { + int valid_bits = SOF_IPC4_AUDIO_FORMAT_CFG_V_BIT_DEPTH(fmt->fmt_cfg); + snd_pcm_format_t snd_fmt; + struct snd_mask *m; + + switch (valid_bits) { + case 16: + snd_fmt = SNDRV_PCM_FORMAT_S16_LE; + break; + case 24: + snd_fmt = SNDRV_PCM_FORMAT_S24_LE; + break; + case 32: + snd_fmt = SNDRV_PCM_FORMAT_S32_LE; + break; + default: + dev_err(sdev->dev, "invalid PCM valid_bits %d\n", valid_bits); + return -EINVAL; + } + + m = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); + snd_mask_none(m); + snd_mask_set_format(m, snd_fmt); }
- m = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); - snd_mask_none(m); - snd_mask_set_format(m, snd_fmt); + if (param_to_update & BIT(SNDRV_PCM_HW_PARAM_RATE)) { + unsigned int rate = fmt->sampling_frequency;
- rate = fmt->sampling_frequency; - i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE); - i->min = rate; - i->max = rate; + i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE); + i->min = rate; + i->max = rate; + }
- channels = SOF_IPC4_AUDIO_FORMAT_CFG_CHANNELS_COUNT(fmt->fmt_cfg); - i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); - i->min = channels; - i->max = channels; + if (param_to_update & BIT(SNDRV_PCM_HW_PARAM_CHANNELS)) { + unsigned int channels = SOF_IPC4_AUDIO_FORMAT_CFG_CHANNELS_COUNT(fmt->fmt_cfg); + + i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); + i->min = channels; + i->max = channels; + }
return 0; } @@ -1844,7 +1852,11 @@ sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget, }
/* modify the input params for the next widget */ - ret = sof_ipc4_update_hw_params(sdev, pipeline_params, &copier_data->out_format); + ret = sof_ipc4_update_hw_params(sdev, pipeline_params, + &copier_data->out_format, + BIT(SNDRV_PCM_HW_PARAM_FORMAT) | + BIT(SNDRV_PCM_HW_PARAM_CHANNELS) | + BIT(SNDRV_PCM_HW_PARAM_RATE)); if (ret) return ret;
@@ -2069,7 +2081,10 @@ static int sof_ipc4_prepare_src_module(struct snd_sof_widget *swidget, src->data.sink_rate = out_audio_fmt->sampling_frequency;
/* update pipeline_params for sink widgets */ - return sof_ipc4_update_hw_params(sdev, pipeline_params, out_audio_fmt); + return sof_ipc4_update_hw_params(sdev, pipeline_params, out_audio_fmt, + BIT(SNDRV_PCM_HW_PARAM_FORMAT) | + BIT(SNDRV_PCM_HW_PARAM_CHANNELS) | + BIT(SNDRV_PCM_HW_PARAM_RATE)); }
static int @@ -2193,7 +2208,11 @@ static int sof_ipc4_prepare_process_module(struct snd_sof_widget *swidget, sizeof(struct sof_ipc4_audio_format));
/* modify the pipeline params with the pin 0 output format */ - ret = sof_ipc4_update_hw_params(sdev, pipeline_params, &process->output_format); + ret = sof_ipc4_update_hw_params(sdev, pipeline_params, + &process->output_format, + BIT(SNDRV_PCM_HW_PARAM_FORMAT) | + BIT(SNDRV_PCM_HW_PARAM_CHANNELS) | + BIT(SNDRV_PCM_HW_PARAM_RATE)); if (ret) return ret; }
From: Peter Ujfalusi peter.ujfalusi@linux.intel.com
In case of capture and when the DAI copier have single bit depth supported on it's input side we should use this format instead of the one in fe_params.
Regardless of the stream direction for the NHLT blob lookup when the DAI copier only supports single bit depth on the DAI side we should only look for a blob which matches with this single configuration.
For DMIC if the DAI copier supports multiple bit depths, try to request 32-bit blob first if the requested bit depth is 16-bit. If the 32-bit blob is available then look for marching (32-bit) copier format to make sure that both the blob and copier have correct parameters.
Reviewed-by: Seppo Ingalsuo seppo.ingalsuo@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/ipc4-topology.c | 162 ++++++++++++++++++++++++++++------ 1 file changed, 136 insertions(+), 26 deletions(-)
diff --git a/sound/soc/sof/ipc4-topology.c b/sound/soc/sof/ipc4-topology.c index f56b7fbbe10f..d0125fd1c09f 100644 --- a/sound/soc/sof/ipc4-topology.c +++ b/sound/soc/sof/ipc4-topology.c @@ -1420,13 +1420,16 @@ static int snd_sof_get_hw_config_params(struct snd_sof_dev *sdev, struct snd_sof return 0; }
-static int snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai, - struct snd_pcm_hw_params *params, u32 dai_index, - u32 linktype, u8 dir, u32 **dst, u32 *len) +static int +snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai, + bool single_format, + struct snd_pcm_hw_params *params, u32 dai_index, + u32 linktype, u8 dir, u32 **dst, u32 *len) { struct sof_ipc4_fw_data *ipc4_data = sdev->private; struct nhlt_specific_cfg *cfg; int sample_rate, channel_count; + bool format_change = false; int bit_depth, ret; u32 nhlt_type; int dev_type = 0; @@ -1435,9 +1438,18 @@ static int snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_s switch (linktype) { case SOF_DAI_INTEL_DMIC: nhlt_type = NHLT_LINK_DMIC; - bit_depth = params_width(params); channel_count = params_channels(params); sample_rate = params_rate(params); + bit_depth = params_width(params); + /* + * Look for 32-bit blob first instead of 16-bit if copier + * supports multiple formats + */ + if (bit_depth == 16 && !single_format) { + dev_dbg(sdev->dev, "Looking for 32-bit blob first for DMIC\n"); + format_change = true; + bit_depth = 32; + } break; case SOF_DAI_INTEL_SSP: nhlt_type = NHLT_LINK_SSP; @@ -1471,22 +1483,56 @@ static int snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_s dir, dev_type);
if (!cfg) { + if (format_change) { + /* + * The 32-bit blob was not found in NHLT table, try to + * look for one based on the params + */ + bit_depth = params_width(params); + format_change = false; + + cfg = intel_nhlt_get_endpoint_blob(sdev->dev, ipc4_data->nhlt, + dai_index, nhlt_type, + bit_depth, bit_depth, + channel_count, sample_rate, + dir, dev_type); + if (cfg) + goto out; + } + dev_err(sdev->dev, "no matching blob for sample rate: %d sample width: %d channels: %d\n", sample_rate, bit_depth, channel_count); return -EINVAL; }
+out: /* config length should be in dwords */ *len = cfg->size >> 2; *dst = (u32 *)cfg->caps;
+ if (format_change) { + /* + * Update the params to reflect that we have loaded 32-bit blob + * instead of the 16-bit. + * This information is going to be used by the caller to find + * matching copier format on the dai side. + */ + struct snd_mask *m; + + m = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); + snd_mask_none(m); + snd_mask_set_format(m, SNDRV_PCM_FORMAT_S32_LE); + } + return 0; } #else -static int snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai, - struct snd_pcm_hw_params *params, u32 dai_index, - u32 linktype, u8 dir, u32 **dst, u32 *len) +static int +snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai, + bool single_format, + struct snd_pcm_hw_params *params, u32 dai_index, + u32 linktype, u8 dir, u32 **dst, u32 *len) { return 0; } @@ -1517,6 +1563,68 @@ bool sof_ipc4_copier_is_single_format(struct snd_sof_dev *sdev, return true; }
+static int +sof_ipc4_prepare_dai_copier(struct snd_sof_dev *sdev, struct snd_sof_dai *dai, + struct snd_pcm_hw_params *params, int dir) +{ + struct sof_ipc4_available_audio_format *available_fmt; + struct snd_pcm_hw_params dai_params = *params; + struct sof_ipc4_copier_data *copier_data; + struct sof_ipc4_copier *ipc4_copier; + bool single_format; + int ret; + + ipc4_copier = dai->private; + copier_data = &ipc4_copier->data; + available_fmt = &ipc4_copier->available_fmt; + + /* + * If the copier on the DAI side supports only single bit depth then + * this depth (format) should be used to look for the NHLT blob (if + * needed) and in case of capture this should be used for the input + * format lookup + */ + if (dir == SNDRV_PCM_STREAM_PLAYBACK) { + single_format = sof_ipc4_copier_is_single_format(sdev, + available_fmt->output_pin_fmts, + available_fmt->num_output_formats); + + /* Update the dai_params with the only supported format */ + if (single_format) { + ret = sof_ipc4_update_hw_params(sdev, &dai_params, + &available_fmt->output_pin_fmts[0].audio_fmt, + BIT(SNDRV_PCM_HW_PARAM_FORMAT)); + if (ret) + return ret; + } + } else { + single_format = sof_ipc4_copier_is_single_format(sdev, + available_fmt->input_pin_fmts, + available_fmt->num_input_formats); + + /* Update the dai_params with the only supported format */ + if (single_format) { + ret = sof_ipc4_update_hw_params(sdev, &dai_params, + &available_fmt->input_pin_fmts[0].audio_fmt, + BIT(SNDRV_PCM_HW_PARAM_FORMAT)); + if (ret) + return ret; + } + } + + ret = snd_sof_get_nhlt_endpoint_data(sdev, dai, single_format, + &dai_params, + ipc4_copier->dai_index, + ipc4_copier->dai_type, dir, + &ipc4_copier->copier_config, + &copier_data->gtw_cfg.config_length); + /* Update the params to reflect the changes made in this function */ + if (!ret) + *params = dai_params; + + return ret; +} + static int sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget, struct snd_pcm_hw_params *fe_params, @@ -1527,7 +1635,7 @@ sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget, struct snd_soc_component *scomp = swidget->scomp; struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp); struct sof_ipc4_copier_data *copier_data; - struct snd_pcm_hw_params *ref_params; + struct snd_pcm_hw_params ref_params; struct sof_ipc4_copier *ipc4_copier; struct snd_sof_dai *dai; u32 gtw_cfg_config_length; @@ -1605,9 +1713,9 @@ sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget, * for capture. */ if (dir == SNDRV_PCM_STREAM_PLAYBACK) - ref_params = fe_params; + ref_params = *fe_params; else - ref_params = pipeline_params; + ref_params = *pipeline_params;
copier_data->gtw_cfg.node_id &= ~SOF_IPC4_NODE_INDEX_MASK; copier_data->gtw_cfg.node_id |= @@ -1633,23 +1741,25 @@ sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget, available_fmt = &ipc4_copier->available_fmt;
/* - * When there is format conversion within a pipeline, the number of supported - * output formats is typically limited to just 1 for the DAI copiers. But when there - * is no format conversion, the DAI copiers input format must match that of the - * FE hw_params for capture and the pipeline params for playback. + * Use the fe_params as a base for the copier configuration. + * The ref_params might get updated to reflect what format is + * supported by the copier on the DAI side. + * + * In case of capture the ref_params returned will be used to + * find the input configuration of the copier. */ - if (dir == SNDRV_PCM_STREAM_PLAYBACK) - ref_params = pipeline_params; - else - ref_params = fe_params; - - ret = snd_sof_get_nhlt_endpoint_data(sdev, dai, fe_params, ipc4_copier->dai_index, - ipc4_copier->dai_type, dir, - &ipc4_copier->copier_config, - &copier_data->gtw_cfg.config_length); + ref_params = *fe_params; + ret = sof_ipc4_prepare_dai_copier(sdev, dai, &ref_params, dir); if (ret < 0) return ret;
+ /* + * For playback the pipeline_params needs to be used to find the + * input configuration of the copier. + */ + if (dir == SNDRV_PCM_STREAM_PLAYBACK) + ref_params = *pipeline_params; + break; } case snd_soc_dapm_buffer: @@ -1657,7 +1767,7 @@ sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget, ipc4_copier = (struct sof_ipc4_copier *)swidget->private; copier_data = &ipc4_copier->data; available_fmt = &ipc4_copier->available_fmt; - ref_params = pipeline_params; + ref_params = *pipeline_params;
break; } @@ -1668,8 +1778,8 @@ sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget, }
/* set input and output audio formats */ - ret = sof_ipc4_init_input_audio_fmt(sdev, swidget, &copier_data->base_config, ref_params, - available_fmt); + ret = sof_ipc4_init_input_audio_fmt(sdev, swidget, &copier_data->base_config, + &ref_params, available_fmt); if (ret < 0) return ret;
On Fri, 03 May 2024 08:32:50 -0500, Pierre-Louis Bossart wrote:
The first patch handles a problematic configuration where the wrong machine driver/topology is used: when the hardware reports an external HDaudio codec the direction is to ignore/discard ACPI SoundWire devices.
The last two patch deal with DMIC format configurations and allow users to select S16_LE even if the DMIC and internal copiers only support 24 or 32-bits. The code changes are located in sound/soc/sof/ but in the scope of Intel DAIs.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/3] ASoC: SOF: Intel: discard SoundWire configuration if HDaudio codec is reported commit: 5a7543d0ca01d68d992f480d151efd693807e0ce [2/3] ASoC: SOF: ipc4-topology: Allow selective update in sof_ipc4_update_hw_params commit: b679302526d637700e998da85ed1e06d8f7d3674 [3/3] ASoC: SOF: ipc4-topology: Correct DAI copier config and NHLT blob request commit: f9209644ae7688e82f629e737417bc8916db7b57
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Mark Brown -
Pierre-Louis Bossart