[RFC PATCH 00/17] ALSA/ASoC: hda: Address format selection limitations and ambiguity

[RFC PATCH 00/17] ALSA/ASoC: hda: Address format selection limitations and ambiguity

[Cezary Rojewski]

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.

Note: No testing done on other drivers yet. First results from
snd_hda_intel and snd_soc_skl will be here on Monday. Will trigger SOF
cycle through github PR.


[1]: https://lore.kernel.org/alsa-devel/20190905053302.9262-1-pawel.harlozinski@linux.intel.com/
[2]: https://github.com/alsa-project/alsa-utils/pull/228
[3]: https://github.com/alsa-project/alsa-lib/pull/342

Cezary Rojewski (17):
  ALSA: pcm: Introduce MSBITS subformat interface
  ALSA: pcm: Honor subformat when configuring substream
  ALSA: hda: Honor subformat when querying PCMs
  ASoC: pcm: Honor subformat when configuring runtime
  ALSA: hda: Upgrade stream-format infrastructure
  ALSA: hda: Switch to new stream-format interface
  ALSA: hda/hdmi: Switch to new stream-format interface
  ALSA: hda/ca0132: Switch to new stream-format interface
  ASoC: codecs: hda: Switch to new stream-format interface
  ASoC: codecs: hdac_hda: Switch to new stream-format interface
  ASoC: codecs: hdac_hdmi: Switch to new stream-format interface
  ASoC: Intel Skylake: Switch to new stream-format interface
  ASoC: SOF: Intel: Switch to new stream-format interface
  ASoC: Intel: avs: Switch to new stream-format interface
  ALSA: hda: Drop snd_hdac_calc_stream_format()
  ASoC: Intel: avs: Unhardcode HDAudio BE DAI drivers description
  ASoC: Intel: avs: Kill S24_LE in HDAudio streaming

 include/sound/hda_codec.h         |   5 +-
 include/sound/hdaudio.h           |  12 +--
 include/sound/pcm.h               |   8 ++
 include/sound/pcm_params.h        |   2 +
 include/sound/soc.h               |   1 +
 include/uapi/sound/asound.h       |   5 +-
 sound/core/pcm_lib.c              |  30 ++++++
 sound/core/pcm_misc.c             |  23 +++++
 sound/core/pcm_native.c           |   5 +-
 sound/hda/hdac_device.c           | 153 +++++++++++++++++++++---------
 sound/pci/hda/hda_codec.c         |   2 +
 sound/pci/hda/hda_controller.c    |  10 +-
 sound/pci/hda/patch_ca0132.c      |   3 +-
 sound/pci/hda/patch_hdmi.c        |   6 +-
 sound/soc/codecs/hda-dai.c        |   6 +-
 sound/soc/codecs/hda.c            |   2 +
 sound/soc/codecs/hdac_hda.c       |   8 +-
 sound/soc/codecs/hdac_hdmi.c      |  10 +-
 sound/soc/intel/avs/loader.c      |   4 +-
 sound/soc/intel/avs/path.c        |   2 +-
 sound/soc/intel/avs/pcm.c         |  60 ++++++------
 sound/soc/intel/avs/probes.c      |   3 +-
 sound/soc/intel/avs/topology.c    |   9 +-
 sound/soc/intel/skylake/skl-pcm.c |  11 ++-
 sound/soc/soc-pcm.c               |  17 ++++
 sound/soc/sof/intel/hda-dai-ops.c |   5 +-
 tools/include/uapi/sound/asound.h |   5 +-
 27 files changed, 290 insertions(+), 117 deletions(-)

-- 
2.25.1

[RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Cezary Rojewski]

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 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 <cezary.rojewski@intel.com>
---
 include/sound/pcm.h               |  8 ++++++++
 include/sound/pcm_params.h        |  2 ++
 include/uapi/sound/asound.h       |  5 ++++-
 sound/core/pcm_lib.c              | 30 ++++++++++++++++++++++++++++++
 sound/core/pcm_misc.c             | 23 +++++++++++++++++++++++
 tools/include/uapi/sound/asound.h |  5 ++++-
 6 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 19f564606ac4..f78441ff9d85 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -31,6 +31,7 @@
 struct snd_pcm_hardware {
 	unsigned int info;		/* SNDRV_PCM_INFO_* */
 	u64 formats;			/* SNDRV_PCM_FMTBIT_* */
+	u64 subformats;			/* SNDRV_PCM_SUBFMTBIT_* */
 	unsigned int rates;		/* SNDRV_PCM_RATE_* */
 	unsigned int rate_min;		/* min rate */
 	unsigned int rate_max;		/* max rate */
@@ -219,6 +220,12 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_U20		SNDRV_PCM_FMTBIT_U20_BE
 #endif
 
+#define _SNDRV_PCM_SUBFMTBIT(fmt)	BIT((__force int)SNDRV_PCM_SUBFORMAT_##fmt)
+#define SNDRV_PCM_SUBFMTBIT_STD		_SNDRV_PCM_SUBFMTBIT(STD)
+#define SNDRV_PCM_SUBFMTBIT_MSBITS_20	_SNDRV_PCM_SUBFMTBIT(MSBITS_20)
+#define SNDRV_PCM_SUBFMTBIT_MSBITS_24	_SNDRV_PCM_SUBFMTBIT(MSBITS_24)
+#define SNDRV_PCM_SUBFMTBIT_MSBITS_32	_SNDRV_PCM_SUBFMTBIT(MSBITS_32)
+
 struct snd_pcm_file {
 	struct snd_pcm_substream *substream;
 	int no_compat_mmap;
@@ -1130,6 +1137,7 @@ int snd_pcm_format_physical_width(snd_pcm_format_t format);		/* in bits */
 ssize_t snd_pcm_format_size(snd_pcm_format_t format, size_t samples);
 const unsigned char *snd_pcm_format_silence_64(snd_pcm_format_t format);
 int snd_pcm_format_set_silence(snd_pcm_format_t format, void *buf, unsigned int frames);
+int snd_pcm_subformat_width(snd_pcm_subformat_t subformat);
 
 void snd_pcm_set_ops(struct snd_pcm * pcm, int direction,
 		     const struct snd_pcm_ops *ops);
diff --git a/include/sound/pcm_params.h b/include/sound/pcm_params.h
index ba184f49f7e1..f09ae37ed44d 100644
--- a/include/sound/pcm_params.h
+++ b/include/sound/pcm_params.h
@@ -362,6 +362,8 @@ static inline int params_physical_width(const struct snd_pcm_hw_params *p)
 	return snd_pcm_format_physical_width(params_format(p));
 }
 
+int params_bps(const struct snd_pcm_hw_params *p);
+
 static inline void
 params_set_format(struct snd_pcm_hw_params *p, snd_pcm_format_t fmt)
 {
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index d6864f672630..27db9f3558fd 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -268,7 +268,10 @@ typedef int __bitwise snd_pcm_format_t;
 
 typedef int __bitwise snd_pcm_subformat_t;
 #define	SNDRV_PCM_SUBFORMAT_STD		((__force snd_pcm_subformat_t) 0)
-#define	SNDRV_PCM_SUBFORMAT_LAST	SNDRV_PCM_SUBFORMAT_STD
+#define	SNDRV_PCM_SUBFORMAT_MSBITS_20	((__force snd_pcm_subformat_t) 1)
+#define	SNDRV_PCM_SUBFORMAT_MSBITS_24	((__force snd_pcm_subformat_t) 2)
+#define	SNDRV_PCM_SUBFORMAT_MSBITS_32	((__force snd_pcm_subformat_t) 3)
+#define	SNDRV_PCM_SUBFORMAT_LAST	SNDRV_PCM_SUBFORMAT_MSBITS_32
 
 #define SNDRV_PCM_INFO_MMAP		0x00000001	/* hardware supports mmap */
 #define SNDRV_PCM_INFO_MMAP_VALID	0x00000002	/* period data are valid during transfer */
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 9c121a921b04..4a60632220c2 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1706,6 +1706,36 @@ int snd_pcm_hw_param_last(struct snd_pcm_substream *pcm,
 }
 EXPORT_SYMBOL(snd_pcm_hw_param_last);
 
+/**
+ * params_bps - Get the number of bits per the sample.
+ * @p: hardware parameters
+ *
+ * Return: The number of bits per sample based on the format,
+ * subformat and msbits the specified hw params has.
+ */
+int params_bps(const struct snd_pcm_hw_params *p)
+{
+	snd_pcm_subformat_t subformat = params_subformat(p);
+	snd_pcm_format_t format = params_format(p);
+	int width;
+
+	switch (format) {
+	case SNDRV_PCM_FORMAT_S32_LE:
+	case SNDRV_PCM_FORMAT_U32_LE:
+	case SNDRV_PCM_FORMAT_S32_BE:
+	case SNDRV_PCM_FORMAT_U32_BE:
+		width = snd_pcm_subformat_width(subformat);
+		if (width)
+			return width;
+		if (p->msbits)
+			return (int)p->msbits;
+		fallthrough;
+	default:
+		return snd_pcm_format_width(format);
+	}
+}
+EXPORT_SYMBOL(params_bps);
+
 static int snd_pcm_lib_ioctl_reset(struct snd_pcm_substream *substream,
 				   void *arg)
 {
diff --git a/sound/core/pcm_misc.c b/sound/core/pcm_misc.c
index 5588b6a1ee8b..127bb36fd2e4 100644
--- a/sound/core/pcm_misc.c
+++ b/sound/core/pcm_misc.c
@@ -482,6 +482,29 @@ int snd_pcm_format_set_silence(snd_pcm_format_t format, void *data, unsigned int
 }
 EXPORT_SYMBOL(snd_pcm_format_set_silence);
 
+/**
+ * snd_pcm_subformat_width - return the bit-width of the subformat
+ * @subformat: the subformat to check
+ *
+ * Return: The bit-width of the subformat, or a negative error code
+ * otherwise.
+ */
+int snd_pcm_subformat_width(snd_pcm_subformat_t subformat)
+{
+	switch (subformat) {
+	case SNDRV_PCM_SUBFORMAT_MSBITS_20:
+		return 20;
+	case SNDRV_PCM_SUBFORMAT_MSBITS_24:
+		return 24;
+	case SNDRV_PCM_SUBFORMAT_MSBITS_32:
+		return 32;
+	case SNDRV_PCM_SUBFORMAT_STD:
+	default:
+		return 0;
+	}
+}
+EXPORT_SYMBOL(snd_pcm_subformat_width);
+
 /**
  * snd_pcm_hw_limit_rates - determine rate_min/rate_max fields
  * @hw: the pcm hw instance
diff --git a/tools/include/uapi/sound/asound.h b/tools/include/uapi/sound/asound.h
index 0aa955aa8246..eb45a698767f 100644
--- a/tools/include/uapi/sound/asound.h
+++ b/tools/include/uapi/sound/asound.h
@@ -267,7 +267,10 @@ typedef int __bitwise snd_pcm_format_t;
 
 typedef int __bitwise snd_pcm_subformat_t;
 #define	SNDRV_PCM_SUBFORMAT_STD		((__force snd_pcm_subformat_t) 0)
-#define	SNDRV_PCM_SUBFORMAT_LAST	SNDRV_PCM_SUBFORMAT_STD
+#define	SNDRV_PCM_SUBFORMAT_MSBITS_20	((__force snd_pcm_subformat_t) 1)
+#define	SNDRV_PCM_SUBFORMAT_MSBITS_24	((__force snd_pcm_subformat_t) 2)
+#define	SNDRV_PCM_SUBFORMAT_MSBITS_32	((__force snd_pcm_subformat_t) 3)
+#define	SNDRV_PCM_SUBFORMAT_LAST	SNDRV_PCM_SUBFORMAT_MSBITS_32
 
 #define SNDRV_PCM_INFO_MMAP		0x00000001	/* hardware supports mmap */
 #define SNDRV_PCM_INFO_MMAP_VALID	0x00000002	/* period data are valid during transfer */
-- 
2.25.1

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Takashi Iwai]

On Fri, 11 Aug 2023 18:48:37 +0200,
Cezary Rojewski wrote:
> 
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -1706,6 +1706,36 @@ int snd_pcm_hw_param_last(struct snd_pcm_substream *pcm,
>  }
>  EXPORT_SYMBOL(snd_pcm_hw_param_last);
>  
> +/**
> + * params_bps - Get the number of bits per the sample.
> + * @p: hardware parameters
> + *
> + * Return: The number of bits per sample based on the format,
> + * subformat and msbits the specified hw params has.
> + */
> +int params_bps(const struct snd_pcm_hw_params *p)

It's a too generic name.  Please put the suffix like snd_xxx_.

There are a few params_xxx() in the current tree, but those are rather
macros, and left so just for laziness to convert every piece.  You
don't need to follow that pattern.


thanks,

Takashi

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Cezary Rojewski]

On 2023-08-14 4:35 PM, Takashi Iwai wrote:
> On Fri, 11 Aug 2023 18:48:37 +0200,
> Cezary Rojewski wrote:
>>
>> --- a/sound/core/pcm_lib.c
>> +++ b/sound/core/pcm_lib.c
>> @@ -1706,6 +1706,36 @@ int snd_pcm_hw_param_last(struct snd_pcm_substream *pcm,
>>   }
>>   EXPORT_SYMBOL(snd_pcm_hw_param_last);
>>   
>> +/**
>> + * params_bps - Get the number of bits per the sample.
>> + * @p: hardware parameters
>> + *
>> + * Return: The number of bits per sample based on the format,
>> + * subformat and msbits the specified hw params has.
>> + */
>> +int params_bps(const struct snd_pcm_hw_params *p)
> 
> It's a too generic name.  Please put the suffix like snd_xxx_.
> 
> There are a few params_xxx() in the current tree, but those are rather
> macros, and left so just for laziness to convert every piece.  You
> don't need to follow that pattern.

Up for that! Followed the existing pattern, no magic there. Guess it is 
going to end up as: snd_pcm_hw_params_bps().

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Jaroslav Kysela]

On 11. 08. 23 18:48, Cezary Rojewski wrote:

> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32	_SNDRV_PCM_SUBFMTBIT(MSBITS_32)

What was reason to add 32/32 format ? Subformat STD + msbits == 32 should 
already handle the maximal resolution. Until we do not have 64 bit formats, it 
seems like an useless extension.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Takashi Iwai]

On Tue, 22 Aug 2023 17:03:02 +0200,
Jaroslav Kysela wrote:
> 
> On 11. 08. 23 18:48, Cezary Rojewski wrote:
> 
> > +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32	_SNDRV_PCM_SUBFMTBIT(MSBITS_32)
> 
> What was reason to add 32/32 format ? Subformat STD + msbits == 32
> should already handle the maximal resolution. Until we do not have 64
> bit formats, it seems like an useless extension.

My understanding is to distinguish the cases "we do fully support
32bit" and "we don't care".  But, the end effect is same for both,
user-space would handle 32bit in both cases, so this difference won't
help much, indeed.


thanks,

Takashi

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Jaroslav Kysela]

On 22. 08. 23 17:07, Takashi Iwai wrote:
> On Tue, 22 Aug 2023 17:03:02 +0200,
> Jaroslav Kysela wrote:
>>
>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
>>
>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32	_SNDRV_PCM_SUBFMTBIT(MSBITS_32)
>>
>> What was reason to add 32/32 format ? Subformat STD + msbits == 32
>> should already handle the maximal resolution. Until we do not have 64
>> bit formats, it seems like an useless extension.
> 
> My understanding is to distinguish the cases "we do fully support
> 32bit" and "we don't care".  But, the end effect is same for both,
> user-space would handle 32bit in both cases, so this difference won't
> help much, indeed.

I don't think that we have a "do not care" situation. The applications 
currently expects to use the maximal msbits for STD subformat. The subformat 
should be used only to refine (downgrade) the resolution on the driver / hw 
side on demand. I would just add only necessary API extensions and save one 
bit for now.

						Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Takashi Iwai]

On Tue, 22 Aug 2023 17:29:47 +0200,
Jaroslav Kysela wrote:
> 
> On 22. 08. 23 17:07, Takashi Iwai wrote:
> > On Tue, 22 Aug 2023 17:03:02 +0200,
> > Jaroslav Kysela wrote:
> >> 
> >> On 11. 08. 23 18:48, Cezary Rojewski wrote:
> >> 
> >>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32	_SNDRV_PCM_SUBFMTBIT(MSBITS_32)
> >> 
> >> What was reason to add 32/32 format ? Subformat STD + msbits == 32
> >> should already handle the maximal resolution. Until we do not have 64
> >> bit formats, it seems like an useless extension.
> > 
> > My understanding is to distinguish the cases "we do fully support
> > 32bit" and "we don't care".  But, the end effect is same for both,
> > user-space would handle 32bit in both cases, so this difference won't
> > help much, indeed.
> 
> I don't think that we have a "do not care" situation. The applications
> currently expects to use the maximal msbits for STD subformat. The
> subformat should be used only to refine (downgrade) the resolution on
> the driver / hw side on demand. I would just add only necessary API
> extensions and save one bit for now.

Well, the current behavior (with STD) is to choose whatever 32bit
format the driver supports, and the driver may set a different value
of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
enforce the hw_params.msbits to be 32, otherwise hw_refine would
fail.  So I see a potential difference.


Takashi

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Jaroslav Kysela]

On 22. 08. 23 17:38, Takashi Iwai wrote:
> On Tue, 22 Aug 2023 17:29:47 +0200,
> Jaroslav Kysela wrote:
>>
>> On 22. 08. 23 17:07, Takashi Iwai wrote:
>>> On Tue, 22 Aug 2023 17:03:02 +0200,
>>> Jaroslav Kysela wrote:
>>>>
>>>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
>>>>
>>>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32	_SNDRV_PCM_SUBFMTBIT(MSBITS_32)
>>>>
>>>> What was reason to add 32/32 format ? Subformat STD + msbits == 32
>>>> should already handle the maximal resolution. Until we do not have 64
>>>> bit formats, it seems like an useless extension.
>>>
>>> My understanding is to distinguish the cases "we do fully support
>>> 32bit" and "we don't care".  But, the end effect is same for both,
>>> user-space would handle 32bit in both cases, so this difference won't
>>> help much, indeed.
>>
>> I don't think that we have a "do not care" situation. The applications
>> currently expects to use the maximal msbits for STD subformat. The
>> subformat should be used only to refine (downgrade) the resolution on
>> the driver / hw side on demand. I would just add only necessary API
>> extensions and save one bit for now.
> 
> Well, the current behavior (with STD) is to choose whatever 32bit
> format the driver supports, and the driver may set a different value
> of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
> enforce the hw_params.msbits to be 32, otherwise hw_refine would
> fail.  So I see a potential difference.

I see. But if our target is to create a complete query/set msbits API, we 
should cover all cases also for other formats.

I vote to replace SUBFMTBIT_MSBITS_32 to SUBFMTBIT_MSBITS_MAX as the second 
bit (right after STD). The format hw parameter already defines the maximal 
width. We can add SUBFMTBIT_MSBITS_32 when it's really required. Note that MAX 
should be handled for all cases (not only for S32_LE or so).

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Cezary Rojewski]

On 2023-08-22 9:03 PM, Jaroslav Kysela wrote:
> On 22. 08. 23 17:38, Takashi Iwai wrote:
>> On Tue, 22 Aug 2023 17:29:47 +0200,
>> Jaroslav Kysela wrote:
>>>
>>> On 22. 08. 23 17:07, Takashi Iwai wrote:
>>>> On Tue, 22 Aug 2023 17:03:02 +0200,
>>>> Jaroslav Kysela wrote:
>>>>>
>>>>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
>>>>>
>>>>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32    
>>>>>> _SNDRV_PCM_SUBFMTBIT(MSBITS_32)
>>>>>
>>>>> What was reason to add 32/32 format ? Subformat STD + msbits == 32
>>>>> should already handle the maximal resolution. Until we do not have 64
>>>>> bit formats, it seems like an useless extension.
>>>>
>>>> My understanding is to distinguish the cases "we do fully support
>>>> 32bit" and "we don't care".  But, the end effect is same for both,
>>>> user-space would handle 32bit in both cases, so this difference won't
>>>> help much, indeed.
>>>
>>> I don't think that we have a "do not care" situation. The applications
>>> currently expects to use the maximal msbits for STD subformat. The
>>> subformat should be used only to refine (downgrade) the resolution on
>>> the driver / hw side on demand. I would just add only necessary API
>>> extensions and save one bit for now.
>>
>> Well, the current behavior (with STD) is to choose whatever 32bit
>> format the driver supports, and the driver may set a different value
>> of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
>> enforce the hw_params.msbits to be 32, otherwise hw_refine would
>> fail.  So I see a potential difference.
> 
> I see. But if our target is to create a complete query/set msbits API, 
> we should cover all cases also for other formats.
> 
> I vote to replace SUBFMTBIT_MSBITS_32 to SUBFMTBIT_MSBITS_MAX as the 
> second bit (right after STD). The format hw parameter already defines 
> the maximal width. We can add SUBFMTBIT_MSBITS_32 when it's really 
> required. Note that MAX should be handled for all cases (not only for 
> S32_LE or so).

