[Sound-open-firmware] [PATCH] Add support to replace stale stream/trace position updates.
From: Yan Wang yan.wang@linux.intel.com
1. Add "replace" parameter for ipc_queue_host_message() to indicate whether check duplicate message in host message queue. 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.
It will avoid that the host message queue is full because there is too many IPC messages of host offset of DMA trace.
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);
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of yan.wang@linux.intel.com Sent: Thursday, November 23, 2017 10:48 AM To: sound-open-firmware@alsa-project.org Cc: Yan Wang yan.wang@linux.intel.com Subject: [Sound-open-firmware] [PATCH] Add support to replace stale stream/trace position updates.
From: Yan Wang yan.wang@linux.intel.com
- Add "replace" parameter for ipc_queue_host_message() to indicate whether
check duplicate message in host message queue. 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.
It will avoid that the host message queue is full because there is too many IPC messages of host offset of DMA trace.
Does the "duplicate" message are true duplicated? IMO, we should not send duplicate message except that it is critical/urgent and no respond for it from host side.
If it is meaning that the DMA trace sending too much messages(with too high frequency), we should reduce the frequency, instead of adding message replacing logic which makes it more complicate and error-prone.
Thanks, ~Keyon
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
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Thu, 2017-11-23 at 03:07 +0000, Jie, Yang wrote:
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound- open- firmware-bounces@alsa-project.org] On Behalf Of yan.wang@linux.inte l.com Sent: Thursday, November 23, 2017 10:48 AM To: sound-open-firmware@alsa-project.org Cc: Yan Wang yan.wang@linux.intel.com Subject: [Sound-open-firmware] [PATCH] Add support to replace stale stream/trace position updates.
From: Yan Wang yan.wang@linux.intel.com
- Add "replace" parameter for ipc_queue_host_message() to indicate
whether check duplicate message in host message queue. 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.
It will avoid that the host message queue is full because there is too many IPC messages of host offset of DMA trace.
Does the "duplicate" message are true duplicated? IMO, we should not send duplicate message except that it is critical/urgent and no respond for it from host side.
If it is meaning that the DMA trace sending too much messages(with too high frequency), we should reduce the frequency, instead of adding message replacing logic which makes it more complicate and error-prone.
Thanks, ~Keyon
Host offset of DMA trace need be updated after DMA transfer completes. This patch wants solve: If previous update message of host offset hasn't been sent because of many the other messages, set the latest host offset for it. "duplicate" may cause some misunderstanding, I could add more description into commit message. Thanks.
Yan Wang
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
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmwar e
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On 2017年11月23日 13:27, yanwang wrote:
On Thu, 2017-11-23 at 03:07 +0000, Jie, Yang wrote:
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound- open- firmware-bounces@alsa-project.org] On Behalf Of yan.wang@linux.inte l.com Sent: Thursday, November 23, 2017 10:48 AM To: sound-open-firmware@alsa-project.org Cc: Yan Wang yan.wang@linux.intel.com Subject: [Sound-open-firmware] [PATCH] Add support to replace stale stream/trace position updates.
From: Yan Wang yan.wang@linux.intel.com
- Add "replace" parameter for ipc_queue_host_message() to indicate
whether check duplicate message in host message queue. 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.
It will avoid that the host message queue is full because there is too many IPC messages of host offset of DMA trace.
Does the "duplicate" message are true duplicated? IMO, we should not send duplicate message except that it is critical/urgent and no respond for it from host side.
If it is meaning that the DMA trace sending too much messages(with too high frequency), we should reduce the frequency, instead of adding message replacing logic which makes it more complicate and error-prone.
Thanks, ~Keyon
Host offset of DMA trace need be updated after DMA transfer completes. This patch wants solve: If previous update message of host offset hasn't been sent because of many the other messages, set the latest host offset for it. "duplicate" may cause some misunderstanding, I could add more description into commit message. Thanks.
OK, that make sense.
So if there are several host offset update messages in the queue, we will only replace one(the most nearly to the head one) of it, is this the intention?
Another question is, for some kind of IPC message, e.g. struct sof_ipc_stream_posn, we should merge only the messages with same header plus same other member(e.g. posn->comp_id), which means they are same kind of messages from same compononents ---- e.g. we should not merge host position update to dai position update messages.
For this case, the msg_find() function may need to be modified.
Thanks, ~Keyon
Yan Wang
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
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmwar e
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Thu, 2017-11-23 at 17:06 +0800, Keyon Jie wrote:
On 2017年11月23日 13:27, yanwang wrote:
On Thu, 2017-11-23 at 03:07 +0000, Jie, Yang wrote:
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound- open- firmware-bounces@alsa-project.org] On Behalf Of yan.wang@linux. inte l.com Sent: Thursday, November 23, 2017 10:48 AM To: sound-open-firmware@alsa-project.org Cc: Yan Wang yan.wang@linux.intel.com Subject: [Sound-open-firmware] [PATCH] Add support to replace stale stream/trace position updates.
From: Yan Wang yan.wang@linux.intel.com
- Add "replace" parameter for ipc_queue_host_message() to
indicate whether check duplicate message in host message queue. 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.
It will avoid that the host message queue is full because there is too many IPC messages of host offset of DMA trace.
Does the "duplicate" message are true duplicated? IMO, we should not send duplicate message except that it is critical/urgent and no respond for it from host side.
If it is meaning that the DMA trace sending too much messages(with too high frequency), we should reduce the frequency, instead of adding message replacing logic which makes it more complicate and error-prone.
Thanks, ~Keyon
Host offset of DMA trace need be updated after DMA transfer completes. This patch wants solve: If previous update message of host offset hasn't been sent because of many the other messages, set the latest host offset for it. "duplicate" may cause some misunderstanding, I could add more description into commit message. Thanks.
OK, that make sense.
So if there are several host offset update messages in the queue, we will only replace one(the most nearly to the head one) of it, is this the intention?
Yes. The first found message should be sent at first. It will notify degbufs the latest offset. It will output available trace data as possible. In fact, it may be less possible that there are multiply trace messages because of current time intervals (500ms) and buffer size. I have not meeted this situation in my BYT MB platform. But it should be possible in some extreme situation e.g. IPC timeout. I could change it and make host offset of all trace mesage are updated to the latest value.
Another question is, for some kind of IPC message, e.g. struct sof_ipc_stream_posn, we should merge only the messages with same header plus same other member(e.g. posn->comp_id), which means they are same kind of messages from same compononents ---- e.g. we should not merge host position update to dai position update messages.
For this case, the msg_find() function may need to be modified.
Yes. Current impelemntation doesn't know the meaning of the message and just comparing with header. So if need more complex logic, need modify it.
Thanks, ~Keyon
Yan Wang
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
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-fir mwar e
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmw are
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmwar e
Hi, Keyon, Previou replying may be wrong about multiply trace messages. Please see the following:
On Thu, 2017-11-23 at 18:10 +0800, yanwang wrote:
On Thu, 2017-11-23 at 17:06 +0800, Keyon Jie wrote:
On 2017年11月23日 13:27, yanwang wrote:
On Thu, 2017-11-23 at 03:07 +0000, Jie, Yang wrote:
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound- open- firmware-bounces@alsa-project.org] On Behalf Of yan.wang@linu x. inte l.com Sent: Thursday, November 23, 2017 10:48 AM To: sound-open-firmware@alsa-project.org Cc: Yan Wang yan.wang@linux.intel.com Subject: [Sound-open-firmware] [PATCH] Add support to replace stale stream/trace position updates.
From: Yan Wang yan.wang@linux.intel.com
- Add "replace" parameter for ipc_queue_host_message() to
indicate whether check duplicate message in host message queue. 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.
It will avoid that the host message queue is full because there is too many IPC messages of host offset of DMA trace.
Does the "duplicate" message are true duplicated? IMO, we should not send duplicate message except that it is critical/urgent and no respond for it from host side.
If it is meaning that the DMA trace sending too much messages(with too high frequency), we should reduce the frequency, instead of adding message replacing logic which makes it more complicate and error-prone.
Thanks, ~Keyon
Host offset of DMA trace need be updated after DMA transfer completes. This patch wants solve: If previous update message of host offset hasn't been sent because of many the other messages, set the latest host offset for it. "duplicate" may cause some misunderstanding, I could add more description into commit message. Thanks.
OK, that make sense.
So if there are several host offset update messages in the queue, we will only replace one(the most nearly to the head one) of it, is this the intention?
Yes. The first found message should be sent at first. It will notify degbufs the latest offset. It will output available trace data as possible. In fact, it may be less possible that there are multiply trace messages because of current time intervals (500ms) and buffer size. I have not meeted this situation in my BYT MB platform. But it should be possible in some extreme situation e.g. IPC timeout. I could change it and make host offset of all trace mesage are updated to the latest value.
Because trace message is always searched at first before try to add into queue, it is impossible that there are more one trace message in queue now. So it is unnecessary to search the queue list until its end. Is it right?
Thanks. Yan Wang
Another question is, for some kind of IPC message, e.g. struct sof_ipc_stream_posn, we should merge only the messages with same header plus same other member(e.g. posn->comp_id), which means they are same kind of messages from same compononents ---- e.g. we should not merge host position update to dai position update messages.
For this case, the msg_find() function may need to be modified.
Yes. Current impelemntation doesn't know the meaning of the message and just comparing with header. So if need more complex logic, need modify it.
Thanks, ~Keyon
Yan Wang
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
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-f ir mwar e
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-fir mw are
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmw ar e
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 -
Keyon Jie -
yan.wang＠linux.intel.com -
yanwang