[Sound-open-firmware] [PATCH v2 00/26] add R/W pointers check for pipeline buffers
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(-)
We may meet src buffer bound, sink buffer bound, and splitting may not finish at one time DMA copy, that is, next_size < split_remaining, ...
This patch added to handle all of those cases, and prepare for the buffer read/write pointer checking in future.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 63 ++++++++++++++++++++++++++------------------------------ 1 file changed, 29 insertions(+), 34 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 217ad4f..5fc99fa 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -111,6 +111,7 @@ static void host_dma_cb_playback(struct comp_dev *dev, struct host_data *hd = comp_get_drvdata(dev); struct dma_sg_elem *local_elem, *source_elem, *sink_elem; struct comp_buffer *dma_buffer; + uint32_t next_size;
local_elem = list_first_item(&hd->config.elem_list, struct dma_sg_elem, list); @@ -148,48 +149,42 @@ static void host_dma_cb_playback(struct comp_dev *dev, ipc_stream_send_notification(dev, &hd->cp); }
- /* are we dealing with a split transfer */ - if (hd->split_remaining) { - - /* update local elem */ - local_elem->dest += local_elem->size; + local_elem->src += local_elem->size; + local_elem->dest += local_elem->size; + if (local_elem->src == hd->source->current_end) { + /* end of elem, so use next */ source_elem = next_buffer(hd->source); hd->source->current_end = source_elem->src + source_elem->size; local_elem->src = source_elem->src; - - /* set up next elem */ - local_elem->size = hd->split_remaining; - hd->next_inc = hd->split_remaining; - hd->split_remaining = 0; - - } else { - /* destination is always DSP period size */ + } + if (local_elem->dest == hd->sink->current_end) { + /* end of elem, so use next */ sink_elem = next_buffer(hd->sink); - + hd->sink->current_end = sink_elem->dest + sink_elem->size; local_elem->dest = sink_elem->dest; - local_elem->size = hd->period->size; - local_elem->src += hd->next_inc; - hd->next_inc = hd->period->size; - - /* are we at end of elem */ - if (local_elem->src == hd->source->current_end) { - - /* end of elem, so use next */ - source_elem = next_buffer(hd->source); - hd->source->current_end = source_elem->src + source_elem->size; - local_elem->src = source_elem->src; + }
- } else if (local_elem->src + hd->period->size > hd->source->current_end) { + next_size = hd->period->size; + if (local_elem->src + hd->period->size > hd->source->current_end) + next_size = hd->source->current_end - local_elem->src; + if (local_elem->dest + next_size > hd->sink->current_end) + next_size = hd->sink->current_end - local_elem->dest;
- /* split copy - split transaction into 2 copies */ - local_elem->size = hd->source->current_end - local_elem->src; - hd->split_remaining = hd->period->size - local_elem->size; + if (!hd->split_remaining) { + if (next_size != hd->period->size) + hd->split_remaining = hd->period->size - next_size; + } else { + next_size = next_size < hd->split_remaining ? + next_size : hd->split_remaining; + hd->split_remaining -= next_size; + } + local_elem->size = next_size;
- next->src = local_elem->src; - next->dest = local_elem->dest; - next->size = local_elem->size; - return; - } + if (hd->split_remaining) { + next->src = local_elem->src; + next->dest = local_elem->dest; + next->size = local_elem->size; + return; }
/* let any waiters know we have completed */
It reset dai_pos at prepare stage before, which will make it wrong when the prepare is called more than one time.
Here move the reset to MMAP_POS command handling, which should happen at stream allocating stage only.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/dai.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/audio/dai.c b/src/audio/dai.c index ab0164c..b015b2a 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -351,11 +351,6 @@ static int dai_prepare(struct comp_dev *dev) return -EINVAL; }
- dd->dai_pos_blks = 0; - - if (dd->dai_pos) - *dd->dai_pos = 0; - ret = dma_set_config(dd->dma, dd->chan, &dd->config); dev->state = COMP_STATE_PREPARE; return ret; @@ -435,6 +430,8 @@ static int dai_cmd(struct comp_dev *dev, int cmd, void *data) break; case COMP_CMD_IPC_MMAP_PPOS: dd->dai_pos = data; + if (dd->dai_pos) + *dd->dai_pos = 0; break; default: break;
We clear the buffer in pipeline/buffer creating stage, here we don't clear them for each preparing. --- src/audio/pipeline.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/src/audio/pipeline.c b/src/audio/pipeline.c index b89863d..9d74e37 100644 --- a/src/audio/pipeline.c +++ b/src/audio/pipeline.c @@ -331,16 +331,6 @@ static int component_op_sink(struct op_data *op_data, struct comp_dev *comp) return 0; }
- /* prepare the buffers first by clearing contents */ - list_for_item(clist, &comp->bsink_list) { - struct comp_buffer *buffer; - - buffer = container_of(clist, struct comp_buffer, - source_list); - - bzero(buffer->addr, buffer->desc.size); - } - /* prepare the component */ err = comp_prepare(comp);
We uses pull mode for running pipeline, that means, we start each copy in pipeline_schedule_copy().
So here remove this superfluous one period copy.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 5fc99fa..48efa4f 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -587,8 +587,6 @@ static int host_cmd(struct comp_dev *dev, int cmd, void *data) dev->state = COMP_STATE_RUNNING; break; case COMP_CMD_START: - dma_set_config(hd->dma, hd->chan, &hd->config); - dma_start(hd->dma, hd->chan); dev->state = COMP_STATE_RUNNING; break; case COMP_CMD_SUSPEND:
The presentation_posn used for dai rendered bytes readback to host side should be uint64_t type, according to the driver side definition.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/dai.c | 2 +- src/include/uapi/intel-ipc.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/audio/dai.c b/src/audio/dai.c index b015b2a..e827045 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -65,7 +65,7 @@ struct dai_data {
uint32_t dai_pos_blks; /* position in bytes (nearest block) */
- volatile uint32_t *dai_pos; + volatile uint64_t *dai_pos; /* host can read back this value without IPC */ };
/* this is called by DMA driver every time descriptor has completed */ diff --git a/src/include/uapi/intel-ipc.h b/src/include/uapi/intel-ipc.h index a4b6d28..a4589b3 100644 --- a/src/include/uapi/intel-ipc.h +++ b/src/include/uapi/intel-ipc.h @@ -612,7 +612,7 @@ struct sst_intel_ipc_stream_vol {
struct sst_intel_ipc_stream_data { uint32_t read_posn; - uint32_t presentation_posn; + uint64_t presentation_posn; struct sst_intel_ipc_stream_vol vol[IPC_INTEL_NO_CHANNELS]; } __attribute__((packed)); #endif
We need the application buffer pointer(write pointer for playback) update from the host side, to make the ALSA pcm buffer pointer checking possible.
Here we introduce component command COMP_CMD_AVAIL_UPDATE for the update event notification.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/include/reef/audio/component.h | 1 + src/ipc/intel-ipc.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+)
diff --git a/src/include/reef/audio/component.h b/src/include/reef/audio/component.h index e521753..bbd9b33 100644 --- a/src/include/reef/audio/component.h +++ b/src/include/reef/audio/component.h @@ -72,6 +72,7 @@ #define COMP_CMD_MUTE 101 #define COMP_CMD_UNMUTE 102 #define COMP_CMD_ROUTE 103 +#define COMP_CMD_AVAIL_UPDATE 104
/* MMAP IPC status */ #define COMP_CMD_IPC_MMAP_RPOS 200 /* host read position */ diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index 127541d..f192917 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -711,9 +711,43 @@ error:
static uint32_t ipc_stage_write_pos(uint32_t header) { + struct ipc_comp_dev *stream_dev; + struct ipc_intel_ipc_stream_set_position app_pos; + uint32_t stream_id; + int err; + trace_ipc("PoW");
+ /* read volume from the inbox */ + mailbox_inbox_read(&app_pos, 0, sizeof(app_pos)); + + trace_value(app_pos.position); + /* the driver uses stream ID to also identify certain mixers */ + stream_id = header & IPC_INTEL_STR_ID_MASK; + stream_id >>= IPC_INTEL_STR_ID_SHIFT; + + /* get the pcm_dev */ + stream_dev = ipc_get_comp(stream_id); + if (stream_dev == NULL) + goto error; + + err = pipeline_cmd(stream_dev->p, stream_dev->cd, + COMP_CMD_AVAIL_UPDATE, &app_pos); + if (err < 0) + goto error; + + /* drain the pipeline for EOS */ + if (app_pos.end_of_buffer) { + err = pipeline_cmd(stream_dev->p, stream_dev->cd, + COMP_CMD_DRAIN, NULL); + if (err < 0) + goto error; + } + return IPC_INTEL_GLB_REPLY_SUCCESS; + +error: + return IPC_INTEL_GLB_REPLY_ERROR_INVALID_PARAM; }
static uint32_t ipc_stage_message(uint32_t header)
It used IPC_INTEL_STR_PAUSE plus IPC_INTEL_STR_RESET to stop a stream, but sometimes we application only intend to stop the stream(e.g. speaker-test with large buffer size( > .wav file size)), which means we only reset the buffer pointers but don't need set params again(for reset, we need set params again).
Here add this stream operation IPC_INTEL_STR_STOP to handle this case.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/include/uapi/intel-ipc.h | 1 + src/ipc/intel-ipc.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)
diff --git a/src/include/uapi/intel-ipc.h b/src/include/uapi/intel-ipc.h index a4589b3..5fae423 100644 --- a/src/include/uapi/intel-ipc.h +++ b/src/include/uapi/intel-ipc.h @@ -182,6 +182,7 @@ enum ipc_str_operation { IPC_INTEL_STR_RESUME = 2, IPC_INTEL_STR_STAGE_MESSAGE = 3, IPC_INTEL_STR_NOTIFICATION = 4, + IPC_INTEL_STR_STOP = 5, IPC_INTEL_STR_MAX_MESSAGE };
diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index f192917..7db4179 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -811,6 +811,39 @@ error: return IPC_INTEL_GLB_REPLY_ERROR_INVALID_PARAM; }
+static uint32_t ipc_stream_stop(uint32_t header) +{ + struct ipc_pcm_dev *pcm_dev; + uint32_t stream_id; + int err; + + trace_ipc("SSt"); + + stream_id = header & IPC_INTEL_STR_ID_MASK; + stream_id >>= IPC_INTEL_STR_ID_SHIFT; + + /* get the pcm_dev */ + pcm_dev = ipc_get_pcm_comp(stream_id); + if (pcm_dev == NULL) { + trace_ipc_error("erg"); + goto error; + } + + /* send stop TODO: this should be done in trigger */ + err = pipeline_cmd(pcm_dev->dev.p, pcm_dev->dev.cd, + COMP_CMD_STOP, NULL); + if (err < 0) { + trace_ipc_error("erc"); + goto error; + } + + /* need prepare again before next start */ + pcm_dev->state = IPC_HOST_ALLOC; + return IPC_INTEL_GLB_REPLY_SUCCESS; +error: + return IPC_INTEL_GLB_REPLY_ERROR_INVALID_PARAM; +} + static uint32_t ipc_stream_pause(uint32_t header) { struct ipc_pcm_dev *pcm_dev; @@ -905,6 +938,8 @@ static uint32_t ipc_stream_message(uint32_t header) return ipc_stream_reset(header); case IPC_INTEL_STR_PAUSE: return ipc_stream_pause(header); + case IPC_INTEL_STR_STOP: + return ipc_stream_stop(header); case IPC_INTEL_STR_RESUME: return ipc_stream_resume(header); case IPC_INTEL_STR_STAGE_MESSAGE:
To implement host side buffer R/W pointer check, we need add buffer update procedures at first, here we introduce host_avail and host_free to indicate the buffer usuage status.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/src/audio/host.c b/src/audio/host.c index 48efa4f..3aba934 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -73,6 +73,9 @@ struct host_data { uint32_t host_pos_blks; /* position in bytes (nearest block) */ uint32_t host_period_bytes; /* host period size in bytes */ uint32_t host_period_pos; /* position in current host perid */ + uint32_t host_app_pos; /* host buffer app write pos, points to mailbox */ + uint32_t host_avail; /* host buffer available size */ + uint32_t host_free; /* host buffer free size */ /* pointers set during params to host or local above */ struct hc_buf *source; struct hc_buf *sink; @@ -84,6 +87,30 @@ struct host_data { struct comp_position cp; };
+static inline void host_update_buffer_produce(struct host_data *hd) +{ + if (hd->host_pos_blks < hd->host_app_pos) + hd->host_avail = hd->host_app_pos - hd->host_pos_blks; + else if (hd->host_pos_blks == hd->host_app_pos) + hd->host_avail = hd->host_size; /* full */ + else + hd->host_avail = hd->host_size -hd->host_pos_blks + + hd->host_app_pos; + hd->host_free = hd->host_size - hd->host_avail; +} + +static inline void host_update_buffer_consume(struct host_data *hd) +{ + if (hd->host_pos_blks < hd->host_app_pos) + hd->host_avail = hd->host_app_pos - hd->host_pos_blks; + else if (hd->host_pos_blks == hd->host_app_pos) + hd->host_avail = 0; /* empty */ + else + hd->host_avail = hd->host_size -hd->host_pos_blks + + hd->host_app_pos; + hd->host_free = hd->host_size - hd->host_avail; +} + static inline struct dma_sg_elem *next_buffer(struct hc_buf *hc) { struct dma_sg_elem *elem; @@ -141,6 +168,7 @@ static void host_dma_cb_playback(struct comp_dev *dev, hd->host_pos_blks = 0; if (hd->host_pos) *hd->host_pos = hd->host_pos_blks; + host_update_buffer_consume(hd);
/* send IPC message to driver if needed */ hd->host_period_pos += local_elem->size; @@ -541,6 +569,7 @@ static int host_prepare(struct comp_dev *dev) if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) host_preload(dev);
+ host_update_buffer_consume(hd); dev->state = COMP_STATE_PREPARE; return 0; }
We use host_pos_blks to store the latest read pointer, and report *host_pos to host side, here rename host_pos_blks to host_pos_read to make it understandable.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 3aba934..ed4af37 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -69,8 +69,8 @@ struct host_data { struct hc_buf host; struct hc_buf local; uint32_t host_size; - volatile uint32_t *host_pos; /* points to mailbox */ - uint32_t host_pos_blks; /* position in bytes (nearest block) */ + volatile uint32_t *host_pos; /* read pos, update to mailbox for host side */ + uint32_t host_pos_read; /* host buffer read pos in bytes */ uint32_t host_period_bytes; /* host period size in bytes */ uint32_t host_period_pos; /* position in current host perid */ uint32_t host_app_pos; /* host buffer app write pos, points to mailbox */ @@ -89,24 +89,24 @@ struct host_data {
static inline void host_update_buffer_produce(struct host_data *hd) { - if (hd->host_pos_blks < hd->host_app_pos) - hd->host_avail = hd->host_app_pos - hd->host_pos_blks; - else if (hd->host_pos_blks == hd->host_app_pos) + if (hd->host_pos_read < hd->host_app_pos) + hd->host_avail = hd->host_app_pos - hd->host_pos_read; + else if (hd->host_pos_read == hd->host_app_pos) hd->host_avail = hd->host_size; /* full */ else - hd->host_avail = hd->host_size -hd->host_pos_blks + + hd->host_avail = hd->host_size -hd->host_pos_read + hd->host_app_pos; hd->host_free = hd->host_size - hd->host_avail; }
static inline void host_update_buffer_consume(struct host_data *hd) { - if (hd->host_pos_blks < hd->host_app_pos) - hd->host_avail = hd->host_app_pos - hd->host_pos_blks; - else if (hd->host_pos_blks == hd->host_app_pos) + if (hd->host_pos_read < hd->host_app_pos) + hd->host_avail = hd->host_app_pos - hd->host_pos_read; + else if (hd->host_pos_read == hd->host_app_pos) hd->host_avail = 0; /* empty */ else - hd->host_avail = hd->host_size -hd->host_pos_blks + + hd->host_avail = hd->host_size -hd->host_pos_read + hd->host_app_pos; hd->host_free = hd->host_size - hd->host_avail; } @@ -161,13 +161,13 @@ static void host_dma_cb_playback(struct comp_dev *dev, comp_update_buffer_consume(hd->dma_buffer);
/* new local period, update host buffer position blks */ - hd->host_pos_blks += local_elem->size; + hd->host_pos_read += local_elem->size;
/* buffer overlap ? */ - if (hd->host_pos_blks >= hd->host_size) - hd->host_pos_blks = 0; + if (hd->host_pos_read >= hd->host_size) + hd->host_pos_read = 0; if (hd->host_pos) - *hd->host_pos = hd->host_pos_blks; + *hd->host_pos = hd->host_pos_read; host_update_buffer_consume(hd);
/* send IPC message to driver if needed */ @@ -250,13 +250,13 @@ static void host_dma_cb_capture(struct comp_dev *dev, #endif
/* new local period, update host buffer position blks */ - hd->host_pos_blks += local_elem->size; + hd->host_pos_read += local_elem->size;
/* buffer overlap ? */ - if (hd->host_pos_blks >= hd->host_size) - hd->host_pos_blks = 0; + if (hd->host_pos_read >= hd->host_size) + hd->host_pos_read = 0; if (hd->host_pos) - *hd->host_pos = hd->host_pos_blks; + *hd->host_pos = hd->host_pos_read;
/* recalc available buffer space */ comp_update_buffer_produce(hd->dma_buffer); @@ -559,7 +559,7 @@ static int host_prepare(struct comp_dev *dev)
if (hd->host_pos) *hd->host_pos = 0; - hd->host_pos_blks = 0; + hd->host_pos_read = 0; hd->host_period_pos = 0; hd->host_period_bytes = hd->params.period_frames * hd->params.frame_size;
This read pointer indicate how much we have read, we can use it to inform host side to refill the free periods.
But as it doesn't really mean that we have rendered all of them (there may be several periods in the pipeline buffer), and for the last bytes/period, host side will trigger stop immediately once it got this notification.
To avoid lost for last several periods issue, we postpone this notification to the finish of last bytes/period copy in dai, which will be add in the following patches.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/audio/host.c b/src/audio/host.c index ed4af37..4bd6406 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -174,7 +174,14 @@ static void host_dma_cb_playback(struct comp_dev *dev, hd->host_period_pos += local_elem->size; if (hd->host_period_pos >= hd->host_period_bytes) { hd->host_period_pos = 0; - ipc_stream_send_notification(dev, &hd->cp); + /* for the last bytes/period, send notification later */ + if (hd->host_avail) { + /* update for host side */ + if (hd->host_pos) { + *hd->host_pos = hd->host_pos_read; + ipc_stream_send_notification(dev, &hd->cp); + } + } }
local_elem->src += local_elem->size;
Here add R/W pointers check for playback, make sure the next copy size is not bigger than the host avail bytes.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 4bd6406..e8265df 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -138,7 +138,7 @@ static void host_dma_cb_playback(struct comp_dev *dev, struct host_data *hd = comp_get_drvdata(dev); struct dma_sg_elem *local_elem, *source_elem, *sink_elem; struct comp_buffer *dma_buffer; - uint32_t next_size; + uint32_t next_size, need_copy = 0;
local_elem = list_first_item(&hd->config.elem_list, struct dma_sg_elem, list); @@ -158,7 +158,7 @@ static void host_dma_cb_playback(struct comp_dev *dev, #endif
/* recalc available buffer space */ - comp_update_buffer_consume(hd->dma_buffer); + comp_update_buffer_produce(hd->dma_buffer);
/* new local period, update host buffer position blks */ hd->host_pos_read += local_elem->size; @@ -166,8 +166,6 @@ static void host_dma_cb_playback(struct comp_dev *dev, /* buffer overlap ? */ if (hd->host_pos_read >= hd->host_size) hd->host_pos_read = 0; - if (hd->host_pos) - *hd->host_pos = hd->host_pos_read; host_update_buffer_consume(hd);
/* send IPC message to driver if needed */ @@ -200,7 +198,7 @@ static void host_dma_cb_playback(struct comp_dev *dev, }
next_size = hd->period->size; - if (local_elem->src + hd->period->size > hd->source->current_end) + if (local_elem->src + next_size > hd->source->current_end) next_size = hd->source->current_end - local_elem->src; if (local_elem->dest + next_size > hd->sink->current_end) next_size = hd->sink->current_end - local_elem->dest; @@ -209,13 +207,33 @@ static void host_dma_cb_playback(struct comp_dev *dev, if (next_size != hd->period->size) hd->split_remaining = hd->period->size - next_size; } else { + need_copy = 1; next_size = next_size < hd->split_remaining ? next_size : hd->split_remaining; hd->split_remaining -= next_size; } local_elem->size = next_size;
- if (hd->split_remaining) { + /* check if avail is enough, otherwise, drain the last bytes and stop */ + if (hd->host_avail < local_elem->size) { + if (hd->host_avail == 0) { + /* end of stream, stop */ + next->size = 0; + need_copy = 0; + goto next_copy; + } + + /* end of stream, drain the last bytes */ + local_elem->size = hd->host_avail; + + /* the split_remaining may not be copied anymore, but, let's make it + correct. we have only hd->host_avail data, so the split_remaining + should be (next_size - hd->host_avail) bigger */ + hd->split_remaining += next_size - hd->host_avail; + } + +next_copy: + if (need_copy) { next->src = local_elem->src; next->dest = local_elem->dest; next->size = local_elem->size; @@ -266,7 +284,7 @@ static void host_dma_cb_capture(struct comp_dev *dev, *hd->host_pos = hd->host_pos_read;
/* recalc available buffer space */ - comp_update_buffer_produce(hd->dma_buffer); + comp_update_buffer_consume(hd->dma_buffer);
/* send IPC message to driver if needed */ hd->host_period_pos += local_elem->size;
We need to wait until the last bytes/period is finished in dai, before we can notify host side about that, otherwise, host side will trigger stop too early, and we will miss rendering them.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 38 +++++++++++++++++++++++ src/audio/pipeline.c | 1 + src/include/reef/audio/pipeline.h | 2 ++ src/platform/baytrail/include/platform/platform.h | 6 ++++ 4 files changed, 47 insertions(+)
diff --git a/src/audio/host.c b/src/audio/host.c index e8265df..6a4c669 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -64,6 +64,7 @@ struct host_data { completion_t complete; struct period_desc *period; struct comp_buffer *dma_buffer; + struct work work;
/* local and host DMA buffer info */ struct hc_buf host; @@ -220,6 +221,10 @@ static void host_dma_cb_playback(struct comp_dev *dev, /* end of stream, stop */ next->size = 0; need_copy = 0; + + /* will notify host side once dai tell us */ + wait_init(&dev->pipeline->complete); + work_schedule_default(&hd->work, PLATFORM_HOST_FINISH_DELAY); goto next_copy; }
@@ -354,6 +359,38 @@ static void host_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) host_dma_cb_capture(dev, next); }
+/* We need to wait until the last bytes/period is finished in dai, before we + * can notify host side about that, otherwise, host side will trigger stop too + * early, and we will miss rendering them. + * This work should be scheduled once the copy for host component is finished, + * and wait a timeout inside the work for pipeline->complete, which should be + * set in the endpoint(dai) rendering finishing callback. + */ +static uint32_t host_finish_work(void *data, uint32_t udelay) +{ + struct comp_dev *dev = (struct comp_dev *)data; + struct host_data *hd = comp_get_drvdata(dev); + int ret; + + dev->pipeline->complete.timeout = PLATFORM_HOST_FINISH_TIMEOUT; + ret = wait_for_completion_timeout(&dev->pipeline->complete); + if (ret < 0) + trace_comp_error("eHf"); + else { + trace_comp("hFw"); + /* update for host side */ + if (hd->host_pos) { + *hd->host_pos = hd->host_pos_read; + /* send the last notification to host */ + ipc_stream_send_notification(dev, &hd->cp); + } + } + + return 0; +} + + + static struct comp_dev *host_new(uint32_t type, uint32_t index, uint32_t direction) { @@ -389,6 +426,7 @@ static struct comp_dev *host_new(uint32_t type, uint32_t index, hd->source = NULL; hd->sink = NULL; hd->split_remaining = 0; + work_init(&hd->work, host_finish_work, dev, WORK_ASYNC);
/* init buffer elems */ list_init(&hd->config.elem_list); diff --git a/src/audio/pipeline.c b/src/audio/pipeline.c index 9d74e37..41693ae 100644 --- a/src/audio/pipeline.c +++ b/src/audio/pipeline.c @@ -141,6 +141,7 @@ struct pipeline *pipeline_new(uint32_t id) list_init(&p->host_ep_list); list_init(&p->dai_ep_list); list_init(&p->buffer_list); + wait_init(&p->complete);
spinlock_init(&p->lock); list_item_prepend(&p->list, &pipe_data->pipeline_list); diff --git a/src/include/reef/audio/pipeline.h b/src/include/reef/audio/pipeline.h index 26c1dd6..e7de501 100644 --- a/src/include/reef/audio/pipeline.h +++ b/src/include/reef/audio/pipeline.h @@ -39,6 +39,7 @@ #include <reef/dma.h> #include <reef/audio/component.h> #include <reef/trace.h> +#include <reef/wait.h>
#define trace_pipe(__e) trace_event(TRACE_CLASS_PIPE, __e) #define trace_pipe_error(__e) trace_error(TRACE_CLASS_PIPE, __e) @@ -50,6 +51,7 @@ struct pipeline { uint32_t id; /* id */ spinlock_t lock; + completion_t complete; /* indicate if the pipeline data is finished*/
/* lists */ struct list_item host_ep_list; /* list of host endpoints */ diff --git a/src/platform/baytrail/include/platform/platform.h b/src/platform/baytrail/include/platform/platform.h index f86019d..7df65b8 100644 --- a/src/platform/baytrail/include/platform/platform.h +++ b/src/platform/baytrail/include/platform/platform.h @@ -86,6 +86,12 @@ /* WorkQ window size in microseconds */ #define PLATFORM_WORKQ_WINDOW 2000
+/* Host finish work schedule delay in microseconds */ +#define PLATFORM_HOST_FINISH_DELAY 100 + +/* Host finish work(drain from host to dai) timeout in microseconds */ +#define PLATFORM_HOST_FINISH_TIMEOUT 50000 + /* Platform defined panic code */ #define platform_panic(__x) \ shim_write(SHIM_IPCXL, ((shim_read(SHIM_IPCXL) & 0xc0000000) |\
We need to clear those pointers at new(), and undo at reset() those initialization what we did in set_params().
For new(), rzalloc() will help set host_data members to 0s and NULLs.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 6a4c669..c8479c7 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -420,12 +420,6 @@ static struct comp_dev *host_new(uint32_t type, uint32_t index, hd->dma = dma_get(DMA_ID_DMAC0); if (hd->dma == NULL) goto error; - hd->host_size = 0; - hd->host_period_bytes = 0; - hd->host_pos = NULL; - hd->source = NULL; - hd->sink = NULL; - hd->split_remaining = 0; work_init(&hd->work, host_finish_work, dev, WORK_ASYNC);
/* init buffer elems */ @@ -740,10 +734,14 @@ static int host_reset(struct comp_dev *dev) }
hd->host_size = 0; + hd->host_avail= 0; if (hd->host_pos) *hd->host_pos = 0; hd->host_pos = NULL; + hd->host_app_pos = 0; + hd->host_period_bytes = 0; + hd->host_pos_read = 0; hd->source = NULL; hd->sink = NULL; dev->state = COMP_STATE_INIT;
1. only pause at running stage; 2. resetting buffer pointers and local_elem for stopping, to let next starting begin from original position; 3. adding processes for COMP_CMD_AVAIL_UPDATE.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 4 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index c8479c7..ccae9b7 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -43,6 +43,7 @@ #include <reef/wait.h> #include <reef/audio/component.h> #include <reef/audio/pipeline.h> +#include <uapi/intel-ipc.h> #include <platform/dma.h>
#define trace_host(__e) trace_event(TRACE_CLASS_HOST, __e) @@ -651,22 +652,66 @@ static struct comp_dev* host_volume_component(struct comp_dev *host) return comp_dev; }
+static int host_stop(struct comp_dev *dev) +{ + struct host_data *hd = comp_get_drvdata(dev); + struct dma_sg_elem *source_elem, *sink_elem, *local_elem; + + /* reset buffer pointers */ + if (hd->host_pos) + *hd->host_pos = 0; + hd->host_app_pos = 0; + hd->host_pos_read = 0; + hd->host_period_pos = 0; + host_update_buffer_consume(hd); + + /* reset buffer pointers and local_elem, to let next start + from original one */ + + /* setup elem to point to first source elem */ + source_elem = list_first_item(&hd->source->elem_list, + struct dma_sg_elem, list); + hd->source->current = &source_elem->list; + hd->source->current_end = source_elem->src + source_elem->size; + + /* setup elem to point to first sink elem */ + sink_elem = list_first_item(&hd->sink->elem_list, + struct dma_sg_elem, list); + hd->sink->current = &sink_elem->list; + hd->sink->current_end = sink_elem->dest + sink_elem->size; + + /* local element */ + local_elem = list_first_item(&hd->config.elem_list, + struct dma_sg_elem, list); + local_elem->dest = sink_elem->dest; + local_elem->size = hd->period->size; + local_elem->src = source_elem->src; + hd->next_inc = hd->period->size; + + dev->state = COMP_STATE_STOPPED; + return 0; +} + /* used to pass standard and bespoke commands (with data) to component */ static int host_cmd(struct comp_dev *dev, int cmd, void *data) { struct host_data *hd = comp_get_drvdata(dev); struct comp_dev *vol_dev = NULL; + struct ipc_intel_ipc_stream_set_position *app_pos; int ret = 0;
// TODO: align cmd macros. switch (cmd) { case COMP_CMD_PAUSE: - /* channel is paused by DAI */ - dev->state = COMP_STATE_PAUSED; + /* only support pausing for running, channel is paused by DAI */ + if (dev->state == COMP_STATE_RUNNING) + dev->state = COMP_STATE_PAUSED; break; case COMP_CMD_STOP: - /* stop any new DMA copies (let existing finish though) */ - dev->state = COMP_STATE_STOPPED; + if (dev->state == COMP_STATE_RUNNING || + dev->state == COMP_STATE_DRAINING || + dev->state == COMP_STATE_PAUSED) + ret = host_stop(dev); break; case COMP_CMD_RELEASE: /* channel is released by DAI */ @@ -681,6 +726,11 @@ static int host_cmd(struct comp_dev *dev, int cmd, void *data) case COMP_CMD_IPC_MMAP_RPOS: hd->host_pos = data; break; + case COMP_CMD_AVAIL_UPDATE: + app_pos = (struct ipc_intel_ipc_stream_set_position *)data; + hd->host_app_pos = app_pos->position; + host_update_buffer_produce(hd); + break; case COMP_CMD_VOLUME: vol_dev = host_volume_component(dev); if (vol_dev != NULL)
We don't copy when host_avail is 0(usually stream finished), at the same time, we need wait until the dma copy is finished before we can start next pulling copy.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/audio/host.c b/src/audio/host.c index ccae9b7..7c0e9f3 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -803,14 +803,26 @@ static int host_reset(struct comp_dev *dev) static int host_copy(struct comp_dev *dev) { struct host_data *hd = comp_get_drvdata(dev); + int ret;
+ trace_host("CpS"); if (dev->state != COMP_STATE_RUNNING) return 0;
- trace_host("CpS"); + if (hd->host_avail == 0) + return 0; + + /* do DMA transfer */ + wait_init(&hd->complete); dma_set_config(hd->dma, hd->chan, &hd->config); dma_start(hd->dma, hd->chan);
+ /* wait for DMA to finish */ + hd->complete.timeout = PLATFORM_DMA_TIMEOUT; + ret = wait_for_completion_timeout(&hd->complete); + if (ret < 0) + trace_comp_error("eHc"); + return 0; } struct comp_driver comp_host = {
If its sink is host, we need set it to 16bit(hard coded ATM); if its sink is dai, we need set it to 32bit SSP format; otherwise, for internal component, we uses STREAM_FORMAT_S32_LE.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/volume.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/audio/volume.c b/src/audio/volume.c index 24a9d11..75684b6 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -341,8 +341,26 @@ static void volume_free(struct comp_dev *dev) /* set component audio stream paramters */ static int volume_params(struct comp_dev *dev, struct stream_params *params) { + struct stream_params sink_params = *params; + struct comp_buffer *sink; + + /* volume components will only ever have 1 source and 1 sink buffer */ + sink = list_first_item(&dev->bsink_list, struct comp_buffer, source_list); + + /* hard coded until new IPC is ready */ + if (sink->sink->is_host) { + sink_params.pcm.format = STREAM_FORMAT_S16_LE; + sink_params.frame_size = 2 * params->channels; /* 16bit container */ + } else if (sink->sink->is_dai) { + sink_params.pcm.format = PLATFORM_SSP_STREAM_FORMAT; + sink_params.frame_size = 4 * params->channels; /* 32bit container */ + } else { + sink_params.pcm.format = STREAM_FORMAT_S32_LE; + sink_params.frame_size = 4 * params->channels; /* 32bit container */ + } + /* dont do any data transformation */ - comp_set_sink_params(dev, params); + comp_set_sink_params(dev, &sink_params);
return 0; }
For component 'inactive, but ready' status, naming 'SETUP' may be more understandable than 'STOPPED', and 'SETUP' is consistent with ALSA.
new() params() start() none ======> init =======> setup ======> running <====== <======= <====== free() reset() stop() --- src/audio/dai.c | 2 +- src/audio/host.c | 2 +- src/audio/mixer.c | 2 +- src/audio/volume.c | 2 +- src/include/reef/audio/component.h | 12 +++++++++--- 5 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/src/audio/dai.c b/src/audio/dai.c index e827045..9c40b2f 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -403,7 +403,7 @@ static int dai_cmd(struct comp_dev *dev, int cmd, void *data) dai_trigger(dd->ssp, cmd, dd->direction); /* go through */ case COMP_STATE_PREPARE: - dev->state = COMP_STATE_INIT; + dev->state = COMP_STATE_SETUP; break; } break; diff --git a/src/audio/host.c b/src/audio/host.c index 7c0e9f3..5c9f1ad 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -688,7 +688,7 @@ static int host_stop(struct comp_dev *dev) local_elem->src = source_elem->src; hd->next_inc = hd->period->size;
- dev->state = COMP_STATE_STOPPED; + dev->state = COMP_STATE_SETUP; return 0; }
diff --git a/src/audio/mixer.c b/src/audio/mixer.c index 3a8d12f..4487862 100644 --- a/src/audio/mixer.c +++ b/src/audio/mixer.c @@ -250,7 +250,7 @@ static int mixer_reset(struct comp_dev *dev) list_for_item(blist, &dev->bsource_list) { source = container_of(blist, struct comp_buffer, sink_list); /* only mix the sources with the same state with mixer*/ - if (source->source->state > COMP_STATE_STOPPED) + if (source->source->state > COMP_STATE_SETUP) return 1; /* should not reset the downstream components */ }
diff --git a/src/audio/volume.c b/src/audio/volume.c index 75684b6..03e9dd4 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -429,7 +429,7 @@ static int volume_cmd(struct comp_dev *dev, int cmd, void *data) dev->state = COMP_STATE_RUNNING; break; case COMP_CMD_STOP: - dev->state = COMP_STATE_STOPPED; + dev->state = COMP_STATE_SETUP; break; case COMP_CMD_PAUSE: dev->state = COMP_STATE_PAUSED; diff --git a/src/include/reef/audio/component.h b/src/include/reef/audio/component.h index bbd9b33..9275abd 100644 --- a/src/include/reef/audio/component.h +++ b/src/include/reef/audio/component.h @@ -40,10 +40,16 @@ #include <reef/dma.h> #include <reef/stream.h>
-/* audio component states */ +/* audio component states + * the states may transform as below: + * new() params() start() + * none -----> init ------> setup -----> running + * none <----- init <------ setup <----- running + * free() reset() stop() + */ #define COMP_STATE_INIT 0 /* component being initialised */ -#define COMP_STATE_SUSPEND 1 /* component suspended */ -#define COMP_STATE_STOPPED 2 /* component inactive, but ready */ +#define COMP_STATE_SETUP 1 /* component inactive, but ready */ +#define COMP_STATE_SUSPEND 2 /* component suspended */ #define COMP_STATE_DRAINING 3 /* component draining */ #define COMP_STATE_PREPARE 4 /* component prepared */ #define COMP_STATE_PAUSED 5 /* component paused */
We should only stop/pause it when the volume component is in running status.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/volume.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/audio/volume.c b/src/audio/volume.c index 03e9dd4..ee49b3a 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -429,10 +429,15 @@ static int volume_cmd(struct comp_dev *dev, int cmd, void *data) dev->state = COMP_STATE_RUNNING; break; case COMP_CMD_STOP: - dev->state = COMP_STATE_SETUP; + if (dev->state == COMP_STATE_RUNNING || + dev->state == COMP_STATE_DRAINING || + dev->state == COMP_STATE_PAUSED) + dev->state = COMP_STATE_SETUP; break; case COMP_CMD_PAUSE: - dev->state = COMP_STATE_PAUSED; + /* only support pausing for running */ + if (dev->state == COMP_STATE_RUNNING) + dev->state = COMP_STATE_PAUSED; break; case COMP_CMD_RELEASE: dev->state = COMP_STATE_RUNNING;
We should only copy when there is data, as we are using pulling mode, should sink side should have free space always, we check source->avail only and copy only some bytes when avail < period size.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/volume.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/audio/volume.c b/src/audio/volume.c index ee49b3a..822c92d 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -473,6 +473,16 @@ static int volume_copy(struct comp_dev *dev) trace_value((uint32_t)(sink->w_ptr - sink->addr)); #endif
+ if (source->avail < cframes * source->params.frame_size || + sink->free < cframes * sink->params.frame_size) + cframes = source->avail / source->params.frame_size; + + /* no data to copy */ + if (cframes == 0) { + trace_value(source->avail); + return 0; + } + /* copy and scale volume */ cd->scale_vol(dev, sink, source, cframes);
@@ -538,7 +548,7 @@ found:
/* copy periods from host */ if (source->params.direction == STREAM_DIRECTION_PLAYBACK) { - for (i = 0; i < PLAT_HOST_PERIODS; i++) + for (i = 0; i < PLAT_INT_PERIODS; i++) volume_copy(dev); }
As its sink won't be host/dai, it is alwasys internal component, let's hard code it to STREAM_FORMAT_S32_LE ATM.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/mixer.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/audio/mixer.c b/src/audio/mixer.c index 4487862..57669a7 100644 --- a/src/audio/mixer.c +++ b/src/audio/mixer.c @@ -116,12 +116,18 @@ static void mixer_free(struct comp_dev *dev) /* set component audio stream paramters */ static int mixer_params(struct comp_dev *dev, struct stream_params *params) { + struct stream_params sink_params = *params; + /* dont do any params downstream setting for running mixer stream */ if (dev->state == COMP_STATE_RUNNING) return 1;
+ /* suppose sink component won't be host/dai, so hard code it */ + sink_params.pcm.format = STREAM_FORMAT_S32_LE; + sink_params.frame_size = 4 * params->channels; /* 32bit container */ + /* dont do any data transformation */ - comp_set_sink_params(dev, params); + comp_set_sink_params(dev, &sink_params);
return 0; }
We should only copy when there is data, as we are using pulling mode, should sink side should have free space always, we check each source->avail and copy only some bytes when avail < period size.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/mixer.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/audio/mixer.c b/src/audio/mixer.c index 57669a7..754090f 100644 --- a/src/audio/mixer.c +++ b/src/audio/mixer.c @@ -214,7 +214,7 @@ static int mixer_copy(struct comp_dev *dev) { struct mixer_data *md = comp_get_drvdata(dev); struct comp_buffer *sink, *sources[5], *source; - uint32_t i = 0, cframes = PLAT_INT_PERIOD_FRAMES; + uint32_t i = 0, num_mix_sources, cframes = PLAT_INT_PERIOD_FRAMES; struct list_item * blist;
trace_mixer("Mix"); @@ -227,13 +227,31 @@ static int mixer_copy(struct comp_dev *dev) sources[i++] = source; }
+ num_mix_sources = i; + sink = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);
+ for(i = 0; i < num_mix_sources; i++) { + if (sources[i]->avail < cframes * sources[i]->params.frame_size) + cframes = sources[i]->avail /sources[i]->params.frame_size; + } + if (sink->free < cframes * sink->params.frame_size) + cframes = sink->free /sink->params.frame_size; + + if (num_mix_sources == 0) + cframes = 0; + + /* no frames to mix */ + if (cframes == 0) { + trace_value(cframes); + return 0; + } + /* mix streams */ md->mix_func(dev, sink, sources, i, cframes);
/* update buffer pointers for overflow */ - for(; i > 0; i--) { + for(i = num_mix_sources; 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_consume(sources[i-1]); @@ -260,6 +278,7 @@ static int mixer_reset(struct comp_dev *dev) return 1; /* should not reset the downstream components */ }
+ dev->state = COMP_STATE_INIT; return 0; }
For the last bytes (< 1 period) copy, we need to remember this for the size caculation at finished, here add dd->last_bytes for this purpos.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/dai.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/audio/dai.c b/src/audio/dai.c index 9c40b2f..3b2ea30 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -63,6 +63,7 @@ struct dai_data { struct dai *ssp; struct dma *dma;
+ uint32_t last_bytes; /* the last bytes(<period size) it copies. */ uint32_t dai_pos_blks; /* position in bytes (nearest block) */
volatile uint64_t *dai_pos; /* host can read back this value without IPC */ @@ -159,6 +160,8 @@ static struct comp_dev *dai_new_ssp(uint32_t type, uint32_t index,
list_init(&dd->config.elem_list); dd->dai_pos = NULL; + dd->dai_pos_blks = 0; + dd->last_bytes = 0;
/* get DMA channel from DMAC1 */ dd->chan = dma_channel_get(dd->dma); @@ -374,6 +377,7 @@ static int dai_reset(struct comp_dev *dev) if (dd->dai_pos) *dd->dai_pos = 0; dd->dai_pos = NULL; + dd->last_bytes = 0;
return 0; } @@ -403,6 +407,7 @@ static int dai_cmd(struct comp_dev *dev, int cmd, void *data) dai_trigger(dd->ssp, cmd, dd->direction); /* go through */ case COMP_STATE_PREPARE: + dd->last_bytes = 0; dev->state = COMP_STATE_SETUP; break; }
We should only copy when there is data, after copied the last bytes or period, we will stop the dai and ssp, and trigger the pipeline finish, which will notify host side the last read pointer and let host side send trigger stop ipc.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/dai.c | 47 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 5 deletions(-)
diff --git a/src/audio/dai.c b/src/audio/dai.c index 3b2ea30..186532f 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -69,6 +69,7 @@ struct dai_data { volatile uint64_t *dai_pos; /* host can read back this value without IPC */ };
+static int dai_cmd(struct comp_dev *dev, int cmd, void *data); /* this is called by DMA driver every time descriptor has completed */ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) { @@ -76,17 +77,22 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) struct dai_data *dd = comp_get_drvdata(dev); struct period_desc *dma_period_desc; struct comp_buffer *dma_buffer; + uint32_t copied_size;
if (dd->direction == STREAM_DIRECTION_PLAYBACK) { dma_buffer = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list);
dma_period_desc = &dma_buffer->desc.sink_period; - dma_buffer->r_ptr += dma_period_desc->size; + copied_size = dd->last_bytes ? dd->last_bytes : dma_period_desc->size; + dma_buffer->r_ptr += copied_size;
/* check for end of buffer */ - if (dma_buffer->r_ptr >= dma_buffer->end_addr) + if (dma_buffer->r_ptr >= dma_buffer->end_addr) { dma_buffer->r_ptr = dma_buffer->addr; + /* update host position(in bytes offset) for drivers */ + dd->dai_pos_blks += dma_buffer->desc.size; + }
#if 0 // TODO: move this to new trace mechanism @@ -109,16 +115,17 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) dma_buffer->w_ptr += dma_period_desc->size;
/* check for end of buffer */ - if (dma_buffer->w_ptr >= dma_buffer->end_addr) + if (dma_buffer->w_ptr >= dma_buffer->end_addr) { dma_buffer->w_ptr = dma_buffer->addr; + /* update host position(in bytes offset) for drivers */ + dd->dai_pos_blks += dma_buffer->desc.size; + }
#if 0 // TODO: move this to new trace mechanism trace_value((uint32_t)(dma_buffer->w_ptr - dma_buffer->addr)); #endif
- /* update host position(in bytes offset) for drivers */ - dd->dai_pos_blks += dma_period_desc->size; if (dd->dai_pos) *dd->dai_pos = dd->dai_pos_blks + dma_buffer->w_ptr - dma_buffer->addr; @@ -127,9 +134,39 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) comp_update_buffer_produce(dma_buffer); }
+ if (dd->direction == STREAM_DIRECTION_PLAYBACK && + dma_buffer->avail < dma_period_desc->size) { + /* end of stream, finish */ + if (dma_buffer->avail == 0) { + dai_cmd(dev, COMP_CMD_STOP, NULL); + + /* stop dma immediatly */ + next->size = 0; + + /* let any waiters know we have completed */ + wait_completed(&dev->pipeline->complete); + + return; + } else { + /* drain the last bytes */ + next->src = (uint32_t)dma_buffer->r_ptr; + next->dest = dai_fifo(dd->ssp, dd->direction); + next->size = dma_buffer->avail; + + dd->last_bytes = next->size; + + goto next_copy; + + } + + } /* notify pipeline that DAI needs it's buffer filled */ // if (dev->state == COMP_STATE_RUNNING) pipeline_schedule_copy(dev->pipeline, dev); + +next_copy: + + return; }
static struct comp_dev *dai_new_ssp(uint32_t type, uint32_t index,
After finished a transfer copy, check for reload channel: 1. if next.size is DMA_RELOAD_END, stop this dma copy; 2. if next.size > 0 but not DMA_RELOAD_LLI, use next element for next copy; 3. if we are waiting for pause, pause it; 4. otherwise, reload lli.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/dai.c | 2 +- src/drivers/dw-dma.c | 28 ++++++++++++++++------------ src/include/reef/dma.h | 7 +++++++ 3 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/src/audio/dai.c b/src/audio/dai.c index 186532f..230acbf 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -141,7 +141,7 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) dai_cmd(dev, COMP_CMD_STOP, NULL);
/* stop dma immediatly */ - next->size = 0; + next->size = DMA_RELOAD_END;
/* let any waiters know we have completed */ wait_completed(&dev->pipeline->complete); diff --git a/src/drivers/dw-dma.c b/src/drivers/dw-dma.c index c073ed5..279d4f4 100644 --- a/src/drivers/dw-dma.c +++ b/src/drivers/dw-dma.c @@ -387,7 +387,6 @@ out: static int dw_dma_release(struct dma *dma, int channel) { struct dma_pdata *p = dma_get_drvdata(dma); - struct dma_sg_elem next; uint32_t flags;
spin_lock_irq(&dma->lock, flags); @@ -395,8 +394,6 @@ static int dw_dma_release(struct dma *dma, int channel) trace_dma("Dpr");
if (p->chan[channel].status == DMA_STATUS_PAUSED) { - if (p->chan[channel].cb && p->chan[channel].cb_type & DMA_IRQ_TYPE_LLIST) - p->chan[channel].cb(p->chan[channel].cb_data, DMA_IRQ_TYPE_LLIST, &next); dw_dma_chan_reload_lli(dma, channel); }
@@ -834,20 +831,27 @@ static void dw_dma_irq_handler(void *data) if ((status_tfr & mask) && (p->chan[i].cb_type & DMA_IRQ_TYPE_LLIST)) {
- if (p->chan[i].status == DMA_STATUS_PAUSING) { - p->chan[i].status = DMA_STATUS_PAUSED; - continue; - } - - next.size = 0; + next.size = DMA_RELOAD_LLI; /* will reload lli by default */ if (p->chan[i].cb) p->chan[i].cb(p->chan[i].cb_data, DMA_IRQ_TYPE_LLIST, &next);
- /* check for reload channel */ - if (next.size > 0) + /* check for reload channel: + * next.size is DMA_RELOAD_END, stop this dma copy; + * next.size > 0 but not DMA_RELOAD_LLI, use next + * element for next copy; + * if we are waiting for pause, pause it; + * otherwise, reload lli + */ + if (next.size == DMA_RELOAD_END) + continue; + else if (next.size != DMA_RELOAD_LLI) dw_dma_chan_reload_next(dma, i, &next); - else + /* reload lli, but let's check if we are pausing first */ + else if (p->chan[i].status == DMA_STATUS_PAUSING) { + p->chan[i].status = DMA_STATUS_PAUSED; + continue; + } else dw_dma_chan_reload_lli(dma, i); } #if DW_USE_HW_LLI diff --git a/src/include/reef/dma.h b/src/include/reef/dma.h index 6de2ede..6311260 100644 --- a/src/include/reef/dma.h +++ b/src/include/reef/dma.h @@ -59,6 +59,13 @@ #define DMA_IRQ_TYPE_BLOCK (1 << 0) #define DMA_IRQ_TYPE_LLIST (1 << 1)
+ +/* We will use this macro in cb handler to inform dma that + * we need to stop the reload for specail purpose + */ +#define DMA_RELOAD_END 0 +#define DMA_RELOAD_LLI 0xFFFFFFFF + struct dma;
struct dma_sg_elem {
For command stop, excepting reset components, we may need reset the buffer next to the component, otherwise, if the next start command comes(without reset before it), the buffer pointers may be disordered.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 3 +++ src/audio/mixer.c | 6 +++++- src/audio/volume.c | 4 +++- src/include/reef/audio/component.h | 27 +++++++++++++++++++++++++++ 4 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 5c9f1ad..b818351 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -688,6 +688,9 @@ static int host_stop(struct comp_dev *dev) local_elem->src = source_elem->src; hd->next_inc = hd->period->size;
+ /* now reset downstream buffer */ + comp_buffer_reset(dev); + dev->state = COMP_STATE_SETUP; return 0; } diff --git a/src/audio/mixer.c b/src/audio/mixer.c index 754090f..c653fd9 100644 --- a/src/audio/mixer.c +++ b/src/audio/mixer.c @@ -184,7 +184,6 @@ static int mixer_cmd(struct comp_dev *dev, int cmd, void *data) switch(cmd) { case COMP_CMD_START: trace_mixer("MSa"); - case COMP_CMD_STOP: case COMP_CMD_PAUSE: case COMP_CMD_RELEASE: case COMP_CMD_DRAIN: @@ -192,6 +191,11 @@ static int mixer_cmd(struct comp_dev *dev, int cmd, void *data) case COMP_CMD_RESUME: finish = mixer_status_change(dev); break; + case COMP_CMD_STOP: + finish = mixer_status_change(dev); + if (finish == 0) + comp_buffer_reset(dev); + break; case COMP_CMD_VOLUME: vol_dev = mixer_volume_component(dev); if (vol_dev != NULL) diff --git a/src/audio/volume.c b/src/audio/volume.c index 822c92d..f347847 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -431,8 +431,10 @@ static int volume_cmd(struct comp_dev *dev, int cmd, void *data) case COMP_CMD_STOP: if (dev->state == COMP_STATE_RUNNING || dev->state == COMP_STATE_DRAINING || - dev->state == COMP_STATE_PAUSED) + dev->state == COMP_STATE_PAUSED) { + comp_buffer_reset(dev); dev->state = COMP_STATE_SETUP; + } break; case COMP_CMD_PAUSE: /* only support pausing for running */ diff --git a/src/include/reef/audio/component.h b/src/include/reef/audio/component.h index 9275abd..3abae3d 100644 --- a/src/include/reef/audio/component.h +++ b/src/include/reef/audio/component.h @@ -39,6 +39,7 @@ #include <reef/reef.h> #include <reef/dma.h> #include <reef/stream.h> +#include <reef/alloc.h>
/* audio component states * the states may transform as below: @@ -310,6 +311,32 @@ static inline int comp_dai_loopback(struct comp_dev *dev, return 0; }
+/* reset component downstream buffers */ +static inline int comp_buffer_reset(struct comp_dev *dev) +{ + struct list_item *clist; + + /* reset downstream buffers */ + list_for_item(clist, &dev->bsink_list) { + struct comp_buffer *buffer; + + buffer = container_of(clist, struct comp_buffer, source_list); + + /* dont reset buffer if the component is not connected */ + if (!buffer->connected) + continue; + + /* reset buffer next to the component*/ + bzero(buffer->addr, buffer->desc.size); + buffer->w_ptr = buffer->r_ptr = buffer->addr; + buffer->end_addr = buffer->addr + buffer->desc.size; + buffer->free = buffer->desc.size; + buffer->avail = 0; + } + + return 0; +} + /* default base component initialisations */ void sys_comp_dai_init(void); void sys_comp_host_init(void);
The elements reset code may be shared by params() and stop(), and the host side pointer resetting code may be used by both reset() and stop().
Here create separate functions for those common codes, to make code more clean.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 90 ++++++++++++++++++++++++++------------------------------ 1 file changed, 41 insertions(+), 49 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index b818351..91039d1 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -498,12 +498,39 @@ unwind: return -ENOMEM; }
+static int host_elements_reset(struct comp_dev *dev) +{ + struct host_data *hd = comp_get_drvdata(dev); + struct dma_sg_elem *source_elem, *sink_elem, *local_elem; + + /* setup elem to point to first source elem */ + source_elem = list_first_item(&hd->source->elem_list, + struct dma_sg_elem, list); + hd->source->current = &source_elem->list; + hd->source->current_end = source_elem->src + source_elem->size; + + /* setup elem to point to first sink elem */ + sink_elem = list_first_item(&hd->sink->elem_list, + struct dma_sg_elem, list); + hd->sink->current = &sink_elem->list; + hd->sink->current_end = sink_elem->dest + sink_elem->size; + + /* local element */ + local_elem = list_first_item(&hd->config.elem_list, + struct dma_sg_elem, list); + local_elem->dest = sink_elem->dest; + local_elem->size = hd->period->size; + local_elem->src = source_elem->src; + hd->next_inc = hd->period->size; + + return 0; +} + /* configure the DMA params and descriptors for host buffer IO */ static int host_params(struct comp_dev *dev, struct stream_params *params) { struct host_data *hd = comp_get_drvdata(dev); struct dma_sg_config *config = &hd->config; - struct dma_sg_elem *source_elem, *sink_elem, *local_elem; int err;
/* set params */ @@ -548,25 +575,7 @@ static int host_params(struct comp_dev *dev, struct stream_params *params) config->dest_width = sizeof(uint32_t); config->cyclic = 0;
- /* setup elem to point to first source elem */ - source_elem = list_first_item(&hd->source->elem_list, - struct dma_sg_elem, list); - hd->source->current = &source_elem->list; - hd->source->current_end = source_elem->src + source_elem->size; - - /* setup elem to point to first sink elem */ - sink_elem = list_first_item(&hd->sink->elem_list, - struct dma_sg_elem, list); - hd->sink->current = &sink_elem->list; - hd->sink->current_end = sink_elem->dest + sink_elem->size; - - /* local element */ - local_elem = list_first_item(&hd->config.elem_list, - struct dma_sg_elem, list); - local_elem->dest = sink_elem->dest; - local_elem->size = hd->period->size; - local_elem->src = source_elem->src; - hd->next_inc = hd->period->size; + host_elements_reset(dev); return 0; }
@@ -652,10 +661,9 @@ static struct comp_dev* host_volume_component(struct comp_dev *host) return comp_dev; }
-static int host_stop(struct comp_dev *dev) +static int host_pointer_reset(struct comp_dev *dev) { struct host_data *hd = comp_get_drvdata(dev); - struct dma_sg_elem *source_elem, *sink_elem, *local_elem;
/* reset buffer pointers */ if (hd->host_pos) @@ -663,30 +671,19 @@ static int host_stop(struct comp_dev *dev) hd->host_app_pos = 0; hd->host_pos_read = 0; hd->host_period_pos = 0; - host_update_buffer_consume(hd); - - /* reset buffer pointers and local_elem, to let next start - from original one */ + hd->host_size = 0; + hd->host_avail= 0;
- /* setup elem to point to first source elem */ - source_elem = list_first_item(&hd->source->elem_list, - struct dma_sg_elem, list); - hd->source->current = &source_elem->list; - hd->source->current_end = source_elem->src + source_elem->size; + return 0; +}
- /* setup elem to point to first sink elem */ - sink_elem = list_first_item(&hd->sink->elem_list, - struct dma_sg_elem, list); - hd->sink->current = &sink_elem->list; - hd->sink->current_end = sink_elem->dest + sink_elem->size; +static int host_stop(struct comp_dev *dev) +{ + /* reset host side buffer pointers */ + host_pointer_reset(dev);
- /* local element */ - local_elem = list_first_item(&hd->config.elem_list, - struct dma_sg_elem, list); - local_elem->dest = sink_elem->dest; - local_elem->size = hd->period->size; - local_elem->src = source_elem->src; - hd->next_inc = hd->period->size; + /* reset elements, to let next start from original one */ + host_elements_reset(dev);
/* now reset downstream buffer */ comp_buffer_reset(dev); @@ -786,15 +783,10 @@ static int host_reset(struct comp_dev *dev) rfree(RZONE_MODULE, RMOD_SYS, e); }
- hd->host_size = 0; - hd->host_avail= 0; - if (hd->host_pos) - *hd->host_pos = 0; + host_pointer_reset(dev); hd->host_pos = NULL; - hd->host_app_pos = 0;
hd->host_period_bytes = 0; - hd->host_pos_read = 0; hd->source = NULL; hd->sink = NULL; dev->state = COMP_STATE_INIT;
On Thu, 2017-02-09 at 23:03 +0800, 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:
- (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;
- (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 ==> ...
Keyon, this is not applying on top of master. Maybe something in your rebase ?
Can you fix and I'll then pull in your branch.
Thanks
Liam
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Saturday, February 11, 2017 12:08 AM To: Keyon Jie yang.jie@linux.intel.com Cc: sound-open-firmware@alsa-project.org; Jie, Yang yang.jie@intel.com; Ingalsuo, Seppo seppo.ingalsuo@intel.com Subject: Re: [PATCH v2 00/26] add R/W pointers check for pipeline buffers
On Thu, 2017-02-09 at 23:03 +0800, 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:
- (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 ==> ...
Keyon, this is not applying on top of master. Maybe something in your rebase ?
Can you fix and I'll then pull in your branch.
That looks somewhat strange, it applied on top of master on my side (based on commit 1659269bfa08fdc52b4139b8984b70139b6f3dd8).
Anyway, let me send my latest v3(one preload patch added) which should work.
Thanks, ~Keyon
Thanks
Liam
participants (3)
-
Jie, Yang -
Keyon Jie -
Liam Girdwood