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

newer
Re: [Sound-open-firmware] [PATCH...

older
[Sound-open-firmware] [PATCH 0/5]...

Keyon Jie

16 Feb 2017 16 Feb '17

3:08 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.

Changes in v2: 1. change to use w_ptr/r_ptr host buffer pointers to make it easy to understand.

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: change host buffer pointer to w_ptr/r_ptr and update comments host: integrate consume and produce to host_update_buffer

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

-- 2.7.4

Show replies by thread

Keyon Jie

16 Feb 16 Feb

3:08 a.m.

New subject: [Sound-open-firmware] [PATCH v2 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

3:08 a.m.

New subject: [Sound-open-firmware] [PATCH v2 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

3:08 a.m.

New subject: [Sound-open-firmware] [PATCH v2 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 | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/audio/host.c b/src/audio/host.c index b604ddc..f15dd33 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -815,7 +815,12 @@ 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)) + return 0; + if ((hd->params.direction == STREAM_DIRECTION_CAPTURE) + && (hd->host_free == 0)) return 0;

/* do DMA transfer */

-- 2.7.4

Keyon Jie

3:08 a.m.

New subject: [Sound-open-firmware] [PATCH v2 4/5] host: change host buffer pointer to w_ptr/r_ptr and update comments

We are using host_pos_read not only for playback, for capture, using it as the host side buffer write pointer, this is somewhat confusing, to make it more clear and simple to understand, here change to use w_ptr and r_ptr, and add comments to explain that.

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

diff --git a/src/audio/host.c b/src/audio/host.c index f15dd33..419aca3 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_w_ptr; /* the host buffer write pos in bytes */ + uint32_t host_r_ptr; /* the host buffer read 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_avail; /* host buffer available size */ uint32_t host_free; /* host buffer free size */ /* pointers set during params to host or local above */ @@ -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_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) - hd->host_avail = hd->host_size; /* full */ + if (hd->host_r_ptr < hd->host_w_ptr) + hd->host_avail = hd->host_w_ptr - hd->host_r_ptr; + else if (hd->host_r_ptr == hd->host_w_ptr) + hd->host_avail = hd->host_size; else - hd->host_avail = hd->host_size -hd->host_pos_read + - hd->host_app_pos; + hd->host_avail = hd->host_size -hd->host_r_ptr + hd->host_w_ptr; hd->host_free = hd->host_size - hd->host_avail; }

-static inline void host_update_buffer_consume(struct host_data *hd) +static 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) - hd->host_avail = 0; /* empty */ + if (hd->host_r_ptr < hd->host_w_ptr) + hd->host_avail = hd->host_w_ptr - hd->host_r_ptr; + else if (hd->host_r_ptr == hd->host_w_ptr) + hd->host_avail = 0; else - hd->host_avail = hd->host_size -hd->host_pos_read + - hd->host_app_pos; + hd->host_avail = hd->host_size -hd->host_r_ptr + hd->host_w_ptr; hd->host_free = hd->host_size - hd->host_avail; }

@@ -163,11 +161,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_r_ptr += local_elem->size;

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

/* send IPC message to driver if needed */ @@ -178,7 +176,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_r_ptr; ipc_stream_send_notification(dev, &hd->cp); } } @@ -281,13 +279,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_w_ptr += local_elem->size;

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

/* recalc available buffer space */ comp_update_buffer_consume(hd->dma_buffer); @@ -381,7 +379,9 @@ 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->params.direction == STREAM_DIRECTION_PLAYBACK ? + hd->host_r_ptr : hd->host_w_ptr; /* send the last notification to host */ ipc_stream_send_notification(dev, &hd->cp); } @@ -617,12 +617,16 @@ static int host_prepare(struct comp_dev *dev) struct comp_buffer *dma_buffer;

if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) { + hd->host_r_ptr = 0; + dma_buffer = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);

dma_buffer->r_ptr = dma_buffer->addr; dma_buffer->w_ptr = dma_buffer->addr; } else { + hd->host_w_ptr = 0; + dma_buffer = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list);

@@ -632,7 +636,6 @@ static int host_prepare(struct comp_dev *dev)

if (hd->host_pos) *hd->host_pos = 0; - hd->host_pos_read = 0; hd->host_period_pos = 0; hd->host_period_bytes = hd->params.period_frames * hd->params.frame_size; @@ -673,8 +676,8 @@ static int host_pointer_reset(struct comp_dev *dev) /* reset buffer pointers */ if (hd->host_pos) *hd->host_pos = 0; - hd->host_app_pos = 0; - hd->host_pos_read = 0; + hd->host_r_ptr = 0; + hd->host_w_ptr = 0; hd->host_period_pos = 0;

