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

[Liam Girdwood]

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 at 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);