diff mbox series

[01/13] ALSA: pcm: add more sample rate definitions

Message ID 20240905-alsa-12-24-128-v1-1-8371948d3921@baylibre.com (mailing list archive)
State Not Applicable
Headers show
Series ALSA: update sample rate definitions | expand

Commit Message

Jerome Brunet Sept. 5, 2024, 2:12 p.m. UTC
This adds a sample rate definition for 12kHz, 24kHz and 128kHz.

Admittedly, just a few drivers are currently using these sample
rates but there is enough of a recurrence to justify adding a definition
for them and remove some custom rate constraint code while at it.

The new definitions are not added to the interval definitions, such as
SNDRV_PCM_RATE_8000_44100, because it would silently add new supported
rates to drivers that may or may not support them. For sure the drivers
have not been tested for these new rates so it is better to leave them out
of interval definitions.

That being said, the added rates are multiples of well know rates families,
it is very likely that a lot of devices out there actually supports them.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 include/sound/pcm.h     | 31 +++++++++++++++++--------------
 sound/core/pcm_native.c |  6 +++---
 2 files changed, 20 insertions(+), 17 deletions(-)

Comments

Charles Keepax Sept. 9, 2024, 4:30 p.m. UTC | #1
On Thu, Sep 05, 2024 at 04:12:52PM +0200, Jerome Brunet wrote:
> This adds a sample rate definition for 12kHz, 24kHz and 128kHz.
> 
> Admittedly, just a few drivers are currently using these sample
> rates but there is enough of a recurrence to justify adding a definition
> for them and remove some custom rate constraint code while at it.
> 
> The new definitions are not added to the interval definitions, such as
> SNDRV_PCM_RATE_8000_44100, because it would silently add new supported
> rates to drivers that may or may not support them. For sure the drivers
> have not been tested for these new rates so it is better to leave them out
> of interval definitions.
> 
> That being said, the added rates are multiples of well know rates families,
> it is very likely that a lot of devices out there actually supports them.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---