In my opinion STD already states "max". The word is not explicit either 
- max in the eyes of whom? The driver'? Then the driver may reply: max 
allowed e.g.: 24/32. And that translates to: fallback to STD.


Kind regards,
Czarek

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Jaroslav Kysela]

On 23. 08. 23 10:11, Cezary Rojewski wrote:
> On 2023-08-22 9:03 PM, Jaroslav Kysela wrote:
>> On 22. 08. 23 17:38, Takashi Iwai wrote:
>>> On Tue, 22 Aug 2023 17:29:47 +0200,
>>> Jaroslav Kysela wrote:
>>>>
>>>> On 22. 08. 23 17:07, Takashi Iwai wrote:
>>>>> On Tue, 22 Aug 2023 17:03:02 +0200,
>>>>> Jaroslav Kysela wrote:
>>>>>>
>>>>>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
>>>>>>
>>>>>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32
>>>>>>> _SNDRV_PCM_SUBFMTBIT(MSBITS_32)
>>>>>>
>>>>>> What was reason to add 32/32 format ? Subformat STD + msbits == 32
>>>>>> should already handle the maximal resolution. Until we do not have 64
>>>>>> bit formats, it seems like an useless extension.
>>>>>
>>>>> My understanding is to distinguish the cases "we do fully support
>>>>> 32bit" and "we don't care".  But, the end effect is same for both,
>>>>> user-space would handle 32bit in both cases, so this difference won't
>>>>> help much, indeed.
>>>>
>>>> I don't think that we have a "do not care" situation. The applications
>>>> currently expects to use the maximal msbits for STD subformat. The
>>>> subformat should be used only to refine (downgrade) the resolution on
>>>> the driver / hw side on demand. I would just add only necessary API
>>>> extensions and save one bit for now.
>>>
>>> Well, the current behavior (with STD) is to choose whatever 32bit
>>> format the driver supports, and the driver may set a different value
>>> of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
>>> enforce the hw_params.msbits to be 32, otherwise hw_refine would
>>> fail.  So I see a potential difference.
>>
>> I see. But if our target is to create a complete query/set msbits API,
>> we should cover all cases also for other formats.
>>
>> I vote to replace SUBFMTBIT_MSBITS_32 to SUBFMTBIT_MSBITS_MAX as the
>> second bit (right after STD). The format hw parameter already defines
>> the maximal width. We can add SUBFMTBIT_MSBITS_32 when it's really
>> required. Note that MAX should be handled for all cases (not only for
>> S32_LE or so).
> 
> In my opinion STD already states "max". The word is not explicit either
> - max in the eyes of whom? The driver'? Then the driver may reply: max
> allowed e.g.: 24/32. And that translates to: fallback to STD.

Max in the contents of the physical sample format (S32 = 32 bits, S24 = 24 
bits, S8 = 8 bits etc). It would mean, if the driver supports S32 but only 
with 24-bit resolution, this bit should not be set/allowed. We can also use 
word full or something other. If we like to extend the API in this way (force 
the specific msbits with the error handling), all formats should be covered. 
For STD - see Takashi's reply.

						Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Takashi Iwai]

On Wed, 23 Aug 2023 11:10:38 +0200,
Jaroslav Kysela wrote:
> 
> On 23. 08. 23 10:11, Cezary Rojewski wrote:
> > On 2023-08-22 9:03 PM, Jaroslav Kysela wrote:
> >> On 22. 08. 23 17:38, Takashi Iwai wrote:
> >>> On Tue, 22 Aug 2023 17:29:47 +0200,
> >>> Jaroslav Kysela wrote:
> >>>> 
> >>>> On 22. 08. 23 17:07, Takashi Iwai wrote:
> >>>>> On Tue, 22 Aug 2023 17:03:02 +0200,
> >>>>> Jaroslav Kysela wrote:
> >>>>>> 
> >>>>>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
> >>>>>> 
> >>>>>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32
> >>>>>>> _SNDRV_PCM_SUBFMTBIT(MSBITS_32)
> >>>>>> 
> >>>>>> What was reason to add 32/32 format ? Subformat STD + msbits == 32
> >>>>>> should already handle the maximal resolution. Until we do not have 64
> >>>>>> bit formats, it seems like an useless extension.
> >>>>> 
> >>>>> My understanding is to distinguish the cases "we do fully support
> >>>>> 32bit" and "we don't care".  But, the end effect is same for both,
> >>>>> user-space would handle 32bit in both cases, so this difference won't
> >>>>> help much, indeed.
> >>>> 
> >>>> I don't think that we have a "do not care" situation. The applications
> >>>> currently expects to use the maximal msbits for STD subformat. The
> >>>> subformat should be used only to refine (downgrade) the resolution on
> >>>> the driver / hw side on demand. I would just add only necessary API
> >>>> extensions and save one bit for now.
> >>> 
> >>> Well, the current behavior (with STD) is to choose whatever 32bit
> >>> format the driver supports, and the driver may set a different value
> >>> of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
> >>> enforce the hw_params.msbits to be 32, otherwise hw_refine would
> >>> fail.  So I see a potential difference.
> >> 
> >> I see. But if our target is to create a complete query/set msbits API,
> >> we should cover all cases also for other formats.
> >> 
> >> I vote to replace SUBFMTBIT_MSBITS_32 to SUBFMTBIT_MSBITS_MAX as the
> >> second bit (right after STD). The format hw parameter already defines
> >> the maximal width. We can add SUBFMTBIT_MSBITS_32 when it's really
> >> required. Note that MAX should be handled for all cases (not only for
> >> S32_LE or so).
> > 
> > In my opinion STD already states "max". The word is not explicit either
> > - max in the eyes of whom? The driver'? Then the driver may reply: max
> > allowed e.g.: 24/32. And that translates to: fallback to STD.
> 
> Max in the contents of the physical sample format (S32 = 32 bits, S24
> = 24 bits, S8 = 8 bits etc). It would mean, if the driver supports S32
> but only with 24-bit resolution, this bit should not be
> set/allowed. We can also use word full or something other. If we like
> to extend the API in this way (force the specific msbits with the
> error handling), all formats should be covered. For STD - see
> Takashi's reply.

I think MAX can be problematic when the device supports multiple
formats, say, 16bit and 32bit.  Then it's not clear which MAX points
to: is 16bit max or 32bit max.

I find the subformat extension OK, as this doesn't need much change in
API.  OTOH, if we want to be more consistent way, we may extend
hw_params for a new interval, e.g. SNDRV_PCM_HW_PARAM_MSBITS, and let
the driver choosing it.  This will need more hw_params rules and
become more complex, but it allows drivers really exotic setups (like
19bit PCM :)  But my gut feeling is that the subformat extension
should suffice.


Takashi

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Jaroslav Kysela]

On 23. 08. 23 11:53, Takashi Iwai wrote:
> On Wed, 23 Aug 2023 11:10:38 +0200,
> Jaroslav Kysela wrote:
>>
>> On 23. 08. 23 10:11, Cezary Rojewski wrote:
>>> On 2023-08-22 9:03 PM, Jaroslav Kysela wrote:
>>>> On 22. 08. 23 17:38, Takashi Iwai wrote:
>>>>> On Tue, 22 Aug 2023 17:29:47 +0200,
>>>>> Jaroslav Kysela wrote:
>>>>>>
>>>>>> On 22. 08. 23 17:07, Takashi Iwai wrote:
>>>>>>> On Tue, 22 Aug 2023 17:03:02 +0200,
>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>
>>>>>>>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
>>>>>>>>
>>>>>>>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32
>>>>>>>>> _SNDRV_PCM_SUBFMTBIT(MSBITS_32)
>>>>>>>>
>>>>>>>> What was reason to add 32/32 format ? Subformat STD + msbits == 32
>>>>>>>> should already handle the maximal resolution. Until we do not have 64
>>>>>>>> bit formats, it seems like an useless extension.
>>>>>>>
>>>>>>> My understanding is to distinguish the cases "we do fully support
>>>>>>> 32bit" and "we don't care".  But, the end effect is same for both,
>>>>>>> user-space would handle 32bit in both cases, so this difference won't
>>>>>>> help much, indeed.
>>>>>>
>>>>>> I don't think that we have a "do not care" situation. The applications
>>>>>> currently expects to use the maximal msbits for STD subformat. The
>>>>>> subformat should be used only to refine (downgrade) the resolution on
>>>>>> the driver / hw side on demand. I would just add only necessary API
>>>>>> extensions and save one bit for now.
>>>>>
>>>>> Well, the current behavior (with STD) is to choose whatever 32bit
>>>>> format the driver supports, and the driver may set a different value
>>>>> of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
>>>>> enforce the hw_params.msbits to be 32, otherwise hw_refine would
>>>>> fail.  So I see a potential difference.
>>>>
>>>> I see. But if our target is to create a complete query/set msbits API,
>>>> we should cover all cases also for other formats.
>>>>
>>>> I vote to replace SUBFMTBIT_MSBITS_32 to SUBFMTBIT_MSBITS_MAX as the
>>>> second bit (right after STD). The format hw parameter already defines
>>>> the maximal width. We can add SUBFMTBIT_MSBITS_32 when it's really
>>>> required. Note that MAX should be handled for all cases (not only for
>>>> S32_LE or so).
>>>
>>> In my opinion STD already states "max". The word is not explicit either
>>> - max in the eyes of whom? The driver'? Then the driver may reply: max
>>> allowed e.g.: 24/32. And that translates to: fallback to STD.
>>
>> Max in the contents of the physical sample format (S32 = 32 bits, S24
>> = 24 bits, S8 = 8 bits etc). It would mean, if the driver supports S32
>> but only with 24-bit resolution, this bit should not be
>> set/allowed. We can also use word full or something other. If we like
>> to extend the API in this way (force the specific msbits with the
>> error handling), all formats should be covered. For STD - see
>> Takashi's reply.
> 
> I think MAX can be problematic when the device supports multiple
> formats, say, 16bit and 32bit.  Then it's not clear which MAX points
> to: is 16bit max or 32bit max.

I don't take this point. The subformat depends on the format, thus if one 
format support max, it should be set for queries.

Theoretically, this problem is in this API extension proposal too. Imagine 
that driver/hw support S24 and S32 formats and 20-bit msbits for one of them. 
How do you handle this? The subformat depends on format and should be refined 
when the format is known (single choice).

> I find the subformat extension OK, as this doesn't need much change in
> API.  OTOH, if we want to be more consistent way, we may extend
> hw_params for a new interval, e.g. SNDRV_PCM_HW_PARAM_MSBITS, and let
> the driver choosing it.  This will need more hw_params rules and
> become more complex, but it allows drivers really exotic setups (like
> 19bit PCM :)  But my gut feeling is that the subformat extension
> should suffice.

I'm not ok with 32 == 32. We should handle this case universally or discard.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Amadeusz Sławiński]

On 8/23/2023 12:00 PM, Jaroslav Kysela wrote:
> On 23. 08. 23 11:53, Takashi Iwai wrote:
>> On Wed, 23 Aug 2023 11:10:38 +0200,
>> Jaroslav Kysela wrote:
>>>
>>> On 23. 08. 23 10:11, Cezary Rojewski wrote:
>>>> On 2023-08-22 9:03 PM, Jaroslav Kysela wrote:
>>>>> On 22. 08. 23 17:38, Takashi Iwai wrote:
>>>>>> On Tue, 22 Aug 2023 17:29:47 +0200,
>>>>>> Jaroslav Kysela wrote:
>>>>>>>
>>>>>>> On 22. 08. 23 17:07, Takashi Iwai wrote:
>>>>>>>> On Tue, 22 Aug 2023 17:03:02 +0200,
>>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>>
>>>>>>>>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
>>>>>>>>>
>>>>>>>>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32
>>>>>>>>>> _SNDRV_PCM_SUBFMTBIT(MSBITS_32)
>>>>>>>>>
>>>>>>>>> What was reason to add 32/32 format ? Subformat STD + msbits == 32
>>>>>>>>> should already handle the maximal resolution. Until we do not 
>>>>>>>>> have 64
>>>>>>>>> bit formats, it seems like an useless extension.
>>>>>>>>
>>>>>>>> My understanding is to distinguish the cases "we do fully support
>>>>>>>> 32bit" and "we don't care".  But, the end effect is same for both,
>>>>>>>> user-space would handle 32bit in both cases, so this difference 
>>>>>>>> won't
>>>>>>>> help much, indeed.
>>>>>>>
>>>>>>> I don't think that we have a "do not care" situation. The 
>>>>>>> applications
>>>>>>> currently expects to use the maximal msbits for STD subformat. The
>>>>>>> subformat should be used only to refine (downgrade) the 
>>>>>>> resolution on
>>>>>>> the driver / hw side on demand. I would just add only necessary API
>>>>>>> extensions and save one bit for now.
>>>>>>
>>>>>> Well, the current behavior (with STD) is to choose whatever 32bit
>>>>>> format the driver supports, and the driver may set a different value
>>>>>> of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
>>>>>> enforce the hw_params.msbits to be 32, otherwise hw_refine would
>>>>>> fail.  So I see a potential difference.
>>>>>
>>>>> I see. But if our target is to create a complete query/set msbits API,
>>>>> we should cover all cases also for other formats.
>>>>>
>>>>> I vote to replace SUBFMTBIT_MSBITS_32 to SUBFMTBIT_MSBITS_MAX as the
>>>>> second bit (right after STD). The format hw parameter already defines
>>>>> the maximal width. We can add SUBFMTBIT_MSBITS_32 when it's really
>>>>> required. Note that MAX should be handled for all cases (not only for
>>>>> S32_LE or so).
>>>>
>>>> In my opinion STD already states "max". The word is not explicit either
>>>> - max in the eyes of whom? The driver'? Then the driver may reply: max
>>>> allowed e.g.: 24/32. And that translates to: fallback to STD.
>>>
>>> Max in the contents of the physical sample format (S32 = 32 bits, S24
>>> = 24 bits, S8 = 8 bits etc). It would mean, if the driver supports S32
>>> but only with 24-bit resolution, this bit should not be
>>> set/allowed. We can also use word full or something other. If we like
>>> to extend the API in this way (force the specific msbits with the
>>> error handling), all formats should be covered. For STD - see
>>> Takashi's reply.
>>
>> I think MAX can be problematic when the device supports multiple
>> formats, say, 16bit and 32bit.  Then it's not clear which MAX points
>> to: is 16bit max or 32bit max.
> 
> I don't take this point. The subformat depends on the format, thus if 
> one format support max, it should be set for queries.
> 
> Theoretically, this problem is in this API extension proposal too. 
> Imagine that driver/hw support S24 and S32 formats and 20-bit msbits for 
> one of them. How do you handle this? The subformat depends on format and 
> should be refined when the format is known (single choice).
> 
>> I find the subformat extension OK, as this doesn't need much change in
>> API.  OTOH, if we want to be more consistent way, we may extend
>> hw_params for a new interval, e.g. SNDRV_PCM_HW_PARAM_MSBITS, and let
>> the driver choosing it.  This will need more hw_params rules and
>> become more complex, but it allows drivers really exotic setups (like
>> 19bit PCM :)  But my gut feeling is that the subformat extension
>> should suffice.
> 
> I'm not ok with 32 == 32. We should handle this case universally or 
> discard.
> 
>                      Jaroslav
> 

The reason for MSBITS32 is that, when only MSBITS_20 and MSBITS_24 are 
defined, when userspace (in this case aplay) asks for usable formats and 
subformat it gets something like:

--------------------
ACCESS: MMAP_INTERLEAVED RW_INTERLEAVED
FORMAT: S16_LE S32_LE
SUBFORMAT: STD MSBITS_20 MSBITS_24
SAMPLE_BITS: [16 32]
FRAME_BITS: [32 64]
CHANNELS: 2
RATE: 48000

when MSBITS_32 is not defined it is not clear if HW supports 24 or 32 
bits as maximum possible value of msbits. However when MSBITS_32 is 
defined it is clear - in the above case maximum possible bps then is 24, 
because MSBITS_32 is missing in output.

STD behaves as before and takes maximum possible value - in above case 
it is 24.

Amadeusz

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Jaroslav Kysela]

On 23. 08. 23 12:20, Amadeusz Sławiński wrote:
> On 8/23/2023 12:00 PM, Jaroslav Kysela wrote:
>> On 23. 08. 23 11:53, Takashi Iwai wrote:
>>> On Wed, 23 Aug 2023 11:10:38 +0200,
>>> Jaroslav Kysela wrote:
>>>>
>>>> On 23. 08. 23 10:11, Cezary Rojewski wrote:
>>>>> On 2023-08-22 9:03 PM, Jaroslav Kysela wrote:
>>>>>> On 22. 08. 23 17:38, Takashi Iwai wrote:
>>>>>>> On Tue, 22 Aug 2023 17:29:47 +0200,
>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>
>>>>>>>> On 22. 08. 23 17:07, Takashi Iwai wrote:
>>>>>>>>> On Tue, 22 Aug 2023 17:03:02 +0200,
>>>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>>>
>>>>>>>>>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
>>>>>>>>>>
>>>>>>>>>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32
>>>>>>>>>>> _SNDRV_PCM_SUBFMTBIT(MSBITS_32)
>>>>>>>>>>
>>>>>>>>>> What was reason to add 32/32 format ? Subformat STD + msbits == 32
>>>>>>>>>> should already handle the maximal resolution. Until we do not
>>>>>>>>>> have 64
>>>>>>>>>> bit formats, it seems like an useless extension.
>>>>>>>>>
>>>>>>>>> My understanding is to distinguish the cases "we do fully support
>>>>>>>>> 32bit" and "we don't care".  But, the end effect is same for both,
>>>>>>>>> user-space would handle 32bit in both cases, so this difference
>>>>>>>>> won't
>>>>>>>>> help much, indeed.
>>>>>>>>
>>>>>>>> I don't think that we have a "do not care" situation. The
>>>>>>>> applications
>>>>>>>> currently expects to use the maximal msbits for STD subformat. The
>>>>>>>> subformat should be used only to refine (downgrade) the
>>>>>>>> resolution on
>>>>>>>> the driver / hw side on demand. I would just add only necessary API
>>>>>>>> extensions and save one bit for now.
>>>>>>>
>>>>>>> Well, the current behavior (with STD) is to choose whatever 32bit
>>>>>>> format the driver supports, and the driver may set a different value
>>>>>>> of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
>>>>>>> enforce the hw_params.msbits to be 32, otherwise hw_refine would
>>>>>>> fail.  So I see a potential difference.
>>>>>>
>>>>>> I see. But if our target is to create a complete query/set msbits API,
>>>>>> we should cover all cases also for other formats.
>>>>>>
>>>>>> I vote to replace SUBFMTBIT_MSBITS_32 to SUBFMTBIT_MSBITS_MAX as the
>>>>>> second bit (right after STD). The format hw parameter already defines
>>>>>> the maximal width. We can add SUBFMTBIT_MSBITS_32 when it's really
>>>>>> required. Note that MAX should be handled for all cases (not only for
>>>>>> S32_LE or so).
>>>>>
>>>>> In my opinion STD already states "max". The word is not explicit either
>>>>> - max in the eyes of whom? The driver'? Then the driver may reply: max
>>>>> allowed e.g.: 24/32. And that translates to: fallback to STD.
>>>>
>>>> Max in the contents of the physical sample format (S32 = 32 bits, S24
>>>> = 24 bits, S8 = 8 bits etc). It would mean, if the driver supports S32
>>>> but only with 24-bit resolution, this bit should not be
>>>> set/allowed. We can also use word full or something other. If we like
>>>> to extend the API in this way (force the specific msbits with the
>>>> error handling), all formats should be covered. For STD - see
>>>> Takashi's reply.
>>>
>>> I think MAX can be problematic when the device supports multiple
>>> formats, say, 16bit and 32bit.  Then it's not clear which MAX points
>>> to: is 16bit max or 32bit max.
>>
>> I don't take this point. The subformat depends on the format, thus if
>> one format support max, it should be set for queries.
>>
>> Theoretically, this problem is in this API extension proposal too.
>> Imagine that driver/hw support S24 and S32 formats and 20-bit msbits for
>> one of them. How do you handle this? The subformat depends on format and
>> should be refined when the format is known (single choice).
>>
>>> I find the subformat extension OK, as this doesn't need much change in
>>> API.  OTOH, if we want to be more consistent way, we may extend
>>> hw_params for a new interval, e.g. SNDRV_PCM_HW_PARAM_MSBITS, and let
>>> the driver choosing it.  This will need more hw_params rules and
>>> become more complex, but it allows drivers really exotic setups (like
>>> 19bit PCM :)  But my gut feeling is that the subformat extension
>>> should suffice.
>>
>> I'm not ok with 32 == 32. We should handle this case universally or
>> discard.
>>
>>                       Jaroslav
>>
> 
> The reason for MSBITS32 is that, when only MSBITS_20 and MSBITS_24 are
> defined, when userspace (in this case aplay) asks for usable formats and
> subformat it gets something like:
> 
> --------------------
> ACCESS: MMAP_INTERLEAVED RW_INTERLEAVED
> FORMAT: S16_LE S32_LE
> SUBFORMAT: STD MSBITS_20 MSBITS_24
> SAMPLE_BITS: [16 32]
> FRAME_BITS: [32 64]
> CHANNELS: 2
> RATE: 48000
> 
> when MSBITS_32 is not defined it is not clear if HW supports 24 or 32
> bits as maximum possible value of msbits. However when MSBITS_32 is
> defined it is clear - in the above case maximum possible bps then is 24,
> because MSBITS_32 is missing in output.
> 
> STD behaves as before and takes maximum possible value - in above case
> it is 24.

