ALSA/ASoC: hda: Address format selection limitations and ambiguity #4539

crojewsk-intel · 2023-08-22T16:32:09Z

This PR is provided for testing purposes so that no regression occurs on sof-driver side and mirrors thread present on upstream. Once tested, PR will be closed.

Patchset aims to address format selection restrictions present currently in the HDAudio library. Formats which we are concerned about are 20 and 24 valid bits per sample within 32 bit depth container. One may identify them as S20_LE and S24_LE except that those, according to comments found in include/uapi/sound/asound.h, are for LSB-aligned scenarios. HDAudio streams expect MSB-aligned data, no matter if we are speaking of HOST (SDxFMT) or LINK (PPLCxFMT) side - chapter 4.5.1 of the public HDAudio specification. In short, S20_LE and S24_LE are invalid options.

Right now, given the implementation of snd_hdac_query_supported_pcm() within sound/hda/hdac_device.c, even if a codec responds with: "I support all possible formats specified within HDAudio specification", there will be no option to open a 20/32 or 24/32 stream. The kernel will force the stream to be opened using the highest available bit depth.

After discussing subject initially with Jaroslav and Takashi, suggestion was made to utilize 'subformat' option to address the problem. The eye-opening discussion begun much earlier though, in 2019 [1].

Paired with PRs for alsa-utils [2] and alsa-lib [3].

Flow of changes:

The very first patch adds MSBITS subformat options to allow for granular
20/32, 24/32 and 32/32 format selection. The next three make sure
subformat is actually honored during runtime. Most of that code is based
on format-related API.

Follow up is upgrade to the hda stream-format interface - several functions are added to make the granular format selection simple in the HDAudio world. Core of the implementation is based on the existing snd_hdac_calc_stream_format(). The next ten patches are straightforward switch from one interface to another with cleanup of now-unsed function as a finishing touch.

Last but not least - the avs-driver, on which the problem analyzed and debugged, is updated to no longer acknowledge S24_LE as a valid format option.

Changes in v1:

fixed UBSAN due to missing snd_pcm_subformat_names[] entries for new subformats
as HDMI stream capabilities are assigned on PCM open, patch 16/17 has been updated to ignore such codecs for now. A separate patchset will take care of this case
params_bps() reworded to snd_pcm_hw_params_bps()
fixed compilation issues in sof-driver, patch 13/17

[1]: https://lore.kernel.org/alsa-devel/[email protected]/
[2]: alsa-project/alsa-utils#228
[3]: alsa-project/alsa-lib#342

Improve granularity of format selection for S32/U32 formats by adding constants representing 20, 24 and 32 most significant bits. To make it easy for drivers to utilize those constants, introduce snd_pcm_subformat_width() and snd_pcm_hw_params_bps(). While the former is self-explanatory, the latter returns the bit-per-sample value based on format, subformat and msbits characteristics of the provided hw_params. Signed-off-by: Cezary Rojewski <[email protected]>

Substream value is currently hardcoded to SNDRV_PCM_SUBFORMAT_STD. Update the constraint procedure so that subformat selection is not ignored. Signed-off-by: Cezary Rojewski <[email protected]>

Update mechanism for querying supported PCMs to allow for granular format selection when container size is 32 bits. Currently always the highest bit depth is selected, regardless of how many actual formats codec in question supports. Signed-off-by: Cezary Rojewski <[email protected]>

Subformat options are ignored when setting up hardware parameters and assigning PCM stream capabilities. Account for them to allow for granular format selection. Signed-off-by: Cezary Rojewski <[email protected]>

Introduce a set of functions that facilite SDxFMT-related calculations in atomic manner: snd_hdac_format_normalize() - format converter. S20_LE, S24_LE and their unsigned and BE friends are invalid from HDAudio perspective but still can be specified as function argument due to compatibility reasons. snd_hdac_stream_format_bps() - obtain just the bits-per-sample value. Does not ignore subformat and msbits parameters. snd_hdac_stream_format() and snd_hdac_spdif_stream_format() - obtain the SDxFMT value given the audio format parameters. The former is stripped away of spdif-related information. Useful for users that do not care about them. Signed-off-by: Cezary Rojewski <[email protected]>

To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism. Signed-off-by: Cezary Rojewski <[email protected]>

