[Sound-open-firmware] [PATCH 3/5] Tone: Fix control ipc and add support for independent tones in channels
Seppo Ingalsuo
seppo.ingalsuo at linux.intel.com
Mon Oct 23 10:48:10 CEST 2017
On 21.10.2017 00:44, Pierre-Louis Bossart wrote:
> On 10/20/17 12:15 PM, Seppo Ingalsuo wrote:
>> The control interface is fixed to follow the component ABI. Separate
>> tone
>> generator instances are used to feed individual channels. It allows
>> simultaneous testing of all channels by use of different tone
>> frequencies.
>> To simplify features there is no mixer so e.g. DTMF telephony UI sounds
>> generation would need a channel mixer component after a two channel tone
>> output in the pipeline.
>
> [...]
>
>> struct comp_data {
>> uint32_t period_bytes;
>> uint32_t channels;
>> uint32_t frame_bytes;
>> uint32_t rate;
>> - struct tone_state sg;
>> + struct tone_state sg[PLATFORM_MAX_CHANNELS];
>> void (*tone_func)(struct comp_dev *dev, struct comp_buffer *sink,
>> - struct comp_buffer *source, uint32_t frames);
>> + uint32_t frames);
>
> should this patch be split in two, one that deals with control IPC and
> one with internal implementation changes? it'd make the review easier,
> no?
Yep, it's a fair request, will do.
>
>> };
>> +static int32_t tonegen(struct tone_state *sg);
>> +static void tonegen_control(struct tone_state *sg);
>> +static void tonegen_update_f(struct tone_state *sg, int32_t f);
>> +
>> /*
>> * Tone generator algorithm code
>> */
>> +static inline void tone_circ_inc_wrap(int32_t **ptr, int32_t *end,
>> size_t size)
>> +{
>> + if (*ptr >= end)
>> + *ptr = (int32_t *) ((size_t) * ptr - size);
>
> This is using pointer and integer arithmetic, not great.
I'd prefer in audio processing code to use indexes and arrays, count
samples and not size_t bytes, etc. The interface from SOF buffers with
pointers to C arrays needs some nice way so good ideas are welcome.
>
> The rest of the patch is hard to review with tons of changes...
The component was with obsolete control interface non-functional for a
long time so a lot changes need was accumulated. But I'll try 1st if
patch can be split (= possible to compile code between patches) without
need to work backwards with the code.
>
>> +}
>> +
>> static void tone_s32_default(struct comp_dev *dev, struct
>> comp_buffer *sink,
>> - struct comp_buffer *source, uint32_t frames)
>> + uint32_t frames)
>> {
>> struct comp_data *cd = comp_get_drvdata(dev);
>> - int32_t sine_sample;
>> int32_t *dest = (int32_t*) sink->w_ptr;
>> int i;
>> int n;
>> int n_wrap_dest;
>> + int n_min;
>> int nch = cd->channels;
>> n = frames * nch;
>> while (n > 0) {
>> n_wrap_dest = (int32_t *) sink->end_addr - dest;
>> - if (n < n_wrap_dest) {
>> - /* No circular wrap need */
>> - while (n > 0) {
>> - /* Update period count for sweeps, etc. */
>> - tonegen_control(&cd->sg);
>> - /* Calculate mono sine wave sample and then
>> - * duplicate to channels.
>> - */
>> - sine_sample = tonegen(&cd->sg);
>> - n -= nch;
>> - for (i = 0; i < nch; i++) {
>> - *dest = sine_sample;
>> - dest++;
>> - }
>> + n_min = (n < n_wrap_dest) ? n : n_wrap_dest;
>> + /* Process until wrap or completed n */
>> + while (n_min > 0) {
>> + n -= nch;
>> + n_min -= nch;
>> + for (i = 0; i < nch; i++) {
>> + tonegen_control(&cd->sg[i]);
>> + *dest = tonegen(&cd->sg[i]);
>> + dest++;
>> }
>> - } else {
>> - /* Process until wrap */
>> - while (n_wrap_dest > 0) {
>> - tonegen_control(&cd->sg);
>> - sine_sample = tonegen(&cd->sg);
>> - n -= nch;
>> - n_wrap_dest -= nch;
>> - for (i = 0; i < nch; i++) {
>> - *dest = sine_sample;
>> - dest++;
>> - }
>> - }
>> - /* No need to check if past end_addr,
>> - * it is so just subtract buffer size.
>> - */
>> - dest = (int32_t *) ((size_t) dest
>> - - sink->ipc_buffer.size);
>> }
>> + tone_circ_inc_wrap(&dest, sink->end_addr, sink->size);
>> }
>> - sink->w_ptr = dest;
>> }
>> static int32_t tonegen(struct tone_state *sg)
>> @@ -226,39 +237,46 @@ static void tonegen_control(struct tone_state *sg)
>> }
>> }
>> +/* Set sine amplitude */
>> static inline void tonegen_set_a(struct tone_state *sg, int32_t a)
>> {
>> sg->a_target = a;
>> }
>> -static inline void tonegen_set_f(struct tone_state *sg, int32_t f)
>> +/* Repeated number of beeps */
>> +static void tonegen_set_repeats(struct tone_state *sg, uint32_t r)
>> {
>> - sg->f = f;
>> + sg->repeats = r;
>> }
>> -#if 0
>> -/* Tone sweep parameters description:
>> - * fc - Multiplication factor for frequency as Q2.30 for logarithmic
>> change
>> - * ac - Multiplication factor for amplitude as Q2.30 for logarithmic
>> change
>> - * l - Tone length in samples, this is the active length of tone
>> - * p - Tone period in samples, this is the length including the
>> pause after beep
>> - * r - Repeated number of beeps
>> +/* The next functions support zero as shortcut for defaults to get
>> + * make a nicer API without need to remember the neutral steady
>> + * non-swept tone settings.
>> */
>> -static void tonegen_set_sweep(struct tone_state *sg, int32_t fc,
>> int32_t ac,
>> - uint32_t l, uint32_t p, uint32_t r)
>> +/* Multiplication factor for frequency as Q2.30 for logarithmic
>> change */
>> +static void tonegen_set_freq_mult(struct tone_state *sg, int32_t fm)
>> {
>> - sg->repeats = r;
>> + sg->freq_coef = (fm > 0) ? fm : ONE_Q2_30; /* Set freq mult to
>> 1.0 */
>> +}
>> - /* Zeros as defaults make a nicer API without need to remember
>> - * the neutral settings for sweep and repeat parameters.
>> - */
>> - sg->freq_coef = (fc > 0) ? fc : ONE_Q2_30; /* Set freq mult to
>> 1.0 */
>> - sg->ampl_coef = (ac > 0) ? ac : ONE_Q2_30; /* Set ampl mult to
>> 1.0 */
>> - sg->tone_length = (l > 0) ? l : INT32_MAXVALUE; /* Count rate
>> 125 us */
>> - sg->tone_period = (p > 0) ? p : INT32_MAXVALUE; /* Count rate
>> 125 us */
>> +/* Multiplication factor for amplitude as Q2.30 for logarithmic
>> change */
>> +static void tonegen_set_ampl_mult(struct tone_state *sg, int32_t am)
>> +{
>> + sg->ampl_coef = (am > 0) ? am : ONE_Q2_30; /* Set ampl mult to
>> 1.0 */
>> +}
>> +
>> +/* Tone length in samples, this is the active length of tone */
>> +static void tonegen_set_length(struct tone_state *sg, uint32_t tl)
>> +{
>> + sg->tone_length = (tl > 0) ? tl : INT32_MAXVALUE; /* Count rate
>> 125 us */
>> +}
>> +
>> +/* Tone period in samples, this is the length including the pause
>> after beep */
>> +static void tonegen_set_period(struct tone_state *sg, uint32_t tp)
>> +{
>> + sg->tone_period = (tp > 0) ? tp : INT32_MAXVALUE; /* Count rate
>> 125 us */
>> }
>> -#endif
>> /* Tone ramp parameters:
>> * step - Value that is added or subtracted to amplitude. A zero or
>> negative
>> @@ -382,6 +400,7 @@ static struct comp_dev *tone_new(struct
>> sof_ipc_comp *comp)
>> struct sof_ipc_comp_tone *tone;
>> struct sof_ipc_comp_tone *ipc_tone = (struct sof_ipc_comp_tone
>> *) comp;
>> struct comp_data *cd;
>> + int i;
>> trace_tone("new");
>> @@ -403,7 +422,8 @@ static struct comp_dev *tone_new(struct
>> sof_ipc_comp *comp)
>> cd->tone_func = tone_s32_default;
>> /* Reset tone generator and set channels volumes to default */
>> - tonegen_reset(&cd->sg);
>> + for (i = 0; i < PLATFORM_MAX_CHANNELS; i++)
>> + tonegen_reset(&cd->sg[i]);
>> dev->state = COMP_STATE_READY;
>> return dev;
>> @@ -441,34 +461,104 @@ static int tone_params(struct comp_dev *dev)
>> return 0;
>> }
>> -static int tone_ctrl_cmd(struct comp_dev *dev, struct
>> sof_ipc_ctrl_data *cdata)
>> +static int tone_cmd_set_value(struct comp_dev *dev, struct
>> sof_ipc_ctrl_data *cdata)
>> {
>> struct comp_data *cd = comp_get_drvdata(dev);
>> + int j;
>> + uint32_t ch;
>> + uint32_t val;
>> +
>> + if (cdata->cmd == SOF_CTRL_CMD_SWITCH) {
>> + trace_tone("mst");
>> + for (j = 0; j < cdata->num_elems; j++) {
>> + ch = cdata->chanv[j].channel;
>> + val = cdata->chanv[j].value;
>> + tracev_value(ch);
>> + tracev_value(val);
>> + if (ch >= PLATFORM_MAX_CHANNELS) {
>> + trace_tone_error("che");
>> + return -EINVAL;
>> + }
>> + if (val > 0)
>> + tonegen_mute(&cd->sg[ch]);
>> + else
>> + tonegen_unmute(&cd->sg[ch]);
>> +
>> + }
>> + } else {
>> + trace_tone_error("ste");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tone_cmd_set_data(struct comp_dev *dev,
>> + struct sof_ipc_ctrl_data *cdata)
>> +{
>> + struct comp_data *cd = comp_get_drvdata(dev);
>> + struct sof_ipc_ctrl_value_comp *compv;
>> + int i;
>> + uint32_t ch;
>> + uint32_t val;
>> trace_tone("tri");
>> + /* Check version from ABI header */
>> + if (cdata->data->comp_abi != SOF_TONE_ABI_VERSION) {
>> + trace_tone_error("abi");
>> + return -EINVAL;
>> + }
>> +
>> switch (cdata->cmd) {
>> - case SOF_CTRL_CMD_MUTE:
>> - trace_tone("TMu");
>> - tonegen_mute(&cd->sg);
>> - break;
>> - case SOF_CTRL_CMD_UNMUTE:
>> - trace_tone("TUm");
>> - tonegen_unmute(&cd->sg);
>> - break;
>> -/* TODO: use comp value list array to set this */
>> -#if 0
>> - case SOF_CTRL_TYPE_VALUE_COMP_SET:
>> - trace_tone("Tto");
>> - ct = (struct sof_ipc_comp_tone *) data;
>> - /* Ignore channels while tone implementation is mono */
>> - tonegen_set_f(&cd->sg, ct->frequency);
>> - tonegen_set_a(&cd->sg, ct->amplitude);
>> - tonegen_set_sweep(&cd->sg, ct->freq_mult, ct->ampl_mult,
>> - ct->length, ct->period, ct->repeats);
>> - tonegen_set_linramp(&cd->sg, ct->ramp_step);
>> + case SOF_CTRL_CMD_ENUM:
>> + trace_tone("ten");
>> + trace_value(cdata->index);
>> + compv = (struct sof_ipc_ctrl_value_comp *) cdata->data->data;
>> + for (i = 0; i < (int) cdata->num_elems; i++) {
>> + ch = compv[i].index;
>> + val = compv[i].svalue;
>> + tracev_value(ch);
>> + tracev_value(val);
>> + switch (cdata->index) {
>> + case SOF_TONE_IDX_FREQUENCY:
>> + trace_tone("tfr");
>> + tonegen_update_f(&cd->sg[ch], val);
>> + break;
>> + case SOF_TONE_IDX_AMPLITUDE:
>> + trace_tone("tam");
>> + tonegen_set_a(&cd->sg[ch], val);
>> + break;
>> + case SOF_TONE_IDX_FREQ_MULT:
>> + trace_tone("tfx");
>> + tonegen_set_freq_mult(&cd->sg[ch], val);
>> + break;
>> + case SOF_TONE_IDX_AMPL_MULT:
>> + trace_tone("tax");
>> + tonegen_set_ampl_mult(&cd->sg[ch], val);
>> + break;
>> + case SOF_TONE_IDX_LENGTH:
>> + trace_tone("tle");
>> + tonegen_set_length(&cd->sg[ch], val);
>> + break;
>> + case SOF_TONE_IDX_PERIOD:
>> + trace_tone("tpe");
>> + tonegen_set_period(&cd->sg[ch], val);
>> + break;
>> + case SOF_TONE_IDX_REPEATS:
>> + trace_tone("trp");
>> + tonegen_set_repeats(&cd->sg[ch], val);
>> + break;
>> + case SOF_TONE_IDX_LIN_RAMP_STEP:
>> + trace_tone("trs");
>> + tonegen_set_linramp(&cd->sg[ch], val);
>> + break;
>> + default:
>> + trace_tone_error("ier");
>> + return -EINVAL;
>> + }
>> + }
>> break;
>> -#endif
>> default:
>> trace_tone_error("ec1");
>> return -EINVAL;
>> @@ -489,17 +579,22 @@ static int tone_cmd(struct comp_dev *dev, int
>> cmd, void *data)
>> if (ret < 0)
>> return ret;
>> - if (cmd == COMP_CMD_SET_VALUE)
>> - ret = tone_ctrl_cmd(dev, cdata);
>> + switch (cmd) {
>> + case COMP_CMD_SET_DATA:
>> + ret = tone_cmd_set_data(dev, cdata);
>> + break;
>> + case COMP_CMD_SET_VALUE:
>> + ret = tone_cmd_set_value(dev, cdata);
>> + break;
>> + }
>> return ret;
>> }
>> /* copy and process stream data from source to sink buffers */
>> -static int tone_copy(struct comp_dev *dev)
>> +static int tone_copy(struct comp_dev * dev)
>> {
>> struct comp_buffer *sink;
>> - struct comp_buffer *source = NULL;
>> struct comp_data *cd = comp_get_drvdata(dev);
>> trace_comp("cpy");
>> @@ -513,19 +608,22 @@ static int tone_copy(struct comp_dev *dev)
>> */
>> if (sink->free >= cd->period_bytes) {
>> /* create tone */
>> - cd->tone_func(dev, sink, source, dev->frames);
>> - comp_update_buffer_produce(sink, 0);
>> + cd->tone_func(dev, sink, dev->frames);
>> +
>> + /* calc new free and available */
>> + comp_update_buffer_produce(sink, cd->period_bytes);
>> }
>> return dev->frames;
>> }
>> -static int tone_prepare(struct comp_dev *dev)
>> +static int tone_prepare(struct comp_dev * dev)
>> {
>> struct comp_data *cd = comp_get_drvdata(dev);
>> int32_t f;
>> int32_t a;
>> int ret;
>> + int i;
>> trace_tone("TPp");
>> @@ -538,30 +636,34 @@ static int tone_prepare(struct comp_dev *dev)
>> tracev_value(cd->channels);
>> tracev_value(cd->rate);
>> - f = tonegen_get_f(&cd->sg);
>> - a = tonegen_get_a(&cd->sg);
>> - if (tonegen_init(&cd->sg, cd->rate, f, a) < 0) {
>> - comp_set_state(dev, COMP_CMD_RESET);
>> - return -EINVAL;
>> + for (i = 0; i < cd->channels; i++) {
>> + f = tonegen_get_f(&cd->sg[i]);
>> + a = tonegen_get_a(&cd->sg[i]);
>> + if (tonegen_init(&cd->sg[i], cd->rate, f, a) < 0) {
>> + comp_set_state(dev, COMP_CMD_RESET);
>> + return -EINVAL;
>> + }
>> }
>> return 0;
>> }
>> -static int tone_preload(struct comp_dev *dev)
>> +static int tone_preload(struct comp_dev * dev)
>> {
>> return tone_copy(dev);
>> }
>> -static int tone_reset(struct comp_dev *dev)
>> +static int tone_reset(struct comp_dev * dev)
>> {
>> struct comp_data *cd = comp_get_drvdata(dev);
>> + int i;
>> trace_tone("TRe");
>> /* Initialize with the defaults */
>> - tonegen_reset(&cd->sg);
>> + for (i = 0; i < PLATFORM_MAX_CHANNELS; i++)
>> + tonegen_reset(&cd->sg[i]);
>> comp_set_state(dev, COMP_CMD_RESET);
>> @@ -570,8 +672,7 @@ static int tone_reset(struct comp_dev *dev)
>> struct comp_driver comp_tone = {
>> .type = SOF_COMP_TONE,
>> - .ops =
>> - {
>> + .ops = {
>> .new = tone_new,
>> .free = tone_free,
>> .params = tone_params,
>> diff --git a/src/audio/tone.h b/src/include/uapi/tone.h
>> similarity index 58%
>> rename from src/audio/tone.h
>> rename to src/include/uapi/tone.h
>> index fcaf404..b04e7dc 100644
>> --- a/src/audio/tone.h
>> +++ b/src/include/uapi/tone.h
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (c) 2016, Intel Corporation
>> + * Copyright (c) 2017, Intel Corporation
>> * All rights reserved.
>> *
>> * Redistribution and use in source and binary forms, with or without
>> @@ -26,33 +26,23 @@
>> * POSSIBILITY OF SUCH DAMAGE.
>> *
>> * Author: Seppo Ingalsuo <seppo.ingalsuo at linux.intel.com>
>> - * Liam Girdwood <liam.r.girdwood at linux.intel.com>
>> - * Keyon Jie <yang.jie at linux.intel.com>
>> */
>> -/* Convert float frequency in Hz to Q16.16 fractional format */
>> -#define TONE_FREQ(f) Q_CONVERT_FLOAT(f, 16)
>> +#ifndef TONE_H
>> +#define TONE_H
>> -/* Convert float gain to Q1.31 fractional format */
>> -#define TONE_GAIN(v) Q_CONVERT_FLOAT(v, 31)
>> +/* Component will reject non-matching configuration. The version
>> number need
>> + * to be incremented with any ABI changes in function fir_cmd().
>> + */
>> +#define SOF_TONE_ABI_VERSION 1
>> +
>> +#define SOF_TONE_IDX_FREQUENCY 0
>> +#define SOF_TONE_IDX_AMPLITUDE 1
>> +#define SOF_TONE_IDX_FREQ_MULT 2
>> +#define SOF_TONE_IDX_AMPL_MULT 3
>> +#define SOF_TONE_IDX_LENGTH 4
>> +#define SOF_TONE_IDX_PERIOD 5
>> +#define SOF_TONE_IDX_REPEATS 6
>> +#define SOF_TONE_IDX_LIN_RAMP_STEP 7
>> -struct tone_state {
>> - int mute;
>> - int32_t a; /* Current amplitude Q1.31 */
>> - int32_t a_target; /* Target amplitude Q1.31 */
>> - int32_t ampl_coef; /* Amplitude multiplier Q2.30 */
>> - int32_t c; /* Coefficient 2*pi/Fs Q1.31 */
>> - int32_t f; /* Frequency Q16.16 */
>> - int32_t freq_coef; /* Frequency multiplier Q2.30 */
>> - int32_t fs; /* Sample rate in Hertz Q32.0 */
>> - int32_t ramp_step; /* Amplitude ramp step Q1.31 */
>> - int32_t w; /* Angle radians Q4.28 */
>> - int32_t w_step; /* Angle step Q4.28 */
>> - uint32_t block_count;
>> - uint32_t repeat_count;
>> - uint32_t repeats; /* Number of repeats for tone (sweep steps) */
>> - uint32_t sample_count;
>> - uint32_t samples_in_block; /* Samples in 125 us block */
>> - uint32_t tone_length; /* Active length in 125 us blocks */
>> - uint32_t tone_period; /* Active + idle time in 125 us blocks */
>> -};
>> +#endif /* TONE_ABI_H */
>>
>
>
More information about the Sound-open-firmware
mailing list