This example is nice for S32_LE but not S16_LE. With the max/full bit, we can 
already cover also S16_LE (so that the application can ask for the maximal 
msbits which fits to the physical format for S16_LE). It would be also a 
preparation for future, when we need to deal with more msbits combos (like 
14bit or 15bit samples stored in the bigger physical words) etc.

So application can set those parameters for in your case:

S16_LE + STD         (maximum bits handled by driver - 16 in this case)
S16_LE + MSBITS_MAX  (maximum physical bits for the format - 16)
S32_LE + STD         (maximum bits handled by driver - 24 in this case)
S32_LE + MSBITS_MAX  (maximum physical bits for the format - 32)
S32_LE + MSBITS_24
S32_LE + MSBITS_20

Dtto for other format like S8, S24 etc. Another way is to define MSBITS_8, 
MSBITS_16 etc. But I'd prefer to save subformat bits. The MSBITS_MAX would 
cover almost all cases for now.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Takashi Iwai]

On Wed, 23 Aug 2023 12:47:33 +0200,
Jaroslav Kysela wrote:
> 
> On 23. 08. 23 12:20, Amadeusz Sławiński wrote:
> > On 8/23/2023 12:00 PM, Jaroslav Kysela wrote:
> >> On 23. 08. 23 11:53, Takashi Iwai wrote:
> >>> On Wed, 23 Aug 2023 11:10:38 +0200,
> >>> Jaroslav Kysela wrote:
> >>>> 
> >>>> On 23. 08. 23 10:11, Cezary Rojewski wrote:
> >>>>> On 2023-08-22 9:03 PM, Jaroslav Kysela wrote:
> >>>>>> On 22. 08. 23 17:38, Takashi Iwai wrote:
> >>>>>>> On Tue, 22 Aug 2023 17:29:47 +0200,
> >>>>>>> Jaroslav Kysela wrote:
> >>>>>>>> 
> >>>>>>>> On 22. 08. 23 17:07, Takashi Iwai wrote:
> >>>>>>>>> On Tue, 22 Aug 2023 17:03:02 +0200,
> >>>>>>>>> Jaroslav Kysela wrote:
> >>>>>>>>>> 
> >>>>>>>>>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
> >>>>>>>>>> 
> >>>>>>>>>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32
> >>>>>>>>>>> _SNDRV_PCM_SUBFMTBIT(MSBITS_32)
> >>>>>>>>>> 
> >>>>>>>>>> What was reason to add 32/32 format ? Subformat STD + msbits == 32
> >>>>>>>>>> should already handle the maximal resolution. Until we do not
> >>>>>>>>>> have 64
> >>>>>>>>>> bit formats, it seems like an useless extension.
> >>>>>>>>> 
> >>>>>>>>> My understanding is to distinguish the cases "we do fully support
> >>>>>>>>> 32bit" and "we don't care".  But, the end effect is same for both,
> >>>>>>>>> user-space would handle 32bit in both cases, so this difference
> >>>>>>>>> won't
> >>>>>>>>> help much, indeed.
> >>>>>>>> 
> >>>>>>>> I don't think that we have a "do not care" situation. The
> >>>>>>>> applications
> >>>>>>>> currently expects to use the maximal msbits for STD subformat. The
> >>>>>>>> subformat should be used only to refine (downgrade) the
> >>>>>>>> resolution on
> >>>>>>>> the driver / hw side on demand. I would just add only necessary API
> >>>>>>>> extensions and save one bit for now.
> >>>>>>> 
> >>>>>>> Well, the current behavior (with STD) is to choose whatever 32bit
> >>>>>>> format the driver supports, and the driver may set a different value
> >>>>>>> of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
> >>>>>>> enforce the hw_params.msbits to be 32, otherwise hw_refine would
> >>>>>>> fail.  So I see a potential difference.
> >>>>>> 
> >>>>>> I see. But if our target is to create a complete query/set msbits API,
> >>>>>> we should cover all cases also for other formats.
> >>>>>> 
> >>>>>> I vote to replace SUBFMTBIT_MSBITS_32 to SUBFMTBIT_MSBITS_MAX as the
> >>>>>> second bit (right after STD). The format hw parameter already defines
> >>>>>> the maximal width. We can add SUBFMTBIT_MSBITS_32 when it's really
> >>>>>> required. Note that MAX should be handled for all cases (not only for
> >>>>>> S32_LE or so).
> >>>>> 
> >>>>> In my opinion STD already states "max". The word is not explicit either
> >>>>> - max in the eyes of whom? The driver'? Then the driver may reply: max
> >>>>> allowed e.g.: 24/32. And that translates to: fallback to STD.
> >>>> 
> >>>> Max in the contents of the physical sample format (S32 = 32 bits, S24
> >>>> = 24 bits, S8 = 8 bits etc). It would mean, if the driver supports S32
> >>>> but only with 24-bit resolution, this bit should not be
> >>>> set/allowed. We can also use word full or something other. If we like
> >>>> to extend the API in this way (force the specific msbits with the
> >>>> error handling), all formats should be covered. For STD - see
> >>>> Takashi's reply.
> >>> 
> >>> I think MAX can be problematic when the device supports multiple
> >>> formats, say, 16bit and 32bit.  Then it's not clear which MAX points
> >>> to: is 16bit max or 32bit max.
> >> 
> >> I don't take this point. The subformat depends on the format, thus if
> >> one format support max, it should be set for queries.
> >> 
> >> Theoretically, this problem is in this API extension proposal too.
> >> Imagine that driver/hw support S24 and S32 formats and 20-bit msbits for
> >> one of them. How do you handle this? The subformat depends on format and
> >> should be refined when the format is known (single choice).
> >> 
> >>> I find the subformat extension OK, as this doesn't need much change in
> >>> API.  OTOH, if we want to be more consistent way, we may extend
> >>> hw_params for a new interval, e.g. SNDRV_PCM_HW_PARAM_MSBITS, and let
> >>> the driver choosing it.  This will need more hw_params rules and
> >>> become more complex, but it allows drivers really exotic setups (like
> >>> 19bit PCM :)  But my gut feeling is that the subformat extension
> >>> should suffice.
> >> 
> >> I'm not ok with 32 == 32. We should handle this case universally or
> >> discard.
> >> 
> >>                       Jaroslav
> >> 
> > 
> > The reason for MSBITS32 is that, when only MSBITS_20 and MSBITS_24 are
> > defined, when userspace (in this case aplay) asks for usable formats and
> > subformat it gets something like:
> > 
> > --------------------
> > ACCESS: MMAP_INTERLEAVED RW_INTERLEAVED
> > FORMAT: S16_LE S32_LE
> > SUBFORMAT: STD MSBITS_20 MSBITS_24
> > SAMPLE_BITS: [16 32]
> > FRAME_BITS: [32 64]
> > CHANNELS: 2
> > RATE: 48000
> > 
> > when MSBITS_32 is not defined it is not clear if HW supports 24 or 32
> > bits as maximum possible value of msbits. However when MSBITS_32 is
> > defined it is clear - in the above case maximum possible bps then is 24,
> > because MSBITS_32 is missing in output.
> > 
> > STD behaves as before and takes maximum possible value - in above case
> > it is 24.
> 
> This example is nice for S32_LE but not S16_LE. With the max/full bit,
> we can already cover also S16_LE (so that the application can ask for
> the maximal msbits which fits to the physical format for S16_LE). It
> would be also a preparation for future, when we need to deal with more
> msbits combos (like 14bit or 15bit samples stored in the bigger
> physical words) etc.
> 
> So application can set those parameters for in your case:
> 
> S16_LE + STD         (maximum bits handled by driver - 16 in this case)
> S16_LE + MSBITS_MAX  (maximum physical bits for the format - 16)
> S32_LE + STD         (maximum bits handled by driver - 24 in this case)
> S32_LE + MSBITS_MAX  (maximum physical bits for the format - 32)
> S32_LE + MSBITS_24
> S32_LE + MSBITS_20
> 
> Dtto for other format like S8, S24 etc. Another way is to define
> MSBITS_8, MSBITS_16 etc. But I'd prefer to save subformat bits. The
> MSBITS_MAX would cover almost all cases for now.

It becomes a bit tricky if we have a device that has 24bit bps on
32bit format and 16bit bps / 16bit format.  Both formats and
subformats are bitmaps in hw_params, and initially formats bitmap is
16|24 and subformats bitmap is MAX|24.

Now, suppose that app determines to use 16bit.  Then we need to we
need to update subformats bitmap to MAX by dropping 24.  OTOH, if app
chooses 32bit format, subformats will be 24 by dropping MAX, as we
don't support 32 bps.  And, it's not so trivial to achieve this
commonly only with the single subformats bitmap of snd_pcm_hardware,
as the meaning of MAX depends on the chosen format.

Meanwhile, if the subformats bitmap is with explicit bit flags,
i.e. 24|16, we can reduce the bitmap easily depending on the format.

But, OTOH, having all subformat bits is cumbersome, as you mentioned.
Hmm...


Takashi

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Jaroslav Kysela]

On 23. 08. 23 13:08, Takashi Iwai wrote:
> On Wed, 23 Aug 2023 12:47:33 +0200,
> Jaroslav Kysela wrote:
>>
>> On 23. 08. 23 12:20, Amadeusz Sławiński wrote:
>>> On 8/23/2023 12:00 PM, Jaroslav Kysela wrote:
>>>> On 23. 08. 23 11:53, Takashi Iwai wrote:
>>>>> On Wed, 23 Aug 2023 11:10:38 +0200,
>>>>> Jaroslav Kysela wrote:
>>>>>>
>>>>>> On 23. 08. 23 10:11, Cezary Rojewski wrote:
>>>>>>> On 2023-08-22 9:03 PM, Jaroslav Kysela wrote:
>>>>>>>> On 22. 08. 23 17:38, Takashi Iwai wrote:
>>>>>>>>> On Tue, 22 Aug 2023 17:29:47 +0200,
>>>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>>>
>>>>>>>>>> On 22. 08. 23 17:07, Takashi Iwai wrote:
>>>>>>>>>>> On Tue, 22 Aug 2023 17:03:02 +0200,
>>>>>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32
>>>>>>>>>>>>> _SNDRV_PCM_SUBFMTBIT(MSBITS_32)
>>>>>>>>>>>>
>>>>>>>>>>>> What was reason to add 32/32 format ? Subformat STD + msbits == 32
>>>>>>>>>>>> should already handle the maximal resolution. Until we do not
>>>>>>>>>>>> have 64
>>>>>>>>>>>> bit formats, it seems like an useless extension.
>>>>>>>>>>>
>>>>>>>>>>> My understanding is to distinguish the cases "we do fully support
>>>>>>>>>>> 32bit" and "we don't care".  But, the end effect is same for both,
>>>>>>>>>>> user-space would handle 32bit in both cases, so this difference
>>>>>>>>>>> won't
>>>>>>>>>>> help much, indeed.
>>>>>>>>>>
>>>>>>>>>> I don't think that we have a "do not care" situation. The
>>>>>>>>>> applications
>>>>>>>>>> currently expects to use the maximal msbits for STD subformat. The
>>>>>>>>>> subformat should be used only to refine (downgrade) the
>>>>>>>>>> resolution on
>>>>>>>>>> the driver / hw side on demand. I would just add only necessary API
>>>>>>>>>> extensions and save one bit for now.
>>>>>>>>>
>>>>>>>>> Well, the current behavior (with STD) is to choose whatever 32bit
>>>>>>>>> format the driver supports, and the driver may set a different value
>>>>>>>>> of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
>>>>>>>>> enforce the hw_params.msbits to be 32, otherwise hw_refine would
>>>>>>>>> fail.  So I see a potential difference.
>>>>>>>>
>>>>>>>> I see. But if our target is to create a complete query/set msbits API,
>>>>>>>> we should cover all cases also for other formats.
>>>>>>>>
>>>>>>>> I vote to replace SUBFMTBIT_MSBITS_32 to SUBFMTBIT_MSBITS_MAX as the
>>>>>>>> second bit (right after STD). The format hw parameter already defines
>>>>>>>> the maximal width. We can add SUBFMTBIT_MSBITS_32 when it's really
>>>>>>>> required. Note that MAX should be handled for all cases (not only for
>>>>>>>> S32_LE or so).
>>>>>>>
>>>>>>> In my opinion STD already states "max". The word is not explicit either
>>>>>>> - max in the eyes of whom? The driver'? Then the driver may reply: max
>>>>>>> allowed e.g.: 24/32. And that translates to: fallback to STD.
>>>>>>
>>>>>> Max in the contents of the physical sample format (S32 = 32 bits, S24
>>>>>> = 24 bits, S8 = 8 bits etc). It would mean, if the driver supports S32
>>>>>> but only with 24-bit resolution, this bit should not be
>>>>>> set/allowed. We can also use word full or something other. If we like
>>>>>> to extend the API in this way (force the specific msbits with the
>>>>>> error handling), all formats should be covered. For STD - see
>>>>>> Takashi's reply.
>>>>>
>>>>> I think MAX can be problematic when the device supports multiple
>>>>> formats, say, 16bit and 32bit.  Then it's not clear which MAX points
>>>>> to: is 16bit max or 32bit max.
>>>>
>>>> I don't take this point. The subformat depends on the format, thus if
>>>> one format support max, it should be set for queries.
>>>>
>>>> Theoretically, this problem is in this API extension proposal too.
>>>> Imagine that driver/hw support S24 and S32 formats and 20-bit msbits for
>>>> one of them. How do you handle this? The subformat depends on format and
>>>> should be refined when the format is known (single choice).
>>>>
>>>>> I find the subformat extension OK, as this doesn't need much change in
>>>>> API.  OTOH, if we want to be more consistent way, we may extend
>>>>> hw_params for a new interval, e.g. SNDRV_PCM_HW_PARAM_MSBITS, and let
>>>>> the driver choosing it.  This will need more hw_params rules and
>>>>> become more complex, but it allows drivers really exotic setups (like
>>>>> 19bit PCM :)  But my gut feeling is that the subformat extension
>>>>> should suffice.
>>>>
>>>> I'm not ok with 32 == 32. We should handle this case universally or
>>>> discard.
>>>>
>>>>                        Jaroslav
>>>>
>>>
>>> The reason for MSBITS32 is that, when only MSBITS_20 and MSBITS_24 are
>>> defined, when userspace (in this case aplay) asks for usable formats and
>>> subformat it gets something like:
>>>
>>> --------------------
>>> ACCESS: MMAP_INTERLEAVED RW_INTERLEAVED
>>> FORMAT: S16_LE S32_LE
>>> SUBFORMAT: STD MSBITS_20 MSBITS_24
>>> SAMPLE_BITS: [16 32]
>>> FRAME_BITS: [32 64]
>>> CHANNELS: 2
>>> RATE: 48000
>>>
>>> when MSBITS_32 is not defined it is not clear if HW supports 24 or 32
>>> bits as maximum possible value of msbits. However when MSBITS_32 is
>>> defined it is clear - in the above case maximum possible bps then is 24,
>>> because MSBITS_32 is missing in output.
>>>
>>> STD behaves as before and takes maximum possible value - in above case
>>> it is 24.
>>
>> This example is nice for S32_LE but not S16_LE. With the max/full bit,
>> we can already cover also S16_LE (so that the application can ask for
>> the maximal msbits which fits to the physical format for S16_LE). It
>> would be also a preparation for future, when we need to deal with more
>> msbits combos (like 14bit or 15bit samples stored in the bigger
>> physical words) etc.
>>
>> So application can set those parameters for in your case:
>>
>> S16_LE + STD         (maximum bits handled by driver - 16 in this case)
>> S16_LE + MSBITS_MAX  (maximum physical bits for the format - 16)
>> S32_LE + STD         (maximum bits handled by driver - 24 in this case)
>> S32_LE + MSBITS_MAX  (maximum physical bits for the format - 32)
>> S32_LE + MSBITS_24
>> S32_LE + MSBITS_20
>>
>> Dtto for other format like S8, S24 etc. Another way is to define
>> MSBITS_8, MSBITS_16 etc. But I'd prefer to save subformat bits. The
>> MSBITS_MAX would cover almost all cases for now.
> 
> It becomes a bit tricky if we have a device that has 24bit bps on
> 32bit format and 16bit bps / 16bit format.  Both formats and
> subformats are bitmaps in hw_params, and initially formats bitmap is
> 16|24 and subformats bitmap is MAX|24.
> 
> Now, suppose that app determines to use 16bit.  Then we need to we
> need to update subformats bitmap to MAX by dropping 24.  OTOH, if app
> chooses 32bit format, subformats will be 24 by dropping MAX, as we
> don't support 32 bps.  And, it's not so trivial to achieve this
> commonly only with the single subformats bitmap of snd_pcm_hardware,
> as the meaning of MAX depends on the chosen format.

It's easy to implement using a code which will go through all set format bits 
and do bitwise OR for all corresponding subformat bits. We can use a callback 
which the driver may override.

Something like (for the above HDA example): - SUBFMTBIT_STD is required so it 
would be handled at the upper level):

snd_pcm_subformat_t (*get_subformat)(struct snd_pcm_substream *substream, 
snd_pcm_format_t format)
{
         switch (format) {
	case SNDRV_PCM_FMTBIT_S16_LE:
		return SNDRV_PCM_SUBFMTBIT_MSBITS_MAX;
         case SNDRV_PCM_FMTBIT_S32_LE:
                 return SNDRV_PCM_SUBFMTBIT_MSBITS_24 |
                        SNDRV_PCM_SUBFMTBIT_MSBITS_20;
	default:
		return 0;
	}
}

SUBFMTBIT_STD is required so it would be handled at the upper level.

> Meanwhile, if the subformats bitmap is with explicit bit flags,
> i.e. 24|16, we can reduce the bitmap easily depending on the format.

I don't think that the current proposed code does any reduction. It will 
return MSBITS_24 when only S16_LE format is selected, too. The refining 
mechanism is ignored. We need to handle subformat bits differently than format 
bits because the dependency.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Amadeusz Sławiński]

