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

[yan.wang]

Hi, Liam,
    Sorry for my some missing.
    I will chnage it based on your comments and submit again.
    Thanks.

Yan Wang



 
From: Liam Girdwood
Date: 2017-12-06 19:36
To: yan.wang; sound-open-firmware
Subject: Re: [Sound-open-firmware] [PATCH v5] Add support to replace stale stream/trace position updates.
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);
_______________________________________________
Sound-open-firmware mailing list
Sound-open-firmware at alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware