[Sound-open-firmware] [PATCH 0/5] Fix capture doesn't work issue

newer
[Sound-open-firmware] [PATCH v2...

Keyon Jie

15 Feb 2017 15 Feb '17

11:07 a.m.

This small series fix the capture doesn't work after W/R pointer check implemented: 1. Make sure the component .params() works for capture; 2. Fix host side buffer for capture; 3. rename host side buffer pointer to make it more understandable for capture also. 4. integrate produce and consume version host_update_buffer to fix capture issue.

Keyon Jie (5): component: set source buffer params for capture component .params() host: only update host_avail and host_free in host_update_buffer_xx() host: host_copy(): add handle for capture host: rename host_pos_read to host_pos_local and update comments host: integrate consume and produce to host_update_buffer and fix for capture

src/audio/host.c | 94 ++++++++++++++++++++++---------------- src/audio/volume.c | 39 ++++++++++------ src/include/reef/audio/component.h | 13 ++++++ 3 files changed, 92 insertions(+), 54 deletions(-)

-- 2.7.4

Show replies by date

Keyon Jie

15 Feb 15 Feb

11:07 a.m.

New subject: [Sound-open-firmware] [PATCH 1/5] component: set source buffer params for capture component .params()

Considering of capture pipeline, when doing .params(), we need set params to each component's source buffer.

This patch implement that for host/volume component, as mixer is only for playback ATM, and dai don't have source buffer for capture, we don't need change them ATM.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 7 ++++++- src/audio/volume.c | 39 ++++++++++++++++++++++++-------------- src/include/reef/audio/component.h | 13 +++++++++++++ 3 files changed, 44 insertions(+), 15 deletions(-)

diff --git a/src/audio/host.c b/src/audio/host.c index 069c6a2..12aee59 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -535,10 +535,12 @@ static int host_params(struct comp_dev *dev, struct stream_params *params)

/* set params */ hd->params = *params; - comp_set_sink_params(dev, params);

/* determine source and sink buffer elems */ if (params->direction == STREAM_DIRECTION_PLAYBACK) { + /* set sink buffer params */ + comp_set_sink_params(dev, params); + hd->source = &hd->host; hd->sink = &hd->local; hd->dma_buffer = list_first_item(&dev->bsink_list, @@ -546,6 +548,9 @@ static int host_params(struct comp_dev *dev, struct stream_params *params) hd->period = &hd->dma_buffer->desc.source_period; config->direction = DMA_DIR_HMEM_TO_LMEM; } else { + /* set source buffer params */ + comp_set_source_params(dev, params); + hd->source = &hd->local; hd->sink = &hd->host; hd->dma_buffer = list_first_item(&dev->bsource_list, diff --git a/src/audio/volume.c b/src/audio/volume.c index 1eb6004..bdaf87f 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -341,26 +341,37 @@ 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); + struct stream_params buffer_params = *params; + struct comp_buffer *next_buf; + struct comp_dev *next_dev; + + if (params->direction == STREAM_DIRECTION_PLAYBACK) { + /* volume components will only ever have 1 sink buffer */ + next_buf = list_first_item(&dev->bsink_list, struct comp_buffer, source_list); + next_dev = next_buf->sink; + } else { + /* volume components will only ever have 1 source buffer */ + next_buf = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list); + next_dev = next_buf->source; + }

/* 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 */ + if (next_dev->is_host) { + buffer_params.pcm.format = STREAM_FORMAT_S16_LE; + buffer_params.frame_size = 2 * params->channels; /* 16bit container */ + } else if (next_dev->is_dai) { + buffer_params.pcm.format = PLATFORM_SSP_STREAM_FORMAT; + buffer_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 */ + buffer_params.pcm.format = STREAM_FORMAT_S32_LE; + buffer_params.frame_size = 4 * params->channels; /* 32bit container */ }

/* dont do any data transformation */ - comp_set_sink_params(dev, &sink_params); + if (params->direction == STREAM_DIRECTION_PLAYBACK) + comp_set_sink_params(dev, &buffer_params); + else + comp_set_source_params(dev, &buffer_params);

return 0; } diff --git a/src/include/reef/audio/component.h b/src/include/reef/audio/component.h index 1e784f5..be45d81 100644 --- a/src/include/reef/audio/component.h +++ b/src/include/reef/audio/component.h @@ -420,4 +420,17 @@ static inline void comp_set_sink_params(struct comp_dev *dev, } }