On 8/23/2023 3:42 PM, Jaroslav Kysela wrote:
> On 23. 08. 23 13:08, Takashi Iwai wrote:
>> On Wed, 23 Aug 2023 12:47:33 +0200,
>> Jaroslav Kysela wrote:
>>>
>>> On 23. 08. 23 12:20, Amadeusz Sławiński wrote:
>>>> On 8/23/2023 12:00 PM, Jaroslav Kysela wrote:
>>>>> On 23. 08. 23 11:53, Takashi Iwai wrote:
>>>>>> On Wed, 23 Aug 2023 11:10:38 +0200,
>>>>>> Jaroslav Kysela wrote:
>>>>>>>
>>>>>>> On 23. 08. 23 10:11, Cezary Rojewski wrote:
>>>>>>>> On 2023-08-22 9:03 PM, Jaroslav Kysela wrote:
>>>>>>>>> On 22. 08. 23 17:38, Takashi Iwai wrote:
>>>>>>>>>> On Tue, 22 Aug 2023 17:29:47 +0200,
>>>>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 22. 08. 23 17:07, Takashi Iwai wrote:
>>>>>>>>>>>> On Tue, 22 Aug 2023 17:03:02 +0200,
>>>>>>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32
>>>>>>>>>>>>>> _SNDRV_PCM_SUBFMTBIT(MSBITS_32)
>>>>>>>>>>>>>
>>>>>>>>>>>>> What was reason to add 32/32 format ? Subformat STD + 
>>>>>>>>>>>>> msbits == 32
>>>>>>>>>>>>> should already handle the maximal resolution. Until we do not
>>>>>>>>>>>>> have 64
>>>>>>>>>>>>> bit formats, it seems like an useless extension.
>>>>>>>>>>>>
>>>>>>>>>>>> My understanding is to distinguish the cases "we do fully 
>>>>>>>>>>>> support
>>>>>>>>>>>> 32bit" and "we don't care".  But, the end effect is same for 
>>>>>>>>>>>> both,
>>>>>>>>>>>> user-space would handle 32bit in both cases, so this difference
>>>>>>>>>>>> won't
>>>>>>>>>>>> help much, indeed.
>>>>>>>>>>>
>>>>>>>>>>> I don't think that we have a "do not care" situation. The
>>>>>>>>>>> applications
>>>>>>>>>>> currently expects to use the maximal msbits for STD 
>>>>>>>>>>> subformat. The
>>>>>>>>>>> subformat should be used only to refine (downgrade) the
>>>>>>>>>>> resolution on
>>>>>>>>>>> the driver / hw side on demand. I would just add only 
>>>>>>>>>>> necessary API
>>>>>>>>>>> extensions and save one bit for now.
>>>>>>>>>>
>>>>>>>>>> Well, the current behavior (with STD) is to choose whatever 32bit
>>>>>>>>>> format the driver supports, and the driver may set a different 
>>>>>>>>>> value
>>>>>>>>>> of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
>>>>>>>>>> enforce the hw_params.msbits to be 32, otherwise hw_refine would
>>>>>>>>>> fail.  So I see a potential difference.
>>>>>>>>>
>>>>>>>>> I see. But if our target is to create a complete query/set 
>>>>>>>>> msbits API,
>>>>>>>>> we should cover all cases also for other formats.
>>>>>>>>>
>>>>>>>>> I vote to replace SUBFMTBIT_MSBITS_32 to SUBFMTBIT_MSBITS_MAX 
>>>>>>>>> as the
>>>>>>>>> second bit (right after STD). The format hw parameter already 
>>>>>>>>> defines
>>>>>>>>> the maximal width. We can add SUBFMTBIT_MSBITS_32 when it's really
>>>>>>>>> required. Note that MAX should be handled for all cases (not 
>>>>>>>>> only for
>>>>>>>>> S32_LE or so).
>>>>>>>>
>>>>>>>> In my opinion STD already states "max". The word is not explicit 
>>>>>>>> either
>>>>>>>> - max in the eyes of whom? The driver'? Then the driver may 
>>>>>>>> reply: max
>>>>>>>> allowed e.g.: 24/32. And that translates to: fallback to STD.
>>>>>>>
>>>>>>> Max in the contents of the physical sample format (S32 = 32 bits, 
>>>>>>> S24
>>>>>>> = 24 bits, S8 = 8 bits etc). It would mean, if the driver 
>>>>>>> supports S32
>>>>>>> but only with 24-bit resolution, this bit should not be
>>>>>>> set/allowed. We can also use word full or something other. If we 
>>>>>>> like
>>>>>>> to extend the API in this way (force the specific msbits with the
>>>>>>> error handling), all formats should be covered. For STD - see
>>>>>>> Takashi's reply.
>>>>>>
>>>>>> I think MAX can be problematic when the device supports multiple
>>>>>> formats, say, 16bit and 32bit.  Then it's not clear which MAX points
>>>>>> to: is 16bit max or 32bit max.
>>>>>
>>>>> I don't take this point. The subformat depends on the format, thus if
>>>>> one format support max, it should be set for queries.
>>>>>
>>>>> Theoretically, this problem is in this API extension proposal too.
>>>>> Imagine that driver/hw support S24 and S32 formats and 20-bit 
>>>>> msbits for
>>>>> one of them. How do you handle this? The subformat depends on 
>>>>> format and
>>>>> should be refined when the format is known (single choice).
>>>>>
>>>>>> I find the subformat extension OK, as this doesn't need much 
>>>>>> change in
>>>>>> API.  OTOH, if we want to be more consistent way, we may extend
>>>>>> hw_params for a new interval, e.g. SNDRV_PCM_HW_PARAM_MSBITS, and let
>>>>>> the driver choosing it.  This will need more hw_params rules and
>>>>>> become more complex, but it allows drivers really exotic setups (like
>>>>>> 19bit PCM :)  But my gut feeling is that the subformat extension
>>>>>> should suffice.
>>>>>
>>>>> I'm not ok with 32 == 32. We should handle this case universally or
>>>>> discard.
>>>>>
>>>>>                        Jaroslav
>>>>>
>>>>
>>>> The reason for MSBITS32 is that, when only MSBITS_20 and MSBITS_24 are
>>>> defined, when userspace (in this case aplay) asks for usable formats 
>>>> and
>>>> subformat it gets something like:
>>>>
>>>> --------------------
>>>> ACCESS: MMAP_INTERLEAVED RW_INTERLEAVED
>>>> FORMAT: S16_LE S32_LE
>>>> SUBFORMAT: STD MSBITS_20 MSBITS_24
>>>> SAMPLE_BITS: [16 32]
>>>> FRAME_BITS: [32 64]
>>>> CHANNELS: 2
>>>> RATE: 48000
>>>>
>>>> when MSBITS_32 is not defined it is not clear if HW supports 24 or 32
>>>> bits as maximum possible value of msbits. However when MSBITS_32 is
>>>> defined it is clear - in the above case maximum possible bps then is 
>>>> 24,
>>>> because MSBITS_32 is missing in output.
>>>>
>>>> STD behaves as before and takes maximum possible value - in above case
>>>> it is 24.
>>>
>>> This example is nice for S32_LE but not S16_LE. With the max/full bit,
>>> we can already cover also S16_LE (so that the application can ask for
>>> the maximal msbits which fits to the physical format for S16_LE). It
>>> would be also a preparation for future, when we need to deal with more
>>> msbits combos (like 14bit or 15bit samples stored in the bigger
>>> physical words) etc.
>>>
>>> So application can set those parameters for in your case:
>>>
>>> S16_LE + STD         (maximum bits handled by driver - 16 in this case)
>>> S16_LE + MSBITS_MAX  (maximum physical bits for the format - 16)
>>> S32_LE + STD         (maximum bits handled by driver - 24 in this case)
>>> S32_LE + MSBITS_MAX  (maximum physical bits for the format - 32)
>>> S32_LE + MSBITS_24
>>> S32_LE + MSBITS_20
>>>
>>> Dtto for other format like S8, S24 etc. Another way is to define
>>> MSBITS_8, MSBITS_16 etc. But I'd prefer to save subformat bits. The
>>> MSBITS_MAX would cover almost all cases for now.
>>
>> It becomes a bit tricky if we have a device that has 24bit bps on
>> 32bit format and 16bit bps / 16bit format.  Both formats and
>> subformats are bitmaps in hw_params, and initially formats bitmap is
>> 16|24 and subformats bitmap is MAX|24.
>>
>> Now, suppose that app determines to use 16bit.  Then we need to we
>> need to update subformats bitmap to MAX by dropping 24.  OTOH, if app
>> chooses 32bit format, subformats will be 24 by dropping MAX, as we
>> don't support 32 bps.  And, it's not so trivial to achieve this
>> commonly only with the single subformats bitmap of snd_pcm_hardware,
>> as the meaning of MAX depends on the chosen format.
> 
> It's easy to implement using a code which will go through all set format 
> bits and do bitwise OR for all corresponding subformat bits. We can use 
> a callback which the driver may override.
> 
> Something like (for the above HDA example): - SUBFMTBIT_STD is required 
> so it would be handled at the upper level):
> 
> snd_pcm_subformat_t (*get_subformat)(struct snd_pcm_substream 
> *substream, snd_pcm_format_t format)
> {
>          switch (format) {
>      case SNDRV_PCM_FMTBIT_S16_LE:
>          return SNDRV_PCM_SUBFMTBIT_MSBITS_MAX;
>          case SNDRV_PCM_FMTBIT_S32_LE:
>                  return SNDRV_PCM_SUBFMTBIT_MSBITS_24 |
>                         SNDRV_PCM_SUBFMTBIT_MSBITS_20;
>      default:
>          return 0;
>      }
> }
> 
> SUBFMTBIT_STD is required so it would be handled at the upper level.
> 
>> Meanwhile, if the subformats bitmap is with explicit bit flags,
>> i.e. 24|16, we can reduce the bitmap easily depending on the format.
> 
> I don't think that the current proposed code does any reduction. It will 
> return MSBITS_24 when only S16_LE format is selected, too. The refining 
> mechanism is ignored. We need to handle subformat bits differently than 
> format bits because the dependency.
> 
>                      Jaroslav
> 

Problem with MSBITS_MAX is that if kernel reports something like this:

FORMAT: S16_LE S32_LE
SUBFORMAT: STD MSBITS_20 MSBITS_MAX

to userspace, is that userspace can't really tell if you should only 
apply it to S16_LE or to S32_LE, or both. On the other hand if at some 
point someone adds S64_LE format, something like:

FORMAT: S16_LE S32_LE S64_LE
SUBFORMAT: STD MSBITS_20 MSBITS_32

will be also problematic as, you can't be sure is MSBITS subformats are 
meant for S32_LE or S64_LE format.

The easiest way would of course be to add specific formats like:
S20_MSB_LE
U20_MSB_LE
S20_MSB_BE
U20_MSB_BE
S24_MSB_LE
U24_MSB_LE
S24_MSB_BE
U24_MSB_BE
and one would know used formats exactly..., although I'm sure it is 
problematic in some ways, considering HDA currently exposes S32_LE and 
just sets msbits quietly and reports it after the fact to userspace.

Alternatively maybe, SUBFORMAT_STD could just mean LSB aligned where 
container is bigger than actual amount of bits and we can add 
SUBFORMAT_MSB, which can be then used with formats like S20_LE and 
S24_LE, to change bit justification, as it is unlikely to differ between 
formats?

Amadeusz

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Kai Vehmanen]

Hi,

On Wed, 23 Aug 2023, Amadeusz Sławiński wrote:

> The easiest way would of course be to add specific formats like:
> S20_MSB_LE
> U20_MSB_LE
> S20_MSB_BE
> U20_MSB_BE
> S24_MSB_LE
> U24_MSB_LE
> S24_MSB_BE
> U24_MSB_BE
> and one would know used formats exactly..., although I'm sure it is
> problematic in some ways, considering HDA currently exposes S32_LE and just
> sets msbits quietly and reports it after the fact to userspace.

Not just HDA but this is basicly all drivers. And this is ok in most 
cases. I mean with the MSB variants, applications can produce/consume data 
with full container bit width (like S32_LE) and for many usages, this will 
just work (the LSB data will be zero, or non-zero and ignored). Reporting 
the effective msbit is really important though, as apps _should_ check the 
field and dither when needed (although mileage varies, if you look up 
common ALSA apps, few bother to check msbit info). Anyways, this 
information definitely needs to be exposed and made available to apps.

But right, this is a bit orthogonal to the ability to _select_ a 
specific variant. That is currently not possible without your patchset.

Br, Kai

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Jaroslav Kysela]

On 23. 08. 23 18:29, Amadeusz Sławiński wrote:
> On 8/23/2023 3:42 PM, Jaroslav Kysela wrote:
>> On 23. 08. 23 13:08, Takashi Iwai wrote:
>>> On Wed, 23 Aug 2023 12:47:33 +0200,
>>> Jaroslav Kysela wrote:
>>>>
>>>> On 23. 08. 23 12:20, Amadeusz Sławiński wrote:
>>>>> On 8/23/2023 12:00 PM, Jaroslav Kysela wrote:
>>>>>> On 23. 08. 23 11:53, Takashi Iwai wrote:
>>>>>>> On Wed, 23 Aug 2023 11:10:38 +0200,
>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>
>>>>>>>> On 23. 08. 23 10:11, Cezary Rojewski wrote:
>>>>>>>>> On 2023-08-22 9:03 PM, Jaroslav Kysela wrote:
>>>>>>>>>> On 22. 08. 23 17:38, Takashi Iwai wrote:
>>>>>>>>>>> On Tue, 22 Aug 2023 17:29:47 +0200,
>>>>>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 22. 08. 23 17:07, Takashi Iwai wrote:
>>>>>>>>>>>>> On Tue, 22 Aug 2023 17:03:02 +0200,
>>>>>>>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32
>>>>>>>>>>>>>>> _SNDRV_PCM_SUBFMTBIT(MSBITS_32)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> What was reason to add 32/32 format ? Subformat STD +
>>>>>>>>>>>>>> msbits == 32
>>>>>>>>>>>>>> should already handle the maximal resolution. Until we do not
>>>>>>>>>>>>>> have 64
>>>>>>>>>>>>>> bit formats, it seems like an useless extension.
>>>>>>>>>>>>>
>>>>>>>>>>>>> My understanding is to distinguish the cases "we do fully
>>>>>>>>>>>>> support
>>>>>>>>>>>>> 32bit" and "we don't care".  But, the end effect is same for
>>>>>>>>>>>>> both,
>>>>>>>>>>>>> user-space would handle 32bit in both cases, so this difference
>>>>>>>>>>>>> won't
>>>>>>>>>>>>> help much, indeed.
>>>>>>>>>>>>
>>>>>>>>>>>> I don't think that we have a "do not care" situation. The
>>>>>>>>>>>> applications
>>>>>>>>>>>> currently expects to use the maximal msbits for STD
>>>>>>>>>>>> subformat. The
>>>>>>>>>>>> subformat should be used only to refine (downgrade) the
>>>>>>>>>>>> resolution on
>>>>>>>>>>>> the driver / hw side on demand. I would just add only
>>>>>>>>>>>> necessary API
>>>>>>>>>>>> extensions and save one bit for now.
>>>>>>>>>>>
>>>>>>>>>>> Well, the current behavior (with STD) is to choose whatever 32bit
>>>>>>>>>>> format the driver supports, and the driver may set a different
>>>>>>>>>>> value
>>>>>>>>>>> of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
>>>>>>>>>>> enforce the hw_params.msbits to be 32, otherwise hw_refine would
>>>>>>>>>>> fail.  So I see a potential difference.
>>>>>>>>>>
>>>>>>>>>> I see. But if our target is to create a complete query/set
>>>>>>>>>> msbits API,
>>>>>>>>>> we should cover all cases also for other formats.
>>>>>>>>>>
>>>>>>>>>> I vote to replace SUBFMTBIT_MSBITS_32 to SUBFMTBIT_MSBITS_MAX
>>>>>>>>>> as the
>>>>>>>>>> second bit (right after STD). The format hw parameter already
>>>>>>>>>> defines
>>>>>>>>>> the maximal width. We can add SUBFMTBIT_MSBITS_32 when it's really
>>>>>>>>>> required. Note that MAX should be handled for all cases (not
>>>>>>>>>> only for
>>>>>>>>>> S32_LE or so).
>>>>>>>>>
>>>>>>>>> In my opinion STD already states "max". The word is not explicit
>>>>>>>>> either
>>>>>>>>> - max in the eyes of whom? The driver'? Then the driver may
>>>>>>>>> reply: max
>>>>>>>>> allowed e.g.: 24/32. And that translates to: fallback to STD.
>>>>>>>>
>>>>>>>> Max in the contents of the physical sample format (S32 = 32 bits,
>>>>>>>> S24
>>>>>>>> = 24 bits, S8 = 8 bits etc). It would mean, if the driver
>>>>>>>> supports S32
>>>>>>>> but only with 24-bit resolution, this bit should not be
>>>>>>>> set/allowed. We can also use word full or something other. If we
>>>>>>>> like
>>>>>>>> to extend the API in this way (force the specific msbits with the
>>>>>>>> error handling), all formats should be covered. For STD - see
>>>>>>>> Takashi's reply.
>>>>>>>
>>>>>>> I think MAX can be problematic when the device supports multiple
>>>>>>> formats, say, 16bit and 32bit.  Then it's not clear which MAX points
>>>>>>> to: is 16bit max or 32bit max.
>>>>>>
>>>>>> I don't take this point. The subformat depends on the format, thus if
>>>>>> one format support max, it should be set for queries.
>>>>>>
>>>>>> Theoretically, this problem is in this API extension proposal too.
>>>>>> Imagine that driver/hw support S24 and S32 formats and 20-bit
>>>>>> msbits for
>>>>>> one of them. How do you handle this? The subformat depends on
>>>>>> format and
>>>>>> should be refined when the format is known (single choice).
>>>>>>
>>>>>>> I find the subformat extension OK, as this doesn't need much
>>>>>>> change in
>>>>>>> API.  OTOH, if we want to be more consistent way, we may extend
>>>>>>> hw_params for a new interval, e.g. SNDRV_PCM_HW_PARAM_MSBITS, and let
>>>>>>> the driver choosing it.  This will need more hw_params rules and
>>>>>>> become more complex, but it allows drivers really exotic setups (like
>>>>>>> 19bit PCM :)  But my gut feeling is that the subformat extension
>>>>>>> should suffice.
>>>>>>
>>>>>> I'm not ok with 32 == 32. We should handle this case universally or
>>>>>> discard.
>>>>>>
>>>>>>                         Jaroslav
>>>>>>
>>>>>
>>>>> The reason for MSBITS32 is that, when only MSBITS_20 and MSBITS_24 are
>>>>> defined, when userspace (in this case aplay) asks for usable formats
>>>>> and
>>>>> subformat it gets something like:
>>>>>
>>>>> --------------------
>>>>> ACCESS: MMAP_INTERLEAVED RW_INTERLEAVED
>>>>> FORMAT: S16_LE S32_LE
>>>>> SUBFORMAT: STD MSBITS_20 MSBITS_24
>>>>> SAMPLE_BITS: [16 32]
>>>>> FRAME_BITS: [32 64]
>>>>> CHANNELS: 2
>>>>> RATE: 48000
>>>>>
>>>>> when MSBITS_32 is not defined it is not clear if HW supports 24 or 32
>>>>> bits as maximum possible value of msbits. However when MSBITS_32 is
>>>>> defined it is clear - in the above case maximum possible bps then is
>>>>> 24,
>>>>> because MSBITS_32 is missing in output.
>>>>>
>>>>> STD behaves as before and takes maximum possible value - in above case
>>>>> it is 24.
>>>>
>>>> This example is nice for S32_LE but not S16_LE. With the max/full bit,
>>>> we can already cover also S16_LE (so that the application can ask for
>>>> the maximal msbits which fits to the physical format for S16_LE). It
>>>> would be also a preparation for future, when we need to deal with more
>>>> msbits combos (like 14bit or 15bit samples stored in the bigger
>>>> physical words) etc.
>>>>
>>>> So application can set those parameters for in your case:
>>>>
>>>> S16_LE + STD         (maximum bits handled by driver - 16 in this case)
>>>> S16_LE + MSBITS_MAX  (maximum physical bits for the format - 16)
>>>> S32_LE + STD         (maximum bits handled by driver - 24 in this case)
>>>> S32_LE + MSBITS_MAX  (maximum physical bits for the format - 32)
>>>> S32_LE + MSBITS_24
>>>> S32_LE + MSBITS_20
>>>>
>>>> Dtto for other format like S8, S24 etc. Another way is to define
>>>> MSBITS_8, MSBITS_16 etc. But I'd prefer to save subformat bits. The
>>>> MSBITS_MAX would cover almost all cases for now.
>>>
>>> It becomes a bit tricky if we have a device that has 24bit bps on
>>> 32bit format and 16bit bps / 16bit format.  Both formats and
>>> subformats are bitmaps in hw_params, and initially formats bitmap is
>>> 16|24 and subformats bitmap is MAX|24.
>>>
>>> Now, suppose that app determines to use 16bit.  Then we need to we
>>> need to update subformats bitmap to MAX by dropping 24.  OTOH, if app
>>> chooses 32bit format, subformats will be 24 by dropping MAX, as we
>>> don't support 32 bps.  And, it's not so trivial to achieve this
>>> commonly only with the single subformats bitmap of snd_pcm_hardware,
>>> as the meaning of MAX depends on the chosen format.
>>
>> It's easy to implement using a code which will go through all set format
>> bits and do bitwise OR for all corresponding subformat bits. We can use
>> a callback which the driver may override.
>>
>> Something like (for the above HDA example): - SUBFMTBIT_STD is required
>> so it would be handled at the upper level):
>>
>> snd_pcm_subformat_t (*get_subformat)(struct snd_pcm_substream
>> *substream, snd_pcm_format_t format)
>> {
>>           switch (format) {
>>       case SNDRV_PCM_FMTBIT_S16_LE:
>>           return SNDRV_PCM_SUBFMTBIT_MSBITS_MAX;
>>           case SNDRV_PCM_FMTBIT_S32_LE:
>>                   return SNDRV_PCM_SUBFMTBIT_MSBITS_24 |
>>                          SNDRV_PCM_SUBFMTBIT_MSBITS_20;
>>       default:
>>           return 0;
>>       }
>> }
>>
>> SUBFMTBIT_STD is required so it would be handled at the upper level.
>>
>>> Meanwhile, if the subformats bitmap is with explicit bit flags,
>>> i.e. 24|16, we can reduce the bitmap easily depending on the format.
>>
>> I don't think that the current proposed code does any reduction. It will
>> return MSBITS_24 when only S16_LE format is selected, too. The refining
>> mechanism is ignored. We need to handle subformat bits differently than
>> format bits because the dependency.
>>
>>                       Jaroslav
>>
> 
> Problem with MSBITS_MAX is that if kernel reports something like this:
> 
> FORMAT: S16_LE S32_LE
> SUBFORMAT: STD MSBITS_20 MSBITS_MAX
> 
> to userspace, is that userspace can't really tell if you should only
> apply it to S16_LE or to S32_LE, or both. On the other hand if at some
> point someone adds S64_LE format, something like:

Unfortunately, you've not got the point that the subformat contents depends on 
the selected format. So the subformat mask is for ALL formats selected in the 
configuration space. The only valid contents for one format is when 
application or kernel reduces the format to single one. And applications can do:

1) set format to S32_LE
2) call refine
3) get subformat bits for single S32_LE format from the refined cfg space

In this case, queries and specific msbits selection will work. It's the 
standard refine mechanism which works also for all other fields from the 
parameter configuration space etc. If you look to all other fields from the 
parameter configuration space, you cannot predict the exact parameters (buffer 
size, period size, channels) until you do more refining to set all parameters 
to exact values (single value).

