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

[Jie, Yang]

>-----Original Message-----
>From: sound-open-firmware-bounces at alsa-project.org [mailto:sound-open-
>firmware-bounces at alsa-project.org] On Behalf Of Liam Girdwood
>Sent: Tuesday, December 5, 2017 6:00 PM
>To: yan.wang at linux.intel.com; sound-open-firmware at 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 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, 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 at alsa-project.org
>http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware