[alsa-devel] [PATCH] Sound: MSM soc : imported alsa for the MSM from codeaurora

[Mark Brown]

On Sun, Nov 08, 2009 at 11:46:10PM +0100, Denis 'GNUtoo' Carikli wrote:

Please do try to rememeber to CC maintainers on patches...

I've read some, but not all of this since I really can't follow how the
code is supposed to be constructed.  At the minute there doesn't feel
like there's enough abstraction between the various parts of the code,
with lots of things cropping up in unexpected places and making it
harder to follow.

It would probably be useful to refactor things so that you've got a
support layer for the DSP that deals with any resource sharing issues
and then build more standard-looking CODEC, DAI and DMA drivers (the DAI
driver would probably be largely empty, I guess) with a machine driver
gluing them together.

> it still has serious runtime problems such as:
> *Can produce kernel oops under theses conditions:
>  start alsamixer and if the second bar is on 0 or 4,
>  so it can play music with aplay,then increase the routing alsamixer bar
>  to the max.
>  Then decrease the routing bar to 4 or less

What are "the second bar" and "routing bar"?  Remember, most of the
people reading your mail will never have seen your system running.

> +config SND_MSM_DAI_SOC
> +	tristate "SoC CPU/CODEC DAI for the MSM chip"
> +	depends on SND_MSM_SOC || SND_QSD_SOC
> +	help
> +	 To add support for ALSA PCM driver for MSM board.

No need to make this one uer visible.

> +
> +config SND_MSM_SOC_MSM7K
> +        tristate "SoC Audio support for MSM7K"
> +        depends on SND_MSM_SOC

Or this.  I know some architectures do this but we should probably just
move them to visibly select only boards.

> +	codec->name = "MSM-CARD";

Probably something like "MSM7xxx internal CODEC" or similar.

> +static int __init msm_dai_init(void)
> +{
> +	return snd_soc_register_dais(msm_dais, ARRAY_SIZE(msm_dais));
> +}
> +
> +static void __exit msm_dai_exit(void)
> +{
> +	snd_soc_unregister_dais(msm_dais, ARRAY_SIZE(msm_dais));
> +}

Your DAIs ought to be registered as platform devices under arch/arm,
probe as normal platform devices and then register the DAIs when these
probe.

> +#define audio_send_queue_recbs(prtd, cmd, len) \
> +	msm_adsp_write(prtd->audrec, QDSP_uPAudRecBitStreamQueue, cmd, len)
> +#define audio_send_queue_rec(prtd, cmd, len) \
> +	msm_adsp_write(prtd->audrec, QDSP_uPAudRecCmdQueue, cmd, len)

Inline functions, please.

> +int msm_audio_volume_update(unsigned id,
> +				int volume, int pan)
> +{
> +	unsigned vol_raw;
> +
> +	vol_raw = compute_db_raw(volume);
> +	printk(KERN_INFO "volume: %8x vol_raw: %8x \n", volume, vol_raw);
> +	return audpp_set_volume_and_pan(id, vol_raw, pan);
> +}
> +EXPORT_SYMBOL(msm_audio_volume_update);

Hrm?