Almost wonder if a comment with the SNDRV_PCM_RATE_8000_xxx
defines might also be an idea to warn they don't include all the
rates, although it is I guess easily seen from the define itself
so not sure if it might be over kill. But I am happy either way.

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles
Pierre-Louis Bossart Sept. 11, 2024, 9:09 a.m. UTC | #2
On 9/5/24 16:12, Jerome Brunet wrote:
> This adds a sample rate definition for 12kHz, 24kHz and 128kHz.
> 
> Admittedly, just a few drivers are currently using these sample
> rates but there is enough of a recurrence to justify adding a definition
> for them and remove some custom rate constraint code while at it.
> 
> The new definitions are not added to the interval definitions, such as
> SNDRV_PCM_RATE_8000_44100, because it would silently add new supported
> rates to drivers that may or may not support them. For sure the drivers
> have not been tested for these new rates so it is better to leave them out
> of interval definitions.
> 
> That being said, the added rates are multiples of well know rates families,
> it is very likely that a lot of devices out there actually supports them.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  include/sound/pcm.h     | 31 +++++++++++++++++--------------
>  sound/core/pcm_native.c |  6 +++---
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 732121b934fd..c993350975a9 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -109,20 +109,23 @@ struct snd_pcm_ops {
>  #define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
>  #define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
>  #define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
> -#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
> -#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
> -#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
> -#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
> -#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
> -#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
> -#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
> -#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
> -#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
> -#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
> -#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
> -#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
> -#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
> -#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
> +#define SNDRV_PCM_RATE_12000		(1U<<3)		/* 12000Hz */
> +#define SNDRV_PCM_RATE_16000		(1U<<4)		/* 16000Hz */
> +#define SNDRV_PCM_RATE_22050		(1U<<5)		/* 22050Hz */
> +#define SNDRV_PCM_RATE_24000		(1U<<6)		/* 24000Hz */
> +#define SNDRV_PCM_RATE_32000		(1U<<7)		/* 32000Hz */
> +#define SNDRV_PCM_RATE_44100		(1U<<8)		/* 44100Hz */
> +#define SNDRV_PCM_RATE_48000		(1U<<9)		/* 48000Hz */
> +#define SNDRV_PCM_RATE_64000		(1U<<10)	/* 64000Hz */
> +#define SNDRV_PCM_RATE_88200		(1U<<11)	/* 88200Hz */
> +#define SNDRV_PCM_RATE_96000		(1U<<12)	/* 96000Hz */
> +#define SNDRV_PCM_RATE_128000		(1U<<13)	/* 128000Hz */
> +#define SNDRV_PCM_RATE_176400		(1U<<14)	/* 176400Hz */
> +#define SNDRV_PCM_RATE_192000		(1U<<15)	/* 192000Hz */
> +#define SNDRV_PCM_RATE_352800		(1U<<16)	/* 352800Hz */
> +#define SNDRV_PCM_RATE_384000		(1U<<17)	/* 384000Hz */
> +#define SNDRV_PCM_RATE_705600		(1U<<18)	/* 705600Hz */
> +#define SNDRV_PCM_RATE_768000		(1U<<19)	/* 768000Hz */
>  
>  #define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
>  #define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 44381514f695..7461a727615c 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2418,13 +2418,13 @@ static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params,
>  	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
>  }
>  
> -#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12
> +#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_768000 != 1 << 19
>  #error "Change this table"
>  #endif
>  
>  static const unsigned int rates[] = {
> -	5512, 8000, 11025, 16000, 22050, 32000, 44100,
> -	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000, 705600, 768000
> +	5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000, 64000,
> +	88200, 96000, 128000, 176400, 192000, 352800, 384000, 705600, 768000,
>  };
>  
>  const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = {

Wondering if this is backwards compatible with the alsa-lib definitions,
specifically the topology parts which did unfortunately have a list of
rates that will map to a different index now:


typedef enum _snd_pcm_rates {
	SND_PCM_RATE_UNKNOWN = -1,
	SND_PCM_RATE_5512 = 0,
	SND_PCM_RATE_8000,
	SND_PCM_RATE_11025,
	SND_PCM_RATE_16000,
	SND_PCM_RATE_22050,
	SND_PCM_RATE_32000,
	SND_PCM_RATE_44100,
	SND_PCM_RATE_48000,
	SND_PCM_RATE_64000,
	SND_PCM_RATE_88200,
	SND_PCM_RATE_96000,
	SND_PCM_RATE_176400,
	SND_PCM_RATE_192000,
	SND_PCM_RATE_CONTINUOUS = 30,
	SND_PCM_RATE_KNOT = 31,
	SND_PCM_RATE_LAST = SND_PCM_RATE_KNOT,
} snd_pcm_rates_t;
Takashi Iwai Sept. 11, 2024, 9:21 a.m. UTC | #3
On Wed, 11 Sep 2024 11:09:59 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 9/5/24 16:12, Jerome Brunet wrote:
> > This adds a sample rate definition for 12kHz, 24kHz and 128kHz.
> > 
> > Admittedly, just a few drivers are currently using these sample
> > rates but there is enough of a recurrence to justify adding a definition
> > for them and remove some custom rate constraint code while at it.
> > 
> > The new definitions are not added to the interval definitions, such as
> > SNDRV_PCM_RATE_8000_44100, because it would silently add new supported
> > rates to drivers that may or may not support them. For sure the drivers
> > have not been tested for these new rates so it is better to leave them out
> > of interval definitions.
> > 
> > That being said, the added rates are multiples of well know rates families,
> > it is very likely that a lot of devices out there actually supports them.
> > 
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > ---
> >  include/sound/pcm.h     | 31 +++++++++++++++++--------------
> >  sound/core/pcm_native.c |  6 +++---
> >  2 files changed, 20 insertions(+), 17 deletions(-)
> > 
> > diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> > index 732121b934fd..c993350975a9 100644
> > --- a/include/sound/pcm.h
> > +++ b/include/sound/pcm.h
> > @@ -109,20 +109,23 @@ struct snd_pcm_ops {
> >  #define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
> >  #define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
> >  #define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
> > -#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
> > -#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
> > -#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
> > -#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
> > -#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
> > -#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
> > -#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
> > -#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
> > -#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
> > -#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
> > -#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
> > -#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
> > -#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
> > -#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
> > +#define SNDRV_PCM_RATE_12000		(1U<<3)		/* 12000Hz */
> > +#define SNDRV_PCM_RATE_16000		(1U<<4)		/* 16000Hz */
> > +#define SNDRV_PCM_RATE_22050		(1U<<5)		/* 22050Hz */
> > +#define SNDRV_PCM_RATE_24000		(1U<<6)		/* 24000Hz */
> > +#define SNDRV_PCM_RATE_32000		(1U<<7)		/* 32000Hz */
> > +#define SNDRV_PCM_RATE_44100		(1U<<8)		/* 44100Hz */
> > +#define SNDRV_PCM_RATE_48000		(1U<<9)		/* 48000Hz */
> > +#define SNDRV_PCM_RATE_64000		(1U<<10)	/* 64000Hz */
> > +#define SNDRV_PCM_RATE_88200		(1U<<11)	/* 88200Hz */
> > +#define SNDRV_PCM_RATE_96000		(1U<<12)	/* 96000Hz */
> > +#define SNDRV_PCM_RATE_128000		(1U<<13)	/* 128000Hz */
> > +#define SNDRV_PCM_RATE_176400		(1U<<14)	/* 176400Hz */
> > +#define SNDRV_PCM_RATE_192000		(1U<<15)	/* 192000Hz */
> > +#define SNDRV_PCM_RATE_352800		(1U<<16)	/* 352800Hz */
> > +#define SNDRV_PCM_RATE_384000		(1U<<17)	/* 384000Hz */
> > +#define SNDRV_PCM_RATE_705600		(1U<<18)	/* 705600Hz */
> > +#define SNDRV_PCM_RATE_768000		(1U<<19)	/* 768000Hz */
> >  
> >  #define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
> >  #define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
> > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > index 44381514f695..7461a727615c 100644
> > --- a/sound/core/pcm_native.c
> > +++ b/sound/core/pcm_native.c
> > @@ -2418,13 +2418,13 @@ static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params,
> >  	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
> >  }
> >  
> > -#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12
> > +#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_768000 != 1 << 19
> >  #error "Change this table"
> >  #endif
> >  
> >  static const unsigned int rates[] = {
> > -	5512, 8000, 11025, 16000, 22050, 32000, 44100,
> > -	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000, 705600, 768000
> > +	5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000, 64000,
> > +	88200, 96000, 128000, 176400, 192000, 352800, 384000, 705600, 768000,
> >  };
> >  
> >  const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = {
> 
> Wondering if this is backwards compatible with the alsa-lib definitions,
> specifically the topology parts which did unfortunately have a list of
> rates that will map to a different index now:
> 
> 
> typedef enum _snd_pcm_rates {
> 	SND_PCM_RATE_UNKNOWN = -1,
> 	SND_PCM_RATE_5512 = 0,
> 	SND_PCM_RATE_8000,
> 	SND_PCM_RATE_11025,
> 	SND_PCM_RATE_16000,
> 	SND_PCM_RATE_22050,
> 	SND_PCM_RATE_32000,
> 	SND_PCM_RATE_44100,
> 	SND_PCM_RATE_48000,
> 	SND_PCM_RATE_64000,
> 	SND_PCM_RATE_88200,
> 	SND_PCM_RATE_96000,
> 	SND_PCM_RATE_176400,
> 	SND_PCM_RATE_192000,
> 	SND_PCM_RATE_CONTINUOUS = 30,
> 	SND_PCM_RATE_KNOT = 31,
> 	SND_PCM_RATE_LAST = SND_PCM_RATE_KNOT,
> } snd_pcm_rates_t;

As far as I understand correctly, those rate bits used for topology
are independent from the bits used for PCM core, although it used to
be the same.  Maybe better to rename (such as SND_TPLG_RATE_*) so that
it's clearer only for topology stuff.

But it'd be better if anyone can double-check.


thanks,

Takashi
Peter Ujfalusi Sept. 11, 2024, 10:33 a.m. UTC | #4
On 11/09/2024 12:21, Takashi Iwai wrote:
>> Wondering if this is backwards compatible with the alsa-lib definitions,
>> specifically the topology parts which did unfortunately have a list of
>> rates that will map to a different index now:
>>
>>
>> typedef enum _snd_pcm_rates {
>> 	SND_PCM_RATE_UNKNOWN = -1,
>> 	SND_PCM_RATE_5512 = 0,
>> 	SND_PCM_RATE_8000,
>> 	SND_PCM_RATE_11025,
>> 	SND_PCM_RATE_16000,
>> 	SND_PCM_RATE_22050,
>> 	SND_PCM_RATE_32000,
>> 	SND_PCM_RATE_44100,
>> 	SND_PCM_RATE_48000,
>> 	SND_PCM_RATE_64000,
>> 	SND_PCM_RATE_88200,
>> 	SND_PCM_RATE_96000,
>> 	SND_PCM_RATE_176400,
>> 	SND_PCM_RATE_192000,
>> 	SND_PCM_RATE_CONTINUOUS = 30,
>> 	SND_PCM_RATE_KNOT = 31,
>> 	SND_PCM_RATE_LAST = SND_PCM_RATE_KNOT,
>> } snd_pcm_rates_t;
> 
> As far as I understand correctly, those rate bits used for topology
> are independent from the bits used for PCM core, although it used to
> be the same.  Maybe better to rename (such as SND_TPLG_RATE_*) so that
> it's clearer only for topology stuff.

Even if we rename these in alsa-lib we will need translation from
SND_TPLG_RATE_ to SND_PCM_RATE_ in kernel likely?

The topology files are out there and this is an ABI...

> But it'd be better if anyone can double-check.

Since the kernel just copies the rates bitfield, any rate above 11025
will be misaligned and result broken setup.

> 
> 
> thanks,
> 
> Takashi
Takashi Iwai Sept. 11, 2024, 10:44 a.m. UTC | #5
On Wed, 11 Sep 2024 11:21:41 +0200,
Takashi Iwai wrote:
> 
> On Wed, 11 Sep 2024 11:09:59 +0200,
> Pierre-Louis Bossart wrote:
> > 
> > 
> > 
> > On 9/5/24 16:12, Jerome Brunet wrote:
> > > This adds a sample rate definition for 12kHz, 24kHz and 128kHz.
> > > 
> > > Admittedly, just a few drivers are currently using these sample
> > > rates but there is enough of a recurrence to justify adding a definition
> > > for them and remove some custom rate constraint code while at it.
> > > 
> > > The new definitions are not added to the interval definitions, such as
> > > SNDRV_PCM_RATE_8000_44100, because it would silently add new supported
> > > rates to drivers that may or may not support them. For sure the drivers
> > > have not been tested for these new rates so it is better to leave them out
> > > of interval definitions.
> > > 
> > > That being said, the added rates are multiples of well know rates families,
> > > it is very likely that a lot of devices out there actually supports them.
> > > 
> > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > > ---
> > >  include/sound/pcm.h     | 31 +++++++++++++++++--------------
> > >  sound/core/pcm_native.c |  6 +++---
> > >  2 files changed, 20 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> > > index 732121b934fd..c993350975a9 100644
> > > --- a/include/sound/pcm.h
> > > +++ b/include/sound/pcm.h
> > > @@ -109,20 +109,23 @@ struct snd_pcm_ops {
> > >  #define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
> > >  #define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
> > >  #define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
> > > -#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
> > > -#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
> > > -#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
> > > -#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
> > > -#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
> > > -#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
> > > -#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
> > > -#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
> > > -#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
> > > -#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
> > > -#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
> > > -#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
> > > -#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
> > > -#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
> > > +#define SNDRV_PCM_RATE_12000		(1U<<3)		/* 12000Hz */
> > > +#define SNDRV_PCM_RATE_16000		(1U<<4)		/* 16000Hz */
> > > +#define SNDRV_PCM_RATE_22050		(1U<<5)		/* 22050Hz */
> > > +#define SNDRV_PCM_RATE_24000		(1U<<6)		/* 24000Hz */
> > > +#define SNDRV_PCM_RATE_32000		(1U<<7)		/* 32000Hz */
> > > +#define SNDRV_PCM_RATE_44100		(1U<<8)		/* 44100Hz */
> > > +#define SNDRV_PCM_RATE_48000		(1U<<9)		/* 48000Hz */
> > > +#define SNDRV_PCM_RATE_64000		(1U<<10)	/* 64000Hz */
> > > +#define SNDRV_PCM_RATE_88200		(1U<<11)	/* 88200Hz */
> > > +#define SNDRV_PCM_RATE_96000		(1U<<12)	/* 96000Hz */
> > > +#define SNDRV_PCM_RATE_128000		(1U<<13)	/* 128000Hz */
> > > +#define SNDRV_PCM_RATE_176400		(1U<<14)	/* 176400Hz */
> > > +#define SNDRV_PCM_RATE_192000		(1U<<15)	/* 192000Hz */
> > > +#define SNDRV_PCM_RATE_352800		(1U<<16)	/* 352800Hz */
> > > +#define SNDRV_PCM_RATE_384000		(1U<<17)	/* 384000Hz */
> > > +#define SNDRV_PCM_RATE_705600		(1U<<18)	/* 705600Hz */
> > > +#define SNDRV_PCM_RATE_768000		(1U<<19)	/* 768000Hz */
> > >  
> > >  #define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
> > >  #define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
> > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > > index 44381514f695..7461a727615c 100644
> > > --- a/sound/core/pcm_native.c
> > > +++ b/sound/core/pcm_native.c
> > > @@ -2418,13 +2418,13 @@ static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params,
> > >  	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
> > >  }
> > >  
> > > -#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12
> > > +#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_768000 != 1 << 19
> > >  #error "Change this table"
> > >  #endif
> > >  
> > >  static const unsigned int rates[] = {
> > > -	5512, 8000, 11025, 16000, 22050, 32000, 44100,
> > > -	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000, 705600, 768000
> > > +	5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000, 64000,
> > > +	88200, 96000, 128000, 176400, 192000, 352800, 384000, 705600, 768000,
> > >  };
> > >  
> > >  const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = {
> > 
> > Wondering if this is backwards compatible with the alsa-lib definitions,
> > specifically the topology parts which did unfortunately have a list of
> > rates that will map to a different index now:
> > 
> > 
> > typedef enum _snd_pcm_rates {
> > 	SND_PCM_RATE_UNKNOWN = -1,
> > 	SND_PCM_RATE_5512 = 0,
> > 	SND_PCM_RATE_8000,
> > 	SND_PCM_RATE_11025,
> > 	SND_PCM_RATE_16000,
> > 	SND_PCM_RATE_22050,
> > 	SND_PCM_RATE_32000,
> > 	SND_PCM_RATE_44100,
> > 	SND_PCM_RATE_48000,
> > 	SND_PCM_RATE_64000,
> > 	SND_PCM_RATE_88200,
> > 	SND_PCM_RATE_96000,
> > 	SND_PCM_RATE_176400,
> > 	SND_PCM_RATE_192000,
> > 	SND_PCM_RATE_CONTINUOUS = 30,
> > 	SND_PCM_RATE_KNOT = 31,
> > 	SND_PCM_RATE_LAST = SND_PCM_RATE_KNOT,
> > } snd_pcm_rates_t;
> 
> As far as I understand correctly, those rate bits used for topology
> are independent from the bits used for PCM core, although it used to
> be the same.  Maybe better to rename (such as SND_TPLG_RATE_*) so that
> it's clearer only for topology stuff.
> 
> But it'd be better if anyone can double-check.

... and I double-check by myself and proved I was wrong :-<

In soc-topology.c, set_stream_info() takes the
snd_soc_pcm_stream.rates bits as is from the given topology data,
so the changes of the bits can break this.

The topology takes both rates and formats bits.  The formats are a
part of uapi, but the rates aren't.  We should move those into uapi,
if any...


Takashi
Takashi Iwai Sept. 11, 2024, 10:51 a.m. UTC | #6
On Wed, 11 Sep 2024 12:33:01 +0200,
Péter Ujfalusi wrote:
> 
> On 11/09/2024 12:21, Takashi Iwai wrote:
> >> Wondering if this is backwards compatible with the alsa-lib definitions,
> >> specifically the topology parts which did unfortunately have a list of
> >> rates that will map to a different index now:
> >>
> >>
> >> typedef enum _snd_pcm_rates {
> >> 	SND_PCM_RATE_UNKNOWN = -1,
> >> 	SND_PCM_RATE_5512 = 0,
> >> 	SND_PCM_RATE_8000,
> >> 	SND_PCM_RATE_11025,
> >> 	SND_PCM_RATE_16000,
> >> 	SND_PCM_RATE_22050,
> >> 	SND_PCM_RATE_32000,
> >> 	SND_PCM_RATE_44100,
> >> 	SND_PCM_RATE_48000,
> >> 	SND_PCM_RATE_64000,
> >> 	SND_PCM_RATE_88200,
> >> 	SND_PCM_RATE_96000,
> >> 	SND_PCM_RATE_176400,
> >> 	SND_PCM_RATE_192000,
> >> 	SND_PCM_RATE_CONTINUOUS = 30,
> >> 	SND_PCM_RATE_KNOT = 31,
> >> 	SND_PCM_RATE_LAST = SND_PCM_RATE_KNOT,
> >> } snd_pcm_rates_t;
> > 
> > As far as I understand correctly, those rate bits used for topology
> > are independent from the bits used for PCM core, although it used to
> > be the same.  Maybe better to rename (such as SND_TPLG_RATE_*) so that
> > it's clearer only for topology stuff.
> 
> Even if we rename these in alsa-lib we will need translation from
> SND_TPLG_RATE_ to SND_PCM_RATE_ in kernel likely?
> 
> The topology files are out there and this is an ABI...
> 
> > But it'd be better if anyone can double-check.
> 
> Since the kernel just copies the rates bitfield, any rate above 11025
> will be misaligned and result broken setup.

Yep, I noticed it now, too.

Below is the fix patch, totally untested.
It'd be appreciated if anyone can test it quickly.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: pcm: Fix breakage of PCM rates used for topology

It turned out that the topology ABI takes the standard PCM rate bits
as is, and it means that the recent change of the PCM rate bits would
lead to the inconsistent rate values used for topology.

This patch reverts the original PCM rate bit definitions while adding
the new rates to the extended bits instead.  This needed the change of
snd_pcm_known_rates, too.  And this also required to fix the handling
in snd_pcm_hw_limit_rates() that blindly assumed that the list is
sorted while it became unsorted now.

Fixes: 090624b7dc83 ("ALSA: pcm: add more sample rate definitions")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/pcm.h     | 35 ++++++++++++++++++-----------------
 sound/core/pcm_misc.c   | 18 ++++++++++--------
 sound/core/pcm_native.c | 10 +++++++---
 3 files changed, 35 insertions(+), 28 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index c993350975a9..824216799070 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -109,23 +109,24 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
 #define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
 #define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
-#define SNDRV_PCM_RATE_12000		(1U<<3)		/* 12000Hz */
-#define SNDRV_PCM_RATE_16000		(1U<<4)		/* 16000Hz */
-#define SNDRV_PCM_RATE_22050		(1U<<5)		/* 22050Hz */
-#define SNDRV_PCM_RATE_24000		(1U<<6)		/* 24000Hz */
-#define SNDRV_PCM_RATE_32000		(1U<<7)		/* 32000Hz */
-#define SNDRV_PCM_RATE_44100		(1U<<8)		/* 44100Hz */
-#define SNDRV_PCM_RATE_48000		(1U<<9)		/* 48000Hz */
-#define SNDRV_PCM_RATE_64000		(1U<<10)	/* 64000Hz */
-#define SNDRV_PCM_RATE_88200		(1U<<11)	/* 88200Hz */
-#define SNDRV_PCM_RATE_96000		(1U<<12)	/* 96000Hz */
-#define SNDRV_PCM_RATE_128000		(1U<<13)	/* 128000Hz */
-#define SNDRV_PCM_RATE_176400		(1U<<14)	/* 176400Hz */
-#define SNDRV_PCM_RATE_192000		(1U<<15)	/* 192000Hz */
-#define SNDRV_PCM_RATE_352800		(1U<<16)	/* 352800Hz */
-#define SNDRV_PCM_RATE_384000		(1U<<17)	/* 384000Hz */
-#define SNDRV_PCM_RATE_705600		(1U<<18)	/* 705600Hz */
-#define SNDRV_PCM_RATE_768000		(1U<<19)	/* 768000Hz */
+#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
+#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
+#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
+#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
+#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
+#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
+#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
+#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
+#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
+#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
+#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
+#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
+#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
+#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
+/* extended rates */
+#define SNDRV_PCM_RATE_12000		(1U<<17)	/* 12000Hz */
+#define SNDRV_PCM_RATE_24000		(1U<<18)	/* 24000Hz */
+#define SNDRV_PCM_RATE_128000		(1U<<19)	/* 128000Hz */
 
 #define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
 #define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
diff --git a/sound/core/pcm_misc.c b/sound/core/pcm_misc.c
index 5588b6a1ee8b..4f556211bb56 100644
--- a/sound/core/pcm_misc.c
+++ b/sound/core/pcm_misc.c
@@ -494,18 +494,20 @@ EXPORT_SYMBOL(snd_pcm_format_set_silence);
 int snd_pcm_hw_limit_rates(struct snd_pcm_hardware *hw)
 {
 	int i;
+	unsigned int rmin, rmax;
+
+	rmin = UINT_MAX;
+	rmax = 0;
 	for (i = 0; i < (int)snd_pcm_known_rates.count; i++) {
 		if (hw->rates & (1 << i)) {
-			hw->rate_min = snd_pcm_known_rates.list[i];
-			break;
-		}
-	}
-	for (i = (int)snd_pcm_known_rates.count - 1; i >= 0; i--) {
-		if (hw->rates & (1 << i)) {
-			hw->rate_max = snd_pcm_known_rates.list[i];
-			break;
+			rmin = min(rmin, snd_pcm_known_rates.list[i]);
+			rmax = max(rmax, snd_pcm_known_rates.list[i]);
 		}
 	}
+	if (rmin > rmax)
+		return -EINVAL;
+	hw->rate_min = rmin;
+	hw->rate_max = rmax;
 	return 0;
 }
 EXPORT_SYMBOL(snd_pcm_hw_limit_rates);
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 7461a727615c..5e1e6006707b 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2418,13 +2418,17 @@ static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params,
 	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
 }
 
-#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_768000 != 1 << 19
+#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12 ||\
+	SNDRV_PCM_RATE_128000 != 1 << 19
 #error "Change this table"
 #endif
 
+/* NOTE: the list is unsorted! */
 static const unsigned int rates[] = {
-	5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000, 64000,
-	88200, 96000, 128000, 176400, 192000, 352800, 384000, 705600, 768000,
+	5512, 8000, 11025, 16000, 22050, 32000, 44100,
+	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000, 705600, 768000,
+	/* extended */
+	12000, 24000, 128000
 };
 
 const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = {
Jaroslav Kysela Sept. 11, 2024, 10:58 a.m. UTC | #7
On 11. 09. 24 12:51, Takashi Iwai wrote:
> On Wed, 11 Sep 2024 12:33:01 +0200,
> Péter Ujfalusi wrote:
>>
>> On 11/09/2024 12:21, Takashi Iwai wrote:
>>>> Wondering if this is backwards compatible with the alsa-lib definitions,
>>>> specifically the topology parts which did unfortunately have a list of
>>>> rates that will map to a different index now:
>>>>
>>>>
>>>> typedef enum _snd_pcm_rates {
>>>> 	SND_PCM_RATE_UNKNOWN = -1,
>>>> 	SND_PCM_RATE_5512 = 0,
>>>> 	SND_PCM_RATE_8000,
>>>> 	SND_PCM_RATE_11025,
>>>> 	SND_PCM_RATE_16000,
>>>> 	SND_PCM_RATE_22050,
>>>> 	SND_PCM_RATE_32000,
>>>> 	SND_PCM_RATE_44100,
>>>> 	SND_PCM_RATE_48000,
>>>> 	SND_PCM_RATE_64000,
>>>> 	SND_PCM_RATE_88200,
>>>> 	SND_PCM_RATE_96000,
>>>> 	SND_PCM_RATE_176400,
>>>> 	SND_PCM_RATE_192000,
>>>> 	SND_PCM_RATE_CONTINUOUS = 30,
>>>> 	SND_PCM_RATE_KNOT = 31,
>>>> 	SND_PCM_RATE_LAST = SND_PCM_RATE_KNOT,
>>>> } snd_pcm_rates_t;
>>>
>>> As far as I understand correctly, those rate bits used for topology
>>> are independent from the bits used for PCM core, although it used to
>>> be the same.  Maybe better to rename (such as SND_TPLG_RATE_*) so that
>>> it's clearer only for topology stuff.
>>
>> Even if we rename these in alsa-lib we will need translation from
>> SND_TPLG_RATE_ to SND_PCM_RATE_ in kernel likely?
>>
>> The topology files are out there and this is an ABI...
>>
>>> But it'd be better if anyone can double-check.
>>
>> Since the kernel just copies the rates bitfield, any rate above 11025
>> will be misaligned and result broken setup.
> 
> Yep, I noticed it now, too.
> 
> Below is the fix patch, totally untested.
> It'd be appreciated if anyone can test it quickly.
> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: pcm: Fix breakage of PCM rates used for topology
> 
> It turned out that the topology ABI takes the standard PCM rate bits
> as is, and it means that the recent change of the PCM rate bits would
> lead to the inconsistent rate values used for topology.
> 
> This patch reverts the original PCM rate bit definitions while adding
> the new rates to the extended bits instead.  This needed the change of
> snd_pcm_known_rates, too.  And this also required to fix the handling
> in snd_pcm_hw_limit_rates() that blindly assumed that the list is
> sorted while it became unsorted now.
> 
> Fixes: 090624b7dc83 ("ALSA: pcm: add more sample rate definitions")
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

This looks fine. But the topology rate bits should not depend on those bits. 
It's a bit pitty that the standard PCM ABI does not use those bits for user 
space and we are doing this change just for topology ABI.

Reviewed-by: Jaroslav Kysela <perex@perex.cz>
Takashi Iwai Sept. 11, 2024, 12:42 p.m. UTC | #8
On Wed, 11 Sep 2024 12:58:53 +0200,
Jaroslav Kysela wrote:
> 
> On 11. 09. 24 12:51, Takashi Iwai wrote:
> > On Wed, 11 Sep 2024 12:33:01 +0200,
> > Péter Ujfalusi wrote:
> >> 
> >> On 11/09/2024 12:21, Takashi Iwai wrote:
> >>>> Wondering if this is backwards compatible with the alsa-lib definitions,
> >>>> specifically the topology parts which did unfortunately have a list of
> >>>> rates that will map to a different index now:
> >>>> 
> >>>> 
> >>>> typedef enum _snd_pcm_rates {
> >>>> 	SND_PCM_RATE_UNKNOWN = -1,
> >>>> 	SND_PCM_RATE_5512 = 0,
> >>>> 	SND_PCM_RATE_8000,
> >>>> 	SND_PCM_RATE_11025,
> >>>> 	SND_PCM_RATE_16000,
> >>>> 	SND_PCM_RATE_22050,
> >>>> 	SND_PCM_RATE_32000,
> >>>> 	SND_PCM_RATE_44100,
> >>>> 	SND_PCM_RATE_48000,
> >>>> 	SND_PCM_RATE_64000,
> >>>> 	SND_PCM_RATE_88200,
> >>>> 	SND_PCM_RATE_96000,
> >>>> 	SND_PCM_RATE_176400,
> >>>> 	SND_PCM_RATE_192000,
> >>>> 	SND_PCM_RATE_CONTINUOUS = 30,
> >>>> 	SND_PCM_RATE_KNOT = 31,
> >>>> 	SND_PCM_RATE_LAST = SND_PCM_RATE_KNOT,
> >>>> } snd_pcm_rates_t;
> >>> 
> >>> As far as I understand correctly, those rate bits used for topology
> >>> are independent from the bits used for PCM core, although it used to
> >>> be the same.  Maybe better to rename (such as SND_TPLG_RATE_*) so that
> >>> it's clearer only for topology stuff.
> >> 
> >> Even if we rename these in alsa-lib we will need translation from
> >> SND_TPLG_RATE_ to SND_PCM_RATE_ in kernel likely?
> >> 
> >> The topology files are out there and this is an ABI...
> >> 
> >>> But it'd be better if anyone can double-check.
> >> 
> >> Since the kernel just copies the rates bitfield, any rate above 11025
> >> will be misaligned and result broken setup.
> > 
> > Yep, I noticed it now, too.
> > 
> > Below is the fix patch, totally untested.
> > It'd be appreciated if anyone can test it quickly.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > -- 8< --
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] ALSA: pcm: Fix breakage of PCM rates used for topology
> > 
> > It turned out that the topology ABI takes the standard PCM rate bits
> > as is, and it means that the recent change of the PCM rate bits would
> > lead to the inconsistent rate values used for topology.
> > 
> > This patch reverts the original PCM rate bit definitions while adding
> > the new rates to the extended bits instead.  This needed the change of
> > snd_pcm_known_rates, too.  And this also required to fix the handling
> > in snd_pcm_hw_limit_rates() that blindly assumed that the list is
> > sorted while it became unsorted now.
> > 
> > Fixes: 090624b7dc83 ("ALSA: pcm: add more sample rate definitions")
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> This looks fine. But the topology rate bits should not depend on those
> bits. It's a bit pitty that the standard PCM ABI does not use those
> bits for user space and we are doing this change just for topology
> ABI.

Yeah, and theoretically it's possible to fix in topology side, but
it'll be more cumbersome.

Although it's not really a part of PCM ABI, I believe we should move
the PCM rate bit definitions to uapi, for showing that it's set in
stone.  Something like below.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: pcm: Move standard rate bit definitions into uapi

Since the standard PCM rate bits are used for the topology ABI, it's a
part of public ABI that must not be changed.  Move the definitions
into uapi to indicate it more clearly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/pcm.h         | 26 --------------------------
 include/uapi/sound/asound.h | 26 ++++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 824216799070..f28f6d6ac996 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -105,32 +105,6 @@ struct snd_pcm_ops {
 
 #define SNDRV_PCM_POS_XRUN		((snd_pcm_uframes_t)-1)
 
-/* If you change this don't forget to change rates[] table in pcm_native.c */
-#define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
-#define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
-#define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
-#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
-#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
-#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
-#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
-#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
-#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
-#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
-#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
-#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
-#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
-#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
-#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
-#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
-#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
-/* extended rates */
-#define SNDRV_PCM_RATE_12000		(1U<<17)	/* 12000Hz */
-#define SNDRV_PCM_RATE_24000		(1U<<18)	/* 24000Hz */
-#define SNDRV_PCM_RATE_128000		(1U<<19)	/* 128000Hz */
-
-#define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
-#define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
-
 #define SNDRV_PCM_RATE_8000_44100	(SNDRV_PCM_RATE_8000|SNDRV_PCM_RATE_11025|\
 					 SNDRV_PCM_RATE_16000|SNDRV_PCM_RATE_22050|\
 					 SNDRV_PCM_RATE_32000|SNDRV_PCM_RATE_44100)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 4cd513215bcd..715ceb3eac7c 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -272,6 +272,32 @@ typedef int __bitwise snd_pcm_subformat_t;
 #define	SNDRV_PCM_SUBFORMAT_MSBITS_24	((__force snd_pcm_subformat_t) 3)
 #define	SNDRV_PCM_SUBFORMAT_LAST	SNDRV_PCM_SUBFORMAT_MSBITS_24
 
+/* Standard rate bits */
+#define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
+#define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
+#define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
+#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
+#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
+#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
+#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
+#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
+#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
+#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
+#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
+#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
+#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
+#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
+#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
+#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
+#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
+/* extended rates */
+#define SNDRV_PCM_RATE_12000		(1U<<17)	/* 12000Hz */
+#define SNDRV_PCM_RATE_24000		(1U<<18)	/* 24000Hz */
+#define SNDRV_PCM_RATE_128000		(1U<<19)	/* 128000Hz */
+
+#define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
+#define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
+
 #define SNDRV_PCM_INFO_MMAP		0x00000001	/* hardware supports mmap */
 #define SNDRV_PCM_INFO_MMAP_VALID	0x00000002	/* period data are valid during transfer */
 #define SNDRV_PCM_INFO_DOUBLE		0x00000004	/* Double buffering needed for PCM start/stop */
Jerome Brunet Sept. 11, 2024, 12:55 p.m. UTC | #9
On Wed 11 Sep 2024 at 12:51, Takashi Iwai <tiwai@suse.de> wrote:

> On Wed, 11 Sep 2024 12:33:01 +0200,
> Péter Ujfalusi wrote:
>> 
>> On 11/09/2024 12:21, Takashi Iwai wrote:
>> >> Wondering if this is backwards compatible with the alsa-lib definitions,
>> >> specifically the topology parts which did unfortunately have a list of
>> >> rates that will map to a different index now:
>> >>
>> >>
>> >> typedef enum _snd_pcm_rates {
>> >> 	SND_PCM_RATE_UNKNOWN = -1,
>> >> 	SND_PCM_RATE_5512 = 0,
>> >> 	SND_PCM_RATE_8000,
>> >> 	SND_PCM_RATE_11025,
>> >> 	SND_PCM_RATE_16000,
>> >> 	SND_PCM_RATE_22050,
>> >> 	SND_PCM_RATE_32000,
>> >> 	SND_PCM_RATE_44100,
>> >> 	SND_PCM_RATE_48000,
>> >> 	SND_PCM_RATE_64000,
>> >> 	SND_PCM_RATE_88200,
>> >> 	SND_PCM_RATE_96000,
>> >> 	SND_PCM_RATE_176400,
>> >> 	SND_PCM_RATE_192000,
>> >> 	SND_PCM_RATE_CONTINUOUS = 30,
>> >> 	SND_PCM_RATE_KNOT = 31,
>> >> 	SND_PCM_RATE_LAST = SND_PCM_RATE_KNOT,
>> >> } snd_pcm_rates_t;
>> > 
>> > As far as I understand correctly, those rate bits used for topology
>> > are independent from the bits used for PCM core, although it used to
>> > be the same.  Maybe better to rename (such as SND_TPLG_RATE_*) so that
>> > it's clearer only for topology stuff.
>> 
>> Even if we rename these in alsa-lib we will need translation from
>> SND_TPLG_RATE_ to SND_PCM_RATE_ in kernel likely?
>> 
>> The topology files are out there and this is an ABI...
>> 
>> > But it'd be better if anyone can double-check.
>> 
>> Since the kernel just copies the rates bitfield, any rate above 11025
>> will be misaligned and result broken setup.
>
> Yep, I noticed it now, too.
>
> Below is the fix patch, totally untested.
> It'd be appreciated if anyone can test it quickly.
>
>
> thanks,
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: pcm: Fix breakage of PCM rates used for topology
>
> It turned out that the topology ABI takes the standard PCM rate bits
> as is, and it means that the recent change of the PCM rate bits would
> lead to the inconsistent rate values used for topology.
>
> This patch reverts the original PCM rate bit definitions while adding
> the new rates to the extended bits instead.  This needed the change of
> snd_pcm_known_rates, too.  And this also required to fix the handling
> in snd_pcm_hw_limit_rates() that blindly assumed that the list is
> sorted while it became unsorted now.
>
> Fixes: 090624b7dc83 ("ALSA: pcm: add more sample rate definitions")
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  include/sound/pcm.h     | 35 ++++++++++++++++++-----------------
>  sound/core/pcm_misc.c   | 18 ++++++++++--------
>  sound/core/pcm_native.c | 10 +++++++---
>  3 files changed, 35 insertions(+), 28 deletions(-)
>
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index c993350975a9..824216799070 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -109,23 +109,24 @@ struct snd_pcm_ops {
>  #define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
>  #define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
>  #define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
> -#define SNDRV_PCM_RATE_12000		(1U<<3)		/* 12000Hz */
> -#define SNDRV_PCM_RATE_16000		(1U<<4)		/* 16000Hz */
> -#define SNDRV_PCM_RATE_22050		(1U<<5)		/* 22050Hz */
> -#define SNDRV_PCM_RATE_24000		(1U<<6)		/* 24000Hz */
> -#define SNDRV_PCM_RATE_32000		(1U<<7)		/* 32000Hz */
> -#define SNDRV_PCM_RATE_44100		(1U<<8)		/* 44100Hz */
> -#define SNDRV_PCM_RATE_48000		(1U<<9)		/* 48000Hz */
> -#define SNDRV_PCM_RATE_64000		(1U<<10)	/* 64000Hz */
> -#define SNDRV_PCM_RATE_88200		(1U<<11)	/* 88200Hz */
> -#define SNDRV_PCM_RATE_96000		(1U<<12)	/* 96000Hz */
> -#define SNDRV_PCM_RATE_128000		(1U<<13)	/* 128000Hz */
> -#define SNDRV_PCM_RATE_176400		(1U<<14)	/* 176400Hz */
> -#define SNDRV_PCM_RATE_192000		(1U<<15)	/* 192000Hz */
> -#define SNDRV_PCM_RATE_352800		(1U<<16)	/* 352800Hz */
> -#define SNDRV_PCM_RATE_384000		(1U<<17)	/* 384000Hz */
> -#define SNDRV_PCM_RATE_705600		(1U<<18)	/* 705600Hz */
> -#define SNDRV_PCM_RATE_768000		(1U<<19)	/* 768000Hz */
> +#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
> +#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
> +#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
> +#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
> +#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
> +#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
> +#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
> +#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
> +#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
> +#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
> +#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
> +#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
> +#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
> +#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
> +/* extended rates */
> +#define SNDRV_PCM_RATE_12000		(1U<<17)	/* 12000Hz */
> +#define SNDRV_PCM_RATE_24000		(1U<<18)	/* 24000Hz */
> +#define SNDRV_PCM_RATE_128000		(1U<<19)	/* 128000Hz */
>  
>  #define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
>  #define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
> diff --git a/sound/core/pcm_misc.c b/sound/core/pcm_misc.c
> index 5588b6a1ee8b..4f556211bb56 100644
> --- a/sound/core/pcm_misc.c
> +++ b/sound/core/pcm_misc.c
> @@ -494,18 +494,20 @@ EXPORT_SYMBOL(snd_pcm_format_set_silence);
>  int snd_pcm_hw_limit_rates(struct snd_pcm_hardware *hw)
>  {
>  	int i;
> +	unsigned int rmin, rmax;
> +
> +	rmin = UINT_MAX;
> +	rmax = 0;
>  	for (i = 0; i < (int)snd_pcm_known_rates.count; i++) {
>  		if (hw->rates & (1 << i)) {
> -			hw->rate_min = snd_pcm_known_rates.list[i];
> -			break;
> -		}
> -	}
> -	for (i = (int)snd_pcm_known_rates.count - 1; i >= 0; i--) {
> -		if (hw->rates & (1 << i)) {
> -			hw->rate_max = snd_pcm_known_rates.list[i];
> -			break;
> +			rmin = min(rmin, snd_pcm_known_rates.list[i]);
> +			rmax = max(rmax, snd_pcm_known_rates.list[i]);
>  		}
>  	}
> +	if (rmin > rmax)
> +		return -EINVAL;
> +	hw->rate_min = rmin;
> +	hw->rate_max = rmax;
>  	return 0;
>  }
>  EXPORT_SYMBOL(snd_pcm_hw_limit_rates);
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 7461a727615c..5e1e6006707b 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2418,13 +2418,17 @@ static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params,
>  	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
>  }
>  
> -#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_768000 != 1 << 19
> +#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12 ||\
> +	SNDRV_PCM_RATE_128000 != 1 << 19
>  #error "Change this table"
>  #endif
>  
> +/* NOTE: the list is unsorted! */
>  static const unsigned int rates[] = {
> -	5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000, 64000,
> -	88200, 96000, 128000, 176400, 192000, 352800, 384000, 705600, 768000,
> +	5512, 8000, 11025, 16000, 22050, 32000, 44100,
> +	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000, 705600, 768000,
> +	/* extended */
> +	12000, 24000, 128000
>  };
>  
>  const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = {

I've quickly tested this version with a few rates (48, 128, 768k),
amlogic device.

Looks Ok.

Tested-by: Jerome Brunet <jbrunet@baylibre.com>

Can't say for topology.
Bard Liao Sept. 11, 2024, 12:59 p.m. UTC | #10
On 9/11/2024 6:51 PM, Takashi Iwai wrote:
> On Wed, 11 Sep 2024 12:33:01 +0200,
> Péter Ujfalusi wrote:
>> On 11/09/2024 12:21, Takashi Iwai wrote:
>>>> Wondering if this is backwards compatible with the alsa-lib definitions,
>>>> specifically the topology parts which did unfortunately have a list of
>>>> rates that will map to a different index now:
>>>>
>>>>
>>>> typedef enum _snd_pcm_rates {
>>>> 	SND_PCM_RATE_UNKNOWN = -1,
>>>> 	SND_PCM_RATE_5512 = 0,
>>>> 	SND_PCM_RATE_8000,
>>>> 	SND_PCM_RATE_11025,
>>>> 	SND_PCM_RATE_16000,
>>>> 	SND_PCM_RATE_22050,
>>>> 	SND_PCM_RATE_32000,
>>>> 	SND_PCM_RATE_44100,
>>>> 	SND_PCM_RATE_48000,
>>>> 	SND_PCM_RATE_64000,
>>>> 	SND_PCM_RATE_88200,
>>>> 	SND_PCM_RATE_96000,
>>>> 	SND_PCM_RATE_176400,
>>>> 	SND_PCM_RATE_192000,
>>>> 	SND_PCM_RATE_CONTINUOUS = 30,
>>>> 	SND_PCM_RATE_KNOT = 31,
>>>> 	SND_PCM_RATE_LAST = SND_PCM_RATE_KNOT,
>>>> } snd_pcm_rates_t;
>>> As far as I understand correctly, those rate bits used for topology
>>> are independent from the bits used for PCM core, although it used to
>>> be the same.  Maybe better to rename (such as SND_TPLG_RATE_*) so that
>>> it's clearer only for topology stuff.
>> Even if we rename these in alsa-lib we will need translation from
>> SND_TPLG_RATE_ to SND_PCM_RATE_ in kernel likely?
>>
>> The topology files are out there and this is an ABI...
>>
>>> But it'd be better if anyone can double-check.
>> Since the kernel just copies the rates bitfield, any rate above 11025
>> will be misaligned and result broken setup.
> Yep, I noticed it now, too.
>
> Below is the fix patch, totally untested.
> It'd be appreciated if anyone can test it quickly.
>
>
> thanks,
>
> Takashi


I tested it on my laptop and it fixed the issue.

Tested-by: Bard Liao <yung-chuan.liao@linux.intel.com>

> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: pcm: Fix breakage of PCM rates used for topology
>
> It turned out that the topology ABI takes the standard PCM rate bits
> as is, and it means that the recent change of the PCM rate bits would
> lead to the inconsistent rate values used for topology.
>
> This patch reverts the original PCM rate bit definitions while adding
> the new rates to the extended bits instead.  This needed the change of
> snd_pcm_known_rates, too.  And this also required to fix the handling
> in snd_pcm_hw_limit_rates() that blindly assumed that the list is
> sorted while it became unsorted now.
>
> Fixes: 090624b7dc83 ("ALSA: pcm: add more sample rate definitions")
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   include/sound/pcm.h     | 35 ++++++++++++++++++-----------------
>   sound/core/pcm_misc.c   | 18 ++++++++++--------
>   sound/core/pcm_native.c | 10 +++++++---
>   3 files changed, 35 insertions(+), 28 deletions(-)
>
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index c993350975a9..824216799070 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -109,23 +109,24 @@ struct snd_pcm_ops {
>   #define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
>   #define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
>   #define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
> -#define SNDRV_PCM_RATE_12000		(1U<<3)		/* 12000Hz */
> -#define SNDRV_PCM_RATE_16000		(1U<<4)		/* 16000Hz */
> -#define SNDRV_PCM_RATE_22050		(1U<<5)		/* 22050Hz */
> -#define SNDRV_PCM_RATE_24000		(1U<<6)		/* 24000Hz */
> -#define SNDRV_PCM_RATE_32000		(1U<<7)		/* 32000Hz */
> -#define SNDRV_PCM_RATE_44100		(1U<<8)		/* 44100Hz */
> -#define SNDRV_PCM_RATE_48000		(1U<<9)		/* 48000Hz */
> -#define SNDRV_PCM_RATE_64000		(1U<<10)	/* 64000Hz */
> -#define SNDRV_PCM_RATE_88200		(1U<<11)	/* 88200Hz */
> -#define SNDRV_PCM_RATE_96000		(1U<<12)	/* 96000Hz */
> -#define SNDRV_PCM_RATE_128000		(1U<<13)	/* 128000Hz */
> -#define SNDRV_PCM_RATE_176400		(1U<<14)	/* 176400Hz */
> -#define SNDRV_PCM_RATE_192000		(1U<<15)	/* 192000Hz */
> -#define SNDRV_PCM_RATE_352800		(1U<<16)	/* 352800Hz */
> -#define SNDRV_PCM_RATE_384000		(1U<<17)	/* 384000Hz */
> -#define SNDRV_PCM_RATE_705600		(1U<<18)	/* 705600Hz */
> -#define SNDRV_PCM_RATE_768000		(1U<<19)	/* 768000Hz */
> +#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
> +#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
> +#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
> +#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
> +#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
> +#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
> +#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
> +#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
> +#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
> +#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
> +#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
> +#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
> +#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
> +#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
> +/* extended rates */
> +#define SNDRV_PCM_RATE_12000		(1U<<17)	/* 12000Hz */
> +#define SNDRV_PCM_RATE_24000		(1U<<18)	/* 24000Hz */
> +#define SNDRV_PCM_RATE_128000		(1U<<19)	/* 128000Hz */
>   
>   #define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
>   #define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
> diff --git a/sound/core/pcm_misc.c b/sound/core/pcm_misc.c
> index 5588b6a1ee8b..4f556211bb56 100644
> --- a/sound/core/pcm_misc.c
> +++ b/sound/core/pcm_misc.c
> @@ -494,18 +494,20 @@ EXPORT_SYMBOL(snd_pcm_format_set_silence);
>   int snd_pcm_hw_limit_rates(struct snd_pcm_hardware *hw)
>   {
>   	int i;
> +	unsigned int rmin, rmax;
> +
> +	rmin = UINT_MAX;
> +	rmax = 0;
>   	for (i = 0; i < (int)snd_pcm_known_rates.count; i++) {
>   		if (hw->rates & (1 << i)) {
> -			hw->rate_min = snd_pcm_known_rates.list[i];
> -			break;
> -		}
> -	}
> -	for (i = (int)snd_pcm_known_rates.count - 1; i >= 0; i--) {
> -		if (hw->rates & (1 << i)) {
> -			hw->rate_max = snd_pcm_known_rates.list[i];
> -			break;
> +			rmin = min(rmin, snd_pcm_known_rates.list[i]);
> +			rmax = max(rmax, snd_pcm_known_rates.list[i]);
>   		}
>   	}
> +	if (rmin > rmax)
> +		return -EINVAL;
> +	hw->rate_min = rmin;
> +	hw->rate_max = rmax;
>   	return 0;
>   }
>   EXPORT_SYMBOL(snd_pcm_hw_limit_rates);
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 7461a727615c..5e1e6006707b 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2418,13 +2418,17 @@ static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params,
>   	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
>   }
>   
> -#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_768000 != 1 << 19
> +#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12 ||\
> +	SNDRV_PCM_RATE_128000 != 1 << 19
>   #error "Change this table"
>   #endif
>   
> +/* NOTE: the list is unsorted! */
>   static const unsigned int rates[] = {
> -	5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000, 64000,
> -	88200, 96000, 128000, 176400, 192000, 352800, 384000, 705600, 768000,
> +	5512, 8000, 11025, 16000, 22050, 32000, 44100,
> +	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000, 705600, 768000,
> +	/* extended */
> +	12000, 24000, 128000
>   };
>   
>   const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = {
Jerome Brunet Sept. 11, 2024, 12:59 p.m. UTC | #11
On Wed 11 Sep 2024 at 14:42, Takashi Iwai <tiwai@suse.de> wrote:

> On Wed, 11 Sep 2024 12:58:53 +0200,
> Jaroslav Kysela wrote:
>> 
>> On 11. 09. 24 12:51, Takashi Iwai wrote:
>> > On Wed, 11 Sep 2024 12:33:01 +0200,
>> > Péter Ujfalusi wrote:
>> >> 
>> >> On 11/09/2024 12:21, Takashi Iwai wrote:
>> >>>> Wondering if this is backwards compatible with the alsa-lib definitions,
>> >>>> specifically the topology parts which did unfortunately have a list of
>> >>>> rates that will map to a different index now:
>> >>>> 
>> >>>> 
>> >>>> typedef enum _snd_pcm_rates {
>> >>>> 	SND_PCM_RATE_UNKNOWN = -1,
>> >>>> 	SND_PCM_RATE_5512 = 0,
>> >>>> 	SND_PCM_RATE_8000,
>> >>>> 	SND_PCM_RATE_11025,
>> >>>> 	SND_PCM_RATE_16000,
>> >>>> 	SND_PCM_RATE_22050,
>> >>>> 	SND_PCM_RATE_32000,
>> >>>> 	SND_PCM_RATE_44100,
>> >>>> 	SND_PCM_RATE_48000,
>> >>>> 	SND_PCM_RATE_64000,
>> >>>> 	SND_PCM_RATE_88200,
>> >>>> 	SND_PCM_RATE_96000,
>> >>>> 	SND_PCM_RATE_176400,
>> >>>> 	SND_PCM_RATE_192000,
>> >>>> 	SND_PCM_RATE_CONTINUOUS = 30,
>> >>>> 	SND_PCM_RATE_KNOT = 31,
>> >>>> 	SND_PCM_RATE_LAST = SND_PCM_RATE_KNOT,
>> >>>> } snd_pcm_rates_t;
>> >>> 
>> >>> As far as I understand correctly, those rate bits used for topology
>> >>> are independent from the bits used for PCM core, although it used to
>> >>> be the same.  Maybe better to rename (such as SND_TPLG_RATE_*) so that
>> >>> it's clearer only for topology stuff.
>> >> 
>> >> Even if we rename these in alsa-lib we will need translation from
>> >> SND_TPLG_RATE_ to SND_PCM_RATE_ in kernel likely?
>> >> 
>> >> The topology files are out there and this is an ABI...
>> >> 
>> >>> But it'd be better if anyone can double-check.
>> >> 
>> >> Since the kernel just copies the rates bitfield, any rate above 11025
>> >> will be misaligned and result broken setup.
>> > 
>> > Yep, I noticed it now, too.
>> > 
>> > Below is the fix patch, totally untested.
>> > It'd be appreciated if anyone can test it quickly.
>> > 
>> > 
>> > thanks,
>> > 
>> > Takashi
>> > 
>> > -- 8< --
>> > From: Takashi Iwai <tiwai@suse.de>
>> > Subject: [PATCH] ALSA: pcm: Fix breakage of PCM rates used for topology
>> > 
>> > It turned out that the topology ABI takes the standard PCM rate bits
>> > as is, and it means that the recent change of the PCM rate bits would
>> > lead to the inconsistent rate values used for topology.
>> > 
>> > This patch reverts the original PCM rate bit definitions while adding
>> > the new rates to the extended bits instead.  This needed the change of
>> > snd_pcm_known_rates, too.  And this also required to fix the handling
>> > in snd_pcm_hw_limit_rates() that blindly assumed that the list is
>> > sorted while it became unsorted now.
>> > 
>> > Fixes: 090624b7dc83 ("ALSA: pcm: add more sample rate definitions")
>> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> 
>> This looks fine. But the topology rate bits should not depend on those
>> bits. It's a bit pitty that the standard PCM ABI does not use those
>> bits for user space and we are doing this change just for topology
>> ABI.
>
> Yeah, and theoretically it's possible to fix in topology side, but
> it'll be more cumbersome.
>
> Although it's not really a part of PCM ABI, I believe we should move
> the PCM rate bit definitions to uapi, for showing that it's set in
> stone.  Something like below.
>
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: pcm: Move standard rate bit definitions into uapi
>
> Since the standard PCM rate bits are used for the topology ABI, it's a
> part of public ABI that must not be changed.  Move the definitions
> into uapi to indicate it more clearly.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  include/sound/pcm.h         | 26 --------------------------
>  include/uapi/sound/asound.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 824216799070..f28f6d6ac996 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -105,32 +105,6 @@ struct snd_pcm_ops {
>  
>  #define SNDRV_PCM_POS_XRUN		((snd_pcm_uframes_t)-1)
>  
> -/* If you change this don't forget to change rates[] table in pcm_native.c */
> -#define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
> -#define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
> -#define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
> -#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
> -#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
> -#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
> -#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
> -#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
> -#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
> -#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
> -#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
> -#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
> -#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
> -#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
> -#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
> -#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
> -#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
> -/* extended rates */

It is cosmetic but I wonder, does the extended really start here ?

From the table Pierre-Louis sent, I suppose that all the recently added rates could
been seen as extended too (352.8 to 768). Those did not mess with the
order though 

> -#define SNDRV_PCM_RATE_12000		(1U<<17)	/* 12000Hz */
> -#define SNDRV_PCM_RATE_24000		(1U<<18)	/* 24000Hz */
> -#define SNDRV_PCM_RATE_128000		(1U<<19)	/* 128000Hz */
> -
> -#define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
> -#define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
> -
>  #define SNDRV_PCM_RATE_8000_44100	(SNDRV_PCM_RATE_8000|SNDRV_PCM_RATE_11025|\
>  					 SNDRV_PCM_RATE_16000|SNDRV_PCM_RATE_22050|\
>  					 SNDRV_PCM_RATE_32000|SNDRV_PCM_RATE_44100)
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 4cd513215bcd..715ceb3eac7c 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -272,6 +272,32 @@ typedef int __bitwise snd_pcm_subformat_t;
>  #define	SNDRV_PCM_SUBFORMAT_MSBITS_24	((__force snd_pcm_subformat_t) 3)
>  #define	SNDRV_PCM_SUBFORMAT_LAST	SNDRV_PCM_SUBFORMAT_MSBITS_24
>  
> +/* Standard rate bits */
> +#define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
> +#define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
> +#define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
> +#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
> +#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
> +#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
> +#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
> +#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
> +#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
> +#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
> +#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
> +#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
> +#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
> +#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
> +#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
> +#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
> +#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
> +/* extended rates */
> +#define SNDRV_PCM_RATE_12000		(1U<<17)	/* 12000Hz */
> +#define SNDRV_PCM_RATE_24000		(1U<<18)	/* 24000Hz */
> +#define SNDRV_PCM_RATE_128000		(1U<<19)	/* 128000Hz */
> +
> +#define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
> +#define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
> +
>  #define SNDRV_PCM_INFO_MMAP		0x00000001	/* hardware supports mmap */
>  #define SNDRV_PCM_INFO_MMAP_VALID	0x00000002	/* period data are valid during transfer */
>  #define SNDRV_PCM_INFO_DOUBLE		0x00000004	/* Double buffering needed for PCM start/stop */
Takashi Iwai Sept. 11, 2024, 1:08 p.m. UTC | #12
On Wed, 11 Sep 2024 14:59:39 +0200,
Jerome Brunet wrote:
> 
> On Wed 11 Sep 2024 at 14:42, Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Wed, 11 Sep 2024 12:58:53 +0200,
> > Jaroslav Kysela wrote:
> >> 
> >> On 11. 09. 24 12:51, Takashi Iwai wrote:
> >> > On Wed, 11 Sep 2024 12:33:01 +0200,
> >> > Péter Ujfalusi wrote:
> >> >> 
> >> >> On 11/09/2024 12:21, Takashi Iwai wrote:
> >> >>>> Wondering if this is backwards compatible with the alsa-lib definitions,
> >> >>>> specifically the topology parts which did unfortunately have a list of
> >> >>>> rates that will map to a different index now:
> >> >>>> 
> >> >>>> 
> >> >>>> typedef enum _snd_pcm_rates {
> >> >>>> 	SND_PCM_RATE_UNKNOWN = -1,
> >> >>>> 	SND_PCM_RATE_5512 = 0,
> >> >>>> 	SND_PCM_RATE_8000,
> >> >>>> 	SND_PCM_RATE_11025,
> >> >>>> 	SND_PCM_RATE_16000,
> >> >>>> 	SND_PCM_RATE_22050,
> >> >>>> 	SND_PCM_RATE_32000,
> >> >>>> 	SND_PCM_RATE_44100,
> >> >>>> 	SND_PCM_RATE_48000,
> >> >>>> 	SND_PCM_RATE_64000,
> >> >>>> 	SND_PCM_RATE_88200,
> >> >>>> 	SND_PCM_RATE_96000,
> >> >>>> 	SND_PCM_RATE_176400,
> >> >>>> 	SND_PCM_RATE_192000,
> >> >>>> 	SND_PCM_RATE_CONTINUOUS = 30,
> >> >>>> 	SND_PCM_RATE_KNOT = 31,
> >> >>>> 	SND_PCM_RATE_LAST = SND_PCM_RATE_KNOT,
> >> >>>> } snd_pcm_rates_t;
> >> >>> 
> >> >>> As far as I understand correctly, those rate bits used for topology
> >> >>> are independent from the bits used for PCM core, although it used to
> >> >>> be the same.  Maybe better to rename (such as SND_TPLG_RATE_*) so that
> >> >>> it's clearer only for topology stuff.
> >> >> 
> >> >> Even if we rename these in alsa-lib we will need translation from
> >> >> SND_TPLG_RATE_ to SND_PCM_RATE_ in kernel likely?
> >> >> 
> >> >> The topology files are out there and this is an ABI...
> >> >> 
> >> >>> But it'd be better if anyone can double-check.
> >> >> 
> >> >> Since the kernel just copies the rates bitfield, any rate above 11025
> >> >> will be misaligned and result broken setup.
> >> > 
> >> > Yep, I noticed it now, too.
> >> > 
> >> > Below is the fix patch, totally untested.
> >> > It'd be appreciated if anyone can test it quickly.
> >> > 
> >> > 
> >> > thanks,
> >> > 
> >> > Takashi
> >> > 
> >> > -- 8< --
> >> > From: Takashi Iwai <tiwai@suse.de>
> >> > Subject: [PATCH] ALSA: pcm: Fix breakage of PCM rates used for topology
> >> > 
> >> > It turned out that the topology ABI takes the standard PCM rate bits
> >> > as is, and it means that the recent change of the PCM rate bits would
> >> > lead to the inconsistent rate values used for topology.
> >> > 
> >> > This patch reverts the original PCM rate bit definitions while adding
> >> > the new rates to the extended bits instead.  This needed the change of
> >> > snd_pcm_known_rates, too.  And this also required to fix the handling
> >> > in snd_pcm_hw_limit_rates() that blindly assumed that the list is
> >> > sorted while it became unsorted now.
> >> > 
> >> > Fixes: 090624b7dc83 ("ALSA: pcm: add more sample rate definitions")
> >> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >> 
> >> This looks fine. But the topology rate bits should not depend on those
> >> bits. It's a bit pitty that the standard PCM ABI does not use those
> >> bits for user space and we are doing this change just for topology
> >> ABI.
> >
> > Yeah, and theoretically it's possible to fix in topology side, but
> > it'll be more cumbersome.
> >
> > Although it's not really a part of PCM ABI, I believe we should move
> > the PCM rate bit definitions to uapi, for showing that it's set in
> > stone.  Something like below.
> >
> >
> > Takashi
> >
> > -- 8< --
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] ALSA: pcm: Move standard rate bit definitions into uapi
> >
> > Since the standard PCM rate bits are used for the topology ABI, it's a
> > part of public ABI that must not be changed.  Move the definitions
> > into uapi to indicate it more clearly.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  include/sound/pcm.h         | 26 --------------------------
> >  include/uapi/sound/asound.h | 26 ++++++++++++++++++++++++++
> >  2 files changed, 26 insertions(+), 26 deletions(-)
> >
> > diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> > index 824216799070..f28f6d6ac996 100644
> > --- a/include/sound/pcm.h
> > +++ b/include/sound/pcm.h
> > @@ -105,32 +105,6 @@ struct snd_pcm_ops {
> >  
> >  #define SNDRV_PCM_POS_XRUN		((snd_pcm_uframes_t)-1)
> >  
> > -/* If you change this don't forget to change rates[] table in pcm_native.c */
> > -#define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
> > -#define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
> > -#define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
> > -#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
> > -#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
> > -#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
> > -#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
> > -#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
> > -#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
> > -#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
> > -#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
> > -#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
> > -#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
> > -#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
> > -#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
> > -#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
> > -#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
> > -/* extended rates */
> 
> It is cosmetic but I wonder, does the extended really start here ?

Maybe a bad choice of the words.  This was rather meant as the
extension since 6.12.  So I'll replace it with "extended rates since
6.12", to be clearer.

> From the table Pierre-Louis sent, I suppose that all the recently added rates could
> been seen as extended too (352.8 to 768). Those did not mess with the
> order though 

AFAIU, the topology stuff seems supporting only up to 192kHz for now,
but it's a matter of topology-only; the limitation could be commented
in somewhere in topology's headers, but it's basically independent
from the definitions in pcm.h.


thanks,

Takashi
Amadeusz Sławiński Sept. 11, 2024, 1:37 p.m. UTC | #13
On 9/11/2024 2:42 PM, Takashi Iwai wrote:
> On Wed, 11 Sep 2024 12:58:53 +0200,
> Jaroslav Kysela wrote:
>>
>> On 11. 09. 24 12:51, Takashi Iwai wrote:
>>> On Wed, 11 Sep 2024 12:33:01 +0200,
>>> Péter Ujfalusi wrote:
>>>>
>>>> On 11/09/2024 12:21, Takashi Iwai wrote:
>>>>>> Wondering if this is backwards compatible with the alsa-lib definitions,
>>>>>> specifically the topology parts which did unfortunately have a list of
>>>>>> rates that will map to a different index now:
>>>>>>
>>>>>>
>>>>>> typedef enum _snd_pcm_rates {
>>>>>> 	SND_PCM_RATE_UNKNOWN = -1,
>>>>>> 	SND_PCM_RATE_5512 = 0,
>>>>>> 	SND_PCM_RATE_8000,
>>>>>> 	SND_PCM_RATE_11025,
>>>>>> 	SND_PCM_RATE_16000,
>>>>>> 	SND_PCM_RATE_22050,
>>>>>> 	SND_PCM_RATE_32000,
>>>>>> 	SND_PCM_RATE_44100,
>>>>>> 	SND_PCM_RATE_48000,
>>>>>> 	SND_PCM_RATE_64000,
>>>>>> 	SND_PCM_RATE_88200,
>>>>>> 	SND_PCM_RATE_96000,
>>>>>> 	SND_PCM_RATE_176400,
>>>>>> 	SND_PCM_RATE_192000,
>>>>>> 	SND_PCM_RATE_CONTINUOUS = 30,
>>>>>> 	SND_PCM_RATE_KNOT = 31,
>>>>>> 	SND_PCM_RATE_LAST = SND_PCM_RATE_KNOT,
>>>>>> } snd_pcm_rates_t;
>>>>>
>>>>> As far as I understand correctly, those rate bits used for topology
>>>>> are independent from the bits used for PCM core, although it used to
>>>>> be the same.  Maybe better to rename (such as SND_TPLG_RATE_*) so that
>>>>> it's clearer only for topology stuff.
>>>>
>>>> Even if we rename these in alsa-lib we will need translation from
>>>> SND_TPLG_RATE_ to SND_PCM_RATE_ in kernel likely?
>>>>
>>>> The topology files are out there and this is an ABI...
>>>>
>>>>> But it'd be better if anyone can double-check.
>>>>
>>>> Since the kernel just copies the rates bitfield, any rate above 11025
>>>> will be misaligned and result broken setup.
>>>
>>> Yep, I noticed it now, too.
>>>
>>> Below is the fix patch, totally untested.
>>> It'd be appreciated if anyone can test it quickly.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>> -- 8< --
>>> From: Takashi Iwai <tiwai@suse.de>
>>> Subject: [PATCH] ALSA: pcm: Fix breakage of PCM rates used for topology
>>>
>>> It turned out that the topology ABI takes the standard PCM rate bits
>>> as is, and it means that the recent change of the PCM rate bits would
>>> lead to the inconsistent rate values used for topology.
>>>
>>> This patch reverts the original PCM rate bit definitions while adding
>>> the new rates to the extended bits instead.  This needed the change of
>>> snd_pcm_known_rates, too.  And this also required to fix the handling
>>> in snd_pcm_hw_limit_rates() that blindly assumed that the list is
>>> sorted while it became unsorted now.
>>>
>>> Fixes: 090624b7dc83 ("ALSA: pcm: add more sample rate definitions")
>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>
>> This looks fine. But the topology rate bits should not depend on those
>> bits. It's a bit pitty that the standard PCM ABI does not use those
>> bits for user space and we are doing this change just for topology
>> ABI.
> 
> Yeah, and theoretically it's possible to fix in topology side, but
> it'll be more cumbersome.
> 
> Although it's not really a part of PCM ABI, I believe we should move
> the PCM rate bit definitions to uapi, for showing that it's set in
> stone.  Something like below.
> 
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: pcm: Move standard rate bit definitions into uapi
> 
> Since the standard PCM rate bits are used for the topology ABI, it's a
> part of public ABI that must not be changed.  Move the definitions
> into uapi to indicate it more clearly.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   include/sound/pcm.h         | 26 --------------------------
>   include/uapi/sound/asound.h | 26 ++++++++++++++++++++++++++
>   2 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 824216799070..f28f6d6ac996 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -105,32 +105,6 @@ struct snd_pcm_ops {
>   
>   #define SNDRV_PCM_POS_XRUN		((snd_pcm_uframes_t)-1)
>   
> -/* If you change this don't forget to change rates[] table in pcm_native.c */
> -#define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
> -#define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
> -#define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
> -#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
> -#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
> -#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
> -#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
> -#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
> -#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
> -#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
> -#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
> -#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
> -#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
> -#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
> -#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
> -#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
> -#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
> -/* extended rates */
> -#define SNDRV_PCM_RATE_12000		(1U<<17)	/* 12000Hz */
> -#define SNDRV_PCM_RATE_24000		(1U<<18)	/* 24000Hz */
> -#define SNDRV_PCM_RATE_128000		(1U<<19)	/* 128000Hz */
> -
> -#define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
> -#define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
> -
>   #define SNDRV_PCM_RATE_8000_44100	(SNDRV_PCM_RATE_8000|SNDRV_PCM_RATE_11025|\
>   					 SNDRV_PCM_RATE_16000|SNDRV_PCM_RATE_22050|\
>   					 SNDRV_PCM_RATE_32000|SNDRV_PCM_RATE_44100)
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 4cd513215bcd..715ceb3eac7c 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -272,6 +272,32 @@ typedef int __bitwise snd_pcm_subformat_t;
>   #define	SNDRV_PCM_SUBFORMAT_MSBITS_24	((__force snd_pcm_subformat_t) 3)
>   #define	SNDRV_PCM_SUBFORMAT_LAST	SNDRV_PCM_SUBFORMAT_MSBITS_24
>   
> +/* Standard rate bits */
> +#define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
> +#define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
> +#define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
> +#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
> +#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
> +#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
> +#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
> +#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
> +#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
> +#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
> +#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
> +#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
> +#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
> +#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
> +#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
> +#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
> +#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
> +/* extended rates */
> +#define SNDRV_PCM_RATE_12000		(1U<<17)	/* 12000Hz */
> +#define SNDRV_PCM_RATE_24000		(1U<<18)	/* 24000Hz */
> +#define SNDRV_PCM_RATE_128000		(1U<<19)	/* 128000Hz */
> +
> +#define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
> +#define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
> +
>   #define SNDRV_PCM_INFO_MMAP		0x00000001	/* hardware supports mmap */
>   #define SNDRV_PCM_INFO_MMAP_VALID	0x00000002	/* period data are valid during transfer */
>   #define SNDRV_PCM_INFO_DOUBLE		0x00000004	/* Double buffering needed for PCM start/stop */

I will just note that alternatively we could bump topology ABI (wouldn't 
be first time) and provide mapping in soc-topology.c for current one.

For ABI+1 we could remove the field from problematic topology struct to 
not make SNDRV_PCM_RATE_* part of UAPI and provide some other way to 
pass expected rates.
diff mbox series

Patch

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 732121b934fd..c993350975a9 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -109,20 +109,23 @@  struct snd_pcm_ops {
 #define SNDRV_PCM_RATE_5512		(1U<<0)		/* 5512Hz */
 #define SNDRV_PCM_RATE_8000		(1U<<1)		/* 8000Hz */
 #define SNDRV_PCM_RATE_11025		(1U<<2)		/* 11025Hz */
-#define SNDRV_PCM_RATE_16000		(1U<<3)		/* 16000Hz */
-#define SNDRV_PCM_RATE_22050		(1U<<4)		/* 22050Hz */
-#define SNDRV_PCM_RATE_32000		(1U<<5)		/* 32000Hz */
-#define SNDRV_PCM_RATE_44100		(1U<<6)		/* 44100Hz */
-#define SNDRV_PCM_RATE_48000		(1U<<7)		/* 48000Hz */
-#define SNDRV_PCM_RATE_64000		(1U<<8)		/* 64000Hz */
-#define SNDRV_PCM_RATE_88200		(1U<<9)		/* 88200Hz */
-#define SNDRV_PCM_RATE_96000		(1U<<10)	/* 96000Hz */
-#define SNDRV_PCM_RATE_176400		(1U<<11)	/* 176400Hz */
-#define SNDRV_PCM_RATE_192000		(1U<<12)	/* 192000Hz */
-#define SNDRV_PCM_RATE_352800		(1U<<13)	/* 352800Hz */
-#define SNDRV_PCM_RATE_384000		(1U<<14)	/* 384000Hz */
-#define SNDRV_PCM_RATE_705600		(1U<<15)	/* 705600Hz */
-#define SNDRV_PCM_RATE_768000		(1U<<16)	/* 768000Hz */
+#define SNDRV_PCM_RATE_12000		(1U<<3)		/* 12000Hz */
+#define SNDRV_PCM_RATE_16000		(1U<<4)		/* 16000Hz */
+#define SNDRV_PCM_RATE_22050		(1U<<5)		/* 22050Hz */
+#define SNDRV_PCM_RATE_24000		(1U<<6)		/* 24000Hz */
+#define SNDRV_PCM_RATE_32000		(1U<<7)		/* 32000Hz */
+#define SNDRV_PCM_RATE_44100		(1U<<8)		/* 44100Hz */
+#define SNDRV_PCM_RATE_48000		(1U<<9)		/* 48000Hz */
+#define SNDRV_PCM_RATE_64000		(1U<<10)	/* 64000Hz */
+#define SNDRV_PCM_RATE_88200		(1U<<11)	/* 88200Hz */
+#define SNDRV_PCM_RATE_96000		(1U<<12)	/* 96000Hz */
+#define SNDRV_PCM_RATE_128000		(1U<<13)	/* 128000Hz */
+#define SNDRV_PCM_RATE_176400		(1U<<14)	/* 176400Hz */
+#define SNDRV_PCM_RATE_192000		(1U<<15)	/* 192000Hz */
+#define SNDRV_PCM_RATE_352800		(1U<<16)	/* 352800Hz */
+#define SNDRV_PCM_RATE_384000		(1U<<17)	/* 384000Hz */
+#define SNDRV_PCM_RATE_705600		(1U<<18)	/* 705600Hz */
+#define SNDRV_PCM_RATE_768000		(1U<<19)	/* 768000Hz */
 
 #define SNDRV_PCM_RATE_CONTINUOUS	(1U<<30)	/* continuous range */
 #define SNDRV_PCM_RATE_KNOT		(1U<<31)	/* supports more non-continuous rates */
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 44381514f695..7461a727615c 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2418,13 +2418,13 @@  static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params,
 	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
 }
 
-#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12
+#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_768000 != 1 << 19
 #error "Change this table"
 #endif
 
 static const unsigned int rates[] = {
-	5512, 8000, 11025, 16000, 22050, 32000, 44100,
-	48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000, 705600, 768000
+	5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000, 64000,
+	88200, 96000, 128000, 176400, 192000, 352800, 384000, 705600, 768000,
 };
 
 const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = {