- Sound-open-firmware - mailman.alsa-project.org

Re: [Sound-open-firmware] [PATCH v3 00/27] add R/W pointers check for pipeline buffers
by Keyon Jie 13 Feb '17

13 Feb '17

On 2017年02月13日 12:31, Ughreja, Rakesh A wrote: > > >> -----Original Message----- >> From: Keyon Jie [mailto:yang.jie@linux.intel.com] >> Sent: Monday, February 13, 2017 7:31 AM >> To: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com>; sound-open- >> firmware(a)alsa-project.org; liam.r.girdwood(a)linux.intel.com >> Cc: Ughreja, Rakesh A <rakesh.a.ughreja(a)intel.com>; Jie, Yang >> <yang.jie(a)intel.com>; Ingalsuo, Seppo <seppo.ingalsuo(a)intel.com> >> Subject: Re: [Sound-open-firmware] [PATCH v3 00/27] add R/W pointers >> check for pipeline buffers >> >> On 2017年02月11日 10:37, Pierre-Louis Bossart wrote: >>> On 2/10/17 8:17 PM, Keyon Jie wrote: >>>> This series adding the R/W buffer pointers check and make sure we won't >>>> copy wrong/missed/repeated data for the whole pipeline. >>>> >>>> For ALSA applications, usually we have two kinds of trigger command >>>> sequences: >>>> 1. (e.g. aplay) prefill datas ==> trigger start ==> refill buffer(when >>>> buffer >>>> free is enough) ==> ... ==> data filling finished ==> trigger >>>> drain ==> >>>> when all datas are rendered(via read pointer feedback) ==> >>>> trigger stop; >>>> 2. (e.g. speaker-test with large buffer size > wav file size): >>>> prefill all datas one time ==> trigger start ==> wait until >>>> all datas are >>>> rendered(via feedback read pointer) ==> trigger stop ==> >>>> prefill another >>>> wav file ==> trigger start ==> ... >>>> >>>> So the feedback from firmware about the read pointer is very important >>>> for >>>> the pipeline's running, to make the firmware can handle these two >>>> kinds of >>>> sequences, this series design/assume with preconditions: >>>> >>>> 1. We do down stream full preload(full fill all pipeline buffers, >>>> except host >>>> side one); >>>> 2. Using pull mode at running stage and pulling one period per time; >>>> 3. When in normal running status, if buffer a->avail is 0, then all those >>>> upstream buffers are empty; >>>> >>>> For those two sequence, we use a same logic to handle it: >>>> >>>> 1. We put all components in running status even some of them are >>>> empty(dai is >>>> an exception); >>>> 2. We stop dai/ssp once we found there is no more data for it to >>>> render, and >>>> notify this last read position to host side; >>>> 3. Host side will trigger stop and we will reset all buffer pointers and >>>> related resources; >>>> 4. For case #1, trigger reset will reset/free more >>>> resources(allocated/init >>>> in set_params()); For case #2, another start cmd will trigger preload >>>> and start >>>> in firmware, before that all pointers are reset to original ones. >>>> >>>> TODOs: It is only verified for playback, for capture, we may need some >>>> vivid >>>> code to implement them. >>> >>> I may be missing something here - the cover letter isn't very clear - , >>> but I see mentions of 'pull one period at a time' and no mention of the >>> start/stop threshold. Is this really going to work in no-irq mode where >>> periods are irrelevant and with start/stop thresholds that are smaller >>> than a period. >> >> Um, we don't actually use start/stop thresholds inside sof, or you can >> think of we are using one period as thresholds and it is >> non-configurable: after preloaded, we trigger dai dma start, in dma tfr >> callback(the tfr copies one period each time), we will schedule upstream >> copy(what I called it 'pull mode' upper) that will copy one period >> upstream. >> > > Hi Keyon, > > There is a register called SDnFIFO. Its defined in the HDA Spec and used > by ALSA framework. FW should advertise the buffering via this parameter. > > By default this value is 1ms in the HDA Spec. (legacy mode) Thanks for sharing that, Rakesh. BTW, does this SDnFIFO register only influence HDA audio, or it is needed for ASoC also? > > The buffer on the firmware side should be configurable by the driver. > By default driver may assign it to 1ms and when no-rewind flag is set > it can set it to larger value for offload playback. Our target is definitely to make the buffer on firmware side configurable, by the driver, we may introduce topology support in next milestone release, where the pipeline/buffer can be changed dynamically. > > It is not wrong to fetch 1 period as long as you advertise that via SDxFIFO. > In order to comply with legacy applications, which does not assume too > much buffering it is better to start with minimum buffering possible that > FW can handle. Um, we haven't advertised this yet, for ASoC, we preset ALSA buffer size/period size/num range in snd_pcm_hw, application will set it in initial stage. For firmware inside buffer size, we are using quite small ones, but will make it configurable in short future. Thanks, ~Keyon > > >>> If you really want to make the transfers reliable then you should also >>> assume that there are no rewinds and compare read/write pointers to >>> avoid fetching stale data. that's a useful mode to have >> >> We have added handling for rewinds and comparing R/W pointers in this >> series, and won't fetch stale data with them applied. >> >> Thanks, >> ~Keyon >> >>> >>>> >>>> Changes in v3: >>>> 1. add one preload patch for pipeline full preload, which make >>>> playback works. >>>> 2. fix DMA channel status wrong after trigger stop issue. >>>> >>>> Changes in v2: >>>> 1. extract some common code to separate functions to save code; >>>> 2. add comments for component status; >>>> 3. other slight changes per Liam's suggestion. >>>> >>>> Keyon Jie (26): >>>> host: handle more possibility for playback spitting >>>> dai: reset dai_pos at MMAP_PPOS command >>>> pipleline: op_sink: don't need clear the buffer for each prepare >>>> host: start: don't real start for start command >>>> ipc: presentation_posn should be uint64_t type >>>> intel-ipc: add application buffer pointer update from host side >>>> intel-ipc: add stream operation IPC_INTEL_STR_STOP >>>> host: add host_update_buffer for produce and consume >>>> host: rename host_pos_blks to host_pos_read >>>> host: for the last bytes/period, send read notification later >>>> host: add R/W pointers check for playback >>>> host: add a completion work to notify host side >>>> host: reset pointers at new() and reset() >>>> host: cleanup host_cmd() cases >>>> host: host_copy: only copy when there is data and wait until finish >>>> volume: volume_params(): set the sink buffer params depending on its >>>> sink >>>> component: status: rename COMP_STATE_STOPPED to >> COMP_STATE_SETUP >>>> volume: volume_cmd(): only stop/pause at running >>>> volume: add R/W pointer check for volume_copy() >>>> mixer: mixer_params(): set the sink buffer params to >>>> STREAM_FORMAT_S32_LE. >>>> mixer: add R/W pointer check for mixer_copy() >>>> mixer: add dd->last_bytes for the last bytes(< period) caculation >>>> dai: add R/W pointer check for dai copy >>>> dw-dma: handle different cases in irq handler >>>> component: host/volume/mixer: reset buffers for stop cmd >>>> host: create host_elements_reset() and host_pointer_reset() for common >>>> use >>>> >>>> Liam Girdwood (1): >>>> pipeline: Add support for full pipeline pre-loading. >>>> >>>> src/audio/dai.c | 71 ++++- >>>> src/audio/host.c | 313 >>>> ++++++++++++++++------ >>>> src/audio/mixer.c | 85 +++++- >>>> src/audio/pipeline.c | 107 +++++++- >>>> src/audio/volume.c | 59 +++- >>>> src/drivers/dw-dma.c | 30 ++- >>>> src/include/reef/audio/component.h | 59 +++- >>>> src/include/reef/audio/pipeline.h | 2 + >>>> src/include/reef/dma.h | 7 + >>>> src/include/uapi/intel-ipc.h | 3 +- >>>> src/ipc/intel-ipc.c | 69 +++++ >>>> src/platform/baytrail/include/platform/platform.h | 6 + >>>> 12 files changed, 661 insertions(+), 150 deletions(-) >>>> >>> >>> _______________________________________________ >>> Sound-open-firmware mailing list >>> Sound-open-firmware(a)alsa-project.org >>> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware >>>

