[alsa-devel] [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