To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism. While at it, complete PCM stream initialization with subformat options. Signed-off-by: Cezary Rojewski <[email protected]>

To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism. Signed-off-by: Cezary Rojewski <[email protected]>

There are no users of the function. Signed-off-by: Cezary Rojewski <[email protected]>

Eliminate all occurrences of S24_LE within the HDAudio related pcm code, both HOST and LINK side. Replace those with MSBITS subformats to allow for granular selection when S32_LE is the format of choice. Signed-off-by: Cezary Rojewski <[email protected]>

To not expose more than in fact is supported by the codec, update CPU DAI initialization procedure to rely on codec capabilities instead of hardcoding them. This includes subformat which is currently ignored. As capabilities for HDMI streams are initialized on PCM open, leave it as is for now. Signed-off-by: Cezary Rojewski <[email protected]>

plbossart · 2023-08-22T17:44:52Z

sound/soc/sof/intel/hda-dai-ops.c

-	format_val = snd_hdac_calc_stream_format(params_rate(params), params_channels(params),
-						 params_format(params),
-						 params_physical_width(params),
-						 0);


there will be a minor conflict with 255f5af, but easy enough to resolve.

crojewsk-intel · 2023-08-22T17:54:28Z

I see some timeouts in suspend/resume scenarios and even a panic in check-kmod-load-unload-after-playback case on GML but I fail to find a connection between my patchset and the negative results.

plbossart · 2023-08-22T17:58:36Z

https://sof-ci.01.org/linuxpr/PR4539/build6261/devicetest/index.html?model=GLK_BOB_DA7219&testcase=check-kmod-load-unload-after-playback is a known issue with the codec driver.

RanderWang

I also notice the mismatch of S24_LE and HDA spec and we do some effort to deal with it in SOF FW. I am glad to remove S24_LE from pcm capability and will make life easier for FW developer.

ujfalusi

@crojewsk-intel, can you explain why the existing msbits support is not good enough when paired with S32_LE?
S32_LE + msbits=24 == 24 bits audio in 32bit
S32_LE + msbits=20 == 20 bits audio in 32bit
S32_LE + msbits=0/32 == 32 bits audio

ujfalusi · 2023-08-25T09:11:49Z

sound/core/pcm_native.c

@@ -2634,8 +2636,7 @@ static int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream)
 	if (err < 0)
 		return err;

-	err = snd_pcm_hw_constraint_mask(runtime, SNDRV_PCM_HW_PARAM_SUBFORMAT,
-					 PARAM_MASK_BIT(SNDRV_PCM_SUBFORMAT_STD));
+	err = snd_pcm_hw_constraint_mask(runtime, SNDRV_PCM_HW_PARAM_SUBFORMAT, hw->subformats);


snd_pcm_hw_constraint_mask64() ?

ujfalusi · 2023-08-25T09:33:14Z

sound/core/pcm_native.c

@@ -2614,6 +2614,8 @@ static int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream)
 	int err;
 	unsigned int mask = 0;

+	/* All PCMs support at least the default STD subformat. */
+	hw->subformats |= PARAM_MASK_BIT(SNDRV_PCM_SUBFORMAT_STD);


There is no definition of SNDRV_PCM_SUBFORMAT_STD that I can find, but it looks to me that it is there to not fail when setting the mask?
SNDRV_PCM_SUBFORMAT_STD does not cover any supported subformat, it is just BIT(0) and to keep snd_pcm_hw_constraint_mask() not to fail few lines down?

STD is the default and expected to be always supported by any device. So, when adding new flags I had to OR it here to avoid breaking audio for most of the world.

Based on the upstream discussion, the SNDRV_PCM_SUBFORMAT_STD tells that the whole physical width is used?
But how that maps with S24_LE for example?

I would expect that drivers would set explicit subformats if they care and they might say that I'm not supporting STD and here you override that statement.

if (!hw->subformats) hw->subformats = PARAM_MASK_BIT(SNDRV_PCM_SUBFORMAT_STD);

might be what should be done here?

sound/hda/hdac_device.c

ujfalusi · 2023-08-25T09:45:01Z

sound/hda/hdac_device.c

@@ -848,8 +849,9 @@ int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid,
 		*ratesp = rates;
 	}