+static inline void comp_set_source_params(struct comp_dev *dev, + struct stream_params *params) +{ + struct list_item *clist; + struct comp_buffer *source; + + list_for_item(clist, &dev->bsource_list) { + + source = container_of(clist, struct comp_buffer, sink_list); + source->params = *params; + } +} + #endif

-- 2.7.4

Keyon Jie

11:07 a.m.

New subject: [Sound-open-firmware] [PATCH 2/5] host: only update host_avail and host_free in host_update_buffer_xx()

We should not modify host_avail or host_free directly, and need replace that with host_update_buffer_produce/consume().

Theoretically, each time we update host_app_pos, host_pos_read, or host_size, we should call host_update_buffer_xx() to update the buffer.

Here we add update to buffer new, host prepare, AVAIL_UPDATE cmd, and pointer_reset.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/audio/host.c b/src/audio/host.c index 12aee59..b604ddc 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -638,6 +638,8 @@ static int host_prepare(struct comp_dev *dev) hd->params.period_frames * hd->params.frame_size; hd->split_remaining = 0;

+ host_update_buffer_consume(hd); + dev->preload = PLAT_HOST_PERIODS;

dev->state = COMP_STATE_PREPARE; @@ -674,8 +676,8 @@ static int host_pointer_reset(struct comp_dev *dev) hd->host_app_pos = 0; hd->host_pos_read = 0; hd->host_period_pos = 0; - hd->host_size = 0; - hd->host_avail= 0; + + host_update_buffer_consume(hd);

return 0; } @@ -732,7 +734,11 @@ static int host_cmd(struct comp_dev *dev, int cmd, void *data) 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); + + if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) + host_update_buffer_produce(hd); + else + host_update_buffer_consume(hd); break; case COMP_CMD_VOLUME: vol_dev = host_volume_component(dev); @@ -760,6 +766,8 @@ static int host_buffer(struct comp_dev *dev, struct dma_sg_elem *elem, *e = *elem; hd->host_size = host_size;

+ host_update_buffer_consume(hd); + list_item_append(&e->list, &hd->host.elem_list); return 0; }

-- 2.7.4

Keyon Jie

11:07 a.m.

New subject: [Sound-open-firmware] [PATCH 3/5] host: host_copy(): add handle for capture

For playback, don't copy if there is no available data; For capture, don't copy if there is no free host side buffer.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/audio/host.c b/src/audio/host.c index b604ddc..61d2b62 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -815,7 +815,10 @@ static int host_copy(struct comp_dev *dev) if (dev->state != COMP_STATE_RUNNING) return 0;

- if (hd->host_avail == 0) + /* don't copy if the host side buffer is not ready */ + if (((hd->params.direction == STREAM_DIRECTION_PLAYBACK) + && (hd->host_avail == 0)) || ((hd->host_free == 0) && + (hd->params.direction == STREAM_DIRECTION_CAPTURE))) return 0;

/* do DMA transfer */

-- 2.7.4

Liam Girdwood

11:33 a.m.

New subject: [Sound-open-firmware] [PATCH 3/5] host: host_copy(): add handle for capture

On Wed, 2017-02-15 at 18:07 +0800, Keyon Jie wrote:

...

For playback, don't copy if there is no available data; For capture, don't copy if there is no free host side buffer.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

src/audio/host.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/audio/host.c b/src/audio/host.c index b604ddc..61d2b62 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -815,7 +815,10 @@ static int host_copy(struct comp_dev *dev) if (dev->state != COMP_STATE_RUNNING) return 0;

if (hd->host_avail == 0)
/* don't copy if the host side buffer is not ready */

if (((hd->params.direction == STREAM_DIRECTION_PLAYBACK)
   &&  (hd->host_avail == 0)) || ((hd->host_free == 0) &&
   (hd->params.direction == STREAM_DIRECTION_CAPTURE)))
return 0;

Better to do for readability :-

if (dir == P && host_avail == 0) return 0; if (dir == C && host_free == 0) return 0;

...

/* do DMA transfer */

Jie, Yang

2:51 p.m.

New subject: [Sound-open-firmware] [PATCH 3/5] host: host_copy(): add handle for capture

