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_SOCOr 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.