[Sound-open-firmware] [RFC/RFT] dai: Make system stable by moving DAI buffer pointer update location
From: Wu Zhigang zhigang.wu@linux.intel.com
Move the DAI's buffer pointer update location from dai_dma_cb() to dai_copy(). This will make pipeline more stable. The xrun possibility will be lower.Even in xrun it will recover, it will not have dsp oops. DAI's buffer pointer will be updated in the pipeline copy sequence as other components did. Then the DMA will not update the buffer pointer by itself. in HOST--->VOL--->DAI pipeline, when DMA finished, it has to reschedule the pipeline to let the pointer update. this could avoid the xrun when adjust the read and write pointers by different componets.
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com Tested-by: Keyon Jie yang.jie@linux.intel.com --- Tested on APL-GPMRB kernel: sof-v4.14 commit <c33cfed51fff5eb156dcf70ba849e6ffb3008377> sof: 1.1-stable commit <949de92142aa9cdd4a31fda68f05d29212683cc1> tools: 1.1-stable commit <b799e3fa3bab7f5df70f2458bda42de167e65fb3>
RFT: can someone help test it on byt also?
src/audio/dai.c | 108 +++++++++++++++++++++++++++++--------------------------- 1 file changed, 56 insertions(+), 52 deletions(-)
diff --git a/src/audio/dai.c b/src/audio/dai.c index e1ea9ab..6d0c1fa 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -78,7 +78,6 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) struct comp_dev *dev = (struct comp_dev *)data; struct dai_data *dd = comp_get_drvdata(dev); struct comp_buffer *dma_buffer; - uint32_t copied_size;
trace_dai("irq");
@@ -114,57 +113,6 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) return; }
- if (dev->params.direction == SOF_IPC_STREAM_PLAYBACK) { - dma_buffer = list_first_item(&dev->bsource_list, - struct comp_buffer, sink_list); - - copied_size = dd->last_bytes ? dd->last_bytes : dd->period_bytes; - - /* recalc available buffer space */ - comp_update_buffer_consume(dma_buffer, copied_size); - - /* writeback buffer contents from cache */ - dcache_writeback_region(dma_buffer->r_ptr, copied_size); - - /* update host position(in bytes offset) for drivers */ - dev->position += copied_size; - if (dd->dai_pos) { - dd->dai_pos_blks += copied_size; - *dd->dai_pos = dd->dai_pos_blks + - dma_buffer->r_ptr - dma_buffer->addr; - } - - /* make sure there is availble bytes for next period */ - if (dma_buffer->avail < dd->period_bytes) { - trace_dai_error("xru"); - comp_underrun(dev, dma_buffer, copied_size, 0); - } - - } else { - dma_buffer = list_first_item(&dev->bsink_list, - struct comp_buffer, source_list); - - /* invalidate buffer contents */ - dcache_invalidate_region(dma_buffer->w_ptr, dd->period_bytes); - - /* recalc available buffer space */ - comp_update_buffer_produce(dma_buffer, dd->period_bytes); - - /* update positions */ - dev->position += dd->period_bytes; - if (dd->dai_pos) { - dd->dai_pos_blks += dd->period_bytes; - *dd->dai_pos = dd->dai_pos_blks + - dma_buffer->w_ptr - dma_buffer->addr; - } - - /* make sure there is free bytes for next period */ - if (dma_buffer->free < dd->period_bytes) { - trace_dai_error("xro"); - comp_overrun(dev, dma_buffer, dd->period_bytes, 0); - } - } - /* notify pipeline that DAI needs its buffer processed */ if (dev->state == COMP_STATE_ACTIVE) pipeline_schedule_copy(dev->pipeline, 0); @@ -607,6 +555,62 @@ static int dai_comp_trigger(struct comp_dev *dev, int cmd) /* copy and process stream data from source to sink buffers */ static int dai_copy(struct comp_dev *dev) { + struct dai_data *dd = comp_get_drvdata(dev); + struct comp_buffer *dma_buffer; + uint32_t copied_size; + + if (dev->params.direction == SOF_IPC_STREAM_PLAYBACK) { + dma_buffer = list_first_item(&dev->bsource_list, + struct comp_buffer, sink_list); + + copied_size = dd->last_bytes ? + dd->last_bytes : dd->period_bytes; + + /* recalc available buffer space */ + comp_update_buffer_consume(dma_buffer, copied_size); + + /* writeback buffer contents from cache */ + dcache_writeback_region(dma_buffer->r_ptr, copied_size); + + /* update host position(in bytes offset) for drivers */ + dev->position += copied_size; + if (dd->dai_pos) { + dd->dai_pos_blks += copied_size; + *dd->dai_pos = dd->dai_pos_blks + + dma_buffer->r_ptr - dma_buffer->addr; + } + + /* make sure there is available bytes for next period */ + if (dma_buffer->avail < dd->period_bytes) { + trace_dai_error("xru"); + comp_underrun(dev, dma_buffer, copied_size, 0); + } + + } else { + dma_buffer = list_first_item(&dev->bsink_list, + struct comp_buffer, source_list); + + /* invalidate buffer contents */ + dcache_invalidate_region(dma_buffer->w_ptr, dd->period_bytes); + + /* recalc available buffer space */ + comp_update_buffer_produce(dma_buffer, dd->period_bytes); + + /* update positions */ + dev->position += dd->period_bytes; + if (dd->dai_pos) { + dd->dai_pos_blks += dd->period_bytes; + *dd->dai_pos = dd->dai_pos_blks + + dma_buffer->w_ptr - dma_buffer->addr; + } + + /* make sure there is free bytes for next period */ + if (dma_buffer->free < dd->period_bytes) { + trace_dai_error("xro"); + comp_overrun(dev, dma_buffer, dd->period_bytes, 0); + } + } + return 0; }
On Sat, 2018-03-31 at 00:31 +0800, Keyon Jie wrote:
From: Wu Zhigang zhigang.wu@linux.intel.com
Move the DAI's buffer pointer update location from dai_dma_cb() to dai_copy(). This will make pipeline more stable. The xrun possibility will be lower.Even in xrun it will recover, it will not have dsp oops. DAI's buffer pointer will be updated in the pipeline copy sequence as other components did. Then the DMA will not update the buffer pointer by itself. in HOST--->VOL--->DAI pipeline, when DMA finished, it has to reschedule the pipeline to let the pointer update. this could avoid the xrun when adjust the read and write pointers by different componets.
I may be wrong here but how do we make sure we are always in sync with the clock rate in this case?
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com Tested-by: Keyon Jie yang.jie@linux.intel.com
Tested on APL-GPMRB kernel: sof-v4.14 commit <c33cfed51fff5eb156dcf70ba849e6ffb3008377> sof: 1.1-stable commit <949de92142aa9cdd4a31fda68f05d29212683cc1> tools: 1.1-stable commit <b799e3fa3bab7f5df70f2458bda42de167e65fb3>
RFT: can someone help test it on byt also?
src/audio/dai.c | 108 +++++++++++++++++++++++++++++-----------------
1 file changed, 56 insertions(+), 52 deletions(-)
diff --git a/src/audio/dai.c b/src/audio/dai.c index e1ea9ab..6d0c1fa 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -78,7 +78,6 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) struct comp_dev *dev = (struct comp_dev *)data; struct dai_data *dd = comp_get_drvdata(dev); struct comp_buffer *dma_buffer;
uint32_t copied_size;
trace_dai("irq");
@@ -114,57 +113,6 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) return; }
- if (dev->params.direction == SOF_IPC_STREAM_PLAYBACK) {
dma_buffer = list_first_item(&dev->bsource_list,struct comp_buffer, sink_list);copied_size = dd->last_bytes ? dd->last_bytes : dd-period_bytes;
/* recalc available buffer space */comp_update_buffer_consume(dma_buffer, copied_size);/* writeback buffer contents from cache */dcache_writeback_region(dma_buffer->r_ptr,copied_size);
/* update host position(in bytes offset) for drivers*/
dev->position += copied_size;if (dd->dai_pos) {dd->dai_pos_blks += copied_size;*dd->dai_pos = dd->dai_pos_blks +dma_buffer->r_ptr - dma_buffer-addr;
}/* make sure there is availble bytes for next period*/
if (dma_buffer->avail < dd->period_bytes) {trace_dai_error("xru");comp_underrun(dev, dma_buffer, copied_size,0);
}- } else {
dma_buffer = list_first_item(&dev->bsink_list,struct comp_buffer, source_list);/* invalidate buffer contents */dcache_invalidate_region(dma_buffer->w_ptr, dd-period_bytes);
/* recalc available buffer space */comp_update_buffer_produce(dma_buffer, dd-period_bytes);
/* update positions */dev->position += dd->period_bytes;if (dd->dai_pos) {dd->dai_pos_blks += dd->period_bytes;*dd->dai_pos = dd->dai_pos_blks +dma_buffer->w_ptr - dma_buffer-addr;
}/* make sure there is free bytes for next period */if (dma_buffer->free < dd->period_bytes) {trace_dai_error("xro");comp_overrun(dev, dma_buffer, dd-period_bytes, 0);
}- }
- /* notify pipeline that DAI needs its buffer processed */ if (dev->state == COMP_STATE_ACTIVE) pipeline_schedule_copy(dev->pipeline, 0);
@@ -607,6 +555,62 @@ static int dai_comp_trigger(struct comp_dev *dev, int cmd) /* copy and process stream data from source to sink buffers */ static int dai_copy(struct comp_dev *dev) {
- struct dai_data *dd = comp_get_drvdata(dev);
- struct comp_buffer *dma_buffer;
- uint32_t copied_size;
- if (dev->params.direction == SOF_IPC_STREAM_PLAYBACK) {
dma_buffer = list_first_item(&dev->bsource_list,struct comp_buffer,sink_list);
copied_size = dd->last_bytes ?dd->last_bytes : dd->period_bytes;/* recalc available buffer space */comp_update_buffer_consume(dma_buffer, copied_size);/* writeback buffer contents from cache */dcache_writeback_region(dma_buffer->r_ptr,copied_size);
/* update host position(in bytes offset) for drivers*/
dev->position += copied_size;if (dd->dai_pos) {dd->dai_pos_blks += copied_size;*dd->dai_pos = dd->dai_pos_blks +dma_buffer->r_ptr - dma_buffer-addr;
}/* make sure there is available bytes for nextperiod */
if (dma_buffer->avail < dd->period_bytes) {trace_dai_error("xru");comp_underrun(dev, dma_buffer, copied_size,0);
}- } else {
dma_buffer = list_first_item(&dev->bsink_list,struct comp_buffer,source_list);
/* invalidate buffer contents */dcache_invalidate_region(dma_buffer->w_ptr, dd-period_bytes);
/* recalc available buffer space */comp_update_buffer_produce(dma_buffer, dd-period_bytes);
/* update positions */dev->position += dd->period_bytes;if (dd->dai_pos) {dd->dai_pos_blks += dd->period_bytes;*dd->dai_pos = dd->dai_pos_blks +dma_buffer->w_ptr - dma_buffer-addr;
}/* make sure there is free bytes for next period */if (dma_buffer->free < dd->period_bytes) {trace_dai_error("xro");comp_overrun(dev, dma_buffer, dd-period_bytes, 0);
}- }
- return 0;
}
On Fri, 2018-03-30 at 09:42 -0700, Ranjani Sridharan wrote:
On Sat, 2018-03-31 at 00:31 +0800, Keyon Jie wrote:
From: Wu Zhigang zhigang.wu@linux.intel.com
Move the DAI's buffer pointer update location from dai_dma_cb() to dai_copy(). This will make pipeline more stable. The xrun possibility will be lower.Even in xrun it will recover, it will not have dsp oops. DAI's buffer pointer will be updated in the pipeline copy sequence as other components did. Then the DMA will not update the buffer pointer by itself. in HOST--->VOL--->DAI pipeline, when DMA finished, it has to reschedule the pipeline to let the pointer update. this could avoid the xrun when adjust the read and write pointers by different componets.
I may be wrong here but how do we make sure we are always in sync with the clock rate in this case?
Pipeline scheduling can either based on DMA IRQ or an internal timer. In this case the r/w pointer updates are aligned with the rest of the pipeline updates and are scheduled by the DMA IRQ.
Liam
On Sat, 2018-03-31 at 00:31 +0800, Keyon Jie wrote:
From: Wu Zhigang zhigang.wu@linux.intel.com
Move the DAI's buffer pointer update location from dai_dma_cb() to dai_copy(). This will make pipeline more stable. The xrun possibility will be lower.Even in xrun it will recover, it will not have dsp oops. DAI's buffer pointer will be updated in the pipeline copy sequence as other components did. Then the DMA will not update the buffer pointer by itself. in HOST--->VOL--->DAI pipeline, when DMA finished, it has to reschedule the pipeline to let the pointer update. this could avoid the xrun when adjust the read and write pointers by different componets.
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com Tested-by: Keyon Jie yang.jie@linux.intel.com
The $SUBJECT and commit message are a bit misleading as this is really fixing a race we have with our xrun handling. I've rewritten and applied.
Thanks
Liam
participants (3)
-
Keyon Jie -
Liam Girdwood -
Ranjani Sridharan