[Sound-open-firmware] [PATCH v2] 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.
If there is un-sent IPC message of host offset of DMA trace in host message queue, update it with the latest host offset directly instead of appending one new IPC message.
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);
On 12/1/17 2:19 AM, yan.wang@linux.intel.com wrote:
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.
Can you explain under what circumstances there would be duplicate messages? is this a feature that both host and DSP can use or an error case?
- Add msg_find() to search duplicate message.
- If replace flag is true, search and replace duplicate message.
Who sets this flag and why?
- For the message of host offset of DMA trace, enable replace logic.
If there is un-sent IPC message of host offset of DMA trace in host message queue, update it with the latest host offset directly instead of appending one new IPC message.
that seems to be a different feature intended at optimizing the number of IPC messages?
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);
Hi, Pierre, Please see the following: Thanks.
Yan Wang
From: Pierre-Louis Bossart Date: 2017-12-01 22:23 To: yan.wang; sound-open-firmware Subject: Re: [Sound-open-firmware] [PATCH v2] Add support to replace stale stream/trace position updates. On 12/1/17 2:19 AM, yan.wang@linux.intel.com wrote:
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.
Can you explain under what circumstances there would be duplicate messages? is this a feature that both host and DSP can use or an error case?
- Add msg_find() to search duplicate message.
- If replace flag is true, search and replace duplicate message.
Who sets this flag and why?
[Yan] The caller of ipc_queue_host_message() will set this flag. In fact, it is dependent on IPC message type. Currently host offset IPC message of DMA trace will set it to true. But stream IPC message of DMA trace will set it to false.
- For the message of host offset of DMA trace, enable replace logic.
If there is un-sent IPC message of host offset of DMA trace in host message queue, update it with the latest host offset directly instead of appending one new IPC message.
that seems to be a different feature intended at optimizing the number of IPC messages?
[Yan] It is not only for optimizaing the number of IPC messages but also send the latest host offset as soon as possible for notifying debugfs output.
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);
_______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
- Add "replace" parameter for ipc_queue_host_message() to indicate whether
check duplicate message in host message queue.
Can you explain under what circumstances there would be duplicateIs the idea behind the contribution sound?
Is the contribution architected correctly? Is the contribution polished?
messages? is this a feature that both host and DSP can use or an error case?
- Add msg_find() to search duplicate message.
- If replace flag is true, search and replace duplicate message.
Who sets this flag and why?
[Yan] The caller of ipc_queue_host_message() will set this flag. In fact, it is dependent on IPC message type. Currently host offset IPC message of DMA trace will set it to true. But stream IPC message of DMA trace will set it to false.
So do I get it right that you have two kinds of IPC messages 1. regular ones that are pushed into the queue 2. specific ones where you want to update the information provided in similar messages already queued so that the host gets the latest information
That should be the information you provide first as context.
In general it helps if you state in this order a. what the problem statement is b. what the suggested solution is (at a high-level) c. what specific implementation you chose.
I encourage you to read the blurb at http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ I personally like the idea of reviewing in steps: 1.Is the idea behind the contribution sound? 2.Is the contribution architected correctly? 3.Is the contribution polished?
and conversely to present the patches so that this logical process is clear.
Nobody's perfect and I get yelled at by Linux overlords, but if you can try to place yourself in the shoes of a reviewer who is not familiar with the details of your contribution - the trace in this example - it'd greatly help. And the review comments are provided to help, not to blame.
- For the message of host offset of DMA trace, enable replace logic.
If there is un-sent IPC message of host offset of DMA trace in host message queue, update it with the latest host offset directly instead of appending one new IPC message.
that seems to be a different feature intended at optimizing the number of IPC messages?
[Yan] It is not only for optimizaing the number of IPC messages but also send the latest host offset as soon as possible for notifying debugfs output.
and that doesn't come across, if you can explain it by first stating the problem then describing the solution you'll get fewer questions during the reviews.
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);
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
From: Pierre-Louis Bossart Date: 2017-12-02 03:18 To: yan.wang; sound-open-firmware Subject: Re: [Sound-open-firmware] [PATCH v2] Add support to replace stale stream/trace position updates.
- Add "replace" parameter for ipc_queue_host_message() to indicate whether
check duplicate message in host message queue.
Can you explain under what circumstances there would be duplicateIs the idea behind the contribution sound?
Is the contribution architected correctly? Is the contribution polished?
messages? is this a feature that both host and DSP can use or an error case?
- Add msg_find() to search duplicate message.
- If replace flag is true, search and replace duplicate message.
Who sets this flag and why?
[Yan] The caller of ipc_queue_host_message() will set this flag. In fact, it is dependent on IPC message type. Currently host offset IPC message of DMA trace will set it to true. But stream IPC message of DMA trace will set it to false.
So do I get it right that you have two kinds of IPC messages 1. regular ones that are pushed into the queue 2. specific ones where you want to update the information provided in similar messages already queued so that the host gets the latest information
That should be the information you provide first as context.
In general it helps if you state in this order a. what the problem statement is b. what the suggested solution is (at a high-level) c. what specific implementation you chose.
I encourage you to read the blurb at http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ I personally like the idea of reviewing in steps: 1.Is the idea behind the contribution sound? 2.Is the contribution architected correctly? 3.Is the contribution polished?
and conversely to present the patches so that this logical process is clear.
Nobody's perfect and I get yelled at by Linux overlords, but if you can try to place yourself in the shoes of a reviewer who is not familiar with the details of your contribution - the trace in this example - it'd greatly help. And the review comments are provided to help, not to blame.
[Yan] Thanks for your comments. I will read the URL link and keep improving my patch for reading and understanding easily.
- For the message of host offset of DMA trace, enable replace logic.
If there is un-sent IPC message of host offset of DMA trace in host message queue, update it with the latest host offset directly instead of appending one new IPC message.
that seems to be a different feature intended at optimizing the number of IPC messages?
[Yan] It is not only for optimizaing the number of IPC messages but also send the latest host offset as soon as possible for notifying debugfs output.
and that doesn't come across, if you can explain it by first stating the problem then describing the solution you'll get fewer questions during the reviews.
[Yan] Agree totally. I will follow this in the future. Thanks again.
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);
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
Hi, Pierre, About the following question, I hadn't noticed it previously. Sorry. Just IMHO, The idea behind the contribution is sound. The patch is architected correctly.
Can you explain under what circumstances there would be duplicate
Is the idea behind the contribution sound?
Is the contribution
architected correctly?
Is the contribution polished?> > > messages?
is this a feature that both host and DSP can use or an
> error case?
I have rewritten the commit description based on your menttioned policy as v3. Could please review it? Thanks.
Yan Wang
On Sat, 2017-12-02 at 11:01 +0800, yan.wang wrote:
From: Pierre-Louis Bossart Date: 2017-12-02 03:18 To: yan.wang; sound-open-firmware Subject: Re: [Sound-open-firmware] [PATCH v2] Add support to replace stale stream/trace position updates.
- Add "replace" parameter for ipc_queue_host_message() to
indicate whether check duplicate message in host message queue.
Can you explain under what circumstances there would be duplicateIs the idea behind the contribution sound?
Is the contribution architected correctly? Is the contribution polished?
messages? is this a feature that both host and DSP can use or an error case?
- Add msg_find() to search duplicate message.
- If replace flag is true, search and replace duplicate message.
Who sets this flag and why?
[Yan] The caller of ipc_queue_host_message() will set this flag. In fact, it is dependent on IPC message type. Currently host offset IPC message of DMA trace will set it to true. But stream IPC message of DMA trace will set it to false.
So do I get it right that you have two kinds of IPC messages
- regular ones that are pushed into the queue
- specific ones where you want to update the information provided
in similar messages already queued so that the host gets the latest information That should be the information you provide first as context. In general it helps if you state in this order a. what the problem statement is b. what the suggested solution is (at a high-level) c. what specific implementation you chose. I encourage you to read the blurb at http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ I personally like the idea of reviewing in steps: 1.Is the idea behind the contribution sound? 2.Is the contribution architected correctly? 3.Is the contribution polished? and conversely to present the patches so that this logical process is clear. Nobody's perfect and I get yelled at by Linux overlords, but if you can try to place yourself in the shoes of a reviewer who is not familiar with the details of your contribution - the trace in this example - it'd greatly help. And the review comments are provided to help, not to blame.
[Yan] Thanks for your comments. I will read the URL link and keep improving my patch for reading and understanding easily.
- For the message of host offset of DMA trace, enable replace
logic.
If there is un-sent IPC message of host offset of DMA trace in host message queue, update it with the latest host offset directly instead of appending one new IPC message.
that seems to be a different feature intended at optimizing the number of IPC messages?
[Yan] It is not only for optimizaing the number of IPC messages but also send the latest host offset as soon as possible for notifying debugfs output.
and that doesn't come across, if you can explain it by first stating the problem then describing the solution you'll get fewer questions during the reviews.
[Yan] Agree totally. I will follow this in the future. Thanks again.
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);
_______________________________________________ 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-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@alsa-project.org
-
Pierre-Louis Bossart -
yan.wang -
yan.wang＠linux.intel.com
-
yanwang