-	if (formatsp || bpsp) {
+	if (formatsp || subformatsp || bpsp) {


commit title is a bit misleading as this patch will not make the code to honor subformat, but it will also querry, set it up based on the hardware.

Honoring a variable from snd_hdac_query_supported_pcm() point of view is: let it be queryable just like format and bsp are.

For me 'honoring' means that based on the subformat do the right thing, take subformat into account.
"Extend supported format query with subformat support", perhaps better?

sound/hda/hdac_device.c

ujfalusi · 2023-08-25T10:00:34Z

sound/hda/hdac_device.c

+ *
+ * Return: The format bitset or zero if invalid.
+ */
+unsigned int snd_hdac_spdif_stream_format(unsigned int channels, unsigned int bps,


The 'spdif' in function name implies that it is for SPDIF format type, but it is not..

Similarly to how things are right now. The goal for the split is different - most users call stream-format calculation routine with '0' (ignore) as last parameter. Having two variants - one for those that do not care about spdif bits and one for those that do care - makes things clear.

So this is to be used in place of snd_hdac_calc_stream_format()? It is doing the same thing at the end.

snd_hdac_calc_stream_format(params_rate(params), params_channels(params), params_format(params), maxbps, 0);

snd_hdac_spdif_stream_format(params_channels(params), snd_pcm_hw_params_bps(params), params_rate(params), 0);

ujfalusi · 2023-08-25T10:01:37Z

sound/pci/hda/hda_controller.c

-						ctls);
+	bps = snd_hdac_stream_format_bps(runtime->format, SNDRV_PCM_SUBFORMAT_STD, hinfo->maxbps);
+
+	format_val = snd_hdac_spdif_stream_format(runtime->channels, bps, runtime->rate, ctls);


I'm not sure what the commit message trying to tell. Is it valid to select smaller bps than what required for the format?

It's the case even now - set msbits/sigbits to 24 and select format=S16_LE.

I don't understand how this achieves the goal set by the commit message. It is switching to a new set of function calls to get the format_val, right?
No change in functionality, or am I missing something?

ujfalusi · 2023-08-25T10:03:53Z

sound/soc/codecs/hdac_hdmi.c

-	format = snd_hdac_calc_stream_format(params_rate(hparams),
-			params_channels(hparams), params_format(hparams),
-			dai->driver->playback.sig_bits, 0);
+	bps = snd_hdac_stream_format_bps(params_format(hparams), SNDRV_PCM_SUBFORMAT_STD,


why with the fixed subformat and not the params_subformat() ?

Ditto as for hda_controller.c case.

ujfalusi · 2023-08-25T10:04:35Z

sound/soc/intel/skylake/skl-pcm.c

@@ -138,8 +139,8 @@ int skl_pcm_host_dma_prepare(struct device *dev, struct skl_pipe_params *params)
 	stream = stream_to_hdac_ext_stream(hstream);
 	snd_hdac_ext_stream_decouple(bus, stream, true);

-	format_val = snd_hdac_calc_stream_format(params->s_freq,
-			params->ch, params->format, params->host_bps, 0);
+	bps = snd_hdac_stream_format_bps(params->format, SNDRV_PCM_SUBFORMAT_STD, params->host_bps);


Here as well. I beginning to think that I'm missing something fundamental now.

ujfalusi · 2023-08-25T10:06:23Z

sound/soc/intel/avs/pcm.c

 	},
 	.capture = {
 		.channels_min	= 1,
 		.channels_max	= 8,
 		.rates		= SNDRV_PCM_RATE_8000_192000,
 		.formats	= SNDRV_PCM_FMTBIT_S16_LE |
-				  SNDRV_PCM_FMTBIT_S24_LE |


Would not just this change could resolve the unsupported S24_LE advertised to userspace?

Such change alone is insufficient. It blocks usage of S24_LE yes, but simultaneously does not allow for selecting 20, 24 or 32 as valid bit depth.

what is blocking the use of msbits?
Yes, we can only say for example that msbits=32 and have no information that the setup actually supports msbits=24, right?

I mean, should the SNDRV_PCM_SUBFMTBIT_MSBITS_24 be more specific, like SNDRV_PCM_SUBFMTBIT_24_MSBITS_IN_32?
If we really want it to be part of this struct snd_soc_pcm_stream or should we have this as refine information for user space that with S32_LE it could use 20, 24 MSB or 24, 32 MSB and let it deal with it?

crojewsk-intel · 2023-08-25T14:38:13Z

@crojewsk-intel, can you explain why the existing msbits support is not good enough when paired with S32_LE? S32_LE + msbits=24 == 24 bits audio in 32bit S32_LE + msbits=20 == 20 bits audio in 32bit S32_LE + msbits=0/32 == 32 bits audio

With all due respect, I'd like to avoid duplicating the upstream discussion. The exact same question has been asked by Pierre [1]. If you feel more answers are needed, host the questions there.
In short, msbits/sigbits is a field that represents a single value, not a mask. A device should be able to tell it supports 20/24/32 valid bit depth within 4-byte wide container back to the user space.

[1]: https://lore.kernel.org/alsa-devel/[email protected]/T/#m66d83b7a47cd1277a4081416b3ec75b4c70ac93d

ujfalusi · 2023-08-29T07:09:49Z

sound/core/pcm_lib.c

+ * Return: The number of bits per sample based on the format,
+ * subformat and msbits the specified hw params has.
+ */
+int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p)


Would it make sense to update the params_width() so all drivers will get to support the subformat out of box?

ujfalusi · 2023-08-29T07:18:59Z

sound/core/pcm_native.c

@@ -2614,6 +2614,8 @@ static int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream)
 	int err;
 	unsigned int mask = 0;

+	/* All PCMs support at least the default STD subformat. */
+	hw->subformats |= PARAM_MASK_BIT(SNDRV_PCM_SUBFORMAT_STD);


Based on the upstream discussion, the SNDRV_PCM_SUBFORMAT_STD tells that the whole physical width is used?
But how that maps with S24_LE for example?

I would expect that drivers would set explicit subformats if they care and they might say that I'm not supporting STD and here you override that statement.

if (!hw->subformats) hw->subformats = PARAM_MASK_BIT(SNDRV_PCM_SUBFORMAT_STD);

might be what should be done here?

ujfalusi · 2023-08-29T07:23:27Z

sound/hda/hdac_device.c

@@ -848,8 +849,9 @@ int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid,
 		*ratesp = rates;
 	}

