[Sound-open-firmware] [PATCH v3] 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 meesage may haven't been sent. It is better to update the previous same type IPC message instead of sending 2 messages because it not only reduces the waiting number of host message queue but also send the latest information as soon as possible. The following is details of implementation: 1. Add "replace" parameter for ipc_queue_host_message() to indicate whether check duplicate message in host message queue which is decided by sender. 2. Add msg_find() to search duplicate message. 3. If replace flag is true, search and replace duplicate message. 4. For the message of host offset of DMA trace, enable replace logic. Signed-off-by: Yan Wang <yan.wang@linux.intel.com> --- src/include/reef/ipc.h | 2 +- src/ipc/intel-ipc.c | 45 ++++++++++++++++++++++++++++++++++----------- 2 files changed, 35 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..391907e 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, 0); } /* 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, 0); } 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,40 @@ static inline struct ipc_msg *msg_get_empty(struct ipc *ipc) return msg; } +static inline struct ipc_msg *msg_find(struct ipc *ipc, uint32_t header) +{ + struct list_item *plist; + struct ipc_msg *msg = NULL; + + list_for_item(plist, &ipc->msg_list) { + msg = container_of(plist, struct ipc_msg, list); + if (msg->header == header) + return msg; + } + + 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); + + /* 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 +950,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); -- 2.7.4
On Tue, 2017-12-05 at 14:38 +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 meesage may haven't been sent. It is better to update the previous same type IPC message instead of sending 2 messages because it not only reduces the waiting number of host message queue but also send the latest information as soon as possible.
The following is details of implementation: 1. Add "replace" parameter for ipc_queue_host_message() to indicate whether check duplicate message in host message queue which is decided by sender. 2. Add msg_find() to search duplicate message. 3. If replace flag is true, search and replace duplicate message. 4. For the message of host offset of DMA trace, enable replace logic.
Signed-off-by: Yan Wang <yan.wang@linux.intel.com> --- src/include/reef/ipc.h | 2 +- src/ipc/intel-ipc.c | 45 ++++++++++++++++++++++++++++++++++---- ------- 2 files changed, 35 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..391907e 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, 0);
Should replace == 1 here since this is stream position ?
}
/* 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, 0);
I would also replace old xrun messages with new ones.
}
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,40 @@ static inline struct ipc_msg *msg_get_empty(struct ipc *ipc) return msg; }
+static inline struct ipc_msg *msg_find(struct ipc *ipc, uint32_t header) +{ + struct list_item *plist; + struct ipc_msg *msg = NULL; + + list_for_item(plist, &ipc->msg_list) { + msg = container_of(plist, struct ipc_msg, list); + if (msg->header == header) + return msg; + } + + 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); + + /* 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 +950,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);
Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Hi, Liam, I changed it based on your comments as v4. Thanks. Yan Wang On Tue, 2017-12-05 at 09:59 +0000, Liam Girdwood wrote:
On Tue, 2017-12-05 at 14:38 +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 meesage may haven't been sent. It is better to update the previous same type IPC message instead of sending 2 messages because it not only reduces the waiting number of host message queue but also send the latest information as soon as possible.
The following is details of implementation: 1. Add "replace" parameter for ipc_queue_host_message() to indicate whether check duplicate message in host message queue which is decided by sender. 2. Add msg_find() to search duplicate message. 3. If replace flag is true, search and replace duplicate message. 4. For the message of host offset of DMA trace, enable replace logic.
Signed-off-by: Yan Wang <yan.wang@linux.intel.com> --- src/include/reef/ipc.h | 2 +- src/ipc/intel-ipc.c | 45 ++++++++++++++++++++++++++++++++++---- ------- 2 files changed, 35 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..391907e 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, 0);
Should replace == 1 here since this is stream position ?
} /* 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, 0);
I would also replace old xrun messages with new ones.
} 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,40 @@ static inline struct ipc_msg *msg_get_empty(struct ipc *ipc) return msg; } +static inline struct ipc_msg *msg_find(struct ipc *ipc, uint32_t header) +{ + struct list_item *plist; + struct ipc_msg *msg = NULL; + + list_for_item(plist, &ipc->msg_list) { + msg = container_of(plist, struct ipc_msg, list); + if (msg->header == header) + return msg; + } + + 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); + + /* 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 +950,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);
--------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of Liam Girdwood Sent: Tuesday, December 5, 2017 6:00 PM To: yan.wang@linux.intel.com; sound-open-firmware@alsa-project.org Subject: Re: [Sound-open-firmware] [PATCH v3] Add support to replace stale stream/trace position updates.
On Tue, 2017-12-05 at 14:38 +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 meesage may haven't been sent. It is better to update the previous same type IPC message instead of sending 2 messages because it not only reduces the waiting number of host message queue but also send the latest information as soon as possible.
The following is details of implementation: 1. Add "replace" parameter for ipc_queue_host_message() to indicate whether check duplicate message in host message queue which is decided by sender. 2. Add msg_find() to search duplicate message. 3. If replace flag is true, search and replace duplicate message. 4. For the message of host offset of DMA trace, enable replace logic.
Signed-off-by: Yan Wang <yan.wang@linux.intel.com> --- src/include/reef/ipc.h | 2 +- src/ipc/intel-ipc.c | 45 ++++++++++++++++++++++++++++++++++---- ------- 2 files changed, 35 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..391907e 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, 0);
Should replace == 1 here since this is stream position ?
Please be careful about this as it may belong to different stream. Thanks, ~Keyon
}
/* 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, 0);
I would also replace old xrun messages with new ones.
}
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,40 @@ static inline struct ipc_msg *msg_get_empty(struct ipc *ipc) return msg; }
+static inline struct ipc_msg *msg_find(struct ipc *ipc, uint32_t header) +{ + struct list_item *plist; + struct ipc_msg *msg = NULL; + + list_for_item(plist, &ipc->msg_list) { + msg = container_of(plist, struct ipc_msg, list); + if (msg->header == header) + return msg; + } + + 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); + + /* 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 +950,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);
Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (4)
-
Jie, Yang -
Liam Girdwood -
yan.wang＠linux.intel.com -
yanwang