...

-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Wednesday, February 15, 2017 6:33 PM To: Keyon Jie yang.jie@linux.intel.com Cc: sound-open-firmware@alsa-project.org; Zhang, Keqiao keqiao.zhang@intel.com; Jie, Yang yang.jie@intel.com; Ingalsuo, Seppo seppo.ingalsuo@intel.com Subject: Re: [Sound-open-firmware] [PATCH 3/5] host: host_copy(): add handle for capture

On Wed, 2017-02-15 at 18:07 +0800, Keyon Jie wrote:

...
For playback, don't copy if there is no available data; For capture, don't copy if there is no free host side buffer.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

src/audio/host.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/audio/host.c b/src/audio/host.c index b604ddc..61d2b62 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -815,7 +815,10 @@ static int host_copy(struct comp_dev *dev) if (dev->state != COMP_STATE_RUNNING) return 0;

if (hd->host_avail == 0)
/* don't copy if the host side buffer is not ready */

if (((hd->params.direction == STREAM_DIRECTION_PLAYBACK)
   &&  (hd->host_avail == 0)) || ((hd->host_free == 0) &&
   (hd->params.direction == STREAM_DIRECTION_CAPTURE)))
return 0;
Better to do for readability :-

if (dir == P && host_avail == 0) return 0; if (dir == C && host_free == 0) return 0;

OK, will modify like that.

Thanks, ~Keyon

...

...
/* do DMA transfer */

Ughreja, Rakesh A

4:15 p.m.

New subject: [Sound-open-firmware] [PATCH 3/5] host: host_copy(): add handle for capture

...

-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of Keyon Jie Sent: Wednesday, February 15, 2017 3:38 PM To: sound-open-firmware@alsa-project.org; liam.r.girdwood@linux.intel.com Cc: Zhang, Keqiao keqiao.zhang@intel.com; Jie, Yang yang.jie@intel.com; Ingalsuo, Seppo seppo.ingalsuo@intel.com; Keyon Jie yang.jie@linux.intel.com Subject: [Sound-open-firmware] [PATCH 3/5] host: host_copy(): add handle for capture

For playback, don't copy if there is no available data; For capture, don't copy if there is no free host side buffer.

How would you know that there is data available in the ring buffer ? Are you talking about SPIB mode ?

...

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

src/audio/host.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/audio/host.c b/src/audio/host.c index b604ddc..61d2b62 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -815,7 +815,10 @@ static int host_copy(struct comp_dev *dev) if (dev->state != COMP_STATE_RUNNING) return 0;

if (hd->host_avail == 0)
/* don't copy if the host side buffer is not ready */

if (((hd->params.direction == STREAM_DIRECTION_PLAYBACK)
   &&  (hd->host_avail == 0)) || ((hd->host_free == 0) &&
   (hd->params.direction == STREAM_DIRECTION_CAPTURE)))
return 0;

/* do DMA transfer */
-- 2.7.4

Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware

Jie, Yang

4:26 p.m.

New subject: [Sound-open-firmware] [PATCH 3/5] host: host_copy(): add handle for capture

...

-----Original Message----- From: Ughreja, Rakesh A Sent: Wednesday, February 15, 2017 11:15 PM To: Keyon Jie yang.jie@linux.intel.com; sound-open-firmware@alsa- project.org; liam.r.girdwood@linux.intel.com Cc: Zhang, Keqiao keqiao.zhang@intel.com; Jie, Yang yang.jie@intel.com; Ingalsuo, Seppo seppo.ingalsuo@intel.com Subject: RE: [Sound-open-firmware] [PATCH 3/5] host: host_copy(): add handle for capture

...
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of Keyon Jie Sent: Wednesday, February 15, 2017 3:38 PM To: sound-open-firmware@alsa-project.org; liam.r.girdwood@linux.intel.com Cc: Zhang, Keqiao keqiao.zhang@intel.com; Jie, Yang yang.jie@intel.com; Ingalsuo, Seppo seppo.ingalsuo@intel.com; Keyon Jie yang.jie@linux.intel.com Subject: [Sound-open-firmware] [PATCH 3/5] host: host_copy(): add handle for capture

For playback, don't copy if there is no available data; For capture, don't copy if there is no free host side buffer.

How would you know that there is data available in the ring buffer ? Are you talking about SPIB mode ?

We implemented the R/W pointers check mechanism, and the host_avail will keep the ring buffer available size at runtime, you can refer to host.c for that.

We don't have SPIB mode concept in sound-open-firmware ATM.

Thanks, ~Keyon

...

...
Signed-off-by: Keyon Jie yang.jie@linux.intel.com

src/audio/host.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/audio/host.c b/src/audio/host.c index b604ddc..61d2b62 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -815,7 +815,10 @@ static int host_copy(struct comp_dev *dev) if (dev->state != COMP_STATE_RUNNING) return 0;

if (hd->host_avail == 0)
/* don't copy if the host side buffer is not ready */

if (((hd->params.direction == STREAM_DIRECTION_PLAYBACK)
   &&  (hd->host_avail == 0)) || ((hd->host_free == 0) &&
   (hd->params.direction == STREAM_DIRECTION_CAPTURE)))
return 0;

/* do DMA transfer */
-- 2.7.4

Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware

Keyon Jie

11:07 a.m.

New subject: [Sound-open-firmware] [PATCH 4/5] host: rename host_pos_read to host_pos_local and update comments

We are using host_pos_read not only for playback, for capture, we use it as the host side buffer write pointer, so, here rename it to host_pos_local and add comments to explain that.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/src/audio/host.c b/src/audio/host.c index 61d2b62..f814603 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -71,11 +71,11 @@ struct host_data { struct hc_buf host; struct hc_buf local; uint32_t host_size; - volatile uint32_t *host_pos; /* read pos, update to mailbox for host side */ - uint32_t host_pos_read; /* host buffer read pos in bytes */ - uint32_t host_period_bytes; /* host period size in bytes */ - uint32_t host_period_pos; /* position in current host perid */ - uint32_t host_app_pos; /* host buffer app write pos, points to mailbox */ + volatile uint32_t *host_pos; /* read/write pos, update to mailbox for host side */ + uint32_t host_pos_local; /* the real host buffer local read/write pos in bytes */ + uint32_t host_period_bytes; /* host period size(host_pos update period) in bytes */ + uint32_t host_period_pos; /* position in current host period */ + uint32_t host_app_pos; /* host buffer app write/read pos, udpate via IPC */ uint32_t host_avail; /* host buffer available size */ uint32_t host_free; /* host buffer free size */ /* pointers set during params to host or local above */ @@ -91,24 +91,24 @@ struct host_data {

static inline void host_update_buffer_produce(struct host_data *hd) { - 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) + if (hd->host_pos_local < hd->host_app_pos) + hd->host_avail = hd->host_app_pos - hd->host_pos_local; + else if (hd->host_pos_local == hd->host_app_pos) hd->host_avail = hd->host_size; /* full */ else - hd->host_avail = hd->host_size -hd->host_pos_read + + hd->host_avail = hd->host_size -hd->host_pos_local + 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_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) + if (hd->host_pos_local < hd->host_app_pos) + hd->host_avail = hd->host_app_pos - hd->host_pos_local; + else if (hd->host_pos_local == hd->host_app_pos) hd->host_avail = 0; /* empty */ else - hd->host_avail = hd->host_size -hd->host_pos_read + + hd->host_avail = hd->host_size -hd->host_pos_local + hd->host_app_pos; hd->host_free = hd->host_size - hd->host_avail; } @@ -163,11 +163,11 @@ static void host_dma_cb_playback(struct comp_dev *dev, comp_update_buffer_produce(hd->dma_buffer);

/* new local period, update host buffer position blks */ - hd->host_pos_read += local_elem->size; + hd->host_pos_local += local_elem->size;

/* buffer overlap ? */ - if (hd->host_pos_read >= hd->host_size) - hd->host_pos_read = 0; + if (hd->host_pos_local >= hd->host_size) + hd->host_pos_local = 0; host_update_buffer_consume(hd);

/* send IPC message to driver if needed */ @@ -178,7 +178,7 @@ static void host_dma_cb_playback(struct comp_dev *dev, if (hd->host_avail) { /* update for host side */ if (hd->host_pos) { - *hd->host_pos = hd->host_pos_read; + *hd->host_pos = hd->host_pos_local; ipc_stream_send_notification(dev, &hd->cp); } } @@ -281,13 +281,13 @@ static void host_dma_cb_capture(struct comp_dev *dev, #endif

/* new local period, update host buffer position blks */ - hd->host_pos_read += local_elem->size; + hd->host_pos_local += local_elem->size;

/* buffer overlap ? */ - if (hd->host_pos_read >= hd->host_size) - hd->host_pos_read = 0; + if (hd->host_pos_local >= hd->host_size) + hd->host_pos_local = 0; if (hd->host_pos) - *hd->host_pos = hd->host_pos_read; + *hd->host_pos = hd->host_pos_local;

/* recalc available buffer space */ comp_update_buffer_consume(hd->dma_buffer); @@ -381,7 +381,7 @@ static uint32_t host_finish_work(void *data, uint32_t udelay) trace_comp("hFw"); /* update for host side */ if (hd->host_pos) { - *hd->host_pos = hd->host_pos_read; + *hd->host_pos = hd->host_pos_local; /* send the last notification to host */ ipc_stream_send_notification(dev, &hd->cp); } @@ -632,7 +632,7 @@ static int host_prepare(struct comp_dev *dev)

if (hd->host_pos) *hd->host_pos = 0; - hd->host_pos_read = 0; + hd->host_pos_local = 0; hd->host_period_pos = 0; hd->host_period_bytes = hd->params.period_frames * hd->params.frame_size; @@ -674,7 +674,7 @@ static int host_pointer_reset(struct comp_dev *dev) if (hd->host_pos) *hd->host_pos = 0; hd->host_app_pos = 0; - hd->host_pos_read = 0; + hd->host_pos_local = 0; hd->host_period_pos = 0;

host_update_buffer_consume(hd);

-- 2.7.4

Liam Girdwood

11:49 a.m.

New subject: [Sound-open-firmware] [PATCH 4/5] host: rename host_pos_read to host_pos_local and update comments

On Wed, 2017-02-15 at 18:07 +0800, Keyon Jie wrote:

...

We are using host_pos_read not only for playback, for capture, we use it as the host side buffer write pointer, so, here rename it to host_pos_local and add comments to explain that.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

src/audio/host.c | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/src/audio/host.c b/src/audio/host.c index 61d2b62..f814603 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -71,11 +71,11 @@ struct host_data { struct hc_buf host; struct hc_buf local; uint32_t host_size;

volatile uint32_t *host_pos; /* read pos, update to mailbox for host side */

uint32_t host_pos_read; /* host buffer read pos in bytes */

uint32_t host_period_bytes; /* host period size in bytes */

uint32_t host_period_pos; /* position in current host perid */

uint32_t host_app_pos; /* host buffer app write pos, points to mailbox */

volatile uint32_t *host_pos; /* read/write pos, update to mailbox for host side */

uint32_t host_pos_local; /* the real host buffer local read/write pos in bytes */

uint32_t host_period_bytes; /* host period size(host_pos update period) in bytes */

uint32_t host_period_pos; /* position in current host period */

uint32_t host_app_pos; /* host buffer app write/read pos, udpate via IPC */

Why dont we just rename to use r_ptr and w_ptr naming, that makes it very simple to understand. This way the copy() always updates read and write ptrs without worrying about naming.

...

uint32_t host_avail; /* host buffer available size */ uint32_t host_free; /* host buffer free size */ /* pointers set during params to host or local above */ @@ -91,24 +91,24 @@ struct host_data {

static inline void host_update_buffer_produce(struct host_data *hd) {
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)
if (hd->host_pos_local < hd->host_app_pos)
hd->host_avail = hd->host_app_pos - hd->host_pos_local;
else if (hd->host_pos_local == hd->host_app_pos) hd->host_avail = hd->host_size; /* full */ else
hd->host_avail = hd->host_size -hd->host_pos_read +
hd->host_avail = hd->host_size -hd->host_pos_local +
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_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)
if (hd->host_pos_local < hd->host_app_pos)
hd->host_avail = hd->host_app_pos - hd->host_pos_local;
else if (hd->host_pos_local == hd->host_app_pos) hd->host_avail = 0; /* empty */ else
hd->host_avail = hd->host_size -hd->host_pos_read +
hd->host_avail = hd->host_size -hd->host_pos_local +
hd->host_app_pos;
hd->host_free = hd->host_size - hd->host_avail;
} @@ -163,11 +163,11 @@ static void host_dma_cb_playback(struct comp_dev *dev, comp_update_buffer_produce(hd->dma_buffer);

/* new local period, update host buffer position blks */

hd->host_pos_read += local_elem->size;

hd->host_pos_local += local_elem->size;

/* buffer overlap ? */
if (hd->host_pos_read >= hd->host_size)
hd->host_pos_read = 0;
if (hd->host_pos_local >= hd->host_size)
hd->host_pos_local = 0;
host_update_buffer_consume(hd);

/* send IPC message to driver if needed */
@@ -178,7 +178,7 @@ static void host_dma_cb_playback(struct comp_dev *dev, if (hd->host_avail) { /* update for host side */ if (hd->host_pos) {
		*hd->host_pos = hd->host_pos_read;
		*hd->host_pos = hd->host_pos_local;
	ipc_stream_send_notification(dev, &hd->cp);
}
}
@@ -281,13 +281,13 @@ static void host_dma_cb_capture(struct comp_dev *dev, #endif

/* new local period, update host buffer position blks */

hd->host_pos_read += local_elem->size;

hd->host_pos_local += local_elem->size;

/* buffer overlap ? */
if (hd->host_pos_read >= hd->host_size)
hd->host_pos_read = 0;
if (hd->host_pos_local >= hd->host_size)
hd->host_pos_local = 0;
if (hd->host_pos)
*hd->host_pos = hd->host_pos_read;
*hd->host_pos = hd->host_pos_local;
/* recalc available buffer space */ comp_update_buffer_consume(hd->dma_buffer);
@@ -381,7 +381,7 @@ static uint32_t host_finish_work(void *data, uint32_t udelay) trace_comp("hFw"); /* update for host side */ if (hd->host_pos) {
	*hd->host_pos = hd->host_pos_read;
	*hd->host_pos = hd->host_pos_local;
/* send the last notification to host */
ipc_stream_send_notification(dev, &hd->cp);
}
@@ -632,7 +632,7 @@ static int host_prepare(struct comp_dev *dev)

if (hd->host_pos) *hd->host_pos = 0;

hd->host_pos_read = 0;

hd->host_pos_local = 0; hd->host_period_pos = 0; hd->host_period_bytes = hd->params.period_frames * hd->params.frame_size;

@@ -674,7 +674,7 @@ static int host_pointer_reset(struct comp_dev *dev) if (hd->host_pos) *hd->host_pos = 0; hd->host_app_pos = 0;

hd->host_pos_read = 0;

hd->host_pos_local = 0; hd->host_period_pos = 0;

host_update_buffer_consume(hd);

Jie, Yang

3:06 p.m.

New subject: [Sound-open-firmware] [PATCH 4/5] host: rename host_pos_read to host_pos_local and update comments

...

-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Wednesday, February 15, 2017 6:49 PM To: Keyon Jie yang.jie@linux.intel.com Cc: sound-open-firmware@alsa-project.org; Zhang, Keqiao keqiao.zhang@intel.com; Jie, Yang yang.jie@intel.com; Ingalsuo, Seppo seppo.ingalsuo@intel.com Subject: Re: [Sound-open-firmware] [PATCH 4/5] host: rename host_pos_read to host_pos_local and update comments

On Wed, 2017-02-15 at 18:07 +0800, Keyon Jie wrote:

...
We are using host_pos_read not only for playback, for capture, we use it as the host side buffer write pointer, so, here rename it to host_pos_local and add comments to explain that.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

src/audio/host.c | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/src/audio/host.c b/src/audio/host.c index 61d2b62..f814603 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -71,11 +71,11 @@ struct host_data { struct hc_buf host; struct hc_buf local; uint32_t host_size;

volatile uint32_t *host_pos; /* read pos, update to mailbox for host

side */

...

uint32_t host_pos_read; /* host buffer read pos in bytes */

uint32_t host_period_bytes; /* host period size in bytes */

uint32_t host_period_pos; /* position in current host perid */

uint32_t host_app_pos; /* host buffer app write pos, points to

mailbox */

...

volatile uint32_t *host_pos; /* read/write pos, update to mailbox for

host side */

...

uint32_t host_pos_local; /* the real host buffer local read/write

pos in bytes */

...

uint32_t host_period_bytes; /* host period size(host_pos update

period) in bytes */

...

uint32_t host_period_pos; /* position in current host period */

uint32_t host_app_pos; /* host buffer app write/read pos, udpate

via IPC */

Why dont we just rename to use r_ptr and w_ptr naming, that makes it very simple to understand. This way the copy() always updates read and write ptrs without worrying about naming.

Um, this means we may need more codes to judge the stream direction, e.g. if(stream->direction == playback) *host_pos = r_ptr; else *host_pos = w_ptr;

but let me think of that further and try to figure out the modification, to see if it can make life easier really.

Thanks, ~Keyon

...

...
uint32_t host_avail; /* host buffer available size */ uint32_t host_free; /* host buffer free size */ /* pointers set during params to host or local above */ @@ -91,24 +91,24 @@ struct host_data {

static inline void host_update_buffer_produce(struct host_data *hd) {
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)
if (hd->host_pos_local < hd->host_app_pos)
hd->host_avail = hd->host_app_pos - hd->host_pos_local;
else if (hd->host_pos_local == hd->host_app_pos) hd->host_avail = hd->host_size; /* full */ else
hd->host_avail = hd->host_size -hd->host_pos_read +
hd->host_avail = hd->host_size -hd->host_pos_local +
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_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)
if (hd->host_pos_local < hd->host_app_pos)
hd->host_avail = hd->host_app_pos - hd->host_pos_local;
else if (hd->host_pos_local == hd->host_app_pos) hd->host_avail = 0; /* empty */ else
hd->host_avail = hd->host_size -hd->host_pos_read +
hd->host_avail = hd->host_size -hd->host_pos_local +
hd->host_app_pos;
hd->host_free = hd->host_size - hd->host_avail; } @@ -163,11
+163,11 @@ static void host_dma_cb_playback(struct comp_dev *dev, comp_update_buffer_produce(hd->dma_buffer);

/* new local period, update host buffer position blks */

hd->host_pos_read += local_elem->size;

hd->host_pos_local += local_elem->size;

/* buffer overlap ? */
if (hd->host_pos_read >= hd->host_size)
hd->host_pos_read = 0;
if (hd->host_pos_local >= hd->host_size)
hd->host_pos_local = 0;
host_update_buffer_consume(hd);

/* send IPC message to driver if needed */ @@ -178,7 +178,7 @@
static void host_dma_cb_playback(struct comp_dev *dev, if (hd->host_avail) { /* update for host side */ if (hd->host_pos) {
		*hd->host_pos = hd->host_pos_read;
		*hd->host_pos = hd->host_pos_local;
	ipc_stream_send_notification(dev, &hd->cp);
}
}
@@ -281,13 +281,13 @@ static void host_dma_cb_capture(struct comp_dev *dev, #endif

/* new local period, update host buffer position blks */

hd->host_pos_read += local_elem->size;

hd->host_pos_local += local_elem->size;

/* buffer overlap ? */
if (hd->host_pos_read >= hd->host_size)
hd->host_pos_read = 0;
if (hd->host_pos_local >= hd->host_size)
hd->host_pos_local = 0;
if (hd->host_pos)
*hd->host_pos = hd->host_pos_read;
*hd->host_pos = hd->host_pos_local;
/* recalc available buffer space */ comp_update_buffer_consume(hd->dma_buffer);
@@ -381,7 +381,7 @@ static uint32_t host_finish_work(void *data, uint32_t
udelay)

...
trace_comp("hFw");
/* update for host side */
if (hd->host_pos) {
	*hd->host_pos = hd->host_pos_read;
	*hd->host_pos = hd->host_pos_local;
/* send the last notification to host */
ipc_stream_send_notification(dev, &hd->cp);
}
@@ -632,7 +632,7 @@ static int host_prepare(struct comp_dev *dev)

if (hd->host_pos) *hd->host_pos = 0;

hd->host_pos_read = 0;

hd->host_pos_local = 0; hd->host_period_pos = 0; hd->host_period_bytes = hd->params.period_frames * hd->params.frame_size; @@ -
674,7 +674,7

...
@@ static int host_pointer_reset(struct comp_dev *dev) if (hd->host_pos) *hd->host_pos = 0; hd->host_app_pos = 0;

hd->host_pos_read = 0;

hd->host_pos_local = 0; hd->host_period_pos = 0;

host_update_buffer_consume(hd);

Keyon Jie

11:07 a.m.

New subject: [Sound-open-firmware] [PATCH 5/5] host: integrate consume and produce to host_update_buffer and fix for capture

For capture, the read/write pointers are opposite, here we integrate produce/consume to host_update_buffer(hd, produce) and introduce local variables w_ptr and r_ptr for more understandable, we also change 'inline' to real function to save code size at the same time.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/audio/host.c | 46 ++++++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/src/audio/host.c b/src/audio/host.c index f814603..62678d2 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -89,27 +89,25 @@ struct host_data { struct comp_position cp; };

-static inline void host_update_buffer_produce(struct host_data *hd) +static void host_update_buffer(struct host_data *hd, uint32_t produce) { - if (hd->host_pos_local < hd->host_app_pos) - hd->host_avail = hd->host_app_pos - hd->host_pos_local; - else if (hd->host_pos_local == hd->host_app_pos) - hd->host_avail = hd->host_size; /* full */ - else - hd->host_avail = hd->host_size -hd->host_pos_local + - hd->host_app_pos; - hd->host_free = hd->host_size - hd->host_avail; -} + uint32_t w_ptr, r_ptr; + if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) { + /* for playback, app_pos is write pointer, pos_local is read pointer */ + w_ptr = hd->host_app_pos; + r_ptr = hd->host_pos_local; + } else { + /* for capture, pos_local is write pointer, app_pos is read pointer */ + w_ptr = hd->host_pos_local; + r_ptr = hd->host_app_pos; + }

-static inline void host_update_buffer_consume(struct host_data *hd) -{ - if (hd->host_pos_local < hd->host_app_pos) - hd->host_avail = hd->host_app_pos - hd->host_pos_local; - else if (hd->host_pos_local == hd->host_app_pos) - hd->host_avail = 0; /* empty */ + if (r_ptr < w_ptr) + hd->host_avail = w_ptr - r_ptr; + else if (r_ptr == w_ptr) + hd->host_avail = produce ? hd->host_size : 0; else - hd->host_avail = hd->host_size -hd->host_pos_local + - hd->host_app_pos; + hd->host_avail = hd->host_size -r_ptr + w_ptr; hd->host_free = hd->host_size - hd->host_avail; }

@@ -168,7 +166,7 @@ static void host_dma_cb_playback(struct comp_dev *dev, /* buffer overlap ? */ if (hd->host_pos_local >= hd->host_size) hd->host_pos_local = 0; - host_update_buffer_consume(hd); + host_update_buffer(hd, 0); /* consume */

/* send IPC message to driver if needed */ hd->host_period_pos += local_elem->size; @@ -638,7 +636,7 @@ static int host_prepare(struct comp_dev *dev) hd->params.period_frames * hd->params.frame_size; hd->split_remaining = 0;

- host_update_buffer_consume(hd); + host_update_buffer(hd, 0); /* consume */

dev->preload = PLAT_HOST_PERIODS;

@@ -677,7 +675,7 @@ static int host_pointer_reset(struct comp_dev *dev) hd->host_pos_local = 0; hd->host_period_pos = 0;

- host_update_buffer_consume(hd); + host_update_buffer(hd, 0); /* consume */

return 0; } @@ -736,9 +734,9 @@ static int host_cmd(struct comp_dev *dev, int cmd, void *data) hd->host_app_pos = app_pos->position;

if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) - host_update_buffer_produce(hd); + host_update_buffer(hd, 1); /* produce */ else - host_update_buffer_consume(hd); + host_update_buffer(hd, 0); /* consume */ break; case COMP_CMD_VOLUME: vol_dev = host_volume_component(dev); @@ -766,7 +764,7 @@ static int host_buffer(struct comp_dev *dev, struct dma_sg_elem *elem, *e = *elem; hd->host_size = host_size;

- host_update_buffer_consume(hd); + host_update_buffer(hd, 0); /* consume */

list_item_append(&e->list, &hd->host.elem_list); return 0;

-- 2.7.4

2866

Age (days ago)

2866

Last active (days ago)

List overview

Download

11 comments

4 participants

tags (0)

participants (4)

Jie, Yang
Keyon Jie
Liam Girdwood
Ughreja, Rakesh A