[Sound-open-firmware] [PATCH v5] Add support to replace stale stream/trace position updates.

[Liam Girdwood]

On Wed, 2017-12-06 at 18:45 +0800, yan.wang at linux.intel.com wrote:
> From: Yan Wang <yan.wang at linux.intel.com>
> 
> Host message queue is long sometimes. So when one new IPC message
> like
> DMA trace host offset is pushed into, the previous same type IPC
> message may
> haven't been sent.
> For every type of IPC message, there is different compare
> condition.So need a 
> group of sub-compare functions to compare IPC messages and user also
> can 
> extend them easily based on new requirements in the future.
> 

Can you add some comments in the code too, plus some minor refactoring
below.

> Signed-off-by: Yan Wang <yan.wang at linux.intel.com>
> ---
>  src/include/reef/ipc.h |   2 +-
>  src/ipc/intel-ipc.c    | 103
> +++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 93 insertions(+), 12 deletions(-)
> 
> diff --git a/src/include/reef/ipc.h b/src/include/reef/ipc.h
> index fcab45b..bb814be 100644
> --- a/src/include/reef/ipc.h
> +++ b/src/include/reef/ipc.h
> @@ -125,7 +125,7 @@ int ipc_stream_send_xrun(struct comp_dev *cdev,
>  
>  int ipc_queue_host_message(struct ipc *ipc, uint32_t header,
>  	void *tx_data, size_t tx_bytes, void *rx_data,
> -	size_t rx_bytes, void (*cb)(void*, void*), void *cb_data);
> +	size_t rx_bytes, void (*cb)(void*, void*), void *cb_data,
> uint32_t replace);
>  int ipc_send_short_msg(uint32_t msg);
>  
>  void ipc_platform_do_cmd(struct ipc *ipc);
> diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c
> index e13b541..787ae05 100644
> --- a/src/ipc/intel-ipc.c
> +++ b/src/ipc/intel-ipc.c
> @@ -367,7 +367,7 @@ int ipc_stream_send_position(struct comp_dev
> *cdev,
>  	posn->comp_id = cdev->comp.id;
>  
>  	return ipc_queue_host_message(_ipc, posn->rhdr.hdr.cmd,
> posn,
> -		sizeof(*posn), NULL, 0, NULL, NULL);
> +		sizeof(*posn), NULL, 0, NULL, NULL, 1);
>  }
>  
>  /* send stream position TODO: send compound message  */
> @@ -379,7 +379,7 @@ int ipc_stream_send_xrun(struct comp_dev *cdev,
>  	posn->comp_id = cdev->comp.id;
>  
>  	return ipc_queue_host_message(_ipc, posn->rhdr.hdr.cmd,
> posn,
> -		sizeof(*posn), NULL, 0, NULL, NULL);
> +		sizeof(*posn), NULL, 0, NULL, NULL, 1);
>  }
>  
>  static int ipc_stream_trigger(uint32_t header)
> @@ -644,7 +644,7 @@ int ipc_dma_trace_send_position(void)
>  	posn.rhdr.hdr.size = sizeof(posn);
>  
>  	return ipc_queue_host_message(_ipc, posn.rhdr.hdr.cmd,
> &posn,
> -		sizeof(posn), NULL, 0, NULL, NULL);
> +		sizeof(posn), NULL, 0, NULL, NULL, 1);
>  }
>  
>  static int ipc_glb_debug_message(uint32_t header)
> @@ -899,19 +899,98 @@ static inline struct ipc_msg
> *msg_get_empty(struct ipc *ipc)
>  	return msg;
>  }
>  
> +static inline uint32_t ipc_glb_stream_message_compare(struct ipc_msg
> *msg,
> +	uint32_t header, void *tx_data)

Better to pass in struct sof_ipc_stream_posn * here instead of void *
since this function will be stream specific.