1 0

[Sound-open-firmware] [PATCH v2 00/26] add R/W pointers check for pipeline buffers
by Keyon Jie 11 Feb '17

11 Feb '17

This series adding the R/W buffer pointers check and make sure we won't copy wrong/missed/repeated data for the whole pipeline. For ALSA applications, usually we have two kinds of trigger command sequences: 1. (e.g. aplay) prefill datas ==> trigger start ==> refill buffer(when buffer free is enough) ==> ... ==> data filling finished ==> trigger drain ==> when all datas are rendered(via read pointer feedback) ==> trigger stop; 2. (e.g. speaker-test with large buffer size > wav file size): prefill all datas one time ==> trigger start ==> wait until all datas are rendered(via feedback read pointer) ==> trigger stop ==> prefill another wav file ==> trigger start ==> ... So the feedback from firmware about the read pointer is very important for the pipeline's running, to make the firmware can handle these two kinds of sequences, this series design/assume with preconditions: 1. We do down stream full preload(full fill all pipeline buffers, except host side one); 2. Using pull mode at running stage and pulling one period per time; 3. When in normal running status, if buffer a->avail is 0, then all those upstream buffers are empty; For those two sequence, we use a same logic to handle it: 1. We put all components in running status even some of them are empty(dai is an exception); 2. We stop dai/ssp once we found there is no more data for it to render, and notify this last read position to host side; 3. Host side will trigger stop and we will reset all buffer pointers and related resources; 4. For case #1, trigger reset will reset/free more resources(allocated/init in set_params()); For case #2, another start cmd will trigger preload and start in firmware, before that all pointers are reset to original ones. TODOs: It is only verified for playback, for capture, we may need some vivid code to implement them. Changes in v2: 1. extract some common code to separate functions to save code; 2. add comments for component status; 3. other slight changes per Liam's suggestion. Keyon Jie (26): host: handle more possibility for playback spitting dai: reset dai_pos at MMAP_PPOS command pipleline: op_sink: don't need clear the buffer for each prepare host: start: don't real start for start command ipc: presentation_posn should be uint64_t type intel-ipc: add application buffer pointer update from host side intel-ipc: add stream operation IPC_INTEL_STR_STOP host: add host_update_buffer for produce and consume host: rename host_pos_blks to host_pos_read host: for the last bytes/period, send read notification later host: add R/W pointers check for playback host: add a completion work to notify host side host: reset pointers at new() and reset() host: cleanup host_cmd() cases host: host_copy: only copy when there is data and wait until finish volume: volume_params(): set the sink buffer params depending on its sink component: status: rename COMP_STATE_STOPPED to COMP_STATE_SETUP volume: volume_cmd(): only stop/pause at running volume: add R/W pointer check for volume_copy() mixer: mixer_params(): set the sink buffer params to STREAM_FORMAT_S32_LE. mixer: add R/W pointer check for mixer_copy() mixer: add dd->last_bytes for the last bytes(< period) caculation dai: add R/W pointer check for dai copy dw-dma: handle different cases in irq handler component: host/volume/mixer: reset buffers for stop cmd host: create host_elements_reset() and host_pointer_reset() for common use src/audio/dai.c | 63 ++++- src/audio/host.c | 306 ++++++++++++++++------ src/audio/mixer.c | 39 ++- src/audio/pipeline.c | 11 +- src/audio/volume.c | 43 ++- src/drivers/dw-dma.c | 28 +- src/include/reef/audio/component.h | 42 ++- src/include/reef/audio/pipeline.h | 2 + src/include/reef/dma.h | 7 + src/include/uapi/intel-ipc.h | 3 +- src/ipc/intel-ipc.c | 69 +++++ src/platform/baytrail/include/platform/platform.h | 6 + 12 files changed, 488 insertions(+), 131 deletions(-) -- 2.7.4