-	if (formatsp || bpsp) {
+	if (formatsp || subformatsp || bpsp) {


For me 'honoring' means that based on the subformat do the right thing, take subformat into account.
"Extend supported format query with subformat support", perhaps better?

ujfalusi · 2023-08-29T07:32:28Z

sound/hda/hdac_device.c

+ *
+ * Return: The format bitset or zero if invalid.
+ */
+unsigned int snd_hdac_spdif_stream_format(unsigned int channels, unsigned int bps,


So this is to be used in place of snd_hdac_calc_stream_format()? It is doing the same thing at the end.

snd_hdac_calc_stream_format(params_rate(params), params_channels(params), params_format(params), maxbps, 0);

snd_hdac_spdif_stream_format(params_channels(params), snd_pcm_hw_params_bps(params), params_rate(params), 0);

ujfalusi · 2023-08-29T07:49:53Z

sound/pci/hda/hda_controller.c

-						ctls);
+	bps = snd_hdac_stream_format_bps(runtime->format, SNDRV_PCM_SUBFORMAT_STD, hinfo->maxbps);
+
+	format_val = snd_hdac_spdif_stream_format(runtime->channels, bps, runtime->rate, ctls);


I don't understand how this achieves the goal set by the commit message. It is switching to a new set of function calls to get the format_val, right?
No change in functionality, or am I missing something?

ujfalusi · 2023-08-29T07:51:11Z

sound/pci/hda/hda_controller.c

-						runtime->format,
-						hinfo->maxbps,
-						ctls);
+	bps = snd_hdac_stream_format_bps(runtime->format, SNDRV_PCM_SUBFORMAT_STD, hinfo->maxbps);


I see, so this is basically a no functional change to switch function calls to get the format_val?

ujfalusi · 2023-08-29T07:52:39Z

sound/soc/codecs/hdac_hda.c

-						 params_format(params),
-						 maxbps,
-						 0);
+	format_val = snd_hdac_stream_format(params_channels(params), bps, params_rate(params));


group the snd_hdac_stream_format_bps() call here, before calling snd_hdac_stream_format()?
or the other way around

