Re: [Sound-open-firmware] [PATCH 3/5] Tone: Fix control ipc and add support for independent tones in channels

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?

};

+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.

The rest of the patch is hard to review with tons of changes...

+}

• 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@linux.intel.com

• •     Liam Girdwood <liam.r.girdwood@linux.intel.com>
    

• •     Keyon Jie <yang.jie@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 */