Re: [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