In other words, the above example:

FORMAT: S16_LE S32_LE
SUBFORMAT: STD MSBITS_20 MSBITS_MAX

.. means - at least one format supports maximal msbits for the given format.

						Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Cezary Rojewski]

On 2023-08-24 9:31 AM, Jaroslav Kysela wrote:
> On 23. 08. 23 18:29, Amadeusz Sławiński wrote:

...

>> Problem with MSBITS_MAX is that if kernel reports something like this:
>>
>> FORMAT: S16_LE S32_LE
>> SUBFORMAT: STD MSBITS_20 MSBITS_MAX
>>
>> to userspace, is that userspace can't really tell if you should only
>> apply it to S16_LE or to S32_LE, or both. On the other hand if at some
>> point someone adds S64_LE format, something like:
> 
> Unfortunately, you've not got the point that the subformat contents 
> depends on the selected format. So the subformat mask is for ALL formats 
> selected in the configuration space. The only valid contents for one 
> format is when application or kernel reduces the format to single one. 
> And applications can do:
> 
> 1) set format to S32_LE
> 2) call refine
> 3) get subformat bits for single S32_LE format from the refined cfg space
> 
> In this case, queries and specific msbits selection will work. It's the 
> standard refine mechanism which works also for all other fields from the 
> parameter configuration space etc. If you look to all other fields from 
> the parameter configuration space, you cannot predict the exact 
> parameters (buffer size, period size, channels) until you do more 
> refining to set all parameters to exact values (single value).
> 
> In other words, the above example:
> 
> FORMAT: S16_LE S32_LE
> SUBFORMAT: STD MSBITS_20 MSBITS_MAX
> 
> .. means - at least one format supports maximal msbits for the given 
> format.

After reading all of this again, I'm fine with rewording MSBITS_32 to 
MSBITS_MAX.

As I do not see any other points to address here and review of v1 has no 
points to address either, I'll send v2 with this single change. If I'd 
missed anything, let me know.


Kind regards,
Czarek

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Jaroslav Kysela]

On 08. 09. 23 16:36, Cezary Rojewski wrote:
> On 2023-08-24 9:31 AM, Jaroslav Kysela wrote:
>> On 23. 08. 23 18:29, Amadeusz Sławiński wrote:
> 
> ...
> 
>>> Problem with MSBITS_MAX is that if kernel reports something like this:
>>>
>>> FORMAT: S16_LE S32_LE
>>> SUBFORMAT: STD MSBITS_20 MSBITS_MAX
>>>
>>> to userspace, is that userspace can't really tell if you should only
>>> apply it to S16_LE or to S32_LE, or both. On the other hand if at some
>>> point someone adds S64_LE format, something like:
>>
>> Unfortunately, you've not got the point that the subformat contents
>> depends on the selected format. So the subformat mask is for ALL formats
>> selected in the configuration space. The only valid contents for one
>> format is when application or kernel reduces the format to single one.
>> And applications can do:
>>
>> 1) set format to S32_LE
>> 2) call refine
>> 3) get subformat bits for single S32_LE format from the refined cfg space
>>
>> In this case, queries and specific msbits selection will work. It's the
>> standard refine mechanism which works also for all other fields from the
>> parameter configuration space etc. If you look to all other fields from
>> the parameter configuration space, you cannot predict the exact
>> parameters (buffer size, period size, channels) until you do more
>> refining to set all parameters to exact values (single value).
>>
>> In other words, the above example:
>>
>> FORMAT: S16_LE S32_LE
>> SUBFORMAT: STD MSBITS_20 MSBITS_MAX
>>
>> .. means - at least one format supports maximal msbits for the given
>> format.
> 
> After reading all of this again, I'm fine with rewording MSBITS_32 to
> MSBITS_MAX.
> 
> As I do not see any other points to address here and review of v1 has no
> points to address either, I'll send v2 with this single change. If I'd
> missed anything, let me know.

The subformat bitmask should be also refined/updated depending on the selected 
format.

https://lore.kernel.org/alsa-devel/f97bbbd5-1397-f5d3-5ccf-420ec813deac@perex.cz/

It requires new code in pcm_lib.c and ASoC PCM core code.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Cezary Rojewski]

On 2023-09-11 9:35 AM, Jaroslav Kysela wrote:
> On 08. 09. 23 16:36, Cezary Rojewski wrote:

...

>> After reading all of this again, I'm fine with rewording MSBITS_32 to
>> MSBITS_MAX.
>>
>> As I do not see any other points to address here and review of v1 has no
>> points to address either, I'll send v2 with this single change. If I'd
>> missed anything, let me know.
> 
> The subformat bitmask should be also refined/updated depending on the 
> selected format.
> 
> https://lore.kernel.org/alsa-devel/f97bbbd5-1397-f5d3-5ccf-420ec813deac@perex.cz/
> 
> It requires new code in pcm_lib.c and ASoC PCM core code.

Could you help me understand what new code is needed? The 
get_subformat() example raised more questions than answers. The patchset 
defines snd_pcm_subformat_width(), perhaps you meant that I should 
update that function by adding paramter 'format' to its parameters list 
and handle it accordingly?

Any guidance would be much appreciated.

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Cezary Rojewski]

On 2023-09-11 10:43 AM, Cezary Rojewski wrote:
> On 2023-09-11 9:35 AM, Jaroslav Kysela wrote:
>> On 08. 09. 23 16:36, Cezary Rojewski wrote:

...

>>> After reading all of this again, I'm fine with rewording MSBITS_32 to
>>> MSBITS_MAX.
>>>
>>> As I do not see any other points to address here and review of v1 has no
>>> points to address either, I'll send v2 with this single change. If I'd
>>> missed anything, let me know.
>>
>> The subformat bitmask should be also refined/updated depending on the 
>> selected format.
>>
>> https://lore.kernel.org/alsa-devel/f97bbbd5-1397-f5d3-5ccf-420ec813deac@perex.cz/
>>
>> It requires new code in pcm_lib.c and ASoC PCM core code.
> 
> Could you help me understand what new code is needed? The 
> get_subformat() example raised more questions than answers. The patchset 
> defines snd_pcm_subformat_width(), perhaps you meant that I should 
> update that function by adding paramter 'format' to its parameters list 
> and handle it accordingly?
> 
> Any guidance would be much appreciated.

What I come up with is a hw_rule for subformat that I add in 
snd_pcm_hw_constraints_init(). That piece, plus both STD and MSBITS_MAX 
ORed into hw->subformats in snd_pcm_hw_constraints_complete() make 
things spin.

static int snd_pcm_hw_rule_subformat(struct snd_pcm_hw_params *params,
				     struct snd_pcm_hw_rule *rule)
{
	struct snd_mask *subformat_mask = hw_param_mask(params, 
SNDRV_PCM_HW_PARAM_SUBFORMAT);
	struct snd_mask *format_mask = hw_param_mask(params, 
SNDRV_PCM_HW_PARAM_FORMAT);
	snd_pcm_format_t f;
	struct snd_mask m;
	int width;

	snd_mask_none(&m);
	snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_STD);
	snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_MAX);

	pcm_for_each_format(f) {
		if (!snd_mask_test_format(format_mask, f))
			continue;

		width = snd_pcm_format_width(f);
		switch (width) {
		case 32:
			snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_20);
			snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_24);
			break;
		default:
			break;
		}
	}

	return snd_mask_refine(subformat_mask, &m);
}


However, this means snd_hdac_query_supported_pcm() becomes confusing as 
you need to MSBITS_MAX regardless of what the codec supports.
After spending additional few hours on this, I'd say I preferred how 
things look with MSBITS_32 instead. STD and MSBITS_MAX existing 
simultaneously is confusing too.

Czarek

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Jaroslav Kysela]

On 11. 09. 23 17:45, Cezary Rojewski wrote:
> On 2023-09-11 10:43 AM, Cezary Rojewski wrote:
>> On 2023-09-11 9:35 AM, Jaroslav Kysela wrote:
>>> On 08. 09. 23 16:36, Cezary Rojewski wrote:
> 
> ...
> 
>>>> After reading all of this again, I'm fine with rewording MSBITS_32 to
>>>> MSBITS_MAX.
>>>>
>>>> As I do not see any other points to address here and review of v1 has no
>>>> points to address either, I'll send v2 with this single change. If I'd
>>>> missed anything, let me know.
>>>
>>> The subformat bitmask should be also refined/updated depending on the
>>> selected format.
>>>
>>> https://lore.kernel.org/alsa-devel/f97bbbd5-1397-f5d3-5ccf-420ec813deac@perex.cz/
>>>
>>> It requires new code in pcm_lib.c and ASoC PCM core code.
>>
>> Could you help me understand what new code is needed? The
>> get_subformat() example raised more questions than answers. The patchset
>> defines snd_pcm_subformat_width(), perhaps you meant that I should
>> update that function by adding paramter 'format' to its parameters list
>> and handle it accordingly?
>>
>> Any guidance would be much appreciated.
> 
> What I come up with is a hw_rule for subformat that I add in
> snd_pcm_hw_constraints_init(). That piece, plus both STD and MSBITS_MAX
> ORed into hw->subformats in snd_pcm_hw_constraints_complete() make
> things spin.
> 
> static int snd_pcm_hw_rule_subformat(struct snd_pcm_hw_params *params,
> 				     struct snd_pcm_hw_rule *rule)
> {
> 	struct snd_mask *subformat_mask = hw_param_mask(params,
> SNDRV_PCM_HW_PARAM_SUBFORMAT);
> 	struct snd_mask *format_mask = hw_param_mask(params,
> SNDRV_PCM_HW_PARAM_FORMAT);
> 	snd_pcm_format_t f;
> 	struct snd_mask m;
> 	int width;
> 
> 	snd_mask_none(&m);
> 	snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_STD);
> 	snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_MAX);
> 
> 	pcm_for_each_format(f) {
> 		if (!snd_mask_test_format(format_mask, f))
> 			continue;
> 
> 		width = snd_pcm_format_width(f);
> 		switch (width) {
> 		case 32:
> 			snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_20);
> 			snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_24);
> 			break;
> 		default:
> 			break;
> 		}
> 	}
> 
> 	return snd_mask_refine(subformat_mask, &m);
> }
> 
> 
> However, this means snd_hdac_query_supported_pcm() becomes confusing as
> you need to MSBITS_MAX regardless of what the codec supports.
> After spending additional few hours on this, I'd say I preferred how
> things look with MSBITS_32 instead. STD and MSBITS_MAX existing
> simultaneously is confusing too.

This is not a correct implementation. Many things are missing including the 
proper subformats filter. I sent my own version here for discussion:

https://lore.kernel.org/alsa-devel/20230912162526.7138-1-perex@perex.cz/

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Cezary Rojewski]

On 2023-09-12 6:30 PM, Jaroslav Kysela wrote:
> On 11. 09. 23 17:45, Cezary Rojewski wrote:
>> On 2023-09-11 10:43 AM, Cezary Rojewski wrote:

...

>>>
>>> Could you help me understand what new code is needed? The
>>> get_subformat() example raised more questions than answers. The patchset
>>> defines snd_pcm_subformat_width(), perhaps you meant that I should
>>> update that function by adding paramter 'format' to its parameters list
>>> and handle it accordingly?
>>>
>>> Any guidance would be much appreciated.
>>
>> What I come up with is a hw_rule for subformat that I add in
>> snd_pcm_hw_constraints_init(). That piece, plus both STD and MSBITS_MAX
>> ORed into hw->subformats in snd_pcm_hw_constraints_complete() make
>> things spin.
>>
>> static int snd_pcm_hw_rule_subformat(struct snd_pcm_hw_params *params,
>>                      struct snd_pcm_hw_rule *rule)
>> {
>>     struct snd_mask *subformat_mask = hw_param_mask(params,
>> SNDRV_PCM_HW_PARAM_SUBFORMAT);
>>     struct snd_mask *format_mask = hw_param_mask(params,
>> SNDRV_PCM_HW_PARAM_FORMAT);
>>     snd_pcm_format_t f;
>>     struct snd_mask m;
>>     int width;
>>
>>     snd_mask_none(&m);
>>     snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_STD);
>>     snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_MAX);
>>
>>     pcm_for_each_format(f) {
>>         if (!snd_mask_test_format(format_mask, f))
>>             continue;
>>
>>         width = snd_pcm_format_width(f);
>>         switch (width) {
>>         case 32:
>>             snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_20);
>>             snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_24);
>>             break;
>>         default:
>>             break;
>>         }
>>     }
>>
>>     return snd_mask_refine(subformat_mask, &m);
>> }
>>
>>
>> However, this means snd_hdac_query_supported_pcm() becomes confusing as
>> you need to MSBITS_MAX regardless of what the codec supports.
>> After spending additional few hours on this, I'd say I preferred how
>> things look with MSBITS_32 instead. STD and MSBITS_MAX existing
>> simultaneously is confusing too.
> 
> This is not a correct implementation. Many things are missing including 
> the proper subformats filter. I sent my own version here for discussion:
> 
> https://lore.kernel.org/alsa-devel/20230912162526.7138-1-perex@perex.cz/

I do appreciate the input but I expected that, through guidance, this 
patch gets updated. Sending a separate patch, not connected to this 
patchset - not even a single reference within the commit message body - 
is not nice.

I'd rather send v2 with your patch incorporated as a part of the 
patchset. The subformat improvement alone, without showcasing the users 
is confusing to the reviewers.


Czarek

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Jaroslav Kysela]

On 13. 09. 23 9:44, Cezary Rojewski wrote:
> On 2023-09-12 6:30 PM, Jaroslav Kysela wrote:
>> On 11. 09. 23 17:45, Cezary Rojewski wrote:
>>> On 2023-09-11 10:43 AM, Cezary Rojewski wrote:
> 
> ...
> 
>>>>
>>>> Could you help me understand what new code is needed? The
>>>> get_subformat() example raised more questions than answers. The patchset
>>>> defines snd_pcm_subformat_width(), perhaps you meant that I should
>>>> update that function by adding paramter 'format' to its parameters list
>>>> and handle it accordingly?
>>>>
>>>> Any guidance would be much appreciated.
>>>
>>> What I come up with is a hw_rule for subformat that I add in
>>> snd_pcm_hw_constraints_init(). That piece, plus both STD and MSBITS_MAX
>>> ORed into hw->subformats in snd_pcm_hw_constraints_complete() make
>>> things spin.
>>>
>>> static int snd_pcm_hw_rule_subformat(struct snd_pcm_hw_params *params,
>>>                       struct snd_pcm_hw_rule *rule)
>>> {
>>>      struct snd_mask *subformat_mask = hw_param_mask(params,
>>> SNDRV_PCM_HW_PARAM_SUBFORMAT);
>>>      struct snd_mask *format_mask = hw_param_mask(params,
>>> SNDRV_PCM_HW_PARAM_FORMAT);
>>>      snd_pcm_format_t f;
>>>      struct snd_mask m;
>>>      int width;
>>>
>>>      snd_mask_none(&m);
>>>      snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_STD);
>>>      snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_MAX);
>>>
>>>      pcm_for_each_format(f) {
>>>          if (!snd_mask_test_format(format_mask, f))
>>>              continue;
>>>
>>>          width = snd_pcm_format_width(f);
>>>          switch (width) {
>>>          case 32:
>>>              snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_20);
>>>              snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_24);
>>>              break;
>>>          default:
>>>              break;
>>>          }
>>>      }
>>>
>>>      return snd_mask_refine(subformat_mask, &m);
>>> }
>>>
>>>
>>> However, this means snd_hdac_query_supported_pcm() becomes confusing as
>>> you need to MSBITS_MAX regardless of what the codec supports.
>>> After spending additional few hours on this, I'd say I preferred how
>>> things look with MSBITS_32 instead. STD and MSBITS_MAX existing
>>> simultaneously is confusing too.
>>
>> This is not a correct implementation. Many things are missing including
>> the proper subformats filter. I sent my own version here for discussion:
>>
>> https://lore.kernel.org/alsa-devel/20230912162526.7138-1-perex@perex.cz/
> 
> I do appreciate the input but I expected that, through guidance, this
> patch gets updated. Sending a separate patch, not connected to this
> patchset - not even a single reference within the commit message body -
> is not nice.

The basic API extension is self-contained and I marked it as RFC. The 
connection was added to this thread.

> I'd rather send v2 with your patch incorporated as a part of the
> patchset.
No problem. Please, add these cosmetic changes to my patch:

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index f414f8fd217b..cb376e428f59 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1412,9 +1412,10 @@ static int snd_pcm_hw_rule_subformats(struct 
snd_pcm_hw_params *params,
         struct snd_mask m;
         struct snd_mask *fmask = hw_param_mask(params, 
SNDRV_PCM_HW_PARAM_FORMAT);
         struct snd_mask *mask = hw_param_mask(params, 
SNDRV_PCM_HW_PARAM_SUBFORMAT);
+       bool found;
+
         snd_mask_none(&m);
         snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_STD);
-       bool found;
         pcm_for_each_format(k) {
                 if (!snd_mask_test(fmask, k))
                         continue;
@@ -1437,7 +1438,6 @@ static int snd_pcm_hw_rule_subformats(struct 
snd_pcm_hw_params *params,
   * @runtime: PCM runtime instance
   * @cond: condition bits
   * @subformats: array with struct snd_pcm_subformat elements
- * @nmemd: size of array with struct snd_pcm_subformat elements
   *
   * This constraint will set relation between format and subformats.
   * The STD and MAX subformats are handled automatically. If the driver

						Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Cezary Rojewski]

On 2023-08-22 5:07 PM, Takashi Iwai wrote:
> On Tue, 22 Aug 2023 17:03:02 +0200,
> Jaroslav Kysela wrote:
>>
>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
>>
>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32	_SNDRV_PCM_SUBFMTBIT(MSBITS_32)
>>
>> What was reason to add 32/32 format ? Subformat STD + msbits == 32
>> should already handle the maximal resolution. Until we do not have 64
>> bit formats, it seems like an useless extension.
> 
> My understanding is to distinguish the cases "we do fully support
> 32bit" and "we don't care".  But, the end effect is same for both,
> user-space would handle 32bit in both cases, so this difference won't
> help much, indeed.

Bull's eye. We do want to be able to distinguish between "default" and 
"must be 32/32" cases. In fact we do have a test where each iteration 
takes a different --subformat option. On some configurations, iteration 
with --subformat=MSBITS_32 is expected to fail and PCM device remains 
closed - negative test case. If given configuration supports up to 
24/32, then 32/32 shall not be allowed. Option --subformat=STD is 
expected to select the default max e.g.: 24/32.

Czarek

[RFC PATCH 02/17] ALSA: pcm: Honor subformat when configuring substream

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 sound/core/pcm_native.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 39a65d1415ab..cb2745f80af3 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2618,6 +2618,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);
         if (hw->info & SNDRV_PCM_INFO_INTERLEAVED)
 		mask |= PARAM_MASK_BIT(SNDRV_PCM_ACCESS_RW_INTERLEAVED);
         if (hw->info & SNDRV_PCM_INFO_NONINTERLEAVED)
@@ -2638,8 +2640,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);
 	if (err < 0)
 		return err;
 
-- 
2.25.1

[RFC PATCH 03/17] ALSA: hda: Honor subformat when querying PCMs

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 include/sound/hda_codec.h    |  5 +++--
 include/sound/hdaudio.h      |  2 +-
 sound/hda/hdac_device.c      | 42 ++++++++++++++++++++----------------
 sound/pci/hda/hda_codec.c    |  2 ++
 sound/pci/hda/patch_hdmi.c   |  1 +
 sound/soc/codecs/hdac_hdmi.c |  3 ++-
 6 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
index bbb7805e85d8..1a1d22b3a3ae 100644
--- a/include/sound/hda_codec.h
+++ b/include/sound/hda_codec.h
@@ -144,6 +144,7 @@ struct hda_pcm_stream {
 	hda_nid_t nid;	/* default NID to query rates/formats/bps, or set up */
 	u32 rates;	/* supported rates */
 	u64 formats;	/* supported formats (SNDRV_PCM_FMTBIT_) */
+	u64 subformats;	/* supported subformats (SNDRV_PCM_SUBFMTBIT_) */
 	unsigned int maxbps;	/* supported max. bit per sample */
 	const struct snd_pcm_chmap_elem *chmap; /* chmap to override */
 	struct hda_pcm_ops ops;
@@ -451,8 +452,8 @@ void __snd_hda_codec_cleanup_stream(struct hda_codec *codec, hda_nid_t nid,
 #define snd_hda_codec_cleanup_stream(codec, nid) \
 	__snd_hda_codec_cleanup_stream(codec, nid, 0)
 
-#define snd_hda_query_supported_pcm(codec, nid, ratesp, fmtsp, bpsp) \
-	snd_hdac_query_supported_pcm(&(codec)->core, nid, ratesp, fmtsp, bpsp)
+#define snd_hda_query_supported_pcm(codec, nid, ratesp, fmtsp, subfmtp, bpsp) \
+	snd_hdac_query_supported_pcm(&(codec)->core, nid, ratesp, fmtsp, subfmtp, bpsp)
 #define snd_hda_is_supported_format(codec, nid, fmt) \
 	snd_hdac_is_supported_format(&(codec)->core, nid, fmt)
 
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 97f09acae302..a549956e102a 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -145,7 +145,7 @@ unsigned int snd_hdac_calc_stream_format(unsigned int rate,
 					 unsigned int maxbps,
 					 unsigned short spdif_ctls);
 int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid,
-				u32 *ratesp, u64 *formatsp, unsigned int *bpsp);
+				 u32 *ratesp, u64 *formatsp, u64 *subformatsp, unsigned int *bpsp);
 bool snd_hdac_is_supported_format(struct hdac_device *codec, hda_nid_t nid,
 				  unsigned int format);
 
diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index 6c043fbd606f..5b01a2b9033d 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -816,15 +816,16 @@ static unsigned int query_stream_param(struct hdac_device *codec, hda_nid_t nid)
  * @nid: NID to query
  * @ratesp: the pointer to store the detected rate bitflags
  * @formatsp: the pointer to store the detected formats
+ * @subformatsp: the pointer to store the detected subformats
  * @bpsp: the pointer to store the detected format widths
  *
- * Queries the supported PCM rates and formats.  The NULL @ratesp, @formatsp
- * or @bsps argument is ignored.
+ * Queries the supported PCM rates and formats.  The NULL @ratesp, @formatsp,
+ * @subformatsp or @bpsp argument is ignored.
  *
  * Returns 0 if successful, otherwise a negative error code.
  */
 int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid,
-				 u32 *ratesp, u64 *formatsp, unsigned int *bpsp)
+				 u32 *ratesp, u64 *formatsp, u64 *subformatsp, unsigned int *bpsp)
 {
 	unsigned int i, val, wcaps;
 
@@ -847,8 +848,9 @@ int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid,
 		*ratesp = rates;
 	}
 
-	if (formatsp || bpsp) {
+	if (formatsp || subformatsp || bpsp) {
 		u64 formats = 0;
+		u64 subformats = 0;
 		unsigned int streams, bps;
 
 		streams = query_stream_param(codec, nid);
@@ -865,24 +867,24 @@ int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid,
 				formats |= SNDRV_PCM_FMTBIT_S16_LE;
 				bps = 16;
 			}
-			if (wcaps & AC_WCAP_DIGITAL) {
-				if (val & AC_SUPPCM_BITS_32)
+			if (val & AC_SUPPCM_BITS_20) {
+				formats |= SNDRV_PCM_FMTBIT_S32_LE;
+				subformats |= SNDRV_PCM_SUBFMTBIT_MSBITS_20;
+				bps = 20;
+			}
+			if (val & AC_SUPPCM_BITS_24) {
+				formats |= SNDRV_PCM_FMTBIT_S32_LE;
+				subformats |= SNDRV_PCM_SUBFMTBIT_MSBITS_24;
+				bps = 24;
+			}
+			if (val & AC_SUPPCM_BITS_32) {
+				subformats |= SNDRV_PCM_SUBFMTBIT_MSBITS_32;
+				if (wcaps & AC_WCAP_DIGITAL) {
 					formats |= SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE;
-				if (val & (AC_SUPPCM_BITS_20|AC_SUPPCM_BITS_24))
+				} else {
 					formats |= SNDRV_PCM_FMTBIT_S32_LE;
-				if (val & AC_SUPPCM_BITS_24)
-					bps = 24;
-				else if (val & AC_SUPPCM_BITS_20)
-					bps = 20;
-			} else if (val & (AC_SUPPCM_BITS_20|AC_SUPPCM_BITS_24|
-					  AC_SUPPCM_BITS_32)) {
-				formats |= SNDRV_PCM_FMTBIT_S32_LE;
-				if (val & AC_SUPPCM_BITS_32)
 					bps = 32;
-				else if (val & AC_SUPPCM_BITS_24)
-					bps = 24;
-				else if (val & AC_SUPPCM_BITS_20)
-					bps = 20;
+				}
 			}
 		}
 #if 0 /* FIXME: CS4206 doesn't work, which is the only codec supporting float */
@@ -910,6 +912,8 @@ int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid,
 		}
 		if (formatsp)
 			*formatsp = formats;