> +{
> +	uint32_t cmd = (header & SOF_CMD_TYPE_MASK) >>
> SOF_CMD_TYPE_SHIFT;
> +	struct sof_ipc_stream_posn *old_pos = NULL;
> +	struct sof_ipc_stream_posn *new_pos = NULL;
> +

we need to iterate over the list here and check that the new message is
using the same header and component ID (comp_id) as the old message in
order to match.

> +	switch (cmd) {
> +	case iCS(SOF_IPC_STREAM_TRIG_XRUN):
> +	case iCS(SOF_IPC_STREAM_POSITION):
> +		new_pos = (struct sof_ipc_stream_posn *)tx_data;
> +		old_pos = (struct sof_ipc_stream_posn *)msg-
> >tx_data;
> +		return old_pos->comp_id == new_pos->comp_id;
> +	default:
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +
> +static inline uint32_t ipc_glb_trace_message_compare(struct ipc_msg
> *msg,
> +	uint32_t header, void *tx_data)
> +{

ditto, pass trace structure instead of void *

iterate list here too, checking header and component ID in the trace
struct.

> +	uint32_t cmd = (header & SOF_CMD_TYPE_MASK) >>
> SOF_CMD_TYPE_SHIFT;
> +
> +	switch (cmd) {
> +	case iCS(SOF_IPC_TRACE_DMA_POSITION):
> +		return 1;
> +	default:
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +
> +static inline struct ipc_msg *msg_find(struct ipc *ipc, uint32_t
> header,
> +	void *tx_data)
> +{
> +	struct list_item *plist;
> +	struct ipc_msg *msg = NULL;
> +	uint32_t type;
> +	uint32_t found = 0;
> +

we should iterate over the list in or stream/trace specific handlers
not here.

> +	list_for_item(plist, &ipc->msg_list) {
> +		msg = container_of(plist, struct ipc_msg, list);
> +		if (msg->header == header) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +
> +	if (!found)
> +		goto not_found;
> +
 

> +	type = (header & SOF_GLB_TYPE_MASK) >> SOF_GLB_TYPE_SHIFT;
> +
> +	switch (type) {
> +	case iGS(SOF_IPC_GLB_STREAM_MSG):
> +		found = ipc_glb_stream_message_compare(msg, header,
> tx_data);
> +	case iGS(SOF_IPC_GLB_TRACE_MSG):
> +		found = ipc_glb_trace_message_compare(msg, header,
> tx_data);
> +	default:
> +		found = 0;;
> +	}
> +

Btw, this switch() is missing break statements.

> +	if (found)
> +		return msg;
> +
> +not_found:
> +	return NULL;
> +}
>  
>  int ipc_queue_host_message(struct ipc *ipc, uint32_t header,
>  	void *tx_data, size_t tx_bytes, void *rx_data,
> -	size_t rx_bytes, void (*cb)(void*, void*), void *cb_data)
> +	size_t rx_bytes, void (*cb)(void*, void*), void *cb_data,
> uint32_t replace)
>  {
> -	struct ipc_msg *msg;
> -	uint32_t flags;
> +	struct ipc_msg *msg = NULL;
> +	uint32_t flags, found = 0;
>  	int ret = 0;
>  
>  	spin_lock_irq(&ipc->lock, flags);
>  
> -	/* get a free message */
> -	msg = msg_get_empty(ipc);
> +	/* do we need to replace an existing message? */
> +	if (replace)
> +		msg = msg_find(ipc, header, tx_data);
> +
> +	/* do we need to use a new empty message? */
> +	if (msg)
> +		found = 1;
> +	else
> +		msg = msg_get_empty(ipc);
> +
>  	if (msg == NULL) {
>  		trace_ipc_error("eQb");
>  		ret = -EBUSY;
> @@ -929,9 +1008,11 @@ int ipc_queue_host_message(struct ipc *ipc,
> uint32_t header,
>  	if (tx_bytes > 0 && tx_bytes < SOF_IPC_MSG_MAX_SIZE)
>  		rmemcpy(msg->tx_data, tx_data, tx_bytes);
>  
> -	/* now queue the message */
> -	ipc->dsp_pending = 1;
> -	list_item_append(&msg->list, &ipc->msg_list);
> +	if (!found) {
> +		/* now queue the message */
> +		ipc->dsp_pending = 1;
> +		list_item_append(&msg->list, &ipc->msg_list);
> +	}
>  
>  out:
>  	spin_unlock_irq(&ipc->lock, flags);