> +			unsigned id = msg[2];
> +			unsigned idx = msg[3] - 1;
> +			if (id != AUDPP_MSG_HOSTPCM_ID_ARM_RX) {
> +				printk(KERN_ERR "bogus id\n");
> +				break;

With stuff like this displaying what the bogus IDs are would be useful.

> +			/* Update with actual sent buffer size */
> +			if (prtd->out[idx].used != BUF_INVALID_LEN)
> +				prtd->pcm_irq_pos += prtd->out[idx].used;
> +
> +			if (prtd->pcm_irq_pos > prtd->pcm_size)
> +				prtd->pcm_irq_pos = prtd->pcm_count;

This looks suspicious - it looks like what's going on is that you're
getting a notification from the DSP that it has consumed a data buffer
but this might go out of bounds and get clamped.

> +			if (prtd->ops->playback)
> +				prtd->ops->playback(prtd);
> +
> +			spin_lock_irqsave(&the_locks.write_dsp_lock, flag);
> +			if (prtd->running) {
> +				prtd->out[idx].used = 0;
> +				frame = prtd->out + prtd->out_tail;
> +				if (frame->used) {
> +					audio_dsp_send_buffer(prtd,
> +							      prtd->out_tail,
> +							      frame->used);
> +					prtd->out_tail ^= 1;

I've got a feeling that this code would be a lot easier to follow if
you used a linked list of buffers.  You could statically allocate all the
buffers still, but it'd make it much more explicit what was going on if
you had a list of buffers queued for transmission to the DSP and a list
of free buffers.

Given what it looks like this is doing I'd really expect
snd_pcm_period_elapsed to be called directly from here - the playback
and capture code paths appear to be completely separate and the
indirections you're using appear not to be needed.  I'd also expect
it to be called after you've finished updating the buffers rather than
before, especially given that you appear to call it outside the lock on
buffer updates.

> +				} else {
> +					prtd->out_needed++;
> +				}
> +				wake_up(&the_locks.write_wait);

Why this?

> +	if (prtd->dir == SNDRV_PCM_STREAM_PLAYBACK) {
> +		prtd->out_buffer_size = PLAYBACK_DMASZ;
> +		prtd->out_sample_rate = 44100;
> +		prtd->out_channel_mode = AUDPP_CMD_PCM_INTF_STEREO_V;
> +		prtd->out_weight = 100;

You're not setting any 

> +int alsa_audio_configure(struct msm_audio *prtd)
> +{
> +	struct audmgr_config cfg;
> +	int rc;
> +
> +	if (prtd->enabled)
> +		return 0;
> +
> +	/* refuse to start if we're not ready with first buffer */
> +	if (!prtd->out[0].used)
> +		return -EIO;
> +
> +	cfg.tx_rate = 0;
> +	cfg.rx_rate = RPC_AUD_DEF_SAMPLE_RATE_48000;

Your code appears to hard code both 44.1k and 48k inconsistently.

> +		audmgr_disable(&prtd->audmgr);
> +		return -ENODEV;
> +	}
> +
> +	prtd->enabled = 1;
> +	return 0;
> +}
> +EXPORT_SYMBOL(alsa_audio_configure);

This looks extremely suspect...

> +ssize_t alsa_send_buffer(struct msm_audio *prtd, const char __user *buf,
> +			  size_t count, loff_t *pos)
> +{
> +	unsigned long flag;
> +	const char __user *start = buf;
> +	struct buffer *frame;
> +	size_t xfer;
> +	int rc = 0;
> +
> +	mutex_lock(&the_locks.write_lock);
> +	while (count > 0) {
> +		frame = prtd->out + prtd->out_head;
> +		rc = wait_event_interruptible(the_locks.write_wait,
> +					      (frame->used == 0)
> +					      || (prtd->stopped));
> +		if (rc < 0)
> +			break;
> +		if (prtd->stopped) {
> +			rc = -EBUSY;
> +			break;
> +		}
> +		xfer = count > frame->size ? frame->size : count;
> +		if (copy_from_user(frame->data, buf, xfer)) {
> +			rc = -EFAULT;
> +			break;
> +		}
> +		frame->used = xfer;
> +		prtd->out_head ^= 1;
> +		count -= xfer;
> +		buf += xfer;
> +
> +		spin_lock_irqsave(&the_locks.write_dsp_lock, flag);
> +		frame = prtd->out + prtd->out_tail;
> +		if (frame->used && prtd->out_needed) {
> +			audio_dsp_send_buffer(prtd, prtd->out_tail,
> +					      frame->used);
> +			prtd->out_tail ^= 1;

These ^s really don't help legibility.

> +			prtd->out_needed--;
> +		}
> +		spin_unlock_irqrestore(&the_locks.write_dsp_lock, flag);
> +	}
> +	mutex_unlock(&the_locks.write_lock);
> +	if (buf > start)
> +		return buf - start;
> +	return rc;
> +}
> +EXPORT_SYMBOL(alsa_send_buffer);

...as does this.  There should be no need to export all this stuff, and
if you are exporting it it ought to be _GPL since all the ASoC APIs are
_GPL.

I'd also really like to see some explanation of what the locking scheme
is supposed to be - I can't quite follow what's supposed to be protected
against what and when but I suspect there's cases where either read or
write lock is taken and the_locks.lock is really needed.

> +EXPORT_SYMBOL(alsa_audio_disable);

> +#include <../arch/arm/mach-msm/qdsp5/adsp.h>
> +#include <../arch/arm/mach-msm/qdsp5/audmgr.h>

Put these in an include directory.

> +#define FRAME_NUM               (8)
> +#define FRAME_SIZE              (2052 * 2)
> +#define MONO_DATA_SIZE          (2048)
> +#define STEREO_DATA_SIZE        (MONO_DATA_SIZE * 2)
> +#define CAPTURE_DMASZ           (FRAME_SIZE * FRAME_NUM)
> +
> +#define BUFSZ			(960 * 5)
> +#define PLAYBACK_DMASZ 		(BUFSZ * 2)

All this stuff needs namespacing.

> +/* Support unconventional sample rates 12000, 24000 as well */
> +#define USE_RATE                \
> +			(SNDRV_PCM_RATE_8000_48000 | SNDRV_PCM_RATE_KNOT)

Your code doesn't appear to actually cope with all these rates.  Also,
these ought to be pushed down into the drivers rather than exported in
the header file - there's no need for other drivers to see them.

> +static int snd_msm_volume_put(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	int change;
> +	int volume;
> +
> +	volume = ucontrol->value.integer.value[0];
> +	spin_lock_irq(&the_locks.mixer_lock);
> +	change = (msm_vol_ctl.volume != volume);
> +	if (change) {
> +		msm_vol_ctl.update = 1;
> +		msm_vol_ctl.volume = volume;
> +	}
> +	spin_unlock_irq(&the_locks.mixer_lock);

This doesn't actually appear to do anything to propagate the changes to
the DSP?

> +static struct snd_kcontrol_new snd_msm_controls[] = {
> +	MSM_EXT_TLV("PCM Playback Volume", 0, snd_msm_volume_info, \
> +	snd_msm_volume_get, snd_msm_volume_put, 0, db_scale_linear),
> +	MSM_EXT("device", 1, snd_msm_device_info, snd_msm_device_get, \
> +						 snd_msm_device_put, 0),
> +};

I have no idea what "device" is supposed to be.

> +static struct snd_soc_dai_link msm_dai = {
> +	.name = "ASOC",
> +	.stream_name = "ASOC",
> +	.codec_dai = &msm_dais[0],
> +	.cpu_dai = &msm_dais[1],
> +	.init	= msm_soc_dai_init,
> +};

Please pick some human readable names describing the setup (eg, "MSM7k
internal CODEC").