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 */