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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
