[Sound-open-firmware] [PATCH v4] Add support to replace stale stream/trace position updates.

[yanwang]

Hi, Liam,
	I have refined it based on your comments as v5.
	Could you please review it?
	Thanks.

Yan Wang

On Tue, 2017-12-05 at 21:32 +0000, Liam Girdwood wrote:
> On Tue, 2017-12-05 at 18:40 +0800, yan.wang at linux.intel.com wrote:
> > 
> > From: Yan Wang <yan.wang at 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 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, 1);
> >  }
> >  
> >  /* send stream position TODO: send compound message  */
> > @@ -379,7 +379,7 @@ int ipc_stream_send_xrun(struct comp_dev *cdev,
> >  	posn->comp_id = cdev->comp.id;
> >  
> >  	return ipc_queue_host_message(_ipc, posn->rhdr.hdr.cmd,
> > posn,
> > -		sizeof(*posn), NULL, 0, NULL, NULL);
> > +		sizeof(*posn), NULL, 0, NULL, NULL, 1);
> >  }
> >  
> >  static int ipc_stream_trigger(uint32_t header)
> > @@ -644,7 +644,7 @@ int ipc_dma_trace_send_position(void)
> >  	posn.rhdr.hdr.size = sizeof(posn);
> >  
> >  	return ipc_queue_host_message(_ipc, posn.rhdr.hdr.cmd,
> > &posn,
> > -		sizeof(posn), NULL, 0, NULL, NULL);
> > +		sizeof(posn), NULL, 0, NULL, NULL, 1);
> >  }
> >  
> >  static int ipc_glb_debug_message(uint32_t header)
> > @@ -899,19 +899,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)
> 
> I think we also need to check struct sof_ipc_stream_posn.comp_id here
> too and check if the comp IDs match.
> 
> I think all the calls to this func are passing a struct
> sof_ipc_stream_pos as the data, but we may need a simple switch
> statement that checks cmd type and determines the correct msg_find()
> to
> use (as future users of this feature may send different structures as
> data).
> 
> Liam
> 
> > 
> > +{
> > +	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-firmware