Please submit any code you have to the mailing list, it's much easier to review things via the standard workflows and getting the drivers into mainline would be good.

Here it is...I hope it's correct and in the proper format sorry for the delay.

Denis.

On Thu, 2009-11-05 at 15:23 +0000, Mark Brown wrote:

On Thu, Nov 05, 2009 at 04:17:24PM +0100, Denis 'GNUtoo' Carikli wrote:

...

...

Please submit any code you have to the mailing list, it's much easier to review things via the standard workflows and getting the drivers into mainline would be good.

...

Here it is...I hope it's correct and in the proper format sorry for the delay.

You appear to have omitted the actual patch...

sorry I had that: Died at /usr/libexec/git-core/git-send-email line 966.

I've bad luck with that mailing list...lol

Denis.

Hi!

I had to make two little change to make it compile: snd_ep = msm_rpc_connect_compatible(snd_rpc_ids.prog, became: snd_ep = msm_rpc_connect(snd_rpc_ids.prog, and I also changed snd_rpc_ids.vers(with ifdefs) to a known and working magick number:

I also had to change from ARCH_MSM_ARM11 to ARCH_MSM in the configuration

Thanks for pointer.

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 Then It may have a null pointer problem That bug could be because it tries to route to route to speakers and handset at the same time(SND_DEVICE_HEADSET_AND_SPEAKER in android): that is to say it could be the same bug than here: http://gitorious.org/replicant/msm7k/commit/370d37a088368ca8cc478e76c928a1ce... but I need time to verify that *can pannick(reboots the phone) if you send things to /dev/dsp when the oss emulation is activated *only aplay works(mplayer,gstreamer don't work) for mplayer an ioctl didn't return...so it get stuck before playing. The explanation of the bug can be found here: http://mailman.alsa-project.org/pipermail/alsa-devel/2009-November/022697.ht...

Perhaps it should go to staging?

Note the following things: *this patch depends on,and doesn't contain arch/arm/mach-msm/qdsp5 *I removed the support for more recents chips because of code-size issue in a mailing list

I had qdsp5 support in staging tree, check 2.6.31. It is not there _now_ but I hope to reinstall it. Pavel

On Fri, 2009-11-13 at 14:50 +0000, Mark Brown wrote:

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").

Thanks a lot!!! I'll try to fix most of the problems and resend a patch,I'll also continue the debugging...which will take some time....

About the debugging: *I noticed that if I change the .buffer_bytes_max to 4800 aplay doesn't work anymore...I'll try to find the good values... *I also noticed that aplay doesn't use the SNDRV_PCM_IOCTL_STATUS which would explain why aplay works and not mplayer... so I'll try to fix that too...

Denis.

Recently I looked again at the code. And I found this:

prtd->out[0].used = BUF_INVALID_LEN; prtd->out_head = 1; /* point to second buffer on startup */

in msm_pcm_open in msm7k-pcm.c

So here how it works: when the sound card is opened the fist buffer is assigned to BUF_INVALID_LEN and is skipped(prtd->out_head = 1).

Then at a point alsa_send_buffer (in msm-pcm.c) is called. But because mplayer uses a bigger buffer it goes this way: The first run goes fine: rc = wait_event_interruptible(the_locks.write_wait, (frame->used == 0) || (prtd->stopped)); is passed. Then in "xfer = count > frame->size ? frame->size : count;",the minimum between frame size and count is assigned to xfer. Then in copy_from_user(frame->data, buf, xfer) the audio frame is copied from userspace. Then xfer is assigned to frame->used Then "prtd->out_head ^= 1;" needs some little explanations: There are 2 buffers prtd->out[0] and prtd->out[1] And both out_head and out_tail can only be 0 or 1 So when prtd->out_head is 0,prtd->out_head ^= 1 changes to 1 into a 0 and vice versa.

Note that I recently changed things like frame = prtd->out + prtd->out_tail into that kind of things: frame = &prtd->out[prtd->out_tail] which makes it more Readable,and still works.

prtd->out is a struct buffer which is described in msm-pcm.h: struct msm_audio { struct buffer out[2]; ...

the struct buffer is also described in msm-pcm.h like this: struct buffer { void *data; unsigned size; unsigned used; unsigned addr; };

So...prtd->out_head was 1 because it was assigned to 1 in msm_pcm_open in msm7k-pcm.c Now it becomes 0

Then it does that: count -= xfer; which doesn't zero count it because mplayer used some big buffer size

Then it sends the data to the dsp...and doesn't exit the "while (count > 0) {" loop because count is not 0

And Finally it reaches that line a second time: "rc = wait_event_interruptible(the_locks.write_wait, (frame->used == 0) || (prtd->stopped));" But...frame->used was, and is still BUF_INVALID_LEN so it blocks.

By the way David Lanzendörfer fixed a lot of issues with kernel oops/panic with the OSS emulation...and merged the adsp and the alsa layer in one driver(that means that they are in a same directory and that the resource sharing issues is gone.)

I bet we are close to something working as mplayer can now work trough oss emulation or trough sdl(tough there are some buffer underrun problem sometimes with sdl and nearly all the time with oss) But we still have some work to do in order to clean the driver and fix the issues you described before re-submiting it.

Denis.

Appendix: ---------

here's the content of alsa_send_buffer: size_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; 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;

And here's the content of msm_pcm_open:

static int msm_pcm_open(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; struct msm_audio *prtd; int ret = 0;

prtd = kzalloc(sizeof(struct msm_audio), GFP_KERNEL); if (prtd == NULL) { ret = -ENOMEM; return ret; } if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { msm_vol_ctl.update = 1; /* Update Volume, with Cached value */ runtime->hw = msm_pcm_playback_hardware; prtd->dir = SNDRV_PCM_STREAM_PLAYBACK; prtd->playback_substream = substream; } else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { runtime->hw = msm_pcm_capture_hardware; prtd->dir = SNDRV_PCM_STREAM_CAPTURE; prtd->capture_substream = substream; }

ret = snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, &constraints_sample_rates); if (ret < 0) goto out;

/* Ensure that buffer size is a multiple of period size */ ret = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); if (ret < 0) goto out;

prtd->ops = &snd_msm_audio_ops; prtd->out[0].used = BUF_INVALID_LEN; prtd->out_head = 1; /* point to second buffer on startup */ runtime->private_data = prtd;

ret = alsa_adsp_configure(prtd); if (ret) goto out; copy_count = 0; return 0;

out: kfree(prtd); return ret; }

Hello, I have a sound card with I2C/PCM interface supported. I am in the process of evaluate the same using USB interface with HID interface class (ie bInterfaceClass ->3 Human Interface Device). Could you please suggest, if the usb interface HID class could be used for sending Audio and PCM signals?

Thanks Swami

M R Swami Reddy wrote:

Could you please suggest, if the usb interface HID class could be used for sending Audio and PCM signals?

No; audio data should be transferred using isochronous transfers, and the HID class does not use those.

Is there any reason why you cannot use a standard audio class interface?

Best regards, Clemens