host_update_buffer_consume(hd); @@ -733,12 +736,15 @@ static int host_cmd(struct comp_dev *dev, int cmd, void *data) break; case COMP_CMD_AVAIL_UPDATE: app_pos = (struct ipc_intel_ipc_stream_set_position *)data; - hd->host_app_pos = app_pos->position;

- if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) + if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) { + hd->host_w_ptr = app_pos->position; host_update_buffer_produce(hd); - else + } + else { + hd->host_r_ptr = app_pos->position; host_update_buffer_consume(hd); + } break; case COMP_CMD_VOLUME: vol_dev = host_volume_component(dev);

-- 2.7.4

Keyon Jie

3:08 a.m.

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

For capture, the read/write pointers are opposite, here we integrate produce/consume to host_update_buffer(hd, produce). 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 | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/src/audio/host.c b/src/audio/host.c index 419aca3..92b5cf3 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -89,23 +89,12 @@ struct host_data { struct comp_position cp; };

-static void host_update_buffer_produce(struct host_data *hd) +static void host_update_buffer(struct host_data *hd, uint32_t produce) { if (hd->host_r_ptr < hd->host_w_ptr) hd->host_avail = hd->host_w_ptr - hd->host_r_ptr; else if (hd->host_r_ptr == hd->host_w_ptr) - hd->host_avail = hd->host_size; - else - hd->host_avail = hd->host_size -hd->host_r_ptr + hd->host_w_ptr; - hd->host_free = hd->host_size - hd->host_avail; -} - -static void host_update_buffer_consume(struct host_data *hd) -{ - if (hd->host_r_ptr < hd->host_w_ptr) - hd->host_avail = hd->host_w_ptr - hd->host_r_ptr; - else if (hd->host_r_ptr == hd->host_w_ptr) - hd->host_avail = 0; + hd->host_avail = produce ? hd->host_size : 0; else hd->host_avail = hd->host_size -hd->host_r_ptr + hd->host_w_ptr; hd->host_free = hd->host_size - hd->host_avail; @@ -166,7 +155,7 @@ static void host_dma_cb_playback(struct comp_dev *dev, /* buffer overlap ? */ if (hd->host_r_ptr >= hd->host_size) hd->host_r_ptr = 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; @@ -641,7 +630,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;

@@ -680,7 +669,7 @@ static int host_pointer_reset(struct comp_dev *dev) hd->host_w_ptr = 0; hd->host_period_pos = 0;

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

return 0; } @@ -739,11 +728,11 @@ static int host_cmd(struct comp_dev *dev, int cmd, void *data)

if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) { hd->host_w_ptr = app_pos->position; - host_update_buffer_produce(hd); + host_update_buffer(hd, 1); /* produce */ } else { hd->host_r_ptr = app_pos->position; - host_update_buffer_consume(hd); + host_update_buffer(hd, 0); /* consume */ } break; case COMP_CMD_VOLUME: @@ -772,7 +761,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

Liam Girdwood

20 Feb 20 Feb

2:56 p.m.

New subject: [Sound-open-firmware] [PATCH v2 4/5] host: change host buffer pointer to w_ptr/r_ptr and update comments

On Thu, 2017-02-16 at 16:08 +0800, Keyon Jie wrote:

...

We are using host_pos_read not only for playback, for capture, using it as the host side buffer write pointer, this is somewhat confusing, to make it more clear and simple to understand, here change to use w_ptr and r_ptr, and add comments to explain that.

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

src/audio/host.c | 74 ++++++++++++++++++++++++++++++-------------------------- 1 file changed, 40 insertions(+), 34 deletions(-)

diff --git a/src/audio/host.c b/src/audio/host.c index f15dd33..419aca3 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_w_ptr; /* the host buffer write pos in bytes */

uint32_t host_r_ptr; /* the host buffer read 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 */

Do we really need the host side buffer data ? We only need to know the host period size, buffer size and base address. We can get rid of the rest except anything used to calculate whether a host period has elapsed.

One other thing is that the host component used to create a list of period elems for local and host buffers. We dont use the elem list for host side any more so this can be removed too.

We also dont need the host prefix either on the names since it's obvious they are host specific, the local data/elems is using struct buffer.

Liam

...

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 */ @@ -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_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)
hd->host_avail = hd->host_size; /* full */
if (hd->host_r_ptr < hd->host_w_ptr)
hd->host_avail = hd->host_w_ptr - hd->host_r_ptr;
else if (hd->host_r_ptr == hd->host_w_ptr)
hd->host_avail = hd->host_size;
else
hd->host_avail = hd->host_size -hd->host_pos_read +
	hd->host_app_pos;
