[alsa-devel] [PATCH 4/7] ASoC: Intel: mrfld - add the dsp sst driver

[Mark Brown]

On Fri, Jul 18, 2014 at 07:43:59PM +0530, Vinod Koul wrote:
> On Fri, Jul 18, 2014 at 12:35:48PM +0100, Mark Brown wrote:
> > On Wed, Jul 09, 2014 at 02:57:52PM +0530, Vinod Koul wrote:

> > Have you looked at the mailbox framework (not merged yet but hopefully
> > getting close to it)?

Not seeing an answer to this...

> > > +	if (list_empty(&drv->rx_list))
> > > +		return IRQ_HANDLED;

> > Why would the IRQ thread be running with nothing to do, and if it can
> > be...

> Cant think of, we need to remove this!

> > > +	spin_lock_irqsave(&drv->rx_msg_lock, irq_flags);
> > > +	list_for_each_entry_safe(msg, __msg, &drv->rx_list, node) {

> > ...won't we handle that gracefully anyway?

> Are you referring to lock + irqsave?

This is part of the thing about the list_empty() check being redundant.

> > > +static inline int sst_pm_runtime_put(struct intel_sst_drv *sst_drv)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = pm_runtime_put_sync(sst_drv->dev);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	atomic_dec(&sst_drv->pm_usage_count);
> > > +
> > > +	pr_debug("%s: count is %d now..\n", __func__,
> > > +			atomic_read(&sst_drv->pm_usage_count));
> > > +	return 0;
> > > +}

> > I'm not entirely clear why this is here but if it's to provide trace
> > just instrument the runtime PM core.

> Well the only usage of pm_usage_count locally is to check in runtime pm
> suspend to S3 transitions. When we are runtime suspended and S3 triggers,
> the device usage count goes up to 1. Then runtime resume and classic suspend
> is invoked in that order. So if we are in this scenario we can't rely on
> checks using frameworks count.

Yes, you can.  You can check to see if the device is runtime suspended
and really this isn't at all specific to this driver - it's a general
thing that affects lots of drivers so shouldn't be open coded in this
one.  IIRC you're looking for pm_runtime_is_suspended().

Essentially any time you need to use atomic variables in a driver you're
probably doing something wrong.

> > > +static inline unsigned int sst_assign_pvt_id(struct intel_sst_drv *sst_drv_ctx)
> > > +{
> > > +	unsigned int local;
> > > +
> > > +	spin_lock(&sst_drv_ctx->block_lock);
> > > +	sst_drv_ctx->pvt_id++;
> > > +	if (sst_drv_ctx->pvt_id > MAX_BLOCKS)
> > > +		sst_drv_ctx->pvt_id = 1;
> > > +	local = sst_drv_ctx->pvt_id;
> > > +	spin_unlock(&sst_drv_ctx->block_lock);
> > > +	return local;
> > > +}

> > I would expect this to be checking to see if the IDs are free but it
> > just hands them out in sequence?  There appears to be an array of
> > streams statically allocated, why not look there for an unused one?

> we use these incrementally. The driver stamps each IPC to DSP with a number.
> The size is limited by bits in the IPCs. So we keep going till we hit max,
> that time we set it back to 1

> These ids are not freed or allocated as IPC latency is small enough and IPCs
> less. We haven't hot scenarios where we go complete round but IPC is still
> pending :)

Yet.

> > > +int free_stream_context(unsigned int str_id)
> > > +{
> > > +	struct stream_info *stream;
> > > +	int ret = 0;
> > > +
> > > +	stream = get_stream_info(str_id);
> > > +	if (stream) {

> > It's a bit scary that this might be used with an invalid ID - what
> > happens if the ID got reallocated?

> The free routine checks for valid ID first

That's not the point - the point is that if you're freeing what might be
an invalid ID I don't see any guarantee that the ID hasn't been
allocated to something else.  It's different if there's a specific ID
that can be used for an invalid ID but this looks like it's working
around double frees.

> > > +	while (header.p.header_high.part.busy) {
> > > +		if (loop_count > 10) {
> > > +			pr_err("sst: Busy wait failed, cant send this msg\n");
> > > +			retval = -EBUSY;
> > > +			goto out;
> > > +		}
> > > +		udelay(500);
> > > +		loop_count++;
> > > +		header.full = sst_shim_read64(sst_drv_ctx->shim, SST_IPCX);
> > > +	}

> > Throw a cpu_relax() in there if we need to spin...

> its more busy wait for the bit to be freed. Since IPC latency is supposed to
> be minimal checking like this helps to minimize.

It's a udelay(500) which is pretty long for a busy wait...

> > > +	if (!list_empty(&sst_drv_ctx->memcpy_list)) {
> > > +		list_for_each_entry_safe(listnode, tmplistnode,
> > > +				&sst_drv_ctx->memcpy_list, memcpylist) {
> > > +			list_del(&listnode->memcpylist);
> > > +			kfree(listnode);
> > > +		}
> > > +	}
> > > +	sst_memcpy_free_lib_resources();
> > > +}

> > I'm having a hard time seeing why we don't just copy the data as we go
> > rather than allocating this list?  It seems to just be making the code
> > more complex.

> because we support DMA too. so common parsing and then later we either do
> memcpy and dma. Will push DMA bits after this series.

Keep it simple for now and just memcpy() directly, leaving the functions
that select I/O vs memcpy() - you can always go back and add the DMA
later.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140718/04cbdff9/attachment.sig>