| Message ID | 20240516075611.18018-1-peter.ujfalusi@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 49cb894d567980235b6e64d5e69950ff77debd8c |
| Headers | show |
| Series | ASoC: SOF: ipc4-topology: Add support for NHLT with 16-bit only DMIC blob | expand |
Hi Mark, On 16/05/2024 10:56, Peter Ujfalusi wrote: > The ACPI NHLT table always had 32-bit DMIC blob even if 16-bit was also > present and taken as a 'rule' which obviously got broken and there is at > least one device on the market which ships with only 16-bit DMIC > configuration blob. > This corner case has never been supported and it is going to need topology > updates for DMIC copier to support multiple formats. > > As for the kernel side: if the copier supports multiple formats and the > preferred 32-bit DMIC blob is not found then we will try to get a 16-bit > DMIC configuration and look for a 16-bit copier config. Please ignore this patch, I will send it again along with few more patch building on this to close the gaps with the logic of selecting the NHLT blob. > Fixes: f9209644ae76 ("ASoC: SOF: ipc4-topology: Correct DAI copier config and NHLT blob request") > Link: https://github.com/thesofproject/linux/issues/4973 > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> > Reviewed-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com> > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> > --- > sound/soc/sof/ipc4-topology.c | 25 ++++++++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/sof/ipc4-topology.c b/sound/soc/sof/ipc4-topology.c > index beff10989324..521b4dcba601 100644 > --- a/sound/soc/sof/ipc4-topology.c > +++ b/sound/soc/sof/ipc4-topology.c > @@ -1483,6 +1483,8 @@ snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai > dir, dev_type); > > if (!cfg) { > + bool get_new_blob = false; > + > if (format_change) { > /* > * The 32-bit blob was not found in NHLT table, try to > @@ -1490,7 +1492,20 @@ snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai > */ > bit_depth = params_width(params); > format_change = false; > + get_new_blob = true; > + } else if (linktype == SOF_DAI_INTEL_DMIC && !single_format) { > + /* > + * The requested 32-bit blob (no format change for the > + * blob request) was not found in NHLT table, try to > + * look for 16-bit blob if the copier supports multiple > + * formats > + */ > + bit_depth = 16; > + format_change = true; > + get_new_blob = true; > + } > > + if (get_new_blob) { > cfg = intel_nhlt_get_endpoint_blob(sdev->dev, ipc4_data->nhlt, > dai_index, nhlt_type, > bit_depth, bit_depth, > @@ -1513,8 +1528,8 @@ snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai > > if (format_change) { > /* > - * Update the params to reflect that we have loaded 32-bit blob > - * instead of the 16-bit. > + * Update the params to reflect that different blob was loaded > + * instead of the requested bit depth (16 -> 32 or 32 -> 16). > * This information is going to be used by the caller to find > * matching copier format on the dai side. > */ > @@ -1522,7 +1537,11 @@ snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai > > m = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); > snd_mask_none(m); > - snd_mask_set_format(m, SNDRV_PCM_FORMAT_S32_LE); > + if (bit_depth == 16) > + snd_mask_set_format(m, SNDRV_PCM_FORMAT_S16_LE); > + else > + snd_mask_set_format(m, SNDRV_PCM_FORMAT_S32_LE); > + > } > > return 0;
On Wed, May 29, 2024 at 02:19:37PM +0300, Péter Ujfalusi wrote: > On 16/05/2024 10:56, Peter Ujfalusi wrote: > > The ACPI NHLT table always had 32-bit DMIC blob even if 16-bit was also > > present and taken as a 'rule' which obviously got broken and there is at > > least one device on the market which ships with only 16-bit DMIC > > configuration blob. > > This corner case has never been supported and it is going to need topology > > updates for DMIC copier to support multiple formats. > Please ignore this patch, I will send it again along with few more patch > building on this to close the gaps with the logic of selecting the NHLT > blob. Sorry, it's already been applied and published with more stuff in CI now based on it.
On 29/05/2024 14:48, Mark Brown wrote: > On Wed, May 29, 2024 at 02:19:37PM +0300, Péter Ujfalusi wrote: >> On 16/05/2024 10:56, Peter Ujfalusi wrote: > >>> The ACPI NHLT table always had 32-bit DMIC blob even if 16-bit was also >>> present and taken as a 'rule' which obviously got broken and there is at >>> least one device on the market which ships with only 16-bit DMIC >>> configuration blob. >>> This corner case has never been supported and it is going to need topology >>> updates for DMIC copier to support multiple formats. > >> Please ignore this patch, I will send it again along with few more patch >> building on this to close the gaps with the logic of selecting the NHLT >> blob. > > Sorry, it's already been applied and published with more stuff in CI now > based on it. Right, I don't see it in for-next, for-6.10 or for-linus. This patch should be sent for 6.10 and I can prepare and send the rest (4 more patches) to close the remaining holes regarding to NHLT blob selection logic.
On Wed, May 29, 2024 at 02:57:02PM +0300, Péter Ujfalusi wrote: > On 29/05/2024 14:48, Mark Brown wrote: > > Sorry, it's already been applied and published with more stuff in CI now > > based on it. > Right, I don't see it in for-next, for-6.10 or for-linus. This patch > should be sent for 6.10 and I can prepare and send the rest (4 more > patches) to close the remaining holes regarding to NHLT blob selection > logic. If something is a bug fix it shouldn't have a subject saying "Add support", that is very obvioulsy a new feature. Have these systems ever worked?
On 29/05/2024 15:17, Mark Brown wrote: > On Wed, May 29, 2024 at 02:57:02PM +0300, Péter Ujfalusi wrote: >> On 29/05/2024 14:48, Mark Brown wrote: > >>> Sorry, it's already been applied and published with more stuff in CI now >>> based on it. > >> Right, I don't see it in for-next, for-6.10 or for-linus. This patch >> should be sent for 6.10 and I can prepare and send the rest (4 more >> patches) to close the remaining holes regarding to NHLT blob selection >> logic. > > If something is a bug fix it shouldn't have a subject saying "Add > support", that is very obvioulsy a new feature. Have these systems ever > worked? Well, it is adding support for something which has never been consider as a valid configuration (16bit only DMIC blob in NHLT table of a laptop [1]). I have added to fixes tag to carry this patch along with the patch that implemented support for preferring 32bit configuration in firmware (the [1] would never worked, so that is unrelated). The mentioned additional patches would future proof this whole DMIC blob lookup by extending it beyond the bytes per sample. These are future proofing and a would be nice fixes for the -rc, but if not than 6.11 is OK. In short, yes it worked well but we were presented with unimaginable vendor creativity. [1] https://github.com/thesofproject/linux/issues/4973
On Wed, May 29, 2024 at 03:32:24PM +0300, Péter Ujfalusi wrote: > On 29/05/2024 15:17, Mark Brown wrote: > > If something is a bug fix it shouldn't have a subject saying "Add > > support", that is very obvioulsy a new feature. Have these systems ever > > worked? > Well, it is adding support for something which has never been consider > as a valid configuration (16bit only DMIC blob in NHLT table of a laptop > [1]). > I have added to fixes tag to carry this patch along with the patch that > implemented support for preferring 32bit configuration in firmware (the > [1] would never worked, so that is unrelated). Fixes tags are so widely abused that I don't generally pay a huge amount of attention to them, the pattern of new features claiming to fix the commit where something was introduced is very common. In any case, if you're going to send a series for 6.10 I guess I'll leave it for now.
On 29/05/2024 16:36, Mark Brown wrote: >> Well, it is adding support for something which has never been consider >> as a valid configuration (16bit only DMIC blob in NHLT table of a laptop >> [1]). >> I have added to fixes tag to carry this patch along with the patch that >> implemented support for preferring 32bit configuration in firmware (the >> [1] would never worked, so that is unrelated). > > Fixes tags are so widely abused that I don't generally pay a huge amount > of attention to them, the pattern of new features claiming to fix the > commit where something was introduced is very common. Understood. > In any case, if you're going to send a series for 6.10 I guess I'll > leave it for now. Should I include this patch to the series for 6.10 or wait until this patch is available somewhere that I can base my series on?
On Wed, May 29, 2024 at 04:49:01PM +0300, Péter Ujfalusi wrote: > On 29/05/2024 16:36, Mark Brown wrote: > > In any case, if you're going to send a series for 6.10 I guess I'll > > leave it for now. > Should I include this patch to the series for 6.10 or wait until this > patch is available somewhere that I can base my series on? Like I say I'll leave things for now so please resend along with everything else.
On Thu, 16 May 2024 10:56:11 +0300, Peter Ujfalusi wrote: > The ACPI NHLT table always had 32-bit DMIC blob even if 16-bit was also > present and taken as a 'rule' which obviously got broken and there is at > least one device on the market which ships with only 16-bit DMIC > configuration blob. > This corner case has never been supported and it is going to need topology > updates for DMIC copier to support multiple formats. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: SOF: ipc4-topology: Add support for NHLT with 16-bit only DMIC blob commit: 49cb894d567980235b6e64d5e69950ff77debd8c 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
diff --git a/sound/soc/sof/ipc4-topology.c b/sound/soc/sof/ipc4-topology.c index beff10989324..521b4dcba601 100644 --- a/sound/soc/sof/ipc4-topology.c +++ b/sound/soc/sof/ipc4-topology.c @@ -1483,6 +1483,8 @@ snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai dir, dev_type); if (!cfg) { + bool get_new_blob = false; + if (format_change) { /* * The 32-bit blob was not found in NHLT table, try to @@ -1490,7 +1492,20 @@ snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai */ bit_depth = params_width(params); format_change = false; + get_new_blob = true; + } else if (linktype == SOF_DAI_INTEL_DMIC && !single_format) { + /* + * The requested 32-bit blob (no format change for the + * blob request) was not found in NHLT table, try to + * look for 16-bit blob if the copier supports multiple + * formats + */ + bit_depth = 16; + format_change = true; + get_new_blob = true; + } + if (get_new_blob) { cfg = intel_nhlt_get_endpoint_blob(sdev->dev, ipc4_data->nhlt, dai_index, nhlt_type, bit_depth, bit_depth, @@ -1513,8 +1528,8 @@ snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai if (format_change) { /* - * Update the params to reflect that we have loaded 32-bit blob - * instead of the 16-bit. + * Update the params to reflect that different blob was loaded + * instead of the requested bit depth (16 -> 32 or 32 -> 16). * This information is going to be used by the caller to find * matching copier format on the dai side. */ @@ -1522,7 +1537,11 @@ snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai m = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); snd_mask_none(m); - snd_mask_set_format(m, SNDRV_PCM_FORMAT_S32_LE); + if (bit_depth == 16) + snd_mask_set_format(m, SNDRV_PCM_FORMAT_S16_LE); + else + snd_mask_set_format(m, SNDRV_PCM_FORMAT_S32_LE); + } return 0;
The ACPI NHLT table always had 32-bit DMIC blob even if 16-bit was also present and taken as a 'rule' which obviously got broken and there is at least one device on the market which ships with only 16-bit DMIC configuration blob. This corner case has never been supported and it is going to need topology updates for DMIC copier to support multiple formats. As for the kernel side: if the copier supports multiple formats and the preferred 32-bit DMIC blob is not found then we will try to get a 16-bit DMIC configuration and look for a 16-bit copier config. Fixes: f9209644ae76 ("ASoC: SOF: ipc4-topology: Correct DAI copier config and NHLT blob request") Link: https://github.com/thesofproject/linux/issues/4973 Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> Reviewed-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> --- sound/soc/sof/ipc4-topology.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)