[Sound-open-firmware] [PATCH 3/8] volume: add 16bit<==>24bit volume copy function and mapping

[Jie, Yang]

> -----Original Message-----
> From: Liam Girdwood [mailto:liam.r.girdwood at linux.intel.com]
> Sent: Tuesday, December 20, 2016 6:31 PM
> To: Keyon Jie <yang.jie at linux.intel.com>
> Cc: sound-open-firmware at alsa-project.org; Zhang, Keqiao
> <keqiao.zhang at intel.com>; Jie, Yang <yang.jie at intel.com>; Ingalsuo, Seppo
> <seppo.ingalsuo at intel.com>; Lin, Mengdong <mengdong.lin at intel.com>
> Subject: Re: [Sound-open-firmware] [PATCH 3/8] volume: add 16bit<==>24bit
> volume copy function and mapping
> 
> On Tue, 2016-12-20 at 15:46 +0800, Keyon Jie wrote:
> > add 16bit<==>24bit volume copy function and mapping, for
> > 24 bits ssp output/input.
> >
> > Signed-off-by: Keyon Jie <yang.jie at linux.intel.com>
> > ---
> >  src/audio/volume.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >
> > diff --git a/src/audio/volume.c b/src/audio/volume.c index
> > f5528cd..5299edd 100644
> > --- a/src/audio/volume.c
> > +++ b/src/audio/volume.c
> > @@ -172,12 +172,55 @@ static void vol_s16_to_s16(struct comp_dev *dev,
> struct comp_buffer *sink,
> >  	sink->w_ptr = dest;
> >  }
> >
> > +/* copy and scale volume from 16 bit source buffer to 24 bit on 32
> > +bit boundary dest buffer */ static void vol_s16_to_s24(struct comp_dev *dev,
> struct comp_buffer *sink,
> > +	struct comp_buffer *source, uint32_t frames) {
> > +	struct comp_data *cd = comp_get_drvdata(dev);
> > +	int16_t *src = (int16_t*) source->r_ptr;
> > +	int32_t val, i, *dest = (int32_t*) sink->w_ptr;
> > +
> > +	/* buffer sizes are always divisible by period frames */
> > +	for (i = 0; i < frames * source->params.channels; i++) {
> > +		val = (int32_t)*src;
> > +		*dest = (val * cd->volume[i % source->params.channels]) >> 8;
> 
> We must not use % or / in pipeline copy code since they are very expensive in
> terms of cycles.

OK, it do add numbers of instruction lines with this, thanks for pointing out,
will change in next version.

Thanks,
~Keyon

> 
> What we should do here is have a version of each function, for 2 chan, 4, chan,
> etc (select at params time).
> 
> e.g. 2 channel volume
> 
> 	/* buffer sizes are always divisible by period frames */
> 	for (i = 0; i < frames * 2; i += 2) {
> 		val[0] = (int32_t)src[i];
> 		val[1] = (int32_t)src[i + 1];
> 
> 		dest[i] = (int16_t)(((val[0] >> 8) *
> 			cd->volume[0]) >> 16);
> 		dest[i + 1] = (int16_t)(((val[1] >> 8) *
> 			cd->volume[1]) >> 16);
> 	}
> 
> The above code does the same, but doesnt use % and is vectorisable by gcc (and
> also easier to add intrinsics).
> 
> Our goal is to keep the copy() functions as optimal as possible (even at the
> expense of adding a little more code for copy functions using a different number
> of channels).
> 
> Liam
> 
> > +		dest++;
> > +		src++;
> > +	}
> > +
> > +	source->r_ptr = src;
> > +	sink->w_ptr = dest;
> > +}
> > +
> > +/* copy and scale volume from 16 bit source buffer to 24 bit on 32
> > +bit boundary dest buffer */ static void vol_s24_to_s16(struct comp_dev *dev,
> struct comp_buffer *sink,
> > +	struct comp_buffer *source, uint32_t frames) {
> > +	struct comp_data *cd = comp_get_drvdata(dev);
> > +	int32_t val, i, *src = (int32_t*) source->r_ptr;
> > +	int16_t *dest = (int16_t*) sink->w_ptr;
> > +
> > +	/* buffer sizes are always divisible by period frames */
> > +	for (i = 0; i < frames * source->params.channels; i++) {
> > +		val = (int32_t)*src;
> > +		*dest = (int16_t)(((val >> 8) *
> > +			cd->volume[i % source->params.channels]) >> 16);
> > +		dest++;
> > +		src++;
> > +	}
> > +
> > +	source->r_ptr = src;
> > +	sink->w_ptr = dest;
> > +}
> > +
> >  /* map of source and sink buffer formats to volume function */
> > static const struct comp_func_map func_map[] = {
> >  	{STREAM_FORMAT_S16_LE, STREAM_FORMAT_S16_LE,
> vol_s16_to_s16},
> >  	{STREAM_FORMAT_S16_LE, STREAM_FORMAT_S32_LE,
> vol_s16_to_s32},
> >  	{STREAM_FORMAT_S32_LE, STREAM_FORMAT_S16_LE,
> vol_s32_to_s16},
> >  	{STREAM_FORMAT_S32_LE, STREAM_FORMAT_S32_LE,
> vol_s32_to_s32},
> > +	{STREAM_FORMAT_S16_LE, STREAM_FORMAT_S24_4LE,
> vol_s16_to_s24},
> > +	{STREAM_FORMAT_S24_4LE, STREAM_FORMAT_S16_LE,
> vol_s24_to_s16},
> >  };
> >
> >  static void vol_update(struct comp_data *cd, uint32_t chan)
>