+		if (subformatsp)
+			*subformatsp = subformats;
 		if (bpsp)
 			*bpsp = bps;
 	}
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 9f79c0ac2bda..fc7b3361284d 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -3161,6 +3161,7 @@ static int set_pcm_default_values(struct hda_codec *codec,
 		err = snd_hda_query_supported_pcm(codec, info->nid,
 				info->rates ? NULL : &info->rates,
 				info->formats ? NULL : &info->formats,
+				info->subformats ? NULL : &info->subformats,
 				info->maxbps ? NULL : &info->maxbps);
 		if (err < 0)
 			return err;
@@ -3755,6 +3756,7 @@ int snd_hda_multi_out_analog_open(struct hda_codec *codec,
 			snd_hda_query_supported_pcm(codec, mout->dig_out_nid,
 						    &mout->spdif_rates,
 						    &mout->spdif_formats,
+						    NULL,
 						    &mout->spdif_maxbps);
 		}
 		mutex_lock(&codec->spdif_mutex);
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 5c0b1a09fd57..49e9fe09ce88 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1977,6 +1977,7 @@ static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t cvt_nid)
 	err = snd_hda_query_supported_pcm(codec, cvt_nid,
 					  &per_cvt->rates,
 					  &per_cvt->formats,
+					  NULL,
 					  &per_cvt->maxbps);
 	if (err < 0)
 		return err;
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 8b6b76029694..fb3f8a565485 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -670,6 +670,7 @@ hdac_hdmi_query_cvt_params(struct hdac_device *hdev, struct hdac_hdmi_cvt *cvt)
 	err = snd_hdac_query_supported_pcm(hdev, cvt->nid,
 			&cvt->params.rates,
 			&cvt->params.formats,
+			NULL,
 			&cvt->params.maxbps);
 	if (err < 0)
 		dev_err(&hdev->dev,
@@ -1577,7 +1578,7 @@ static int hdac_hdmi_create_dais(struct hdac_device *hdev,
 
 	list_for_each_entry(cvt, &hdmi->cvt_list, head) {
 		ret = snd_hdac_query_supported_pcm(hdev, cvt->nid,
-					&rates,	&formats, &bps);
+					&rates,	&formats, NULL, &bps);
 		if (ret)
 			return ret;
 
-- 
2.25.1

[RFC PATCH 04/17] ASoC: pcm: Honor subformat when configuring runtime

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 include/sound/soc.h |  1 +
 sound/soc/soc-pcm.c | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 755907fe82b4..cb74295ab020 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -619,6 +619,7 @@ enum snd_soc_trigger_order {
 struct snd_soc_pcm_stream {
 	const char *stream_name;
 	u64 formats;			/* SNDRV_PCM_FMTBIT_* */
+	u64 subformats;			/* SNDRV_PCM_SUBFMTBIT_* */
 	unsigned int rates;		/* SNDRV_PCM_RATE_* */
 	unsigned int rate_min;		/* min rate */
 	unsigned int rate_max;		/* max rate */
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index dc5f41879cc0..80118c5cb194 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -525,6 +525,7 @@ static void soc_pcm_hw_init(struct snd_pcm_hardware *hw)
 	hw->channels_min	= 0;
 	hw->channels_max	= UINT_MAX;
 	hw->formats		= ULLONG_MAX;
+	hw->subformats		= ULLONG_MAX;
 }
 
 static void soc_pcm_hw_update_rate(struct snd_pcm_hardware *hw,
@@ -553,6 +554,12 @@ static void soc_pcm_hw_update_format(struct snd_pcm_hardware *hw,
 	hw->formats &= p->formats;
 }
 
+static void soc_pcm_hw_update_subformat(struct snd_pcm_hardware *hw,
+					struct snd_soc_pcm_stream *p)
+{
+	hw->subformats &= p->subformats;
+}
+
 /**
  * snd_soc_runtime_calc_hw() - Calculate hw limits for a PCM stream
  * @rtd: ASoC PCM runtime
@@ -591,6 +598,7 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 		soc_pcm_hw_update_chan(hw, cpu_stream);
 		soc_pcm_hw_update_rate(hw, cpu_stream);
 		soc_pcm_hw_update_format(hw, cpu_stream);
+		soc_pcm_hw_update_subformat(hw, cpu_stream);
 	}
 	cpu_chan_min = hw->channels_min;
 	cpu_chan_max = hw->channels_max;
@@ -612,6 +620,7 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 		soc_pcm_hw_update_chan(hw, codec_stream);
 		soc_pcm_hw_update_rate(hw, codec_stream);
 		soc_pcm_hw_update_format(hw, codec_stream);
+		soc_pcm_hw_update_subformat(hw, codec_stream);
 	}
 
 	/* Verify both a valid CPU DAI and a valid CODEC DAI were found */
@@ -636,6 +645,7 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_hardware *hw = &substream->runtime->hw;
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	u64 subformats = hw->subformats;
 	u64 formats = hw->formats;
 
 	/*
@@ -647,6 +657,8 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
 
 	if (formats)
 		hw->formats &= formats;
+	if (subformats)
+		hw->subformats &= subformats;
 }
 
 static int soc_pcm_components_open(struct snd_pcm_substream *substream)
@@ -1675,6 +1687,7 @@ static void dpcm_runtime_setup_fe(struct snd_pcm_substream *substream)
 	struct snd_pcm_hardware *hw = &runtime->hw;
 	struct snd_soc_dai *dai;
 	int stream = substream->stream;
+	u64 subformats = hw->subformats;
 	u64 formats = hw->formats;
 	int i;
 
@@ -1682,6 +1695,8 @@ static void dpcm_runtime_setup_fe(struct snd_pcm_substream *substream)
 
 	if (formats)
 		hw->formats &= formats;
+	if (subformats)
+		hw->subformats &= subformats;
 
 	for_each_rtd_cpu_dais(fe, i, dai) {
 		struct snd_soc_pcm_stream *cpu_stream;
@@ -1698,6 +1713,7 @@ static void dpcm_runtime_setup_fe(struct snd_pcm_substream *substream)
 		soc_pcm_hw_update_rate(hw, cpu_stream);
 		soc_pcm_hw_update_chan(hw, cpu_stream);
 		soc_pcm_hw_update_format(hw, cpu_stream);
+		soc_pcm_hw_update_subformat(hw, cpu_stream);
 	}
 
 }
@@ -1735,6 +1751,7 @@ static void dpcm_runtime_setup_be_format(struct snd_pcm_substream *substream)
 			codec_stream = snd_soc_dai_get_pcm_stream(dai, stream);
 
 			soc_pcm_hw_update_format(hw, codec_stream);
+			soc_pcm_hw_update_subformat(hw, codec_stream);
 		}
 	}
 }
-- 
2.25.1

[RFC PATCH 05/17] ALSA: hda: Upgrade stream-format infrastructure

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 include/sound/hdaudio.h |   5 ++
 sound/hda/hdac_device.c | 124 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index a549956e102a..f6d76c23ffa4 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -139,6 +139,11 @@ int snd_hdac_get_connections(struct hdac_device *codec, hda_nid_t nid,
 			     hda_nid_t *conn_list, int max_conns);
 int snd_hdac_get_sub_nodes(struct hdac_device *codec, hda_nid_t nid,
 			   hda_nid_t *start_id);
+unsigned int snd_hdac_stream_format_bps(snd_pcm_format_t format, snd_pcm_subformat_t subformat,
+					unsigned int maxbps);
+unsigned int snd_hdac_stream_format(unsigned int channels, unsigned int bps, unsigned int rate);
+unsigned int snd_hdac_spdif_stream_format(unsigned int channels, unsigned int bps,
+					  unsigned int rate, unsigned short spdif_ctls);
 unsigned int snd_hdac_calc_stream_format(unsigned int rate,
 					 unsigned int channels,
 					 snd_pcm_format_t format,
diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index 5b01a2b9033d..e592469d63b9 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -13,6 +13,7 @@
 #include <sound/hdaudio.h>
 #include <sound/hda_regmap.h>
 #include <sound/pcm.h>
+#include <sound/pcm_params.h>
 #include "local.h"
 
 static void setup_fg_nodes(struct hdac_device *codec);
@@ -724,6 +725,129 @@ static const struct hda_rate_tbl rate_bits[] = {
 	{ 0 } /* terminator */
 };
 
+static snd_pcm_format_t snd_hdac_format_normalize(snd_pcm_format_t format)
+{
+	switch (format) {
+	case SNDRV_PCM_FORMAT_S20_LE:
+	case SNDRV_PCM_FORMAT_S24_LE:
+		return SNDRV_PCM_FORMAT_S32_LE;
+
+	case SNDRV_PCM_FORMAT_U20_LE:
+	case SNDRV_PCM_FORMAT_U24_LE:
+		return SNDRV_PCM_FORMAT_U32_LE;
+
+	case SNDRV_PCM_FORMAT_S20_BE:
+	case SNDRV_PCM_FORMAT_S24_BE:
+		return SNDRV_PCM_FORMAT_S32_BE;
+
+	case SNDRV_PCM_FORMAT_U20_BE:
+	case SNDRV_PCM_FORMAT_U24_BE:
+		return SNDRV_PCM_FORMAT_U32_BE;
+
+	default:
+		return format;
+	}
+}
+
+/**
+ * snd_hdac_stream_format_bps - obtain bits per sample value.
+ * @format:	the PCM format.
+ * @subformat:	the PCM subformat.
+ * @maxbps:	the maximum bits per sample.
+ *
+ * Return: The number of bits per sample.
+ */
+unsigned int snd_hdac_stream_format_bps(snd_pcm_format_t format, snd_pcm_subformat_t subformat,
+					unsigned int maxbps)
+{
+	struct snd_pcm_hw_params params;
+	unsigned int bps;
+
+	memset(&params, 0, sizeof(params));
+
+	params_set_format(&params, snd_hdac_format_normalize(format));
+	snd_mask_set(hw_param_mask(&params, SNDRV_PCM_HW_PARAM_SUBFORMAT),
+		     (__force unsigned int)subformat);
+
+	bps = params_bps(&params);
+	if (maxbps)
+		return min(bps, maxbps);
+	return bps;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_stream_format_bps);
+
+/**
+ * snd_hdac_stream_format - convert format parameters to SDxFMT value.
+ * @channels:	the number of channels.
+ * @bps:	bits per sample.
+ * @rate:	the sample rate.
+ *
+ * Return: The format bitset or zero if invalid.
+ */
+unsigned int snd_hdac_stream_format(unsigned int channels, unsigned int bps, unsigned int rate)
+{
+	unsigned int val = 0;
+	int i;
+
+	for (i = 0; rate_bits[i].hz; i++) {
+		if (rate_bits[i].hz == rate) {
+			val = rate_bits[i].hda_fmt;
+			break;
+		}
+	}
+
+	if (!rate_bits[i].hz)
+		return 0;
+
+	if (channels == 0 || channels > 8)
+		return 0;
+	val |= channels - 1;
+
+	switch (bps) {
+	case 8:
+		val |= AC_FMT_BITS_8;
+		break;
+	case 16:
+		val |= AC_FMT_BITS_16;
+		break;
+	case 20:
+		val |= AC_FMT_BITS_20;
+		break;
+	case 24:
+		val |= AC_FMT_BITS_24;
+		break;
+	case 32:
+		val |= AC_FMT_BITS_32;
+		break;
+	default:
+		return 0;
+	}
+
+	return val;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_stream_format);
+
+/**
+ * snd_hdac_spdif_stream_format - convert format parameters to SDxFMT value.
+ * @channels:	the number of channels.
+ * @bps:	bits per sample.
+ * @rate:	the sample rate.
+ * @spdif_ctls:	HD-audio SPDIF status bits (0 if irrelevant).
+ *
+ * Return: The format bitset or zero if invalid.
+ */
+unsigned int snd_hdac_spdif_stream_format(unsigned int channels, unsigned int bps,
+					  unsigned int rate, unsigned short spdif_ctls)
+{
+	unsigned int val = snd_hdac_stream_format(channels, bps, rate);
+
+	if (val && spdif_ctls & AC_DIG1_NONAUDIO)
+		val |= AC_FMT_TYPE_NON_PCM;
+
+	return val;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_spdif_stream_format);
+
 /**
  * snd_hdac_calc_stream_format - calculate the format bitset
  * @rate: the sample rate
-- 
2.25.1

Re: [RFC PATCH 05/17] ALSA: hda: Upgrade stream-format infrastructure

[Takashi Iwai]

On Fri, 11 Aug 2023 18:48:41 +0200,
Cezary Rojewski wrote:
> 
> 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.

Which compatibility reason?  Those formats should have been never used
for HD-audio codecs at the first place, so shouldn't it rather return
an error?


thanks,

Takashi

Re: [RFC PATCH 05/17] ALSA: hda: Upgrade stream-format infrastructure

[Cezary Rojewski]

On 2023-08-14 4:41 PM, Takashi Iwai wrote:
> On Fri, 11 Aug 2023 18:48:41 +0200,
> Cezary Rojewski wrote:
>>
>> 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.
> 
> Which compatibility reason?  Those formats should have been never used
> for HD-audio codecs at the first place, so shouldn't it rather return
> an error?

In regard to avs-driver we can "force" the S24_LE out, no problem. 
However, I do not believe the same is true for the skylake-driver. I 
agree, S24_LE and its friends should not have been used, but they were.


Czarek

Re: [RFC PATCH 05/17] ALSA: hda: Upgrade stream-format infrastructure

[Takashi Iwai]

On Mon, 14 Aug 2023 16:47:05 +0200,
Cezary Rojewski wrote:
> 
> On 2023-08-14 4:41 PM, Takashi Iwai wrote:
> > On Fri, 11 Aug 2023 18:48:41 +0200,
> > Cezary Rojewski wrote:
> >> 
> >> 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.
> > 
> > Which compatibility reason?  Those formats should have been never used
> > for HD-audio codecs at the first place, so shouldn't it rather return
> > an error?
> 
> In regard to avs-driver we can "force" the S24_LE out, no
> problem. However, I do not believe the same is true for the
> skylake-driver. I agree, S24_LE and its friends should not have been
> used, but they were.

Hm, if those formats are actually passed to the codec side, then it's
rather a bug, I suppose.  It can be used for the controller where DSP
converts the format, though.


Takashi

[RFC PATCH 06/17] ALSA: hda: Switch to new stream-format interface

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 sound/pci/hda/hda_controller.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 406779625fb5..0646ef0afd49 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -151,7 +151,7 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
 	struct azx_dev *azx_dev = get_azx_dev(substream);
 	struct hda_pcm_stream *hinfo = to_hda_pcm_stream(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	unsigned int format_val, stream_tag;
+	unsigned int format_val, stream_tag, bps;
 	int err;
 	struct hda_spdif_out *spdif =
 		snd_hda_spdif_out_of_nid(apcm->codec, hinfo->nid);
@@ -165,11 +165,9 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
 	}
 
 	snd_hdac_stream_reset(azx_stream(azx_dev));
-	format_val = snd_hdac_calc_stream_format(runtime->rate,
-						runtime->channels,
-						runtime->format,
-						hinfo->maxbps,
-						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);
 	if (!format_val) {
 		dev_err(chip->card->dev,
 			"invalid format_val, rate=%d, ch=%d, format=%d\n",
-- 
2.25.1

[RFC PATCH 07/17] ALSA: hda/hdmi: Switch to new stream-format interface

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 sound/pci/hda/patch_hdmi.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 49e9fe09ce88..d6818221427c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1655,7 +1655,6 @@ static void hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 
 #define I915_SILENT_RATE		48000
 #define I915_SILENT_CHANNELS		2
-#define I915_SILENT_FORMAT		SNDRV_PCM_FORMAT_S16_LE
 #define I915_SILENT_FORMAT_BITS	16
 #define I915_SILENT_FMT_MASK		0xf
 
@@ -1668,8 +1667,8 @@ static void silent_stream_enable_i915(struct hda_codec *codec,
 				 per_pin->dev_id, I915_SILENT_RATE);
 
 	/* trigger silent stream generation in hw */
-	format = snd_hdac_calc_stream_format(I915_SILENT_RATE, I915_SILENT_CHANNELS,
-					     I915_SILENT_FORMAT, I915_SILENT_FORMAT_BITS, 0);
+	format = snd_hdac_stream_format(I915_SILENT_CHANNELS, I915_SILENT_FORMAT_BITS,
+					I915_SILENT_RATE);
 	snd_hda_codec_setup_stream(codec, per_pin->cvt_nid,
 				   I915_SILENT_FMT_MASK, I915_SILENT_FMT_MASK, format);
 	usleep_range(100, 200);
-- 
2.25.1

[RFC PATCH 08/17] ALSA: hda/ca0132: Switch to new stream-format interface

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 sound/pci/hda/patch_ca0132.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index 099722ebaed8..faefbcceb5ed 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -3021,8 +3021,7 @@ static int dma_convert_to_hda_format(struct hda_codec *codec,
 {
 	unsigned int format_val;
 
-	format_val = snd_hdac_calc_stream_format(sample_rate,
-				channels, SNDRV_PCM_FORMAT_S32_LE, 32, 0);
+	format_val = snd_hdac_stream_format(channels, 32, sample_rate);
 
 	if (hda_format)
 		*hda_format = (unsigned short)format_val;
-- 
2.25.1

[RFC PATCH 09/17] ASoC: codecs: hda: Switch to new stream-format interface

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 sound/soc/codecs/hda-dai.c | 6 ++++--
 sound/soc/codecs/hda.c     | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/hda-dai.c b/sound/soc/codecs/hda-dai.c
index 5371ff086261..20b070c66c6f 100644
--- a/sound/soc/codecs/hda-dai.c
+++ b/sound/soc/codecs/hda-dai.c
@@ -76,13 +76,15 @@ static int hda_codec_dai_prepare(struct snd_pcm_substream *substream, struct snd
 	struct hdac_stream *stream;
 	struct hda_codec *codec;
 	unsigned int format;
+	unsigned int bps;
 	int ret;
 
 	codec = dev_to_hda_codec(dai->dev);
 	stream = substream->runtime->private_data;
 	stream_info = snd_soc_dai_get_dma_data(dai, substream);
-	format = snd_hdac_calc_stream_format(runtime->rate, runtime->channels, runtime->format,
-					     runtime->sample_bits, 0);
+
+	bps = snd_hdac_stream_format_bps(runtime->format, runtime->subformat, stream_info->maxbps);
+	format = snd_hdac_stream_format(runtime->channels, bps, runtime->rate);
 
 	ret = snd_hda_codec_prepare(codec, stream_info, stream->stream_tag, format, substream);
 	if (ret < 0) {
diff --git a/sound/soc/codecs/hda.c b/sound/soc/codecs/hda.c
index 7f53b1981a14..8a9d0674555e 100644
--- a/sound/soc/codecs/hda.c
+++ b/sound/soc/codecs/hda.c
@@ -52,6 +52,7 @@ static int hda_codec_create_dais(struct hda_codec *codec, int pcm_count,
 		stream->channels_max = pcm->stream[dir].channels_max;
 		stream->rates = pcm->stream[dir].rates;
 		stream->formats = pcm->stream[dir].formats;
+		stream->subformats = pcm->stream[dir].subformats;
 		stream->sig_bits = pcm->stream[dir].maxbps;
 
 capture_dais:
@@ -71,6 +72,7 @@ static int hda_codec_create_dais(struct hda_codec *codec, int pcm_count,
 		stream->channels_max = pcm->stream[dir].channels_max;
 		stream->rates = pcm->stream[dir].rates;
 		stream->formats = pcm->stream[dir].formats;
+		stream->subformats = pcm->stream[dir].subformats;
 		stream->sig_bits = pcm->stream[dir].maxbps;
 	}
 
-- 
2.25.1

[RFC PATCH 10/17] ASoC: codecs: hdac_hda: Switch to new stream-format interface

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 sound/soc/codecs/hdac_hda.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
index be66853afbe2..7e308dac07d4 100644
--- a/sound/soc/codecs/hdac_hda.c
+++ b/sound/soc/codecs/hdac_hda.c
@@ -207,18 +207,16 @@ static int hdac_hda_dai_hw_params(struct snd_pcm_substream *substream,
 	struct hdac_hda_priv *hda_pvt;
 	unsigned int format_val;
 	unsigned int maxbps;
+	unsigned int bps;
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		maxbps = dai->driver->playback.sig_bits;
 	else
 		maxbps = dai->driver->capture.sig_bits;
+	bps = snd_hdac_stream_format_bps(params_format(params), SNDRV_PCM_SUBFORMAT_STD, maxbps);
 
 	hda_pvt = snd_soc_component_get_drvdata(component);
-	format_val = snd_hdac_calc_stream_format(params_rate(params),
-						 params_channels(params),
-						 params_format(params),
-						 maxbps,
-						 0);
+	format_val = snd_hdac_stream_format(params_channels(params), bps, params_rate(params));
 	if (!format_val) {
 		dev_err(dai->dev,
 			"invalid format_val, rate=%d, ch=%d, format=%d, maxbps=%d\n",
-- 
2.25.1

[RFC PATCH 11/17] ASoC: codecs: hdac_hdmi: Switch to new stream-format interface

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index fb3f8a565485..44837462dcd3 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -468,13 +468,14 @@ static int hdac_hdmi_set_hw_params(struct snd_pcm_substream *substream,
 	struct hdac_hdmi_priv *hdmi = snd_soc_dai_get_drvdata(dai);
 	struct hdac_hdmi_dai_port_map *dai_map;
 	struct hdac_hdmi_pcm *pcm;
+	unsigned int bps;
 	int format;
 
 	dai_map = &hdmi->dai_map[dai->id];
 
-	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,
+					 dai->driver->playback.sig_bits);
+	format = snd_hdac_stream_format(params_channels(hparams), bps, params_rate(hparams));
 
 	pcm = hdac_hdmi_get_pcm_from_cvt(hdmi, dai_map->cvt);
 	if (!pcm)
-- 
2.25.1

[RFC PATCH 12/17] ASoC: Intel Skylake: Switch to new stream-format interface

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 sound/soc/intel/skylake/skl-pcm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index a4209d88b0c6..b87f046f1cad 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -127,6 +127,7 @@ int skl_pcm_host_dma_prepare(struct device *dev, struct skl_pipe_params *params)
 	unsigned int format_val;
 	struct hdac_stream *hstream;
 	struct hdac_ext_stream *stream;
+	unsigned int bps;
 	int err;
 
 	hstream = snd_hdac_get_stream(bus, params->stream,
@@ -137,8 +138,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);
+	format_val = snd_hdac_stream_format(params->ch, bps, params->s_freq);
 
 	dev_dbg(dev, "format_val=%d, rate=%d, ch=%d, format=%d\n",
 		format_val, params->s_freq, params->ch, params->format);
@@ -176,6 +177,7 @@ int skl_pcm_link_dma_prepare(struct device *dev, struct skl_pipe_params *params)
 	struct hdac_ext_stream *stream;
 	struct hdac_ext_link *link;
 	unsigned char stream_tag;
+	unsigned int bps;
 
 	hstream = snd_hdac_get_stream(bus, params->stream,
 					params->link_dma_id + 1);
@@ -184,8 +186,9 @@ int skl_pcm_link_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->link_bps, 0);
+
+	bps = snd_hdac_stream_format_bps(params->format, SNDRV_PCM_SUBFORMAT_STD, params->link_bps);
+	format_val = snd_hdac_stream_format(params->ch, bps, params->s_freq);
 
 	dev_dbg(dev, "format_val=%d, rate=%d, ch=%d, format=%d\n",
 		format_val, params->s_freq, params->ch, params->format);
-- 
2.25.1

[RFC PATCH 13/17] ASoC: SOF: Intel: Switch to new stream-format interface

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 sound/soc/sof/intel/hda-dai-ops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sof/intel/hda-dai-ops.c b/sound/soc/sof/intel/hda-dai-ops.c
index f3513796c189..00703999e91b 100644
--- a/sound/soc/sof/intel/hda-dai-ops.c
+++ b/sound/soc/sof/intel/hda-dai-ops.c
@@ -194,14 +194,15 @@ static unsigned int hda_calc_stream_format(struct snd_sof_dev *sdev,
 	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
 	unsigned int link_bps;
 	unsigned int format_val;
+	unsigned int bps;
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		link_bps = codec_dai->driver->playback.sig_bits;
 	else
 		link_bps = codec_dai->driver->capture.sig_bits;
+	bps = snd_hdac_stream_format_bps(params_format(params), SNDRV_PCM_SUBFORMAT_STD, link_bps);
 
-	format_val = snd_hdac_calc_stream_format(params_rate(params), params_channels(params),
-						 params_format(params), link_bps, 0);
+	format_val = snd_hdac_stream_format(params_channels(params), bps, params_rate(params));
 
 	dev_dbg(sdev->dev, "format_val=%#x, rate=%d, ch=%d, format=%d\n", format_val,
 		params_rate(params), params_channels(params), params_format(params));
-- 
2.25.1

Re: [RFC PATCH 13/17] ASoC: SOF: Intel: Switch to new stream-format interface

[Pierre-Louis Bossart]

On 8/11/23 11:48, Cezary Rojewski wrote:
> To provide option for selecting different bit-per-sample than just the
> maximum one, use the new format calculation mechanism.

Can you remind me what the issue is in selecting the maximum resolution?

Isn't it a good thing to select the maximum resolution when possible so
as to provide more headroom and prevent clipping? Usually we try to make
sure the resolution becomes limited when we reach the analog parts. I am
not sure I see a compelling reason to reduce the resolution on the host
side.

I am also not sure what this patch actually changes, given that the
firmware actually converts everything to the full 32-bit resolution.

I must be missing something on the problem statement.

> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  sound/soc/sof/intel/hda-dai-ops.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/sof/intel/hda-dai-ops.c b/sound/soc/sof/intel/hda-dai-ops.c
> index f3513796c189..00703999e91b 100644
> --- a/sound/soc/sof/intel/hda-dai-ops.c
> +++ b/sound/soc/sof/intel/hda-dai-ops.c
> @@ -194,14 +194,15 @@ static unsigned int hda_calc_stream_format(struct snd_sof_dev *sdev,
>  	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
>  	unsigned int link_bps;
>  	unsigned int format_val;
> +	unsigned int bps;
>  
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>  		link_bps = codec_dai->driver->playback.sig_bits;
>  	else
>  		link_bps = codec_dai->driver->capture.sig_bits;
> +	bps = snd_hdac_stream_format_bps(params_format(params), SNDRV_PCM_SUBFORMAT_STD, link_bps);

I can't say I fully understand the difference between 'bps' and
'link_bps'. The naming is far from self-explanatory just by looking at
the code.

> -	format_val = snd_hdac_calc_stream_format(params_rate(params), params_channels(params),
> -						 params_format(params), link_bps, 0);
> +	format_val = snd_hdac_stream_format(params_channels(params), bps, params_rate(params));
>  
>  	dev_dbg(sdev->dev, "format_val=%#x, rate=%d, ch=%d, format=%d\n", format_val,
>  		params_rate(params), params_channels(params), params_format(params));

Re: [RFC PATCH 13/17] ASoC: SOF: Intel: Switch to new stream-format interface

[Cezary Rojewski]

On 2023-08-11 8:21 PM, Pierre-Louis Bossart wrote:
> On 8/11/23 11:48, Cezary Rojewski wrote:
>> To provide option for selecting different bit-per-sample than just the
>> maximum one, use the new format calculation mechanism.
> 
> Can you remind me what the issue is in selecting the maximum resolution?
> 
> Isn't it a good thing to select the maximum resolution when possible so
> as to provide more headroom and prevent clipping? Usually we try to make
> sure the resolution becomes limited when we reach the analog parts. I am
> not sure I see a compelling reason to reduce the resolution on the host
> side.

Maximum resolution is still the default, nothing changes there. 
Moreover, new subformat options are not added to any driver apart from 
the avs-driver.

The patchset provides _an option_ to change bits-per-sample. Right now 
there's no option to do that so the hardware - here, SDxFMT and PPLCxFMT 
- is not tested. We have enough recommended flows already. Frankly there 
is one for SDxFMT for the APL-based platforms (or BXT-based if one 
prefers that naming) present within snd_hda_intel and DSP drivers alike.

> I am also not sure what this patch actually changes, given that the
> firmware actually converts everything to the full 32-bit resolution.

The issue does not concern firmware, so we leave firmware out of the 
discussion - this is purely about hardware capabilities.

> I must be missing something on the problem statement.
> 
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   sound/soc/sof/intel/hda-dai-ops.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/sof/intel/hda-dai-ops.c b/sound/soc/sof/intel/hda-dai-ops.c
>> index f3513796c189..00703999e91b 100644
>> --- a/sound/soc/sof/intel/hda-dai-ops.c
>> +++ b/sound/soc/sof/intel/hda-dai-ops.c
>> @@ -194,14 +194,15 @@ static unsigned int hda_calc_stream_format(struct snd_sof_dev *sdev,
>>   	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
>>   	unsigned int link_bps;
>>   	unsigned int format_val;
>> +	unsigned int bps;
>>   
>>   	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>   		link_bps = codec_dai->driver->playback.sig_bits;
>>   	else
>>   		link_bps = codec_dai->driver->capture.sig_bits;
>> +	bps = snd_hdac_stream_format_bps(params_format(params), SNDRV_PCM_SUBFORMAT_STD, link_bps);
> 
> I can't say I fully understand the difference between 'bps' and
> 'link_bps'. The naming is far from self-explanatory just by looking at
> the code.

There's none. I just didn't reuse the 'link_bps' variable. I can reuse 
it if you like.

>> -	format_val = snd_hdac_calc_stream_format(params_rate(params), params_channels(params),
>> -						 params_format(params), link_bps, 0);
>> +	format_val = snd_hdac_stream_format(params_channels(params), bps, params_rate(params));
>>   
>>   	dev_dbg(sdev->dev, "format_val=%#x, rate=%d, ch=%d, format=%d\n", format_val,
>>   		params_rate(params), params_channels(params), params_format(params));

Re: [RFC PATCH 13/17] ASoC: SOF: Intel: Switch to new stream-format interface

[Pierre-Louis Bossart]

On 8/14/23 05:51, Cezary Rojewski wrote:
> On 2023-08-11 8:21 PM, Pierre-Louis Bossart wrote:
>> On 8/11/23 11:48, Cezary Rojewski wrote:
>>> To provide option for selecting different bit-per-sample than just the
>>> maximum one, use the new format calculation mechanism.
>>
>> Can you remind me what the issue is in selecting the maximum resolution?
>>
>> Isn't it a good thing to select the maximum resolution when possible so
>> as to provide more headroom and prevent clipping? Usually we try to make
>> sure the resolution becomes limited when we reach the analog parts. I am
>> not sure I see a compelling reason to reduce the resolution on the host
>> side.
> 
> Maximum resolution is still the default, nothing changes there.
> Moreover, new subformat options are not added to any driver apart from
> the avs-driver.

What's so special about this driver that it needs more capabilities for
a standard interface?

> The patchset provides _an option_ to change bits-per-sample. Right now
> there's no option to do that so the hardware - here, SDxFMT and PPLCxFMT
> - is not tested. We have enough recommended flows already. Frankly there
> is one for SDxFMT for the APL-based platforms (or BXT-based if one
> prefers that naming) present within snd_hda_intel and DSP drivers alike.
> 
>> I am also not sure what this patch actually changes, given that the
>> firmware actually converts everything to the full 32-bit resolution.
> 
> The issue does not concern firmware, so we leave firmware out of the
> discussion - this is purely about hardware capabilities.

I don't see how you can leave firmware aside, if the hardware
configuration is selected to be based on 24 bits and the firmware
generated 32 there's clearly a mismatch.

If this is saying that we are adding an option that will never be used,
then why bother?

Lost in space on this one.

>> I must be missing something on the problem statement.
>>
>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>> ---
>>>   sound/soc/sof/intel/hda-dai-ops.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/sound/soc/sof/intel/hda-dai-ops.c
>>> b/sound/soc/sof/intel/hda-dai-ops.c
>>> index f3513796c189..00703999e91b 100644
>>> --- a/sound/soc/sof/intel/hda-dai-ops.c
>>> +++ b/sound/soc/sof/intel/hda-dai-ops.c
>>> @@ -194,14 +194,15 @@ static unsigned int
>>> hda_calc_stream_format(struct snd_sof_dev *sdev,
>>>       struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
>>>       unsigned int link_bps;
>>>       unsigned int format_val;
>>> +    unsigned int bps;
>>>         if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>>           link_bps = codec_dai->driver->playback.sig_bits;
>>>       else
>>>           link_bps = codec_dai->driver->capture.sig_bits;
>>> +    bps = snd_hdac_stream_format_bps(params_format(params),
>>> SNDRV_PCM_SUBFORMAT_STD, link_bps);
>>
>> I can't say I fully understand the difference between 'bps' and
>> 'link_bps'. The naming is far from self-explanatory just by looking at
>> the code.
> 
> There's none. I just didn't reuse the 'link_bps' variable. I can reuse
> it if you like.
> 
>>> -    format_val = snd_hdac_calc_stream_format(params_rate(params),
>>> params_channels(params),
>>> -                         params_format(params), link_bps, 0);
>>> +    format_val = snd_hdac_stream_format(params_channels(params),
>>> bps, params_rate(params));
>>>         dev_dbg(sdev->dev, "format_val=%#x, rate=%d, ch=%d,
>>> format=%d\n", format_val,
>>>           params_rate(params), params_channels(params),
>>> params_format(params));

Re: [RFC PATCH 13/17] ASoC: SOF: Intel: Switch to new stream-format interface

[Cezary Rojewski]

On 2023-08-14 4:01 PM, Pierre-Louis Bossart wrote:
> On 8/14/23 05:51, Cezary Rojewski wrote:
>> On 2023-08-11 8:21 PM, Pierre-Louis Bossart wrote:
>>> On 8/11/23 11:48, Cezary Rojewski wrote:
>>>> To provide option for selecting different bit-per-sample than just the
>>>> maximum one, use the new format calculation mechanism.
>>>
>>> Can you remind me what the issue is in selecting the maximum resolution?
>>>
>>> Isn't it a good thing to select the maximum resolution when possible so
>>> as to provide more headroom and prevent clipping? Usually we try to make
>>> sure the resolution becomes limited when we reach the analog parts. I am
>>> not sure I see a compelling reason to reduce the resolution on the host
>>> side.
>>
>> Maximum resolution is still the default, nothing changes there.
>> Moreover, new subformat options are not added to any driver apart from
>> the avs-driver.
> 
> What's so special about this driver that it needs more capabilities for
> a standard interface?

This is kind of an off-topic question.

While maintaining status quo from user perspective, we want to test the 
hardware with full-stack, just like it's the case on Windows side. 
Tested hardware yields less recommended flows.

>> The patchset provides _an option_ to change bits-per-sample. Right now
>> there's no option to do that so the hardware - here, SDxFMT and PPLCxFMT
>> - is not tested. We have enough recommended flows already. Frankly there
>> is one for SDxFMT for the APL-based platforms (or BXT-based if one
>> prefers that naming) present within snd_hda_intel and DSP drivers alike.
>>
>>> I am also not sure what this patch actually changes, given that the
>>> firmware actually converts everything to the full 32-bit resolution.
>>
>> The issue does not concern firmware, so we leave firmware out of the
>> discussion - this is purely about hardware capabilities.
> 
> I don't see how you can leave firmware aside, if the hardware
> configuration is selected to be based on 24 bits and the firmware
> generated 32 there's clearly a mismatch.
> 
> If this is saying that we are adding an option that will never be used,
> then why bother?
> 
> Lost in space on this one.

We are still on planet Earth though. Many codecs present on the market 
support more than just 24/32 format. It is a valid testcase to check if 
indeed the exposed functionality works.

In regard to firmware, please note that the AudioDSP firmware has no 
direct access to the HOST space, so it cannot alter SDxFMT and PPLCxFMT 
on its own. Hardcoding particular YYYxFMT value restricts testing 
capabilities. The firmware expects that provided valid and container bit 
depths values (copier's INIT_INSTANCE) match YYYxFMT the software had 
assigned.

>>> I must be missing something on the problem statement.
>>>
>>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>>> ---
>>>>    sound/soc/sof/intel/hda-dai-ops.c | 5 +++--
>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/sound/soc/sof/intel/hda-dai-ops.c
>>>> b/sound/soc/sof/intel/hda-dai-ops.c
>>>> index f3513796c189..00703999e91b 100644
>>>> --- a/sound/soc/sof/intel/hda-dai-ops.c
>>>> +++ b/sound/soc/sof/intel/hda-dai-ops.c
>>>> @@ -194,14 +194,15 @@ static unsigned int
>>>> hda_calc_stream_format(struct snd_sof_dev *sdev,
>>>>        struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
>>>>        unsigned int link_bps;
>>>>        unsigned int format_val;
>>>> +    unsigned int bps;
>>>>          if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>>>            link_bps = codec_dai->driver->playback.sig_bits;
>>>>        else
>>>>            link_bps = codec_dai->driver->capture.sig_bits;
>>>> +    bps = snd_hdac_stream_format_bps(params_format(params),
>>>> SNDRV_PCM_SUBFORMAT_STD, link_bps);
>>>
>>> I can't say I fully understand the difference between 'bps' and
>>> 'link_bps'. The naming is far from self-explanatory just by looking at
>>> the code.
>>
>> There's none. I just didn't reuse the 'link_bps' variable. I can reuse
>> it if you like.
>>
>>>> -    format_val = snd_hdac_calc_stream_format(params_rate(params),
>>>> params_channels(params),
>>>> -                         params_format(params), link_bps, 0);
>>>> +    format_val = snd_hdac_stream_format(params_channels(params),
>>>> bps, params_rate(params));
>>>>          dev_dbg(sdev->dev, "format_val=%#x, rate=%d, ch=%d,
>>>> format=%d\n", format_val,
>>>>            params_rate(params), params_channels(params),
>>>> params_format(params));

Re: [RFC PATCH 13/17] ASoC: SOF: Intel: Switch to new stream-format interface

[Pierre-Louis Bossart]

>>>>> To provide option for selecting different bit-per-sample than just the
>>>>> maximum one, use the new format calculation mechanism.
>>>>
>>>> Can you remind me what the issue is in selecting the maximum
>>>> resolution?
>>>>
>>>> Isn't it a good thing to select the maximum resolution when possible so
>>>> as to provide more headroom and prevent clipping? Usually we try to
>>>> make
>>>> sure the resolution becomes limited when we reach the analog parts.
>>>> I am
>>>> not sure I see a compelling reason to reduce the resolution on the host
>>>> side.
>>>
>>> Maximum resolution is still the default, nothing changes there.
>>> Moreover, new subformat options are not added to any driver apart from
>>> the avs-driver.
>>
>> What's so special about this driver that it needs more capabilities for
>> a standard interface?
> 
> This is kind of an off-topic question.
> 
> While maintaining status quo from user perspective, we want to test the
> hardware with full-stack, just like it's the case on Windows side.
> Tested hardware yields less recommended flows.

yeah, but at some point you have to draw a line. It's not because the
codec exposes all kinds of capabilities that you want the host driver to
enable each single possible option.

Take I2S codecs for example, there are countless degrees of freedom
between 18, 20, 24, 32 bits and all sorts of formatting (DSP_X, I2S),
slot size, etc. We use the same subsets in 90% of the cases.

>>> The patchset provides _an option_ to change bits-per-sample. Right now
>>> there's no option to do that so the hardware - here, SDxFMT and PPLCxFMT
>>> - is not tested. We have enough recommended flows already. Frankly there
>>> is one for SDxFMT for the APL-based platforms (or BXT-based if one
>>> prefers that naming) present within snd_hda_intel and DSP drivers alike.
>>>
>>>> I am also not sure what this patch actually changes, given that the
>>>> firmware actually converts everything to the full 32-bit resolution.
>>>
>>> The issue does not concern firmware, so we leave firmware out of the
>>> discussion - this is purely about hardware capabilities.
>>
>> I don't see how you can leave firmware aside, if the hardware
>> configuration is selected to be based on 24 bits and the firmware
>> generated 32 there's clearly a mismatch.
>>
>> If this is saying that we are adding an option that will never be used,
>> then why bother?
>>
>> Lost in space on this one.
> 
> We are still on planet Earth though. Many codecs present on the market
> support more than just 24/32 format. It is a valid testcase to check if
> indeed the exposed functionality works.

Sure, if you have spare cycles you can test all kinds of things, but the
impact on products will be very limited. No one in their right mind is
going to use 20 bits even if the codec advertises support for it. The
recommended practice is to use the maximum resolution on the host side.

It's not because we *can* use a lower resolution that we *should*.

> In regard to firmware, please note that the AudioDSP firmware has no
> direct access to the HOST space, so it cannot alter SDxFMT and PPLCxFMT
> on its own. Hardcoding particular YYYxFMT value restricts testing
> capabilities. The firmware expects that provided valid and container bit
> depths values (copier's INIT_INSTANCE) match YYYxFMT the software had
> assigned.

what could possibly go wrong here...completely different layers that
need to be joined to reconcile codec-specific information. D'oh.

I am not going to object further to these patches, I just don't see them
as having any material impact on any of the existing/future products.

[RFC PATCH 14/17] ASoC: Intel: avs: Switch to new stream-format interface

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/loader.c |  4 ++--
 sound/soc/intel/avs/path.c   |  2 +-
 sound/soc/intel/avs/pcm.c    | 19 ++++++++++++++-----
 sound/soc/intel/avs/probes.c |  3 +--
 4 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/sound/soc/intel/avs/loader.c b/sound/soc/intel/avs/loader.c
index b81b555846e6..2c682e844d22 100644
--- a/sound/soc/intel/avs/loader.c
+++ b/sound/soc/intel/avs/loader.c
@@ -373,7 +373,7 @@ int avs_hda_load_basefw(struct avs_dev *adev, struct firmware *fw)
 	hstream = hdac_stream(estream);
 
 	/* code loading performed with default format */
-	sdfmt = snd_hdac_calc_stream_format(48000, 1, SNDRV_PCM_FORMAT_S32_LE, 32, 0);
+	sdfmt = snd_hdac_stream_format(1, 32, 48000);
 	ret = snd_hdac_dsp_prepare(hstream, sdfmt, fw->size, &dmab);
 	if (ret < 0)
 		goto release_stream;
@@ -440,7 +440,7 @@ int avs_hda_load_library(struct avs_dev *adev, struct firmware *lib, u32 id)
 	stream = hdac_stream(estream);
 
 	/* code loading performed with default format */
-	sdfmt = snd_hdac_calc_stream_format(48000, 1, SNDRV_PCM_FORMAT_S32_LE, 32, 0);
+	sdfmt = snd_hdac_stream_format(1, 32, 48000);
 	ret = snd_hdac_dsp_prepare(stream, sdfmt, lib->size, &dmab);
 	if (ret < 0)
 		goto release_stream;
diff --git a/sound/soc/intel/avs/path.c b/sound/soc/intel/avs/path.c
index 494e357e3b12..0475aa9c85ce 100644
--- a/sound/soc/intel/avs/path.c
+++ b/sound/soc/intel/avs/path.c
@@ -88,7 +88,7 @@ static bool avs_test_hw_params(struct snd_pcm_hw_params *params,
 	return (params_rate(params) == fmt->sampling_freq &&
 		params_channels(params) == fmt->num_channels &&
 		params_physical_width(params) == fmt->bit_depth &&
-		params_width(params) == fmt->valid_bit_depth);
+		params_bps(params) == fmt->valid_bit_depth);
 }
 
 static struct avs_tplg_path *
diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c
index 7f38cfc86492..fa9d3292fe82 100644
--- a/sound/soc/intel/avs/pcm.c
+++ b/sound/soc/intel/avs/pcm.c
@@ -336,20 +336,25 @@ static int avs_dai_hda_be_prepare(struct snd_pcm_substream *substream, struct sn
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	struct hdac_ext_stream *link_stream = runtime->private_data;
+	struct snd_soc_pcm_stream *stream_info;
+	struct hdac_ext_stream *link_stream;
 	struct hdac_ext_link *link;
 	struct hda_codec *codec;
 	struct hdac_bus *bus;
 	unsigned int format_val;
+	unsigned int bps;
 	int ret;
 
+	link_stream = runtime->private_data;
 	if (link_stream->link_prepared)
 		return 0;
 
 	codec = dev_to_hda_codec(asoc_rtd_to_codec(rtd, 0)->dev);
 	bus = &codec->bus->core;
-	format_val = snd_hdac_calc_stream_format(runtime->rate, runtime->channels, runtime->format,
-						 runtime->sample_bits, 0);
+	stream_info = snd_soc_dai_get_pcm_stream(dai, substream->stream);
+	bps = snd_hdac_stream_format_bps(runtime->format, runtime->subformat,
+					 stream_info->sig_bits);
+	format_val = snd_hdac_stream_format(runtime->channels, bps, runtime->rate);
 
 	snd_hdac_ext_stream_reset(link_stream);
 	snd_hdac_ext_stream_setup(link_stream, format_val);
@@ -602,11 +607,13 @@ static int avs_dai_fe_hw_free(struct snd_pcm_substream *substream, struct snd_so
 static int avs_dai_fe_prepare(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_soc_pcm_stream *stream_info;
 	struct avs_dma_data *data;
 	struct avs_dev *adev = to_avs_dev(dai->dev);
 	struct hdac_ext_stream *host_stream;
 	struct hdac_bus *bus;
 	unsigned int format_val;
+	unsigned int bps;
 	int ret;
 
 	data = snd_soc_dai_get_dma_data(dai, substream);
@@ -618,8 +625,10 @@ static int avs_dai_fe_prepare(struct snd_pcm_substream *substream, struct snd_so
 	bus = hdac_stream(host_stream)->bus;
 	snd_hdac_stream_reset(hdac_stream(host_stream));
 
-	format_val = snd_hdac_calc_stream_format(runtime->rate, runtime->channels, runtime->format,
-						 runtime->sample_bits, 0);
+	stream_info = snd_soc_dai_get_pcm_stream(dai, substream->stream);
+	bps = snd_hdac_stream_format_bps(runtime->format, runtime->subformat,
+					 stream_info->sig_bits);
+	format_val = snd_hdac_stream_format(runtime->channels, bps, runtime->rate);
 
 	ret = snd_hdac_stream_set_params(hdac_stream(host_stream), format_val);
 	if (ret < 0)
diff --git a/sound/soc/intel/avs/probes.c b/sound/soc/intel/avs/probes.c
index 275928281c6c..1e67b6549be7 100644
--- a/sound/soc/intel/avs/probes.c
+++ b/sound/soc/intel/avs/probes.c
@@ -140,8 +140,7 @@ static int avs_probe_compr_set_params(struct snd_compr_stream *cstream,
 	bps = snd_pcm_format_physical_width(format);
 	if (bps < 0)
 		return bps;
-	format_val = snd_hdac_calc_stream_format(params->codec.sample_rate, params->codec.ch_out,
-						 format, bps, 0);
+	format_val = snd_hdac_stream_format(params->codec.ch_out, bps, params->codec.sample_rate);
 	ret = snd_hdac_stream_set_params(hdac_stream(host_stream), format_val);
 	if (ret < 0)
 		return ret;
-- 
2.25.1

[RFC PATCH 15/17] ALSA: hda: Drop snd_hdac_calc_stream_format()

[Cezary Rojewski]

There are no users of the function.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/sound/hdaudio.h |  5 ----
 sound/hda/hdac_device.c | 61 -----------------------------------------
 2 files changed, 66 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index f6d76c23ffa4..5a4bbcec6cc5 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -144,11 +144,6 @@ unsigned int snd_hdac_stream_format_bps(snd_pcm_format_t format, snd_pcm_subform
 unsigned int snd_hdac_stream_format(unsigned int channels, unsigned int bps, unsigned int rate);
 unsigned int snd_hdac_spdif_stream_format(unsigned int channels, unsigned int bps,
 					  unsigned int rate, unsigned short spdif_ctls);
-unsigned int snd_hdac_calc_stream_format(unsigned int rate,
-					 unsigned int channels,
-					 snd_pcm_format_t format,
-					 unsigned int maxbps,
-					 unsigned short spdif_ctls);
 int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid,
 				 u32 *ratesp, u64 *formatsp, u64 *subformatsp, unsigned int *bpsp);
 bool snd_hdac_is_supported_format(struct hdac_device *codec, hda_nid_t nid,
diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index e592469d63b9..122c773d14eb 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -848,67 +848,6 @@ unsigned int snd_hdac_spdif_stream_format(unsigned int channels, unsigned int bp
 }
 EXPORT_SYMBOL_GPL(snd_hdac_spdif_stream_format);
 
-/**
- * snd_hdac_calc_stream_format - calculate the format bitset
- * @rate: the sample rate
- * @channels: the number of channels
- * @format: the PCM format (SNDRV_PCM_FORMAT_XXX)
- * @maxbps: the max. bps
- * @spdif_ctls: HD-audio SPDIF status bits (0 if irrelevant)
- *
- * Calculate the format bitset from the given rate, channels and th PCM format.
- *
- * Return zero if invalid.
- */
-unsigned int snd_hdac_calc_stream_format(unsigned int rate,
-					 unsigned int channels,
-					 snd_pcm_format_t format,
-					 unsigned int maxbps,
-					 unsigned short spdif_ctls)
-{
-	int i;
-	unsigned int val = 0;
-
-	for (i = 0; rate_bits[i].hz; i++)
-		if (rate_bits[i].hz == rate) {
-			val = rate_bits[i].hda_fmt;
-			break;
-		}
-	if (!rate_bits[i].hz)
-		return 0;
-
-	if (channels == 0 || channels > 8)
-		return 0;
-	val |= channels - 1;
-
-	switch (snd_pcm_format_width(format)) {
-	case 8:
-		val |= AC_FMT_BITS_8;
-		break;
-	case 16:
-		val |= AC_FMT_BITS_16;
-		break;
-	case 20:
-	case 24:
-	case 32:
-		if (maxbps >= 32 || format == SNDRV_PCM_FORMAT_FLOAT_LE)
-			val |= AC_FMT_BITS_32;
-		else if (maxbps >= 24)
-			val |= AC_FMT_BITS_24;
-		else
-			val |= AC_FMT_BITS_20;
-		break;
-	default:
-		return 0;
-	}
-
-	if (spdif_ctls & AC_DIG1_NONAUDIO)
-		val |= AC_FMT_TYPE_NON_PCM;
-
-	return val;
-}
-EXPORT_SYMBOL_GPL(snd_hdac_calc_stream_format);
-
 static unsigned int query_pcm_param(struct hdac_device *codec, hda_nid_t nid)
 {
 	unsigned int val = 0;
-- 
2.25.1

[RFC PATCH 16/17] ASoC: Intel: avs: Unhardcode HDAudio BE DAI drivers description

[Cezary Rojewski]

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.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/pcm.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c
index fa9d3292fe82..31737133b991 100644
--- a/sound/soc/intel/avs/pcm.c
+++ b/sound/soc/intel/avs/pcm.c
@@ -1413,27 +1413,6 @@ int avs_i2s_platform_register(struct avs_dev *adev, const char *name, unsigned l
 	return avs_soc_component_register(adev->dev, name, &avs_component_driver, cpus, cpu_count);
 }
 
-/* HD-Audio CPU DAI template */
-static const struct snd_soc_dai_driver hda_cpu_dai = {
-	.ops = &avs_dai_hda_be_ops,
-	.playback = {
-		.channels_min	= 1,
-		.channels_max	= 8,
-		.rates		= SNDRV_PCM_RATE_8000_192000,
-		.formats	= SNDRV_PCM_FMTBIT_S16_LE |
-				  SNDRV_PCM_FMTBIT_S24_LE |
-				  SNDRV_PCM_FMTBIT_S32_LE,
-	},
-	.capture = {
-		.channels_min	= 1,
-		.channels_max	= 8,
-		.rates		= SNDRV_PCM_RATE_8000_192000,
-		.formats	= SNDRV_PCM_FMTBIT_S16_LE |
-				  SNDRV_PCM_FMTBIT_S24_LE |
-				  SNDRV_PCM_FMTBIT_S32_LE,
-	},
-};
-
 static void avs_component_hda_unregister_dais(struct snd_soc_component *component)
 {
 	struct snd_soc_acpi_mach *mach;
@@ -1490,8 +1469,8 @@ static int avs_component_hda_probe(struct snd_soc_component *component)
 	for (i = 0; i < pcm_count; i++, pcm = list_next_entry(pcm, list)) {
 		struct snd_soc_dai *dai;
 
-		memcpy(&dais[i], &hda_cpu_dai, sizeof(*dais));
 		dais[i].id = i;
+		dais[i].ops = &avs_dai_hda_be_ops;
 		dais[i].name = devm_kasprintf(component->dev, GFP_KERNEL,
 					      "%s-cpu%d", cname, i);
 		if (!dais[i].name) {
@@ -1507,6 +1486,13 @@ static int avs_component_hda_probe(struct snd_soc_component *component)
 				ret = -ENOMEM;
 				goto exit;
 			}
+
+			dais[i].playback.formats = pcm->stream[0].formats;
+			dais[i].playback.subformats = pcm->stream[0].subformats;
+			dais[i].playback.rates = pcm->stream[0].rates;
+			dais[i].playback.channels_min = pcm->stream[0].channels_min;
+			dais[i].playback.channels_max = pcm->stream[0].channels_max;
+			dais[i].playback.sig_bits = pcm->stream[0].maxbps;
 		}
 
 		if (pcm->stream[1].substreams) {
@@ -1517,6 +1503,13 @@ static int avs_component_hda_probe(struct snd_soc_component *component)
 				ret = -ENOMEM;
 				goto exit;
 			}
+
+			dais[i].capture.formats = pcm->stream[1].formats;
+			dais[i].capture.subformats = pcm->stream[1].subformats;
+			dais[i].capture.rates = pcm->stream[1].rates;
+			dais[i].capture.channels_min = pcm->stream[1].channels_min;
+			dais[i].capture.channels_max = pcm->stream[1].channels_max;
+			dais[i].capture.sig_bits = pcm->stream[1].maxbps;
 		}
 
 		dai = snd_soc_register_dai(component, &dais[i], false);
-- 
2.25.1

[RFC PATCH 17/17] ASoC: Intel: avs: Kill S24_LE in HDAudio streaming

[Cezary Rojewski]

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 <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/pcm.c      | 4 +++-
 sound/soc/intel/avs/topology.c | 9 ++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c
index 31737133b991..83cdce8fead4 100644
--- a/sound/soc/intel/avs/pcm.c
+++ b/sound/soc/intel/avs/pcm.c
@@ -1160,8 +1160,10 @@ static const struct snd_pcm_hardware avs_pcm_hardware = {
 				  SNDRV_PCM_INFO_RESUME |
 				  SNDRV_PCM_INFO_NO_PERIOD_WAKEUP,
 	.formats		= SNDRV_PCM_FMTBIT_S16_LE |
-				  SNDRV_PCM_FMTBIT_S24_LE |
 				  SNDRV_PCM_FMTBIT_S32_LE,
+	.subformats		= SNDRV_PCM_SUBFMTBIT_MSBITS_20 |
+				  SNDRV_PCM_SUBFMTBIT_MSBITS_24 |
+				  SNDRV_PCM_SUBFMTBIT_MSBITS_32,
 	.buffer_bytes_max	= AZX_MAX_BUF_SIZE,
 	.period_bytes_min	= 128,
 	.period_bytes_max	= AZX_MAX_BUF_SIZE / 2,
diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
index 118f386d8582..5130261f0b33 100644
--- a/sound/soc/intel/avs/topology.c
+++ b/sound/soc/intel/avs/topology.c
@@ -1620,8 +1620,15 @@ static int avs_dai_load(struct snd_soc_component *comp, int index,
 			struct snd_soc_dai_driver *dai_drv, struct snd_soc_tplg_pcm *pcm,
 			struct snd_soc_dai *dai)
 {
-	if (pcm)
+	u64 subformats = SNDRV_PCM_SUBFMTBIT_MSBITS_20 |
+			 SNDRV_PCM_SUBFMTBIT_MSBITS_24;
+
+	if (pcm) {
 		dai_drv->ops = &avs_dai_fe_ops;
+		dai_drv->capture.subformats |= subformats;
+		dai_drv->playback.subformats |= subformats;
+	}
+
 	return 0;
 }
 
-- 
2.25.1

Re: [RFC PATCH 17/17] ASoC: Intel: avs: Kill S24_LE in HDAudio streaming

[Amadeusz Sławiński]

On 8/11/2023 6:48 PM, Cezary Rojewski wrote:
> 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 <cezary.rojewski@intel.com>
> ---
>   sound/soc/intel/avs/pcm.c      | 4 +++-
>   sound/soc/intel/avs/topology.c | 9 ++++++++-
>   2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c
> index 31737133b991..83cdce8fead4 100644
> --- a/sound/soc/intel/avs/pcm.c
> +++ b/sound/soc/intel/avs/pcm.c
> @@ -1160,8 +1160,10 @@ static const struct snd_pcm_hardware avs_pcm_hardware = {
>   				  SNDRV_PCM_INFO_RESUME |
>   				  SNDRV_PCM_INFO_NO_PERIOD_WAKEUP,
>   	.formats		= SNDRV_PCM_FMTBIT_S16_LE |
> -				  SNDRV_PCM_FMTBIT_S24_LE |
>   				  SNDRV_PCM_FMTBIT_S32_LE,
> +	.subformats		= SNDRV_PCM_SUBFMTBIT_MSBITS_20 |
> +				  SNDRV_PCM_SUBFMTBIT_MSBITS_24 |
> +				  SNDRV_PCM_SUBFMTBIT_MSBITS_32,
>   	.buffer_bytes_max	= AZX_MAX_BUF_SIZE,
>   	.period_bytes_min	= 128,
>   	.period_bytes_max	= AZX_MAX_BUF_SIZE / 2,
> diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
> index 118f386d8582..5130261f0b33 100644
> --- a/sound/soc/intel/avs/topology.c
> +++ b/sound/soc/intel/avs/topology.c
> @@ -1620,8 +1620,15 @@ static int avs_dai_load(struct snd_soc_component *comp, int index,
>   			struct snd_soc_dai_driver *dai_drv, struct snd_soc_tplg_pcm *pcm,
>   			struct snd_soc_dai *dai)
>   {
> -	if (pcm)
> +	u64 subformats = SNDRV_PCM_SUBFMTBIT_MSBITS_20 |
> +			 SNDRV_PCM_SUBFMTBIT_MSBITS_24;

SNDRV_PCM_SUBFMTBIT_MSBITS_32 seems to be missing in the above?

> +
> +	if (pcm) {
>   		dai_drv->ops = &avs_dai_fe_ops;
> +		dai_drv->capture.subformats |= subformats;
> +		dai_drv->playback.subformats |= subformats;
> +	}
> +
>   	return 0;
>   }
>