[Sound-open-firmware] [PATCH v5] Add support to replace stale stream/trace position updates.
From: Yan Wang yan.wang@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.
Signed-off-by: Yan Wang yan.wang@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) +{ + 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; + + 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) +{ + 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; + + 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;; + } + + 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);
On Wed, 2017-12-06 at 18:45 +0800, yan.wang@linux.intel.com wrote:
From: Yan Wang yan.wang@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@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);
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@linux.intel.com wrote:
From: Yan Wang yan.wang@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@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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
sound-open-firmware@alsa-project.org
-
Liam Girdwood -
yan.wang -
yan.wang＠linux.intel.com