hd->host_avail = hd->host_size -hd->host_r_ptr + hd->host_w_ptr;
hd->host_free = hd->host_size - hd->host_avail;
}

-static inline void host_update_buffer_consume(struct host_data *hd) +static 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)
hd->host_avail = 0; /* empty */
if (hd->host_r_ptr < hd->host_w_ptr)
hd->host_avail = hd->host_w_ptr - hd->host_r_ptr;
else if (hd->host_r_ptr == hd->host_w_ptr)
hd->host_avail = 0;
else
hd->host_avail = hd->host_size -hd->host_pos_read +
	hd->host_app_pos;
hd->host_avail = hd->host_size -hd->host_r_ptr + hd->host_w_ptr;
hd->host_free = hd->host_size - hd->host_avail;
}

@@ -163,11 +161,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_r_ptr += local_elem->size;

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

/* send IPC message to driver if needed */
@@ -178,7 +176,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_r_ptr;
	ipc_stream_send_notification(dev, &hd->cp);
}
}
@@ -281,13 +279,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_w_ptr += local_elem->size;

/* buffer overlap ? */
if (hd->host_pos_read >= hd->host_size)
hd->host_pos_read = 0;
if (hd->host_w_ptr >= hd->host_size)
hd->host_w_ptr = 0;
if (hd->host_pos)
*hd->host_pos = hd->host_pos_read;
*hd->host_pos = hd->host_w_ptr;
/* recalc available buffer space */ comp_update_buffer_consume(hd->dma_buffer);
@@ -381,7 +379,9 @@ 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->params.direction == STREAM_DIRECTION_PLAYBACK ?
		hd->host_r_ptr : hd->host_w_ptr;
/* send the last notification to host */
ipc_stream_send_notification(dev, &hd->cp);
}
@@ -617,12 +617,16 @@ static int host_prepare(struct comp_dev *dev) struct comp_buffer *dma_buffer;

if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) {
hd->host_r_ptr = 0;
dma_buffer = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);

dma_buffer->r_ptr = dma_buffer->addr; dma_buffer->w_ptr = dma_buffer->addr; } else {
hd->host_w_ptr = 0;
dma_buffer = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list);
@@ -632,7 +636,6 @@ static int host_prepare(struct comp_dev *dev)

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

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

@@ -673,8 +676,8 @@ static int host_pointer_reset(struct comp_dev *dev) /* reset buffer pointers */ if (hd->host_pos) *hd->host_pos = 0;

hd->host_app_pos = 0;

hd->host_pos_read = 0;

hd->host_r_ptr = 0;

hd->host_w_ptr = 0; hd->host_period_pos = 0;

host_update_buffer_consume(hd);

@@ -733,12 +736,15 @@ static int host_cmd(struct comp_dev *dev, int cmd, void *data) break; case COMP_CMD_AVAIL_UPDATE: app_pos = (struct ipc_intel_ipc_stream_set_position *)data;
hd->host_app_pos = app_pos->position;
if (hd->params.direction == STREAM_DIRECTION_PLAYBACK)
if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) {
	hd->host_w_ptr = app_pos->position;
host_update_buffer_produce(hd);
else
}
else {
	hd->host_r_ptr = app_pos->position;
host_update_buffer_consume(hd);
}
break; case COMP_CMD_VOLUME: vol_dev = host_volume_component(dev);

Keyon Jie

21 Feb 21 Feb

12:42 a.m.

New subject: [Sound-open-firmware] [PATCH v2 4/5] host: change host buffer pointer to w_ptr/r_ptr and update comments

On 2017年02月21日 03:56, Liam Girdwood wrote:

...

On Thu, 2017-02-16 at 16:08 +0800, Keyon Jie wrote:

...
We are using host_pos_read not only for playback, for capture, using it as the host side buffer write pointer, this is somewhat confusing, to make it more clear and simple to understand, here change to use w_ptr and r_ptr, and add comments to explain that.

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

src/audio/host.c | 74 ++++++++++++++++++++++++++++++-------------------------- 1 file changed, 40 insertions(+), 34 deletions(-)

diff --git a/src/audio/host.c b/src/audio/host.c index f15dd33..419aca3 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_w_ptr; /* the host buffer write pos in bytes */

uint32_t host_r_ptr; /* the host buffer read 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 */

Do we really need the host side buffer data ? We only need to know the host period size, buffer size and base address. We can get rid of the rest except anything used to calculate whether a host period has elapsed.

