[alsa-devel] [PATCH] pcm: softvol: add support for S24_LE
Tested with a Wolfson WM8524 DAC on a i.MX6UL board running Linux version 4.13.0-next-20170907.
Signed-off-by: Jörg Krause joerg.krause@embedded.rocks --- src/pcm/pcm_softvol.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/pcm/pcm_softvol.c b/src/pcm/pcm_softvol.c index 1fe5784d..d159fb1f 100644 --- a/src/pcm/pcm_softvol.c +++ b/src/pcm/pcm_softvol.c @@ -309,6 +309,8 @@ static void softvol_convert_stereo_vol(snd_pcm_softvol_t *svol, CONVERT_AREA(short, !snd_pcm_format_cpu_endian(svol->sformat)); break; + case SND_PCM_FORMAT_S24_LE: + /* 24bit samples, fallthrough */ case SND_PCM_FORMAT_S32_LE: case SND_PCM_FORMAT_S32_BE: /* 32bit samples */ @@ -360,6 +362,8 @@ static void softvol_convert_mono_vol(snd_pcm_softvol_t *svol, CONVERT_AREA(short, !snd_pcm_format_cpu_endian(svol->sformat)); break; + case SND_PCM_FORMAT_S24_LE: + /* 24bit samples, fallthrough */ case SND_PCM_FORMAT_S32_LE: case SND_PCM_FORMAT_S32_BE: /* 32bit samples */ @@ -422,6 +426,7 @@ static int snd_pcm_softvol_hw_refine_cprepare(snd_pcm_t *pcm, { (1ULL << SND_PCM_FORMAT_S16_LE) | (1ULL << SND_PCM_FORMAT_S16_BE) | + (1ULL << SND_PCM_FORMAT_S24_LE) | (1ULL << SND_PCM_FORMAT_S32_LE) | (1ULL << SND_PCM_FORMAT_S32_BE), (1ULL << (SND_PCM_FORMAT_S24_3LE - 32)) @@ -577,10 +582,11 @@ static int snd_pcm_softvol_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t * param if (slave->format != SND_PCM_FORMAT_S16_LE && slave->format != SND_PCM_FORMAT_S16_BE && slave->format != SND_PCM_FORMAT_S24_3LE && + slave->format != SND_PCM_FORMAT_S24_LE && slave->format != SND_PCM_FORMAT_S32_LE && slave->format != SND_PCM_FORMAT_S32_BE) { - SNDERR("softvol supports only S16_LE, S16_BE, S24_3LE, S32_LE " - " or S32_BE"); + SNDERR("softvol supports only S16_LE, S16_BE, S24_LE, S24_3LE, " + "S32_LE or S32_BE"); return -EINVAL; } svol->sformat = slave->format; @@ -863,6 +869,7 @@ int snd_pcm_softvol_open(snd_pcm_t **pcmp, const char *name, sformat != SND_PCM_FORMAT_S16_LE && sformat != SND_PCM_FORMAT_S16_BE && sformat != SND_PCM_FORMAT_S24_3LE && + sformat != SND_PCM_FORMAT_S24_LE && sformat != SND_PCM_FORMAT_S32_LE && sformat != SND_PCM_FORMAT_S32_BE) return -EINVAL; @@ -1082,9 +1089,10 @@ int _snd_pcm_softvol_open(snd_pcm_t **pcmp, const char *name, sformat != SND_PCM_FORMAT_S16_LE && sformat != SND_PCM_FORMAT_S16_BE && sformat != SND_PCM_FORMAT_S24_3LE && + sformat != SND_PCM_FORMAT_S24_LE && sformat != SND_PCM_FORMAT_S32_LE && sformat != SND_PCM_FORMAT_S32_BE) { - SNDERR("only S16_LE, S16_BE, S24_3LE, S32_LE or S32_BE format is supported"); + SNDERR("only S16_LE, S16_BE, S24_LE, S24_3LE, S32_LE or S32_BE format is supported"); snd_config_delete(sconf); return -EINVAL; }
Jörg Krause wrote:
Tested with a Wolfson WM8524 DAC on a i.MX6UL board running Linux version 4.13.0-next-20170907.
- case SND_PCM_FORMAT_S24_LE:
/* 24bit samples, fallthrough */
S24_LE cannot be handled with the same algorithm as S32_LE.
I suspect that the hardware does not actually use S24_LE, and that the driver uses the wrong label for this format. Please describe exactly how the 24-bit sample is aligned into the 32-bit memory cell.
Regards, Clemens
On Fri, 2017-09-08 at 12:52 +0200, Clemens Ladisch wrote:
Jörg Krause wrote:
Tested with a Wolfson WM8524 DAC on a i.MX6UL board running Linux version 4.13.0-next-20170907.
- case SND_PCM_FORMAT_S24_LE:
/* 24bit samples, fallthrough */S24_LE cannot be handled with the same algorithm as S32_LE.
That would have been too easy.
I suspect that the hardware does not actually use S24_LE, and that the driver uses the wrong label for this format.
Maybe you are right about that. I will have a look.
Please describe exactly how the 24-bit sample is aligned into the 32-bit memory cell.
According to this old post [1] it is aligned as followed:
00ffff3f 00ffff3f 00ebd96e 00ebd96e 00ffff7f 00ffff7f Meaning the left-most byte is zeroed, right?
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2008-July/0090 99.html
Best regards, Jörg Krause
Jörg Krause wrote:
On Fri, 2017-09-08 at 12:52 +0200, Clemens Ladisch wrote:
Please describe exactly how the 24-bit sample is aligned into the 32-bit memory cell.
According to this old post [1] it is aligned as followed:
That post talks about S24_LE. I was asking about the actual hardware.
Meaning the left-most byte is zeroed, right?
What does "left" mean when talking about memory?
Regards, Clemens
On Tue, 2017-09-12 at 08:19 +0200, Clemens Ladisch wrote:
Jörg Krause wrote:
On Fri, 2017-09-08 at 12:52 +0200, Clemens Ladisch wrote:
Please describe exactly how the 24-bit sample is aligned into the 32-bit memory cell.
According to this old post [1] it is aligned as followed:
That post talks about S24_LE. I was asking about the actual hardware.
The i.MX6UL has a Synchronous Audio Interface (SAI), where the data in the FIFO can be aligned anywhere within the 32-bit wide register through the use of the First Bit Shifted configuration field. In the corresponding Linux SAI driver the FBS field is set the way that data alignment for 24-bit data is:
31 30 29 28 | 27 26 25 24 | 23 22 21 20 | .. | 3 2 1 0 ## ## ## ## ## ## ## ## [ DATA[23:0] ]
Meaning the left-most byte is zeroed, right?
What does "left" mean when talking about memory?
I should have said MSB first.
Jörg Krause wrote:
The i.MX6UL has a Synchronous Audio Interface (SAI), where the data in the FIFO can be aligned anywhere within the 32-bit wide register through the use of the First Bit Shifted configuration field. In the corresponding Linux SAI driver the FBS field is set the way that data alignment for 24-bit data is:
31 30 29 28 | 27 26 25 24 | 23 22 21 20 | .. | 3 2 1 0 ## ## ## ## ## ## ## ## [ DATA[23:0] ]
This indeed is S24_LE.
Which is an extremely uncommon format. If possible, the driver should be changed to align to the sample's MSB to the memory word's MSB, and then label it as S32_LE.
S24_LE cannot be handled with the same algorithm as S32_LE.
To be precise: the sign in bit 23 must be preserved.
Regards, Clemens
On Tue, 12 Sep 2017 09:02:47 +0200, Clemens Ladisch wrote:
Jörg Krause wrote:
The i.MX6UL has a Synchronous Audio Interface (SAI), where the data in the FIFO can be aligned anywhere within the 32-bit wide register through the use of the First Bit Shifted configuration field. In the corresponding Linux SAI driver the FBS field is set the way that data alignment for 24-bit data is:
31 30 29 28 | 27 26 25 24 | 23 22 21 20 | .. | 3 2 1 0 ## ## ## ## ## ## ## ## [ DATA[23:0] ]
This indeed is S24_LE.
Which is an extremely uncommon format. If possible, the driver should be changed to align to the sample's MSB to the memory word's MSB, and then label it as S32_LE.
S24_LE cannot be handled with the same algorithm as S32_LE.
To be precise: the sign in bit 23 must be preserved.
I guess the simple calculation with S24_LE using the current code would work as long as the upper 8 bits are ignored by hardware. The upper 8 bits would hold bogus bits, and clearing this would be an extra action we'd need in addition.
thanks,
Takashi
On Tue, 2017-09-12 at 09:12 +0200, Takashi Iwai wrote:
On Tue, 12 Sep 2017 09:02:47 +0200, Clemens Ladisch wrote:
Jörg Krause wrote:
The i.MX6UL has a Synchronous Audio Interface (SAI), where the data in the FIFO can be aligned anywhere within the 32-bit wide register through the use of the First Bit Shifted configuration field. In the corresponding Linux SAI driver the FBS field is set the way that data alignment for 24-bit data is:
31 30 29 28 | 27 26 25 24 | 23 22 21 20 | .. | 3 2 1 0 ## ## ## ## ## ## ## ## [ DATA[23:0] ]
This indeed is S24_LE.
Which is an extremely uncommon format. If possible, the driver should be changed to align to the sample's MSB to the memory word's MSB, and then label it as S32_LE.
S24_LE cannot be handled with the same algorithm as S32_LE.
To be precise: the sign in bit 23 must be preserved.
I guess the simple calculation with S24_LE using the current code would work as long as the upper 8 bits are ignored by hardware. The upper 8 bits would hold bogus bits, and clearing this would be an extra action we'd need in addition.
Thanks! I've copied the area convertion macro for S24_3LE and explicitly set the upper 8 bits to 0:
""" #define CONVERT_AREA_S24_LE() do { \ unsigned int ch, fr; \ unsigned char *src, *dst; \ int tmp; \ for (ch = 0; ch < channels; ch++) { \ src_area = &src_areas[ch]; \ dst_area = &dst_areas[ch]; \ src = snd_pcm_channel_area_addr(src_area, src_offset); \ dst = snd_pcm_channel_area_addr(dst_area, dst_offset); \ src_step = snd_pcm_channel_area_step(src_area); \ dst_step = snd_pcm_channel_area_step(dst_area); \ GET_VOL_SCALE; \ fr = frames; \ if (! vol_scale) { \ while (fr--) { \ dst[0] = dst[1] = dst[2] = dst[3] = 0; \ dst += dst_step; \ } \ } else if (vol_scale == 0xffff) { \ while (fr--) { \ dst[0] = src[0]; \ dst[1] = src[1]; \ dst[2] = src[2]; \ dst[3] = 0; \ src += dst_step; \ dst += src_step; \ } \ } else { \ while (fr--) { \ tmp = src[0] | \ (src[1] << 8) | \ (((signed char *) src)[2] << 16); \ tmp = MULTI_DIV_24(tmp, vol_scale); \ dst[0] = tmp; \ dst[1] = tmp >> 8; \ dst[2] = tmp >> 16; \ dst[3] = 0; \ src += dst_step; \ dst += src_step; \ } \ } \ } \ } while (0) """
Is that okay?
Jörg Krause
On Tue, 12 Sep 2017 16:15:58 +0200, Jörg Krause wrote:
On Tue, 2017-09-12 at 09:12 +0200, Takashi Iwai wrote:
On Tue, 12 Sep 2017 09:02:47 +0200, Clemens Ladisch wrote:
Jörg Krause wrote:
The i.MX6UL has a Synchronous Audio Interface (SAI), where the data in the FIFO can be aligned anywhere within the 32-bit wide register through the use of the First Bit Shifted configuration field. In the corresponding Linux SAI driver the FBS field is set the way that data alignment for 24-bit data is:
31 30 29 28 | 27 26 25 24 | 23 22 21 20 | .. | 3 2 1 0 ## ## ## ## ## ## ## ## [ DATA[23:0] ]
This indeed is S24_LE.
Which is an extremely uncommon format. If possible, the driver should be changed to align to the sample's MSB to the memory word's MSB, and then label it as S32_LE.
S24_LE cannot be handled with the same algorithm as S32_LE.
To be precise: the sign in bit 23 must be preserved.
I guess the simple calculation with S24_LE using the current code would work as long as the upper 8 bits are ignored by hardware. The upper 8 bits would hold bogus bits, and clearing this would be an extra action we'd need in addition.
Thanks! I've copied the area convertion macro for S24_3LE and explicitly set the upper 8 bits to 0:
""" #define CONVERT_AREA_S24_LE() do { \ unsigned int ch, fr; \ unsigned char *src, *dst; \ int tmp; \ for (ch = 0; ch < channels; ch++) { \ src_area = &src_areas[ch]; \ dst_area = &dst_areas[ch]; \ src = snd_pcm_channel_area_addr(src_area, src_offset); \ dst = snd_pcm_channel_area_addr(dst_area, dst_offset); \ src_step = snd_pcm_channel_area_step(src_area); \ dst_step = snd_pcm_channel_area_step(dst_area); \ GET_VOL_SCALE; \ fr = frames; \ if (! vol_scale) { \ while (fr--) { \ dst[0] = dst[1] = dst[2] = dst[3] = 0; \ dst += dst_step; \ } \ } else if (vol_scale == 0xffff) { \ while (fr--) { \ dst[0] = src[0]; \ dst[1] = src[1]; \ dst[2] = src[2]; \ dst[3] = 0; \ src += dst_step; \ dst += src_step; \ } \ } else { \ while (fr--) { \ tmp = src[0] | \ (src[1] << 8) | \ (((signed char *) src)[2] << 16); \ tmp = MULTI_DIV_24(tmp, vol_scale); \ dst[0] = tmp; \ dst[1] = tmp >> 8; \ dst[2] = tmp >> 16; \ dst[3] = 0; \ src += dst_step; \ dst += src_step; \ } \ } \ } \ } while (0) """
Is that okay?
You can simply mask 0xffffff (or 0xffffff00, depending on CPU-native or not) to the int value instead of assigning / calculating each byte. And it's needed only when vol_scale !=0 && vol_scale != 0xffff.
thanks,
Takashi
Hi,
On 2017年09月12日 23:32, Takashi Iwai wrote:
You can simply mask 0xffffff (or 0xffffff00, depending on CPU-native or not) to the int value instead of assigning / calculating each byte. And it's needed only when vol_scale !=0 && vol_scale != 0xffff.
Vumeter implementation of aplay in alsa-utils.git generates mask from return value of 'snd_pcm_format_silence[|_16|_32|_64]()'. I suppose this is also useful in your case.
Regards
Takashi Sakamoto
participants (4)
-
Clemens Ladisch -
Jörg Krause -
Takashi Iwai -
Takashi Sakamoto