ujfalusi · 2023-08-29T08:57:55Z

sound/soc/intel/avs/pcm.c

 	},
 	.capture = {
 		.channels_min	= 1,
 		.channels_max	= 8,
 		.rates		= SNDRV_PCM_RATE_8000_192000,
 		.formats	= SNDRV_PCM_FMTBIT_S16_LE |
-				  SNDRV_PCM_FMTBIT_S24_LE |


what is blocking the use of msbits?
Yes, we can only say for example that msbits=32 and have no information that the setup actually supports msbits=24, right?

I mean, should the SNDRV_PCM_SUBFMTBIT_MSBITS_24 be more specific, like SNDRV_PCM_SUBFMTBIT_24_MSBITS_IN_32?
If we really want it to be part of this struct snd_soc_pcm_stream or should we have this as refine information for user space that with S32_LE it could use 20, 24 MSB or 24, 32 MSB and let it deal with it?

plbossart · 2023-09-12T22:42:18Z

The thread has now moved upstream and there's another alternate proposal from @perexg, closing this PR

crojewsk-intel added 17 commits August 22, 2023 18:17

ALSA: pcm: Honor subformat when configuring substream

fcd069e

Substream value is currently hardcoded to SNDRV_PCM_SUBFORMAT_STD. Update the constraint procedure so that subformat selection is not ignored. Signed-off-by: Cezary Rojewski <[email protected]>

ASoC: pcm: Honor subformat when configuring runtime

1f8b4a0

Subformat options are ignored when setting up hardware parameters and assigning PCM stream capabilities. Account for them to allow for granular format selection. Signed-off-by: Cezary Rojewski <[email protected]>

ALSA: hda: Switch to new stream-format interface

ce49474

To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism. Signed-off-by: Cezary Rojewski <[email protected]>

ALSA: hda/hdmi: Switch to new stream-format interface

9259b37

To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism. Signed-off-by: Cezary Rojewski <[email protected]>

ALSA: hda/ca0132: Switch to new stream-format interface

20c2126

To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism. Signed-off-by: Cezary Rojewski <[email protected]>

ASoC: codecs: hdac_hda: Switch to new stream-format interface

5736c7e

To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism. Signed-off-by: Cezary Rojewski <[email protected]>

ASoC: codecs: hdac_hdmi: Switch to new stream-format interface

a9bb7c8

To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism. Signed-off-by: Cezary Rojewski <[email protected]>

ASoC: Intel Skylake: Switch to new stream-format interface

015312b

To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism. Signed-off-by: Cezary Rojewski <[email protected]>

ASoC: SOF: Intel: Switch to new stream-format interface

47ab5ec

To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism. Signed-off-by: Cezary Rojewski <[email protected]>

ASoC: Intel: avs: Switch to new stream-format interface

177d6b6

To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism. Signed-off-by: Cezary Rojewski <[email protected]>

ALSA: hda: Drop snd_hdac_calc_stream_format()

eb37244

There are no users of the function. Signed-off-by: Cezary Rojewski <[email protected]>

crojewsk-intel requested review from lgirdwood, plbossart, ranj063, kv2019i, dbaluta, bardliao, ujfalusi, vijendarmukunda, kuanhsuncheng and yaochunhung as code owners August 22, 2023 16:32

plbossart reviewed Aug 22, 2023

View reviewed changes

kv2019i requested a review from RanderWang August 23, 2023 08:33

kv2019i mentioned this pull request Aug 23, 2023

[BUG][IPC4] wrong 24bit variant selected #4383

Closed

RanderWang approved these changes Aug 25, 2023

View reviewed changes

ujfalusi reviewed Aug 25, 2023

View reviewed changes

ujfalusi reviewed Aug 29, 2023

View reviewed changes

plbossart closed this Sep 12, 2023

ALSA/ASoC: hda: Address format selection limitations and ambiguity #4539

ALSA/ASoC: hda: Address format selection limitations and ambiguity #4539

Conversation

crojewsk-intel commented Aug 22, 2023

Choose a reason for hiding this comment

crojewsk-intel commented Aug 22, 2023

plbossart commented Aug 22, 2023

RanderWang left a comment

Choose a reason for hiding this comment

ujfalusi left a 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crojewsk-intel commented Aug 25, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart commented Sep 12, 2023