The host_period_bytes, host_period_pos, and *host_pos are used for host period elapsed calculating and sharing, we don't have much rest items here, most of them are for buffer status management(w_ptr/r_ptr/avail/free).

...

One other thing is that the host component used to create a list of period elems for local and host buffers. We dont use the elem list for host side any more so this can be removed too.

Currently we are still using element list for host buffers, at least for byt, we cannot remove it ATM, right?

...

We also dont need the host prefix either on the names since it's obvious they are host specific, the local data/elems is using struct buffer.

The 'host' prefix is intend to meaning that those members are for host[buffer] side, others(without 'host' prefix) are for host component(local) itself.

Thanks, ~Keyon

...

Liam

...
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 */ @@ -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_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)
hd->host_avail = hd->host_size; /* full */
if (hd->host_r_ptr < hd->host_w_ptr)
hd->host_avail = hd->host_w_ptr - hd->host_r_ptr;
else if (hd->host_r_ptr == hd->host_w_ptr)
hd->host_avail = hd->host_size;
else
hd->host_avail = hd->host_size -hd->host_pos_read +
	hd->host_app_pos;
hd->host_avail = hd->host_size -hd->host_r_ptr + hd->host_w_ptr;
hd->host_free = hd->host_size - hd->host_avail; }
-static inline void host_update_buffer_consume(struct host_data *hd) +static 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)
hd->host_avail = 0; /* empty */
if (hd->host_r_ptr < hd->host_w_ptr)
hd->host_avail = hd->host_w_ptr - hd->host_r_ptr;
else if (hd->host_r_ptr == hd->host_w_ptr)
hd->host_avail = 0;
else
hd->host_avail = hd->host_size -hd->host_pos_read +
	hd->host_app_pos;
hd->host_avail = hd->host_size -hd->host_r_ptr + hd->host_w_ptr;
hd->host_free = hd->host_size - hd->host_avail; }
@@ -163,11 +161,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_r_ptr += local_elem->size;

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

/* send IPC message to driver if needed */
@@ -178,7 +176,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_r_ptr;
	ipc_stream_send_notification(dev, &hd->cp);
}
}
@@ -281,13 +279,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_w_ptr += local_elem->size;

/* buffer overlap ? */
if (hd->host_pos_read >= hd->host_size)
hd->host_pos_read = 0;
if (hd->host_w_ptr >= hd->host_size)
hd->host_w_ptr = 0;
if (hd->host_pos)
*hd->host_pos = hd->host_pos_read;
*hd->host_pos = hd->host_w_ptr;
/* recalc available buffer space */ comp_update_buffer_consume(hd->dma_buffer);
@@ -381,7 +379,9 @@ 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->params.direction == STREAM_DIRECTION_PLAYBACK ?
		hd->host_r_ptr : hd->host_w_ptr;
/* send the last notification to host */
ipc_stream_send_notification(dev, &hd->cp);
}
@@ -617,12 +617,16 @@ static int host_prepare(struct comp_dev *dev) struct comp_buffer *dma_buffer;

if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) {
hd->host_r_ptr = 0;
dma_buffer = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);

dma_buffer->r_ptr = dma_buffer->addr; dma_buffer->w_ptr = dma_buffer->addr; } else {
hd->host_w_ptr = 0;
dma_buffer = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list);
@@ -632,7 +636,6 @@ static int host_prepare(struct comp_dev *dev)

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

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

@@ -673,8 +676,8 @@ static int host_pointer_reset(struct comp_dev *dev) /* reset buffer pointers */ if (hd->host_pos) *hd->host_pos = 0;

hd->host_app_pos = 0;

hd->host_pos_read = 0;

hd->host_r_ptr = 0;

hd->host_w_ptr = 0; hd->host_period_pos = 0;

host_update_buffer_consume(hd);

@@ -733,12 +736,15 @@ static int host_cmd(struct comp_dev *dev, int cmd, void *data) break; case COMP_CMD_AVAIL_UPDATE: app_pos = (struct ipc_intel_ipc_stream_set_position *)data;
hd->host_app_pos = app_pos->position;
if (hd->params.direction == STREAM_DIRECTION_PLAYBACK)
if (hd->params.direction == STREAM_DIRECTION_PLAYBACK) {
	hd->host_w_ptr = app_pos->position;
host_update_buffer_produce(hd);
else
}
else {
	hd->host_r_ptr = app_pos->position;
host_update_buffer_consume(hd);
}
break; case COMP_CMD_VOLUME: vol_dev = host_volume_component(dev);
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware

2628

Age (days ago)

2633

Last active (days ago)

List overview

Download

7 comments

2 participants

tags (0)

participants (2)

Keyon Jie
Liam Girdwood