[Sound-open-firmware] [PATCH 00/25] 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.
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(-)
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 add COMP_CMD_IPC_MMAP_WPOS for the application pointer mmap command, and 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 | 2 ++ src/include/uapi/intel-ipc.h | 1 + src/ipc/intel-ipc.c | 42 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+)
diff --git a/src/include/reef/audio/component.h b/src/include/reef/audio/component.h index e521753..fcef11e 100644 --- a/src/include/reef/audio/component.h +++ b/src/include/reef/audio/component.h @@ -72,10 +72,12 @@ #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 */ #define COMP_CMD_IPC_MMAP_PPOS 201 /* DAI presentation position */ +#define COMP_CMD_IPC_MMAP_WPOS 202 /* host write position */
#define COMP_CMD_IPC_MMAP_VOL(chan) (216 + chan) /* Volume */
diff --git a/src/include/uapi/intel-ipc.h b/src/include/uapi/intel-ipc.h index a4589b3..ec2ef32 100644 --- a/src/include/uapi/intel-ipc.h +++ b/src/include/uapi/intel-ipc.h @@ -613,6 +613,7 @@ struct sst_intel_ipc_stream_vol { struct sst_intel_ipc_stream_data { uint32_t read_posn; uint64_t presentation_posn; + uint32_t write_posn; struct sst_intel_ipc_stream_vol vol[IPC_INTEL_NO_CHANNELS]; } __attribute__((packed)); #endif diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index 127541d..6084fa4 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -393,6 +393,14 @@ static uint32_t ipc_stream_alloc(uint32_t header) goto error; }
+ /* pass the IPC write posn pointer to the host */ + err = comp_cmd(pcm_dev->dev.cd, COMP_CMD_IPC_MMAP_WPOS, + &_stream_data->write_posn); + if (err < 0) { + trace_ipc_error("eAW"); + goto error; + } + /* pass the volume readback posn to the host */ err = comp_cmd(volume_dev->cd, COMP_CMD_IPC_MMAP_VOL(0), &_stream_data->vol[0].vol); @@ -711,9 +719,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)
On Tue, 2017-02-07 at 22:02 +0800, Keyon Jie wrote:
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 add COMP_CMD_IPC_MMAP_WPOS for the application pointer mmap command, and 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 | 2 ++ src/include/uapi/intel-ipc.h | 1 + src/ipc/intel-ipc.c | 42 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+)
diff --git a/src/include/reef/audio/component.h b/src/include/reef/audio/component.h index e521753..fcef11e 100644 --- a/src/include/reef/audio/component.h +++ b/src/include/reef/audio/component.h @@ -72,10 +72,12 @@ #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 */ #define COMP_CMD_IPC_MMAP_PPOS 201 /* DAI presentation position */ +#define COMP_CMD_IPC_MMAP_WPOS 202 /* host write position */
Is the host writing this directly to DSP memory via mmap() or IPC. If IPC then use a different macro name.
Liam
#define COMP_CMD_IPC_MMAP_VOL(chan) (216 + chan) /* Volume */
diff --git a/src/include/uapi/intel-ipc.h b/src/include/uapi/intel-ipc.h index a4589b3..ec2ef32 100644 --- a/src/include/uapi/intel-ipc.h +++ b/src/include/uapi/intel-ipc.h @@ -613,6 +613,7 @@ struct sst_intel_ipc_stream_vol { struct sst_intel_ipc_stream_data { uint32_t read_posn; uint64_t presentation_posn;
- uint32_t write_posn; struct sst_intel_ipc_stream_vol vol[IPC_INTEL_NO_CHANNELS];
} __attribute__((packed)); #endif diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index 127541d..6084fa4 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -393,6 +393,14 @@ static uint32_t ipc_stream_alloc(uint32_t header) goto error; }
- /* pass the IPC write posn pointer to the host */
- err = comp_cmd(pcm_dev->dev.cd, COMP_CMD_IPC_MMAP_WPOS,
&_stream_data->write_posn);- if (err < 0) {
trace_ipc_error("eAW");goto error;- }
- /* pass the volume readback posn to the host */ err = comp_cmd(volume_dev->cd, COMP_CMD_IPC_MMAP_VOL(0), &_stream_data->vol[0].vol);
@@ -711,9 +719,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)
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Wednesday, February 8, 2017 8:01 PM 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: [Sound-open-firmware] [PATCH 06/25] intel-ipc: add application buffer pointer update from host side
On Tue, 2017-02-07 at 22:02 +0800, Keyon Jie wrote:
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 add COMP_CMD_IPC_MMAP_WPOS for the application pointer
mmap
command, and 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 | 2 ++ src/include/uapi/intel-ipc.h | 1 + src/ipc/intel-ipc.c | 42 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+)
diff --git a/src/include/reef/audio/component.h b/src/include/reef/audio/component.h index e521753..fcef11e 100644 --- a/src/include/reef/audio/component.h +++ b/src/include/reef/audio/component.h @@ -72,10 +72,12 @@ #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
*/
#define COMP_CMD_IPC_MMAP_PPOS 201 /* DAI presentation
position */
+#define COMP_CMD_IPC_MMAP_WPOS 202 /* host write position */
Is the host writing this directly to DSP memory via mmap() or IPC. If IPC then use a different macro name.
It is IPC, let me change it to COMP_CMD_IPC_WPOS.
Thanks, ~Keyon
Liam
#define COMP_CMD_IPC_MMAP_VOL(chan) (216 + chan) /* Volume */
diff --git a/src/include/uapi/intel-ipc.h b/src/include/uapi/intel-ipc.h index a4589b3..ec2ef32 100644 --- a/src/include/uapi/intel-ipc.h +++ b/src/include/uapi/intel-ipc.h @@ -613,6 +613,7 @@ struct sst_intel_ipc_stream_vol { struct sst_intel_ipc_stream_data { uint32_t read_posn; uint64_t presentation_posn;
- uint32_t write_posn; struct sst_intel_ipc_stream_vol vol[IPC_INTEL_NO_CHANNELS]; }
__attribute__((packed)); #endif diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index 127541d..6084fa4 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -393,6 +393,14 @@ static uint32_t ipc_stream_alloc(uint32_t header) goto error; }
- /* pass the IPC write posn pointer to the host */
- err = comp_cmd(pcm_dev->dev.cd, COMP_CMD_IPC_MMAP_WPOS,
&_stream_data->write_posn);- if (err < 0) {
trace_ipc_error("eAW");goto error;- }
- /* pass the volume readback posn to the host */ err = comp_cmd(volume_dev->cd, COMP_CMD_IPC_MMAP_VOL(0), &_stream_data->vol[0].vol);
@@ -711,9 +719,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 ec2ef32..8dae038 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 6084fa4..c60f6c8 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -819,6 +819,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; @@ -913,6 +946,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..d6209c5 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 */ + volatile uint32_t *host_app_pos; /* host buffer app write pos, points to mailbox */ + volatile uint32_t host_avail; /* host buffer available size */ + volatile 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; }
On Tue, 2017-02-07 at 22:02 +0800, Keyon Jie wrote:
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..d6209c5 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 */
- volatile uint32_t *host_app_pos; /* host buffer app write pos, points to mailbox */
- volatile uint32_t host_avail; /* host buffer available size */
- volatile uint32_t host_free; /* host buffer free size */
Volatile only needed for members read back directly by host (with different physical caches).
Liam
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Wednesday, February 8, 2017 8:02 PM 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: [Sound-open-firmware] [PATCH 08/25] host: add host_update_buffer for produce and consume
On Tue, 2017-02-07 at 22:02 +0800, Keyon Jie wrote:
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..d6209c5 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 */
- volatile uint32_t *host_app_pos; /* host buffer app write pos,
points to mailbox */
- volatile uint32_t host_avail; /* host buffer available size */
- volatile uint32_t host_free; /* host buffer free size */
Volatile only needed for members read back directly by host (with different physical caches).
OK, let me remove volatile for host_avail and host_free.
Thanks, ~Keyon
Liam
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 d6209c5..083261c 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 */ volatile 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 083261c..2eaea6a 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 2eaea6a..806a505 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 806a505..3074c8e 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 | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 3074c8e..8e13391 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,16 @@ 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; + if (hd->host_app_pos) + *hd->host_app_pos = 0; + hd->host_app_pos = NULL; + 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_IPC_MMAP_WPOS and COMP_CMD_AVAIL_UPDATE.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 4 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 8e13391..4b60943 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,67 @@ 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; + if (hd->host_app_pos) + *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 +727,14 @@ 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_IPC_MMAP_WPOS: + hd->host_app_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)
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
- only pause at running stage;
- resetting buffer pointers and local_elem for stopping, to let next
starting begin from original position; 3. adding processes for COMP_CMD_IPC_MMAP_WPOS and COMP_CMD_AVAIL_UPDATE.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com
src/audio/host.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 4 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 8e13391..4b60943 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,67 @@ 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;- if (hd->host_app_pos)
*hd->host_app_pos = 0;- hd->host_pos_read = 0;
- hd->host_period_pos = 0;
- host_update_buffer_consume(hd);
Lets make this a seperate func for resetting pointers
- /* 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;
This looks like it can also be a new func that is shared with parts of init.
Liam
- 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;
/* 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);@@ -681,6 +727,14 @@ 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_IPC_MMAP_WPOS:
hd->host_app_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;
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Wednesday, February 8, 2017 8:07 PM 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: [Sound-open-firmware] [PATCH 14/25] host: cleanup host_cmd() cases
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
- only pause at running stage;
- resetting buffer pointers and local_elem for stopping, to let next
starting begin from original position; 3. adding processes for COMP_CMD_IPC_MMAP_WPOS and COMP_CMD_AVAIL_UPDATE.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com
src/audio/host.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 4 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 8e13391..4b60943 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,67 @@ 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;- if (hd->host_app_pos)
*hd->host_app_pos = 0;- hd->host_pos_read = 0;
- hd->host_period_pos = 0;
- host_update_buffer_consume(hd);
Lets make this a seperate func for resetting pointers
OK.
- /* 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;
This looks like it can also be a new func that is shared with parts of init.
OK, let me figure out one.
Thanks, ~Keyon
Liam
- 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;
/* 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);@@ -681,6 +727,14 @@ 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_IPC_MMAP_WPOS:
hd->host_app_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;
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 4b60943..0580837 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -809,14 +809,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) + 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..ddab23b 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 now */ + 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; }
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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..ddab23b 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 now */
/* 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;
}
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Wednesday, February 8, 2017 8:08 PM 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: [Sound-open-firmware] [PATCH 16/25] volume: volume_params(): set the sink buffer params depending on its sink
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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..ddab23b 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 now */
/* hard coded until new IPC is ready */
OK.
Thanks, ~Keyon
- if (sink->sink->is_host) {
sink_params.pcm.format = STREAM_FORMAT_S16_LE;sink_params.frame_size = 2 * params->channels; /* 16bitcontainer */
- } else if (sink->sink->is_dai) {
sink_params.pcm.format = PLATFORM_SSP_STREAM_FORMAT;sink_params.frame_size = 4 * params->channels; /* 32bitcontainer */
- } else {
sink_params.pcm.format = STREAM_FORMAT_S32_LE;sink_params.frame_size = 4 * params->channels; /* 32bitcontainer */
- }
- /* 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 | 4 ++-- 5 files changed, 6 insertions(+), 6 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 0580837..28b5e7b 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -689,7 +689,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 ddab23b..7a3f608 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 fcef11e..2e88f19 100644 --- a/src/include/reef/audio/component.h +++ b/src/include/reef/audio/component.h @@ -42,8 +42,8 @@
/* audio component states */ #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 */
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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()
Please add this flow as comments to the COMP_STATE_ macros
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 | 4 ++-- 5 files changed, 6 insertions(+), 6 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;diff --git a/src/audio/host.c b/src/audio/host.c index 0580837..28b5e7b 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -689,7 +689,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 ddab23b..7a3f608 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;diff --git a/src/include/reef/audio/component.h b/src/include/reef/audio/component.h index fcef11e..2e88f19 100644 --- a/src/include/reef/audio/component.h +++ b/src/include/reef/audio/component.h @@ -42,8 +42,8 @@
/* audio component states */ #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 */
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Wednesday, February 8, 2017 8:09 PM 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: [Sound-open-firmware] [PATCH 17/25] component: status: rename COMP_STATE_STOPPED to COMP_STATE_SETUP
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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()
Please add this flow as comments to the COMP_STATE_ macros
Okay, will do that in next version.
Thanks, ~Keyon
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 | 4 ++-- 5 files changed, 6 insertions(+), 6 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;diff --git a/src/audio/host.c b/src/audio/host.c index 0580837..28b5e7b 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -689,7 +689,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 downstreamcomponents */
}
diff --git a/src/audio/volume.c b/src/audio/volume.c index ddab23b..7a3f608 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;diff --git a/src/include/reef/audio/component.h b/src/include/reef/audio/component.h index fcef11e..2e88f19 100644 --- a/src/include/reef/audio/component.h +++ b/src/include/reef/audio/component.h @@ -42,8 +42,8 @@
/* audio component states */ #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 7a3f608..eb1530f 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;
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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 7a3f608..eb1530f 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;
indentation
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;
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of Liam Girdwood Sent: Wednesday, February 8, 2017 8:10 PM To: Keyon Jie yang.jie@linux.intel.com Cc: Jie, Yang yang.jie@intel.com; Ingalsuo, Seppo seppo.ingalsuo@intel.com; sound-open-firmware@alsa-project.org Subject: Re: [Sound-open-firmware] [PATCH 18/25] volume: volume_cmd(): only stop/pause at running
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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 7a3f608..eb1530f 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;indentation
Do you mean changing like this:
case COMP_CMD_STOP: if (dev->state == COMP_STATE_RUNNING || dev->state == COMP_STATE_DRAINING || dev->state == COMP_STATE_PAUSED) dev->state = COMP_STATE_SETUP;
Thanks, ~Keyon
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;
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Thu, 2017-02-09 at 02:21 +0000, Jie, Yang wrote:
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of Liam Girdwood Sent: Wednesday, February 8, 2017 8:10 PM To: Keyon Jie yang.jie@linux.intel.com Cc: Jie, Yang yang.jie@intel.com; Ingalsuo, Seppo seppo.ingalsuo@intel.com; sound-open-firmware@alsa-project.org Subject: Re: [Sound-open-firmware] [PATCH 18/25] volume: volume_cmd(): only stop/pause at running
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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 7a3f608..eb1530f 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;indentation
Do you mean changing like this:
case COMP_CMD_STOP: if (dev->state == COMP_STATE_RUNNING || dev->state == COMP_STATE_DRAINING || dev->state == COMP_STATE_PAUSED) dev->state = COMP_STATE_SETUP;
To be honest tabs alone don't make following if statements like this easy. Sometimes you need to add some spaces to some of the conditions to get alignment :-
if (x == 1 || y == 2 || z == 0) state = a;
Liam
Thanks, ~Keyon
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;
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
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 eb1530f..a226334 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..2d021b7 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, mixing_num, 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; }
+ mixing_num = i; + sink = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);
+ for(i = 0; i < mixing_num; 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 (mixing_num == 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 = mixing_num; 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; }
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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..2d021b7 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, mixing_num, 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; }
- mixing_num = i;
number of sources or frames ?
num_mix_sources or num_mix_frames.
sink = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);
for(i = 0; i < mixing_num; 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 (mixing_num == 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 = mixing_num; 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;
}
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Wednesday, February 8, 2017 8:12 PM 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: [Sound-open-firmware] [PATCH 21/25] mixer: add R/W pointer check for mixer_copy()
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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..2d021b7 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, mixing_num, 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; }
- mixing_num = i;
number of sources or frames ?
num_mix_sources or num_mix_frames.
OK, let me change it to num_mix_sources
Thanks, ~Keyon
- sink = list_first_item(&dev->bsink_list, struct comp_buffer,
source_list);
- for(i = 0; i < mixing_num; 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 (mixing_num == 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 = mixing_num; 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 | 44 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/src/audio/dai.c b/src/audio/dai.c index 3b2ea30..a9b46a5 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,36 @@ 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 = 0xFFFFFFFF; + /* 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,
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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 | 44 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/src/audio/dai.c b/src/audio/dai.c index 3b2ea30..a9b46a5 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;@@ -127,9 +134,36 @@ 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);
newline
/* stop dma immediatly */next->size = 0xFFFFFFFF;
magic number
/* 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,
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Wednesday, February 8, 2017 8:14 PM 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: [Sound-open-firmware] [PATCH 23/25] dai: add R/W pointer check for dai copy
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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 | 44 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/src/audio/dai.c b/src/audio/dai.c index 3b2ea30..a9b46a5 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;127,9 +134,36 @@ 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);newline
OK.
/* stop dma immediatly */next->size = 0xFFFFFFFF;magic number
It is a little strange that this is rolled back to non-magic number version. Will fix it.
Thanks, ~Keyon
/* 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 0xFFFFFFFF, stop this dma copy; 2. if next.size > 0 but not 0xFFFFFFFF, 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 | 25 ++++++++++++++----------- src/include/reef/dma.h | 6 ++++++ 3 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/src/audio/dai.c b/src/audio/dai.c index a9b46a5..e365ed9 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -140,7 +140,7 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) if (dma_buffer->avail == 0) { dai_cmd(dev, COMP_CMD_STOP, NULL); /* stop dma immediatly */ - next->size = 0xFFFFFFFF; + next->size = DMA_RELOAD_STOP_SIZE; /* let any waiters know we have completed */ wait_completed(&dev->pipeline->complete); return; diff --git a/src/drivers/dw-dma.c b/src/drivers/dw-dma.c index c073ed5..baa773e 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,26 @@ 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; 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_STOP_SIZE, stop this dma copy; + * next.size > 0 but not DMA_RELOAD_STOP_SIZE, use next + * element for next copy; + * if we are waiting for pause, pause it; + * otherwise, reload lli + */ + if (next.size == DMA_RELOAD_STOP_SIZE) + continue; + else if (next.size > 0) dw_dma_chan_reload_next(dma, i, &next); - else + 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..273371e 100644 --- a/src/include/reef/dma.h +++ b/src/include/reef/dma.h @@ -59,6 +59,12 @@ #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_STOP_SIZE 0xFFFFFFFF + struct dma;
struct dma_sg_elem {
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
After finished a transfer copy, check for reload channel:
- if next.size is 0xFFFFFFFF, stop this dma copy;
- if next.size > 0 but not 0xFFFFFFFF, use next element
magic numbers.
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 | 25 ++++++++++++++----------- src/include/reef/dma.h | 6 ++++++ 3 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/src/audio/dai.c b/src/audio/dai.c index a9b46a5..e365ed9 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -140,7 +140,7 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) if (dma_buffer->avail == 0) { dai_cmd(dev, COMP_CMD_STOP, NULL); /* stop dma immediatly */
next->size = 0xFFFFFFFF;
next->size = DMA_RELOAD_STOP_SIZE; /* let any waiters know we have completed */ wait_completed(&dev->pipeline->complete); return;diff --git a/src/drivers/dw-dma.c b/src/drivers/dw-dma.c index c073ed5..baa773e 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);@@ -834,20 +831,26 @@ 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; 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_STOP_SIZE, stop this dma copy;* next.size > 0 but not DMA_RELOAD_STOP_SIZE, use next* element for next copy;* if we are waiting for pause, pause it;* otherwise, reload lli*/if (next.size == DMA_RELOAD_STOP_SIZE)continue;else if (next.size > 0) dw_dma_chan_reload_next(dma, i, &next);
else
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..273371e 100644 --- a/src/include/reef/dma.h +++ b/src/include/reef/dma.h @@ -59,6 +59,12 @@ #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_STOP_SIZE 0xFFFFFFFF
if we are using the size variable then it makes more sense to use 0 here to indicate end of transfer.
#define DMA_RELOAD_END 0
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 | 18 ++++++++++++++++++ src/audio/mixer.c | 31 ++++++++++++++++++++++++++++++- src/audio/volume.c | 27 ++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 2 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 28b5e7b..07f4953 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -656,6 +656,7 @@ 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; + struct list_item *clist;
/* reset buffer pointers */ if (hd->host_pos) @@ -689,6 +690,23 @@ static int host_stop(struct comp_dev *dev) local_elem->src = source_elem->src; hd->next_inc = hd->period->size;
+ /* now reset downstream buffer */ + 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; + } + dev->state = COMP_STATE_SETUP; return 0; } diff --git a/src/audio/mixer.c b/src/audio/mixer.c index 2d021b7..4dccf7a 100644 --- a/src/audio/mixer.c +++ b/src/audio/mixer.c @@ -175,6 +175,31 @@ static struct comp_dev* mixer_volume_component(struct comp_dev *mixer) return comp_dev; }
+static int mixer_stop(struct comp_dev *dev) +{ + struct list_item *clist; + + /* now reset downstream buffer */ + 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; + } + + dev->state = COMP_STATE_SETUP; + return 0; +} + /* used to pass standard and bespoke commands (with data) to component */ static int mixer_cmd(struct comp_dev *dev, int cmd, void *data) { @@ -184,7 +209,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 +216,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) + mixer_stop(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 a226334..032e78b 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -389,6 +389,31 @@ static inline void volume_set_chan_unmute(struct comp_dev *dev, int chan) cd->tvolume[chan] = cd->mvolume[chan]; }
+static int volume_stop(struct comp_dev *dev) +{ + struct list_item *clist; + + /* now reset downstream buffer */ + 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; + } + + dev->state = COMP_STATE_SETUP; + return 0; +} + /* used to pass standard and bespoke commands (with data) to component */ static int volume_cmd(struct comp_dev *dev, int cmd, void *data) { @@ -432,7 +457,7 @@ static int volume_cmd(struct comp_dev *dev, int cmd, void *data) if (dev->state == COMP_STATE_RUNNING || dev->state == COMP_STATE_DRAINING || dev->state == COMP_STATE_PAUSED) - dev->state = COMP_STATE_SETUP; + volume_stop(dev); break; case COMP_CMD_PAUSE: /* only support pausing for running */
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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 | 18 ++++++++++++++++++ src/audio/mixer.c | 31 ++++++++++++++++++++++++++++++- src/audio/volume.c | 27 ++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 2 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 28b5e7b..07f4953 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -656,6 +656,7 @@ 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;
struct list_item *clist;
/* reset buffer pointers */ if (hd->host_pos)
@@ -689,6 +690,23 @@ static int host_stop(struct comp_dev *dev) local_elem->src = source_elem->src; hd->next_inc = hd->period->size;
- /* now reset downstream buffer */
- 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;
I'm seeing this reset code duplicated in a lot of places. lets create a new component API call that does this reset.
- }
- dev->state = COMP_STATE_SETUP; return 0;
} diff --git a/src/audio/mixer.c b/src/audio/mixer.c index 2d021b7..4dccf7a 100644 --- a/src/audio/mixer.c +++ b/src/audio/mixer.c @@ -175,6 +175,31 @@ static struct comp_dev* mixer_volume_component(struct comp_dev *mixer) return comp_dev; }
+static int mixer_stop(struct comp_dev *dev) +{
- struct list_item *clist;
- /* now reset downstream buffer */
- 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;- }
- dev->state = COMP_STATE_SETUP;
- return 0;
+}
/* used to pass standard and bespoke commands (with data) to component */ static int mixer_cmd(struct comp_dev *dev, int cmd, void *data) { @@ -184,7 +209,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 +216,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)mixer_stop(dev);break;diff --git a/src/audio/volume.c b/src/audio/volume.c index a226334..032e78b 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -389,6 +389,31 @@ static inline void volume_set_chan_unmute(struct comp_dev *dev, int chan) cd->tvolume[chan] = cd->mvolume[chan]; }
+static int volume_stop(struct comp_dev *dev) +{
- struct list_item *clist;
- /* now reset downstream buffer */
- 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;- }
- dev->state = COMP_STATE_SETUP;
- return 0;
+}
/* used to pass standard and bespoke commands (with data) to component */ static int volume_cmd(struct comp_dev *dev, int cmd, void *data) { @@ -432,7 +457,7 @@ static int volume_cmd(struct comp_dev *dev, int cmd, void *data) if (dev->state == COMP_STATE_RUNNING || dev->state == COMP_STATE_DRAINING || dev->state == COMP_STATE_PAUSED)
dev->state = COMP_STATE_SETUP;
volume_stop(dev);
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Wednesday, February 8, 2017 8:17 PM 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: [Sound-open-firmware] [PATCH 25/25] component: host/volume/mixer: reset buffers for stop cmd
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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 | 18 ++++++++++++++++++ src/audio/mixer.c | 31 ++++++++++++++++++++++++++++++- src/audio/volume.c | 27 ++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 2 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 28b5e7b..07f4953 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -656,6 +656,7 @@ 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;
struct list_item *clist;
/* reset buffer pointers */ if (hd->host_pos)
@@ -689,6 +690,23 @@ static int host_stop(struct comp_dev *dev) local_elem->src = source_elem->src; hd->next_inc = hd->period->size;
- /* now reset downstream buffer */
- 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;I'm seeing this reset code duplicated in a lot of places. lets create a new component API call that does this reset.
OK, I will create inline one comp_buffer_stop() and put it component.h?
Thanks, ~Keyon
- }
- dev->state = COMP_STATE_SETUP; return 0;
} diff --git a/src/audio/mixer.c b/src/audio/mixer.c index 2d021b7..4dccf7a 100644 --- a/src/audio/mixer.c +++ b/src/audio/mixer.c @@ -175,6 +175,31 @@ static struct comp_dev*
mixer_volume_component(struct comp_dev *mixer)
return comp_dev; }
+static int mixer_stop(struct comp_dev *dev) {
- struct list_item *clist;
- /* now reset downstream buffer */
- 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;- }
- dev->state = COMP_STATE_SETUP;
- return 0;
+}
/* used to pass standard and bespoke commands (with data) to component */ static int mixer_cmd(struct comp_dev *dev, int cmd, void *data) { @@ -184,7 +209,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 +216,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)mixer_stop(dev);break;diff --git a/src/audio/volume.c b/src/audio/volume.c index a226334..032e78b 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -389,6 +389,31 @@ static inline void volume_set_chan_unmute(struct
comp_dev *dev, int chan)
cd->tvolume[chan] = cd->mvolume[chan]; }
+static int volume_stop(struct comp_dev *dev) {
- struct list_item *clist;
- /* now reset downstream buffer */
- 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;- }
- dev->state = COMP_STATE_SETUP;
- return 0;
+}
/* used to pass standard and bespoke commands (with data) to component */ static int volume_cmd(struct comp_dev *dev, int cmd, void *data) { @@ -432,7 +457,7 @@ static int volume_cmd(struct comp_dev *dev, int cmd, void *data) if (dev->state == COMP_STATE_RUNNING || dev->state == COMP_STATE_DRAINING || dev->state == COMP_STATE_PAUSED)
dev->state = COMP_STATE_SETUP;
volume_stop(dev);
On Thu, 2017-02-09 at 02:37 +0000, Jie, Yang wrote:
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Wednesday, February 8, 2017 8:17 PM 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: [Sound-open-firmware] [PATCH 25/25] component: host/volume/mixer: reset buffers for stop cmd
On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
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 | 18 ++++++++++++++++++ src/audio/mixer.c | 31 ++++++++++++++++++++++++++++++- src/audio/volume.c | 27 ++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 2 deletions(-)
diff --git a/src/audio/host.c b/src/audio/host.c index 28b5e7b..07f4953 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -656,6 +656,7 @@ 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;
struct list_item *clist;
/* reset buffer pointers */ if (hd->host_pos)
@@ -689,6 +690,23 @@ static int host_stop(struct comp_dev *dev) local_elem->src = source_elem->src; hd->next_inc = hd->period->size;
- /* now reset downstream buffer */
- 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;I'm seeing this reset code duplicated in a lot of places. lets create a new component API call that does this reset.
OK, I will create inline one comp_buffer_stop() and put it component.h?
Lets call it comp_buffer_reset()
It should contain :-
/* 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;
i.e. it clears buffer and resets pointers.
Liam
Thanks, ~Keyon
- }
- dev->state = COMP_STATE_SETUP; return 0;
} diff --git a/src/audio/mixer.c b/src/audio/mixer.c index 2d021b7..4dccf7a 100644 --- a/src/audio/mixer.c +++ b/src/audio/mixer.c @@ -175,6 +175,31 @@ static struct comp_dev*
mixer_volume_component(struct comp_dev *mixer)
return comp_dev; }
+static int mixer_stop(struct comp_dev *dev) {
- struct list_item *clist;
- /* now reset downstream buffer */
- 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;- }
- dev->state = COMP_STATE_SETUP;
- return 0;
+}
/* used to pass standard and bespoke commands (with data) to component */ static int mixer_cmd(struct comp_dev *dev, int cmd, void *data) { @@ -184,7 +209,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 +216,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)mixer_stop(dev);break;diff --git a/src/audio/volume.c b/src/audio/volume.c index a226334..032e78b 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -389,6 +389,31 @@ static inline void volume_set_chan_unmute(struct
comp_dev *dev, int chan)
cd->tvolume[chan] = cd->mvolume[chan]; }
+static int volume_stop(struct comp_dev *dev) {
- struct list_item *clist;
- /* now reset downstream buffer */
- 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;- }
- dev->state = COMP_STATE_SETUP;
- return 0;
+}
/* used to pass standard and bespoke commands (with data) to component */ static int volume_cmd(struct comp_dev *dev, int cmd, void *data) { @@ -432,7 +457,7 @@ static int volume_cmd(struct comp_dev *dev, int cmd, void *data) if (dev->state == COMP_STATE_RUNNING || dev->state == COMP_STATE_DRAINING || dev->state == COMP_STATE_PAUSED)
dev->state = COMP_STATE_SETUP;
volume_stop(dev);
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (3)
-
Jie, Yang -
Keyon Jie -
Liam Girdwood