3 28

[Sound-open-firmware] [PATCH] pipeline: Add support for full pipeline pre-loading.
by Keyon Jie 10 Feb '17

10 Feb '17

From: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> Preload entire pipeline and all components by walking the pipeline graph. Signed-off-by: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> Modified-by: Keyon Jie <yang.jie(a)linux.intel.com> --- src/audio/dai.c | 8 ++++ src/audio/host.c | 9 ++-- src/audio/mixer.c | 46 ++++++++++++++++-- src/audio/pipeline.c | 96 ++++++++++++++++++++++++++++++++++++-- src/audio/volume.c | 18 ++++--- src/include/reef/audio/component.h | 17 ++++++- 6 files changed, 173 insertions(+), 21 deletions(-) diff --git a/src/audio/dai.c b/src/audio/dai.c index 230acbf..75f9acf 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -488,6 +488,12 @@ static int dai_copy(struct comp_dev *dev) return 0; } +/* source component will preload dai */ +static int dai_preload(struct comp_dev *dev) +{ + return 0; +} + static int dai_config(struct comp_dev *dev, struct dai_config *dai_config) { struct dai_data *dd = comp_get_drvdata(dev); @@ -513,6 +519,7 @@ static struct comp_driver comp_dai_ssp = { .prepare = dai_prepare, .reset = dai_reset, .dai_config = dai_config, + .preload = dai_preload, .dai_set_loopback = dai_set_loopback, }, }; @@ -526,6 +533,7 @@ static struct comp_driver comp_dai_hda = { .cmd = dai_cmd, .copy = dai_copy, .prepare = dai_prepare, + .preload = dai_preload, }, }; diff --git a/src/audio/host.c b/src/audio/host.c index d71c010..d1e4eaf 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -586,8 +586,9 @@ static int host_preload(struct comp_dev *dev) int ret, i; trace_host("PrL"); + /* preload all periods */ - for (i = 0; i < PLAT_HOST_PERIODS; i++) { + for (i = 0; i < dev->preload; i++) { /* do DMA transfer */ wait_init(&hd->complete); dma_set_config(hd->dma, hd->chan, &hd->config); @@ -632,11 +633,8 @@ static int host_prepare(struct comp_dev *dev) hd->params.period_frames * hd->params.frame_size; hd->split_remaining = 0; + dev->preload = PLAT_HOST_PERIODS; - if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) - host_preload(dev); - - host_update_buffer_consume(hd); dev->state = COMP_STATE_PREPARE; return 0; } @@ -830,6 +828,7 @@ struct comp_driver comp_host = { .cmd = host_cmd, .copy = host_copy, .prepare = host_prepare, + .preload = host_preload, .host_buffer = host_buffer, }, }; diff --git a/src/audio/mixer.c b/src/audio/mixer.c index c653fd9..740ccaf 100644 --- a/src/audio/mixer.c +++ b/src/audio/mixer.c @@ -286,18 +286,53 @@ static int mixer_reset(struct comp_dev *dev) return 0; } +/* + * Prepare the mixer. The mixer may already be running at this point with other + * sources. Make sure we only prepare the "prepared" source streams and not + * the active or inactive sources. + * + * We should also make sure that we propagate the prepare call to downstream + * if downstream is not currently active. + */ static int mixer_prepare(struct comp_dev *dev) { struct mixer_data *md = comp_get_drvdata(dev); - int i; + struct list_item * blist; + struct comp_buffer *source; + int downstream = 0; trace_mixer("MPp"); - md->mix_func = mix_n; - dev->state = COMP_STATE_PREPARE; + if (dev->state != COMP_STATE_RUNNING) { + md->mix_func = mix_n; + dev->state = COMP_STATE_PREPARE; + dev->preload = PLAT_INT_PERIODS; + } + + /* check each mixer source state */ + list_for_item(blist, &dev->bsource_list) { + source = container_of(blist, struct comp_buffer, sink_list); + + /* only prepare downstream if we have no active sources */ + if (source->source->state == COMP_STATE_PAUSED || + source->source->state == COMP_STATE_RUNNING) { + downstream = 1; + } + } + + /* prepare downstream */ + return downstream; +} + +static int mixer_preload(struct comp_dev *dev) +{ + int i; + + if (dev->state != COMP_STATE_PREPARE) + return 1; - /* mix periods */ - for (i = 0; i < PLAT_HOST_PERIODS; i++) + /* preload and mix periods if inactive */ + for (i = 0; i < dev->preload; i++) mixer_copy(dev); return 0; @@ -310,6 +345,7 @@ struct comp_driver comp_mixer = { .free = mixer_free, .params = mixer_params, .prepare = mixer_prepare, + .preload = mixer_preload, .cmd = mixer_cmd, .copy = mixer_copy, .reset = mixer_reset, diff --git a/src/audio/pipeline.c b/src/audio/pipeline.c index 41693ae..5c03659 100644 --- a/src/audio/pipeline.c +++ b/src/audio/pipeline.c @@ -306,6 +306,78 @@ int pipeline_comp_connect(struct pipeline *p, struct comp_dev *source, return 0; } +/* preload component buffers prior to playback start */ +static int component_preload(struct comp_dev *comp, uint32_t depth, + uint32_t max_depth) +{ + struct list_item *clist; + int err; + + /* bail if we are at max_depth */ + if (depth > max_depth) + return 0; + + trace_value(comp->id); + err = comp_preload(comp); + + /* dont walk the graph any further if this component fails */ + if (err < 0) { + trace_pipe_error("eOp"); + return err; + } else if (err > 0 || comp->is_dai) { + /* we finish walking the graph if we reach the DAI or component is + * currently active and configured already (err > 0). + */ + trace_pipe("prl"); + trace_value((uint32_t)err); + trace_value(comp->is_dai); + trace_value(comp->id); + trace_value(comp->is_host); + return 0; + } + + /* now run this operation downstream */ + list_for_item(clist, &comp->bsink_list) { + struct comp_buffer *buffer; + + buffer = container_of(clist, struct comp_buffer, source_list); + + /* dont go downstream if this component is not connected */ + if (!buffer->connected) + continue; + + err = component_preload(buffer->sink, depth + 1, max_depth); + if (err < 0) + break; + } + + return err; +} + +/* work out the pipeline depth from this component to the farthest endpoint */ +static int pipeline_depth(struct comp_dev *comp, int current) +{ + struct list_item *clist; + int depth = 0; + + /* go down stream to deepest endpoint */ + list_for_item(clist, &comp->bsink_list) { + struct comp_buffer *buffer; + + buffer = container_of(clist, struct comp_buffer, source_list); + + /* dont go downstream if this component is not connected */ + if (!buffer->connected) + continue; + + depth = pipeline_depth(buffer->sink, current + 1); + if (depth > current) + current = depth; + } + + return current; +} + /* call op on all downstream components - locks held by caller */ static int component_op_sink(struct op_data *op_data, struct comp_dev *comp) { @@ -459,7 +531,7 @@ static int component_op_source(struct op_data *op_data, struct comp_dev *comp) int pipeline_prepare(struct pipeline *p, struct comp_dev *host) { struct op_data op_data; - int ret; + int ret, depth, i; trace_pipe("Ppr"); @@ -467,12 +539,28 @@ int pipeline_prepare(struct pipeline *p, struct comp_dev *host) op_data.op = COMP_OPS_PREPARE; spin_lock(&p->lock); - if (host->direction == STREAM_DIRECTION_PLAYBACK) + if (host->direction == STREAM_DIRECTION_PLAYBACK) { + + /* first of all prepare the pipeline */ ret = component_op_sink(&op_data, host); - else + if (ret < 0) + goto out; + + /* then preload buffers - the buffers must be moved + * downstream so that every component has full buffers for + * trigger start */ + depth = pipeline_depth(host, 0); + for (i = depth; i > 0; i--) { + ret = component_preload(host, 0, i); + if (ret < 0) + break; + } + } else { ret = component_op_source(&op_data, host); - spin_unlock(&p->lock); + } +out: + spin_unlock(&p->lock); return ret; } diff --git a/src/audio/volume.c b/src/audio/volume.c index f347847..1eb6004 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -548,16 +548,21 @@ found: *cd->hvol[i] = cd->volume[i]; } - /* copy periods from host */ - if (source->params.direction == STREAM_DIRECTION_PLAYBACK) { - for (i = 0; i < PLAT_INT_PERIODS; i++) - volume_copy(dev); - } - + dev->preload = PLAT_INT_PERIODS; dev->state = COMP_STATE_PREPARE; return 0; } +static int volume_preload(struct comp_dev *dev) +{ + int i; + + for (i = 0; i < dev->preload; i++) + volume_copy(dev); + + return 0; +} + static int volume_reset(struct comp_dev *dev) { dev->state = COMP_STATE_INIT; @@ -575,6 +580,7 @@ struct comp_driver comp_volume = { .copy = volume_copy, .prepare = volume_prepare, .reset = volume_reset, + .preload = volume_preload, }, }; diff --git a/src/include/reef/audio/component.h b/src/include/reef/audio/component.h index 3abae3d..1e784f5 100644 --- a/src/include/reef/audio/component.h +++ b/src/include/reef/audio/component.h @@ -140,7 +140,12 @@ struct buffer_desc { struct period_desc source_period; }; -/* audio component operations - all mandatory */ +/* + * Audio component operations - all mandatory. + * + * All component operations must return 0 for success, negative values for + * errors and 1 to stop the pipeline walk operation. + */ struct comp_ops { /* component creation and destruction */ struct comp_dev *(*new)(uint32_t type, uint32_t index, @@ -150,6 +155,9 @@ struct comp_ops { /* set component audio stream paramters */ int (*params)(struct comp_dev *dev, struct stream_params *params); + /* preload buffers */ + int (*preload)(struct comp_dev *dev); + /* set component audio stream paramters */ int (*dai_config)(struct comp_dev *dev, struct dai_config *dai_config); @@ -194,6 +202,7 @@ struct comp_dev { uint32_t is_host; /* component is graph host endpoint */ uint32_t is_mixer; /* component is mixer */ uint32_t direction; /* STREAM_DIRECTION_ */ + uint16_t preload; /* number of periods to preload during prepare() */ spinlock_t lock; /* lock for this component */ void *private; /* private data */ struct comp_driver *drv; @@ -281,6 +290,12 @@ static inline int comp_prepare(struct comp_dev *dev) return dev->drv->ops.prepare(dev); } +/* component preload buffers -mandatory */ +static inline int comp_preload(struct comp_dev *dev) +{ + return dev->drv->ops.preload(dev); +} + /* copy component buffers - mandatory */ static inline int comp_copy(struct comp_dev *dev) { -- 2.7.4

1 0

[Sound-open-firmware] [PATCH 00/25] add R/W pointers check for pipeline buffers
by Keyon Jie 09 Feb '17

09 Feb '17

This series adding the R/W buffer pointers check and make sure we won't copy wrong/missed/repeated data for the whole pipeline. For ALSA applications, usually we have two kinds of trigger command sequences: 1. (e.g. aplay) prefill datas ==> trigger start ==> refill buffer(when buffer free is enough) ==> ... ==> data filling finished ==> trigger drain ==> when all datas are rendered(via read pointer feedback) ==> trigger stop; 2. (e.g. speaker-test with large buffer size > wav file size): prefill all datas one time ==> trigger start ==> wait until all datas are rendered(via feedback read pointer) ==> trigger stop ==> prefill another wav file ==> trigger start ==> ... So the feedback from firmware about the read pointer is very important for the pipeline's running, to make the firmware can handle these two kinds of sequences, this series design/assume with preconditions: 1. We do down stream full preload(full fill all pipeline buffers, except host side one); 2. Using pull mode at running stage and pulling one period per time; 3. When in normal running status, if buffer a->avail is 0, then all those upstream buffers are empty; For those two sequence, we use a same logic to handle it: 1. We put all components in running status even some of them are empty(dai is an exception); 2. We stop dai/ssp once we found there is no more data for it to render, and notify this last read position to host side; 3. Host side will trigger stop and we will reset all buffer pointers and related resources; 4. For case #1, trigger reset will reset/free more resources(allocated/init in set_params()); For case #2, another start cmd will trigger preload and start in firmware, before that all pointers are reset to original ones. TODOs: It is only verified for playback, for capture, we may need some vivid code to implement them. Keyon Jie (25): host: handle more possibility for playback spitting dai: reset dai_pos at MMAP_PPOS command pipleline: op_sink: don't need clear the buffer for each prepare host: start: don't real start for start command ipc: presentation_posn should be uint64_t type intel-ipc: add application buffer pointer update from host side intel-ipc: add stream operation IPC_INTEL_STR_STOP host: add host_update_buffer for produce and consume host: rename host_pos_blks to host_pos_read host: for the last bytes/period, send read notification later host: add R/W pointers check for playback host: add a completion work to notify host side host: reset pointers at new() and reset() host: cleanup host_cmd() cases host: host_copy: only copy when there is data and wait until finish volume: volume_params(): set the sink buffer params depending on its sink component: status: rename COMP_STATE_STOPPED to COMP_STATE_SETUP volume: volume_cmd(): only stop/pause at running volume: add R/W pointer check for volume_copy() mixer: mixer_params(): set the sink buffer params to STREAM_FORMAT_S32_LE. mixer: add R/W pointer check for mixer_copy() mixer: add dd->last_bytes for the last bytes(< period) caculation dai: add R/W pointer check for dai copy dw-dma: handle different cases in irq handler component: host/volume/mixer: reset buffers for stop cmd src/audio/dai.c | 60 ++++- src/audio/host.c | 289 +++++++++++++++++----- src/audio/mixer.c | 64 ++++- src/audio/pipeline.c | 11 +- src/audio/volume.c | 66 ++++- src/drivers/dw-dma.c | 25 +- src/include/reef/audio/component.h | 6 +- src/include/reef/audio/pipeline.h | 2 + src/include/reef/dma.h | 6 + src/include/uapi/intel-ipc.h | 4 +- src/ipc/intel-ipc.c | 77 ++++++ src/platform/baytrail/include/platform/platform.h | 6 + 12 files changed, 511 insertions(+), 105 deletions(-) -- 2.7.4

3 46

[Sound-open-firmware] [PATCH] volume: fix the L/R channel mixed issue
by Keyon Jie 11 Jan '17

11 Jan '17

The Right channel was assigned with Left channel by mistake in vol_s16_to_s32 and vol_s32_to_s32, here correct it. Signed-off-by: Keyon Jie <yang.jie(a)linux.intel.com> --- src/audio/volume.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/audio/volume.c b/src/audio/volume.c index 65c41b9..24a9d11 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -91,7 +91,7 @@ static void vol_s16_to_s32(struct comp_dev *dev, struct comp_buffer *sink, /* buffer sizes are always divisible by period frames */ for (i = 0; i < frames * 2; i += 2) { dest[i] = (int32_t)src[i] * cd->volume[0]; - dest[i + 1] = (int32_t)src[i] * cd->volume[1]; + dest[i + 1] = (int32_t)src[i + 1] * cd->volume[1]; } source->r_ptr = src + i; @@ -127,7 +127,7 @@ static void vol_s32_to_s32(struct comp_dev *dev, struct comp_buffer *sink, /* buffer sizes are always divisible by period frames */ for (i = 0; i < frames * 2; i += 2) { dest[i] = ((int64_t)src[i] * cd->volume[0]) >> 16; - dest[i + 1] = ((int64_t)src[i] * cd->volume[1]) >> 16; + dest[i + 1] = ((int64_t)src[i + 1] * cd->volume[1]) >> 16; } source->r_ptr = src + i; -- 2.5.0

2 1

[Sound-open-firmware] [PATCH] component: fix comp_update_buffer() when buffer full
by Keyon Jie 10 Jan '17

10 Jan '17

It will treat full buffer as empty one at current comp_update_buffer(), as w_ptr is equal to r_ptr, this will make us missing those datas or at least the buffer->avail/free are not correct. Here spit comp_update_buffer() into consuming one and producing one, for consuming, 'r_ptr == w_ptr' update means buffer is becoming empty, and for producing, 'w_ptr == r_ptr' means buffer is becoming full. Signed-off-by: Keyon Jie <yang.jie(a)linux.intel.com> --- src/audio/dai.c | 8 +++++--- src/audio/host.c | 4 ++-- src/audio/mixer.c | 4 ++-- src/audio/volume.c | 4 ++-- src/include/reef/audio/component.h | 18 ++++++++++++++++-- 5 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/audio/dai.c b/src/audio/dai.c index 64e4761..ab0164c 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -98,6 +98,8 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) *dd->dai_pos = dd->dai_pos_blks + dma_buffer->r_ptr - dma_buffer->addr; + /* recalc available buffer space */ + comp_update_buffer_consume(dma_buffer); } else { dma_buffer = list_first_item(&dev->bsink_list, struct comp_buffer, source_list); @@ -119,10 +121,10 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) if (dd->dai_pos) *dd->dai_pos = dd->dai_pos_blks + dma_buffer->w_ptr - dma_buffer->addr; - } - /* recalc available buffer space */ - comp_update_buffer(dma_buffer); + /* recalc available buffer space */ + comp_update_buffer_produce(dma_buffer); + } /* notify pipeline that DAI needs it's buffer filled */ // if (dev->state == COMP_STATE_RUNNING) diff --git a/src/audio/host.c b/src/audio/host.c index fe43cf8..217ad4f 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -130,7 +130,7 @@ static void host_dma_cb_playback(struct comp_dev *dev, #endif /* recalc available buffer space */ - comp_update_buffer(hd->dma_buffer); + comp_update_buffer_consume(hd->dma_buffer); /* new local period, update host buffer position blks */ hd->host_pos_blks += local_elem->size; @@ -236,7 +236,7 @@ static void host_dma_cb_capture(struct comp_dev *dev, *hd->host_pos = hd->host_pos_blks; /* recalc available buffer space */ - comp_update_buffer(hd->dma_buffer); + comp_update_buffer_produce(hd->dma_buffer); /* send IPC message to driver if needed */ hd->host_period_pos += local_elem->size; diff --git a/src/audio/mixer.c b/src/audio/mixer.c index b996dae..3a8d12f 100644 --- a/src/audio/mixer.c +++ b/src/audio/mixer.c @@ -230,13 +230,13 @@ static int mixer_copy(struct comp_dev *dev) for(; i > 0; i--) { if (sources[i-1]->r_ptr >= sources[i-1]->end_addr) sources[i-1]->r_ptr = sources[i-1]->addr; - comp_update_buffer(sources[i-1]); + comp_update_buffer_consume(sources[i-1]); } if (sink->w_ptr >= sink->end_addr) sink->w_ptr = sink->addr; /* calc new free and available */ - comp_update_buffer(sink); + comp_update_buffer_produce(sink); /* number of frames sent downstream */ return cframes; diff --git a/src/audio/volume.c b/src/audio/volume.c index e2badb8..75a48f9 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -460,8 +460,8 @@ static int volume_copy(struct comp_dev *dev) sink->w_ptr = sink->addr; /* calc new free and available */ - comp_update_buffer(sink); - comp_update_buffer(source); + comp_update_buffer_produce(sink); + comp_update_buffer_consume(source); /* number of frames sent downstream */ return cframes; diff --git a/src/include/reef/audio/component.h b/src/include/reef/audio/component.h index fbd29e4..e521753 100644 --- a/src/include/reef/audio/component.h +++ b/src/include/reef/audio/component.h @@ -311,10 +311,24 @@ void sys_comp_mux_init(void); void sys_comp_switch_init(void); void sys_comp_volume_init(void); -static inline void comp_update_buffer(struct comp_buffer *buffer) +static inline void comp_update_buffer_produce(struct comp_buffer *buffer) { - if (buffer->r_ptr <= buffer->w_ptr) + if (buffer->r_ptr < buffer->w_ptr) buffer->avail = buffer->w_ptr - buffer->r_ptr; + else if (buffer->r_ptr == buffer->w_ptr) + buffer->avail = buffer->end_addr - buffer->addr; /* full */ + else + buffer->avail = buffer->end_addr - buffer->r_ptr + + buffer->w_ptr - buffer->addr; + buffer->free = buffer->desc.size - buffer->avail; +} + +static inline void comp_update_buffer_consume(struct comp_buffer *buffer) +{ + if (buffer->r_ptr < buffer->w_ptr) + buffer->avail = buffer->w_ptr - buffer->r_ptr; + else if (buffer->r_ptr == buffer->w_ptr) + buffer->avail = 0; /* empty */ else buffer->avail = buffer->end_addr - buffer->r_ptr + buffer->w_ptr - buffer->addr; -- 2.7.4

2 1

[Sound-open-firmware] [PATCH] volume: use rzalloc for component new
by Keyon Jie 10 Jan '17

10 Jan '17

Replace rmalloc() with rzalloc() to make sure the new allocated structs are clear to all 0s. Signed-off-by: Keyon Jie <yang.jie(a)linux.intel.com> --- src/audio/volume.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/audio/volume.c b/src/audio/volume.c index e2badb8..4b5b2e3 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -306,11 +306,11 @@ static struct comp_dev *volume_new(uint32_t type, uint32_t index, struct comp_data *cd; int i; - dev = rmalloc(RZONE_MODULE, RMOD_SYS, sizeof(*dev)); + dev = rzalloc(RZONE_MODULE, RMOD_SYS, sizeof(*dev)); if (dev == NULL) return NULL; - cd = rmalloc(RZONE_MODULE, RMOD_SYS, sizeof(*cd)); + cd = rzalloc(RZONE_MODULE, RMOD_SYS, sizeof(*cd)); if (cd == NULL) { rfree(RZONE_MODULE, RMOD_SYS, dev); return NULL; -- 2.5.0

2 1

[Sound-open-firmware] [PATCH] dai: rename frame_size in dai_config to sample_size
by Keyon Jie 03 Jan '17

03 Jan '17

The element frame_size in dai_config struct is actually sample size for each channel, e.g. 24 for S24_4LE, so rename it to sample_size to avoid confusion. Signed-off-by: Keyon Jie <yang.jie(a)linux.intel.com> --- src/drivers/ssp.c | 12 ++++++------ src/include/reef/dai.h | 2 +- src/ipc/intel-ipc.c | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/drivers/ssp.c b/src/drivers/ssp.c index c285af3..2b5b89e 100644 --- a/src/drivers/ssp.c +++ b/src/drivers/ssp.c @@ -260,8 +260,8 @@ static inline int ssp_set_config(struct dai *dai, struct dai_config *dai_config) case DAI_FMT_I2S: sscr0 |= SSCR0_PSP; sscr1 |= SSCR1_TRAIL; - sspsp |= SSPSP_SFRMWDTH(dai->config.frame_size + 1); - sspsp |= SSPSP_SFRMDLY((dai->config.frame_size + 1) * 2); + sspsp |= SSPSP_SFRMWDTH(dai->config.sample_size + 1); + sspsp |= SSPSP_SFRMDLY((dai->config.sample_size + 1) * 2); sspsp |= SSPSP_DMYSTRT(1); break; case DAI_FMT_DSP_A: @@ -274,11 +274,11 @@ static inline int ssp_set_config(struct dai *dai, struct dai_config *dai_config) return -EINVAL; } - /* frame size */ - if (dai->config.frame_size > 16) - sscr0 |= (SSCR0_EDSS | SSCR0_DSIZE(dai->config.frame_size - 16)); + /* sample size */ + if (dai->config.sample_size > 16) + sscr0 |= (SSCR0_EDSS | SSCR0_DSIZE(dai->config.sample_size - 16)); else - sscr0 |= SSCR0_DSIZE(dai->config.frame_size); + sscr0 |= SSCR0_DSIZE(dai->config.sample_size); /* watermarks - TODO: do we still need old sscr1 method ?? */ sscr1 |= (SSCR1_TX(4) | SSCR1_RX(4)); diff --git a/src/include/reef/dai.h b/src/include/reef/dai.h index cfcc428..5e301e1 100644 --- a/src/include/reef/dai.h +++ b/src/include/reef/dai.h @@ -100,7 +100,7 @@ struct dai_slot_map { /* DAI runtime hardware configuration */ struct dai_config { uint32_t format; - uint32_t frame_size; /* in BCLKs */ + uint32_t sample_size; /* in BCLKs */ struct dai_slot_map tx_slot_map[DAI_NUM_SLOT_MAPS]; struct dai_slot_map rx_slot_map[DAI_NUM_SLOT_MAPS]; uint32_t bclk; /* BCLK frequency in Hz */ diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index bfffaf6..127541d 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -565,7 +565,7 @@ static uint32_t ipc_device_set_formats(uint32_t header) dai_dev->dai_config.mclk = config_req.clock_frequency; dai_dev->dai_config.format = DAI_FMT_I2S | DAI_FMT_CONT | DAI_FMT_NB_NF | DAI_FMT_CBS_CFS; - dai_dev->dai_config.frame_size = 24; /* really sample size */ + dai_dev->dai_config.sample_size = 24; dai_dev->dai_config.bclk = 2400000; dai_dev->dai_config.mclk = 19200000; dai_dev->dai_config.clk_src = SSP_CLK_EXT; -- 2.7.4

2 1

[Sound-open-firmware] [PATCH 1/3] volume: add 24bit<==>32bit scaler and mapping
by Keyon Jie 03 Jan '17

03 Jan '17

As we are using 32 bits data in internal buffer, we need 24bit<==>32bit volume scaler for host<==>processing<==>dai converting. Signed-off-by: Keyon Jie <yang.jie(a)linux.intel.com> --- src/audio/volume.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/src/audio/volume.c b/src/audio/volume.c index d457632..e2badb8 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -191,6 +191,44 @@ static void vol_s24_to_s16(struct comp_dev *dev, struct comp_buffer *sink, sink->w_ptr = dest + i; } +/* copy and scale volume from 32 bit source buffer to 24 bit on 32 bit boundary dest buffer */ +static void vol_s32_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); + int32_t *src = (int32_t*) source->r_ptr; + int32_t i, *dest = (int32_t*) sink->w_ptr; + + /* buffer sizes are always divisible by period frames */ + for (i = 0; i < frames * 2; i += 2) { + dest[i] = ((int64_t)src[i] * cd->volume[0]) >> 24; + dest[i + 1] = ((int64_t)src[i + 1] * cd->volume[1]) >> 24; + } + + source->r_ptr = src + i; + sink->w_ptr = dest + i; +} + +/* copy and scale volume from 16 bit source buffer to 24 bit on 32 bit boundary dest buffer */ +static void vol_s24_to_s32(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 i, *src = (int32_t*) source->r_ptr; + int32_t *dest = (int32_t*) sink->w_ptr; + + /* buffer sizes are always divisible by period frames */ + for (i = 0; i < frames * 2; i += 2) { + dest[i] = (int32_t)(((int64_t)src[i] * + cd->volume[0]) >> 8); + dest[i + 1] = (int32_t)(((int64_t)src[i + 1] * + cd->volume[1]) >> 8); + } + + source->r_ptr = src + i; + sink->w_ptr = dest + i; +} + /* 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, 2, vol_s16_to_s16}, @@ -199,6 +237,8 @@ static const struct comp_func_map func_map[] = { {STREAM_FORMAT_S32_LE, STREAM_FORMAT_S32_LE, 2, vol_s32_to_s32}, {STREAM_FORMAT_S16_LE, STREAM_FORMAT_S24_4LE, 2, vol_s16_to_s24}, {STREAM_FORMAT_S24_4LE, STREAM_FORMAT_S16_LE, 2, vol_s24_to_s16}, + {STREAM_FORMAT_S32_LE, STREAM_FORMAT_S24_4LE, 2, vol_s32_to_s24}, + {STREAM_FORMAT_S24_4LE, STREAM_FORMAT_S32_LE, 2, vol_s24_to_s32}, }; static void vol_update(struct comp_data *cd, uint32_t chan) @@ -447,7 +487,7 @@ static int volume_prepare(struct comp_dev *dev) /* TODO tmp hard coded for 24 bit - need fixed for capture*/ /* is sink a host or DAI ? */ if (sink->sink->is_host || sink->sink->is_dai) - sink_format = STREAM_FORMAT_S24_4LE;//sink->params.pcm.format; + sink_format = sink->params.pcm.format; else sink_format = STREAM_FORMAT_S32_LE; -- 2.7.4

2 3

[Sound-open-firmware] [PATCH 1/2] dai: add stream_format to dai_data for codec stream
by Keyon Jie 29 Dec '16

29 Dec '16

Adding stream_format to indicate the stream format that the dai buffer is using, which will be used for volume copy functions mapping. Signed-off-by: Keyon Jie <yang.jie(a)linux.intel.com> --- src/audio/dai.c | 9 +++++++++ src/platform/baytrail/include/platform/platform.h | 3 +++ 2 files changed, 12 insertions(+) diff --git a/src/audio/dai.c b/src/audio/dai.c index 7bbde08..64e4761 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -59,6 +59,7 @@ struct dai_data { struct dma_sg_config config; int direction; + uint32_t stream_format; struct dai *ssp; struct dma *dma; @@ -162,6 +163,8 @@ static struct comp_dev *dai_new_ssp(uint32_t type, uint32_t index, if (dd->chan < 0) goto error; + dd->stream_format = PLATFORM_SSP_STREAM_FORMAT; + /* set up callback */ //if (dd->ssp->plat_data.flags & DAI_FLAGS_IRQ_CB) dma_set_cb(dd->dma, dd->chan, DMA_IRQ_TYPE_LLIST, dai_dma_cb, dev); @@ -217,6 +220,9 @@ static int dai_playback_params(struct comp_dev *dev, dma_period_desc = &dma_buffer->desc.sink_period; dma_buffer->params = *params; + /* set it to dai stream format, for volume func correct mapping */ + dma_buffer->params.pcm.format = dd->stream_format; + if (list_is_empty(&config->elem_list)) { /* set up cyclic list of DMA elems */ for (i = 0; i < dma_period_desc->number; i++) { @@ -275,6 +281,9 @@ static int dai_capture_params(struct comp_dev *dev, dma_period_desc = &dma_buffer->desc.source_period; dma_buffer->params = *params; + /* set it to dai stream format, for volume func correct mapping */ + dma_buffer->params.pcm.format = dd->stream_format; + if (list_is_empty(&config->elem_list)) { /* set up cyclic list of DMA elems */ for (i = 0; i < dma_period_desc->number; i++) { diff --git a/src/platform/baytrail/include/platform/platform.h b/src/platform/baytrail/include/platform/platform.h index b1bfbf2..f86019d 100644 --- a/src/platform/baytrail/include/platform/platform.h +++ b/src/platform/baytrail/include/platform/platform.h @@ -38,6 +38,9 @@ /* default static pipeline SSP port - not used for dynamic pipes */ #define PLATFORM_SSP_PORT 2 +/* default SSP stream format - need aligned with codec setting*/ +#define PLATFORM_SSP_STREAM_FORMAT STREAM_FORMAT_S24_4LE + /* IPC Interrupt */ #define PLATFORM_IPC_INTERUPT IRQ_NUM_EXT_IA -- 2.5.0

2 2