[PATCH] pcm: softvol: add support for S24_LE

[PATCH] pcm: softvol: add support for S24_LE

[Jörg Krause]

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;
 		}
-- 
2.14.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] pcm: softvol: add support for S24_LE

[Clemens Ladisch]

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 */
>  	case SND_PCM_FORMAT_S32_LE:

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] pcm: softvol: add support for S24_LE

[Jörg Krause]

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 */
> >  	case SND_PCM_FORMAT_S32_LE:
> 
> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] pcm: softvol: add support for S24_LE

[Clemens Ladisch]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] pcm: softvol: add support for S24_LE

[Jörg Krause]

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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] pcm: softvol: add support for S24_LE

[Clemens Ladisch]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] pcm: softvol: add support for S24_LE

[Takashi Iwai]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] pcm: softvol: add support for S24_LE

[Jörg Krause]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] pcm: softvol: add support for S24_LE

[Takashi Iwai]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] pcm: softvol: add support for S24_LE

[Takashi Sakamoto]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel