[PATCH] ALSA: pcm: clarify and fix default msbits value for all formats

[PATCH] ALSA: pcm: clarify and fix default msbits value for all formats

[Jaroslav Kysela]

Return used most significant bits from sample bit-width rather than the whole
physical sample word size. The starting bit offset is defined in the format
itself.

The behaviour is not changed for 32-bit formats like S32_LE. But with this
change - msbits value 24 instead 32 is returned for 24-bit formats like S24_LE
etc.

Also, commit 2112aa034907 ("ALSA: pcm: Introduce MSBITS subformat interface")
compares sample bit-width not physical sample bit-width to reset MSBITS_MAX bit
from the subformat bitmask.

Probably no applications are using msbits value for other than S32_LE/U32_LE
formats, because no drivers are reducing msbits value for other formats (with
the msb offset) at the moment.

For sanity, increase PCM protocol version, letting the user space to detect
the changed behaviour.

Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 include/uapi/sound/asound.h | 4 ++--
 sound/core/pcm_native.c     | 5 +++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index d5b9cfbd9cea..628d46a0da92 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -142,7 +142,7 @@ struct snd_hwdep_dsp_image {
  *                                                                           *
  *****************************************************************************/
 
-#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 16)
+#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 17)
 
 typedef unsigned long snd_pcm_uframes_t;
 typedef signed long snd_pcm_sframes_t;
@@ -416,7 +416,7 @@ struct snd_pcm_hw_params {
 	unsigned int rmask;		/* W: requested masks */
 	unsigned int cmask;		/* R: changed masks */
 	unsigned int info;		/* R: Info flags for returned setup */
-	unsigned int msbits;		/* R: used most significant bits */
+	unsigned int msbits;		/* R: used most significant bits (in sample bit-width) */
 	unsigned int rate_num;		/* R: rate numerator */
 	unsigned int rate_den;		/* R: rate denominator */
 	snd_pcm_uframes_t fifo_size;	/* R: chip FIFO size in frames */
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index f5ff00f99788..21baf6bf7e25 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -486,6 +486,11 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream,
 		i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS);
 		if (snd_interval_single(i))
 			params->msbits = snd_interval_value(i);
+		m = hw_param_mask_c(params, SNDRV_PCM_HW_PARAM_FORMAT);
+		if (snd_mask_single(m)) {
+			snd_pcm_format_t format = (__force snd_pcm_format_t)snd_mask_min(m);
+			params->msbits = snd_pcm_format_width(format);
+		}
 	}
 
 	if (params->msbits) {
-- 
2.43.0

Re: [PATCH] ALSA: pcm: clarify and fix default msbits value for all formats

[Takashi Iwai]

On Thu, 22 Feb 2024 18:36:49 +0100,
Jaroslav Kysela wrote:
> 
> Return used most significant bits from sample bit-width rather than the whole
> physical sample word size. The starting bit offset is defined in the format
> itself.
> 
> The behaviour is not changed for 32-bit formats like S32_LE. But with this
> change - msbits value 24 instead 32 is returned for 24-bit formats like S24_LE
> etc.
> 
> Also, commit 2112aa034907 ("ALSA: pcm: Introduce MSBITS subformat interface")
> compares sample bit-width not physical sample bit-width to reset MSBITS_MAX bit
> from the subformat bitmask.
> 
> Probably no applications are using msbits value for other than S32_LE/U32_LE
> formats, because no drivers are reducing msbits value for other formats (with
> the msb offset) at the moment.
> 
> For sanity, increase PCM protocol version, letting the user space to detect
> the changed behaviour.
> 
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>

Hmm, the idea is nice, but I hesitate to take this as is.

Basically the kernel should keep the backward compatibility, and this
won't (although the feature is very minor).

IMO, it'd be safer to check user_pversion and applies the new behavior
only with it being >= 2.0.17, for example.


thanks,

Takashi

> ---
>  include/uapi/sound/asound.h | 4 ++--
>  sound/core/pcm_native.c     | 5 +++++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index d5b9cfbd9cea..628d46a0da92 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -142,7 +142,7 @@ struct snd_hwdep_dsp_image {
>   *                                                                           *
>   *****************************************************************************/
>  
> -#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 16)
> +#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 17)
>  
>  typedef unsigned long snd_pcm_uframes_t;
>  typedef signed long snd_pcm_sframes_t;
> @@ -416,7 +416,7 @@ struct snd_pcm_hw_params {
>  	unsigned int rmask;		/* W: requested masks */
>  	unsigned int cmask;		/* R: changed masks */
>  	unsigned int info;		/* R: Info flags for returned setup */
> -	unsigned int msbits;		/* R: used most significant bits */
> +	unsigned int msbits;		/* R: used most significant bits (in sample bit-width) */
>  	unsigned int rate_num;		/* R: rate numerator */
>  	unsigned int rate_den;		/* R: rate denominator */
>  	snd_pcm_uframes_t fifo_size;	/* R: chip FIFO size in frames */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index f5ff00f99788..21baf6bf7e25 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -486,6 +486,11 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream,
>  		i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS);
>  		if (snd_interval_single(i))
>  			params->msbits = snd_interval_value(i);
> +		m = hw_param_mask_c(params, SNDRV_PCM_HW_PARAM_FORMAT);
> +		if (snd_mask_single(m)) {
> +			snd_pcm_format_t format = (__force snd_pcm_format_t)snd_mask_min(m);
> +			params->msbits = snd_pcm_format_width(format);
> +		}
>  	}
>  
>  	if (params->msbits) {
> -- 
> 2.43.0
> 

Re: [PATCH] ALSA: pcm: clarify and fix default msbits value for all formats

[Jaroslav Kysela]

On 22. 02. 24 19:33, Takashi Iwai wrote:
> On Thu, 22 Feb 2024 18:36:49 +0100,
> Jaroslav Kysela wrote:
>>
>> Return used most significant bits from sample bit-width rather than the whole
>> physical sample word size. The starting bit offset is defined in the format
>> itself.
>>
>> The behaviour is not changed for 32-bit formats like S32_LE. But with this
>> change - msbits value 24 instead 32 is returned for 24-bit formats like S24_LE
>> etc.
>>
>> Also, commit 2112aa034907 ("ALSA: pcm: Introduce MSBITS subformat interface")
>> compares sample bit-width not physical sample bit-width to reset MSBITS_MAX bit
>> from the subformat bitmask.
>>
>> Probably no applications are using msbits value for other than S32_LE/U32_LE
>> formats, because no drivers are reducing msbits value for other formats (with
>> the msb offset) at the moment.
>>
>> For sanity, increase PCM protocol version, letting the user space to detect
>> the changed behaviour.
>>
>> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> 
> Hmm, the idea is nice, but I hesitate to take this as is.
> 
> Basically the kernel should keep the backward compatibility, and this
> won't (although the feature is very minor).
> 
> IMO, it'd be safer to check user_pversion and applies the new behavior
> only with it being >= 2.0.17, for example.

It's not so easy. The user_pversion is detached from substream (pcm_file 
structure) and propagating this value to all related functions creates really 
ugly code.

If you won't accept this ASIS, I propose to add new flags to the hw_params 
structure to alter the msbits behaviour. But I can hardly see any impact to 
the user space. The affected format/msbits combination was never used in the 
kernel drivers. It's more like a clarification than a fix.

					Jaroslav

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

Re: [PATCH] ALSA: pcm: clarify and fix default msbits value for all formats

[Takashi Iwai]

On Thu, 22 Feb 2024 19:59:14 +0100,
Jaroslav Kysela wrote:
> 
> On 22. 02. 24 19:33, Takashi Iwai wrote:
> > On Thu, 22 Feb 2024 18:36:49 +0100,
> > Jaroslav Kysela wrote:
> >> 
> >> Return used most significant bits from sample bit-width rather than the whole
> >> physical sample word size. The starting bit offset is defined in the format
> >> itself.
> >> 
> >> The behaviour is not changed for 32-bit formats like S32_LE. But with this
> >> change - msbits value 24 instead 32 is returned for 24-bit formats like S24_LE
> >> etc.
> >> 
> >> Also, commit 2112aa034907 ("ALSA: pcm: Introduce MSBITS subformat interface")
> >> compares sample bit-width not physical sample bit-width to reset MSBITS_MAX bit
> >> from the subformat bitmask.
> >> 
> >> Probably no applications are using msbits value for other than S32_LE/U32_LE
> >> formats, because no drivers are reducing msbits value for other formats (with
> >> the msb offset) at the moment.
> >> 
> >> For sanity, increase PCM protocol version, letting the user space to detect
> >> the changed behaviour.
> >> 
> >> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> > 
> > Hmm, the idea is nice, but I hesitate to take this as is.
> > 
> > Basically the kernel should keep the backward compatibility, and this
> > won't (although the feature is very minor).
> > 
> > IMO, it'd be safer to check user_pversion and applies the new behavior
> > only with it being >= 2.0.17, for example.
> 
> It's not so easy. The user_pversion is detached from substream
> (pcm_file structure) and propagating this value to all related
> functions creates really ugly code.
> 
> If you won't accept this ASIS, I propose to add new flags to the
> hw_params structure to alter the msbits behaviour. But I can hardly
> see any impact to the user space. The affected format/msbits
> combination was never used in the kernel drivers. It's more like a
> clarification than a fix.

OK, then let's take this.  I checked the major sound backend apps, and
they don't care about msbits, so the influence must be pretty low.


thanks,

Takashi

Re: [PATCH] ALSA: pcm: clarify and fix default msbits value for all formats

[Takashi Iwai]

On Fri, 23 Feb 2024 09:45:40 +0100,
Takashi Iwai wrote:
> 
> On Thu, 22 Feb 2024 19:59:14 +0100,
> Jaroslav Kysela wrote:
> > 
> > On 22. 02. 24 19:33, Takashi Iwai wrote:
> > > On Thu, 22 Feb 2024 18:36:49 +0100,
> > > Jaroslav Kysela wrote:
> > >> 
> > >> Return used most significant bits from sample bit-width rather than the whole
> > >> physical sample word size. The starting bit offset is defined in the format
> > >> itself.
> > >> 
> > >> The behaviour is not changed for 32-bit formats like S32_LE. But with this
> > >> change - msbits value 24 instead 32 is returned for 24-bit formats like S24_LE
> > >> etc.
> > >> 
> > >> Also, commit 2112aa034907 ("ALSA: pcm: Introduce MSBITS subformat interface")
> > >> compares sample bit-width not physical sample bit-width to reset MSBITS_MAX bit
> > >> from the subformat bitmask.
> > >> 
> > >> Probably no applications are using msbits value for other than S32_LE/U32_LE
> > >> formats, because no drivers are reducing msbits value for other formats (with
> > >> the msb offset) at the moment.
> > >> 
> > >> For sanity, increase PCM protocol version, letting the user space to detect
> > >> the changed behaviour.
> > >> 
> > >> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> > > 
> > > Hmm, the idea is nice, but I hesitate to take this as is.
> > > 
> > > Basically the kernel should keep the backward compatibility, and this
> > > won't (although the feature is very minor).
> > > 
> > > IMO, it'd be safer to check user_pversion and applies the new behavior
> > > only with it being >= 2.0.17, for example.
> > 
> > It's not so easy. The user_pversion is detached from substream
> > (pcm_file structure) and propagating this value to all related
> > functions creates really ugly code.
> > 
> > If you won't accept this ASIS, I propose to add new flags to the
> > hw_params structure to alter the msbits behaviour. But I can hardly
> > see any impact to the user space. The affected format/msbits
> > combination was never used in the kernel drivers. It's more like a
> > clarification than a fix.
> 
> OK, then let's take this.  I checked the major sound backend apps, and
> they don't care about msbits, so the influence must be pretty low.

To be more exact, both PA and PW have the same code to check the
msbits but they merely set "alsa.resolution_bits" property, and it's
not referred in the standard configs.


Takashi