[Sound-open-firmware] [PATCH v2] Add support to replace stale stream/trace position updates.
yanwang
yan.wang at linux.intel.com
Tue Dec 5 07:38:10 CET 2017
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.
>
> >
> > >
> > > 1. 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?
> >
> > >
> > > 2. Add msg_find() to search duplicate message.
> > > 3. 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.
>
> >
> >
> > >
> > > 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.
> >
> > 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 at 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 at alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmwar
> > e
> > _______________________________________________
> > Sound-open-firmware mailing list
> > Sound-open-firmware at alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmwar
> > e
> >
>
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
More information about the Sound-open-firmware
mailing list