Re: [RFC 2/9] ASoC: hda: Add IPC library for SKL platform

[Vinod Koul <vinod.koul@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3110 bytes --]

On Mon, Apr 20, 2015 at 10:56:39PM +0100, Mark Brown wrote:
> On Fri, Apr 17, 2015 at 06:46:00PM +0530, Vinod Koul wrote:
> 
> > +
> > +/* IPC data */
> > +struct ssth_ipc {
> > +	struct device *dev;
> > +	struct ssth_lib *dsp;
> > +
> > +/* IPC messaging */
> > +	struct list_head tx_list;
> 
> Lots of odd indentation of comments in this code (well, several examples
> I noticed so far anyway).
> 
> > +	if (ret == 0)
> > +		ret = -ETIMEDOUT;
> > +	else {
> > +	/* copy the data returned from DSP */
> 
> Coding style - { } on both sides of the if (and the comments again).
Ah sorry about that, I did try to fix style issues in code but looks like
few were missed. I will fix them in the patch series.
> 
> > +		if (msg->rx_size) {
> > +			if (rx_data)
> > +				memcpy(rx_data, msg->rx_data, msg->rx_size);
> > +			else
> > +				dev_err(ipc->dev, "error: no output buffer");
> > +		}
> > +		ret = msg->errno;
> > +	}
> > +
> > +	ssth_ipc_msg_put_empty(ipc, msg);
> > +
> > +	spin_unlock_irqrestore(&ipc->ipc_lock, irq_flags);
> 
> Can we pop the message off the list, release the lock and then copy?
> That way we can avoid having interrupts disabled while we do the
> memcpy().  In general there seems to be a lot of interrupts disabled
> copying going on (which is there for some of the other drivers too)
> which might be avoidable.
Thanks for pointing, yes this should be done.
> 
> > +static void ssth_ipc_reply_remove(struct ssth_ipc *ipc, struct ssth_ipc_message *msg)
> > +{
> > +	unsigned long irq_flags;
> > +
> > +	spin_lock_irqsave(&ipc->ipc_lock, irq_flags);
> > +	if (list_empty(&ipc->rx_list)) {
> > +		dev_dbg(ipc->dev, "empty rx list");
> > +		goto out;
> > +	}
> > +	list_del(&msg->list);
> > +
> > +out:
> > +	spin_unlock_irqrestore(&ipc->ipc_lock, irq_flags);
> > +}
> 
> Are we expecting to not find the message/
For reply case, I am not sure. I think if thats true would make sense to
complain loudly..

> 
> > +	if (IPC_GLB_NOTIFI_MSG_TYPE(header.primary)) {
> > +		switch (IPC_GLB_NOTIFI_TYPE(header.primary)) {
> > +		case IPC_GLB_NOTIFCATION_GLITCH:
> > +			break;
> > +		case IPC_GLB_NOTIFCATION_OVERRUN:
> > +			break;
> > +		case IPC_GLB_NOTIFCATION_UNDERRUN:
> > +			dev_dbg(ipc->dev, "FW UNDERRUN\n");
> > +			break;
> > +		case IPC_GLB_NOTIFCATION_END_STREAM:
> > +			break;
> > +		case IPC_GLB_NOTIFCATION_PHRASE_DETECTED:
> > +			break;
> > +		case IPC_GLB_NOTIFCATION_RESOURCE_EVENT:
> > +			dev_dbg(ipc->dev, "MCPS Budget Violation\n");
> > +			break;
> > +		case IPC_GLB_NOTIFCATION_LOG_BUFFER_STATUS:
> > +			break;
> > +		case IPC_GLB_NOTIFCATION_TIMESTAMP_CAPTURED:
> > +			break;
> > +		case IPC_GLB_NOTIFCATION_FW_READY:
> > +			ipc->boot_complete = true;
> > +			wake_up(&ipc->boot_wait);
> > +			break;
> 
> More prints perhaps?  _PHRASE_DETECTED looks interesting, as does
> _OVERRUN.
Overrun is FW detectiong its pipelines are overunning the buffer, no the
ALSA ring buffer though. _PHRASE_DETECTED is for keyword detection which can
run on dsp

Thanks
-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]