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

[Takashi Iwai <tiwai@xxxxxxx>]

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@xxxxxxxx>

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
>